OCPSTRAT-1679: enhancement: StorageClass KMS Key for AWS Hosted Control Planes#2039
OCPSTRAT-1679: enhancement: StorageClass KMS Key for AWS Hosted Control Planes#2039devguyio wants to merge 1 commit into
Conversation
devguyio
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@devguyio: This pull request references OCPSTRAT-1679 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target either version "5.0." or "openshift-5.0.", but it targets "openshift-5.1" instead. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
|
||
| ## Motivation | ||
|
|
||
| ROSA classic clusters allow customers to configure KMS encryption for volumes created |
There was a problem hiding this comment.
can we elaborate how rosa classic leverage this semantic, and how rosa hcp is planning to do the same, e.g. is there any efforts we can link to expose this in ocm?
There was a problem hiding this comment.
Addressed in the latest push. Added a paragraph in the Motivation section explaining the ROSA classic precedent (rosa create cluster --kms-key-arn). ROSA CLI, Terraform, and CAPI integration is tracked downstream by the ROSA product team. OCM CLI/UI are not in scope even on the ROSA side. Updated Non-Goals to clarify this.
| cluster (day-2 operation) so that new PVCs use an updated or rotated KMS key without | ||
| requiring cluster recreation. | ||
|
|
||
| - As a self-managed HyperShift operator on AWS, I want to specify a KMS key for the |
There was a problem hiding this comment.
"hypershift" is a dev cli, "hcp" is product one. Let's remove any ambiguity so we get agents straight
There was a problem hiding this comment.
Fixed. The user story now says 'developer CLI (hypershift)' to disambiguate from the product CLI (hcp). Both CLIs are covered via the shared options mechanism.
|
|
||
| ### Goals | ||
|
|
||
| - Add `storageKMSKeyARN` to `AWSPlatformSpec` (shared by `HostedCluster` and |
There was a problem hiding this comment.
I think storageKMSKeyARN should belong to parent api struct that can potentially grow cohesively
There was a problem hiding this comment.
Agreed. The field has been moved to spec.operatorConfiguration.aws.csiDriverConfig.kmsKeyARN. This introduces new types AWSOperatorConfiguration and AWSCSIDriverConfig under OperatorConfiguration. Platform-specific operator configs go under operatorConfiguration., while platform-agnostic ones (CVO, CNO, Ingress) stay at the top level. See the updated API Extensions section.
| approvers: | ||
| - "@csrwng" | ||
| - "@enxebre" | ||
| api-approvers: |
There was a problem hiding this comment.
please let's include here @JoelSpeed @everettraven so we diversify knowledge
There was a problem hiding this comment.
Done - added @JoelSpeed and @everettraven to the reviewers list in the frontmatter.
There was a problem hiding this comment.
I think @enxebre meant to put Joel/Bryce as api-approvers, but a minor point
| title: storageclass-kms-key | ||
| authors: | ||
| - "@devguyio" | ||
| reviewers: |
There was a problem hiding this comment.
please let's include here managed services and storage representatives so we diversify knowledge
cc @joshbranham, @jsafrane
There was a problem hiding this comment.
Done - added @jsafrane and @joshbranham to the reviewers list.
| reads this value and configures the default StorageClass with the KMS key ID. | ||
| 5. New EBS volumes provisioned via the StorageClass carry the KMS encryption. | ||
|
|
||
| When the field is cleared, the HCCO clears `DriverConfig.AWS.KMSKeyARN`, the CSO reverts |
There was a problem hiding this comment.
There needs to be a way to opt-out from reconciliation so in cluster changes UX is preserved, is this how you achieve that?
There was a problem hiding this comment.
Done. HCCO now uses write-once (same as ReconcileDefaultIngressController). Writes DriverConfig only on initial creation, in-cluster edits are preserved.
(posted by Claude Code after review from devguyio)
|
|
||
| 1. The HC controller mirrors the field to `HostedControlPlane.spec.platform.aws.storageKMSKeyARN`, | ||
| following the existing mirroring pattern for other `AWSPlatformSpec` fields. | ||
| 2. The HCCO validates the key by assuming the `StorageARN` role (already provisioned for |
There was a problem hiding this comment.
is 2 something that the storage does today? If not, should that be an epic for storage operator?
There was a problem hiding this comment.
Done. Validation moved from the HCCO to the aws-ebs-csi-driver-operator in openshift/csi-operator. The operator already has a withKMSKeyHook in pkg/driver/aws-ebs/aws_ebs.go that reads kmsKeyARN from ClusterCSIDriver and injects it into the StorageClass. We extend that hook to call kms:Encrypt before injecting. If it fails, WithSyncDegradedOnError sets StorageClassControllerDegraded. Standalone clusters get the same validation. Scoped within this feature, not a separate epic.
(posted by Claude Code after review from devguyio)
| the HC controller bubbles it to `HostedCluster` status. | ||
| 4. The HCCO storage reconciliation writes `ClusterCSIDriver.spec.driverConfig.aws.kmsKeyARN` | ||
| to the guest cluster via its guest cluster client. The cluster-storage-operator (CSO) | ||
| reads this value and configures the default StorageClass with the KMS key ID. |
There was a problem hiding this comment.
does introduces a race for stateful workloads using the default class? can we outline the risks of that?
There was a problem hiding this comment.
No longer a concern. Day-2 rotation is now done directly on ClusterCSIDriver in the guest cluster. HC field is day-1 only.
(posted by Claude Code after review from devguyio)
| - **Per-StorageClass KMS key granularity.** This enhancement targets only the default | ||
| StorageClass managed by the cluster-storage-operator. The CSI operator's | ||
| `ClusterCSIDriver.DriverConfig` applies to the driver globally; per-StorageClass |
There was a problem hiding this comment.
Users can still create their own StorageClasses with their own keys, at least on standalone clusters. The enhancement affects only the default StorageClasses.
There was a problem hiding this comment.
Done, reflected in Non-Goals.
(posted by Claude Code after review from devguyio)
|
|
||
| ## Proposal | ||
|
|
||
| When a customer sets `spec.platform.aws.storageKMSKeyARN` on a `HostedCluster`, the |
There was a problem hiding this comment.
So far the pattern for operator configs is to place them under hc.spec.operatorConfiguration. Would this not be better under something like: hc.spec.operatorConfiguration.csiDrivers.ebs = {blah} ?
ebs may not be the only csi driver for the aws platform (afaik for aws there is at least efs and s3 as well)
There was a problem hiding this comment.
Echoing Cesar's (and Alberto's points above), this should probably have additional nesting instead of just a subfield in spec.platform.aws.
On the point of
nesting adds indirection
with no benefit. A nested struct can be introduced in a follow-on enhancement if
additional storage fields are needed.
below, this is not really true. APIs are hard to modify after creation especially in OCP, as we retain support for a major release's lifecycle, so we should consider future API additions in the design.
It's probably overkill but the direct equivalent would probably be something like hc.spec.operatorConfiguration.csiDriverConfig.aws.kmsKeyARN
There was a problem hiding this comment.
Done. Field moved to spec.operatorConfiguration.csiDriverConfig.aws.kmsKeyARN, which is essentially what @yuqi-zhang suggested. Introduces CSIDriverOperatorConfig and AWSCSIDriverConfig types under OperatorConfiguration, so it extends naturally to other drivers and platforms.
(posted by Claude Code after review from devguyio)
|
|
||
| ## Summary | ||
|
|
||
| This enhancement adds an optional `storageKMSKeyARN` field to the AWS platform spec |
There was a problem hiding this comment.
will existing iam policies support encryption?
There was a problem hiding this comment.
Self-managed yes, inline policies in cmd/infra/aws/iam.go already have the KMS actions. ROSA HCP no, ROSAAmazonEBSCSIDriverOperatorPolicy has zero KMS actions. Managed policy change request being submitted to AWS. Documented under Implementation Details > IAM Permissions.
(posted by Claude Code after review from devguyio)
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Adding some comments from the openshift/api perspective
|
|
||
| ## Proposal | ||
|
|
||
| When a customer sets `spec.platform.aws.storageKMSKeyARN` on a `HostedCluster`, the |
There was a problem hiding this comment.
Echoing Cesar's (and Alberto's points above), this should probably have additional nesting instead of just a subfield in spec.platform.aws.
On the point of
nesting adds indirection
with no benefit. A nested struct can be introduced in a follow-on enhancement if
additional storage fields are needed.
below, this is not really true. APIs are hard to modify after creation especially in OCP, as we retain support for a major release's lifecycle, so we should consider future API additions in the design.
It's probably overkill but the direct equivalent would probably be something like hc.spec.operatorConfiguration.csiDriverConfig.aws.kmsKeyARN
| StorageKMSKeyARN string `json:"storageKMSKeyARN,omitempty"` | ||
| ``` | ||
|
|
||
| The field is validated with a CEL `XValidation` rule accepting both KMS key ARNs and |
There was a problem hiding this comment.
Let's add these following points directly to the validation above, i.e..
// +optional
// +kubebuilder:validation:MaxLength=2048
// +kubebuilder:validation:XValidation:rule="...",message="..."
StorageKMSKeyARN string `json:"storageKMSKeyARN,omitempty"`
There was a problem hiding this comment.
Done.
(posted by Claude Code after review from devguyio)
| alias ARNs: | ||
|
|
||
| ```text | ||
| ^arn:(aws|aws-cn|aws-us-gov):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/[a-zA-Z0-9:/_-]+$ |
There was a problem hiding this comment.
I assume this is the hypershift equivalent of https://github.com/openshift/api/blob/master/operator/v1/types_csi_cluster_driver.go#L176, perhaps we can see if matching the validation and godocs there makes sense
There was a problem hiding this comment.
Done. CEL regex now aligns with the downstream ClusterCSIDriver field including all AWS partitions.
(posted by Claude Code after review from devguyio)
|
|
||
| **Day-2 (key removal):** | ||
|
|
||
| 1. The administrator sets `storageKMSKeyARN` to empty string. |
There was a problem hiding this comment.
Technically conflicts with the field being optional and omitempty. The right user behaviour is deleting the field. Empty string should not be allowed since there's no difference between that and an unset field.
There was a problem hiding this comment.
Done. Day-2 changes are now made directly on ClusterCSIDriver in-cluster. No empty-string workflow.
(posted by Claude Code after review from devguyio)
| // Updating this field does not re-encrypt existing volumes; only newly | ||
| // created PVCs use the new key. | ||
| // +optional | ||
| StorageKMSKeyARN string `json:"storageKMSKeyARN,omitempty"` |
There was a problem hiding this comment.
Not sure how far we got with featuregating in hypershift, but this might be a good candidate for one.
There was a problem hiding this comment.
Ships directly to GA. Single optional passthrough field with write-once semantics. Open to discussion if there's a specific concern.
(posted by Claude Code after review from devguyio)
| 2. The HC controller updates the `HostedControlPlane` field; the HCCO re-validates and | ||
| updates `ClusterCSIDriver`. | ||
| 3. The CSO updates the default StorageClass. There is a brief period between CSO | ||
| reconciliation cycles where the StorageClass references neither the old nor the new |
There was a problem hiding this comment.
A bit confused on this point, how would it be neither? I thought it would just be using the old key until reconciliation, and there's no "gap" period.
There was a problem hiding this comment.
You were right, the wording was wrong. Moot now: day-2 rotation is in-cluster on ClusterCSIDriver, single-hop.
(posted by Claude Code after review from devguyio)
| include the `alias/` ARN resource type and the broader alias name character set | ||
| (`:`, `/`, `_` are valid in alias names). | ||
|
|
||
| #### New condition type: `ValidAWSStorageKMSConfig` |
There was a problem hiding this comment.
I'm not familiar with general hypershift status/conditions, but just thinking from a user perspective, let's say I updated my key, I don't think there's a easy way to see if/when that gets applied just by looking at the conditions, since it doesn't actually tell me what the last success was.
Would there be an equivalent status field we could populate for the last successful key reconciliation?
There was a problem hiding this comment.
Dedicated condition is gone. KMS errors surface through StorageClassControllerDegraded -> ClusterOperator "storage" -> ClusterVersionSucceeding. Since day-2 changes are in-cluster, oc get co storage gives immediate feedback.
(posted by Claude Code after review from devguyio)
|
apologies for some comment spam that were posted by mistake by the agent. |
b185d86 to
76358ae
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
76358ae to
6f3eb83
Compare
|
Pushed a major revision addressing all review feedback. API locationField moved from Day-1-only reconciliationHCCO now writes Validation moved to the driver operatorKMS key validation is now in the IAM permissionsDocumented under Implementation Details. Self-managed works out of the box. ROSA HCP depends on an AWS managed policy update (in progress). OtherCEL regex aligned with openshift/api, full validation markers inline, reviewers list expanded, graduation goes straight to GA. All inline comments have been replied to individually. (posted by Claude Code after review from devguyio) |
|
@devguyio: 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. |
| approvers: | ||
| - "@csrwng" | ||
| - "@enxebre" | ||
| api-approvers: |
There was a problem hiding this comment.
I think @enxebre meant to put Joel/Bryce as api-approvers, but a minor point
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate="",rule="matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$')",message="kmsKeyARN must be a valid AWS KMS key ARN in the format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>" |
There was a problem hiding this comment.
I think FeatureGateAwareXValidation:featureGate="" means just always on? I don't think that's needed here since https://github.com/openshift/api/blob/72066cc9718bcab23b025ead4925db0b823adaf0/operator/v1/types_csi_cluster_driver.go#L182-L183 has per-gate validation (actually, looking at that, do you care about AWSEuropeanSovereignCloudInstall at all?)
Also I think we would prefer featuregating, but I defer to @enxebre and @csrwng if you want to go with #2039 (comment) direct to GA
| // permissions on the specified key. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MaxLength=2048 |
There was a problem hiding this comment.
a note to explicitly document the length requirements and what happens when this field is omitted (since it's optional, omitempty), as required by o/api guidelines
|
|
||
| ## Operational Aspects of API Extensions | ||
|
|
||
| #### SLIs The condition transition from `Unknown` to |
There was a problem hiding this comment.
Some line break issues here
8 participants
Add enhancement proposal for customer-controlled default encryption of
guest-cluster EBS volumes using a specified KMS key in HyperShift AWS
hosted control planes.
This closes a parity gap between ROSA classic and ROSA HCP for storage
encryption by adding
storageKMSKeyARNtoAWSPlatformSpec, propagatingit through the HC controller → HCCO →
ClusterCSIDriverchain, andexposing a
--storage-volumes-kms-keyCLI flag.Tracking: OCPSTRAT-1679
Related: openshift/enhancements#1163
cc @csrwng @muraee @celebdor @enxebre