feat(byok): pass diskEncryptionSet ID to CS if AFEC is registered - #6662
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables BYOK for nodePool OS disk encryption by conditionally passing the OS disk diskEncryptionSet resource ID through the OCM-to-Cluster-Service conversion path when the subscription has the ExperimentalReleaseFeatures (AFEC) feature registered at node pool creation time. It does this by persisting an internal “experimental features enabled” flag on the node pool and using it to gate emitting the experimental CS field.
Changes:
- Add
ExperimentalFeaturesEnabledto node pool ServiceProviderProperties and set it during admission on CREATE based on AFEC registration. - Gate setting
SseEncryptionSetResourceIdin Cluster Service node pool OS disk builder behindExperimentalFeaturesEnabled. - Add unit test coverage for the conversion gating and admission mutation, and update fuzz round-trip setup to include the new field.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ocm/convert.go | Adds gated OS disk conversion that sets SseEncryptionSetResourceId only when AFEC was registered at node pool creation time. |
| internal/ocm/convert_test.go | Adds conversion test cases covering “AFEC enabled → DES passed” and “AFEC disabled → DES not passed”. |
| internal/apitesting/coreapitesting/fuzz.go | Zeros the new ServiceProviderProperties flag in round-trip fuzz funcs to keep internal-only fields deterministic. |
| internal/api/coreapi/types_nodepool.go | Introduces ExperimentalFeaturesEnabled on node pool ServiceProviderProperties with writer annotation and rationale comment. |
| internal/admission/admit_nodepool.go / internal/admission/admit_nodepool_test.go | Sets the flag on CREATE based on subscription AFEC registration and adds unit tests for the mutation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| SizeGibibytes(int(*osDisk.SizeGiB)). | ||
| StorageAccountType(storageAccountType). | ||
| Persistence(persistence) | ||
| if experimentalFeaturesEnabled && osDisk.EncryptionSetID != nil { |
There was a problem hiding this comment.
n.b. the field is ignored today, which is why it's correct to do this
Steve Kuznetsov (stevekuznetsov)
left a comment
Steve Kuznetsov (stevekuznetsov)
left a comment
There was a problem hiding this comment.
/lgtm
/approve
2d0010c to
87c9d3c
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cadenmarchese, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
/hold Revision 87c9d3c was retested 3 times: holding |
|
/retest-required |
This is required for BYOK on nodePool OS Disks. CS will validate the DES, and then CAPZ will create the nodePool with that DES' key
Since the change needs to be released to stage (at minumum) to be fully tested, release behind a feature flag such that we can verify full functionality of the e2e test before merging it
87c9d3c to
5b6c113
Compare
|
New changes are detected. LGTM label has been removed. |
| return arohcpv1alpha1.NewAzureNodePoolEncryptionAtHost().State(state) | ||
| } | ||
|
|
||
| func buildCSOsDisk(osDisk coreapi.OSDiskProfile, storageAccountType, persistence string, experimentalFeaturesEnabled bool) *arohcpv1alpha1.AzureNodePoolOsDiskBuilder { |
| // ExperimentalFeaturesEnabled records whether the FeatureExperimentalReleaseFeatures AFEC was | ||
| // registered on the subscription at node pool creation time. Used by internal/ocm/convert.go | ||
| // to gate experimental CS fields (e.g. SseEncryptionSetResourceId on the OS disk) that are | ||
| // only sent to Cluster Service when the AFEC is registered. | ||
| // Written by: Frontend PUT NodePool (Create) | ||
| ExperimentalFeaturesEnabled bool `json:"experimentalFeaturesEnabled,omitempty"` |
|
/retest-required |
|
Caden Marchese (@cadenmarchese): The following test 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. |
https://redhat.atlassian.net/browse/ARO-27738
What
/test stage-e2e-parallel prod-e2e-parallelin that PR to confirm that e2e works in prod and stage environments (we already know it works in lower environments). Merging this feature this way is required because workload identities in dev, CI, and INT are mocked with a custom role, such that the role assignment scopes in the e2e test are not fully testable (workload identities are over-privileged). This is the cleanest way to ensure that e2e functions everywhere before release.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