NO-JIRA: fix(api): replace CEL url() validators with regex to fix CRD cost budget - #8004
Conversation
The isURL()/url() CEL functions used in HCPEtcdBackup encryptionKeyURL and snapshotURL validators are extremely expensive in the API server's cost estimator. This caused the HostedCluster CRD to exceed the x-kubernetes-validations cost budget by >100x, breaking TechPreview installs. Replace isURL()/url() CEL rules with equivalent self.matches() regex validation and reduce encryptionKeyURL maxLength from 512 to 210 to further lower the estimated cost. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe pull request refactors URL validation rules in the etcdbackup API types. Multiple EncryptionKeyURL fields across HCPEtcdBackupAzureBlob, HCPEtcdBackupEncryptionMetadataAzure, and HCPEtcdBackupConfigAzure are consolidated to use a single Azure Key Vault HTTPS URL pattern validation, with their maximum length reduced from 512 to 210 characters. Additionally, the SnapshotURL field validation in HCPEtcdBackupStatus is updated to use a unified regex pattern that enforces https or s3 URI schemes, replacing separate scheme and URL validation checks. No public API signatures are modified. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
|
@enxebre: This pull request explicitly references no 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. |
|
/test verify |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/etcdbackup_types.go`:
- Around line 186-189: The kubebuilder validation for EncryptionKeyURL uses
MaxLength=210 which is too small for worst-case Azure Key Vault URLs; update the
kubebuilder tag on the EncryptionKeyURL field (the struct field EncryptionKeyURL
in etcdbackup_types.go) to a larger value (e.g., 220) to safely accommodate the
documented maximum component lengths, leaving the XValidation regex/immutability
rules unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 72bc48ba-df12-4a5f-a572-2b9bd2a6555b
⛔ Files ignored due to path filters (10)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hcpetcdbackups.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hcpetcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hcpetcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/etcdbackup_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
api/hypershift/v1beta1/etcdbackup_types.go
| // +kubebuilder:validation:MaxLength=210 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^https://[a-zA-Z0-9-]+\\\\.vault\\\\.azure\\\\.net/keys/[a-zA-Z0-9-]+(/[a-zA-Z0-9]+)?$')",message="encryptionKeyURL must be a valid Azure Key Vault HTTPS URL (https://<vault>.vault.azure.net/keys/<key-name>[/<key-version>])" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="encryptionKeyURL is immutable" | ||
| EncryptionKeyURL string `json:"encryptionKeyURL,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Azure Key Vault naming constraints
echo "Checking Azure Key Vault documentation references in codebase..."
rg -n "vault.azure.net" --type go -C2
echo ""
echo "Finding any existing MaxLength constraints for Azure URLs..."
rg -n "MaxLength.*[0-9]+" api/ --type go | grep -i azureRepository: openshift/hypershift
Length of output: 16951
MaxLength=210 is insufficient for maximum-length Azure Key Vault URLs.
The documented format https://<vault-name>.vault.azure.net/keys/<key-name>[/<key-version>] with maximum allowed component lengths requires 213 characters:
https://(8) + vault-name (24 max) +.vault.azure.net/keys/(21) + key-name (127 max) +/+ version (32 max) = 213
While practical real-world URLs are significantly shorter, consider increasing to 215–220 to fully accommodate the documented format specification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/hypershift/v1beta1/etcdbackup_types.go` around lines 186 - 189, The
kubebuilder validation for EncryptionKeyURL uses MaxLength=210 which is too
small for worst-case Azure Key Vault URLs; update the kubebuilder tag on the
EncryptionKeyURL field (the struct field EncryptionKeyURL in
etcdbackup_types.go) to a larger value (e.g., 220) to safely accommodate the
documented maximum component lengths, leaving the XValidation regex/immutability
rules unchanged.
|
/test e2e-aws-techpreview |
|
@enxebre: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
spot test failed for unrelated reason https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_hypershift/8004/pull-ci-openshift-hypershift-main-e2e-aws-techpreview/2034429330443997184 /e2e-aws-autonode |
Summary
isURL()/url()CEL functions introduced in CNTRLPLANE-2677: Add HCPEtcdBackup CRD API and feature gate #7898 forencryptionKeyURLandsnapshotURLfields are extremely expensive in the API server's cost estimator. This causes the HostedCluster CRD to exceed thex-kubernetes-validationscost budget by >100x, breaking TechPreview HyperShift installs.isURL()/url()CEL rules with equivalentself.matches()regex validation which has a much lower cost.encryptionKeyURLmaxLengthfrom 512 to 210 to further lower the estimated cost. Azure Key Vault URLs are well under 210 characters.The size is reduced to 210 which based on https://learn.microsoft.com/en-us/azure/key-vault/general/about-keys-secrets-certificates should give 4 chars headroom.
This was caught by the
e2e-aws-autonodeCI job failing during thehypershift-installstep:See failing test https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_hypershift/7939/pull-ci-openshift-hypershift-main-e2e-aws-autonode/2034351694233473024
We'll add envtest-based testing to catch techpreview CRD cost budget violations before merge going forward.
Test plan
make update && make verifypassesmake buildsucceeds🤖 Generated with Claude Code