From 63691f42cce933ee5936d5909568a96e7fb8a9e6 Mon Sep 17 00:00:00 2001 From: Grzegorz Piotrowski Date: Fri, 2 May 2025 15:30:14 +0100 Subject: [PATCH] Add IngressController .spec.domain API validation 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 --- .../generated_openapi/zz_generated.openapi.go | 2 +- openapi/openapi.json | 2 +- .../AAA_ungated.yaml | 149 ++++++++++++++++++ operator/v1/types_ingress.go | 17 ++ ..._50_ingress_00_ingresscontrollers.crd.yaml | 25 +++ .../AAA_ungated.yaml | 25 +++ .../v1/zz_generated.swagger_doc_generated.go | 2 +- 7 files changed, 219 insertions(+), 3 deletions(-) diff --git a/openapi/generated_openapi/zz_generated.openapi.go b/openapi/generated_openapi/zz_generated.openapi.go index b267d878aca..f888beda699 100644 --- a/openapi/generated_openapi/zz_generated.openapi.go +++ b/openapi/generated_openapi/zz_generated.openapi.go @@ -51414,7 +51414,7 @@ func schema_openshift_api_operator_v1_IngressControllerSpec(ref common.Reference Properties: map[string]spec.Schema{ "domain": { SchemaProps: spec.SchemaProps{ - Description: "domain is a DNS name serviced by the ingress controller and is used to configure multiple features:\n\n* For the LoadBalancerService endpoint publishing strategy, domain is\n used to configure DNS records. See endpointPublishingStrategy.\n\n* When using a generated default certificate, the certificate will be valid\n for domain and its subdomains. See defaultCertificate.\n\n* The value is published to individual Route statuses so that end-users\n know where to target external DNS records.\n\ndomain must be unique among all IngressControllers, and cannot be updated.\n\nIf empty, defaults to ingress.config.openshift.io/cluster .spec.domain.", + Description: "domain is a DNS name serviced by the ingress controller and is used to configure multiple features:\n\n* For the LoadBalancerService endpoint publishing strategy, domain is\n used to configure DNS records. See endpointPublishingStrategy.\n\n* When using a generated default certificate, the certificate will be valid\n for domain and its subdomains. See defaultCertificate.\n\n* The value is published to individual Route statuses so that end-users\n know where to target external DNS records.\n\ndomain must be unique among all IngressControllers, and cannot be updated.\n\nIf empty, defaults to ingress.config.openshift.io/cluster .spec.domain.\n\nThe domain value must be a valid DNS name. It must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters. Maximum length of a valid DNS domain is 253 characters.\n\nThe implementation may add a prefix such as \"router-default.\" to the domain when constructing the router canonical hostname. To ensure the resulting hostname does not exceed the DNS maximum length of 253 characters, the domain length is additionally validated at the IngressController object level. For the maximum length of the domain value itself, the shortest possible variant of the prefix and the ingress controller name was considered for example \"router-a.\"", Type: []string{"string"}, Format: "", }, diff --git a/openapi/openapi.json b/openapi/openapi.json index d2bf4d75c32..9cedd957b71 100644 --- a/openapi/openapi.json +++ b/openapi/openapi.json @@ -29844,7 +29844,7 @@ "$ref": "#/definitions/io.k8s.api.core.v1.LocalObjectReference" }, "domain": { - "description": "domain is a DNS name serviced by the ingress controller and is used to configure multiple features:\n\n* For the LoadBalancerService endpoint publishing strategy, domain is\n used to configure DNS records. See endpointPublishingStrategy.\n\n* When using a generated default certificate, the certificate will be valid\n for domain and its subdomains. See defaultCertificate.\n\n* The value is published to individual Route statuses so that end-users\n know where to target external DNS records.\n\ndomain must be unique among all IngressControllers, and cannot be updated.\n\nIf empty, defaults to ingress.config.openshift.io/cluster .spec.domain.", + "description": "domain is a DNS name serviced by the ingress controller and is used to configure multiple features:\n\n* For the LoadBalancerService endpoint publishing strategy, domain is\n used to configure DNS records. See endpointPublishingStrategy.\n\n* When using a generated default certificate, the certificate will be valid\n for domain and its subdomains. See defaultCertificate.\n\n* The value is published to individual Route statuses so that end-users\n know where to target external DNS records.\n\ndomain must be unique among all IngressControllers, and cannot be updated.\n\nIf empty, defaults to ingress.config.openshift.io/cluster .spec.domain.\n\nThe domain value must be a valid DNS name. It must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters. Maximum length of a valid DNS domain is 253 characters.\n\nThe implementation may add a prefix such as \"router-default.\" to the domain when constructing the router canonical hostname. To ensure the resulting hostname does not exceed the DNS maximum length of 253 characters, the domain length is additionally validated at the IngressController object level. For the maximum length of the domain value itself, the shortest possible variant of the prefix and the ingress controller name was considered for example \"router-a.\"", "type": "string" }, "endpointPublishingStrategy": { diff --git a/operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml b/operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml index bb950028ef8..b7f6317ae93 100644 --- a/operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml +++ b/operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml @@ -563,3 +563,152 @@ tests: tuningOptions: connectTimeout: "4 s" expectedError: "IngressController.operator.openshift.io \"default\" is invalid: spec.tuningOptions.connectTimeout: Invalid value: \"4 s\": spec.tuningOptions.connectTimeout in body should match '^(0|([0-9]+(\\.[0-9]+)?(ns|us|µs|μs|ms|s|m|h))+)$'" + - name: Should be able to create an IngressController with valid domain + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "this-label-has-exactly-63-characters-validating-at-the-boundary.com" + expected: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + httpEmptyRequestsPolicy: Respond + idleConnectionTerminationPolicy: Immediate + domain: "this-label-has-exactly-63-characters-validating-at-the-boundary.com" + - name: Should not be able to create an IngressController with invalid domain + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "*.foo.com" + expectedError: "domain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" + - name: Should not be able to create an IngressController with domain label exceeding 63 characters + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "foo.this-label-exceeds-63-characters-for-the-purpose-of-testing-the-domain-validation.com" + expectedError: "each DNS label must not exceed 63 characters" + - name: Should not be able to create an IngressController with the domain exceeding 244 characters + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + 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" + expectedError: "Too long: may not be more than 244 bytes" + - name: Should not be able to create an IngressController with the combined canonical domain exceeding 253 characters + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + 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" + expectedError: "The combined 'router-' + metadata.name + '.' + .spec.domain cannot exceed 253 characters" + onUpdate: + - name: Should be able to update invalid domain to a valid domain + initialCRDPatches: + - op: remove + path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/domain/x-kubernetes-validations + - op: remove + path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/domain/maxLength + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "*.foo.com" + updated: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "123-foo.com" + expected: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + httpEmptyRequestsPolicy: Respond + idleConnectionTerminationPolicy: Immediate + domain: "123-foo.com" + - name: Should not be able to update already invalid domain to another invalid domain + initialCRDPatches: + - op: remove + path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/domain/x-kubernetes-validations + - op: remove + path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/domain/maxLength + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "*.foo.com" + updated: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "foo.*.com" + expectedError: "domain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character" + - name: Should be able to update other fields while retaining invalid domain due to ratcheting + initialCRDPatches: + - op: remove + path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/domain/x-kubernetes-validations + - op: remove + path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/domain/maxLength + initial: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "*.foo.com" + updated: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + domain: "*.foo.com" + replicas: 3 + expected: | + apiVersion: operator.openshift.io/v1 + kind: IngressController + metadata: + name: ic-spec-domain-test + namespace: openshift-ingress-operator + spec: + httpEmptyRequestsPolicy: Respond + idleConnectionTerminationPolicy: Immediate + domain: "*.foo.com" + replicas: 3 diff --git a/operator/v1/types_ingress.go b/operator/v1/types_ingress.go index 2dac08f099b..46b906518d9 100644 --- a/operator/v1/types_ingress.go +++ b/operator/v1/types_ingress.go @@ -35,6 +35,7 @@ import ( // // 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-' + metadata.name + '.' + .spec.domain cannot exceed 253 characters" type IngressController struct { metav1.TypeMeta `json:",inline"` @@ -68,6 +69,22 @@ type IngressControllerSpec struct { // // If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. // + // The domain value must be a valid DNS name. It must consist of lowercase + // alphanumeric characters, '-' or '.', and each label must start and end + // with an alphanumeric character and not exceed 63 characters. Maximum + // length of a valid DNS domain is 253 characters. + // + // The implementation may add a prefix such as "router-default." to the domain + // when constructing the router canonical hostname. To ensure the resulting + // hostname does not exceed the DNS maximum length of 253 characters, + // the domain length is additionally validated at the IngressController object + // level. For the maximum length of the domain value itself, the shortest + // possible variant of the prefix and the ingress controller name was considered + // for example "router-a." + // + // +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" // +optional Domain string `json:"domain,omitempty"` diff --git a/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml b/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml index 10ca42895c3..25c51d79566 100644 --- a/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml +++ b/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yaml @@ -164,7 +164,27 @@ spec: updated. If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. + + The domain value must be a valid DNS name. It must consist of lowercase + alphanumeric characters, '-' or '.', and each label must start and end + with an alphanumeric character and not exceed 63 characters. Maximum + length of a valid DNS domain is 253 characters. + + The implementation may add a prefix such as "router-default." to the domain + when constructing the router canonical hostname. To ensure the resulting + hostname does not exceed the DNS maximum length of 253 characters, + the domain length is additionally validated at the IngressController object + level. For the maximum length of the domain value itself, the shortest + possible variant of the prefix and the ingress controller name was considered + for example "router-a." + maxLength: 244 type: string + x-kubernetes-validations: + - message: domain must consist of lower case alphanumeric characters, + '-' or '.', and must start and end with an alphanumeric character + rule: '!format.dns1123Subdomain().validate(self).hasValue()' + - message: each DNS label must not exceed 63 characters + rule: self.split('.').all(label, size(label) <= 63) endpointPublishingStrategy: description: |- endpointPublishingStrategy is used to publish the ingress controller @@ -3234,6 +3254,11 @@ spec: type: object type: object type: object + x-kubernetes-validations: + - message: The combined 'router-' + metadata.name + '.' + .spec.domain cannot + exceed 253 characters + rule: '!has(self.spec.domain) || size(''router-'' + self.metadata.name + + ''.'' + self.spec.domain) <= 253' served: true storage: true subresources: diff --git a/operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml b/operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml index 6cadc7f0617..c271527c418 100644 --- a/operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml +++ b/operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml @@ -165,7 +165,27 @@ spec: updated. If empty, defaults to ingress.config.openshift.io/cluster .spec.domain. + + The domain value must be a valid DNS name. It must consist of lowercase + alphanumeric characters, '-' or '.', and each label must start and end + with an alphanumeric character and not exceed 63 characters. Maximum + length of a valid DNS domain is 253 characters. + + The implementation may add a prefix such as "router-default." to the domain + when constructing the router canonical hostname. To ensure the resulting + hostname does not exceed the DNS maximum length of 253 characters, + the domain length is additionally validated at the IngressController object + level. For the maximum length of the domain value itself, the shortest + possible variant of the prefix and the ingress controller name was considered + for example "router-a." + maxLength: 244 type: string + x-kubernetes-validations: + - message: domain must consist of lower case alphanumeric characters, + '-' or '.', and must start and end with an alphanumeric character + rule: '!format.dns1123Subdomain().validate(self).hasValue()' + - message: each DNS label must not exceed 63 characters + rule: self.split('.').all(label, size(label) <= 63) endpointPublishingStrategy: description: |- endpointPublishingStrategy is used to publish the ingress controller @@ -3217,6 +3237,11 @@ spec: type: object type: object type: object + x-kubernetes-validations: + - message: The combined 'router-' + metadata.name + '.' + .spec.domain cannot + exceed 253 characters + rule: '!has(self.spec.domain) || size(''router-'' + self.metadata.name + + ''.'' + self.spec.domain) <= 253' served: true storage: true subresources: diff --git a/operator/v1/zz_generated.swagger_doc_generated.go b/operator/v1/zz_generated.swagger_doc_generated.go index be0185eda9b..d23ea7e3285 100644 --- a/operator/v1/zz_generated.swagger_doc_generated.go +++ b/operator/v1/zz_generated.swagger_doc_generated.go @@ -1063,7 +1063,7 @@ func (IngressControllerSetHTTPHeader) SwaggerDoc() map[string]string { var map_IngressControllerSpec = map[string]string{ "": "IngressControllerSpec is the specification of the desired behavior of the IngressController.", - "domain": "domain is a DNS name serviced by the ingress controller and is used to configure multiple features:\n\n* For the LoadBalancerService endpoint publishing strategy, domain is\n used to configure DNS records. See endpointPublishingStrategy.\n\n* When using a generated default certificate, the certificate will be valid\n for domain and its subdomains. See defaultCertificate.\n\n* The value is published to individual Route statuses so that end-users\n know where to target external DNS records.\n\ndomain must be unique among all IngressControllers, and cannot be updated.\n\nIf empty, defaults to ingress.config.openshift.io/cluster .spec.domain.", + "domain": "domain is a DNS name serviced by the ingress controller and is used to configure multiple features:\n\n* For the LoadBalancerService endpoint publishing strategy, domain is\n used to configure DNS records. See endpointPublishingStrategy.\n\n* When using a generated default certificate, the certificate will be valid\n for domain and its subdomains. See defaultCertificate.\n\n* The value is published to individual Route statuses so that end-users\n know where to target external DNS records.\n\ndomain must be unique among all IngressControllers, and cannot be updated.\n\nIf empty, defaults to ingress.config.openshift.io/cluster .spec.domain.\n\nThe domain value must be a valid DNS name. It must consist of lowercase alphanumeric characters, '-' or '.', and each label must start and end with an alphanumeric character and not exceed 63 characters. Maximum length of a valid DNS domain is 253 characters.\n\nThe implementation may add a prefix such as \"router-default.\" to the domain when constructing the router canonical hostname. To ensure the resulting hostname does not exceed the DNS maximum length of 253 characters, the domain length is additionally validated at the IngressController object level. For the maximum length of the domain value itself, the shortest possible variant of the prefix and the ingress controller name was considered for example \"router-a.\"", "httpErrorCodePages": "httpErrorCodePages specifies a configmap with custom error pages. The administrator must create this configmap in the openshift-config namespace. This configmap should have keys in the format \"error-page-.http\", where is an HTTP error code. For example, \"error-page-503.http\" defines an error page for HTTP 503 responses. Currently only error pages for 503 and 404 responses can be customized. Each value in the configmap should be the full response, including HTTP headers. Eg- https://raw.githubusercontent.com/openshift/router/fadab45747a9b30cc3f0a4b41ad2871f95827a93/images/router/haproxy/conf/error-page-503.http If this field is empty, the ingress controller uses the default error pages.", "replicas": "replicas is the desired number of ingress controller replicas. If unset, the default depends on the value of the defaultPlacement field in the cluster config.openshift.io/v1/ingresses status.\n\nThe value of replicas is set based on the value of a chosen field in the Infrastructure CR. If defaultPlacement is set to ControlPlane, the chosen field will be controlPlaneTopology. If it is set to Workers the chosen field will be infrastructureTopology. Replicas will then be set to 1 or 2 based whether the chosen field's value is SingleReplica or HighlyAvailable, respectively.\n\nThese defaults are subject to change.", "endpointPublishingStrategy": "endpointPublishingStrategy is used to publish the ingress controller endpoints to other networks, enable load balancer integrations, etc.\n\nIf unset, the default is based on infrastructure.config.openshift.io/cluster .status.platform:\n\n AWS: LoadBalancerService (with External scope)\n Azure: LoadBalancerService (with External scope)\n GCP: LoadBalancerService (with External scope)\n IBMCloud: LoadBalancerService (with External scope)\n AlibabaCloud: LoadBalancerService (with External scope)\n Libvirt: HostNetwork\n\nAny other platform types (including None) default to HostNetwork.\n\nendpointPublishingStrategy cannot be updated.",