OCPBUGS-32275: Add ingress.spec.domain immutability validation #2695
Conversation
Review Summary by QodoAdd immutability validation for Ingress spec.domain field
WalkthroughsDescription• Add immutability validation for spec.domain field in Ingress CRD • Prevent domain changes after initial set to avoid cluster operator degradation • Make domain field optional in validation schema • Add comprehensive test cases for domain immutability behavior Diagramflowchart LR
A["Ingress spec.domain"] -->|Add XValidation rule| B["Immutability constraint"]
B -->|oldSelf == '' OR self == oldSelf| C["Allow initial set or no change"]
C -->|Reject domain changes| D["Prevent operator degradation"]
A -->|Make optional| E["Updated CRD schema"]
F["Test cases"] -->|Validate behavior| G["Domain immutability verified"]
File Changes1. config/v1/types_ingress.go
|
Code Review by Qodo
1. Domain markers undocumented
|
This commit fixes OCPBUGS-32275. https://issues.redhat.com/browse/OCPBUGS-32275 Adds `spec.domain` field in the Ingress config CRD validation to make it immutable and match the documentation. Prior to this commit the domain value could be changed and cause degraded state of some cluster operators.
grzpiotrowski
force-pushed
the
OCPBUGS-32275
branch
from
1057ced to
a58ab73
Compare
📝 WalkthroughWalkthroughThis pull request implements immutability validation for the Ingress domain field. The changes include adding a kubebuilder validation annotation to the Domain field in the IngressSpec type definition, updating the CRD schema validation to include x-kubernetes-validations enforcing that the domain field cannot be changed after initial creation, and adding test cases that verify domain immutability behavior and validate the error message when attempting to update the domain. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@config/v1/tests/ingresses.config.openshift.io/AAA_ungated.yaml`:
- Around line 26-38: Add a new onUpdate test case named "Should not be able to
set domain if initially empty" that uses an initial Ingress manifest with spec:
{} and an updated manifest that sets spec.domain to apps.example.com, and assert
expectedError: "domain is immutable once set"; place this case alongside the
existing "Should not be able to change domain once set" entry so the suite
verifies the transition from empty→set and ensures the validation rule for
immutability-once-set is covered.
In `@payload-manifests/crds/0000_10_config-operator_01_ingresses.crd.yaml`:
- Around line 132-134: The current validation rule "self == oldSelf" (under
x-kubernetes-validations) prevents any change including setting domain from
empty; update the rule to allow initial population by changing it to something
like "oldSelf.domain == null || oldSelf.domain == '' || self == oldSelf" so the
domain can be set when previously empty but remains immutable afterwards;
alternatively, if domain is always set at creation, simply reword the message to
"domain is immutable" instead of implying an initial set is allowed.
| onUpdate: | ||
| - name: Should not be able to change domain once set | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: Ingress | ||
| spec: | ||
| domain: apps.example.com | ||
| updated: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: Ingress | ||
| spec: | ||
| domain: test.example.com | ||
| expectedError: "domain is immutable once set" |
There was a problem hiding this comment.
Missing test for setting domain on an initially empty resource.
There is no test covering the case where the Ingress is created without a domain and then updated to set one. This is the edge case that distinguishes "immutable" from "immutable once set" and directly relates to the concern about the validation rule. Adding this test would clarify the intended behavior:
- name: Should not be able to set domain if initially empty
initial: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec: {}
updated: |
apiVersion: config.openshift.io/v1
kind: Ingress
spec:
domain: apps.example.com
expectedError: "domain is immutable once set"If this transition should be allowed, this test (and the rule) need adjusting accordingly.
🤖 Prompt for AI Agents
In `@config/v1/tests/ingresses.config.openshift.io/AAA_ungated.yaml` around lines
26 - 38, Add a new onUpdate test case named "Should not be able to set domain if
initially empty" that uses an initial Ingress manifest with spec: {} and an
updated manifest that sets spec.domain to apps.example.com, and assert
expectedError: "domain is immutable once set"; place this case alongside the
existing "Should not be able to change domain once set" entry so the suite
verifies the transition from empty→set and ensures the validation rule for
immutability-once-set is covered.
| x-kubernetes-validations: | ||
| - message: domain is immutable once set | ||
| rule: self == oldSelf |
There was a problem hiding this comment.
Rule self == oldSelf prevents setting domain for the first time, not just "once set".
The message says "domain is immutable once set," implying it should be possible to set the domain initially on a resource created without one. However, self == oldSelf blocks all changes, including transitioning from an empty string to a value. If the Ingress resource is ever created (or exists) with an empty domain, this rule locks it to empty permanently.
If the intent is truly "immutable once set," the rule should allow setting an empty field:
Proposed fix
x-kubernetes-validations:
- - message: domain is immutable once set
- rule: self == oldSelf
+ - message: domain is immutable once set
+ rule: oldSelf == '' || self == oldSelfIf, on the other hand, the domain is always populated at creation time by the installer and an empty-to-non-empty transition is never expected, then self == oldSelf is correct but the message is misleading — consider rewording to "domain is immutable".
🤖 Prompt for AI Agents
In `@payload-manifests/crds/0000_10_config-operator_01_ingresses.crd.yaml` around
lines 132 - 134, The current validation rule "self == oldSelf" (under
x-kubernetes-validations) prevents any change including setting domain from
empty; update the rule to allow initial population by changing it to something
like "oldSelf.domain == null || oldSelf.domain == '' || self == oldSelf" so the
domain can be set when previously empty but remains immutable afterwards;
alternatively, if domain is always set at creation, simply reword the message to
"domain is immutable" instead of implying an initial set is allowed.
There was a problem hiding this comment.
Looking at the test cases the domain value can be set initially when the self == oldSelf validation is in place. Rewording the validation error to "domain is immutable" is something to consider maybe, but the current on follows the wording existing in the documentation.
There was a problem hiding this comment.
@grzpiotrowski, you're absolutely right, and I apologize for the confusion. I misunderstood how self == oldSelf behaves in Kubernetes CEL validations.
On CREATE operations, oldSelf is not available, so the rule doesn't block setting the domain initially. On UPDATE operations, the rule correctly prevents any changes. Your test cases confirm this works as intended—the domain can be set on creation but cannot be modified afterward.
The message "domain is immutable once set" accurately describes this behavior and aligns with your documentation. Thank you for the clarification!
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test verify |
|
PR-Agent: could not fine a component named |
|
@grzpiotrowski: all tests passed! 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/severity-important
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-32275, 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-reference
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-32275, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/jira refresh |
|
@grzpiotrowski: An error was encountered querying GitHub for users with public email (hongli@redhat.com) for bug OCPBUGS-32275 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 429 Too Many Requests body: "{\"message\":\"This endpoint is temporarily being throttled. Please try again later. For more on scraping GitHub and how it may affect your rights, please review our Terms of Service (https://docs.github.com/en/site-policy/github-terms/github-terms-of-service)\",\"documentation_url\":\"https://docs.github.com/graphql/using-the-rest-api/rate-limits-for-the-rest-api\",\"status\":\"429\"}"
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@lihongan: This pull request references Jira Issue OCPBUGS-32275, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
3 participants
This PR fixes OCPBUGS-32275.
Adds
spec.domainfield in the Ingress config CRD validation to make it immutable and match the documentation. Prior to this commit the domain value could be changed and cause degraded state of some cluster operators.