CORS-4403: Add enhancement for GCP customer-managed KMS encryption by barbacbd · Pull Request #1975 · openshift/enhancements

barbacbd · 2026-04-16T11:21:00Z

Proposes adding support for customer-managed Cloud KMS keys
to encrypt GCS buckets used by the OpenShift installer on GCP.
Covers bootstrap ignition bucket and image registry bucket
encryption for compliance and regulatory requirements.

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Reviewed by: @barbacbd

openshift-ci-robot · 2026-04-16T11:21:04Z

@barbacbd: This pull request references CORS-4403 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 story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Proposes adding support for customer-managed Cloud KMS keys
to encrypt GCS buckets used by the OpenShift installer on GCP.
Covers bootstrap ignition bucket and image registry bucket
encryption for compliance and regulatory requirements.

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Reviewed by: @barbacbd

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.

openshift-ci · 2026-04-16T11:21:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JoelSpeed · 2026-05-13T09:19:20Z

+    // used to store bootstrap ignition configs. When omitted, the bucket uses
+    // Google-managed encryption.
+    // +optional
+    IgnitionStorageEncryptionKey *StorageEncryptionKeyReference `json:"ignitionStorageEncryptionKey,omitempty"`


ignitionStorageEncryptionKey.kmsKey has key twice in quick succession, maybe drop the first key?

JoelSpeed · 2026-05-13T09:20:05Z

+    // the image registry GCS bucket. This is consumed by the cluster-image-registry-operator
+    // when creating the registry storage bucket.
+    // +optional
+    RegistryStorageKMSKey *GCPKMSKeyReference `json:"registryStorageKMSKey,omitempty"`


NIt: Recommendation now is to avoid the pointer and use omitzero instead of omitempty

JoelSpeed · 2026-05-13T09:20:53Z

+    // the image registry GCS bucket. This is consumed by the cluster-image-registry-operator
+    // when creating the registry storage bucket.
+    // +optional
+    RegistryStorageKMSKey *GCPKMSKeyReference `json:"registryStorageKMSKey,omitempty"`


Might want to make this more structured in case we have additional stuff to add later

registry: storage: kmsKey: name: ...

JoelSpeed · 2026-05-13T09:22:09Z

+
+#### Standalone Clusters
+
+This enhancement is fully applicable to standalone IPI (Installer Provisioned Infrastructure) clusters on GCP. It does not apply to UPI (User Provisioned Infrastructure) as users manage their own storage infrastructure.


For a UPI cluster, how does the platform status get populated so that the image registry operator can create the bucket?

Or now, what is the expectation for the image registry, do users configure that when they generate the manifests?

I don't think so. The manifest should be generated by the installer still. Even if this is run as upi, the installer is still executed and those manifests would be generated based on your input in the install-config.

JoelSpeed · 2026-05-13T09:24:17Z

+2. **Validation Logic**: The `validatePlatformKMSKeys` function in `pkg/asset/installconfig/gcp/validation.go`
+3. **Default Handling**: The pattern in `pkg/types/gcp/defaults/machinepool.go` for defaulting ProjectID
+
+This provides consistency across the API and reduces implementation complexity.


What happens if a new field is added to KMSKeyReference, you'd expect that to be supported in all three usage locations?

not necessarily. The fields are validated individually so if a new value is added they don't have to support it but ideally any new field should apply to all of them.

JoelSpeed · 2026-05-13T09:28:05Z

+1. **Should we support cross-project KMS keys in the initial implementation?**
+   - This would allow using keys from a central "security" project
+   - Requires additional IAM configuration and validation complexity
+   - Can be deferred to a future enhancement


How would you expect the API to evolve to support this in the future?

JoelSpeed · 2026-05-13T09:29:01Z

+4. **How should we handle the cluster-image-registry-operator changes?**
+   - Option A: Hold this enhancement until operator PR is merged
+   - Option B: Merge installer changes first, operator support comes in a follow-up release
+   - Proposed: Coordinate with operator team, merge in the same release cycle


