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 (6)
📝 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. 🔧 Checkov (3.3.8)config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yamlTraceback (most recent call last): config/v1/tests/apiservers.config.openshift.io/VaultKMS.yamlTraceback (most recent call last): payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlTraceback (most recent call last):
🔧 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.
|
/test integration |
| 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 |
|
@JoelSpeed @everettraven could you PTAL, when you have a chance?. |
4414e4c to
e4253b7
Compare
|
/test images |
1 similar comment
|
/test images |
|
/test images |
|
FYI it looks like verify-hypershift-integration will be fixed by openshift/hypershift#8695 |
e4253b7 to
91ec72c
Compare
|
@JoelSpeed could you PTAL, when you have time?. I've rebased onto the other API changes. |
|
/test api-review |
|
/test evals-api-review |
![openshift-api-review-bot[bot]](https://avatars.githubusercontent.com/u/84759374?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed config/v1/types_kmsencryption.go against OpenShift API review rules.
No issues found.
vaultAuthNamespace: optional field with proper omitted-behavior documentation, length constraints (1–4096) documented and enforced, XValidation rules (no trailing slash, no spaces, no reserved strings) match the existingvaultNamespacepattern.vaultKeyPath: required field with format/example documentation, length constraints (8–1542) documented and enforced, six XValidation rules correctly enforce the<mount>/keys/<key-name>structure, RFC 3986 character restrictions, and dot-segment exclusion.- Tombstoned fields (
transitMount,transitKey) are informational comments only — no active API surface. zz_generated.swagger_doc_generated.go: generated file consistent with source comments.
API ReviewIssues were found and posted as inline comments on this PR.
|
|
/lgtm These are known breaking changes but we are ok making these because it's tech preview. The HyperShift job is broken currently on deps being out of date, this PR shouldn't affect hypershift integration anyway AFAICT |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema, ci/prow/verify-crdify, ci/prow/verify-hypershift-integration These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, tjungblu 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 |
openshift-ci
Bot
added
the
approved
|
@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. |
|
/retest |
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.