CORS-4029: Promote AWS ClusterHostedDNSInstall to Default#2775
CORS-4029: Promote AWS ClusterHostedDNSInstall 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 PR 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.4)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.
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
changed the title
|
@sadasu: This pull request references CORS-4029 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. |
|
/test verify-feature-promotion |
sadasu
changed the title
|
/test verify-feature-promotion |
|
PR-Agent: could not fine a component named |
openshift-ci
bot
added
the
needs-rebase
Also make it available in OKD.
sadasu
force-pushed
the
awsClusterHostedDNS-default
branch
from
e14c8ae to
7d595eb
Compare
openshift-ci
bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1)
1483-1583: Align the CEL rule with the documentedClusterHostedcontract.Lines 1487-1489 say
ClusterHostedneeds LB IP data, but the current validation only forbidsclusterHostedin the non-ClusterHostedcase. An object like{dnsType: ClusterHosted}still validates, so the schema does not distinguish a validClusterHostedpayload from a partial/invalid one. Either requireclusterHostedwhendnsType == ClusterHosted, or relax the description to make partial status explicit.Possible validation tightening
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 when dnsType is ClusterHosted and forbidden otherwise + rule: 'self.dnsType == ''ClusterHosted'' ? has(self.clusterHosted) : !has(self.clusterHosted)'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml` around lines 1483 - 1583, The current CEL validation (x-kubernetes-validations) on the cloudLoadBalancerConfig allows dnsType == "ClusterHosted" without requiring clusterHosted data; change the rule to require clusterHosted when dnsType == "ClusterHosted" (and conversely forbid clusterHosted unless dnsType == "ClusterHosted") by replacing the existing CEL expression with one that enforces: if self.dnsType == 'ClusterHosted' then has(self.clusterHosted) else !has(self.clusterHosted); update the message to reflect the new requirement and keep references to dnsType and clusterHosted in the x-kubernetes-validations entry so the schema enforces the documented contract.
🤖 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_80_machine-config_01_controllerconfigs-OKD.crd.yaml`:
- Around line 1481-1482: The CRD sets a default dnsType: PlatformDefault on the
mirrored status copy (cloudLoadBalancerConfig.dnsType) which, combined with the
immutability check for ClusterHosted (oldSelf == '' || self == oldSelf), can
freeze updates; remove the default on the mirrored status field (dnsType) in the
cloudLoadBalancerConfig block so the mirrored copy does not get an implicit
value, or alternatively remove the immutability constraint from this mirrored
copy—locate the dnsType default in the cloudLoadBalancerConfig section and
either delete that default entry or remove the immutability rule tied to
ClusterHosted to prevent unintentional locking.
---
Nitpick comments:
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml`:
- Around line 1483-1583: The current CEL validation (x-kubernetes-validations)
on the cloudLoadBalancerConfig allows dnsType == "ClusterHosted" without
requiring clusterHosted data; change the rule to require clusterHosted when
dnsType == "ClusterHosted" (and conversely forbid clusterHosted unless dnsType
== "ClusterHosted") by replacing the existing CEL expression with one that
enforces: if self.dnsType == 'ClusterHosted' then has(self.clusterHosted) else
!has(self.clusterHosted); update the message to reflect the new requirement and
keep references to dnsType and clusterHosted in the x-kubernetes-validations
entry so the schema enforces the documented contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d544b5f8-8451-411c-a724-a86c05e2b38a
⛔ 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 (5)
- config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNS.yaml
- config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- config/v1/tests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- features.md
| default: | ||
| dnsType: PlatformDefault |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the file size and structure
wc -l payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 141
🏁 Script executed:
# Read the relevant section around lines 1481-1582
sed -n '1470,1590p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 8753
🏁 Script executed:
# Also check what's immediately before to understand the schema hierarchy
sed -n '1450,1490p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 2420
🏁 Script executed:
# Check broader context to understand if this is spec or status section
sed -n '1350,1490p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | head -150Repository: openshift/api
Length of output: 8552
🏁 Script executed:
# Search for "status" and "spec" keywords around the cloudLoadBalancerConfig section
grep -n "status:\|spec:" payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -E "(14[0-9]{2}|15[0-9]{2}|16[0-9]{2})"Repository: openshift/api
Length of output: 39
🏁 Script executed:
# Check the path context by examining indentation levels
sed -n '1470,1520p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -E "^\s{0,30}(properties|type|description|default|status|spec):"Repository: openshift/api
Length of output: 223
🏁 Script executed:
# Find where platformStatus is defined and what it's under
grep -n "platformStatus:" payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | head -5Repository: openshift/api
Length of output: 99
🏁 Script executed:
# Check the lines just before cloudLoadBalancerConfig to see the full path
sed -n '1200,1500p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -B 100 "cloudLoadBalancerConfig:" | head -120Repository: openshift/api
Length of output: 5598
🏁 Script executed:
# Let's find what the top-level structure of this CRD is
sed -n '1,100p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 4804
🏁 Script executed:
# Look for infra: and status: sections to understand the structure
grep -n "^\s\{0,20\}infra:" payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 81
🏁 Script executed:
# Get the spec properties to see the full hierarchy
sed -n '25,150p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -E "^\s{10,20}(infra|spec|status|properties):"Repository: openshift/api
Length of output: 180
🏁 Script executed:
# More detailed: let's see the spec.properties structure around infra
sed -n '25,200p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | tail -175Repository: openshift/api
Length of output: 9728
🏁 Script executed:
# Let's look at what's at line 305 - the infra field definition
sed -n '300,450p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 9879
🏁 Script executed:
# Now let's search more carefully - check if there are two different platformStatus sections
# One in spec.dns and one in spec.infra
grep -n "platformStatus:" payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 99
🏁 Script executed:
# Let's look at the section around line 1427 to understand this platformStatus better
sed -n '1400,1500p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 5461
🏁 Script executed:
# Let's see the exact immutability rule and its context around line 1576
sed -n '1555,1585p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 2598
🏁 Script executed:
# Check the exact line numbers for the dnsType default and immutability rule
sed -n '1555,1580p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | cat -nRepository: openshift/api
Length of output: 2437
🏁 Script executed:
# Let's verify the exact path - look for "status:" at the right indentation level in infra
sed -n '305,1600p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -n "^\s\{18,22\}status:" | head -5Repository: openshift/api
Length of output: 87
🏁 Script executed:
# Let's see the full structure of spec.infra to understand if there's both spec and status
sed -n '305,350p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 3071
🏁 Script executed:
# Check what comes after spec in the infra definition
sed -n '305,800p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -A 2 "^\s\{18,22\}spec:" | head -20Repository: openshift/api
Length of output: 195
🏁 Script executed:
# Confirm the full path by looking at the structure around line 1025 (where status: is)
sed -n '1015,1035p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 1826
🏁 Script executed:
# Now let's look at the exact sequence: both defaults and the immutability rule
sed -n '1478,1582p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -E "^[^#]*default:|^[^#]*rule:"Repository: openshift/api
Length of output: 514
🏁 Script executed:
# Let's confirm this is in spec.infra.status by tracing the path structure
sed -n '1015,1050p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | head -40Repository: openshift/api
Length of output: 2716
🏁 Script executed:
# Verify the exact line numbers where defaults and rule appear
sed -n '1478,1582p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -n "default:\|rule:" | head -10Repository: openshift/api
Length of output: 536
🏁 Script executed:
# Get exact line numbers for the cloudLoadBalancerConfig default and dnsType default
grep -n "cloudLoadBalancerConfig:" payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | tail -1Repository: openshift/api
Length of output: 116
🏁 Script executed:
# Get exact line numbers for both defaults and the rule
sed -n '1478,1582p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | cat -n | grep -E "(cloudLoadBalancerConfig:|default:|dnsType:|rule:)"Repository: openshift/api
Length of output: 741
🏁 Script executed:
# Calculate exact line numbers and verify the structure one more time
python3 << 'EOF'
# Line 1478 is the start
# Line 3 in output is 1478 + 3 - 1 = 1480
# Line 4 = 1481, Line 5 = 1482
# Line 81 = 1478 + 81 - 1 = 1558
# Line 82 = 1559
# Line 100 = 1577
lines = {
1480: "cloudLoadBalancerConfig:",
1481: "default:",
1482: "dnsType: PlatformDefault",
1559: "default: PlatformDefault (on dnsType)",
1577: "rule: oldSelf == '' || self == oldSelf"
}
for line_num, content in sorted(lines.items()):
print(f"Line {line_num}: {content}")
EOFRepository: openshift/api
Length of output: 247
🏁 Script executed:
# Let's confirm the complete path by checking indentation context for the status field
sed -n '1020,1040p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 1737
🏁 Script executed:
# Verify if platformStatus is under spec or status within infra
sed -n '1420,1435p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 804
🏁 Script executed:
# Check if cloudLoadBalancerConfig is required anywhere or if it can be omitted/empty
sed -n '1478,1600p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | grep -E "required:|cloudLoadBalancerConfig"Repository: openshift/api
Length of output: 214
🏁 Script executed:
# Look at the full description of what this embedded infra object is
sed -n '305,320p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlRepository: openshift/api
Length of output: 977
🏁 Script executed:
# Let's verify the exact indentation to understand the scope of defaults and the immutability rule
sed -n '1478,1585p' payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml | head -110Repository: openshift/api
Length of output: 8022
Avoid defaulting an immutable value on this mirrored status field.
Line 1481 and Line 1559 default dnsType to PlatformDefault at both the object and field levels. If cloudLoadBalancerConfig is created empty, the defaults will apply and the immutability rule at Line 1577 (oldSelf == '' || self == oldSelf) will then prevent any update to ClusterHosted. On an embedded infra.status.platformStatus.aws copy, this can freeze the mirrored value before a producer has set the intended state. Confirm this block is never created without an explicit dnsType value, or remove the default/immutability pairing from this copy.
Also applies to: 1559-1577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml`
around lines 1481 - 1482, The CRD sets a default dnsType: PlatformDefault on the
mirrored status copy (cloudLoadBalancerConfig.dnsType) which, combined with the
immutability check for ClusterHosted (oldSelf == '' || self == oldSelf), can
freeze updates; remove the default on the mirrored status field (dnsType) in the
cloudLoadBalancerConfig block so the mirrored copy does not get an implicit
value, or alternatively remove the immutability constraint from this mirrored
copy—locate the dnsType default in the cloudLoadBalancerConfig section and
either delete that default entry or remove the immutability rule tied to
ClusterHosted to prevent unintentional locking.
|
@sadasu: The following tests 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. |
4 participants
No description provided.