Treat x-validations as a map when merging for manifest merge#2896
Treat x-validations as a map when merging for manifest merge#2896JoelSpeed wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe pull request updates 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
payload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yaml (1)
76-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTechPreview DNS CRD still contains conflicting partition constraints for
privateZoneIAMRole.Line 77 still lists
aws-eusc; Line 84 excludes it; Line 87 keeps the old allow-list. This should be normalized to one rule plus matching docs.Suggested fix
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, or aws-us-gov), @@ x-kubernetes-validations: - message: 'privateZoneIAMRole must be a valid AWS IAM role ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.*$') - - message: 'privateZoneIAMRole must be a valid AWS IAM role - ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' - rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role/.*$')As per coding guidelines, validation constraints documented in comments must match enforced validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yaml` around lines 76 - 87, The privateZoneIAMRole field has duplicate and conflicting validation rules in the x-kubernetes-validations section where one rule allows aws-eusc partition and the other excludes it, and the description also lists aws-eusc inconsistently. Remove one of the two duplicate validation rules (either the rule matching aws|aws-cn|aws-us-gov or the rule matching aws|aws-cn|aws-us-gov|aws-eusc) to eliminate the conflict, then update the description text to match the partition list in the remaining validation rule to ensure consistency between documentation and enforced constraints.Source: Coding guidelines
payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yaml (1)
129-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDevPreview ClusterCSIDriver CRD still carries conflicting
kmsKeyARNpartition constraints.Line 130 includes
aws-eusc, Line 138 excludes it, and Line 141 keeps the previous allow-list. Consolidate to one regex and update docs to the exact supported partitions.Suggested fix
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, aws-iso-f, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, or aws-iso-f), @@ x-kubernetes-validations: - message: 'kmsKeyARN must be a valid AWS KMS key ARN in the format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>' rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$') - - message: 'kmsKeyARN must be a valid AWS KMS key ARN in the - format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>' - rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f|aws-eusc):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$')As per coding guidelines, validation constraints documented in comments must match enforced validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yaml` around lines 129 - 141, The kmsKeyARN field has duplicate and conflicting x-kubernetes-validations rules in the CRD specification. One validation rule on line 138 excludes the aws-eusc partition while another on line 141 includes it, creating a contradiction. Consolidate these two validation rules into a single x-kubernetes-validations entry that includes all the correct AWS partitions (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, aws-iso-f, and aws-eusc), remove the duplicate conflicting rule, and update the description text at the beginning of the kmsKeyARN field to accurately reflect the exact list of supported AWS partitions to ensure consistency between the documentation and the actual validation constraint.Source: Coding guidelines
payload-manifests/crds/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yaml (1)
76-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
privateZoneIAMRolevalidation is internally inconsistent and still carries staleaws-eusclogic.Line 77 advertises
aws-eusc, but Line 84 rejects it; Line 87 keeps an old permissive rule. Please keep a single validator and update the field description accordingly.Suggested fix
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, or aws-us-gov), @@ x-kubernetes-validations: - message: 'privateZoneIAMRole must be a valid AWS IAM role ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.*$') - - message: 'privateZoneIAMRole must be a valid AWS IAM role - ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' - rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role/.*$')As per coding guidelines, validation constraints documented in comments must match enforced validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yaml` around lines 76 - 87, The privateZoneIAMRole field has conflicting validation rules in the x-kubernetes-validations list: the first rule on line 84 excludes aws-eusc from the allowed partitions while the second rule on line 87 includes it, yet the field description on line 76 advertises aws-eusc as a valid partition. Remove the duplicate validation rule and keep only the one that correctly includes aws-eusc (the second rule), ensuring the description and the remaining validation rule are consistent with each other.Source: Coding guidelines
payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml (1)
129-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
kmsKeyARNpartition docs/rules are out of sync, and the oldaws-euscrule is still present.Line 130 documents
aws-eusc, but Line 138 excludes it; Line 141 still keeps a legacy permissive rule. Please keep only one validator and align the description.Suggested fix
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, aws-iso-f, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, or aws-iso-f), @@ x-kubernetes-validations: - message: 'kmsKeyARN must be a valid AWS KMS key ARN in the format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>' rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$') - - message: 'kmsKeyARN must be a valid AWS KMS key ARN in the - format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>' - rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f|aws-eusc):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$')As per coding guidelines, validation constraints documented in comments must match enforced validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml` around lines 129 - 141, The kmsKeyARN field has two duplicate x-kubernetes-validations rules with conflicting regex patterns - the first rule excludes aws-eusc partition while the second includes it, creating inconsistency with the documentation that lists aws-eusc. Remove the first validation rule (the one matching the pattern without aws-eusc) and keep only the second validation rule that includes the aws-eusc partition. Ensure the description of the field at the beginning of the spec matches the partitions listed in the kept validation rule.Source: Coding guidelines
payload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yaml (1)
76-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
privateZoneIAMRoledocs and CEL rules; remove the staleaws-euscvalidator.Line 77 still documents
aws-eusc, while Line 84 removes it and Line 87 still re-allows it in a second rule. Keep one canonical rule and update docs to match the enforced partition set.Suggested fix
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, or aws-us-gov), @@ x-kubernetes-validations: - message: 'privateZoneIAMRole must be a valid AWS IAM role ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.*$') - - message: 'privateZoneIAMRole must be a valid AWS IAM role - ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' - rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role/.*$')As per coding guidelines, validation constraints documented in comments must match enforced validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yaml` around lines 76 - 87, The documentation for the privateZoneIAMRole field mentions aws-eusc as a valid partition, but there are two conflicting CEL validation rules: the first rule on line 84 excludes aws-eusc while the second rule on line 87 includes it. Remove aws-eusc from the documentation comment describing the partition values, and remove the duplicate validation rule on line 87 entirely, keeping only the first validation rule that correctly excludes aws-eusc. Ensure the documentation and the single remaining CEL rule in the x-kubernetes-validations section are fully aligned on which partitions are actually supported.Source: Coding guidelines
payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml (1)
129-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTechPreview
kmsKeyARNvalidation needs cleanup: staleaws-euscdoc + legacy rule remain.Line 130 still advertises
aws-eusc, while Line 138 removes it and Line 141 still keeps the old matcher. Please remove the legacy rule and make docs/rules consistent.Suggested fix
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, aws-iso-f, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, or aws-iso-f), @@ x-kubernetes-validations: - message: 'kmsKeyARN must be a valid AWS KMS key ARN in the format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>' rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$') - - message: 'kmsKeyARN must be a valid AWS KMS key ARN in the - format: arn:<partition>:kms:<region>:<account-id>:(key|alias)/<key-id-or-alias>' - rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f|aws-eusc):kms:[a-z0-9-]+:[0-9]{12}:(key|alias)/.*$')As per coding guidelines, validation constraints documented in comments must match enforced validation rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml` around lines 129 - 141, Remove the duplicate and outdated validation rule that lacks aws-eusc support. Delete the first x-kubernetes-validations rule block (lines 137-141) that contains the regex pattern without aws-eusc: '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:...' Keep only the second x-kubernetes-validations rule block (lines 142-144) that includes aws-eusc in its regex pattern: '^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f|aws-eusc):kms:...' This ensures the documentation comment mentioning aws-eusc on line 130 matches the actual enforced validation rules.Source: Coding guidelines
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (1)
132-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumentation does not match the validation rule.
The description at lines 132-133 lists
aws-euscas a valid partition value, but the new validation rule at line 140 only acceptsaws,aws-cn, andaws-us-gov. This creates user-facing confusion where the documentation promises support that the validation rejects.Additionally, there are now two validation rules for
privateZoneIAMRole:
- Lines 138-140: New stricter rule (excludes
aws-eusc)- Lines 141-143: Existing permissive rule (includes
aws-eusc)Since
x-kubernetes-validationsrules use AND semantics (both must pass), the stricter rule effectively blocksaws-eusc, making the old permissive rule redundant. This appears to be a result of the manifest merge producing both rules, but the documentation should be updated to reflect the actual allowed values.📝 Suggested documentation fix
The description at line 133 should be updated to match the stricter validation:
- <partition> is the AWS partition (aws, aws-cn, aws-us-gov, or aws-eusc), + <partition> is the AWS partition (aws, aws-cn, or aws-us-gov),Note: This change should be made in the source
types.gofile, then regenerated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml` around lines 132 - 143, The documentation for the privateZoneIAMRole field lists aws-eusc as a valid AWS partition, but the stricter validation rule only accepts aws, aws-cn, and aws-us-gov. Since both validation rules use AND semantics, the stricter rule effectively blocks aws-eusc. Update the description documentation for privateZoneIAMRole in the source types.go file to remove aws-eusc from the list of valid partitions and state only aws, aws-cn, and aws-us-gov as supported, ensuring consistency with the actual validation constraints. Then regenerate the CRD manifest files from the updated source so the documentation in the generated YAML reflects the true allowed values.payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (1)
1379-1393:⚠️ Potential issue | 🟠 MajorConflicting vcenters validation rules in DevPreviewNoUpgrade variant.
The vcenters field has duplicate and conflicting validation rules in the DevPreviewNoUpgrade CRD (and CustomNoUpgrade/TechPreviewNoUpgrade variants):
- New restrictive rule (lines 1379-1382): "vcenters cannot be added or removed once set" — blocks any size changes after initial population
- Old permissive rules (lines 1383-1390): "Cannot add and remove vCenters at the same time" (two instances) — allow adding or removing vcenters individually if all old/new servers match
These rules conflict logically: the new rule prohibits size changes, while the old rules permit them under specific conditions. Since all validation rules must pass, the effective behavior is overly restrictive.
The Default and OKD variants do not have this issue—they only include the new restrictive rule plus the uniqueness check. The inconsistency suggests the old rules should have been removed when the new rule was added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml` around lines 1379 - 1393, Remove the two conflicting validation rules with the message "Cannot add and remove vCenters at the same time" from the vcenters field validation rules in the DevPreviewNoUpgrade CRD variant (and apply the same fix to CustomNoUpgrade and TechPreviewNoUpgrade variants if they have the same issue). Keep only the new restrictive rule that states "vcenters cannot be added or removed once set" along with the uniqueness check "vcenters must have unique server values", making these variants consistent with the Default and OKD variants which only have these two rules without the conflicting permissive rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml`:
- Around line 1087-1089: The validation rule for vcenters at lines 1087-1089
creates a conflict between three parts of the schema: the description stating
TechPreview allows adding/removing vcenters (lines 1035-1038), the new
restrictive rule that only permits 0 to 1 vcenter transitions, and the
VSphereMultiVCenterDay2-gated rules (lines 1090-1095) designed to constrain how
vcenters can be modified. Choose one approach to resolve this: either update the
description to accurately reflect the stricter immutability behavior after the
first vcenter is set, or refactor the validation rule to be feature-gate-aware
so it only applies when VSphereMultiVCenterDay2 is disabled, allowing the gated
rules to handle size changes with their own constraints, or revise the rule
logic to permit size changes that are constrained by the downstream gated rules
rather than blocking all changes post-initialization.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml`:
- Around line 137-143: The privateZoneIAMRole field has duplicate
x-kubernetes-validations rules with identical messages but different regex
patterns in six CRD files. Remove the second validation rule (the one containing
`aws-eusc` in the partition pattern) from all affected files to eliminate
redundancy and match the single-rule pattern used in the stable variants. The
files requiring this fix are the CustomNoUpgrade, DevPreviewNoUpgrade, and
TechPreviewNoUpgrade variants of both the DNS and ControllerConfig CRDs, where
you should keep only the stricter rule that excludes `aws-eusc` from the
partition list.
---
Outside diff comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yaml`:
- Around line 76-87: The documentation for the privateZoneIAMRole field mentions
aws-eusc as a valid partition, but there are two conflicting CEL validation
rules: the first rule on line 84 excludes aws-eusc while the second rule on line
87 includes it. Remove aws-eusc from the documentation comment describing the
partition values, and remove the duplicate validation rule on line 87 entirely,
keeping only the first validation rule that correctly excludes aws-eusc. Ensure
the documentation and the single remaining CEL rule in the
x-kubernetes-validations section are fully aligned on which partitions are
actually supported.
In
`@payload-manifests/crds/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yaml`:
- Around line 76-87: The privateZoneIAMRole field has conflicting validation
rules in the x-kubernetes-validations list: the first rule on line 84 excludes
aws-eusc from the allowed partitions while the second rule on line 87 includes
it, yet the field description on line 76 advertises aws-eusc as a valid
partition. Remove the duplicate validation rule and keep only the one that
correctly includes aws-eusc (the second rule), ensuring the description and the
remaining validation rule are consistent with each other.
In
`@payload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yaml`:
- Around line 76-87: The privateZoneIAMRole field has duplicate and conflicting
validation rules in the x-kubernetes-validations section where one rule allows
aws-eusc partition and the other excludes it, and the description also lists
aws-eusc inconsistently. Remove one of the two duplicate validation rules
(either the rule matching aws|aws-cn|aws-us-gov or the rule matching
aws|aws-cn|aws-us-gov|aws-eusc) to eliminate the conflict, then update the
description text to match the partition list in the remaining validation rule to
ensure consistency between documentation and enforced constraints.
In
`@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml`:
- Around line 129-141: The kmsKeyARN field has two duplicate
x-kubernetes-validations rules with conflicting regex patterns - the first rule
excludes aws-eusc partition while the second includes it, creating inconsistency
with the documentation that lists aws-eusc. Remove the first validation rule
(the one matching the pattern without aws-eusc) and keep only the second
validation rule that includes the aws-eusc partition. Ensure the description of
the field at the beginning of the spec matches the partitions listed in the kept
validation rule.
In
`@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yaml`:
- Around line 129-141: The kmsKeyARN field has duplicate and conflicting
x-kubernetes-validations rules in the CRD specification. One validation rule on
line 138 excludes the aws-eusc partition while another on line 141 includes it,
creating a contradiction. Consolidate these two validation rules into a single
x-kubernetes-validations entry that includes all the correct AWS partitions
(aws, aws-cn, aws-us-gov, aws-iso, aws-iso-b, aws-iso-e, aws-iso-f, and
aws-eusc), remove the duplicate conflicting rule, and update the description
text at the beginning of the kmsKeyARN field to accurately reflect the exact
list of supported AWS partitions to ensure consistency between the documentation
and the actual validation constraint.
In
`@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yaml`:
- Around line 129-141: Remove the duplicate and outdated validation rule that
lacks aws-eusc support. Delete the first x-kubernetes-validations rule block
(lines 137-141) that contains the regex pattern without aws-eusc:
'^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f):kms:...'
Keep only the second x-kubernetes-validations rule block (lines 142-144) that
includes aws-eusc in its regex pattern:
'^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b|aws-iso-e|aws-iso-f|aws-eusc):kms:...'
This ensures the documentation comment mentioning aws-eusc on line 130 matches
the actual enforced validation rules.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 132-143: The documentation for the privateZoneIAMRole field lists
aws-eusc as a valid AWS partition, but the stricter validation rule only accepts
aws, aws-cn, and aws-us-gov. Since both validation rules use AND semantics, the
stricter rule effectively blocks aws-eusc. Update the description documentation
for privateZoneIAMRole in the source types.go file to remove aws-eusc from the
list of valid partitions and state only aws, aws-cn, and aws-us-gov as
supported, ensuring consistency with the actual validation constraints. Then
regenerate the CRD manifest files from the updated source so the documentation
in the generated YAML reflects the true allowed values.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml`:
- Around line 1379-1393: Remove the two conflicting validation rules with the
message "Cannot add and remove vCenters at the same time" from the vcenters
field validation rules in the DevPreviewNoUpgrade CRD variant (and apply the
same fix to CustomNoUpgrade and TechPreviewNoUpgrade variants if they have the
same issue). Keep only the new restrictive rule that states "vcenters cannot be
added or removed once set" along with the uniqueness check "vcenters must have
unique server values", making these variants consistent with the Default and OKD
variants which only have these two rules without the conflicting permissive
rules.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 26ffff9c-d1af-4604-9ae6-7e41a0d47d37
⛔ Files ignored due to path filters (16)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*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-DevPreviewNoUpgrade.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/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.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-DevPreviewNoUpgrade.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/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*
📒 Files selected for processing (17)
payload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamltools/codegen/pkg/manifestmerge/crd-schema.json
| - message: vcenters cannot be added or removed once set | ||
| rule: 'size(self) != size(oldSelf) ? size(oldSelf) == 0 | ||
| && size(self) < 2 : true' |
There was a problem hiding this comment.
Validation rule conflicts with TechPreview description and feature-gated rules.
The new validation rule forbids changing the vcenters list size except during initial population (size 0 to <2). However, this conflicts with:
-
Description at lines 1035-1038: States "in TechPreview you are able to add and remove vCenters but may not remove all vCenters." This promises flexibility that the rule prevents.
-
VSphereMultiVCenterDay2-gated rules (lines 1090-1095): These rules constrain how vcenters are added/removed (preventing simultaneous add/remove), but the new rule prevents size changes entirely, making the gated rules largely ineffective.
The rule logic size(oldSelf) == 0 && size(self) < 2 allows only the transition from 0 to 1 vcenter (given minItems: 1). This blocks subsequent additions (1→2, 1→3) and removals (2→1, 3→2) that the description and gated rules appear designed to support.
Either:
- The description should be updated to reflect the new stricter behavior, or
- The rule should be feature-gate-aware and not apply when
VSphereMultiVCenterDay2is enabled, or - The rule logic should be revised to allow size changes constrained by the gated rules
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml`
around lines 1087 - 1089, The validation rule for vcenters at lines 1087-1089
creates a conflict between three parts of the schema: the description stating
TechPreview allows adding/removing vcenters (lines 1035-1038), the new
restrictive rule that only permits 0 to 1 vcenter transitions, and the
VSphereMultiVCenterDay2-gated rules (lines 1090-1095) designed to constrain how
vcenters can be modified. Choose one approach to resolve this: either update the
description to accurately reflect the stricter immutability behavior after the
first vcenter is set, or refactor the validation rule to be feature-gate-aware
so it only applies when VSphereMultiVCenterDay2 is disabled, allowing the gated
rules to handle size changes with their own constraints, or revise the rule
logic to permit size changes that are constrained by the downstream gated rules
rather than blocking all changes post-initialization.
| x-kubernetes-validations: | ||
| - message: 'privateZoneIAMRole must be a valid AWS | ||
| IAM role ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' | ||
| rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.*$') | ||
| - message: 'privateZoneIAMRole must be a valid AWS | ||
| IAM role ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' | ||
| rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role/.*$') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for duplicate privateZoneIAMRole validations across DNS and ControllerConfig CRDs
echo "=== Checking DNS CRDs ==="
for variant in CustomNoUpgrade DevPreviewNoUpgrade TechPreviewNoUpgrade; do
echo "Variant: $variant"
rg -A 2 'privateZoneIAMRole.*x-kubernetes-validations' \
"payload-manifests/crds/0000_10_config-operator_01_dnses-${variant}.crd.yaml" | \
rg 'rule.*matches.*arn'
done
echo -e "\n=== Checking ControllerConfig CRDs ==="
for variant in CustomNoUpgrade DevPreviewNoUpgrade TechPreviewNoUpgrade; do
echo "Variant: $variant"
rg -A 2 'privateZoneIAMRole.*x-kubernetes-validations' \
"payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-${variant}.crd.yaml" | \
rg 'rule.*matches.*arn'
doneRepository: openshift/api
Length of output: 291
🏁 Script executed:
#!/bin/bash
# First, check if the file exists
echo "=== Checking file existence ==="
ls -la "payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml" 2>&1 | head -5
# Check content around lines 137-143
echo -e "\n=== Content at lines 137-143 ==="
sed -n '137,143p' "payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml" 2>&1
# Search for privateZoneIAMRole field across all CRD files
echo -e "\n=== Searching for privateZoneIAMRole in CRD files ==="
find payload-manifests/crds -name "*.crd.yaml" -exec grep -l "privateZoneIAMRole" {} \; 2>&1
# Check context around privateZoneIAMRole in ControllerConfig CRDs
echo -e "\n=== Content around privateZoneIAMRole in ControllerConfig ==="
grep -n "privateZoneIAMRole" "payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml" 2>&1 | head -5
# Check if there are x-kubernetes-validations under privateZoneIAMRole
echo -e "\n=== Checking validation rules structure ==="
grep -A 10 "privateZoneIAMRole:" "payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml" 2>&1 | head -20Repository: openshift/api
Length of output: 3184
🏁 Script executed:
#!/bin/bash
# Check all ControllerConfig variants for the privateZoneIAMRole validation rules
echo "=== ControllerConfig variants - privateZoneIAMRole validation rules ==="
for variant in Default OKD CustomNoUpgrade DevPreviewNoUpgrade TechPreviewNoUpgrade; do
echo -e "\n--- Variant: $variant ---"
file="payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-${variant}.crd.yaml"
if [ -f "$file" ]; then
# Find line number of privateZoneIAMRole and show context
line_num=$(grep -n "privateZoneIAMRole:" "$file" | head -1 | cut -d: -f1)
if [ -n "$line_num" ]; then
# Show the validation rules (about 20 lines after the field name)
sed -n "${line_num},$((line_num+25))p" "$file" | grep -E "(rule:|message:)" | head -10
fi
fi
done
# Also check DNS variants for comparison
echo -e "\n\n=== DNS variants - privateZoneIAMRole validation rules ==="
for variant in Default OKD CustomNoUpgrade DevPreviewNoUpgrade TechPreviewNoUpgrade; do
echo -e "\n--- Variant: $variant ---"
file="payload-manifests/crds/0000_10_config-operator_01_dnses-${variant}.crd.yaml"
if [ -f "$file" ]; then
# Find line number of privateZoneIAMRole and show context
line_num=$(grep -n "privateZoneIAMRole:" "$file" | head -1 | cut -d: -f1)
if [ -n "$line_num" ]; then
# Show the validation rules (about 20 lines after the field name)
sed -n "${line_num},$((line_num+25))p" "$file" | grep -E "(rule:|message:)" | head -10
fi
fi
doneRepository: openshift/api
Length of output: 3594
Remove duplicate x-kubernetes-validations rules for privateZoneIAMRole across multiple CRD variants.
Two validation rules are defined for the same field with identical messages but different regex patterns. While CEL AND-logic means the stricter rule (excluding aws-eusc) takes effect, having both rules is redundant and confusing.
This issue affects three variants across both DNS and ControllerConfig CRDs:
CustomNoUpgrade(2 files)DevPreviewNoUpgrade(2 files)TechPreviewNoUpgrade(2 files)
The stable variants (Default and OKD) correctly contain only a single rule excluding aws-eusc. All affected variants should match this pattern by removing the second rule that includes aws-eusc.
♻️ Affected files and fix location
Each file has duplicate rules at slightly different line numbers:
payload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
Remove the second validation rule (the one including aws-eusc) from each file to match the single-rule pattern in stable variants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml`
around lines 137 - 143, The privateZoneIAMRole field has duplicate
x-kubernetes-validations rules with identical messages but different regex
patterns in six CRD files. Remove the second validation rule (the one containing
`aws-eusc` in the partition pattern) from all affected files to eliminate
redundancy and match the single-rule pattern used in the stable variants. The
files requiring this fix are the CustomNoUpgrade, DevPreviewNoUpgrade, and
TechPreviewNoUpgrade variants of both the DNS and ControllerConfig CRDs, where
you should keep only the stricter rule that excludes `aws-eusc` from the
partition list.
|
@JoelSpeed: 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. |
| - message: 'privateZoneIAMRole must be a valid AWS IAM role | ||
| ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' | ||
| rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.*$') | ||
| - message: 'privateZoneIAMRole must be a valid AWS IAM role | ||
| ARN in the format: arn:<partition>:iam::<account-id>:role/<role-name>' | ||
| rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role/.*$') |
There was a problem hiding this comment.
AFAIK, the intention is that aws-eusc is only allowed behind gate. Thus, the partition regex is defined based on whether AWSEuropeanSovereignCloudInstall feature gate is enabled 👇
Lines 143 to 146 in 5346161
That is the 2 validation entries should be mutually exclusive (same KMS key ARN). I remember we discussed it in #2708 (comment) and #2770.
Any ideas how to achieve the same thing with the new tooling change here 🤔?
2 participants
When gated manifests contain different XValidations, because the list is currently treated as atomic, last write wins. This means we lose some XValidations during the merge. The first commit here switches the manifest merge to treat
x-kubernetes-validationsas a map so that we can merge multiple different opinions together.CC @miyadav This should fix your issue with capabilities