Can the installer team propose the changes to the operator as part of their work? You'd need commitment from the registry team to review

Is anyone from registry reviewing this EP?

JoelSpeed · 2026-05-13T09:29:46Z

+
+**Downgrading TO a release without this feature:**
+- Cluster continues to function normally
+- Buckets remain encrypted with customer-managed keys


You don't need to know anything about the encryption to read from the bucket right? So registry is not impacted?

that should be true. The service account only needs permission to read so that is more than likely a documentation update (if it doesn't already exist) to ensure the user has this permission when using this feature.

barbacbd · 2026-05-13T15:43:05Z

Response to Review Comments

Thank you @JoelSpeed for the thorough review! I've addressed your feedback and made some important discoveries during analysis. Here's the status of each comment:

✅ Addressed in Commit b96e1e99

Comments 1, 3, 4, 5: API structure, naming, and omitzero

Changed from flat structure to nested: ignition.storage.encryptionKey.kmsKey
Applied omitzero instead of pointers with omitempty
Added structured namespaces for future extensibility

New structure:

platform:
  gcp:
    ignition:
      storage:
        encryptionKey:
          kmsKey:
            name: ignition-key
            keyRing: openshift-keyring
            location: us-east1
    registry:
      storage:
        encryptionKey:
          kmsKey:
            name: registry-key

✅ Resolved by Technical Analysis

Comment 2: Day-2 cluster access to these values

Answer: Not needed. GCS stores the KMS key in bucket metadata at creation time. All subsequent encryption/decryption is automatic if the service account has cloudkms.cryptoKeyDecrypter IAM permission. No cluster component needs to reference the install-config values after install.

Comment 8: Operator changes and registry team coordination

Great news: No operator changes needed! The cluster-image-registry-operator has had full KMS support since 2019 (commit 8c08cc6805).
The ImageRegistry CR field spec.storage.gcs.keyID already exists in openshift/api since 2019.
This is now an installer-only change - we just populate the existing field.
Updated the enhancement (commit eca22306) to remove Infrastructure CR API changes and document existing operator support.

Comment 9: Encryption knowledge for reading

Correct - you don't need to know the encryption key to read. GCS handles encryption/decryption transparently once the key is attached to bucket metadata. The service account just needs cloudkms.cryptoKeyDecrypter IAM permission.
Registry operator is not impacted by encryption.

💬 Discussion Needed

Comment 6: Type reuse concerns with KMSKeyReference

Currently reused across disk encryption, ignition storage, and registry storage
My position: Type reuse is correct because GCP KMS keys have uniform structure regardless of use case
Any new fields Google adds (versions, aliases, HSM level) would likely apply universally
Alternative: Split into use-case-specific types for more flexibility
Question: Do you prefer keeping the shared type or splitting?

Comment 7: Cross-project KMS keys - API evolution

Good news: Current API already supports it! The ProjectID field enables cross-project keys.
Evolution path:
- Phase 1 (initial): Same-project only, validate ProjectID matches cluster
- Phase 2 (future): Support cross-project with updated validation and documentation
- No breaking changes needed
When ProjectID differs from cluster project, we'd validate cross-project IAM permissions are set up
Question: Does this evolution path work for you?

I've documented detailed responses in my analysis files. Happy to discuss Comments 6 and 7 further or adjust the approach based on your feedback.

JoelSpeed · 2026-05-14T10:06:52Z

+reviewers:
+  - "@rochacbruno"
+  - "@patrickdillon"
+approvers:


Missing api-approvers metadata, should be a linter flagging that

done. Thanks!

JoelSpeed · 2026-05-14T10:08:36Z

+
+2. **No New Kubernetes API Changes Needed**: The ImageRegistry CR (`configs.imageregistry.operator.openshift.io/cluster`) already has the necessary field. We do not need to add fields to the Infrastructure CR or create new CRDs.
+
+3. **No Feature Gates Required**: This is install-time configuration (not runtime cluster features), so feature gates are not appropriate. Install-config fields are naturally opt-in (omit = default behavior).


What does your flow look like for testing this feature and making sure that it is GA ready before merge?

We have plenty of install time features that are tested first in tech preview before being promoted to GA, so I'd challenge "feature gates are not appropriate"

We will have to create a new step in the CI that will test this feature. This should ensure it is "green" or good to go before merging.

JoelSpeed · 2026-05-14T10:23:43Z

+
+#### Standalone Clusters
+
+This enhancement is fully applicable to standalone IPI (Installer Provisioned Infrastructure) clusters on GCP. It does not apply to UPI (User Provisioned Infrastructure) as users manage their own storage infrastructure.


Or now, what is the expectation for the image registry, do users configure that when they generate the manifests?

@barbacbd

Proposes adding support for customer-managed Cloud KMS keys to encrypt GCS buckets used by the OpenShift installer on GCP. Covers bootstrap ignition bucket and image registry bucket encryption for compliance and regulatory requirements. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Reviewed by: @barbacbd Update GCP KMS enhancement based on analysis findings Key updates: - Remove Infrastructure CR API changes (use existing ImageRegistry CR) - Remove feature gate references (not needed for install-time config) - Document that registry operator already supports KMS since 2019 - Update IAM permissions to scope to specific service accounts only - Simplify to installer-only changes (no cross-repo coordination) - Update graduation criteria to ship as GA (no Tech Preview needed) - Add Key Findings section summarizing discoveries Analysis documented in CORS-4391 revealed: - ImageRegistry CR already has spec.storage.gcs.keyID field (2019) - cluster-image-registry-operator has full KMS support (commit 8c08cc6805) - No new Kubernetes APIs needed - Feature is naturally opt-in via install-config - Similar to AWS/Azure encryption features (all shipped as GA) Scope reduced by ~50%: no openshift/api changes, no operator changes, no feature gates, no cross-repository coordination overhead. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Use nested API structure for KMS configuration Addresses PR review feedback from @JoelSpeed: - Use nested structure: ignition.storage.encryptionKey instead of flat - Use nested structure: registry.storage.encryptionKey instead of flat - Apply omitzero instead of pointers with omitempty - Provides better extensibility for future fields - Clear separation of ignition vs registry configuration This resolves comments about: - API structure (line 98, line 251) - Naming (line 200 - 'key' duplication removed) - Pointer usage (line 251 - now using omitzero) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

openshift-ci · 2026-05-14T15:20:34Z

@barbacbd: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 16, 2026

openshift-ci Bot requested review from dustymabe and jcantrill April 16, 2026 11:21

JoelSpeed reviewed May 13, 2026

View reviewed changes

barbacbd force-pushed the gcp-kms-encryption branch from 4c6818c to 98ee7ba Compare May 13, 2026 15:43

JoelSpeed reviewed May 14, 2026

View reviewed changes

barbacbd force-pushed the gcp-kms-encryption branch from 98ee7ba to 0723409 Compare May 14, 2026 10:59

barbacbd force-pushed the gcp-kms-encryption branch from 0723409 to e5f818c Compare May 14, 2026 14:51


		#### Standalone Clusters

		This enhancement is fully applicable to standalone IPI (Installer Provisioned Infrastructure) clusters on GCP. It does not apply to UPI (User Provisioned Infrastructure) as users manage their own storage infrastructure.


		2. No New Kubernetes API Changes Needed: The ImageRegistry CR (`configs.imageregistry.operator.openshift.io/cluster`) already has the necessary field. We do not need to add fields to the Infrastructure CR or create new CRDs.

		3. No Feature Gates Required: This is install-time configuration (not runtime cluster features), so feature gates are not appropriate. Install-config fields are naturally opt-in (omit = default behavior).

Conversation

barbacbd commented Apr 16, 2026

Uh oh!

openshift-ci-robot commented Apr 16, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Apr 16, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barbacbd commented May 13, 2026

Response to Review Comments

✅ Addressed in Commit b96e1e99

✅ Resolved by Technical Analysis

💬 Discussion Needed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented Apr 16, 2026 •

edited by openshift-ci Bot

Loading