feat(byok): enable customer-managed key for nodePool OS disk encryption - #6647
Conversation
feat: add e2e test for BYOK OS Disks use Go SDK rather than bicep templates for des e2e test revert cleanup removals, switch to errors.join approach fix(e2e): skip purge protected keyvaults on test cleanup also, delete role assignments before cleaning up the resource group. This is required because the BYOK OS Disk e2e test requires a non-purgeable keyvault already configured to automatically delete in 7 days assign reader role to both CAPZ and SMI with scope of DES
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cadenmarchese The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Enables customer-managed keys (BYOK) for NodePool OS disk encryption by wiring the disk encryption set (DES) resource ID through the RP→Cluster Service conversion path, and adds an E2E test that provisions a Key Vault–backed DES and verifies worker OS disks are encrypted with it.
Changes:
- Pass NodePool OS disk
encryptionSetIdthroughinternal/ocmconversion into the Cluster Service node pool model. - Add E2E coverage that creates a Key Vault + DES, assigns required Reader/KV roles, creates an encrypted nodepool, and verifies disk encryption via ARM.
- Update test framework cleanup to handle DES-related role assignments and to skip purging purge-protected soft-deleted Key Vaults.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/util/verifiers/osdisk_encryption.go | New verifier that checks VM OS disks use customer-managed encryption and match the expected DES ID. |
| test/util/framework/per_test_framework.go | Cleanup enhancements: delete tracked role assignments before RG deletion; skip purging purge-protected deleted Key Vaults. |
| test/util/framework/per_test_framework_test.go | Unit tests for the new keyVaultPurgeProtected helper. |
| test/util/framework/helpers_v20260901preview.go | Adds EncryptionSetID param and sets nodepool OSDisk.EncryptionSetID when provided. |
| test/util/framework/helpers_v20260630preview.go | Same as above for the 20260630 preview helpers. |
| test/util/framework/helpers_v20251223preview.go | Same as above for the 20251223 preview helpers. |
| test/util/framework/helpers_v20240610preview.go | Same as above for the 20240610 preview helpers. |
| test/util/framework/disk_encryption_set_helper.go | New helper to create a Key Vault–backed DES and assign KV crypto + DES Reader roles to required principals. |
| test/e2e/nodepool_osdisk_encryption.go | New E2E test validating nodepool OS disk encryption via DES (BYOK). |
| test/e2e-setup/bicep/modules/customer-infra.bicep | Adds Key Vault soft delete + purge protection configuration parameters for BYOK scenarios. |
| internal/ocm/convert.go | Sets Cluster Service SseEncryptionSetResourceId from RP nodepool OSDisk.EncryptionSetID. |
| internal/ocm/convert_test.go | Adds unit coverage for passing DES ID (and not setting it when nil). |
| test/testdata/zz_fixture_TestMainListSuitesForEachSuite_stage_parallelstage_parallel.txt | Registers the new E2E in the stage parallel suite fixture. |
| test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallelrp_api_compat_all_parallel.txt | Registers the new E2E in rp_api_compat_all_parallel suite fixture. |
| test/testdata/zz_fixture_TestMainListSuitesForEachSuite_rp_api_compat_all_parallel_01rp_api_compat_all_parallel_development.txt | Registers the new E2E in rp_api_compat_all_parallel development suite fixture. |
| test/testdata/zz_fixture_TestMainListSuitesForEachSuite_prod_parallelprod_parallel.txt | Registers the new E2E in the prod parallel suite fixture. |
| test/testdata/zz_fixture_TestMainListSuitesForEachSuite_integration_parallelintegration_parallel.txt | Registers the new E2E in the integration parallel suite fixture. |
| test/testdata/zz_fixture_TestMainListSuitesForEachSuite_dev_cd_check_paralleldev_cd_check_parallel.txt | Registers the new E2E in the dev CD check parallel suite fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| enableSoftDelete: enableKeyVaultSoftDelete ? true : null | ||
| enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null | ||
| softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null |
|
/test integration-e2e-parallel |
| labels.AroRpApiCompatible, | ||
| labels.MIContainers(1), |
There was a problem hiding this comment.
| labels.AroRpApiCompatible, | |
| labels.MIContainers(1), | |
| labels.AroRpApiCompatible, | |
| labels.IntegrationOnly, | |
| labels.MIContainers(1), |
This should still fail in Staging and Production, right?
There was a problem hiding this comment.
This should pass in all envs, including stage and prod, but I'm not able to confirm in INT because cluster identities in INT use the aro-hcp-int-msi-mock service principal, which grants everything diskEncryptionSets/read over the entire subscription (see role assignments, and the role definition, which gets permission over the whole sub)
In stage/prod, each identity gets its built-in role with minimally scoped permissions. Is there a way to run this e2e test in prod before merge to validate the fix?
There was a problem hiding this comment.
mvacula02 I don't believe /test stage-e2e-parallel prod-e2e-parallel will work, since the e2e test here depends on the ocm/internal changes being deployed there. Steve suggested these steps:
- add code to the RP to handle the new feature, behind an AFEC flag, such that it can be deployed to all envs without impacting production
- deploy RP
- write a test to use it, since the test subs (and only the test subs) have the AFEC
- once validated, remove requirement on afec and allow users
We may already have an existing AFEC flag we can use to deploy the RP changes safely, and then repurpose this PR to just be for the test later. I'll look into it, if that all sounds OK.
|
/test stage-e2e-parallel prod-e2e-parallel |
|
/retest-required |
|
Caden Marchese (@cadenmarchese): The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Per conversations about the e2e test, and since it previously caused issues during release, I've been recommended a phased approach here:
This is required since workload identities are mocked in dev, CI and INT, making the e2e test (which scopes disk encryption set role assignments to specific identities) not useful in dev, CI and INT. |
https://redhat.atlassian.net/browse/ARO-27738
What
Why
This is required to get BYOK for nodePool OS Disk encryption working, both for regular keyvaults, and mHSM keyvaults. Today the HCP API will accept a disk encryption set but won't use it for the actual encryption, which is unexpected behavior.
Testing
Special notes for your reviewer
PR Checklist