WIP: OCPBUGS-66104: Add CloudDNSIPs to CloudLoadBalancerConfig#2718
Open
sadasu wants to merge 1 commit intoopenshift:masterfrom
Open
WIP: OCPBUGS-66104: Add CloudDNSIPs to CloudLoadBalancerConfig#2718sadasu wants to merge 1 commit intoopenshift:masterfrom
sadasu wants to merge 1 commit intoopenshift:masterfrom
Conversation
CloudDNSIPs hold the upstream DNS server IPs. These will be used to configure the in-cluster CoreDNS instance when DNSType is set to ClusterHostedDNS.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
sadasu
changed the title
openshift-ci
bot
added
the
size/XXL
openshift-ci-robot
added
jira/severity-critical
|
@sadasu: This pull request references Jira Issue OCPBUGS-66104, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
Contributor
|
[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 |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd CloudDnsIPs field to CloudLoadBalancerConfig for cluster-hosted DNS configuration
WalkthroughsDescription• Add cloudDnsIPs field to CloudLoadBalancerConfig struct for upstream DNS server IPs • Field holds cloud platform DNS server addresses used when cluster DNS type is ClusterHosted • Includes validation: IP format, maximum 16 addresses, unique entries as set type • Update all generated manifests, CRDs, OpenAPI specs, and deep copy functions File Changes1. config/v1/types_infrastructure.go
|
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Code Review by Qodo
1. CloudDnsIPs missing FeatureGate marker
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+902
to
+913
| // cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| // These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| // For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| // This field is only valid when dnsType is set to ClusterHosted. | ||
| // When dnsType is not ClusterHosted, this field must not be set. | ||
| // Entries in cloudDnsIPs must be unique. | ||
| // A maximum of 16 IP addresses are permitted. | ||
| // +kubebuilder:validation:Format=ip | ||
| // +listType=set | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| // +optional | ||
| CloudDnsIPs []IP `json:"cloudDnsIPs,omitempty"` |
There was a problem hiding this comment.
1. clouddnsips missing featuregate marker 📘 Rule violation ✓ Correctness
The new CloudDnsIPs field is added directly to the stable config/v1 API (CloudLoadBalancerConfig) without a +openshift:enable:FeatureGate= marker. Other new fields in the same file consistently use this gating pattern to protect stable API contracts.
Agent Prompt
## Issue description
The `CloudDnsIPs` field is added to the stable `config/v1` `CloudLoadBalancerConfig` struct without the required `+openshift:enable:FeatureGate=` marker, exposing an ungated field on a stable API.
## Issue Context
Other fields in the same file follow this pattern (e.g., `+openshift:enable:FeatureGate=AWSClusterHostedDNSInstall` at line 562). The referenced FeatureGate must also exist in `features/features.go`.
## Fix Focus Areas
- config/v1/types_infrastructure.go[902-913]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
872
to
876
| // +kubebuilder:validation:XValidation:rule="has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !has(self.clusterHosted) : true",message="clusterHosted is permitted only when dnsType is ClusterHosted" | ||
| // TODO: Is it useful to have a kubebuilder validation for CloudDnsIPs too? | ||
| // +union | ||
| type CloudLoadBalancerConfig struct { | ||
| // dnsType indicates the type of DNS solution in use within the cluster. Its default value of |
There was a problem hiding this comment.
2. clouddnsips constraint not xvalidation-enforced 📘 Rule violation ✓ Correctness
The field comment explicitly states 'When dnsType is not ClusterHosted, this field must not be set', but no +kubebuilder:validation:XValidation rule enforces this constraint. The PR even includes a TODO comment acknowledging this enforcement gap.
Agent Prompt
## Issue description
The documented constraint 'When dnsType is not ClusterHosted, this field must not be set' for `cloudDnsIPs` is not machine-enforced by any CEL XValidation rule, allowing invalid configurations to be accepted by the API server.
## Issue Context
The existing rule on `CloudLoadBalancerConfig` covers only `clusterHosted`. A parallel rule for `cloudDnsIPs` needs to be added following the same pattern. Suggested addition:
```
// +kubebuilder:validation:XValidation:rule="has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !has(self.cloudDnsIPs) || self.cloudDnsIPs.size() == 0 : true",message="cloudDnsIPs is permitted only when dnsType is ClusterHosted"
```
The TODO comment at line 873 should also be removed.
## Fix Focus Areas
- config/v1/types_infrastructure.go[872-874]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
📝 WalkthroughWalkthroughThis change adds a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
config/v1/types_infrastructure.go (1)
872-913:⚠️ Potential issue | 🟠 MajorAdd XValidation to enforce
cloudDnsIPsgating bydnsTypeThe field documentation states
cloudDnsIPsmust only be set whendnsTypeisClusterHosted, but there is no validation rule enforcing this. Invalid configurations could be admitted. Add a CEL rule to match the constraint already documented and remove the TODO.Proposed validation rule
-// TODO: Is it useful to have a kubebuilder validation for CloudDnsIPs too? +// +kubebuilder:validation:XValidation:rule="!has(self.cloudDnsIPs) || self.dnsType == 'ClusterHosted'",message="cloudDnsIPs is permitted only when dnsType is ClusterHosted"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_infrastructure.go` around lines 872 - 913, The CloudLoadBalancerConfig struct documents that CloudDnsIPs may only be set when DNSType (dnsType) == ClusterHosted but lacks a CEL validation; add a kubebuilder XValidation rule on CloudDnsIPs to enforce that condition (e.g. +kubebuilder:validation:XValidation:rule="has(self.cloudDnsIPs) ? self.dnsType == 'ClusterHosted' : true",message="cloudDnsIPs is permitted only when dnsType is ClusterHosted"), add +optional as needed, and remove the TODO comment; update references to CloudLoadBalancerConfig, CloudDnsIPs, DNSType/dnsType in the diff so the validator targets the correct field.payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (3)
1492-1605:⚠️ Potential issue | 🟠 MajorEnforce
dnsTypegating + fix AWS-specific wording in Azure docstring.
cloudDnsIPsis described as valid only forClusterHosted, but there is no validation rule. Also, the AWS VPC example is misleading in the Azure block.🔧 Suggested validation rule
x-kubernetes-validations: - message: clusterHosted is permitted only when dnsType is ClusterHosted rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' ? !has(self.clusterHosted) : true' + - message: cloudDnsIPs is permitted only when dnsType is + ClusterHosted + rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' + ? !has(self.cloudDnsIPs) : 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-CustomNoUpgrade.crd.yaml` around lines 1492 - 1605, The schema currently allows cloudDnsIPs even when dnsType != ClusterHosted and the cloudDnsIPs description contains an AWS-specific VPC example in an Azure CRD; add an x-kubernetes-validations entry on the cloudDnsIPs property to require it only when dnsType == "ClusterHosted" (rule: has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !self : true or equivalent using oldSelf pattern consistent with existing dnsType immutability validation) and update the cloudDnsIPs description to remove or neutralize the AWS-specific VPC example (make the IP example generic like "10.0.0.2" or remove provider-specific wording); target the cloudDnsIPs, dnsType, and clusterHosted symbols and the x-kubernetes-validations block for these changes.
1265-1379:⚠️ Potential issue | 🟠 MajorAdd schema validation to forbid
cloudDnsIPsunlessdnsTypeisClusterHosted.The description says this field is only valid for
ClusterHosted, but the schema doesn’t enforce it. This allows invalid configs to pass validation.🔧 Suggested validation rule
x-kubernetes-validations: - message: clusterHosted is permitted only when dnsType is ClusterHosted rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' ? !has(self.clusterHosted) : true' + - message: cloudDnsIPs is permitted only when dnsType is + ClusterHosted + rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' + ? !has(self.cloudDnsIPs) : 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-CustomNoUpgrade.crd.yaml` around lines 1265 - 1379, Add a schema-level validation on the cloudDnsIPs property to forbid it unless dnsType == 'ClusterHosted'; update the cloudDnsIPs block (property name: cloudDnsIPs) to include an x-kubernetes-validations entry that checks dnsType (symbol: dnsType) and rejects cloudDnsIPs when dnsType is absent or not 'ClusterHosted' (use a rule like: has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !self || size(self) == 0 : true, or equivalent expression) so the CRD enforces the description text. Ensure the validation message clearly states that cloudDnsIPs is only permitted when dnsType is ClusterHosted and add it alongside the existing validations for cloudDnsIPs.
1891-2004:⚠️ Potential issue | 🟠 MajorEnforce
dnsTypegating + fix AWS-specific wording in GCP docstring.
cloudDnsIPsis described as valid only forClusterHosted, but there is no validation rule. Also, the AWS VPC example is misleading in the GCP block.🔧 Suggested validation rule
x-kubernetes-validations: - message: clusterHosted is permitted only when dnsType is ClusterHosted rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' ? !has(self.clusterHosted) : true' + - message: cloudDnsIPs is permitted only when dnsType is + ClusterHosted + rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' + ? !has(self.cloudDnsIPs) : 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-CustomNoUpgrade.crd.yaml` around lines 1891 - 2004, Add a validation to ensure cloudDnsIPs is only set when dnsType == "ClusterHosted" and remove the AWS-specific VPC example from the cloudDnsIPs description; specifically, update the cloudDnsIPs property (symbol: cloudDnsIPs) to include an x-kubernetes-validations rule that enforces "has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !has(self.cloudDnsIPs) : true" (or equivalent) and update the description text to a neutral example (remove the AWS VPC-specific line); reference the dnsType and clusterHosted fields as needed to mirror the existing validation style used for clusterHosted (x-kubernetes-validations at the same parent object).payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (1)
1373-1378: 🛠️ Refactor suggestion | 🟠 MajorExisting validation pattern for
clusterHostedshould be extended tocloudDnsIPs.This existing validation rule correctly enforces that
clusterHostedis only permitted whendnsTypeisClusterHosted. The same pattern should be applied to the newcloudDnsIPsfield to maintain consistency and enforce the documented constraint.🤖 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-TechPreviewNoUpgrade.crd.yaml` around lines 1373 - 1378, The current x-kubernetes-validations entry enforces that clusterHosted is only allowed when dnsType == 'ClusterHosted'; extend this same validation to also restrict cloudDnsIPs by updating the validation rule for the object under x-kubernetes-validations so it checks both fields (clusterHosted and cloudDnsIPs) — e.g., modify the rule that currently references has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !has(self.clusterHosted) : true to also require !has(self.cloudDnsIPs) when dnsType is not 'ClusterHosted', keeping the same message/validation mechanism for the fields clusterHosted and cloudDnsIPs.
🧹 Nitpick comments (2)
config/v1/types_infrastructure.go (1)
902-913: RenameCloudDnsIPs→CloudDNSIPsfor consistency with DNS naming patternsThe field should use the
DNSinitialism in uppercase to align with existing types (DNSType,DNSRecordsType,NodeDNSIP). Since this is a newly added public field, making this change now prevents a breaking change for Go clients later.♻️ Suggested rename
-CloudDnsIPs []IP `json:"cloudDnsIPs,omitempty"` +CloudDNSIPs []IP `json:"cloudDnsIPs,omitempty"`payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml (1)
1799-1799: AWS-specific example in GCP platform section may confuse users.The description references AWS VPC CIDR addressing ("For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2"). Since this field appears under the GCP
platformStatus, consider making the description platform-agnostic or adding GCP-specific guidance.🤖 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` at line 1799, The description under platformStatus currently mentions an AWS-specific example ("For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2"), which can confuse users of the GCP section; update the description in the platformStatus/GCP entry to either use platform-agnostic wording (e.g., "Set the DNS IP(s) for the network; consult your cloud provider or VPC/subnet configuration for the correct address") or add a GCP-specific example (e.g., explain how to determine the DNS IP for GCP VPCs), and ensure the field name referenced (platformStatus -> GCP -> dns IPs) is clear so readers know where to find provider-specific guidance.
🤖 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 1509-1530: Add a CEL validation on the cloudDnsIPs property to
enforce the dnsType gating: add an x-kubernetes-validations entry for
cloudDnsIPs with a rule like 'self == null || size(self) == 0 ||
(root.spec.dnsType == "ClusterHosted")' and an appropriate message (e.g.,
"cloudDnsIPs may only be set when dnsType is ClusterHosted"); reference the
cloudDnsIPs field and the dnsType value "ClusterHosted" so the CRD rejects
cloudDnsIPs unless root.spec.dnsType == "ClusterHosted".
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml`:
- Around line 1265-1286: Add a CEL validation at the parent
cloudLoadBalancerConfig level to enforce that cloudDnsIPs may only be set when
dnsType is ClusterHosted: implement an x-kubernetes-validations rule (adjacent
to the existing validation in cloudLoadBalancerConfig) whose expression requires
cloudDnsIPs to be empty or dnsType to equal "ClusterHosted" and provide a clear
message when violated; apply the same change in the Azure, GCP and OpenStack
cloudLoadBalancerConfig sections so each provider enforces the same constraint
for the cloudDnsIPs field.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml`:
- Around line 1509-1530: Add an x-kubernetes-validations CEL check on the
cloudDnsIPs array to enforce it may only be set when dnsType equals
"ClusterHosted": add an x-kubernetes-validations entry under cloudDnsIPs with a
message like "cloudDnsIPs may only be set when dnsType is ClusterHosted" and a
rule such as object.dnsType == "ClusterHosted" || self == null || size(self) ==
0; reference the cloudDnsIPs field name and dnsType in the rule so attempts to
set cloudDnsIPs when dnsType != "ClusterHosted" fail schema validation.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml`:
- Around line 1265-1286: Add an x-kubernetes-validations rule to the cloudDnsIPs
array to enforce that it is only set when dnsType == "ClusterHosted" (mirror the
existing validation used by the sibling clusterHosted field); specifically, add
a validation entry under cloudDnsIPs that uses a rule like 'self == null or
<dnsType check>'/or the equivalent CEL expression ensuring cloudDnsIPs is empty
unless spec.dnsType == "ClusterHosted", referencing the cloudDnsIPs field name
and dnsType in the rule; apply the same change for the cloudDnsIPs blocks in the
AWS, Azure, and GCP sections so each cloudDnsIPs has the same
x-kubernetes-validations constraint.
- Around line 1891-1912: The GCP cloudDnsIPs field in the
cloudLoadBalancerConfig lacks the IP validation and its description incorrectly
references an AWS VPC example; add the same x-kubernetes-validations block
(rule: isIP(self), message: value must be a valid IP address) under the
cloudDnsIPs items for the cloudLoadBalancerConfig section and update the
cloudDnsIPs description (and the earlier cloudDnsIPs description where AWS is
referenced) to be platform-agnostic or GCP-appropriate (remove the AWS VPC CIDR
example) so the field's description and validation for cloudDnsIPs and dnsType
are consistent across platforms.
- Around line 1492-1513: The Azure section is missing the same IP validation and
has an AWS-specific example; update the cloudLoadBalancerConfig/cloudDnsIPs
entry to include the x-kubernetes-validations rule (use rule: isIP(self) with
message: value must be a valid IP address) and ensure cloudDnsIPs keeps
items/type/format/maxItems/x-kubernetes-list-type consistent with the other
cloudDnsIPs definition (e.g., same items.description, minLength/maxLength, type
string); also edit the cloudDnsIPs description to remove the AWS VPC CIDR
example and make it Azure-appropriate or platform-agnostic so it no longer
references AWS-specific addressing.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 1551-1572: Add a CEL validation for the cloudDnsIPs array to
enforce it's only set when dnsType == "ClusterHosted": in each
cloudLoadBalancerConfig block (the AWS, Azure, and GCP occurrences) add an
x-kubernetes-validations entry for cloudDnsIPs with a message like "cloudDnsIPs
is only valid when dnsType is ClusterHosted" and a rule such as
(self.cloudDnsIPs.empty()) || (self.dnsType == "ClusterHosted") (mirroring the
existing clusterHosted validation logic); target the cloudDnsIPs schema block
(field name cloudDnsIPs) so the CRD rejects non-empty cloudDnsIPs unless dnsType
equals "ClusterHosted".
- Around line 1779-1800: The description for the cloudDnsIPs field contains an
AWS-specific sentence ("For AWS...") in the Azure platform section; update the
description for cloudDnsIPs to either be Azure-specific (e.g., reference the
Azure DNS resolver 168.63.129.16 and how Azure handles DNS) or make it generic
(remove the AWS sentence entirely and state that these are IP addresses for
cloud platform DNS servers used when dnsType is ClusterHosted), keeping the
existing constraints (unique entries, max 16, only valid when dnsType is
ClusterHosted) and ensuring references to dnsType and ClusterHosted remain
consistent with the rest of the CRD.
- Around line 2182-2203: The description for the cloudDnsIPs field contains an
AWS-specific example ("For AWS, the DNS IPs are typically the VPC's CIDR base
address plus 2") which is wrong for the GCP section; update the cloudDnsIPs
description (the multi-line description under the cloudDnsIPs schema block) to
either use a GCP-appropriate note (e.g., reference Cloud DNS or metadata server
behavior such as 169.254.169.254 if relevant) or make the text platform-neutral
(describe that these are cluster DNS server IPs used when dnsType is
ClusterHosted, must be unique, max 16, and must not be set when dnsType is not
ClusterHosted) so the comment no longer references AWS-specific behavior.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml`:
- Around line 1799-1800: The description for the field under
gcp.cloudLoadBalancerConfig includes an AWS-specific example which is confusing;
update the description in the CRD for gcp.cloudLoadBalancerConfig to be
platform-agnostic or replace the AWS example with a GCP-specific example (e.g.,
explain GCP uses reserved VIPs or point to how to compute the DNS IP in GCP),
and ensure the explanatory sentence clearly states it only applies when dnsType
is ClusterHosted while removing or changing the "For AWS, ..." text so it
matches the gcp.cloudLoadBalancerConfig context.
- Around line 1795-1816: Add a CEL x-kubernetes-validations rule to the
cloudDnsIPs schema to forbid it unless dnsType == "ClusterHosted": validate that
when cloudDnsIPs is set/has items then spec.dnsType == "ClusterHosted" (or
equivalently dnsType == 'ClusterHosted' || size(self) == 0). Locate the
cloudDnsIPs array block and add an x-kubernetes-validations entry with a message
like "cloudDnsIPs is only allowed when dnsType is ClusterHosted" and a rule
checking spec.dnsType (or the equivalent path used by the existing clusterHosted
validation) to ensure the field cannot be set for other dnsType values.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml`:
- Around line 1779-1800: The description for the cloudDnsIPs field incorrectly
references AWS-specific behavior; update the description text for the
cloudDnsIPs property (under the Cloud/Azure controller CRD) to remove the AWS
VPC CIDR example and either make it platform-generic (explain these addresses
are the cloud platform DNS servers used when dnsType is ClusterHosted, must be
unique, max 16 entries) or replace with Azure-appropriate guidance (e.g.,
mention Azure-provided DNS like 168.63.129.16) and ensure the rest of the text
still references dnsType, maxItems, and the x-kubernetes-validations constraints
unchanged.
- Around line 2182-2203: The cloudDnsIPs description currently contains an
AWS-specific example ("For AWS, the DNS IPs are typically the VPC's CIDR base
address plus 2") which is a copy-paste error in the GCP section; update the
description for the cloudDnsIPs field (the description under the cloudDnsIPs
array definition, which references dnsType and maxItems) to be platform-generic
or GCP-appropriate (e.g., explain that these are the cloud platform DNS server
IPs used when dnsType is ClusterHosted and remove the AWS VPC example), keeping
validation notes (unique entries, max 16) intact.
- Around line 1551-1572: Add a CEL validation to the cloudDnsIPs field's
x-kubernetes-validations that enforces the documented conditional: cloudDnsIPs
may be set only when dnsType == "ClusterHosted" (i.e., clusterHosted == true)
and must be empty/absent otherwise; mirror the existing pattern used for
clusterHosted validations (same x-kubernetes-validations style used for
AWS/Azure/GCP) so each platform-specific block validates cloudDnsIPs accordingly
and returns a clear message when cloudDnsIPs is set while dnsType is not
ClusterHosted.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml`:
- Around line 1795-1816: Add a CEL validation entry under the cloudDnsIPs schema
(x-kubernetes-validations) that enforces the conditional requirement against
spec.dnsType: ensure the rule allows empty/absent cloudDnsIPs or requires
spec.dnsType == "ClusterHosted" otherwise reject; e.g., a rule like 'size(self)
== 0 || spec.dnsType == "ClusterHosted"' with a message such as "cloudDnsIPs may
only be set when dnsType is ClusterHosted" so the cloudDnsIPs field is rejected
when dnsType is not ClusterHosted.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml`:
- Around line 1551-1572: Add a schema-level validation to reject cloudDnsIPs
unless dnsType equals "ClusterHosted": update the cloudDnsIPs schema (field name
cloudDnsIPs) to include an x-kubernetes-validations rule that checks dnsType
(e.g., rule: self == null || $root.spec.dnsType == "ClusterHosted" or equivalent
depending on CR path) and returns an error message when cloudDnsIPs is set while
dnsType != "ClusterHosted"; apply the same conditional validation to the
duplicate cloudDnsIPs blocks referenced (the other AWS/Azure/GCP
cloudLoadBalancerConfig occurrences) so the schema enforces "cloudDnsIPs only
valid when dnsType=ClusterHosted".
- Around line 1781-1787: The description for the cloudDnsIPs field currently
contains an AWS-specific example (VPC base + 2) which is misleading for
Azure/GCP; update the description text for cloudDnsIPs (and any repeated
occurrences) to remove the AWS example and use neutral wording (e.g., "For some
providers, this may be the platform's primary DNS IP") while preserving the
constraints about uniqueness, maximum of 16, and that the field is only valid
when dnsType is ClusterHosted; ensure you update all other occurrences of the
same description in this CRD (e.g., the other cloudDnsIPs block later in the
file) so Azure/GCP docs no longer reference AWS conventions.
---
Outside diff comments:
In `@config/v1/types_infrastructure.go`:
- Around line 872-913: The CloudLoadBalancerConfig struct documents that
CloudDnsIPs may only be set when DNSType (dnsType) == ClusterHosted but lacks a
CEL validation; add a kubebuilder XValidation rule on CloudDnsIPs to enforce
that condition (e.g.
+kubebuilder:validation:XValidation:rule="has(self.cloudDnsIPs) ? self.dnsType
== 'ClusterHosted' : true",message="cloudDnsIPs is permitted only when dnsType
is ClusterHosted"), add +optional as needed, and remove the TODO comment; update
references to CloudLoadBalancerConfig, CloudDnsIPs, DNSType/dnsType in the diff
so the validator targets the correct field.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml`:
- Around line 1492-1605: The schema currently allows cloudDnsIPs even when
dnsType != ClusterHosted and the cloudDnsIPs description contains an
AWS-specific VPC example in an Azure CRD; add an x-kubernetes-validations entry
on the cloudDnsIPs property to require it only when dnsType == "ClusterHosted"
(rule: has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !self : true or
equivalent using oldSelf pattern consistent with existing dnsType immutability
validation) and update the cloudDnsIPs description to remove or neutralize the
AWS-specific VPC example (make the IP example generic like "10.0.0.2" or remove
provider-specific wording); target the cloudDnsIPs, dnsType, and clusterHosted
symbols and the x-kubernetes-validations block for these changes.
- Around line 1265-1379: Add a schema-level validation on the cloudDnsIPs
property to forbid it unless dnsType == 'ClusterHosted'; update the cloudDnsIPs
block (property name: cloudDnsIPs) to include an x-kubernetes-validations entry
that checks dnsType (symbol: dnsType) and rejects cloudDnsIPs when dnsType is
absent or not 'ClusterHosted' (use a rule like: has(self.dnsType) &&
self.dnsType != 'ClusterHosted' ? !self || size(self) == 0 : true, or equivalent
expression) so the CRD enforces the description text. Ensure the validation
message clearly states that cloudDnsIPs is only permitted when dnsType is
ClusterHosted and add it alongside the existing validations for cloudDnsIPs.
- Around line 1891-2004: Add a validation to ensure cloudDnsIPs is only set when
dnsType == "ClusterHosted" and remove the AWS-specific VPC example from the
cloudDnsIPs description; specifically, update the cloudDnsIPs property (symbol:
cloudDnsIPs) to include an x-kubernetes-validations rule that enforces
"has(self.dnsType) && self.dnsType != 'ClusterHosted' ? !has(self.cloudDnsIPs) :
true" (or equivalent) and update the description text to a neutral example
(remove the AWS VPC-specific line); reference the dnsType and clusterHosted
fields as needed to mirror the existing validation style used for clusterHosted
(x-kubernetes-validations at the same parent object).
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml`:
- Around line 1373-1378: The current x-kubernetes-validations entry enforces
that clusterHosted is only allowed when dnsType == 'ClusterHosted'; extend this
same validation to also restrict cloudDnsIPs by updating the validation rule for
the object under x-kubernetes-validations so it checks both fields
(clusterHosted and cloudDnsIPs) — e.g., modify the rule that currently
references has(self.dnsType) && self.dnsType != 'ClusterHosted' ?
!has(self.clusterHosted) : true to also require !has(self.cloudDnsIPs) when
dnsType is not 'ClusterHosted', keeping the same message/validation mechanism
for the fields clusterHosted and cloudDnsIPs.
---
Nitpick comments:
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml`:
- Line 1799: The description under platformStatus currently mentions an
AWS-specific example ("For AWS, the DNS IPs are typically the VPC's CIDR base
address plus 2"), which can confuse users of the GCP section; update the
description in the platformStatus/GCP entry to either use platform-agnostic
wording (e.g., "Set the DNS IP(s) for the network; consult your cloud provider
or VPC/subnet configuration for the correct address") or add a GCP-specific
example (e.g., explain how to determine the DNS IP for GCP VPCs), and ensure the
field name referenced (platformStatus -> GCP -> dns IPs) is clear so readers
know where to find provider-specific guidance.
Comment on lines
+1509
to
+1530
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Mirror the dnsType gating validation for cloudDnsIPs.
The schema currently enforces this invariant for clusterHosted but not for cloudDnsIPs, even though the description says it must not be set when dnsType isn’t ClusterHosted.
🔧 Suggested validation rule
x-kubernetes-validations:
- message: clusterHosted is permitted only when dnsType is
ClusterHosted
rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
? !has(self.clusterHosted) : true'
+ - message: cloudDnsIPs is permitted only when dnsType is ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
+ ? !has(self.cloudDnsIPs) : 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-Default.crd.yaml`
around lines 1509 - 1530, Add a CEL validation on the cloudDnsIPs property to
enforce the dnsType gating: add an x-kubernetes-validations entry for
cloudDnsIPs with a rule like 'self == null || size(self) == 0 ||
(root.spec.dnsType == "ClusterHosted")' and an appropriate message (e.g.,
"cloudDnsIPs may only be set when dnsType is ClusterHosted"); reference the
cloudDnsIPs field and the dnsType value "ClusterHosted" so the CRD rejects
cloudDnsIPs unless root.spec.dnsType == "ClusterHosted".
Comment on lines
+1265
to
+1286
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Add CEL validation to enforce dnsType constraint for cloudDnsIPs.
The description states "This field is only valid when dnsType is set to ClusterHosted. When dnsType is not ClusterHosted, this field must not be set," but no validation rule enforces this constraint. Without validation, users can set cloudDnsIPs when dnsType is PlatformDefault, creating an invalid configuration.
🛡️ Proposed fix to add validation rule
Add a validation rule at the parent cloudLoadBalancerConfig level (after the existing validation at line 1378):
x-kubernetes-validations:
- message: clusterHosted is permitted only when dnsType is
ClusterHosted
rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
? !has(self.clusterHosted) : true'
+ - message: cloudDnsIPs is permitted only when dnsType is
+ ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
+ ? !has(self.cloudDnsIPs) : true'Note: The same validation must be added for Azure (after line 1605), GCP (after line 2004), and OpenStack sections as well.
🤖 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-DevPreviewNoUpgrade.crd.yaml`
around lines 1265 - 1286, Add a CEL validation at the parent
cloudLoadBalancerConfig level to enforce that cloudDnsIPs may only be set when
dnsType is ClusterHosted: implement an x-kubernetes-validations rule (adjacent
to the existing validation in cloudLoadBalancerConfig) whose expression requires
cloudDnsIPs to be empty or dnsType to equal "ClusterHosted" and provide a clear
message when violated; apply the same change in the Azure, GCP and OpenStack
cloudLoadBalancerConfig sections so each provider enforces the same constraint
for the cloudDnsIPs field.
Comment on lines
+1509
to
+1530
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Add schema validation to enforce the “only when dnsType=ClusterHosted” rule for cloudDnsIPs.
The description says cloudDnsIPs must not be set unless dnsType is ClusterHosted, but there’s no matching x-kubernetes-validations rule (unlike clusterHosted). This allows invalid configs to pass schema validation.
🔧 Suggested validation rule
x-kubernetes-validations:
- message: clusterHosted is permitted only when dnsType is
ClusterHosted
rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
? !has(self.clusterHosted) : true'
+ - message: cloudDnsIPs is permitted only when dnsType is ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
+ ? !has(self.cloudDnsIPs) : 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 1509 - 1530, Add an x-kubernetes-validations CEL check on the
cloudDnsIPs array to enforce it may only be set when dnsType equals
"ClusterHosted": add an x-kubernetes-validations entry under cloudDnsIPs with a
message like "cloudDnsIPs may only be set when dnsType is ClusterHosted" and a
rule such as object.dnsType == "ClusterHosted" || self == null || size(self) ==
0; reference the cloudDnsIPs field name and dnsType in the rule so attempts to
set cloudDnsIPs when dnsType != "ClusterHosted" fail schema validation.
Comment on lines
+1265
to
+1286
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Missing validation rule to enforce cloudDnsIPs is only valid when dnsType is ClusterHosted.
The description states that cloudDnsIPs "is only valid when dnsType is set to ClusterHosted" and "When dnsType is not ClusterHosted, this field must not be set." However, there is no x-kubernetes-validations rule enforcing this constraint.
The sibling field clusterHosted has such a validation rule at line 1375-1378, but cloudDnsIPs lacks an equivalent. This means users could set cloudDnsIPs when dnsType is PlatformDefault, which contradicts the documented behavior.
Proposed fix: Add validation rule to cloudLoadBalancerConfig for AWS
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: cloudDnsIPs is permitted only when dnsType is
+ ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
+ ? !has(self.cloudDnsIPs) : true'The same validation rule should be added for Azure (after line 1605) and GCP (after line 2004) sections as well.
🤖 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-TechPreviewNoUpgrade.crd.yaml`
around lines 1265 - 1286, Add an x-kubernetes-validations rule to the
cloudDnsIPs array to enforce that it is only set when dnsType == "ClusterHosted"
(mirror the existing validation used by the sibling clusterHosted field);
specifically, add a validation entry under cloudDnsIPs that uses a rule like
'self == null or <dnsType check>'/or the equivalent CEL expression ensuring
cloudDnsIPs is empty unless spec.dnsType == "ClusterHosted", referencing the
cloudDnsIPs field name and dnsType in the rule; apply the same change for the
cloudDnsIPs blocks in the AWS, Azure, and GCP sections so each cloudDnsIPs has
the same x-kubernetes-validations constraint.
Comment on lines
+1492
to
+1513
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Same missing validation issue for Azure cloudDnsIPs.
The Azure cloudLoadBalancerConfig section at lines 1601-1605 also lacks the validation rule for cloudDnsIPs. Additionally, the description references AWS VPC CIDR addressing ("For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2"), which is not applicable to Azure.
Proposed fixes
- Add validation rule after line 1605:
x-kubernetes-validations:
- message: clusterHosted is permitted only when dnsType is
ClusterHosted
rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
? !has(self.clusterHosted) : true'
+ - message: cloudDnsIPs is permitted only when dnsType is
+ ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted''
+ ? !has(self.cloudDnsIPs) : true'- Update description to be Azure-specific or platform-agnostic (remove the AWS VPC example from lines 1496-1497).
🤖 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-TechPreviewNoUpgrade.crd.yaml`
around lines 1492 - 1513, The Azure section is missing the same IP validation
and has an AWS-specific example; update the cloudLoadBalancerConfig/cloudDnsIPs
entry to include the x-kubernetes-validations rule (use rule: isIP(self) with
message: value must be a valid IP address) and ensure cloudDnsIPs keeps
items/type/format/maxItems/x-kubernetes-list-type consistent with the other
cloudDnsIPs definition (e.g., same items.description, minLength/maxLength, type
string); also edit the cloudDnsIPs description to remove the AWS VPC CIDR
example and make it Azure-appropriate or platform-agnostic so it no longer
references AWS-specific addressing.
Comment on lines
+1779
to
+1800
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Copy-paste error: Azure description incorrectly references AWS VPC.
The description states "For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2" but this is in the Azure section. Azure has different DNS semantics (e.g., Azure-provided DNS at 168.63.129.16).
Consider updating the description to be platform-generic or Azure-specific.
🤖 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-DevPreviewNoUpgrade.crd.yaml`
around lines 1779 - 1800, The description for the cloudDnsIPs field incorrectly
references AWS-specific behavior; update the description text for the
cloudDnsIPs property (under the Cloud/Azure controller CRD) to remove the AWS
VPC CIDR example and either make it platform-generic (explain these addresses
are the cloud platform DNS servers used when dnsType is ClusterHosted, must be
unique, max 16 entries) or replace with Azure-appropriate guidance (e.g.,
mention Azure-provided DNS like 168.63.129.16) and ensure the rest of the text
still references dnsType, maxItems, and the x-kubernetes-validations constraints
unchanged.
Comment on lines
+2182
to
+2203
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Copy-paste error: GCP description incorrectly references AWS VPC.
Same issue as Azure: the description mentions "For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2" but this is in the GCP section. GCP has different DNS infrastructure (e.g., metadata server or Cloud DNS).
Consider updating the description to be platform-generic or GCP-specific.
🤖 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-DevPreviewNoUpgrade.crd.yaml`
around lines 2182 - 2203, The cloudDnsIPs description currently contains an
AWS-specific example ("For AWS, the DNS IPs are typically the VPC's CIDR base
address plus 2") which is a copy-paste error in the GCP section; update the
description for the cloudDnsIPs field (the description under the cloudDnsIPs
array definition, which references dnsType and maxItems) to be platform-generic
or GCP-appropriate (e.g., explain that these are the cloud platform DNS server
IPs used when dnsType is ClusterHosted and remove the AWS VPC example), keeping
validation notes (unique entries, max 16) intact.
Comment on lines
+1795
to
+1816
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Missing CEL validation to enforce cloudDnsIPs conditional requirement.
The description states that cloudDnsIPs must not be set when dnsType is not ClusterHosted (lines 1800-1801), but there's no CEL validation rule to enforce this constraint. The sibling field clusterHosted has proper validation at lines 1905-1908, but cloudDnsIPs lacks the equivalent rule.
Without this validation, the API will accept invalid configurations where cloudDnsIPs is set with dnsType: PlatformDefault.
Proposed fix: Add validation rule for cloudDnsIPs
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: cloudDnsIPs is permitted only when dnsType is ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' ? !has(self.cloudDnsIPs) : true'🤖 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 1795 - 1816, Add a CEL validation entry under the cloudDnsIPs
schema (x-kubernetes-validations) that enforces the conditional requirement
against spec.dnsType: ensure the rule allows empty/absent cloudDnsIPs or
requires spec.dnsType == "ClusterHosted" otherwise reject; e.g., a rule like
'size(self) == 0 || spec.dnsType == "ClusterHosted"' with a message such as
"cloudDnsIPs may only be set when dnsType is ClusterHosted" so the cloudDnsIPs
field is rejected when dnsType is not ClusterHosted.
Comment on lines
+1551
to
+1572
| cloudDnsIPs: | ||
| description: |- | ||
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs 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 |
There was a problem hiding this comment.
Add schema validation to forbid cloudDnsIPs unless dnsType=ClusterHosted.
The description says the field is invalid when dnsType is not ClusterHosted, but the schema doesn’t enforce this (only clusterHosted is guarded). This allows unsupported configs to be accepted.
🔧 Suggested validation (apply to AWS/Azure/GCP cloudLoadBalancerConfig)
x-kubernetes-validations:
- message: clusterHosted is permitted only when dnsType is ClusterHosted
rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' ? !has(self.clusterHosted) : true'
+ - message: cloudDnsIPs is permitted only when dnsType is ClusterHosted
+ rule: 'has(self.dnsType) && self.dnsType != ''ClusterHosted'' ? !has(self.cloudDnsIPs) : true'Also applies to: 1779-1800, 2182-2203
🤖 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-TechPreviewNoUpgrade.crd.yaml`
around lines 1551 - 1572, Add a schema-level validation to reject cloudDnsIPs
unless dnsType equals "ClusterHosted": update the cloudDnsIPs schema (field name
cloudDnsIPs) to include an x-kubernetes-validations rule that checks dnsType
(e.g., rule: self == null || $root.spec.dnsType == "ClusterHosted" or equivalent
depending on CR path) and returns an error message when cloudDnsIPs is set while
dnsType != "ClusterHosted"; apply the same conditional validation to the
duplicate cloudDnsIPs blocks referenced (the other AWS/Azure/GCP
cloudLoadBalancerConfig occurrences) so the schema enforces "cloudDnsIPs only
valid when dnsType=ClusterHosted".
Comment on lines
+1781
to
+1787
| cloudDnsIPs holds the IP addresses of the cloud platform DNS servers. | ||
| These DNS server IP addresses are used when the cluster DNS type is ClusterHosted. | ||
| For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16). | ||
| This field is only valid when dnsType is set to ClusterHosted. | ||
| When dnsType is not ClusterHosted, this field must not be set. | ||
| Entries in cloudDnsIPs must be unique. | ||
| A maximum of 16 IP addresses are permitted. |
There was a problem hiding this comment.
Remove AWS-specific example from Azure/GCP descriptions.
The Azure and GCP schema text references the AWS VPC DNS IP convention, which is misleading for those providers.
✏️ Suggested wording cleanup (apply to Azure/GCP sections)
- For AWS, the DNS IPs are typically the VPC's CIDR base address plus 2 (e.g., 10.0.0.2 for VPC 10.0.0.0/16).Also applies to: 2184-2190
🤖 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-TechPreviewNoUpgrade.crd.yaml`
around lines 1781 - 1787, The description for the cloudDnsIPs field currently
contains an AWS-specific example (VPC base + 2) which is misleading for
Azure/GCP; update the description text for cloudDnsIPs (and any repeated
occurrences) to remove the AWS example and use neutral wording (e.g., "For some
providers, this may be the platform's primary DNS IP") while preserving the
constraints about uniqueness, maximum of 16, and that the field is only valid
when dnsType is ClusterHosted; ensure you update all other occurrences of the
same description in this CRD (e.g., the other cloudDnsIPs block later in the
file) so Azure/GCP docs no longer reference AWS conventions.
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CloudDNSIPs hold the upstream DNS server IPs. These will be used to configure the in-cluster CoreDNS instance when DNSType is set to ClusterHostedDNS.