CORS-4028: Promote AWS ClusterHostedDNS to Default#2775
CORS-4028: Promote AWS ClusterHostedDNS to Default#2775sadasu wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references CORS-4028 which is a valid jira issue. 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: |
📝 WalkthroughWalkthroughThe pull request adds a nullable ✨ 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.11.3)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/XXL
|
[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: 2
🧹 Nitpick comments (1)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml (1)
1733-1734: Add targeted tests for newcloudLoadBalancerConfigvalidation pathsGood defaulting assertions. Please also add cases for
dnsTypeimmutability and theclusterHosted/dnsTypeconditional rule so the new CEL behavior is regression-safe.Also applies to: 1781-1782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml` around lines 1733 - 1734, Add targeted unit tests covering the new cloudLoadBalancerConfig validation paths: (1) a test that asserts dnsType is immutable on updates (ensure attempts to change cloudLoadBalancerConfig.dnsType are rejected), and (2) tests exercising the conditional CEL rule between clusterHosted and dnsType (e.g., when clusterHosted is true/false verify allowed dnsType values and that invalid combinations are rejected). Locate and modify tests referencing cloudLoadBalancerConfig and dnsType (and where clusterHosted is set) around the existing defaulting assertions at the same test file and add cases for both create and update flows so the new CEL validation is covered and regression-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml`:
- Around line 1182-1184: Update the CRD OpenAPI v3 schema to enforce the
documented ClusterHosted DNS requirements: add a conditional "if/then" (or
oneOf/required) that when spec.dnsType == "ClusterHosted" then
spec.clusterHosted must be present, and within that clusterHosted require
non-empty arrays for apiIntLoadBalancerIPs and ingressLoadBalancerIPs while
leaving apiLoadBalancerIPs optional for private clusters; adjust the schema
blocks referencing dnsType, clusterHosted, apiIntLoadBalancerIPs,
ingressLoadBalancerIPs, and apiLoadBalancerIPs (and the PlatformDefault branch)
so the CRD rejects a ClusterHosted dnsType without the required clusterHosted
fields.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml`:
- Around line 1187-1278: The current x-kubernetes-validations rule on the parent
object only forbids clusterHosted when dnsType != 'ClusterHosted' but does not
require clusterHosted (and its required subfields) when dnsType ==
'ClusterHosted'; update the validation logic to enforce presence of
clusterHosted when dnsType is 'ClusterHosted' and ensure required inner fields
(e.g., apiLoadBalancerIPs, apiIntLoadBalancerIPs, ingressLoadBalancerIPs) are
present/non-empty as appropriate. Specifically, modify the parent object's
x-kubernetes-validations rule referencing dnsType and clusterHosted to a
conditional that demands has(self.clusterHosted) when self.dnsType ==
'ClusterHosted', and add x-kubernetes-validations on the clusterHosted object
(or its properties) to require the specific arrays or keys (apiLoadBalancerIPs,
apiIntLoadBalancerIPs, ingressLoadBalancerIPs) to exist or be non-empty when
dnsType == 'ClusterHosted' using rules that reference self and oldSelf as
needed.
---
Nitpick comments:
In `@config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml`:
- Around line 1733-1734: Add targeted unit tests covering the new
cloudLoadBalancerConfig validation paths: (1) a test that asserts dnsType is
immutable on updates (ensure attempts to change cloudLoadBalancerConfig.dnsType
are rejected), and (2) tests exercising the conditional CEL rule between
clusterHosted and dnsType (e.g., when clusterHosted is true/false verify allowed
dnsType values and that invalid combinations are rejected). Locate and modify
tests referencing cloudLoadBalancerConfig and dnsType (and where clusterHosted
is set) around the existing defaulting assertions at the same test file and add
cases for both create and update flows so the new CEL validation is covered and
regression-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6524f40a-3cde-499b-96e4-1fda248beeaf
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (11)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yamlfeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
| 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. |
There was a problem hiding this comment.
Enforce the documented ClusterHosted requirements.
These lines say ClusterHosted DNS needs API/API-Int/ingress load-balancer IPs, but the schema only forbids clusterHosted for PlatformDefault. dnsType: ClusterHosted with no clusterHosted block—or with empty apiIntLoadBalancerIPs / ingressLoadBalancerIPs—still passes, so the CRD admits a status shape that contradicts its own contract. apiLoadBalancerIPs can stay optional for private clusters, but the other two lists should be conditionally required.
Possible schema tightening
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:
@@
+ minItems: 1
maxItems: 16
type: array
x-kubernetes-list-type: set
@@
ingressLoadBalancerIPs:
@@
+ minItems: 1
maxItems: 16
type: array
x-kubernetes-list-type: set
+ required:
+ - apiIntLoadBalancerIPs
+ - ingressLoadBalancerIPs
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'
+ - message: clusterHosted is required exactly when dnsType is ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType == ''ClusterHosted''
+ ? has(self.clusterHosted) : !has(self.clusterHosted)'Also applies to: 1187-1252, 1274-1278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml`
around lines 1182 - 1184, Update the CRD OpenAPI v3 schema to enforce the
documented ClusterHosted DNS requirements: add a conditional "if/then" (or
oneOf/required) that when spec.dnsType == "ClusterHosted" then
spec.clusterHosted must be present, and within that clusterHosted require
non-empty arrays for apiIntLoadBalancerIPs and ingressLoadBalancerIPs while
leaving apiLoadBalancerIPs optional for private clusters; adjust the schema
blocks referencing dnsType, clusterHosted, apiIntLoadBalancerIPs,
ingressLoadBalancerIPs, and apiLoadBalancerIPs (and the PlatformDefault branch)
so the CRD rejects a ClusterHosted dnsType without the required clusterHosted
fields.
| 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.
Enforce required clusterHosted payload when dnsType is ClusterHosted
Line 1275 only forbids clusterHosted for non-ClusterHosted values; it does not require clusterHosted (and its key fields) when dnsType=ClusterHosted, despite the contract in Lines 1182-1184. This allows incomplete configs to pass schema validation.
Suggested schema hardening
properties:
clusterHosted:
@@
type: object
+ required:
+ - apiIntLoadBalancerIPs
+ - apiLoadBalancerIPs
+ - ingressLoadBalancerIPs
@@
x-kubernetes-validations:
+ - message: clusterHosted is required when dnsType is ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType == ''ClusterHosted'' ? has(self.clusterHosted) : true'
- message: clusterHosted is permitted only when dnsType is
ClusterHosted
rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
? !has(self.clusterHosted) : true'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml`
around lines 1187 - 1278, The current x-kubernetes-validations rule on the
parent object only forbids clusterHosted when dnsType != 'ClusterHosted' but
does not require clusterHosted (and its required subfields) when dnsType ==
'ClusterHosted'; update the validation logic to enforce presence of
clusterHosted when dnsType is 'ClusterHosted' and ensure required inner fields
(e.g., apiLoadBalancerIPs, apiIntLoadBalancerIPs, ingressLoadBalancerIPs) are
present/non-empty as appropriate. Specifically, modify the parent object's
x-kubernetes-validations rule referencing dnsType and clusterHosted to a
conditional that demands has(self.clusterHosted) when self.dnsType ==
'ClusterHosted', and add x-kubernetes-validations on the clusterHosted object
(or its properties) to require the specific arrays or keys (apiLoadBalancerIPs,
apiIntLoadBalancerIPs, ingressLoadBalancerIPs) to exist or be non-empty when
dnsType == 'ClusterHosted' using rules that reference self and oldSelf as
needed.
Also make it available in OKD.
sadasu
force-pushed
the
awsClusterHostedDNS-default
branch
from
b3407a0 to
e14c8ae
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml (1)
1182-1278:⚠️ Potential issue | 🟠 MajorRequire the
ClusterHostedstatus payload whendnsTypeisClusterHosted.Line 1182 documents that
ClusterHostedneeds LB IPs, but Lines 1274-1278 only rejectclusterHostedfor non-ClusterHostedvalues. The schema still acceptsdnsType: ClusterHostedwith noclusterHostedobject, or with missing/empty mandatory IP lists, so an incomplete status shape can be stored.apiLoadBalancerIPscan keep the private-cluster exception, but the positive requirement forclusterHostedand the non-optional lists is still missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml` around lines 1182 - 1278, The schema must require the clusterHosted object and its IP lists when dnsType == "ClusterHosted": update the top-level x-kubernetes-validations rule (the one currently preventing clusterHosted when dnsType != ClusterHosted) to instead assert that when self.dnsType == 'ClusterHosted' then has(self.clusterHosted) is true; inside the clusterHosted object (properties clusterHosted -> apiIntLoadBalancerIPs, apiLoadBalancerIPs, ingressLoadBalancerIPs) mark the three arrays as required properties and add minItems: 1 for apiIntLoadBalancerIPs and ingressLoadBalancerIPs (keep apiLoadBalancerIPs allowed to be empty to retain the private-cluster exception), so the status cannot omit or provide empty required LB IP lists; reference the clusterHosted property and the top-level x-kubernetes-validations rule when making these changes.payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml (1)
1182-1278:⚠️ Potential issue | 🟠 MajorRequire the
ClusterHostedstatus payload whendnsTypeisClusterHosted.Line 1182 documents that
ClusterHostedneeds LB IPs, but Lines 1274-1278 only rejectclusterHostedfor non-ClusterHostedvalues. The schema still acceptsdnsType: ClusterHostedwith noclusterHostedobject, or with missing/empty mandatory IP lists, so an incomplete status shape can be stored.apiLoadBalancerIPscan keep the private-cluster exception, but the positive requirement forclusterHostedand the non-optional lists is still missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml` around lines 1182 - 1278, The schema currently allows dnsType: ClusterHosted without requiring the clusterHosted object or its LB IP lists; update the validations so that when dnsType == "ClusterHosted" the clusterHosted object must exist and must contain apiIntLoadBalancerIPs and ingressLoadBalancerIPs with at least one IP each (minItems >= 1 / validation rule checking length > 0), and ensure apiLoadBalancerIPs property is present (but may remain empty to allow private clusters); implement this by adding an x-kubernetes-validations rule referencing dnsType and clusterHosted (and per-field validations for apiIntLoadBalancerIPs, ingressLoadBalancerIPs, and apiLoadBalancerIPs) to enforce existence and non-empty requirements for the required lists when dnsType is ClusterHosted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml`:
- Around line 1182-1278: The schema currently allows dnsType: ClusterHosted
without requiring the clusterHosted object or its LB IP lists; update the
validations so that when dnsType == "ClusterHosted" the clusterHosted object
must exist and must contain apiIntLoadBalancerIPs and ingressLoadBalancerIPs
with at least one IP each (minItems >= 1 / validation rule checking length > 0),
and ensure apiLoadBalancerIPs property is present (but may remain empty to allow
private clusters); implement this by adding an x-kubernetes-validations rule
referencing dnsType and clusterHosted (and per-field validations for
apiIntLoadBalancerIPs, ingressLoadBalancerIPs, and apiLoadBalancerIPs) to
enforce existence and non-empty requirements for the required lists when dnsType
is ClusterHosted.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml`:
- Around line 1182-1278: The schema must require the clusterHosted object and
its IP lists when dnsType == "ClusterHosted": update the top-level
x-kubernetes-validations rule (the one currently preventing clusterHosted when
dnsType != ClusterHosted) to instead assert that when self.dnsType ==
'ClusterHosted' then has(self.clusterHosted) is true; inside the clusterHosted
object (properties clusterHosted -> apiIntLoadBalancerIPs, apiLoadBalancerIPs,
ingressLoadBalancerIPs) mark the three arrays as required properties and add
minItems: 1 for apiIntLoadBalancerIPs and ingressLoadBalancerIPs (keep
apiLoadBalancerIPs allowed to be empty to retain the private-cluster exception),
so the status cannot omit or provide empty required LB IP lists; reference the
clusterHosted property and the top-level x-kubernetes-validations rule when
making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c5e6dd21-2f4c-4662-aa75-8601a5486e5f
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (13)
config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yamlconfig/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNS.yamlconfig/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlfeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
✅ Files skipped from review due to trivial changes (3)
- config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNS.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- features.md
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- features/features.go
|
/retest-required |
|
@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. |
2 participants
No description provided.