feat: enable customer-managed key for nodePool OS disk encryption#5682
feat: enable customer-managed key for nodePool OS disk encryption#5682cadenmarchese wants to merge 4 commits into
Conversation
allows BYOK for nodePool OS disk encryption, unblocks mHSM support requires service managed identity to have Reader role over the disk encryption set
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. |
| func buildCSOsDisk(osDisk api.OSDiskProfile, storageAccountType, persistence string) *arohcpv1alpha1.AzureNodePoolOsDiskBuilder { | ||
| builder := arohcpv1alpha1.NewAzureNodePoolOsDisk(). | ||
| SizeGibibytes(int(*osDisk.SizeGiB)). | ||
| StorageAccountType(storageAccountType). | ||
| Persistence(persistence) | ||
| if osDisk.EncryptionSetID != nil { | ||
| builder.SseEncryptionSetResourceId(osDisk.EncryptionSetID.String()) | ||
| } | ||
| return builder | ||
| } |
There was a problem hiding this comment.
This is carry-over from current behavior. I am not familiar enough with the HCP API to know if this truly needs a nil check or not, but I guess it can't hurt. Deferring to others for input.
|
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? |
|
[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 |
| By("creating a KeyVault and DES for OS disk encryption") | ||
| subscriptionID, err := tc.SubscriptionID(ctx) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to get subscription ID") | ||
|
|
||
| desResourceID := fmt.Sprintf( | ||
| "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/diskEncryptionSets/%s-des", | ||
| subscriptionID, *resourceGroup.Name, customerClusterName, | ||
| ) |
There was a problem hiding this comment.
I'm going to let e2e run first to identify any other potential issues before proceeding with fixing this.
| // AutoScaling enables nodepool autoscaling. When set, Replicas is ignored. | ||
| AutoScaling *NodePoolAutoScalingParams | ||
| AvailabilityZone string | ||
| EncryptionSetID string | ||
| } |
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.
This requires service managed identity (or the mock) to have Reader role over the disk encryption set.
Why
This is required to get BYOK working, both for regular keyvaults, and mHSM keyvaults.
Testing
Special notes for your reviewer
PR Checklist