CORS-4029: Promote AWSClusterHostedDNSInstall to Default #2589
Conversation
|
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. 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. |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
WalkthroughThe PR extends Infrastructure and MachineConfiguration CRDs by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes Areas requiring extra attention:
✨ 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 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. DetailsNeeds 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.
|
/retest |
This promotes AWS Cluster Hosted DNS feature from techpreview to available by default.
sadasu
force-pushed
the
aws-custom-dns-ga
branch
from
d54365f to
6266c16
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNS.yaml (1)
6-6: Same as AWSClusterHostedDNSInstall tests: gate + ipFamily checks.Concerns and verification steps are identical to the previous file; please address there.
Also applies to: 32-36, 49-53
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1193-1250: Moveformat: ipfrom array level to items schema (duplicate issue).This is the same issue flagged in the previous review. The
format: ipkeyword must be placed on theitemsschema for OpenAPI v3 validation to work correctly. In OpenAPI/Kubernetes schemas,formatapplies only to string types, so for arrays it must be onitems, not the array container.Affected fields in both AWS and GCP sections:
apiIntLoadBalancerIPs(lines 1193–1211 AWS, 1619–1637 GCP)apiLoadBalancerIPs(lines 1212–1231 AWS, 1638–1657 GCP)ingressLoadBalancerIPs(lines 1232–1250 AWS, 1658–1676 GCP)Apply this diff pattern to all six fields:
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: 39 minLength: 1Also applies to: 1619-1676
🧹 Nitpick comments (1)
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1)
1476-1533: Optional: remove array-levelformat: ip; keep it on items only.
formatapplies to string-typed fields, not arrays. You already validate each item withisIP(self). Dropping array-levelformat: ipavoids schema-lint noise without changing behavior.- format: ip items: ... - format: ip items: ... - format: ip items: ...Also applies to: 1906-1963
📜 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 (11)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml(6 hunks)config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNS.yaml(3 hunks)config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yaml(3 hunks)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)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)
🚧 Files skipped from review as they are similar to previous changes (3)
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- features.md
- features/features.go
🧰 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:
config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNS.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlconfig/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yamlconfig/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlmachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml
🔇 Additional comments (9)
config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yaml (1)
6-6: Gate addition: AWSDualStackInstall.Ensure this gate is defined in the feature gates registry and consumed consistently across related controllers and CRDs.
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml (2)
5-5: Confirm gate-negation syntax for feature gates.The syntax "- -AzureClusterHostedDNSInstall" (list item with negation prefix) requires verification that the test harness and Kubernetes feature gate system properly interpret this as an explicit disable. Please confirm this negation form is the intended convention used throughout the configuration and aligns with how the infrastructure configuration system processes feature gate lists.
1695-1697: Verify CRD parity for cloudLoadBalancerConfig and dnsType fields.The test file assumes Infrastructure CRD exposes
status.platformStatus.aws.cloudLoadBalancerConfigwithdnsType=PlatformDefaultas a default value. Confirm theinfrastructures-Default.crd.yamldefines this field structure and default to ensure test stability and prevent flaky behavior. This applies to lines 1695-1697, 1716-1718, 1737-1738, 1766-1768, and 1787-1788.machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1)
1456-1559: Schema LGTM; ensure Infra CRD mirrors the same shape and validators.The dnsType default, immutability, and clusterHosted gating look correct; set-typed IP arrays match the duplicate-IP tests. Please verify the Infrastructure CRD carries the same cloudLoadBalancerConfig for AWS and GCP to avoid drift between ControllerConfig and Infrastructure schemas.
payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml (1)
230-232: Confirm the intentional split between AWSClusterHostedDNS and AWSClusterHostedDNSInstall feature gates.Both gates are OpenShift TechPreview features, but the codebase structure and cross-payload alignment for this specific split cannot be verified automatically. Ensure features/features.go and other FeatureGate payloads consistently reflect whether this represents a distinct install-only vs runtime behavior, and document the reasoning if intentional.
payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml (2)
1174-1277: Immutability and cross-field validation rules are well-constructed.The CEL rules correctly enforce the constraints:
- Line 1270-1271: dnsType immutability allows initial set (oldSelf == '') but prevents changes thereafter
- Lines 1273-1277: Cross-field validation prevents clusterHosted configuration unless dnsType is ClusterHosted
The per-item IP validation with maxItems: 16 and list-type: set ensures data integrity.
1600-1703: GCP cloudLoadBalancerConfig mirrors AWS implementation consistently.Structure, defaults, validation rules, and descriptions are identical across both providers. This consistency improves maintainability and reduces the surface for inconsistency-related bugs.
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (2)
1456-1560: Comprehensive schema addition for AWS cloudLoadBalancerConfig looks well-structured.The new
cloudLoadBalancerConfigfield correctly introduces cluster-hosted DNS configuration with proper validation:
- Default value of
PlatformDefaultfor backward compatibility- Immutability constraint on
dnsTypefield after initial setup- Cross-field validation ensuring
clusterHostedis only populated whendnsTypeisClusterHosted- IP list validation with uniqueness enforced via
x-kubernetes-list-type: setand individual IP address validation rules- Appropriate
maxItems: 16limit per IP list to prevent unbounded growthThe schema properly supports the promotion of AWSClusterHostedDNSInstall from tech preview to default.
1886-1990: GCP cloudLoadBalancerConfig mirrors AWS implementation consistently.The GCP section maintains identical schema structure, validation rules, and semantics to AWS, ensuring platform parity. This symmetry is valuable for maintainability and expected behavior across cloud providers.
| aws: | ||
| cloudLoadBalancerConfig: | ||
| dnsType: PlatformDefault | ||
| ipFamily: IPv4 | ||
| type: AWS |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*infrastructures*crd*.yaml" | head -20Repository: openshift/api
Length of output: 170
🌐 Web query:
OpenShift Infrastructure CRD status platformStatus aws ipFamily schema
💡 Result:
Short answer: The Infrastructure.config.openshift.io status.platformStatus.aws schema does not include an ipFamily field. platformStatus.aws contains AWS-specific status like region, resourceTags and serviceEndpoints; ipFamily is an AWS/EKS cluster networking setting (not part of the OpenShift Infrastructure CRD). [1][2][3]
Sources
- OpenShift Infrastructure API reference — .status.platformStatus.aws (shows aws properties: region, resourceTags, serviceEndpoints). [1][2]
- AWS EKS KubernetesNetworkConfig / ipFamily documentation (ipFamily is an EKS cluster/network setting: "ipv4" or "ipv6"). [3]
References
[1] OpenShift Container Platform — Infrastructure .status.platformStatus (aws properties). (docs.redhat.com)
[2] OKD / OpenShift API reference — .status.platformStatus.aws (region, resourceTags, serviceEndpoints). (docs.okd.io)
[3] AWS EKS KubernetesNetworkConfig / KubernetesNetworkConfigRequest (ipFamily description). (docs.aws.amazon.com)
If you want, I can paste the exact Infrastructure CRD aws JSON/YAML snippet or show how to record IP-family-related info (e.g., where EKS exposes it).
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_container_platform/4.19/html/config_apis/infrastructure-config-openshift-io-v1?utm_source=openai
- 2: https://docs.okd.io/4.11/rest_api/config_apis/infrastructure-config-openshift-io-v1.html?utm_source=openai
- 3: https://docs.aws.amazon.com/eks/latest/APIReference/API_KubernetesNetworkConfigRequest.html?utm_source=openai
Remove ipFamily from status.platformStatus.aws—field not supported by Infrastructure CRD schema.
The Infrastructure CRD schema for status.platformStatus.aws only supports region, resourceTags, and serviceEndpoints. The ipFamily field does not exist at that path and will cause validation failures. This appears in lines 32-36 and 49-53 of the test file.
🤖 Prompt for AI Agents
In
config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yaml
around lines 32-36 (and similarly lines 49-53), the status.platformStatus.aws
object includes an unsupported ipFamily field; remove the ipFamily entries from
status.platformStatus.aws so the YAML conforms to the Infrastructure CRD schema
(which only accepts region, resourceTags, and serviceEndpoints at that path),
leaving the aws block with only supported fields.
|
@sadasu: 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. |
| 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 |
There was a problem hiding this comment.
Why is this marked nullable? We shouldn't have anything marked nullable 🤔
|
@sadasu Looking at the verify, I don't see any hypershift testing, I assume this feature isn't being supported on HyperShift at present? |
3 participants
This promotes AWS Cluster Hosted DNS feature from techpreview to available by default.