Use regex instead of format help for KMS secret name validation - #2966
Use regex instead of format help for KMS secret name validation#2966JoelSpeed wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/M
PR Summary by QodoReplace CEL format-based DNS1123 validation with regex for KMS/Vault refs
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change replaces CEL format-based validation with explicit regular expressions and length checks. It updates DNS validation for KMS, API server, and Authentication fields. It also updates ingress label-value and label-key validation and adjusts the related test error. Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTo customize comments, go to the Qodo configuration screen, or learn more in the docs. |
|
/lgtm |
|
Scheduling tests matching the |
|
/approve |
![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
🤖 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 `@config/v1/types_ingress.go`:
- Line 281: Update the XValidation rule on the ingress label-key field in
config/v1/types_ingress.go so each DNS prefix label and the name segment are
length-validated independently, enforcing the qualified-name limits while
allowing a valid 253-character prefix plus “/x”; then regenerate all three CRDs.
Apply the generated schema changes to
payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml:122-123,
payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml:122-123,
and
payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml:122-123,
and add boundary tests covering valid and invalid segment lengths.
- Line 174: Make the LabelValue XValidation regex optional so empty values pass
while preserving rejection of invalid non-empty values and trailing separators;
update the validation in config/v1/types_ingress.go:174, regenerate the affected
config/v1/zz_generated* CRDs, and update the corresponding schemas in
payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml:93,
payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml:93,
and
payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml:93.
Add regression coverage for empty, one-character, valid, and trailing-separator
LabelValue inputs.
🪄 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: cc9f98bb-1b58-48ca-9793-d1b3bd9082f7
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/ingresses.config.openshift.io/IngressComponentRouteLabels.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (4)
config/v1/types_ingress.gopayload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml
openshift-ci
Bot
added
size/L
5377c74 to
d74ec2a
Compare
|
/lgtm |
|
Scheduling tests matching the |
d74ec2a to
5289f85
Compare
|
/verified by @JoelSpeed I've locally vendored these changes into hypershift and checked that the integration tests now pass |
openshift-ci-robot
added
the
verified
|
@JoelSpeed: This PR has been marked as verified by 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_ingresses-TechPreviewNoUpgrade.crd.yaml`:
- Around line 118-127: The first validation rule at line 120 applies a
63-character limit to all label keys, which incorrectly rejects valid qualified
names with DNS prefixes that exceed 63 characters but remain within the
253-character total limit. Update the first rule to only enforce the
63-character constraint on simple keys without a DNS prefix, or condition its
application appropriately. In the second rule, refactor the regex pattern to
explicitly enforce the 63-character limit on just the final name segment after
the slash (if present) rather than applying constraints that fail to isolate
that segment, while keeping the overall key-size limit at 253 characters as
specified.
🪄 Autofix
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: 393fa27a-d8c6-419e-9176-71782b7baaeb
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/ingresses.config.openshift.io/IngressComponentRouteLabels.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (9)
config/v1/tests/ingresses.config.openshift.io/IngressComponentRouteLabels.yamlconfig/v1/types_authentication.goconfig/v1/types_ingress.gopayload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- payload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml
- config/v1/types_ingress.go
- payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml
- config/v1/types_authentication.go
| - message: label keys must be between 1 and 63 characters in | ||
| length | ||
| rule: self.all(key, key.size() <= 63 && key.size() > 0) | ||
| - message: label keys must be valid qualified names, consisting | ||
| of an optional DNS subdomain prefix of up to 253 characters | ||
| followed by a slash and a name segment of 1-63 characters, | ||
| that consists only of alphanumeric characters, dashes, underscores, | ||
| and dots, and must start and end with an alphanumeric character | ||
| rule: self.all(key, !format.qualifiedName().validate(key).hasValue()) | ||
| rule: self.all(key, key.matches('^([a-z0-9]([a-z0-9\\-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9\\-]*[a-z0-9])?)*\\/)?[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$') | ||
| && key.size() <= 253) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow a 63-character name segment after a DNS prefix.
Line 120 applies the 63-character limit to the complete key. A valid qualified key with a DNS prefix and a 63-character name segment is rejected when its total length exceeds 63. The regex also does not limit the final name segment independently.
Keep the total-key limit at 253. Encode the 63-character limit on the final name segment in the regex. Add a test for a valid prefixed key longer than 63 characters.
Proposed validation change
- rule: self.all(key, key.size() <= 63 && key.size() > 0)
+ rule: self.all(key, key.size() > 0 && key.size() <= 253)
...
- rule: self.all(key, key.matches('^([a-z0-9]([a-z0-9\\-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9\\-]*[a-z0-9])?)*\\/)?[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$')
- && key.size() <= 253)
+ rule: self.all(key, key.matches('^([a-z0-9]([a-z0-9\\-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9\\-]*[a-z0-9])?)*\\/)?[a-zA-Z0-9]([a-zA-Z0-9._-]{0,61}[a-zA-Z0-9])?$')
+ && key.size() <= 253)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - message: label keys must be between 1 and 63 characters in | |
| length | |
| rule: self.all(key, key.size() <= 63 && key.size() > 0) | |
| - message: label keys must be valid qualified names, consisting | |
| of an optional DNS subdomain prefix of up to 253 characters | |
| followed by a slash and a name segment of 1-63 characters, | |
| that consists only of alphanumeric characters, dashes, underscores, | |
| and dots, and must start and end with an alphanumeric character | |
| rule: self.all(key, !format.qualifiedName().validate(key).hasValue()) | |
| rule: self.all(key, key.matches('^([a-z0-9]([a-z0-9\\-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9\\-]*[a-z0-9])?)*\\/)?[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$') | |
| && key.size() <= 253) | |
| - message: label keys must be between 1 and 63 characters in | |
| length | |
| rule: self.all(key, key.size() > 0 && key.size() <= 253) | |
| - message: label keys must be valid qualified names, consisting | |
| of an optional DNS subdomain prefix of up to 253 characters | |
| followed by a slash and a name segment of 1-63 characters, | |
| that consists only of alphanumeric characters, dashes, underscores, | |
| and dots, and must start and end with an alphanumeric character | |
| rule: self.all(key, key.matches('^([a-z0-9]([a-z0-9\\-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9\\-]*[a-z0-9])?)*\\/)?[a-zA-Z0-9]([a-zA-Z0-9._-]{0,61}[a-zA-Z0-9])?$') | |
| && key.size() <= 253) |
🤖 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_ingresses-TechPreviewNoUpgrade.crd.yaml`
around lines 118 - 127, The first validation rule at line 120 applies a
63-character limit to all label keys, which incorrectly rejects valid qualified
names with DNS prefixes that exceed 63 characters but remain within the
253-character total limit. Update the first rule to only enforce the
63-character constraint on simple keys without a DNS prefix, or condition its
application appropriately. In the second rule, refactor the regex pattern to
explicitly enforce the 63-character limit on just the final name segment after
the slash (if present) rather than applying constraints that fail to isolate
that segment, while keeping the overall key-size limit at 253 characters as
specified.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling tests matching the |
openshift-ci
Bot
added
the
approved
|
/override ci/prow/verify-crdify |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crdify 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 kubernetes-sigs/prow repository. |
|
@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. |
4 participants
The current validation using the format library is breaking HyperShift integration testing, this will be functionally equivalent but not rely on the library which came later
CC @ardaguclu