GCP-391: Add ImageRegistry GSA field and IAM binding for Image Registry Operator#7828
GCP-391: Add ImageRegistry GSA field and IAM binding for Image Registry Operator#7828cblecker wants to merge 5 commits intoopenshift:mainfrom
Conversation
…stry Operator Add the ImageRegistry field to GCPServiceAccountsEmails struct to support the Image Registry Operator on GCP Hosted Control Planes. This is the 5th GSA field, following the established pattern of NodePool, ControlPlane, CloudController, and Storage. Changes: - Add ImageRegistry field to GCPServiceAccountsEmails with validation, immutability CEL rule, and project-scoping CEL validation on GCPPlatformSpec - Add image-registry service account definition in iam-bindings.json with roles/storage.admin for GCS bucket operations - Add ImageRegistryCredsSecret() and extend ReconcileCredentials() credential map and validateWorkloadIdentityConfiguration() validation - Update tests for new field across gcp_test.go and create_iam_test.go Signed-off-by: Christoph Blecker <cblecker@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regenerated after rebuilding codegen tool for Go 1.26 compatibility. Signed-off-by: Christoph Blecker <cblecker@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@cblecker: This pull request references GCP-391 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 "4.22.0" version, but no target version was set. 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. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
@cblecker: This pull request references GCP-391 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 "4.22.0" version, but no target version was set. 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. |
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (4)
WalkthroughAdds an ImageRegistry GCP service account field across API, CRD manifests, client apply configs, docs, IAM bindings, tests, and controller logic; adjusts several GCP service-account minLength validations from 38→37 and enforces immutability and same-project validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as HostedCluster Controller
participant GCP as GCP IAM
participant K8s as Kubernetes API (Secret)
participant ImageReg as Image Registry Operator
Controller->>GCP: Validate imageRegistry GSA format & project (same-project rule)
Controller->>K8s: Create/Update `image-registry-creds` Secret (ImageRegistryCredsSecret)
K8s-->>Controller: Secret persisted
ImageReg->>K8s: Mount/use `image-registry-creds` Secret to access GCS
Note over Controller,GCP: Immutability and same-project validations enforced
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cblecker 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 |
openshift-ci
bot
added
area/cli
|
@cblecker: This pull request references GCP-391 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 "4.22.0" version, but no target version was set. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/infra/gcp/iam-bindings.json (1)
61-65: Consider restricting Image Registry Operator GCS permissions to bucket scope.Line 64 assigns
roles/storage.admin, a project-level role with broad storage management permissions. Thecluster-image-registry-operatorrequires only object-level operations (list, read, write, delete, update). If buckets are pre-created, use bucket-scoped roles instead:roles/storage.objectUserpaired withroles/storage.legacyBucketReader(orroles/storage.objectAdminalone) bound to the specific registry bucket to reduce blast radius. Verify the cluster provisioning workflow does not require project-level bucket management before narrowing scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/infra/gcp/iam-bindings.json` around lines 61 - 65, The current service account entry "image-registry" assigns the project-level role "roles/storage.admin"; replace this with bucket-scoped roles appropriate for object operations (e.g., use "roles/storage.objectAdmin" on the specific registry bucket, or combine "roles/storage.objectUser" + "roles/storage.legacyBucketReader" on that bucket) and ensure the binding is applied to the specific registry bucket rather than the project; update the "roles" array for the "image-registry" entry and confirm the cluster provisioning workflow does not require project-level bucket creation before narrowing the scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml`:
- Around line 5077-5079: The new imageRegistry field is marked +required and
uses an immutability validation rule (self == oldSelf), which prevents adding it
to existing HostedControlPlane objects; either relax the schema or make the
validation transition-safe: remove the +required marker or change the validation
rule for imageRegistry (and the other four service account fields nodePool,
controlPlane, cloudController, storage) to allow initial setting on updates
(e.g., permit when oldSelf is null) or implement a backfill/reconciliation that
populates missing values before enforcing immutability; locate the field
definitions and their x-kubernetes-validations in the generated manifest and
update the required/validation logic accordingly.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 6190-6194: The CRD currently lists imageRegistry as required under
the HostedCluster spec which is incompatible with the immutable workloadIdentity
field; remove imageRegistry from the required array (the block that enumerates
cloudController, controlPlane, imageRegistry, nodePool) so imageRegistry remains
optional while workloadIdentity stays immutable, and ensure you also remove any
duplicate mentions of imageRegistry being required at the other occurrences
noted (the other required arrays around the workloadIdentity definition) so
existing HostedCluster objects can be updated without breaking status writes.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 6082-6087: The CRD currently makes the spec.imageRegistry field
required while spec.workloadIdentity is immutable, which prevents safe
backfilling of existing HostedControlPlane objects; to fix this, remove
"imageRegistry" from the required: list in the HostedControlPlane CRD (the
zz_generated.crd-manifests entry where required includes cloudController,
controlPlane, imageRegistry, nodePool, storage) so the field remains optional
for this release, or alternatively implement a migration/defaulting path (e.g.,
webhook/defaulting logic tied to the same CRD) before making spec.imageRegistry
required in a follow-up release; do not change the immutability of
spec.workloadIdentity in this change.
- Around line 6032-6034: The CRD field currently sets minLength: 38 for GSA
email validation which incorrectly rejects the shortest valid addresses; update
the schema entry that pairs minLength with the regex pattern
^[a-z][a-z0-9-]{4,28}[a-z0-9]@[a-z][a-z0-9-]{4,28}[a-z0-9]\.iam\.gserviceaccount\.com$
by changing minLength from 38 to 37 so the minimal valid service account email
is accepted.
---
Nitpick comments:
In `@cmd/infra/gcp/iam-bindings.json`:
- Around line 61-65: The current service account entry "image-registry" assigns
the project-level role "roles/storage.admin"; replace this with bucket-scoped
roles appropriate for object operations (e.g., use "roles/storage.objectAdmin"
on the specific registry bucket, or combine "roles/storage.objectUser" +
"roles/storage.legacyBucketReader" on that bucket) and ensure the binding is
applied to the specific registry bucket rather than the project; update the
"roles" array for the "image-registry" entry and confirm the cluster
provisioning workflow does not require project-level bucket creation before
narrowing the scope.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (14)
api/hypershift/v1beta1/gcp.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlclient/applyconfiguration/hypershift/v1beta1/gcpserviceaccountsemails.gocmd/infra/gcp/create_iam_test.gocmd/infra/gcp/iam-bindings.jsoncmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamldocs/content/reference/aggregated-docs.mddocs/content/reference/api.mdhypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.gohypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp_test.go
...ated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml
Show resolved
Hide resolved
...shift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
...-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
...-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
|
/pipeline required |
|
Scheduling tests matching the |
The minimum length for a GCP service account email (a@b.iam.gserviceaccount.com) is 37 characters per GCP documentation, not 38. Update all 5 GCPServiceAccountsEmails fields and regenerate CRDs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/test e2e-aws-techpreview |
|
Scheduling tests matching the |
|
@cblecker: This pull request references GCP-391 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 "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml (1)
6082-6087:⚠️ Potential issue | 🟠 MajorRequired
imageRegistry+ immutableworkloadIdentitycan block updates for pre-existing objects.At Line 6085,
imageRegistryis newly required, while Line 6096 keepsworkloadIdentityimmutable. Existing HostedControlPlanes created before this field may fail future updates because they cannot be backfilled.Suggested mitigation
required: - cloudController - controlPlane - - imageRegistry - nodePool - storageAlso applies to: 6096-6097
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml` around lines 6082 - 6087, Newly making spec.imageRegistry required while spec.workloadIdentity remains immutable will block updates for pre-existing HostedControlPlane objects; change the CRD so imageRegistry is not added to the required list (remove "imageRegistry" from the required array in the HostedControlPlane schema) and/or remove the immutability constraint on workloadIdentity (clear the immutable flag on spec.workloadIdentity) so existing resources can be backfilled during upgrade; if you must keep imageRegistry required, instead implement a migration/admission path to populate defaults before enforcing the requirement. Ensure you update the schema entries named imageRegistry and workloadIdentity in the HostedControlPlane CRD.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml (1)
5077-5079:⚠️ Potential issue | 🔴 CriticalRequired + immutable
imageRegistryremains migration-breaking for existing objects.Line 5126 makes
imageRegistrymandatory, while Line 5079 enforces strict immutability. Existing HostedControlPlanes that predate this field and lack it cannot be updated to add it later.Suggested transition-safe direction
- - message: ImageRegistry is immutable - rule: self == oldSelf + - message: ImageRegistry is immutable once set + rule: '!has(oldSelf) || self == oldSelf' ... - - imageRegistry + # keep optional unless backfilled before enforcementPlease apply this in the source API markers (not directly in generated YAML) and regenerate CRDs.
Also applies to: 5123-5127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml` around lines 5077 - 5079, The imageRegistry field is currently marked required and immutable in the generated CRD which blocks upgrades for existing HostedControlPlane objects that lack the field; update the source API markers on the HostedControlPlane spec (the ImageRegistry field in the API Go type) to remove the required validation and make the field optional (add/ensure the kubebuilder Optional marker or remove the +kubebuilder:validation:Required marker), and remove or relax the immutability x-kubernetes-validations marker (the self == oldSelf rule) so that existing objects can be populated during migration; after changing the ImageRegistry field markers in the API type, regenerate the CRDs.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml (1)
6190-6195:⚠️ Potential issue | 🟠 MajorKeep
imageRegistryoptional whileworkloadIdentityis immutable (compat risk).At Line 6193,
imageRegistryis required, butworkloadIdentityis immutable at Line 6204-Line 6205. Existing HostedClusters created before this field existed can fail validation on update/status paths. Also, the Line 6231-Line 6235 CEL rule should be optional-aware if this field is transitional/optional.Proposed transition-safe CRD diff
required: - cloudController - controlPlane - - imageRegistry - nodePool - storage type: object @@ - - message: imageRegistry service account must belong to the same - project - rule: self.workloadIdentity.serviceAccountsEmails.imageRegistry.contains('@') - && self.workloadIdentity.serviceAccountsEmails.imageRegistry.endsWith('@' - + self.project + '.iam.gserviceaccount.com') + - message: if set, imageRegistry service account must belong to the same + project + rule: '!has(self.workloadIdentity.serviceAccountsEmails.imageRegistry) + || (self.workloadIdentity.serviceAccountsEmails.imageRegistry.contains(''@'') + && self.workloadIdentity.serviceAccountsEmails.imageRegistry.endsWith(''@'' + + self.project + ''.iam.gserviceaccount.com''))'Also applies to: 6204-6205, 6231-6235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` around lines 6190 - 6195, The CRD currently lists imageRegistry as required while workloadIdentity is immutable, which can break updates for existing HostedClusters; remove imageRegistry from the required array (so imageRegistry becomes optional) and update the CEL validation (the rule referenced around the CEL block at lines ~6231-6235) to be presence-aware—i.e., guard the CEL expression with a has(self.spec.imageRegistry) check (or equivalent) so the rule only runs when imageRegistry is set; ensure no change makes workloadIdentity (the immutable field) required or subject to this CEL check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 5939-5957: The new required + immutable imageRegistry field will
block updates for existing HostedClusters; make the transition
backward-compatible by (1) removing the required constraint for imageRegistry
(so pre-existing resources without the field aren’t rejected) and (2) relax the
x-kubernetes-validations rule on imageRegistry to allow one-time initialization
by changing the rule from "self == oldSelf" to "oldSelf == null || self ==
oldSelf" (refer to the imageRegistry field and its x-kubernetes-validations
entry in the CRD).
---
Duplicate comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml`:
- Around line 5077-5079: The imageRegistry field is currently marked required
and immutable in the generated CRD which blocks upgrades for existing
HostedControlPlane objects that lack the field; update the source API markers on
the HostedControlPlane spec (the ImageRegistry field in the API Go type) to
remove the required validation and make the field optional (add/ensure the
kubebuilder Optional marker or remove the +kubebuilder:validation:Required
marker), and remove or relax the immutability x-kubernetes-validations marker
(the self == oldSelf rule) so that existing objects can be populated during
migration; after changing the ImageRegistry field markers in the API type,
regenerate the CRDs.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 6190-6195: The CRD currently lists imageRegistry as required while
workloadIdentity is immutable, which can break updates for existing
HostedClusters; remove imageRegistry from the required array (so imageRegistry
becomes optional) and update the CEL validation (the rule referenced around the
CEL block at lines ~6231-6235) to be presence-aware—i.e., guard the CEL
expression with a has(self.spec.imageRegistry) check (or equivalent) so the rule
only runs when imageRegistry is set; ensure no change makes workloadIdentity
(the immutable field) required or subject to this CEL check.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 6082-6087: Newly making spec.imageRegistry required while
spec.workloadIdentity remains immutable will block updates for pre-existing
HostedControlPlane objects; change the CRD so imageRegistry is not added to the
required list (remove "imageRegistry" from the required array in the
HostedControlPlane schema) and/or remove the immutability constraint on
workloadIdentity (clear the immutable flag on spec.workloadIdentity) so existing
resources can be backfilled during upgrade; if you must keep imageRegistry
required, instead implement a migration/admission path to populate defaults
before enforcing the requirement. Ensure you update the schema entries named
imageRegistry and workloadIdentity in the HostedControlPlane CRD.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/gcp.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml
...-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
Test Resultse2e-aks
e2e-aws
|
|
@cblecker: The following tests 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. |
|
The TestOnCreateAPIUX e2e test is failing because the validGCPPlatformSpec() helper in test/e2e/create_cluster_test.go (line 55-60) is missing the new ImageRegistry field. Since it's +required, any GCP HostedCluster created without it fails CRD validation. The fix is adding ImageRegistry to the ServiceAccountsEmails struct there: |
The TestOnCreateAPIUX e2e test was failing because the validGCPPlatformSpec helper was missing the new required ImageRegistry field in ServiceAccountsEmails, causing CRD validation failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Fixed — added |
openshift-ci
bot
added
the
area/testing
|
@cblecker: This pull request references GCP-391 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 "4.22.0" version, but no target version was set. 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. |
|
/test e2e-aws-techpreview |
|
/lgtm |
|
Scheduling tests matching the |
|
File: cmd/cluster/gcp/create.go (not modified in this PR) It looks like the --image-registry-service-account CLI flag is missing from cmd/cluster/gcp/create.go. Since imageRegistry is +required in the API, running hypershift create cluster gcp without a For reference, the existing --cloud-controller-service-account flag covers:
Should the equivalent be added here, or is it intentionally deferred to the next PR? |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Entry("it should reject invalid ImageRegistry service account email",
func(spec *hyperv1.GCPPlatformSpec) {
spec.WorkloadIdentity.ServiceAccountsEmails.ImageRegistry = "invalid-image-registry-email"
},
"imageRegistry in body"), |
…add-gcp-service-account skill Review of PR openshift/hypershift#7828 (ImageRegistry GSA) revealed three gaps not covered by the existing skill documentation: - Step 1b: CLI flag in cmd/cluster/gcp/create.go (required for hypershift create cluster gcp to set +required API fields) - Step 1c: Operator credential secret and WIF validation in hypershift-operator platform code - Step 1d: E2E test fixtures and API UX validation test entries Also updates the PR template to include the new files in staging and test plan. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dation test Add the missing CLI flag for the Image Registry GSA to `hypershift create cluster gcp`, following the established pattern for the 4 existing GSA flags. Also add the ImageRegistry validation test entry to the e2e v2 API UX tests. Signed-off-by: Christoph Blecker <cblecker@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@cristianoveiga Both items addressed in 85e6b04:
|
|
/test e2e-aws-techpreview |
|
@cblecker: This pull request references GCP-391 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 "4.22.0" version, but no target version was set. 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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest ci/prow/docs-preview |
|
/test docs-preview |
|
/lgtm |
|
Scheduling tests matching the |
4 participants
What this PR does / why we need it:
Adds the
ImageRegistryfield toGCPServiceAccountsEmailsand the corresponding IAM binding, enabling the Image Registry Operator on GCP Hosted Control Planes. This is the foundational API change (GCP-391) and IAM binding (GCP-408) that all downstream image registry stories depend on.The
GCPServiceAccountsEmailsstruct already has 4 GSA fields (NodePool,ControlPlane,CloudController,Storage). This PR adds a 5th:ImageRegistry. The IAM bindings JSON already has 4 entries — this adds a 5th. Both changes follow the exact established patterns.Changes:
api/hypershift/v1beta1/gcp.go): AddImageRegistryfield toGCPServiceAccountsEmailswith validation, immutability CEL rule, and project-scoping CEL validation onGCPPlatformSpeccmd/infra/gcp/iam-bindings.json): Addimage-registryservice account withroles/storage.adminfor GCS bucket operations, targetingopenshift-image-registry/cluster-image-registry-operatorhypershift-operator/.../gcp/gcp.go): AddImageRegistryCredsSecret(), extendReconcileCredentials()credential map andvalidateWorkloadIdentityConfiguration()validationimageRegistryfield present in all CRD variantsReference:
/add-gcp-service-accountskill — this PR covers Steps 1-2 (hypershift repo). Steps 3-5 are tracked by GCP-409.Which issue(s) this PR fixes:
Fixes GCP-391, GCP-408
Special notes for your reviewer:
This follows the exact pattern used for the 4 existing GSA fields. No Go code changes needed for IAM bindings — the JSON is loaded via
//go:embedand iterated generically byCreateServiceAccounts()andDeleteServiceAccounts().Checklist:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Validation
Tests
Chores