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 2 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)
|
📝 WalkthroughWalkthroughThis change replaces CEL format-based validation with explicit regular expressions. It updates Vault secret names, ConfigMap names, and TLS server names in Go types and three generated APIServer CRDs. It also updates ingress label-value and label-key validation in Go types and three generated Ingress CRDs. Label keys retain the 253-character maximum. Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ardaguclu 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 |
|
New changes are detected. LGTM label has been removed. |
![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
| // an alphanumeric character. | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| // +kubebuilder:validation:XValidation:rule="!format.labelValue().validate(self).hasValue()",message="label values must be valid Kubernetes label values (at most 63 characters, alphanumeric, '-', '_', or '.', must start and end with alphanumeric)" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$')",message="label values must be valid Kubernetes label values (at most 63 characters, alphanumeric, '-', '_', or '.', must start and end with alphanumeric)" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
files = [
"config/v1/types_ingress.go",
"payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml",
"payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml",
"payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml",
]
expected = "self.matches('^([a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?)?$')"
for filename in files:
assert expected in Path(filename).read_text(), filename
pattern = re.compile(r"^([a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?)?$")
assert pattern.fullmatch("")
assert pattern.fullmatch("a")
assert pattern.fullmatch("a.b")
assert not pattern.fullmatch("a-")
PYRepository: openshift/api
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
config/v1/types_ingress.go \
payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml \
payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml \
payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml
do
echo "===== $file ====="
case "$file" in
*.go) sed -n '160,182p' "$file" ;;
*.yaml) sed -n '84,100p' "$file" ;;
esac
done
echo "===== matching ingress label rules ====="
rg -n -C 2 "label values|self\.matches\('\^\[a-zA-Z0-9\]" \
config/v1/types_ingress.go payload-manifests/crds
echo "===== relevant source context ====="
sed -n '130,190p' config/v1/types_ingress.goRepository: openshift/api
Length of output: 16795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("config/v1/types_ingress.go"),
Path("payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml"),
Path("payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml"),
Path("payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml"),
]
old = "self.matches('^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$')"
new = "self.matches('^([a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?)?$')"
print("Rule occurrences:")
for path in files:
text = path.read_text()
print(f"{path}: old={text.count(old)}, proposed={text.count(new)}")
old_pattern = re.compile(r"^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$")
new_pattern = re.compile(r"^([a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?)?$")
values = ["", "a", "a.b", "a-b", "a_", "a-", ".a", "a.", "a--b", "a" * 63, "a" * 64]
print("\nInput behavior:")
for value in values:
print(repr(value), "old=", bool(old_pattern.fullmatch(value)), "proposed=", bool(new_pattern.fullmatch(value)))
print("\nLabelValue references:")
for path in Path(".").rglob("*"):
if path.is_file() and path.suffix in {".go", ".yaml", ".yml"}:
try:
text = path.read_text()
except UnicodeDecodeError:
continue
if "LabelValue" in text:
print(path)
PYRepository: openshift/api
Length of output: 5632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
old="self.matches('^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$')"
echo "===== all tracked occurrences of the current rule ====="
git grep -n -F "$old" -- \
'config/v1/types_ingress.go' \
'config/v1/zz_generated*' \
'payload-manifests/crds/*ingresses*.yaml' || true
echo "===== LabelValue declaration and field usage ====="
rg -n -C 4 'type LabelValue|string\(LabelValue\)|LabelValue|ComponentRouteLabels|RouteLabels' \
config/v1 --glob '*.go' --glob '*.yaml'
echo "===== generated schema locations ====="
for file in config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-*.crd.yaml \
config/v1/zz_generated.featuregated-crd-manifests/ingresses.config.openshift.io/IngressComponentRouteLabels.yaml
do
if [ -f "$file" ]; then
echo "===== $file ====="
rg -n -C 5 'label values|self\.matches|labels defines additional labels' "$file"
fi
done
echo "===== focused tests mentioning ingress labels or LabelValue ====="
rg -n -i -C 3 'LabelValue|component route labels|route labels|empty label|label value' \
config tests --glob '*.go' --glob '*.yaml' --glob '*.yml' | head -240 || trueRepository: openshift/api
Length of output: 16722
Allow empty LabelValue values in all generated CRDs.
The current validation rejects "", although the type documentation and Kubernetes label rules permit empty values. Make the complete non-empty regex alternative optional, regenerate the affected CRDs under config/v1/zz_generated* and payload-manifests/crds, and add regression coverage for empty, one-character, valid, and trailing-separator values.
📍 Affects 4 files
config/v1/types_ingress.go#L174-L174(this comment)payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml#L93-L93payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml#L93-L93payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml#L93-L93
🤖 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 `@config/v1/types_ingress.go` at 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.
| // +kubebuilder:validation:MinProperties=1 | ||
| // +kubebuilder:validation:MaxProperties=8 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(key, !format.qualifiedName().validate(key).hasValue())",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" | ||
| // +kubebuilder:validation:XValidation: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 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" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
config/v1/types_ingress.go
payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml
payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml
payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml
)
if rg -n --fixed-strings 'key.size() <= 253' "${files[@]}"; then
echo "The total-key length check is still present." >&2
exit 1
fi
python - <<'PY'
prefix = ".".join(["a" * 63, "b" * 63, "c" * 63, "d" * 61])
assert len(prefix) == 253
assert len(prefix + "/x") == 255
assert len("a" * 64) == 64
print("Qualified-name boundary fixtures are ready for API-server validation tests.")
PYRepository: openshift/api
Length of output: 1184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- source and generated rules ---'
sed -n '270,288p' config/v1/types_ingress.go
for f in \
payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml \
payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml \
payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml
do
echo "--- $f ---"
sed -n '115,128p' "$f"
done
echo '--- related validation and tests ---'
rg -n -S 'key\.size\(\)|qualified name|QualifiedName|label keys|types_ingress|Ingress' \
config payload-manifests --glob '*.go' --glob '*.yaml' --glob '*_test.go' \
| head -200Repository: openshift/api
Length of output: 43807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
# The Go annotation and CRDs express this same logical regex after escaping.
pattern = re.compile(
r'^([a-z0-9]([a-z0-9\-]*[a-z0-9])?'
r'(\.[a-z0-9]([a-z0-9\-]*[a-z0-9])?)*\/)?'
r'[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$'
)
def current_accepts(key):
return bool(pattern.fullmatch(key)) and len(key) <= 253
cases = {
"64-character name": "n" * 64,
"64-character DNS label": ("a" * 64) + "/x",
"253-character prefix plus /x":
".".join(["a" * 63, "b" * 63, "c" * 63, "d" * 61]) + "/x",
"valid 63-character name": "n" * 63,
"valid 63-character DNS label": ("a" * 63) + "/x",
}
for name, key in cases.items():
print(f"{name}: key_length={len(key)} regex={bool(pattern.fullmatch(key))} current_rule={current_accepts(key)}")
PYRepository: openshift/api
Length of output: 496
Validate qualified-name segments independently.
The rule accepts 64-character name and DNS-label segments and rejects a valid 253-character prefix followed by /x. Bound each prefix label and the name segment separately, then regenerate all three CRDs and add boundary tests.
📍 Affects 4 files
config/v1/types_ingress.go#L281-L281(this comment)payload-manifests/crds/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml#L122-L123payload-manifests/crds/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml#L122-L123payload-manifests/crds/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml#L122-L123
🤖 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 `@config/v1/types_ingress.go` at 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.
|
@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. |
2 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