CORS-4029: Promote AWSClusterHostedDNSInstall to Default #2589
Conversation
This promotes AWS Cluster Hosted DNS feature from techpreview to available by default.
|
Pipeline controller notification For optional jobs, comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references CORS-4029 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.21.0" version, but no target version was set. 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. |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
WalkthroughThis pull request introduces cloud load balancer configuration capabilities to infrastructure and machine configuration CRDs. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ 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 Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
openshift-ci
bot
added
the
size/L
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![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
🧹 Nitpick comments (5)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1)
1456-1559: AWS cloudLoadBalancerConfig schema is well-structured and consistent with GCPThe new
aws.cloudLoadBalancerConfigblock is backward compatible (optional + defaulted), reuses established IP validation patterns, and mirrors the existing GCP schema, including immutability fordnsTypeand the CEL guard tyingclusterHostedtodnsType == ClusterHosted.If you want to harden this further later, you could add CEL rules to require non-empty IP lists when
dnsTypeisClusterHosted, but that’s an optional enhancement, not a blocker.payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1199-1199: Minor: Moveformat: ipvalidation from array level to items level for OpenAPI compliance.The
format: ipdirective is currently placed at the array container level (e.g., line 1199), but according to OpenAPI/Kubernetes validation semantics, format validation only applies to individual items, not the array itself. Move eachformat: ipinside theitems:block to ensure the schema accurately reflects validation intent.Example fix for
apiIntLoadBalancerIPs:apiIntLoadBalancerIPs: description: ... - format: ip items: + format: ip description: IP is an IP address... maxLength: 39Note: This appears to be a project-wide pattern; if this is intentional or already handled by tooling, this can be deferred to a separate cleanup effort.
Also applies to: 1219-1219, 1238-1238, 1625-1625, 1645-1645, 1664-1664
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1)
1481-1481:format: ipat array level is redundant in OpenAPI schema.The
format: ipdirective is applied to arrays (lines 1481, 1501, 1520) rather than the individual string items. In OpenAPI 3.0,formatapplies to scalar types, not arrays. The actual IP validation is correctly handled byx-kubernetes-validations: isIP(self)rules on the item strings, which is the proper Kubernetes approach.While this pattern exists elsewhere in the file and is harmless (Kubernetes ignores the misplaced directive), consider moving
formatinside theitemsschema for correctness, or removing it since the CEL validation already ensures correctness.Also applies to: 1501-1501, 1520-1520
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1269-1271: Consider stricter immutability semantics for dnsType.The immutability validation rule
oldSelf == '' || self == oldSelfpermits transitions from an empty string to any value, effectively allowing modification during initial setup. While this may be intentional for the install-time configuration workflow, it deviates from typical immutability semantics where a field cannot change once set. Clarify whether this permissive rule aligns with the intended behavior of promoting AWSClusterHostedDNSInstall to default—should users be able to change dnsType post-install?Also applies to: 1695-1697
openapi/openapi.json (1)
30792-30795: Add schema-level duration constraints to httpKeepAliveTimeout.The description documents a valid range of "1 millisecond to 15 minutes," but the schema uses a generic duration type reference without explicit min/max validation constraints. Consider adding
minDurationandmaxDurationannotations or CEL validation rules to the schema to enforce these bounds programmatically, rather than relying on documentation alone."httpKeepAliveTimeout": { "description": "...", "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Duration", + "x-kubernetes-validations": [ + { + "rule": "self >= 'PT1ms' && self <= 'PT15m'", + "message": "duration must be between 1 millisecond and 15 minutes" + } + ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml(1 hunks)features.md(1 hunks)features/features.go(1 hunks)machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml(1 hunks)openapi/openapi.json(7 hunks)payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml(1 hunks)payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
features/features.gofeatures.mdpayload-manifests/featuregates/featureGate-Hypershift-Default.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml
🔇 Additional comments (11)
features.md (1)
75-75: AWSClusterHostedDNSInstall row looks consistent with code and manifestsThe row correctly shows this gate enabled across Default/DevPreview/TechPreview for both Hypershift and SelfManagedHA, matching the FeatureGate JSON and
enableInconfiguration.payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml (1)
227-229: Enabling AWSClusterHostedDNSInstall for SelfManagedHA Default is coherentMoving
AWSClusterHostedDNSInstallinto theenabledlist here aligns SelfManagedHA defaults with the documented feature matrix and Go feature-gate definitions.payload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
233-235: Hypershift Default feature gate now correctly enables AWSClusterHostedDNSInstallAdding
AWSClusterHostedDNSInstallto theenabledlist keeps Hypershift’s Default behavior consistent with SelfManagedHA and the documented feature set.features/features.go (1)
826-832: AWSClusterHostedDNSInstall enablement set matches manifests/docsIncluding
configv1.DefaultinenableIn(...)forFeatureGateAWSClusterHostedDNSInstallis consistent with the updated FeatureGate payloads and the feature matrix; no logic or API-compat issues in this change.payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1174-1277: ✓ Structure and validations are sound across both AWS and GCP implementations.The cloudLoadBalancerConfig additions are well-structured with appropriate defaults, immutability constraints, and conditional validations. Both AWS (lines 1174–1277) and GCP (lines 1600–1703) implementations are correctly mirrored, ensuring consistency.
Key strengths:
- Immutability validation on
dnsTypeprevents accidental DNS configuration changes post-installation- Conditional validation ensures
clusterHostedonly appears whendnsType: ClusterHosted, preventing invalid configurations- Set semantics (
x-kubernetes-list-type: set) enforce unique IP addresses across all load balancer IP arraysmaxItems: 16provides a reasonable cap on IP addressesAlso applies to: 1600-1703
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (3)
1456-1559: Well-structured cloud load balancer configuration with appropriate validations.The AWS cloudLoadBalancerConfig addition is properly designed with:
- Immutability constraint on
dnsTypeafter initial set- Cross-field validation ensuring
clusterHostedonly permitted whendnsType=ClusterHosted- Clear descriptions for operator guidance
- Backward-compatible default value (
PlatformDefault)
1886-1989: GCP cloudLoadBalancerConfig mirrors AWS implementation—good consistency.The GCP section (lines 1886-1989) is structurally identical to the AWS implementation, ensuring consistency across cloud providers. Schema validation rules, defaults, and field structure align properly.
1554-1554: Cross-field validation rule correctly enforces conditional field presence.The validation rule at line 1558 (
has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !has(self.clusterHosted) : true) correctly preventsclusterHostedfrom being set unlessdnsType=ClusterHosted. This safeguard prevents configuration errors and maintains semantic integrity.Also applies to: 1558-1558
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1174-1277: Well-designed cross-field validation and consistent AWS/GCP parity.The cross-field validation rule at lines 1273–1277 (AWS) and 1699–1703 (GCP) correctly enforces that
clusterHostedis only present whendnsTypeisClusterHosted. The identical structure and constraints across both providers ensure consistency and maintainability. The three IP list fields with maxItems: 16 and set semantics provide appropriate constraints for load balancer IP management.Also applies to: 1600-1703
openapi/openapi.json (2)
30662-30666: Add enum constraint to idleConnectionTerminationPolicy.The
idleConnectionTerminationPolicydescription documents two allowed values ("Immediate" and "Deferred"), but the schema snippet shows onlytype: "string"without anenumconstraint. Add the enum to enforce these values and prevent invalid inputs.Is the full schema context available? If yes, verify whether the enum constraint is already present elsewhere in the definition.
Suggested fix:
"idleConnectionTerminationPolicy": { "description": "...", "type": "string", + "enum": ["Immediate", "Deferred"], "default": "Immediate" }
4782-4788: Verify dnsRecordsType additions align with corresponding Go type definition and infrastructure CRD changes.The
dnsRecordsTypefield repetition across five locations (lines 4782-4788, 8935-8941, 9553-9559, 9751-9757, 11670-11676) is expected since openapi.json is auto-generated from Go CRD type definitions. However, verify that the schema changes in this PR correspond to actual changes in the infrastructure CRD manifests and Go type definitions, as the related source files were not provided for inspection.
| cloudLoadBalancerConfig: | ||
| default: | ||
| dnsType: PlatformDefault | ||
| description: |- | ||
| cloudLoadBalancerConfig holds configuration related to DNS and cloud | ||
| load balancers. It allows configuration of in-cluster DNS as an alternative | ||
| to the platform default DNS implementation. | ||
| When using the ClusterHosted DNS type, Load Balancer IP addresses | ||
| must be provided for the API and internal API load balancers as well as the | ||
| ingress load balancer. | ||
| nullable: true | ||
| properties: | ||
| clusterHosted: | ||
| description: |- | ||
| clusterHosted holds the IP addresses of API, API-Int and Ingress Load | ||
| Balancers on Cloud Platforms. The DNS solution hosted within the cluster | ||
| use these IP addresses to provide resolution for API, API-Int and Ingress | ||
| services. | ||
| properties: | ||
| apiIntLoadBalancerIPs: | ||
| description: |- | ||
| apiIntLoadBalancerIPs holds Load Balancer IPs for the internal API service. | ||
| These Load Balancer IP addresses can be IPv4 and/or IPv6 addresses. | ||
| Entries in the apiIntLoadBalancerIPs must be unique. | ||
| A maximum of 16 IP addresses are permitted. | ||
| format: ip | ||
| items: | ||
| description: IP is an IP address (for example, "10.0.0.0" | ||
| or "fd00::"). | ||
| maxLength: 39 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: value must be a valid IP address | ||
| rule: isIP(self) | ||
| maxItems: 16 | ||
| type: array | ||
| x-kubernetes-list-type: set | ||
| apiLoadBalancerIPs: | ||
| description: |- | ||
| apiLoadBalancerIPs holds Load Balancer IPs for the API service. | ||
| These Load Balancer IP addresses can be IPv4 and/or IPv6 addresses. | ||
| Could be empty for private clusters. | ||
| Entries in the apiLoadBalancerIPs must be unique. | ||
| A maximum of 16 IP addresses are permitted. | ||
| format: ip | ||
| items: | ||
| description: IP is an IP address (for example, "10.0.0.0" | ||
| or "fd00::"). | ||
| maxLength: 39 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: value must be a valid IP address | ||
| rule: isIP(self) | ||
| maxItems: 16 | ||
| type: array | ||
| x-kubernetes-list-type: set | ||
| ingressLoadBalancerIPs: | ||
| description: |- | ||
| ingressLoadBalancerIPs holds IPs for Ingress Load Balancers. | ||
| These Load Balancer IP addresses can be IPv4 and/or IPv6 addresses. | ||
| Entries in the ingressLoadBalancerIPs must be unique. | ||
| A maximum of 16 IP addresses are permitted. | ||
| format: ip | ||
| items: | ||
| description: IP is an IP address (for example, "10.0.0.0" | ||
| or "fd00::"). | ||
| maxLength: 39 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: value must be a valid IP address | ||
| rule: isIP(self) | ||
| maxItems: 16 | ||
| type: array | ||
| x-kubernetes-list-type: set | ||
| type: object | ||
| dnsType: | ||
| default: PlatformDefault | ||
| description: |- | ||
| dnsType indicates the type of DNS solution in use within the cluster. Its default value of | ||
| `PlatformDefault` indicates that the cluster's DNS is the default provided by the cloud platform. | ||
| It can be set to `ClusterHosted` to bypass the configuration of the cloud default DNS. In this mode, | ||
| the cluster needs to provide a self-hosted DNS solution for the cluster's installation to succeed. | ||
| The cluster's use of the cloud's Load Balancers is unaffected by this setting. | ||
| The value is immutable after it has been set at install time. | ||
| Currently, there is no way for the customer to add additional DNS entries into the cluster hosted DNS. | ||
| Enabling this functionality allows the user to start their own DNS solution outside the cluster after | ||
| installation is complete. The customer would be responsible for configuring this custom DNS solution, | ||
| and it can be run in addition to the in-cluster DNS solution. | ||
| enum: | ||
| - ClusterHosted | ||
| - PlatformDefault | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: dnsType is immutable | ||
| rule: oldSelf == '' || self == oldSelf | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: clusterHosted is permitted only when dnsType is | ||
| ClusterHosted | ||
| rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' | ||
| ? !has(self.clusterHosted) : true' |
There was a problem hiding this comment.
🧩 Analysis chain
Verify format: ip placement for schema validation compatibility.
The format: ip field is placed at the array level (lines 1199, 1219, 1238 for AWS; lines 1625, 1645, 1664 for GCP) rather than on individual items. In OpenAPI and Kubernetes validation schemas, the format keyword typically applies to the string type of items, not the array container. This placement may not enforce IP validation as intended.
If the format should be on items, apply this diff for the AWS section (and mirror for GCP):
apiIntLoadBalancerIPs:
description: |-
apiIntLoadBalancerIPs holds Load Balancer IPs for the internal API service.
These Load Balancer IP addresses can be IPv4 and/or IPv6 addresses.
Entries in the apiIntLoadBalancerIPs must be unique.
A maximum of 16 IP addresses are permitted.
- format: ip
items:
description: IP is an IP address (for example, "10.0.0.0"
or "fd00::").
+ format: ip
maxLength: 39Also applies to: 1600-1703
🌐 Web query:
Does Kubernetes CRD validation support `format: ip` on array fields in OpenAPI v3 schemas, or should it be on the items?
💡 Result:
Use items. The OpenAPI/CRD schema "format" applies to string types only, so for an array you must put format (e.g. "ip", "ipv4", "ipv6") on the items schema (items.type: string + items.format: ...). Kubernetes docs note format can only be set when type is string, and examples/CEL examples validate items rather than the array itself. [1][2]
Sources:
- Kubernetes API docs: openAPIV3Schema field — "format is an OpenAPI v3 format string... NOTE: Can only be set if type is string." [turn1search2].
- Kubernetes CRD examples / CEL validation showing array items use (put format/constraints under items). [turn1search0].
Move format: ip from array level to items schema for proper IP validation.
The OpenAPI v3 format keyword applies only to string types, so for arrays you must place format (e.g., "ip", "ipv4", "ipv6") on the items schema, not the array container. Currently, format: ip is placed at the array level (lines 1199, 1219, 1238 for AWS; lines 1625, 1645, 1664 for GCP), which will not enforce IP validation as intended.
Apply this fix to all three IP list fields in both AWS and GCP sections:
apiIntLoadBalancerIPs:
description: |-
apiIntLoadBalancerIPs holds Load Balancer IPs for the internal API service.
These Load Balancer IP addresses can be IPv4 and/or IPv6 addresses.
Entries in the apiIntLoadBalancerIPs must be unique.
A maximum of 16 IP addresses are permitted.
- format: ip
items:
description: IP is an IP address (for example, "10.0.0.0"
or "fd00::").
+ format: ip
maxLength: 39Apply the same pattern to apiLoadBalancerIPs and ingressLoadBalancerIPs in both AWS (lines 1199, 1219, 1238) and GCP (lines 1625, 1645, 1664) sections.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml
around lines 1174-1277 (and similarly apply to the GCP blocks referenced at
~1625-1664), the array-level "format: ip" is incorrectly placed on the array
node for apiIntLoadBalancerIPs, apiLoadBalancerIPs and ingressLoadBalancerIPs;
move the "format: ip" key out of the array object and into each field's "items"
schema (next to type: string and its validations), and remove the array-level
format so the OpenAPI string items actually enforce IP format validation for all
three IP list fields in both AWS and GCP sections.
|
@sadasu: The following tests failed, say
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. |
2 participants
This promotes AWS Cluster Hosted DNS feature from techpreview to available by default.