-
Notifications
You must be signed in to change notification settings - Fork 580
OCPBUGS-55192: Add IngressController .spec.domain validation #2308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-55192: Add IngressController .spec.domain validation #2308
Conversation
|
Hello @grzpiotrowski! Some important instructions when contributing to openshift/api: |
|
@grzpiotrowski: This pull request references Jira Issue OCPBUGS-55192, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
2e10452 to
a538d6c
Compare
a538d6c to
d5e7974
Compare
d5e7974 to
a179eae
Compare
c4e9ebf to
9a9eb85
Compare
|
/retitle OCPBUGS-55192: Add IngressController .spec.domain validation |
|
/jira refresh |
0e36abc to
eb307b4
Compare
eb307b4 to
2aa449e
Compare
operator/v1/types_ingress.go
Outdated
| // | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:XValidation:rule="!format.dns1123Subdomain().validate(self).hasValue()",message="domain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" | ||
| // +kubebuilder:validation:XValidation:rule="self.split('.').all(label, !format.dns1123Label().validate(label).hasValue())",message="each DNS label must not exceed 63 characters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does validating against the dns1123Label gain us anything here? Anything that isn't a valid label (apart from length) is validated in the previous rule. So here we just need to check that each label segment has length <= 63 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I agree that it could be replaced with just a length check for each label, which would be cheaper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the label validation to only check for the max length of 63 chars, since the regex check is done in format.dns1123Subdomain() already as you mentioned.
2aa449e to
677e4a4
Compare
677e4a4 to
a6fda06
Compare
| // | ||
| // +kubebuilder:validation:MaxLength=244 | ||
| // +kubebuilder:validation:XValidation:rule="!format.dns1123Subdomain().validate(self).hasValue()",message="domain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" | ||
| // +kubebuilder:validation:XValidation:rule="self.split('.').all(label, size(label) <= 63)",message="each DNS label must not exceed 63 characters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to summarize so far.
The current approach I have is to validate at the level of the IngressController struct, to check for the length (253 max) of the combined domain there, since the ingress controller name can vary, impacting how long the .spec.domain can be in consequence.
I have two validations of the dns domain on the .spec.domain field.
One that uses format.dns1123Subdomain() and the other to check if each label does not exceed 63 chars limit, since the function above doesn't include this.
I needed to set some max length on the .spec.domain field, as to satisfy the validation budget. It is (I think) somewhat superficial, since the real length restriction is checked including the IC.name. I set it to 244 chars, which would be the shortest IC.name (1 char), resulting in the router-a. prefix (9 chars) for the router's canonical domain. Hence 244 chars max for the .spec.domain
Though the format.dns1123Subdomain() also checks for the max length of 253 chars, but this wouldn't come into play here.
JoelSpeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small docs update and then I'm happy with this
operator/v1/types_ingress.go
Outdated
| // | ||
| // Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer). | ||
| // +openshift:compatibility-gen:level=1 | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.spec.domain) || size('router-' + self.metadata.name + '.' + self.spec.domain) <= 253",message="The combined 'router-' + IngressController.Name + '.' + .spec.domain cannot exceed 253 characters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably easier to understand as metadata.name no? Since that's the first we are checking?
| // +kubebuilder:validation:XValidation:rule="!has(self.spec.domain) || size('router-' + self.metadata.name + '.' + self.spec.domain) <= 253",message="The combined 'router-' + IngressController.Name + '.' + .spec.domain cannot exceed 253 characters" | |
| // +kubebuilder:validation:XValidation:rule="!has(self.spec.domain) || size('router-' + self.metadata.name + '.' + self.spec.domain) <= 253",message="The combined 'router-' + metadata.name + '.' + .spec.domain cannot exceed 253 characters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, done now.
This commit fixes OCPBUGS-55192. https://issues.redhat.com/browse/OCPBUGS-55192 Add ratcheting validation of the .spec.domain field of ingress controller. Domain must consist of DNS labels separated by periods, where each label contains only lowercase alphanumeric characters and hyphens, must start and end with an alphanumeric character, and must not exceed 63 characters. The domain must not exceed 253 characters. As part of the ingressController's implementation, cluster-ingress-operator always composes the router's canonical hostname from: `"router-" + ingressController.Name + ingressController.Status.Domain`. For this reason, the maximum length of the .spec.domain field can vary based on the ingressController name set by the user (if it is not "default"). Validation of the IC's .spec.domain field introduced by this commit takes into consideration the actual length of the ingressController.Name, by adding a CEL validation rule on the IngressController object. The .spec.domain field itself needed an additional length limit to pass the validation budget check. The shortest possible variant of the prefix and the ingress controller name was considered: for example "router-a.", which sets the max length of the domain field itself at 244 characters. * operator/v1/types_ingress.go (IngressControllerSpec): Add ratcheting validation of the Domain field. * operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml Add test cases for the ingress controller .spec.domain field validation Generated files: * openapi/openapi.json * openapi/generated_openapi/zz_generated.openapi.go * operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml * operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml * operator/v1/zz_generated.swagger_doc_generated.go
a6fda06 to
63691f4
Compare
|
About ERROR: (v1) ^.spec.domain - maxLength: maximum constraint added when there was none previously : 244Is this something to worry about or override? |
In this case, it is likely overridable. IIRC this is adding validation for something that would have never been valid in the first place. |
|
Tested manually in a cluster: $ oc create -f -<<'EOF'
kind: IngressController
apiVersion: operator.openshift.io/v1
metadata:
name: internal
namespace: openshift-ingress-operator
spec:
domain: '*.myexampledomain.com'
replicas: 1
endpointPublishingStrategy:
loadBalancer:
scope: Internal
type: LoadBalancerService
EOF
The IngressController "internal" is invalid: spec.domain: Invalid value: "string": domain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character$ oc create -f -<<'EOF'
kind: IngressController
apiVersion: operator.openshift.io/v1
metadata:
name: ic-name-containing-exactly-40-characters
namespace: openshift-ingress-operator
spec:
domain: 'this-domain.has-208-characters.which-on.its-own-would-not-exceed.the-limit-of-253-chars.but-combined-with-the-ingress-controller-name.with-40-chars.and-the-router-prefix.ends-up-as-a-too-long.canonical-domain'
replicas: 1
endpointPublishingStrategy:
loadBalancer:
scope: Internal
type: LoadBalancerService
EOF
The IngressController "ic-name-containing-exactly-40-characters" is invalid: <nil>: Invalid value: "object": The combined 'router-' + metadata.name + '.' + .spec.domain cannot exceed 253 characters$ oc create -f -<<'EOF'
kind: IngressController
apiVersion: operator.openshift.io/v1
metadata:
name: a
namespace: openshift-ingress-operator
spec:
domain: 'this-domain.has-exactly-245-characters.for-the-purpose-of-testing.the-spec-domain-field-length-validation.it-exceeds-the-limit-set-on-the-spec-domain-field.by-one-character-to-test-the-error.message-from-this-validation.otherwise-it-is-valid.com'
replicas: 1
endpointPublishingStrategy:
loadBalancer:
scope: Internal
type: LoadBalancerService
EOF
The IngressController "a" is invalid:
* spec.domain: Too long: may not be more than 244 bytes
* <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation$ oc create -f -<<'EOF'
kind: IngressController
apiVersion: operator.openshift.io/v1
metadata:
name: internal
namespace: openshift-ingress-operator
spec:
domain: 'foo.this-label-exceeds-63-characters-for-the-purpose-of-testing-the-domain-validation.com'
replicas: 1
endpointPublishingStrategy:
loadBalancer:
scope: Internal
type: LoadBalancerService
EOF
The IngressController "internal" is invalid: spec.domain: Invalid value: "string": each DNS label must not exceed 63 characters$ oc create -f -<<'EOF'
kind: IngressController
apiVersion: operator.openshift.io/v1
metadata:
name: ic-name-containing-exactly-40-characters
namespace: openshift-ingress-operator
spec:
domain: '*.this-domain.has-210-characters.which-on.its-own-would-not-exceed.the-limit-of-253-chars.but-combined-with-the-ingress-controller-name.with-40-chars.and-the-router-prefix.ends-up-as-a-too-long.canonical-domain'
replicas: 1
endpointPublishingStrategy:
loadBalancer:
scope: Internal
type: LoadBalancerService
EOF
The IngressController "ic-name-containing-exactly-40-characters" is invalid:
* <nil>: Invalid value: "object": The combined 'router-' + metadata.name + '.' + .spec.domain cannot exceed 253 characters
* spec.domain: Invalid value: "string": domain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character$ oc create -f -<<'EOF'
kind: IngressController
apiVersion: operator.openshift.io/v1
metadata:
name: internal
namespace: openshift-ingress-operator
spec:
domain: 'exampledomain.com'
replicas: 1
endpointPublishingStrategy:
loadBalancer:
scope: Internal
type: LoadBalancerService
EOF
ingresscontroller.operator.openshift.io/internal created |
|
Tested along with openshift/cluster-ingress-operator#1295. mjoseph@mjoseph-mac Downloads % oc get clusterversion mjoseph@mjoseph-mac Downloads % oc create -f - < kind: IngressController mjoseph@mjoseph-mac Downloads % oc create -f -<<'EOF' mjoseph@mjoseph-mac Downloads % oc create -f -<<'EOF'
mjoseph@mjoseph-mac Downloads % oc create -f -<<'EOF' mjoseph@mjoseph-mac Downloads % oc create -f -<<'EOF'
/label qe-approved |
|
@melvinjoseph86: This PR has been marked as verified by In 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. |
JoelSpeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/verify-crdify The complaint is about adding a new maximum length constraint, but that is a property that ratchets, so I'm happy we are ok doing this CC @everettraven we might want to teach crdify to ignore (or have an option to ignore) ratchetable changes |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crdify In 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 kubernetes-sigs/prow repository. |
|
@grzpiotrowski: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@grzpiotrowski: Jira Issue Verification Checks: Jira Issue OCPBUGS-55192 Jira Issue OCPBUGS-55192 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 In 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. |
|
Fix included in accepted release 4.21.0-0.nightly-2025-10-22-123727 |
This PR fixes OCPBUGS-55192.
Add ratcheting validation of the .spec.domain field of ingress controller.
Previously, the user could configure the .spec.domain field incorrectly which could result in the router pods entering a
CrashLoopBackOffstate immediately upon creation witherror: invalid canonical hostname: [...].Domain must consist of DNS labels separated by periods, where each label contains only lowercase alphanumeric characters and hyphens, must start and end with an alphanumeric character, and must not exceed 63 characters. The domain must not exceed 253 characters.
As part of the ingressController's implementation, cluster-ingress-operator always composes the router's canonical hostname from:
"router-" + ingressController.Name + ingressController.Status.Domain.For this reason, the maximum length of the .spec.domain field can vary based on the ingressController name set by the user (if it is not "default").
Validation of the IC's .spec.domain field introduced by this commit takes into consideration the actual length of the ingressController.Name, by adding a CEL validation rule on the IngressController object.
The .spec.domain field itself needed an additional length limit to pass the validation budget check. The shortest possible variant of the prefix and the ingress controller name was considered: for example "router-a.", which sets the max length of the domain field itself at 244 characters.
Changes to:
(IngressControllerSpec): Add ratcheting validation of the Domain field.
Add test cases for the ingress controller .spec.domain field validation
Generated files: