[WIP] CNTRLPLANE-2711: add vault kms plug configuration api#2805
Open
flavianmissi wants to merge 4 commits intoopenshift:masterfrom
Open
[WIP] CNTRLPLANE-2711: add vault kms plug configuration api#2805flavianmissi wants to merge 4 commits intoopenshift:masterfrom
flavianmissi wants to merge 4 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
Contributor
|
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 and its test YAML and feature-gate registration entries. Replaced AWS-specific KMS configuration with a Vault-based provider: added 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.