CNTRLPLANE-3361: Align with latest beta API of Vault#2936
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-3361 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe Vault KMS API replaces 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 |
|
Hello @ardaguclu! Some important instructions when contributing to openshift/api: |
|
/hold |
openshift-ci
Bot
added
size/XXL
![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/tests/apiservers.config.openshift.io/VaultKMS.yaml`:
- Around line 365-366: Extend the vaultKeyPath invalid-test fixtures near
“Should reject vaultKeyPath without /keys/ separator” with cases for a
1025-character mount and a 513-character key, ensuring each complete path
remains under 1542 characters and is rejected by the schema.
In `@config/v1/types_kmsencryption.go`:
- Around line 212-219: Preserve independent mount and key length limits for the
vaultKeyPath validation. In config/v1/types_kmsencryption.go lines 212-219, add
CEL checks limiting the mount component to 1024 characters and the key name
component to 512 characters; regenerate the corresponding validation in
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
lines 379-407,
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
lines 379-407, and
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
lines 379-407; add rejection fixtures for independently overlong components in
config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml lines 365-366.
🪄 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: de2ae300-c773-4f55-9ac3-09c5a6e2950e
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (6)
config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yamlconfig/v1/tests/apiservers.config.openshift.io/VaultKMS.yamlconfig/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
| # vaultKeyPath invalid tests | ||
| - name: Should reject vaultKeyPath without /keys/ separator |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add component-length rejection cases.
Add fixtures for a 1025-character mount and a 513-character key whose complete paths remain below 1542 characters. They currently demonstrate that the schema admits values rejected by the former component contracts.
🤖 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/tests/apiservers.config.openshift.io/VaultKMS.yaml` around lines
365 - 366, Extend the vaultKeyPath invalid-test fixtures near “Should reject
vaultKeyPath without /keys/ separator” with cases for a 1025-character mount and
a 513-character key, ensuring each complete path remains under 1542 characters
and is rejected by the schema.
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=1542 | ||
| // +kubebuilder:validation:XValidation:rule="!self.startsWith('/')",message="vaultKeyPath cannot start with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="vaultKeyPath cannot end with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains('//')",message="vaultKeyPath cannot contain consecutive forward slashes" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9._~/-]+$')",message="vaultKeyPath must only contain RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward slashes" | ||
| // +kubebuilder:validation:XValidation:rule="self.split('/').filter(s, s == '.' || s == '..').size() == 0",message="vaultKeyPath must not contain '.' or '..' path segments" | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-zA-Z0-9._~-]+(/[a-zA-Z0-9._~-]+)*/keys/[a-zA-Z0-9_]([a-zA-Z0-9_.-]*[a-zA-Z0-9_])?$')`,message="vaultKeyPath must follow the format <mount>/keys/<key-name> where the key name starts and ends with an alphanumeric character or underscore and may contain alphanumeric characters, underscores, hyphens, and periods" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the independent mount and key length limits. The aggregate 1542-character limit permits either component to exceed its previous contract.
config/v1/types_kmsencryption.go#L212-L219: add CEL validation limiting the mount to 1024 and key name to 512 characters.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml#L379-L407: regenerate the corrected validation.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml#L379-L407: regenerate the corrected validation.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml#L379-L407: regenerate the corrected validation.config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml#L365-L366: add rejection fixtures for independently overlong components.
📍 Affects 5 files
config/v1/types_kmsencryption.go#L212-L219(this comment)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml#L379-L407payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml#L379-L407payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml#L379-L407config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml#L365-L366
🤖 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_kmsencryption.go` around lines 212 - 219, Preserve
independent mount and key length limits for the vaultKeyPath validation. In
config/v1/types_kmsencryption.go lines 212-219, add CEL checks limiting the
mount component to 1024 characters and the key name component to 512 characters;
regenerate the corresponding validation in
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
lines 379-407,
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
lines 379-407, and
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
lines 379-407; add rejection fixtures for independently overlong components in
config/v1/tests/apiservers.config.openshift.io/VaultKMS.yaml lines 365-366.
Source: Path instructions
|
/test integration |
| // +required | ||
| VaultKeyPath string `json:"vaultKeyPath,omitempty"` | ||
|
|
||
| // --- TOMBSTONE --- |
There was a problem hiding this comment.
do we really need a tombstone value here?
the API was introduced in this release and the entire feature is behind a GA feature gate. I’m sure there are no clusters where this struct would already be populated.
There was a problem hiding this comment.
I agree with you and I would be happy to remove those fields without tombstoning. But I think, tombstoning will be asked by API reviewers cc: @JoelSpeed @everettraven
There was a problem hiding this comment.
I think, this comment #2931 (comment) also applies here.
There was a problem hiding this comment.
Yep, tombstone required for both of these removed fields
| TransitMount string `json:"transitMount,omitempty"` | ||
| // TransitMount string `json:"transitMount,omitempty"` | ||
|
|
||
| // --- TOMBSTONE --- |
|
verify-crd-schema, verify-crdify are expected. verify-hypershift-integration seems to be perma-failing on every PR. |
|
/test verify-hypershift-integration |
|
Rebase has finished |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tjungblu 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 |
|
@JoelSpeed @everettraven could you PTAL, when you have a chance?. |
| // +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="vaultAuthNamespace cannot end with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains(' ')",message="vaultAuthNamespace cannot contain spaces" |
There was a problem hiding this comment.
Are there any other validations applied to the name?
There was a problem hiding this comment.
I've adopted the same validations of existing vault-namespace field. So I think, that is all.
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=4096 | ||
| // +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="vaultAuthNamespace cannot end with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains(' ')",message="vaultAuthNamespace cannot contain spaces" | ||
| // +kubebuilder:validation:XValidation:rule="!(self in ['root', 'sys', 'audit', 'auth', 'cubbyhole', 'identity'])",message="vaultAuthNamespace cannot be a reserved string (root, sys, audit, auth, cubbyhole, identity)" |
There was a problem hiding this comment.
All of these validations should be described in prose in the godoc above
There was a problem hiding this comment.
Thanks for review. I've added validations into the godoc. Please let me know your thoughts.
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=1542 | ||
| // +kubebuilder:validation:XValidation:rule="!self.startsWith('/')",message="vaultKeyPath cannot start with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="vaultKeyPath cannot end with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains('//')",message="vaultKeyPath cannot contain consecutive forward slashes" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9._~/-]+$')",message="vaultKeyPath must only contain RFC 3986 unreserved characters (alphanumeric, hyphen, period, underscore, tilde) and forward slashes" | ||
| // +kubebuilder:validation:XValidation:rule="self.split('/').filter(s, s == '.' || s == '..').size() == 0",message="vaultKeyPath must not contain '.' or '..' path segments" | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^[a-zA-Z0-9._~-]+(/[a-zA-Z0-9._~-]+)*/keys/[a-zA-Z0-9_]([a-zA-Z0-9_.-]*[a-zA-Z0-9_])?$')`,message="vaultKeyPath must follow the format <mount>/keys/<key-name> where the key name starts and ends with an alphanumeric character or underscore and may contain alphanumeric characters, underscores, hyphens, and periods" |
There was a problem hiding this comment.
All of these validations should be described in prose in the godoc above
| // +required | ||
| VaultKeyPath string `json:"vaultKeyPath,omitempty"` | ||
|
|
||
| // --- TOMBSTONE --- |
There was a problem hiding this comment.
Yep, tombstone required for both of these removed fields
4414e4c to
e4253b7
Compare
|
New changes are detected. LGTM label has been removed. |
|
/test images |
1 similar comment
|
/test images |
|
/test images |
|
@ardaguclu: 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. |
5 participants
Vault notifies us about their planned API changes in the new beta image. This new version brings some breaking changes. If we were in GA, we must have made changes in backwards compatible way. However, we haven't promoted this feature to GA yet. That's why, this PR tombstones the deprecated fields (i.e. transit-mount, transit-key), introduces one required field
vault-key-pathand one optional fieldvault-auth-namespace.