CORS-4337: allow AWS Europe Sovereign Cloud partition#2708
CORS-4337: allow AWS Europe Sovereign Cloud partition#2708tthvo wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@tthvo: This pull request references CORS-4337 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @tthvo! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
📝 WalkthroughWalkthroughThis pull request expands AWS ARN validation patterns across multiple configuration and CSI driver resources to support an additional AWS partition, 🚥 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)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
Review Summary by QodoSupport AWS Europe Sovereign Cloud partition in ARN validation
WalkthroughsDescription• Add support for AWS Europe Sovereign Cloud partition (aws-eusc) • Update ARN validation patterns for DNS and KMS configurations • Add comprehensive test cases for all AWS partitions • Update documentation and generated manifests Diagramflowchart LR
A["AWS Partitions<br/>aws, aws-cn, aws-us-gov, aws-eusc"] -->|Update Validation| B["DNS IAM Role ARN"]
A -->|Update Validation| C["KMS Key ARN"]
B -->|Pattern Match| D["^arn:partition:iam::account:role/.*$"]
C -->|Pattern Match| E["^arn:partition:kms:region:account:key/id$"]
B -->|Test Cases| F["All Partitions Validated"]
C -->|Test Cases| F
File Changes1. config/v1/types_dns.go
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/v1/types_kmsencryption.go`:
- Around line 26-36: Update the XValidation message for keyARN to accurately
describe allowed characters: reference the keyARN field and its
+kubebuilder:validation:XValidation rule and change the message text to state
that the region may contain lowercase letters, digits and hyphens and that the
key ID must be lowercase hexadecimal characters and hyphens; ensure the new
message keeps the format example
`arn:<partition>:kms:<region>:<account_id>:key/<key_id>` and mentions the
account ID must be 12 digits and the region is lowercase letters/digits/hyphens
while the key ID is lowercase hex and hyphens.
Code Review by Qodo
1. PrivateZoneIAMRole pattern undocumented
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall, this change seems reasonable.
Only question - is every component that relies on the input provided in these fields able to successfully handle the newly introduced partitions?
|
/hold
Ah sorry, I am the middle of checking these scenarios. So, I am holding this PR for now... Background: AWS Europe Sovereign Cloud (EUSC) requires AWS SDK v2 for out-of-the-box support (i.e. able to resolve the correct endpoint for EUSC regions). However, several cluster operators still use AWS SDK v1 (now EOL), and migrating them to SDK v2 is not feasible within the 4.22 timeline or near future. As a workaround, we're enabling EUSC support through user-provided service endpoints. I'm currently testing whether the new EUSC region honors these custom endpoints, or if only minor patches are needed to make it work. |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
operator/v1/tests/clustercsidrivers.operator.openshift.io/AAA_ungated.yaml (1)
134-156: Consider adding positive test coverage for the remaining ISO-variant partitions.The validation regex includes
aws-iso-b,aws-iso-e, andaws-iso-f, but onlyaws-isohas a positive test case. Since this PR is already adding broad partition coverage, it would be a natural opportunity to close this gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/clustercsidrivers.operator.openshift.io/AAA_ungated.yaml` around lines 134 - 156, Add positive test cases alongside the existing "Should be able to specify an AWS ISO KMS key ARN" case to cover the other ISO partitions: aws-iso-b, aws-iso-e, and aws-iso-f. For each new test, copy the existing initial/expected blocks (preserving apiVersion/kind/metadata/spec structure) and change the kmsKeyARN host partition portion to use arn:aws-iso-b:kms:..., arn:aws-iso-e:kms:..., and arn:aws-iso-f:kms:... respectively so the validation regex is exercised for each partition variant (match the format used in the original kmsKeyARN string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1/tests/clustercsidrivers.operator.openshift.io/AAA_ungated.yaml`:
- Around line 180-191: The negative test uses an ARN that matches the CRD regex,
so update the initial test case for "Should not be able to specify invalid AWS
KMS key ARN" by replacing spec.driverConfig.aws.kmsKeyARN with a deliberately
malformed ARN that fails the pattern—e.g. use an invalid partition name (not
matching arn:(aws|aws-cn|...)) or an account ID with incorrect digit count (not
12 digits); ensure you edit the initial block for the test case that sets
spec.driverConfig.aws.kmsKeyARN so the value no longer matches the regex and the
expectedError for spec.driverConfig.aws.kmsKeyARN triggers.
---
Nitpick comments:
In `@operator/v1/tests/clustercsidrivers.operator.openshift.io/AAA_ungated.yaml`:
- Around line 134-156: Add positive test cases alongside the existing "Should be
able to specify an AWS ISO KMS key ARN" case to cover the other ISO partitions:
aws-iso-b, aws-iso-e, and aws-iso-f. For each new test, copy the existing
initial/expected blocks (preserving apiVersion/kind/metadata/spec structure) and
change the kmsKeyARN host partition portion to use arn:aws-iso-b:kms:...,
arn:aws-iso-e:kms:..., and arn:aws-iso-f:kms:... respectively so the validation
regex is exercised for each partition variant (match the format used in the
original kmsKeyARN string).
operator/v1/tests/clustercsidrivers.operator.openshift.io/AAA_ungated.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_dnses.crd.yaml (1)
74-77: LGTM —aws-euscpartition and updated description are correct.The AWS European Sovereign Cloud is accessed using partition name
aws-eusc, so the regex addition is accurate. The documentation on lines 74–76 precisely maps to the updated pattern.One concern worth tracking (which the PR is already on hold for): the AWS European Sovereign Cloud uses a distinct domain namespace (
*.amazonaws.eu) separate from commercial AWS. Validating the ARN format here is a necessary first step, but ensure every downstream operator that consumesprivateZoneIAMRolecan resolve*.amazonaws.euendpoints — AWS SDK (Terraform AWS provider) 6.x and Terraform 1.14+ support ESC natively; older versions require manual endpoint configuration. Components still on AWS SDK v1 (EOL) will not resolve EUSC endpoints out of the box, which the PR discussion already flags.🤖 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_dnses.crd.yaml` around lines 74 - 77, CRD change adds the aws-eusc partition to the ARN regex for privateZoneIAMRole but downstream consumers may not resolve AWS European Sovereign Cloud endpoints; update the `privateZoneIAMRole` description (and any operator docs) to explicitly state the requirement that consumers support the aws-eusc (`*.amazonaws.eu`) endpoints and list the minimum SDK/provider versions (e.g., AWS SDK v2+/Terraform AWS provider 6.x, Terraform 1.14+) or add a validating webhook/annotation check to reject configurations when the operator runtime is on unsupported SDK/provider versions; reference `privateZoneIAMRole` and the updated ARN pattern in your edits so the note is colocated with the schema change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@payload-manifests/crds/0000_10_config-operator_01_dnses.crd.yaml`:
- Around line 74-77: CRD change adds the aws-eusc partition to the ARN regex for
privateZoneIAMRole but downstream consumers may not resolve AWS European
Sovereign Cloud endpoints; update the `privateZoneIAMRole` description (and any
operator docs) to explicitly state the requirement that consumers support the
aws-eusc (`*.amazonaws.eu`) endpoints and list the minimum SDK/provider versions
(e.g., AWS SDK v2+/Terraform AWS provider 6.x, Terraform 1.14+) or add a
validating webhook/annotation check to reject configurations when the operator
runtime is on unsupported SDK/provider versions; reference `privateZoneIAMRole`
and the updated ARN pattern in your edits so the note is colocated with the
schema change.
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@tthvo It might make sense for us to feature gate this change so that we can merge the validation change and add automated regression testing that helps us identify cases where this causes things to break. From there, we can update things accordingly under the same feature gate. |
Right... I am just a bit hesitant if we should (or have ever) create a feature gate for installing into a (new) region. I'll double check with @patrickdillon when he's back next week. My understand is that allowing
To answer your original question, after some testing, I'd say yes with a small adjustment to make sure the operators honour necessary custom service endpoints, which they really should. They may have missed it when adding these arn input fields previously - bug? The following API fields are updated to allow
|
|
@everettraven Thanks for having a look! I added my views and (local) test results above. Please let me know what you think 🙏 I'll check with Patrick about the feature gate when he's back. |
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration The profile uses vault secret: cluster-secrets-aws-eusc-qe Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708 Tested installation scenarios (from installer PR openshift#10303): - ✅ Default minimal config - ✅ Ingress NLB type - ✅ BYO Private Hosted zone - ✅ BYO KMS key - ✅ BYO VPC and subnets Installation time: ~45m26s Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
|
@everettraven @JoelSpeed Following our discussions here, I open #2740 to add the feature gate for EU Sovereign Cloud support. PTAL 👀 I'll update this PR once that's merged :D |
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
According to AWS docs, ARNs in AWS European Sovereign Cloud begin with arn:aws-eusc: Thus, to support EUS Cloud, we need to update the validation to allow this new format. This commits only focuses on kmsKeyARN and privateZoneIAMRole.
openshift-ci
bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yaml (1)
7-104: Add gate-specific aws-eusc ARN coverage in this fixture.This gate-scoped file currently validates only a minimal create path, so it does not actually assert
AWSEuropeanSovereignCloudInstallbehavior for ControllerConfig. Please add at least one explicitspec.dns.spec.platform.aws.privateZoneIAMRolecase usingarn:aws-eusc:...(and ideally one invalid-partition rejection case) to make this test meaningful.Suggested test addition (example)
tests: onCreate: @@ - name: Should be able to create a minimal ControllerConfig initial: | ... expected: | ... + - name: Should allow aws-eusc privateZoneIAMRole when feature gate is enabled + initial: | + apiVersion: machineconfiguration.openshift.io/v1 + kind: ControllerConfig + spec: + ... + dns: + apiVersion: config.openshift.io/v1 + kind: DNS + spec: + platform: + type: AWS + aws: + privateZoneIAMRole: arn:aws-eusc:iam::123456789012:role/my-role + expected: | + apiVersion: machineconfiguration.openshift.io/v1 + kind: ControllerConfig + spec: + ... + dns: + apiVersion: config.openshift.io/v1 + kind: DNS + spec: + platform: + type: AWS + aws: + privateZoneIAMRole: arn:aws-eusc:iam::123456789012:role/my-roleAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yaml` around lines 7 - 104, Add gate-specific aws-eusc ARN coverage by updating the onCreate test cases for ControllerConfig: add at least one test (under the existing onCreate block or a new named test) that sets spec.dns.spec.platform.aws.privateZoneIAMRole to a valid aws-eusc ARN (e.g., "arn:aws-eusc:...") and include the same value in the expected output; also add a negative test that supplies an invalid partition ARN (e.g., wrong partition prefix) and asserts rejection. Locate and modify the initial/expected YAML for the "Should be able to create a minimal ControllerConfig" or add a new test entry so it references spec.dns.spec.platform.aws.privateZoneIAMRole and validates both the valid arn:aws-eusc:... case and an invalid-partition rejection case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_dns.go`:
- Around line 137-138: The unconditional validation annotation that rejects
aws-eusc (the line with FeatureGateAwareXValidation:featureGate="") must be
removed/converted so it does not coexist with the gate-enabled validator;
instead ensure there are two mutually-exclusive validators for the same field:
one that applies only when AWSEuropeanSovereignCloudInstall is disabled (the
rule that excludes aws-eusc) and the other that applies only when
AWSEuropeanSovereignCloudInstall is enabled (the rule that includes aws-eusc).
Update the FeatureGateAwareXValidation annotations around the IAM role ARN (the
two rules shown) so the base “reject aws-eusc” rule is not unconditional but
tied to the inverse/disabled state and the gate-enabled rule
(featureGate=AWSEuropeanSovereignCloudInstall) replaces it when the gate is
enabled, preventing both validators from being appended simultaneously.
In
`@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml`:
- Around line 129-133: The two x-kubernetes-validations entries are conjunctive
so the first rule rejects aws-eusc ARNs even though the second allows them;
update the validation to use a single rule that accepts both by merging the
patterns (include aws-eusc in the alternation) or remove the first redundant
rule so only the correct pattern remains—target the x-kubernetes-validations
block and adjust the rule string(s) where the KMS key ARN regex is defined to
include aws-eusc in the allowed provider list.
---
Nitpick comments:
In
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yaml`:
- Around line 7-104: Add gate-specific aws-eusc ARN coverage by updating the
onCreate test cases for ControllerConfig: add at least one test (under the
existing onCreate block or a new named test) that sets
spec.dns.spec.platform.aws.privateZoneIAMRole to a valid aws-eusc ARN (e.g.,
"arn:aws-eusc:...") and include the same value in the expected output; also add
a negative test that supplies an invalid partition ARN (e.g., wrong partition
prefix) and asserts rejection. Locate and modify the initial/expected YAML for
the "Should be able to create a minimal ControllerConfig" or add a new test
entry so it references spec.dns.spec.platform.aws.privateZoneIAMRole and
validates both the valid arn:aws-eusc:... case and an invalid-partition
rejection case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4d9914cc-47c7-4448-a1d0-053442dd5401
⛔ Files ignored due to path filters (35)
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-Default.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-OKD.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.featuregated-crd-manifests/dnses.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/dnses.config.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-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/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/GCPClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/HighlyAvailableArbiter+DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/HighlyAvailableArbiter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-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-Default.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-OKD.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/*operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/clustercsidrivers.operator.openshift.io/VSphereConfigurableMaxAllowedBlockVolumesPerNode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (14)
config/v1/tests/dnses.config.openshift.io/AWSEuropeanSovereignCloudInstall.yamlconfig/v1/types_dns.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlmachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/tests/clustercsidrivers.operator.openshift.io/AWSEuropeanSovereignCloudInstall.yamloperator/v1/types_csi_cluster_driver.gooperator/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- operator/v1/types_csi_cluster_driver.go
| // +openshift:validation:FeatureGateAwareXValidation:featureGate="",rule=`matches(self, '^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$')`,message=`IAM role ARN must match "^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$"` | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=AWSEuropeanSovereignCloudInstall,rule=`matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role\/.*$')`,message=`IAM role ARN must match "^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:
# First, let's locate and examine the file
fd "types_dns.go" --type fRepository: openshift/api
Length of output: 104
🏁 Script executed:
# Read the specific lines in context
head -160 config/v1/types_dns.go | tail -40Repository: openshift/api
Length of output: 2135
🏁 Script executed:
# Get more context around those lines to understand the field
sed -n '120,150p' config/v1/types_dns.goRepository: openshift/api
Length of output: 1386
🏁 Script executed:
# Search for FeatureGateAwareXValidation in the codebase
rg "FeatureGateAwareXValidation" --type go -A 5 -B 5Repository: openshift/api
Length of output: 40908
🏁 Script executed:
# Look for validation generation code
fd "*.go" --type f | xargs rg -l "FeatureGateAwareXValidation|featureGate" | head -10Repository: openshift/api
Length of output: 784
🏁 Script executed:
# Check for any documentation or comments about the validation behavior
rg "featureGate.*replacement|featureGate.*additive|validation.*replacement" --type go -iRepository: openshift/api
Length of output: 933
🏁 Script executed:
# Search for the FeatureGateXValidation type definition and ApplyToSchema implementation
rg -A 30 "type FeatureGateXValidation struct"Repository: openshift/api
Length of output: 3854
🏁 Script executed:
# Search for code that processes or applies these markers to schema
rg "ApplyToSchema" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.go -A 50 | head -100Repository: openshift/api
Length of output: 4003
🏁 Script executed:
# Look for how x-kubernetes-validations are accumulated when multiple markers exist
rg "x-kubernetes-validations" -A 3 -B 3 --type go | head -60Repository: openshift/api
Length of output: 5789
🏁 Script executed:
# Find the XValidation type and its ApplyToSchema method
rg -B 5 -A 20 "type XValidation struct" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.goRepository: openshift/api
Length of output: 39
🏁 Script executed:
# Find the ApplyToSchema for XValidation to see if it appends or replaces
rg -B 2 -A 20 "func (m XValidation) ApplyToSchema" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.goRepository: openshift/api
Length of output: 39
🏁 Script executed:
# Look for how multiple validations are handled - search for Validations field manipulation
rg "\.Validations = |\.Validations =" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.go -B 3 -A 3Repository: openshift/api
Length of output: 39
🏁 Script executed:
# Search for XValidation definition
rg "type XValidation" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/ -A 20Repository: openshift/api
Length of output: 2497
🏁 Script executed:
# Search for how validation rules are applied
rg "Validations" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.go -B 2 -A 2 | head -80Repository: openshift/api
Length of output: 39
🏁 Script executed:
# Check the complete patch_validation.go file structure
wc -l tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.goRepository: openshift/api
Length of output: 138
🏁 Script executed:
# Read the entire patch_validation.go file to understand the full logic
cat tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/patch_validation.goRepository: openshift/api
Length of output: 9963
🏁 Script executed:
# Also check the validation.go file for XValidation.ApplyToSchema
cat tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/validation.go | head -200Repository: openshift/api
Length of output: 8081
🏁 Script executed:
# Find the XValidation.ApplyToSchema method
rg -A 30 "func (m XValidation) ApplyToSchema" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/validation.goRepository: openshift/api
Length of output: 39
🏁 Script executed:
# Also check for how Validations field is used - look for append or assignment patterns
rg -B 5 -A 15 "XValidations" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/validation.go | head -60Repository: openshift/api
Length of output: 788
🏁 Script executed:
# Confirm that multiple XValidations are indeed appended - look at the append pattern
rg "schema.XValidations = append" tools/vendor/sigs.k8s.io/controller-tools/pkg/crd/markers/ -B 3 -A 3Repository: openshift/api
Length of output: 754
🏁 Script executed:
# Look for other examples of dual FeatureGateAwareXValidation in the codebase to understand the pattern
rg -B 3 "featureGate=\"\".*\n.*FeatureGateAwareXValidation:featureGate=" config/ | head -40Repository: openshift/api
Length of output: 248
🏁 Script executed:
# Look for examples of dual FeatureGateAwareXValidation annotations on same field
rg -U "featureGate=\"\".*\n.*FeatureGateAwareXValidation" config/ -A 1 | head -80Repository: openshift/api
Length of output: 1681
🏁 Script executed:
# Get a clearer picture - search for fields with multiple FeatureGateAwareXValidation markers
grep -r "FeatureGateAwareXValidation:featureGate=" config/ | grep -A 1 "featureGate=\"\"" | head -20Repository: openshift/api
Length of output: 1262
🏁 Script executed:
# Check the HasAny implementation to understand empty featureGate behavior
rg -B 5 -A 10 "func.*HasAny" tools/vendor -A 5 | head -40Repository: openshift/api
Length of output: 2736
🏁 Script executed:
# Check if there are any generated manifest files in this repo that show the actual output
fd "types_dns" --type f -path "*/config/*" | grep -E "(generated|manifest|openapi)"Repository: openshift/api
Length of output: 225
The feature-gated DNS validator rules are both present in gate-enabled variants, not replaced.
Lines 137–138 both append validation rules to the same field. When the AWSEuropeanSovereignCloudInstall gate is enabled, both the base rule (rejecting aws-eusc) and the gate-enabled rule (accepting aws-eusc) are appended. Since all x-kubernetes-validations rules must pass, the base rule's rejection of aws-eusc takes precedence, making the gate ineffective. The architecture only supports additive validators; the gate-enabled rule must replace the base rule instead of coexisting with it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_dns.go` around lines 137 - 138, The unconditional validation
annotation that rejects aws-eusc (the line with
FeatureGateAwareXValidation:featureGate="") must be removed/converted so it does
not coexist with the gate-enabled validator; instead ensure there are two
mutually-exclusive validators for the same field: one that applies only when
AWSEuropeanSovereignCloudInstall is disabled (the rule that excludes aws-eusc)
and the other that applies only when AWSEuropeanSovereignCloudInstall is enabled
(the rule that includes aws-eusc). Update the FeatureGateAwareXValidation
annotations around the IAM role ARN (the two rules shown) so the base “reject
aws-eusc” rule is not unconditional but tied to the inverse/disabled state and
the gate-enabled rule (featureGate=AWSEuropeanSovereignCloudInstall) replaces it
when the gate is enabled, preventing both validators from being appended
simultaneously.
| x-kubernetes-validations: | ||
| - message: KMS key ARN must match "^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)\/.*$" | ||
| 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: KMS key ARN must match "^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)\/.*$" | ||
| 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)\/.*$') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd payload-manifests/crds && head -150 0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml | tail -30Repository: openshift/api
Length of output: 2260
🌐 Web query:
Kubernetes x-kubernetes-validations conjunctive AND semantics CRD validation rules
💡 Result:
In CRD schemas, each entry in x-kubernetes-validations is an independent CEL rule, and Kubernetes applies them with conjunctive (logical AND) semantics:
- A custom resource is considered valid only if every applicable validation rule evaluates to
true; if any rule fails, the API server rejects the request and reports the failing rule’s message (example shown where one of multiple rules rejects the object). [1] - You can define multiple rules under the same
x-kubernetes-validationslist (and rules can exist at multiple schema locations); all of them participate in overall validity, and Kubernetes even tracks a resource budget for “executing all of its validation rules”. [1] - Therefore, if you need OR semantics, you must write it inside a single CEL expression (e.g.,
rule: "condA || condB"), rather than splittingcondAandcondBinto separate list items (because separate items are effectively ANDed). CEL supports&&/||as shown in Kubernetes’ CEL examples. [1][2]
Sources: Kubernetes CRD validation rules documentation (CustomResourceDefinitions task doc) [1] and Kubernetes CEL reference [2].
[1] https://v1-34.docs.kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/
[2] https://kubernetes.io/docs/reference/using-api/cel/
The first KMS ARN validator blocks aws-eusc ARNs despite the second rule allowing them.
x-kubernetes-validations entries are conjunctive—all rules must pass for validation to succeed. The first rule on Line 130 rejects any ARN containing aws-eusc, while the second rule on Line 132 accepts it. Since both rules apply, any aws-eusc ARN fails the first rule and is rejected, defeating the purpose of the second rule. Remove the first rule or merge both patterns into a single rule using OR logic (||).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml`
around lines 129 - 133, The two x-kubernetes-validations entries are conjunctive
so the first rule rejects aws-eusc ARNs even though the second allows them;
update the validation to use a single rule that accepts both by merging the
patterns (include aws-eusc in the alternation) or remove the first redundant
rule so only the correct pattern remains—target the x-kubernetes-validations
block and adjust the rule string(s) where the KMS key ARN regex is defined to
include aws-eusc in the allowed provider list.
There was a problem hiding this comment.
hmm, fair point. Let me see 👀
There was a problem hiding this comment.
@tthvo — the fix for the CustomNoUpgrade CRD (where AWSEuropeanSovereignCloudInstall is active) is straightforward: replace the two conjunctive rules with a single rule using the expanded pattern that already includes aws-eusc:
x-kubernetes-validations:
- message: KMS key ARN must match "^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)\/.*$"
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)\/.*$')Since CustomNoUpgrade activates all feature gates including AWSEuropeanSovereignCloudInstall, there's no need for the narrower first rule at all. The same pattern applies to TechPreviewNoUpgrade, DevPreviewNoUpgrade, and OKD CRD variants if they also include the feature gate. For the Default CRD, the narrower rule (without aws-eusc) should be the only rule present.
Worth checking: does the code generator that produces these YAML files from FeatureGateAwareXValidation annotations handle this correctly? If it's emitting two AND-ed rules instead of a single merged rule, the generator logic itself may need a fix.
There was a problem hiding this comment.
I need different validations based on weather a feature gate is enabled or not :D
There was a problem hiding this comment.
🧩 Analysis chain
@tthvo — the intent is right, but the issue is that the mechanism generates both rules in the feature-gate-enabled variants (e.g., CustomNoUpgrade), where they're AND-ed together. So in those variants, aws-eusc ARNs still fail the first rule even though the feature gate is active.
Let me check how the FeatureGateAwareXValidation generator handles similar cases elsewhere:
🏁 Script executed:
#!/bin/bash
# Check how FeatureGateAwareXValidation is used in the Go source files for the CSI driver
rg -n "FeatureGateAwareXValidation" operator/v1/types_csi_cluster_driver.go
# Check the generator/tooling to understand how it handles co-existing rules
rg -rn "FeatureGateAwareXValidation" --type=go -A3 -B3 | grep -A6 -B6 "featureGate.*:" | head -80
# Check if there's a marker/annotation that replaces rather than adds rules
rg -rn "featuregated" --type=go -l | head -10
Length of output: 10657
@tthvo — the intent is right, but the generator's semantics don't match it. featureGate="" means "always include this rule", not "include only when no feature gate is active." So in CustomNoUpgrade the generator emits both:
- Base rule (no
aws-eusc) — always included - Expanded rule (with
aws-eusc) — included because the feature gate is active
Both appear in the CRD's x-kubernetes-validations list and are AND-ed. An aws-eusc ARN must satisfy both, but it fails rule #1. The feature-gate-aware CRD generates a stricter validator than the default one, which is backwards.
For the feature-gate-aware pattern to work, the base rule must be absent in feature-gate-active variants. There are two practical paths:
Option A — Drop the base rule, keep only the expanded one (simplest):
In feature-gate-active CRD variants (CustomNoUpgrade, TechPreviewNoUpgrade, etc.), only the AWSEuropeanSovereignCloudInstall-gated rule should be present. If the generator has no "replace on feature gate" semantic, removing the featureGate="" annotation and only keeping the feature-gate-gated rule would mean Default has no CEL validation (the old pattern: regex could be kept for Default instead).
Option B — Single rule with OR, no base rule:
Remove the base featureGate="" CEL rule entirely and keep the pattern: regex for the non-feature-gate variants. The CEL rule with aws-eusc is only added in feature-gate-active CRDs. Since the pattern: regex is already stricter and present in Default, this avoids the AND conflict.
The core issue: featureGate="" does not mean "only when the gate is absent" — it means "unconditionally." So both rules will always coexist in activated variants.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
Option B won't work because the spec descriptor is generate as:
pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$
type: string
x-kubernetes-validations:
- message: privateZoneIAMRole must be a valid AWS IAM role ARN
rule: matches(self, '^arn:(aws|aws-cn|aws-us-gov|aws-eusc):iam::[0-9]{12}:role\/.*$')Would it means the same conflict?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the DNS CRD variants to see if pattern: + x-kubernetes-validations coexist
echo "=== CustomNoUpgrade ==="
grep -A8 "privateZoneIAMRole" payload-manifests/crds/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yaml | head -20
echo "=== Default ==="
grep -A8 "privateZoneIAMRole" payload-manifests/crds/0000_10_config-operator_01_dnses-Default.crd.yaml | head -20
echo "=== TechPreviewNoUpgrade ==="
grep -A8 "privateZoneIAMRole" payload-manifests/crds/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yaml | head -20
echo "=== CSI CustomNoUpgrade kmsKeyARN ==="
grep -A10 "kmsKeyARN" payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yaml | head -20
echo "=== CSI Default kmsKeyARN ==="
grep -A10 "kmsKeyARN" payload-manifests/crds/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yaml | head -20
Length of output: 4824
@tthvo — yes, pattern: and x-kubernetes-validations are also AND-ed in Kubernetes validation, so having a narrower pattern: alongside a wider CEL rule creates exactly the same conflict — aws-eusc would fail the pattern: check regardless of what the CEL rule allows.
However, looking at the actual generated CRDs, there's no pattern: + CEL combination — the generator has already converted everything to CEL rules:
| Variant | kmsKeyARN / privateZoneIAMRole |
|---|---|
Default |
1 CEL rule (narrow, no aws-eusc) ✅ |
CustomNoUpgrade |
2 AND-ed CEL rules (narrow + expanded) ❌ |
TechPreviewNoUpgrade |
2 AND-ed CEL rules (narrow + expanded) ❌ |
So the conflict is the same as originally flagged, just manifested as two AND-ed CEL rules. The featureGate="" base rule is always emitted, and the feature-gate-specific expanded rule is added on top — but since both must pass, aws-eusc ARNs are still rejected in every variant.
The fix needs to be in the generator: when a feature gate is active for a given CRD variant, the feature-gated rule should replace the base rule rather than be AND-ed with it. Specifically, in CustomNoUpgrade / TechPreviewNoUpgrade / DevPreviewNoUpgrade, only the expanded rule should appear:
x-kubernetes-validations:
- message: KMS key ARN must match "^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)\/.*$"
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)\/.*$')Does the generator have a way to express "replace base rule when this feature gate is active" rather than "add this rule alongside the base rule"? If not, that's the gap to address.
|
@tthvo: 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. |
|
/jira refresh |
|
@liweinan: This pull request references CORS-4337 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
|
/hold Figuring out why some manifests have conflicting xvalidation regex check 😞 |
openshift-ci
bot
added
the
do-not-merge/hold
|
You may need to update https://github.com/openshift/kubernetes-sigs-controller-tools/blob/master/pkg/crd/markers/patch_validation.go to support negation of feature gates for the XValidation marker to support this. I don't think we have had this use case before |
|
@JoelSpeed I think you should be able to do the negation semantics for XValidations. We did it in api/config/v1/types_authentication.go Lines 353 to 356 in 5c75e62 |
This commit adds CI infrastructure support for AWS European Sovereign Cloud (EUSC) testing using the eusc-de-east-1 region. Changes: - Add cluster-secrets-aws-eusc-qe to secret bootstrap config - Add aws-eusc-qe-quota-slice boskos resource pool with 5 quota slices in eusc-de-east-1 region - Generate updated _boskos.yaml configuration Region Details: - Region: eusc-de-east-1 (Brandenburg, Germany) - Availability zones: eusc-de-east-1a, eusc-de-east-1b (2 zones only) - Note: No edge zones (Local/Wavelength) available in this region Dependencies: - Installer support: openshift/installer#10303 - Ingress operator support: openshift/cluster-ingress-operator#1360 - API support (optional): openshift/api#2708
|
I did a bit more digging as to why the example I shared previously seems to work but the same thing here might not be working. It looks like because api/tools/codegen/pkg/manifestmerge/crd-schema.json Lines 719 to 736 in 54a3998 Because the keys for the map here are unique, and our manifest merge logic uses a different field manager based on the CRD path ( ), the merged rules is what the "correct" behavior is here.@JoelSpeed This seems like expected behavior based on our tooling, but I'm doing some additional exploration to see if we should modify the CRD schema semantics we pass through to our tooling to enable this case. |
|
@tthvo I've had a chat with @JoelSpeed and it seems reasonable for us to make some tooling changes to address this. I've got #2770 up as a draft that I'm going to get polished up and take it out of draft to unblock you. |
|
Ahha, thank you guys! That sounds good to me! Phew, I couldn't get it to work all day yesterday 😅 I'll regenerate the manifests asap after 🙏 |
6 participants
According to AWS docs, ARNs in AWS European Sovereign Cloud begin with
Thus, to support EUS Cloud, we need to update the validation to allow this new format.