OCPBUGS-98065: Prevent infraID and clusterID removal via parent-level CEL rules - #9102
OCPBUGS-98065: Prevent infraID and clusterID removal via parent-level CEL rules#9102hypershift-jira-solve-ci[bot] wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98065, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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-robot
added
the
jira/invalid-bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] 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/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 395: Update the package filter in the Makefile command to exclude both
the exact ./api path and paths beneath it, using an end-of-string-aware pattern
such as the suggested ./api(/|$) form while preserving the existing vendor and
hack/tools exclusions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 993c044d-2888-45ab-a8d9-45eba670c5b8
⛔ Files ignored due to path filters (19)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/EtcdSharding.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.validation.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
Makefileapi/hypershift/v1beta1/hostedcluster_types.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9102 +/- ##
==========================================
+ Coverage 44.95% 45.09% +0.14%
==========================================
Files 778 780 +2
Lines 97434 97658 +224
==========================================
+ Hits 43797 44043 +246
+ Misses 50615 50571 -44
- Partials 3022 3044 +22 see 20 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.infraID) || has(self.infraID)",message="infraID cannot be removed once set" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.clusterID) || has(self.clusterID)",message="clusterID cannot be removed once set" | ||
| type HostedClusterSpec struct { |
There was a problem hiding this comment.
While this does work if spec cannot be emptied, I took a look and it looks like the HostedClusterSpec type is used in an optional spec field for the HostedCluster resource:
hypershift/api/hypershift/v1beta1/hostedcluster_types.go
Lines 2918 to 2931 in 144cca9
In order for this to be truly immutable this rule must be enforced at the nearest required parent - which in this case would be the root HostedCluster type.
Without enforcing it on the nearest required parent, someone could unset the spec and then re-set the spec with new values for these fields.
There was a problem hiding this comment.
This comment has still not been addressed.
There was a problem hiding this comment.
Done. Moved the CEL rules from HostedClusterSpec to the root HostedCluster type to prevent the two-step bypass via spec removal. The rules now check through self.spec: !has(oldSelf.spec) || !has(oldSelf.spec.infraID) || (has(self.spec) && has(self.spec.infraID)).
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Addressed in the latest push — the rules are now on the root HostedCluster type.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. The CEL rules have been moved from HostedClusterSpec to the root HostedCluster type (lines 2918-2919 in hostedcluster_types.go), preventing the two-step bypass via spec removal. CRDs regenerated and validation test cases added in commit e63a2c0.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
openshift-ci
Bot
added
the
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
c1d9614 to
ea1ab95
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test address-review-comments |
|
Review agent triggered. View job |
openshift-ci
Bot
added
the
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
ea1ab95 to
51bf4a7
Compare
openshift-ci
Bot
added
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
51bf4a7 to
54c3833
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
54c3833 to
8fcee1e
Compare
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
|
/uncc |
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
8fcee1e to
e9fed3b
Compare
|
/test address-review-comments |
… removal Field-level CEL rules on infraID and clusterID only fire when the field is present in both old and new objects. Since both fields are +optional with omitempty, a JSON merge patch setting the field to null removes the key entirely, bypassing validation. The controller then auto-generates a new infraID, causing catastrophic infrastructure re-provisioning. Add parent-level XValidation rules on HostedClusterSpec following the existing secretEncryption pattern: - !has(oldSelf.infraID) || has(self.infraID) - !has(oldSelf.clusterID) || has(self.clusterID) These rules fire at the parent struct level where both old and new objects are always present, preventing removal via null-patch. Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Regenerate CRD manifests and vendor the updated API module to pick up the new parent-level CEL rules for infraID and clusterID immutability. - Update hostedclusters CRD manifests (Default, CustomNoUpgrade, TechPreviewNoUpgrade) with the new XValidation rules - Vendor the updated hostedcluster_types.go - Add CRD validation test cases for infraID and clusterID removal prevention and value change prevention Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
The api/ directory is a separate Go module (github.com/openshift/hypershift/api) and cannot be tested via go test from the root module. The test-changed Makefile target already excludes vendor/ and hack/tools/ but was missing api/, causing failures when commits modify API types. Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
e9fed3b to
290045f
Compare
|
@hypershift-jira-solve-ci[bot]: 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. |
|
Review agent triggered. View job |
5 participants
What this PR does / why we need it:
Field-level CEL rules (
oldSelf == "" || self == oldSelf) oninfraIDandclusterIDonly fire when the field is present in both old and new objects. Since both fields are+optionalwithomitempty, a JSON merge patch setting the field tonullremoves the key entirely, bypassing validation. The controller then auto-generates a new infraID, causing catastrophic infrastructure re-provisioning.This PR adds parent-level
XValidationrules onHostedClusterSpecfollowing the existingsecretEncryptionpattern (!has(oldSelf.infraID) || has(self.infraID)) and the equivalent forclusterID. These rules fire at the parent struct level where both old and new objects are always present.Changes:
infraIDandclusterIDremoval onHostedClusterSpectest-changedMakefile target to exclude theapi/module (separate Go module)Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-98065
Special notes for your reviewer:
The parent-level CEL rules follow the same pattern already used for
secretEncryptionimmutability in the codebase. The field-level rules are preserved for value-change protection; the new parent-level rules specifically guard against field removal.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit
New Features
spec.infraIDandspec.clusterIDfrom being removed after they have been set.Bug Fixes