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 3 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. |
WalkthroughAdds an ImageRegistry GCP service account field to workload identity across API types, CRD manifests, client apply configs, docs, IAM bindings, tests, and controller logic; adjusts several GCP service-account minLength validations from 38→37 and adds immutability and same-project validation. Changes
Sequence Diagram(s)mermaid 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
| x-kubernetes-validations: | ||
| - message: ImageRegistry is immutable | ||
| rule: self == oldSelf |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the exact content at the reported lines
echo "=== Checking GCPPlatform.yaml at reported lines ==="
sed -n '5075,5082p' api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml
echo
echo "=== Checking required fields section around line 5126 ==="
sed -n '5120,5135p' api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml
echo
echo "=== Search for imageRegistry definition in entire file ==="
rg -n "imageRegistry" api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml | head -20Repository: openshift/hypershift
Length of output: 3097
🏁 Script executed:
# Check the source API type for defaulting/conversion markers
echo "=== Search for imageRegistry in API type definitions ==="
find api/hypershift -name "*.go" -type f | xargs rg -l "imageRegistry|ImageRegistry" | head -10
echo
echo "=== Check for webhooks or conversion strategies ==="
find . -path "./vendor" -prune -o -name "*webhook*" -type f -print | head -10
find . -path "./vendor" -prune -o -name "*conversion*" -type f -print | head -10Repository: openshift/hypershift
Length of output: 2109
🏁 Script executed:
# Check similar immutable + required patterns in CRD
echo "=== Look for other immutable required fields ==="
rg -B5 "is immutable" api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/ | grep -E "(required:|message:|rule:)" | head -30Repository: openshift/hypershift
Length of output: 5533
🏁 Script executed:
echo "=== Search for imageRegistry field definition in gcp.go ==="
rg -B5 -A10 "imageRegistry.*string|ImageRegistry" api/hypershift/v1beta1/gcp.go | head -100
echo
echo "=== Check for +kubebuilder markers on imageRegistry ==="
rg -B10 "imageRegistry" api/hypershift/v1beta1/gcp.go | grep -E "^\+" | head -20
echo
echo "=== Check if imageRegistry has default value or omitempty tag ==="
rg -B3 -A3 'imageRegistry.*string' api/hypershift/v1beta1/gcp.go | head -50Repository: openshift/hypershift
Length of output: 1038
🏁 Script executed:
echo "=== Check GCPServiceAccountsEmails struct definition ==="
ast-grep --pattern 'type GCPServiceAccountsEmails struct {
$$$
}'
echo
echo "=== Look for SetDefaults or defaulting webhooks ==="
rg -n "SetDefaults|Default.*GCP|webhook" api/hypershift/v1beta1/gcp.go | head -20Repository: openshift/hypershift
Length of output: 23726
🏁 Script executed:
echo "=== Check if imageRegistry was recently added or if field is conditionally required ==="
git log --oneline --all -- api/hypershift/v1beta1/gcp.go | head -10
git log --oneline --all -- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml | head -10Repository: openshift/hypershift
Length of output: 1544
🏁 Script executed:
echo "=== Check if imageRegistry is auto-populated by reconciliation or webhooks ==="
rg -n "imageRegistry|ImageRegistry" api/hypershift/v1beta1/ --glob="*.go" | grep -iE "default|set|populate|webhook|reconcil" | head -20
echo
echo "=== Verify the contradiction: field is required but omitempty ==="
grep -A2 "ImageRegistry string" api/hypershift/v1beta1/gcp.go | head -20Repository: openshift/hypershift
Length of output: 267
🏁 Script executed:
echo "=== Check if this breaks existing GCP platform objects without imageRegistry ==="
rg -n "GCPPlatform" api/hypershift/v1beta1/ --glob="*.go" -A5 | grep -E "required|imageRegistry" | head -30
echo
echo "=== See if other newly added fields follow the same unsafe pattern ==="
git show 598274204 --statRepository: openshift/hypershift
Length of output: 50378
imageRegistry field is both required and immutable, blocking updates to pre-existing HostedControlPlane objects.
This field was just added (commit 5982742) with the marker +required and immutability rule self == oldSelf. Pre-existing HostedControlPlane objects created before this change will fail validation during the next update because they lack this newly required field. The immutability rule self == oldSelf prevents adding the field after creation—when oldSelf is absent and self is set, they never match.
Transition-safe fix
imageRegistry:
x-kubernetes-validations:
- - message: ImageRegistry is immutable
- rule: self == oldSelf
+ - message: ImageRegistry is immutable once set
+ rule: '!has(oldSelf) || self == oldSelf'
required:
- cloudController
- controlPlane
- - imageRegistry
+ # Remove imageRegistry from required if it cannot be auto-populatedNote: The same pattern affects all five service account fields (nodePool, controlPlane, cloudController, storage, imageRegistry). Verify whether there is a reconciliation mechanism to backfill missing imageRegistry on existing objects, or whether this field should not be required.
🤖 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 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.
There was a problem hiding this comment.
Not applicable. GCP HCP is pre-GA (behind the GCPPlatform feature gate) with no long-lived clusters in existence. All 5 GSA fields follow the same +required + +immutable pattern established when the struct was first introduced. There are no existing objects to break.
There was a problem hiding this comment.
@cblecker Thanks for the context. Acknowledged—since GCP HCP is pre-GA behind the GCPPlatform feature gate and there are no long-lived clusters, the required + immutable pattern for all five GSA fields (including imageRegistry) is intentional and safe. I’m withdrawing the earlier “migration risk” as non-blocking and won’t suggest schema changes here.
If you’d like, I can tune future review heuristics to avoid flagging “required + immutable” in this feature-gated GCPPlatform area for this repo.
(=^-^=)っ∩? Thanks for the clarification!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| required: | ||
| - cloudController | ||
| - controlPlane | ||
| - imageRegistry | ||
| - nodePool |
There was a problem hiding this comment.
Avoid making imageRegistry required inside an immutable workloadIdentity.
At Line 6193, imageRegistry becomes required, while workloadIdentity is immutable at Line 6204. Existing HostedClusters created before this field existed cannot add it later, which can cause update/status write failures on those objects. Keep this field optional for compatibility (at least for one transition release), and gate enforcement elsewhere.
💡 Proposed CRD-side transition fix
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 - 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.
There was a problem hiding this comment.
Not applicable. GCP HCP is pre-GA (behind the GCPPlatform feature gate) with no long-lived clusters in existence. All 5 GSA fields follow the same +required + +immutable pattern established when the struct was first introduced. There are no existing objects to break.
There was a problem hiding this comment.
@cblecker
Thanks for the clarification. Given GCP HCP is pre-GA and gated behind GCPPlatform, and there are no long-lived HostedClusters, keeping imageRegistry required and maintaining the existing required+immutable pattern across all five GSAs is acceptable. I’m retracting the earlier suggestion about making imageRegistry optional. The validations around project scoping (e.g., Lines 6231–6235) also remain appropriate under this context.
As a minor follow-up, I’ll just note for release notes/docs that imageRegistry is now part of the required GSA set when the GCPPlatform gate is enabled.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
...-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
| required: | ||
| - cloudController | ||
| - controlPlane | ||
| - imageRegistry | ||
| - nodePool | ||
| - storage |
There was a problem hiding this comment.
Required + immutable combination is migration-hostile for existing objects.
Line 6085 makes imageRegistry mandatory, but workloadIdentity is immutable (Line 6096 context). Existing HCPs missing this field cannot be backfilled safely after CRD rollout, which risks breaking updates/reconciliation.
A safer rollout pattern is to keep this field optional for one release (or introduce a migration/defaulting path), then enforce requiredness once legacy objects are migrated.
🤖 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, 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.
There was a problem hiding this comment.
Not applicable. GCP HCP is pre-GA (behind the GCPPlatform feature gate) with no long-lived clusters in existence. All 5 GSA fields follow the same +required + +immutable pattern established when the struct was first introduced. There are no existing objects to break.
There was a problem hiding this comment.
@cblecker Thanks, acknowledged. I verified this CRD carries the merged-by-featuregates annotation and the imageRegistry field is required alongside the other GSAs. Given GCP HCP is pre-GA behind the GCPPlatform feature gate with no long‑lived clusters, I agree the required+immutable combo is acceptable here and consistent with the existing pattern. No changes requested; I’ll just keep a note to re-check requiredness before/when the gate is lifted/GA.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
/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
| imageRegistry: | ||
| description: |- | ||
| imageRegistry is the Google Service Account email for the Image Registry Operator | ||
| that manages GCS storage for the internal container image registry. | ||
| This GSA requires the following IAM roles: | ||
| - roles/storage.admin (Storage Admin - for creating and managing GCS buckets and objects) | ||
| See cmd/infra/gcp/iam-bindings.json for the authoritative role definitions. | ||
| Format: service-account-name@project-id.iam.gserviceaccount.com | ||
|
|
||
| This is a user-provided value referencing a pre-created Google Service Account. | ||
| Typically obtained from the output of `hypershift infra create gcp` which creates | ||
| the required service accounts with appropriate IAM roles and WIF bindings. | ||
| maxLength: 100 | ||
| minLength: 37 | ||
| pattern: ^[a-z][a-z0-9-]{4,28}[a-z0-9]@[a-z][a-z0-9-]{4,28}[a-z0-9]\.iam\.gserviceaccount\.com$ | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: ImageRegistry is immutable | ||
| rule: self == oldSelf |
There was a problem hiding this comment.
Required + immutable imageRegistry may block updates for pre-existing HostedClusters.
Adding imageRegistry as required while also enforcing strict immutability can break updates for existing resources created before this field existed. Please add a migration/backfill path or allow one-time initialization before immutability is enforced.
🔧 Backward-compatible transition example
imageRegistry:
...
x-kubernetes-validations:
- - message: ImageRegistry is immutable
- rule: self == oldSelf
+ - message: ImageRegistry is immutable after initial set
+ rule: oldSelf == null || self == oldSelf
required:
- cloudController
- controlPlane
- - imageRegistry
- nodePool
- storageAlso applies to: 6001-6005
🤖 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-TechPreviewNoUpgrade.crd.yaml`
around lines 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).
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. |
3 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
Tests
Chores