CNTRLPLANE-2711: add vault kms plug configuration api#2805
CNTRLPLANE-2711: add vault kms plug configuration api#2805flavianmissi wants to merge 6 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@flavianmissi: This pull request references CNTRLPLANE-2711 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 task 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @flavianmissi! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
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:
📝 WalkthroughWalkthroughRemoved the KMSEncryptionProvider feature gate, its test YAML, and its feature-gate registration. Replaced AWS KMS provider with a Vault-only model: added 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
0e6ccdb to
9204eb8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 67-73: Add explicit Kubernetes validation markers to enforce the
documented bounds: annotate VaultNamespace, TransitMount, and TransitKey with
kubebuilder validation comments before each field (e.g. //
+kubebuilder:validation:MinLength=1 and //
+kubebuilder:validation:MaxLength=256) and keep the existing json tag
(json:"...,omitempty"). This ensures the CRD rejects empty strings and overly
long values for the VaultNamespace, TransitMount, and TransitKey fields.
- Around line 57-65: The VaultAddress field currently allows both http and
https; update its validation to require HTTPS only by changing the kubebuilder
XValidation rule on VaultAddress to match '^https://', update the validation
message to say "vaultAddress must be a valid URL starting with 'https://'...",
and ensure the example and any documentation reference use https (e.g.,
'https://vault.example.com:8200'); modify the comment and the
+kubebuilder:validation:XValidation line for VaultAddress in
types_kmsencryption.go accordingly.
- Around line 5-24: The change removed AWS KMS support and will break upgrades
for clusters with persisted AWS configs; restore backward compatibility by
either reintroducing AWS as a deprecated union member on KMSConfig (add
AWSKMSProvider value to KMSProviderType and an AWSKMSConfig struct as a
+unionMember with deprecation tags) and implement conversion logic in the API
conversion/webhook handlers to migrate existing AWSKMSConfig to the new model,
or implement and wire a documented controller migration path that detects
existing KMSConfig entries with AWS data and converts them to the new Vault-only
shape (or marks them as exempt) during upgrade; touch KMSConfig,
KMSProviderType, VaultKMSConfig, and add AWSKMSConfig/AWSKMSProvider identifiers
and ensure feature-gate annotations and validation rules account for
preserved/deprecated AWS entries.
🪄 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: Pro Plus
Run ID: 31c84276-1bd6-44d1-aef1-c88652b3bb69
⛔ Files ignored due to path filters (10)
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.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.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 (17)
config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yamlconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_apiserver.goconfig/v1/types_kmsencryption.gofeatures.mdfeatures/features.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.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (12)
- config/v1/tests/apiservers.config.openshift.io/KMSEncryption.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- features/features.go
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- features.md
- config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
config/v1/types_kmsencryption.go (2)
67-73:⚠️ Potential issue | 🟠 MajorAdd the length validators promised by the API contract.
The documentation states bounds for
vaultNamespace(1-256),transitMount(1-128), andtransitKey(1-128), but the kubebuilder markers are missing. This allows empty strings and arbitrarily long values to pass schema validation, causing runtime failures instead of admission-time rejection.Suggested fix for vaultNamespace (lines 67-73)
// vaultNamespace specifies the Vault namespace where the Transit secrets engine is mounted. // This is only applicable for Vault Enterprise installations. // The value can be between 1 and 256 characters. // When this field is not set, no namespace is used. // + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=256 // +optional VaultNamespace string `json:"vaultNamespace,omitempty"`Suggested fix for transitMount (lines 128-134)
// transitMount specifies the mount path of the Vault Transit engine. // The value can be between 1 and 128 characters. // When this field is not set, it defaults to "transit". // + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=128 // +kubebuilder:default="transit" // +optional TransitMount string `json:"transitMount,omitempty"`Suggested fix for transitKey (lines 136-141)
// transitKey specifies the name of the encryption key in Vault's Transit engine. // This key is used to encrypt and decrypt data. // The value must be between 1 and 128 characters. // + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=128 // +required TransitKey string `json:"transitKey,omitempty"`Also applies to: 128-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_kmsencryption.go` around lines 67 - 73, Add kubebuilder validation markers to enforce the documented length bounds on the three Vault fields: annotate VaultNamespace with +kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=256, annotate TransitMount with +kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=128, and annotate TransitKey with +kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=128 so the API server rejects empty or overly long values at admission time; ensure the markers appear immediately above the corresponding struct fields VaultNamespace, TransitMount, and TransitKey.
57-65:⚠️ Potential issue | 🟠 MajorRequire HTTPS for the Vault endpoint.
Allowing
http://enables cleartext transmission of AppRole credentials and KMS traffic. ThetlsVerify: SkipVerifyoption already covers insecure testing scenarios without disabling transport encryption entirely.Suggested fix
- // vaultAddress specifies the address of the HashiCorp Vault instance. - // The value must be a valid URL with scheme (http:// or https://) and can be up to 512 characters. + // vaultAddress specifies the address of the HashiCorp Vault instance. + // The value must be a valid HTTPS URL and can be up to 512 characters. // Example: https://vault.example.com:8200 // - // +kubebuilder:validation:XValidation:rule="self.matches(r'^https?://')",message="vaultAddress must be a valid URL starting with 'http://' or 'https://' (e.g., 'https://vault.example.com:8200')." + // +kubebuilder:validation:XValidation:rule="self.matches(r'^https://')",message="vaultAddress must be a valid HTTPS URL (e.g., 'https://vault.example.com:8200')." // +kubebuilder:validation:MaxLength=512 // +kubebuilder:validation:MinLength=1 // +required VaultAddress string `json:"vaultAddress,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_kmsencryption.go` around lines 57 - 65, The VaultAddress field currently allows both http and https; change its validation to require HTTPS only by updating the kubebuilder XValidation rule for VaultAddress (symbol: VaultAddress) to match r'^https://', adjust the validation message to state "vaultAddress must be a valid HTTPS URL starting with 'https://'" and keep the MaxLength/MinLength/required tags intact so the API rejects non-HTTPS Vault endpoints.
🧹 Nitpick comments (2)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
342-346: Validation message references KMSEncryption feature gate but rule doesn't enforce it.The message says "kms config is required when encryption type is KMS and KMSEncryption feature gate is enabled" but the CEL rule itself doesn't check the feature gate state—it just validates
kmspresence based ontype == 'KMS'. This is likely correct behavior (the feature gate controls schema presence, not this runtime rule), but the message is potentially misleading.Consider simplifying the message to match the actual rule behavior:
-- message: kms config is required when encryption type is KMS and - KMSEncryption feature gate is enabled, and forbidden otherwise +- message: kms config is required when encryption type is KMS, and forbidden otherwise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml` around lines 342 - 346, The validation message is misleading because it mentions the KMSEncryption feature gate while the CEL rule only checks self.type and self.kms; update the x-kubernetes-validations.message to reflect the actual rule (e.g., "kms config is required when encryption type is 'KMS' and forbidden otherwise") so it matches the rule for x-kubernetes-validations.rule that evaluates has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms); adjust only the message string associated with that rule.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
342-346: Same validation message inconsistency as CustomNoUpgrade CRD.The message mentions "KMSEncryption feature gate is enabled" but the CEL rule doesn't actually check feature gate state. Consider the same simplification suggested for the CustomNoUpgrade variant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml` around lines 342 - 346, The validation message for the x-kubernetes-validations entry in the apiservers-TechPreviewNoUpgrade CRD is inconsistent with the CEL rule: the message references "KMSEncryption feature gate is enabled" but the rule only checks self.type and self.kms. Update the message to match the rule (e.g., "kms config is required when encryption type is KMS and forbidden otherwise") or, if you intend to gate on a feature flag, modify the CEL rule to include that feature check; edit the x-kubernetes-validations -> message and/or the rule that uses has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms) so they are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 48-55: The validation regex for VaultKMSPluginImage rejects
repository names containing underscores; update the kubebuilder XValidation rule
on VaultKMSPluginImage to allow underscores in the repository path by changing
the character class for the path portion from [a-zA-Z0-9./-]+ to include
underscores (e.g., [a-zA-Z0-9._/-]+) so OCI-compliant names like
"registry.example.com/my_project/vault_plugin@sha256:..." pass validation while
preserving the rest of the pattern and MinLength/MaxLength constraints.
---
Duplicate comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 67-73: Add kubebuilder validation markers to enforce the
documented length bounds on the three Vault fields: annotate VaultNamespace with
+kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=256,
annotate TransitMount with +kubebuilder:validation:MinLength=1 and
+kubebuilder:validation:MaxLength=128, and annotate TransitKey with
+kubebuilder:validation:MinLength=1 and +kubebuilder:validation:MaxLength=128 so
the API server rejects empty or overly long values at admission time; ensure the
markers appear immediately above the corresponding struct fields VaultNamespace,
TransitMount, and TransitKey.
- Around line 57-65: The VaultAddress field currently allows both http and
https; change its validation to require HTTPS only by updating the kubebuilder
XValidation rule for VaultAddress (symbol: VaultAddress) to match r'^https://',
adjust the validation message to state "vaultAddress must be a valid HTTPS URL
starting with 'https://'" and keep the MaxLength/MinLength/required tags intact
so the API rejects non-HTTPS Vault endpoints.
---
Nitpick comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml`:
- Around line 342-346: The validation message is misleading because it mentions
the KMSEncryption feature gate while the CEL rule only checks self.type and
self.kms; update the x-kubernetes-validations.message to reflect the actual rule
(e.g., "kms config is required when encryption type is 'KMS' and forbidden
otherwise") so it matches the rule for x-kubernetes-validations.rule that
evaluates has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms);
adjust only the message string associated with that rule.
In
`@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml`:
- Around line 342-346: The validation message for the x-kubernetes-validations
entry in the apiservers-TechPreviewNoUpgrade CRD is inconsistent with the CEL
rule: the message references "KMSEncryption feature gate is enabled" but the
rule only checks self.type and self.kms. Update the message to match the rule
(e.g., "kms config is required when encryption type is KMS and forbidden
otherwise") or, if you intend to gate on a feature flag, modify the CEL rule to
include that feature check; edit the x-kubernetes-validations -> message and/or
the rule that uses has(self.type) && self.type == 'KMS' ? has(self.kms) :
!has(self.kms) so they are consistent.
🪄 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: Pro Plus
Run ID: 3b8d09f5-f990-4d5b-8bf2-27856efe1d7c
⛔ Files ignored due to path filters (8)
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.deepcopy.gois excluded by!**/zz_generated*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 (12)
config/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.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (8)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
9204eb8 to
adcbfe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/v1/types_kmsencryption.go (1)
5-24:⚠️ Potential issue | 🔴 CriticalPreserve an upgrade path for existing
awsKMS configs.This change removes
awsfrom the publickmsunion and provider enum without a deprecated member or any migration path in the API surface. Clusters that already persistedspec.encryption.kms.awsunder the previous gate will be rejected on upgrade once this validation becomes authoritative.Also applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_kmsencryption.go` around lines 5 - 24, The KMS union removed the previously allowed "aws" provider with no migration or deprecated field, which will reject clusters that already have spec.encryption.kms.aws; restore backward compatibility by reintroducing a deprecated/hidden union member for the AWS provider (e.g. add an Aws *AWSKMSConfig `json:"aws,omitempty"` with appropriate +optional and +deprecated markers or a FeatureGate-aware validation exception) and keep the union discriminator Type KMSProviderType accepting the old "aws" enum value (or add a deprecated enum entry) so existing persisted KMSConfig objects with Type == "AWS" and the Aws member continue to validate during upgrade while new API users are prevented from creating new AWS configs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 37-79: The CRD manifests are out of sync with the VaultKMSConfig
Go type: the Go struct now uses kmsPluginImage and a nested TLS struct
(VaultTLSConfig with caBundle and serverName) but the published CRDs still
expose vaultKMSPluginImage, flat tlsCA/tlsServerName/tlsVerify and old
http:///length validations; update the CRD schema to match the Go API by
renaming fields (vaultKMSPluginImage -> kmsPluginImage), nesting TLS under tls
with caBundle and serverName fields, removing the old http:// rule and
correcting MinLength/MaxLength rules to match KMSPluginImage and VaultAddress,
then regenerate the CRD manifests (e.g., run your controller-gen/CRD generation
or project make target) so the published YAML matches VaultKMSConfig,
KMSPluginImage, VaultAddress and TLS/VaultTLSConfig definitions.
---
Duplicate comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 5-24: The KMS union removed the previously allowed "aws" provider
with no migration or deprecated field, which will reject clusters that already
have spec.encryption.kms.aws; restore backward compatibility by reintroducing a
deprecated/hidden union member for the AWS provider (e.g. add an Aws
*AWSKMSConfig `json:"aws,omitempty"` with appropriate +optional and +deprecated
markers or a FeatureGate-aware validation exception) and keep the union
discriminator Type KMSProviderType accepting the old "aws" enum value (or add a
deprecated enum entry) so existing persisted KMSConfig objects with Type ==
"AWS" and the Aws member continue to validate during upgrade while new API users
are prevented from creating new AWS configs.
🪄 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: Pro Plus
Run ID: 6481d801-9501-491c-91d0-022980739a42
⛔ Files ignored due to path filters (8)
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.deepcopy.gois excluded by!**/zz_generated*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 (12)
config/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.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (8)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
2 times, most recently
from
42d4c42 to
bd0b2a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 56-60: The XValidation on vaultAddress (self.matches('^https://'))
only enforces the prefix and allows invalid values like "https://"; update the
validation to either enforce a full URL shape (replace the XValidation rule with
a stronger regex/pattern that requires a host and optional port/path, e.g.
require at least "https://<host>" semantics) or, if you prefer the lighter
check, change the documentation/message to state "must start with 'https://'" to
match the current rule; update the kubebuilder:XValidation rule and the
vaultAddress comment/message accordingly so they are consistent (referencing the
vaultAddress field and its XValidation rule).
- Line 50: The XValidation message still references the old field name
vaultKMSPluginImage; update the validation annotation on the kmsPluginImage
field so the message uses the new field name (kmsPluginImage) and keep the rest
of the guidance unchanged. Locate the kubebuilder XValidation tag shown and
replace "vaultKMSPluginImage" in the message with "kmsPluginImage" so admission
errors point to the correct field.
🪄 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: Pro Plus
Run ID: 55d2598e-91dd-493c-9853-e17140c344fb
⛔ Files ignored due to path filters (8)
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.deepcopy.gois excluded by!**/zz_generated*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 (12)
config/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.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
💤 Files with no reviewable changes (8)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
bd0b2a0 to
4e09913
Compare
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
3 times, most recently
from
64253d0 to
3d4bd99
Compare
|
/retitle CNTRLPLANE-2711: add vault kms plug configuration api |
openshift-ci
Bot
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Looks like the |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Yes, failures are expected and needs to be overridden. |
| } | ||
|
|
||
| // APIServerEncryption is used to encrypt sensitive resources on the cluster. | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=KMSEncryptionProvider,rule="has(self.type) && self.type == 'KMS' ? has(self.kms) : !has(self.kms)",message="kms config is required when encryption type is KMS, and forbidden otherwise" |
There was a problem hiding this comment.
Do we still want this validation in effect when the KMSEncryption gate is enabled??
There was a problem hiding this comment.
Good catch. Right now we don't want that because Tech Preview v1 relies on KMS being configured without vault - in this case users install their own plugin. We will (before GA) move away from supporting this BYO plugin configuration, and when we do we might bring this rule back.
There was a problem hiding this comment.
We should be allowed to break existing users of a tech preview feature like this. They are in a state where they cannot upgrade after enabling the TechPreviewNoUpgrade feature set.
I won't block on this, but if the desired end state is that to use KMS you have to configure something we support, why not enforce that now?
There was a problem hiding this comment.
This is for our own sake really, so we have time to change our CI tests to the new implementation without rush.
| // approleSecret references a secret in the openshift-config namespace containing | ||
| // the AppRole credentials used to authenticate with Vault. | ||
| // The secret must contain the following keys: | ||
| // - "roleID": The AppRole Role ID | ||
| // - "secretID": The AppRole Secret ID | ||
| // | ||
| // The namespace for the secret referenced by approleSecret is openshift-config. |
There was a problem hiding this comment.
If you folks feel strongly in that direction that is fine, but I think that changing this from something like:
approleSecret:
name: approle-secretto
authentication:
type: AppRole
appRole:
secret: approle-secretis a pretty low effort change to make now that saves potential headache in the future if the vault team does decide to add a new authentication method in the future.
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
3d4bd99 to
78e7fb3
Compare
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from a few very minor comments/questions this looks good to me
| // the AppRole credentials used to authenticate with Vault. | ||
| // The secret must contain two keys: "roleID" for the AppRole Role ID and "secretID" for the AppRole Secret ID. | ||
| // | ||
| // The namespace for the secret is openshift-config. |
There was a problem hiding this comment.
Seems like a repeat of information present in the first sentence?
| // +kubebuilder:validation:XValidation:rule="!self.startsWith('/')",message="transitMount cannot start with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.endsWith('/')",message="transitMount cannot end with a forward slash" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains(' ')",message="transitMount cannot contain spaces" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains('//')",message="transitMount cannot contain consecutive forward slashes" |
There was a problem hiding this comment.
Would something like transit/../../thing be something that would be allowed?
There was a problem hiding this comment.
No that's not allowed, however transit.. is valid. I've been doing some testing on the validation and I think we might need to revisit this shortly. Is it possible to merge this and revisit it in an incoming PR? I might need to sync with the Vault team to fine tune the values, online documentation is lacking.
| // +kubebuilder:validation:XValidation:rule="!self.contains(' ')",message="transitKey cannot contain spaces" | ||
| // +kubebuilder:validation:XValidation:rule="!self.contains('/')",message="transitKey cannot contain forward slashes" |
There was a problem hiding this comment.
Any other formatting constraints that are reasonable to apply beyond these?
There was a problem hiding this comment.
Probably. I'm doing some more testing and would like to iterate this in follow-up PRs if possible
|
/test verify |
|
/lgtm Known breaking changes to the API surface, this API surface was only ever present in TPNU enabled clusters and has not gone GA yet. /override ci/prow/verify-crdify |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema, 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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
the
approved
|
/retest-required |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@bertinatto: 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. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest-required |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@flavianmissi: 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-required |
5 participants
No description provided.