feat: enable customer-managed key for nodePool OS disk encryption - #5682
feat: enable customer-managed key for nodePool OS disk encryption#5682Caden Marchese (cadenmarchese) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds support for propagating an OS disk Disk Encryption Set (DES) resource ID from the RP node pool model into the Cluster Service (CS) node pool representation.
Changes:
- Added a dedicated helper to build the CS OS disk builder, including optional SSE encryption-set wiring.
- Updated
BuildCSNodePoolto use the new OS disk builder helper. - Added test coverage to verify DES ID propagation and the nil case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/ocm/convert.go | Introduces buildCSOsDisk and uses it in BuildCSNodePool to set SseEncryptionSetResourceId when present. |
| internal/ocm/convert_test.go | Adds test cases validating the DES resource ID mapping and ensuring the field is omitted when EncryptionSetID is nil. |
|
Can you add an e2e test that creates a nodepool with os disk encryption set, and uses an azure client to confirm they're actually set when the node pools are created? |
|
What are the code changes themselves look good to me. About the e2e, without being familiar with the functionality on Azure side, the test conceptually seems correct. I will let others more familiar with it give their view. I left some minor comments. |
|
/retest |
e930d35 to
ff2fba2
Compare
|
The CI custom role was updated to accommodate this e2e test. Context: |
ba4f0a3 to
f3055a7
Compare
|
/retest-required |
|
Rael Garcia (@raelga) the change you mentioned was intentional but was due to a misunderstanding. I removed the sweeper because I assumed that we used the same keyvault across all e2e tests (meaning that the sweeper wouldn't function anymore as part of my purge protection changes). Now that I see this was not correct and we use 1 keyvault per test case, I have reverted this so that it can continue sweeping keyvaults that are not part of my test. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/e2e/nodepool_osdisk_encryption.go:71
- This test creates a Key Vault key via the Key Vault data-plane client (azkeys.CreateKey) when building the Disk Encryption Set. With Key Vault RBAC enabled, the test runner typically needs an explicit Key Vault Crypto Officer assignment (as done in kms_key_rotation.go) or the CreateKey call can fail with 403.
Add the "assignKeyVaultCryptoOfficer" parameter to the customer-infra deployment inputs so the test runner can create/rotate keys.
map[string]interface{}{
"enableKeyVaultSoftDelete": true,
"enableKeyVaultPurgeProtection": true,
"keyVaultSoftDeleteRetentionInDays": 7,
},
test/e2e-setup/bicep/modules/customer-infra.bicep:113
- The module description says purge protection "requires soft delete", but the template currently allows
enableKeyVaultPurgeProtection=truewhileenableKeyVaultSoftDelete=false. Azure will reject that combination at deploy time.
Guard enablePurgeProtection behind enableKeyVaultSoftDelete (or add an assert) so invalid parameter combinations can’t be passed.
enableRbacAuthorization: true
enableSoftDelete: enableKeyVaultSoftDelete
enablePurgeProtection: enableKeyVaultPurgeProtection
softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null
test/util/framework/per_test_framework.go:687
- Key Vault purge-on-teardown was removed here, but
NewResourceGroupstill documents a "best-effort teardown purge" as a second layer of defense against Key Vault soft-delete name collisions (per_test_framework.go:432-445). This makes the file’s documentation inconsistent with behavior, and it’s unclear whether dropping the purge is intentional for all E2E runs.
Either reintroduce the best-effort purge (if still desired) or update the NewResourceGroup comment to remove references to a purge step and clarify the collision-mitigation strategy.
ginkgo.GinkgoLogr.Info("deleting resource group", "resourceGroup", resourceGroupName)
if err := DeleteResourceGroup(ctx, resourceClientFactory.NewResourceGroupsClient(), networkClientFactory, resourceGroupName, false, timeout); err != nil {
return fmt.Errorf("failed to cleanup resource group: %w", err)
}
// we want non-conformant clusters to be visible at the end, without impeding our ability to clean up the resource group
return nonConformantErr
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
test/e2e-setup/bicep/modules/customer-infra.bicep:113
enableKeyVaultPurgeProtectionis documented as requiring soft delete, but the resource setsenablePurgeProtectionbased only onenableKeyVaultPurgeProtection. If a caller enables purge protection while leaving soft delete disabled, the deployment may be rejected or behave unexpectedly. Gate purge protection onenableKeyVaultSoftDeleteto enforce the documented invariant.
properties: {
enableRbacAuthorization: true
enableSoftDelete: enableKeyVaultSoftDelete ? true : null
enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null
softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/util/verifiers/osdisk_encryption.go:45
VirtualMachinesClient.NewListPageronly listsMicrosoft.Compute/virtualMachines. HCP worker nodes are VMSS instances (.../virtualMachineScaleSets/<vmss>/virtualMachines/<id>), so this verifier will likely find zero VMs and fail even when the nodepool exists. Consider switching the discovery to VMSS resources (list VMSS in the managed RG, filter by nodepool name/tag, then list VMSS VMs and read their OS disks).
vmClient := v.computeFactory.NewVirtualMachinesClient()
disksClient := v.computeFactory.NewDisksClient()
var vms []*armcompute.VirtualMachine
pager := vmClient.NewListPager(v.managedResourceGroup, nil)
for pager.More() {
page, err := pager.NextPage(ctx)
test/e2e-setup/bicep/modules/customer-infra.bicep:113
enablePurgeProtectionis documented as requiring soft delete, but the template allowsenableKeyVaultPurgeProtection=truewhileenableKeyVaultSoftDelete=false, which can cause a deployment failure. Gate purge protection on soft delete (or add a parameter assertion).
enableRbacAuthorization: true
enableSoftDelete: enableKeyVaultSoftDelete ? true : null
enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null
softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/e2e-setup/bicep/modules/customer-infra.bicep:113
enablePurgeProtectioncan be set to true whileenableSoftDeleteis omitted (null) whenenableKeyVaultPurgeProtection=truebutenableKeyVaultSoftDelete=false. Key Vault purge protection requires soft delete; this combination can cause an ARM validation failure or unexpected behavior. Consider deriving both properties from a single "soft delete enabled" condition so purge protection always implies soft delete (and sets retention days).
enableSoftDelete: enableKeyVaultSoftDelete ? true : null
enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null
softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null
|
/retest-required |
b2de3db to
0574156
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/e2e-setup/bicep/modules/customer-infra.bicep:112
enableKeyVaultPurgeProtectionis documented as requiring soft delete, but the template can currently setenablePurgeProtectioneven whenenableKeyVaultSoftDeleteis false (since the properties are gated independently). That combination is invalid for Key Vault and would make deployments fail if a caller accidentally enables purge protection without soft delete.
enableSoftDelete: enableKeyVaultSoftDelete ? true : null
enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null
Martin Bukatovic (mbukatov)
left a comment
Martin Bukatovic (mbukatov)
left a comment
There was a problem hiding this comment.
Thank you for updating the E2E test code, now the test code is aligned with the existing code.
I have 2 comments though:
- one minor note about improvements in error reporting in the new verifier
- I still see the removal of key vault cleanup in the PR, I assumed we want to preserve it
| } | ||
| } | ||
| if len(errs) > 0 { | ||
| return fmt.Errorf("OS disk encryption verification failed for %d/%d VMs: %v", len(errs), len(workerVMs), errs) |
There was a problem hiding this comment.
Instead of passing list of errors errs via %v, join the errors together as errors.Join(errs...) and pass it as %w, that would preserve more context about the errors.
There was a problem hiding this comment.
Done, let me know if this works.
allows BYOK for nodePool OS disk encryption, unblocks mHSM support requires service managed identity to have Reader role over the disk encryption set
use Go SDK rather than bicep templates for des e2e test
0574156 to
3412e08
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/e2e-setup/bicep/modules/customer-infra.bicep:113
enableKeyVaultPurgeProtectionis documented as requiring soft delete, but the current expressions allowenableKeyVaultPurgeProtection=truewhile leavingenableSoftDeleteandsoftDeleteRetentionInDaysunset. Depending on Key Vault API behavior, that combination can fail deployment or produce an unexpected default retention configuration. Consider making soft delete implicit whenever purge protection is enabled (or add an explicit validation/assert) so the parameters can’t be set to an invalid combination.
enableSoftDelete: enableKeyVaultSoftDelete ? true : null
enablePurgeProtection: enableKeyVaultPurgeProtection ? true : null
softDeleteRetentionInDays: enableKeyVaultSoftDelete ? keyVaultSoftDeleteRetentionInDays : null
https://redhat.atlassian.net/browse/ARO-27738
What
allows BYOK for nodePool OS disk encryption. Currently, since we don't pass the diskEncryptionSet ID to cluster service, all nodePool OS disks fall back to platform-managed keys.
This also unblocks mHSM support for nodePool OS Disk encryption (but not etcd, that requires HyperShift changes)
This requires service managed identity (or the mock) to have Reader role over the disk encryption set, otherwise e2e fails with the following:
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.
Testing
Special notes for your reviewer
PR Checklist