CNTRLPLANE-2711: add vault kms plug configuration api#2805
Open
flavianmissi wants to merge 6 commits intoopenshift:masterfrom
Open
CNTRLPLANE-2711: add vault kms plug configuration api#2805flavianmissi wants to merge 6 commits intoopenshift:masterfrom
flavianmissi 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
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, 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)
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.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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
Comment on lines
-24
to
-25
| // AWSKMSConfig defines the KMS config specific to AWS KMS provider | ||
| type AWSKMSConfig struct { |
Contributor
There was a problem hiding this comment.
If we tombstone the field, we will also need to tombstone this type.
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +default="transit" |
Contributor
There was a problem hiding this comment.
For configuration APIs, we normally try to avoid setting default values in this way.
Instead, we make omission of the field mean "no opinion" and have a standard terminology we use like
api/config/v1/types_ingress.go
Lines 126 to 127 in 34e6087
This allows us to modify the underlying default behavior as needed without being contractually tied to a default value.
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
2 times, most recently
from
9818cfa to
64253d0
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. |
flavianmissi
force-pushed
the
CNTRLPLANE-2711-kms-api-2
branch
from
64253d0 to
3d4bd99
Compare
Member
Author
|
/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
Member
Author
|
Looks like the |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/types_kmsencryption.go (1)
220-223: ServerName validation message may be misleading.The validation message says
serverName must be a valid DNS hostnamebut usesformat.dns1123Subdomain()validation. DNS subdomains and DNS hostnames have similar but not identical rules. For SNI purposes, this validation is acceptable, but the message could be more precise.This is a minor terminology inconsistency. The validation will work correctly in practice.
🤖 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 220 - 223, The validation message for serverName is misleading: update the XValidation message tied to format.dns1123Subdomain().validate(self).hasValue() so it refers to a DNS subdomain/DNS-1123 rule instead of "DNS hostname" (e.g., "serverName must be a valid DNS subdomain (DNS-1123)"). Locate the kubebuilder annotation on serverName in types_kmsencryption.go and replace the message text in the +kubebuilder:validation:XValidation line accordingly.
🤖 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 104-105: The OCI Image name XValidation annotation in
config/v1/types_kmsencryption.go currently only permits a single optional
namespace segment; update the kubebuilder:validation:XValidation rule (the
annotation string that starts with self.split('@')[0].matches(...)) so the
namespace portion allows multiple path segments by replacing the single optional
"/([a-zA-Z0-9-_]{0,61}/)?" segment with a repeating group that permits zero or
more "/segment" components (each segment 1–61 chars of [a-zA-Z0-9-_]) while
keeping the existing hostname[:port] and final image name constraints intact.
Ensure the modified regex still enforces anchors and overall format and retains
the separate XValidation for the "@sha256:..." suffix.
---
Nitpick comments:
In `@config/v1/types_kmsencryption.go`:
- Around line 220-223: The validation message for serverName is misleading:
update the XValidation message tied to
format.dns1123Subdomain().validate(self).hasValue() so it refers to a DNS
subdomain/DNS-1123 rule instead of "DNS hostname" (e.g., "serverName must be a
valid DNS subdomain (DNS-1123)"). Locate the kubebuilder annotation on
serverName in types_kmsencryption.go and replace the message text in the
+kubebuilder:validation:XValidation line accordingly.
🪄 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: e0986733-5de8-45b5-b628-182582fb0240
⛔ 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 (14)
config/v1/tests/apiservers.config.openshift.io/VaultKMS.yamlconfig/v1/types_apiserver.goconfig/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-Hypershift-OKD.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-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Comment on lines
+104
to
+105
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]+$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme. Short names are not allowed, the registry hostname must be included." |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current regex behavior against common OCI image patterns
echo "Testing OCI image patterns against the current regex..."
echo ""
# The current regex pattern from the code
PATTERN='^([a-zA-Z0-9-]+\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]+$'
# Test cases - valid images that should pass
VALID_IMAGES=(
"registry.example.com/vault-plugin"
"registry.example.com/hashicorp/vault-plugin"
"registry.example.com:5000/vault-plugin"
"quay.io/hashicorp/vault-plugin"
)
# Test cases - images with deeper paths that may fail
DEEP_PATH_IMAGES=(
"registry.example.com/org/team/vault-plugin"
"gcr.io/project-id/subpath/image"
"registry.example.com/a/b/c/vault-plugin"
)
echo "=== Testing valid single/double path images ==="
for img in "${VALID_IMAGES[@]}"; do
if echo "$img" | grep -qP "$PATTERN"; then
echo "✓ PASS: $img"
else
echo "✗ FAIL: $img"
fi
done
echo ""
echo "=== Testing multi-level path images ==="
for img in "${DEEP_PATH_IMAGES[@]}"; do
if echo "$img" | grep -qP "$PATTERN"; then
echo "✓ PASS: $img"
else
echo "✗ FAIL: $img (may be expected based on regex)"
fi
doneRepository: openshift/api
Length of output: 621
🏁 Script executed:
cd /tmp && git clone --quiet https://github.com/openshift/api.git && cd apiRepository: openshift/api
Length of output: 119
🏁 Script executed:
head -n 110 config/v1/types_kmsencryption.go | tail -n 10Repository: openshift/api
Length of output: 994
🏁 Script executed:
# Test the proposed regex fix
python3 << 'EOF'
import re
# Current regex
current = r'^([a-zA-Z0-9-]+\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]+$'
# Proposed fix
proposed = r'^([a-zA-Z0-9-]+\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'
test_cases = [
# Valid single/double path
("registry.example.com/vault-plugin", True),
("registry.example.com/hashicorp/vault-plugin", True),
("registry.example.com:5000/vault-plugin", True),
("quay.io/hashicorp/vault-plugin", True),
# Multi-level paths
("registry.example.com/org/team/vault-plugin", True),
("gcr.io/project-id/subpath/image", True),
("registry.example.com/a/b/c/vault-plugin", True),
# Invalid cases that should fail
("vault-plugin", False), # no registry
("localhost/vault-plugin", False), # localhost without port
("registry.example.com", False), # no image name
("registry.example.com/", False), # trailing slash only
# Edge cases
("docker.io/library/alpine", True),
("ghcr.io/owner/repo/image", True),
]
print("=" * 80)
print("CURRENT REGEX RESULTS:")
print("=" * 80)
for img, expected in test_cases:
result = bool(re.match(current, img))
status = "✓" if result == expected else "✗"
print(f"{status} {img:50} | Expected: {expected}, Got: {result}")
print("\n" + "=" * 80)
print("PROPOSED REGEX RESULTS:")
print("=" * 80)
for img, expected in test_cases:
result = bool(re.match(proposed, img))
status = "✓" if result == expected else "✗"
print(f"{status} {img:50} | Expected: {expected}, Got: {result}")
EOFRepository: openshift/api
Length of output: 2555
OCI image regex rejects valid multi-level namespace paths.
The regex at line 105 restricts namespace paths to a single optional segment (/([a-zA-Z0-9-_]{0,61}/)?). This rejects standard OCI images with multiple path components like registry.example.com/org/team/vault-plugin, gcr.io/project-id/subpath/image, and ghcr.io/owner/repo/image.
Update the regex to allow multiple path segments:
Proposed regex fix
- // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]+$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme. Short names are not allowed, the registry hostname must be included."
+ // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme. Short names are not allowed, the registry hostname must be included."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | |
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]+$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme. Short names are not allowed, the registry hostname must be included." | |
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | |
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?(/[a-zA-Z0-9-_.]+)+$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme. Short names are not allowed, the registry hostname must be included." |
🤖 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 104 - 105, The OCI Image name
XValidation annotation in config/v1/types_kmsencryption.go currently only
permits a single optional namespace segment; update the
kubebuilder:validation:XValidation rule (the annotation string that starts with
self.split('@')[0].matches(...)) so the namespace portion allows multiple path
segments by replacing the single optional "/([a-zA-Z0-9-_]{0,61}/)?" segment
with a repeating group that permits zero or more "/segment" components (each
segment 1–61 chars of [a-zA-Z0-9-_]) while keeping the existing hostname[:port]
and final image name constraints intact. Ensure the modified regex still
enforces anchors and overall format and retains the separate XValidation for the
"@sha256:..." suffix.
ardaguclu
reviewed
Apr 24, 2026
Member
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
From kms feature team point of view this looks nice to me. I've just dropped a few questions for referenced types.
| // +unionMember | ||
| // +optional | ||
| AWS *AWSKMSConfig `json:"aws,omitempty"` | ||
| Vault VaultKMSConfig `json:"vault,omitempty,omitzero"` |
Member
There was a problem hiding this comment.
Wouldn't be better to have it referenced type as AWS?
| // tls contains the TLS configuration for connecting to the Vault server. | ||
| // When this field is not set, system default TLS settings are used. | ||
| // +optional | ||
| TLS VaultTLSConfig `json:"tls,omitzero"` |
Member
There was a problem hiding this comment.
Wouldn't it be better to have this referenced type?. So that consumers can check it with nil?
| // The namespace for the secret referenced by approleSecret is openshift-config. | ||
| // | ||
| // +required | ||
| ApproleSecret VaultSecretReference `json:"approleSecret,omitzero"` |
Member
There was a problem hiding this comment.
Wouldn't it be better to have this referenced type to check with nil for existence?
ardaguclu
reviewed
Apr 24, 2026
Member
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
From kms feature team point of view this looks nice to me. I've just dropped a few questions for referenced types.
Member
Yes, failures are expected and needs to be overridden. |
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
5 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.