API-1844: Update KMSConfig to support all external kms plugins #2622
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references API-1844 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 "4.21.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. |
|
Hello @ardaguclu! Some important instructions when contributing to openshift/api: |
|
/hold |
WalkthroughReplaces AWS-specific KMS configuration with an External-only KMS model using a UNIX-domain socket Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ 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.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
do-not-merge/hold
|
@ardaguclu: This pull request references API-1844 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 "4.21.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
161-195: Endpoint CEL validation likely allows whitespace later in the string; confirm intent.
self.matches('^unix:///(@[^/\\s]+|[^@\\s].*)$')prevents leading whitespace afterunix:///, but.*can still match whitespace later (e.g.,unix:///var/run/kms plugin.sock). If whitespace should be invalid anywhere, tighten the regex (e.g., use[^\\s]*for the remainder).config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
161-195: Keep endpoint CEL validation consistent with desired whitespace policy.Same note as the non-featuregated CRD:
[^@\\s].*can allow whitespace after the first character; tighten if you want to reject whitespace anywhere in the endpoint.
🧹 Nitpick comments (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-182: Consider stricter endpoint path validation.The regex pattern allows spaces in socket paths after the first character (e.g.,
unix:///my socket) and doesn't enforce absolute filesystem paths starting with/as the description states. The pattern[^@\s].*only prevents the first character from being@or whitespace.If the intent is to strictly enforce absolute paths and prevent spaces anywhere in the path, consider updating the regex to:
^unix:///(@[^/\s]+|/[^\s]+)$This would:
- Require filesystem paths to start with
/(matching "absolute filesystem path" in the description)- Disallow spaces throughout the entire path
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-182: Redundant validation between pattern and x-kubernetes-validations.The
x-kubernetes-validationsrule at lines 180-182 checksself.startsWith('unix:///'), which is already enforced by thepatternat line 178. The pattern^unix:///(@[^/\s]+|[^@\s].*)$ensures the string starts withunix:///through the^anchor.Consider removing the redundant
startsWithvalidation or using a single comprehensive validation approach. The pattern validation alone is sufficient for the format check.Additionally, the same path validation refinement mentioned for other CRD variants applies here - consider stricter enforcement of absolute paths.
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-182: Same validation redundancy as DevPreviewNoUpgrade payload variant.The combination of
patternandx-kubernetes-validations.startsWithcreates redundant checks. The pattern already enforces theunix:///prefix requirement.config/v1/types_kmsencryption.go (1)
15-23: Endpoint validation is consistent with CRD definitions.The kubebuilder validation tags match the CRD OpenAPI schema validations. The XValidation regex correctly uses Go string escaping (
\\s).However, the same validation refinement suggestion applies: consider stricter enforcement to require absolute paths starting with
/and disallow spaces throughout the path, matching the description's intent of "absolute filesystem path."openapi/openapi.json (1)
7977-7981: Consider adding schema-level validation for endpoint format.The endpoint field description documents the required format (
unix:///path/to/socketorunix:///@abstractsocket), but this validation is not enforced by the schema itself (nopatternorformatconstraint). Moving validation to the schema level would improve API usability and catch errors earlier.Consider adding a regex pattern to validate the Unix socket endpoint format:
"endpoint": { "description": "endpoint specifies the unix domain socket endpoint...", "type": "string", "pattern": "^unix:///.+$" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlopenapi/openapi.jsonopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🔇 Additional comments (8)
config/v1/zz_generated.deepcopy.go (1)
43-51: Deepcopy change looks correct for the newKMSConfigshape (shallow-copy-safe).
**out = **inis the expected deepcopy-gen pattern here, andKMSConfig.DeepCopyIntobeing*out = *inindicates it’s safe with the current fields.config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
7-132: Test updates align with the new External/endpoint schema and cover key failure modes.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
183-194: LGTM - Clean migration to External provider model.The type field correctly defaults to
External, and makingendpointrequired ensures users provide the necessary configuration. The single-value enum is appropriate for this migration phase.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-194: Validation and field definitions are consistent with CustomNoUpgrade variant.The endpoint field validations and type field configuration match the CustomNoUpgrade CRD variant. The same optional refinement regarding stricter path validation applies here.
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-182: Consistent validation structure with other variants.The endpoint validation follows the same pattern as other payload manifest variants, including the redundant
startsWithcheck alongside the comprehensive pattern validation.config/v1/types_kmsencryption.go (2)
6-13: LGTM - Type field properly configured.The
Typefield correctly defaults toExternaland is marked optional, allowing the default behavior to take effect when unspecified. The enum restriction toExternalonly is appropriate for this migration.
27-34: LGTM - Clean enum definition.The
KMSProviderTypeenum restriction andExternalKMSProviderconstant properly reflect the External-only KMS provider model. The migration from AWS-specific types is breaking but justified by the tech preview status noted in the PR description.openapi/openapi.json (1)
9856-9856: Verify scope of policyType changes.The policyType description update (hunk 2) appears unrelated to the KMS configuration changes described in the PR objectives. Confirm whether this change is intentional and part of this PR's scope or a merge artifact.
|
@ardaguclu: This pull request references API-1844 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 "4.21.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. |
|
@ardaguclu: This pull request references API-1844 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 "4.21.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. |
config/v1/types_kmsencryption.go
Outdated
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-z0-9]+(-[a-z0-9]+)*$')",message="region must be a valid AWS region, consisting of lowercase characters, digits and hyphens (-) only." | ||
| // +kubebuilder:validation:MaxLength=120 | ||
| // +kubebuilder:validation:MinLength=9 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/\\s]+|[^@\\s].*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractsocket'" |
There was a problem hiding this comment.
Based on https://kubernetes.io/docs/tasks/administer-cluster/kms-provider/#developing-a-kms-plugin-gRPC-server-notes-kms-v2, valid values are unix:///path or unix:///@abstractsocket.
| // +kubebuilder:validation:MaxLength=64 | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-z0-9]+(-[a-z0-9]+)*$')",message="region must be a valid AWS region, consisting of lowercase characters, digits and hyphens (-) only." | ||
| // +kubebuilder:validation:MaxLength=120 |
There was a problem hiding this comment.
Based on https://man7.org/linux/man-pages/man7/unix.7.html ("Address format" section), maximum length is 108. It is less for mac. So I decided to set it to 120.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
openapi/generated_openapi/zz_generated.openapi.go (1)
15140-15146:typefield still missing Enum and DefaultThe previously flagged issue persists: the
typefield lacksEnum: []interface{}{"External"}andDefault: "External"in the schema, despite the description indicating these constraints. Fix this at the generator or source annotation level.openapi/openapi.json (1)
7973-7986: Remove invalid empty-string default for requiredendpoint(and tighten validation).
endpointis required (Line 7973-7975) but defaults to""(Line 7980), which is not a valid unix socket endpoint. Also,typereads like an enum with a default but isn’t encoded as one."required": [ "endpoint" ], "properties": { "endpoint": { "description": "endpoint specifies the unix domain socket endpoint for communicating with the external KMS plugin. The endpoint must follow the format \"unix:///path/to/socket\" or \"unix:///@abstractsocket\" for abstract sockets. The path must start with \"unix:///\" followed by either an absolute filesystem path or an abstract socket name (prefixed with @).", "type": "string", - "default": "" + "minLength": 1 + // Consider also adding a pattern for unix domain sockets if compatible with your validation strategy: + // "pattern": "^unix:///.*|^unix:///@.+" }, "type": { "description": "type defines the kind of KMS provider. Valid values are \"External\". When set to External, the KMS provider is an external plugin accessed via unix socket. When not specified, defaults to External.", - "type": "string" + "type": "string", + "enum": ["External"], + "default": "External" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
- config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.deepcopy.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlopenapi/openapi.json
🔇 Additional comments (5)
config/v1/zz_generated.deepcopy.go (1)
43-50: Confirm shallow-copy semantics forKMSConfigare safe here
**out = **inperforms a value copy of theKMSConfigstruct. This is fine ifKMSConfigonly contains value fields (e.g., strings/enums), but would be incorrect if it contains slices/maps/pointers that need deep copy. Since this is generated, please just verify the currentKMSConfigshape matches this assumption.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
170-195:endpoint.minLength: 9likely rejects validunix:///aendpoints; regex also allows whitespaceIf
unix:///ashould be valid,minLengthshould be8(not9). Also, the current rule allows whitespace after the first character in the filesystem-path branch ([^@\s].*). Consider tightening to disallow whitespace anywhere.- minLength: 9 + minLength: 8 ... - rule: self.matches('^unix:///(@[^/\s]+|[^@\s].*)$') + rule: self.matches('^unix:///(@[^/\s]+|[^@\s][^\s]*)$')Likely an incorrect or invalid review comment.
config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2198: KMSConfig swagger doc update matches the new External+endpoint model
This looks consistent with the API shift to an external KMS plugin over a unix-domain socket.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-195: The endpoint regex is correct as written;\sis properly supported in Kubernetes CEL regex (which uses RE2)Kubernetes' CEL
matches()function uses the RE2 regex dialect via cel-go, which fully supports\sas a whitespace character class. The original pattern'^unix:///(@[^/\s]+|[^@\s].*)$'is valid and works as intended. No change is needed.config/v1/types_kmsencryption.go (1)
26-34: External-only provider enum + renamed const look consistent with the new API.
RestrictingKMSProviderTypetoExternaland usingExternalKMSProvideraligns with the CRD defaulting/enum.
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
|
@ardaguclu: This pull request references API-1844 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 "4.21.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-195: Generated TechPreview CRD still has weaker/different endpoint regex than payload CRDs.This file uses
rule: self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$'), while the payload CRDs in this PR use the\s-based regex. Please update the source definition and re-run generation so TechPreviewNoUpgrade’s generated CRD matches the payload and fully rejects whitespace (not just spaces).openapi/generated_openapi/zz_generated.openapi.go (1)
15139-15156: Generated schema still doesn’t enforcetypeconstraints (enum/default) and may under-validateendpoint
type’s description says it defaults to and only allows"External", but the schema doesn’t setEnum/Default, so clients won’t get validation from OpenAPI (and may accept invalid values). Also consider encoding the unix-socket constraints forendpointasPattern/MinLength(not just in the description) if those markers exist on the Go type. Since this is generated, please fix the source markers (or generator mapping) and regenerate, rather than editing this file directly.config/v1/types_kmsencryption.go (1)
15-23: Update regex to use standard escape sequences.The endpoint validation regex currently uses literal space characters, which is inconsistent with the payload CRD manifest at
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(line 182). Use\sescape sequences for consistency and regenerate all CRD manifests.Apply this diff:
- // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'" + // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/\\s]+|[^@\\s][^\\s]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'"Note: The
omitemptytag on the required field is intentional per project linting requirements.openapi/openapi.json (2)
7973-7986: Remove invalid empty-string default for requiredendpoint(and consider constrainingtype).
endpointis required (Line 7973-7975) but has"default": ""(Line 7980), which contradicts the described unix socket format and can generate invalid defaults in clients. Also,typeclaims only"External"is valid but isn’t constrained/defaulted in the schema."required": [ "endpoint" ], "properties": { "endpoint": { "description": "endpoint specifies the unix domain socket endpoint ...", "type": "string", - "default": "" + "minLength": 1 }, "type": { "description": "type defines the kind of KMS provider. Valid values are \"External\". ... When not specified, defaults to External.", - "type": "string" + "type": "string", + "enum": ["External"], + "default": "External" } }
9855-9859: Don’t default requiredpolicyTypeto""; enforce allowed values.
The description sayspolicyTypeis required and only allows specific values (Line 9855-9856), but the schema sets"default": ""(Line 9858)."policyType": { "description": "policyType is a required field ... Allowed values are \"PublicKey\", \"FulcioCAWithRekor\", and \"PKI\". ...", "type": "string", - "default": "" + "enum": ["PublicKey", "FulcioCAWithRekor", "PKI"] },Also ensure the parent object’s
"required"list includes"policyType"(not shown in this hunk).
🧹 Nitpick comments (1)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-182: Generated file - review source instead.This is an auto-generated CRD manifest. Any validation or regex pattern issues should be addressed in the source Go file
config/v1/types_kmsencryption.go, which will then regenerate this manifest.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.deepcopy.goconfig/v1/types_kmsencryption.goopenapi/openapi.json
🔇 Additional comments (13)
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-195: Endpoint-based External KMS schema looks good in payload CRD.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-195: Payload CustomNoUpgrade CRD endpoint-based External KMS schema looks good.config/v1/zz_generated.deepcopy.go (1)
43-50: No action needed. TheKMSConfigstruct contains only value types (TypeasKMSProviderType, a string alias, andEndpointasstring), making the shallow copy**out = **insafe.config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2202: KMSConfig SwaggerDoc looks consistent with the new External+endpoint model
Docs clearly communicatetype=Externaland the two supported unix socket endpoint forms; this matches the direction of the CRD/test updates.config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (4)
8-39: Good: positive External endpoint case + clear rejection of non-unix:///...endpoints
The success path and the “missing unix scheme” failure case directly exercise the new endpoint validation behavior.
40-59: Good: test asserts kms is forbidden unlessspec.encryption.type=KMS, and endpoint is required
These cases validate the key guardrail and the required-ness ofendpoint.
60-110: Verify defaulting + error-message stability in the test harness
IfexpectedErrorrequires exact matching, messages likespec.encryption.kms.endpoint: Required value(and similar) can vary across validation layers/versions; consider ensuring the harness matches substrings (or adjust expected strings accordingly).
111-143: Endpoint edge-case coverage is solid; verify abstract socket format is truly supported end-to-end
Schema validation allowsunix:///@abstractname; please confirm the downstream KMS client/dialer path supports this format (not just admission).config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-195: CRD schema update is coherent: External-only + requiredendpointwith format validation
The required field, enum restriction, defaulting toExternal, and CEL rule appear consistent with the updated tests/docs; please just ensure the chosen endpoint regex/length bounds match the intended supported plugin endpoint conventions.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
183-195: LGTM: External provider configuration.The type field correctly defaults to External with appropriate enum constraints, and the required field change from AWS-specific to endpoint aligns with the new External KMS provider model.
config/v1/types_kmsencryption.go (3)
6-13: LGTM: Type field updated correctly.The Type field correctly defaults to External, with updated description and validation. The
omitemptytag is intentional per this project's linting requirements to ensure consistent behavior between structured and unstructured clients.
27-29: LGTM: KMSProviderType correctly updated.The enum validation correctly restricts the type to External, aligning with the removal of AWS-specific provider support.
32-33: LGTM: Constant correctly renamed.The constant rename from
AWSKMSProvidertoExternalKMSProviderwith value "External" correctly reflects the new provider model. This breaking change is acceptable given the tech preview status and absence of controllers watching these resources.
...nerated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
|
verify-crd-schema and verify-crdify failures are valid. Because we are removing aws related fields. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
config/v1/types_kmsencryption.go (1)
15-23: Tighten endpoint regex to reject all whitespace (not just spaces).
self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')rejects literal spaces but still allows tabs/newlines. If the intent is “no whitespace anywhere”, switch to\s(escaped for CEL).- // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'" + // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/\\s]+|[^@\\s][^\\s]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'"payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
170-195: CRD endpoint validation should reject all whitespace (use\\s).Mirror the stricter pattern used in the Go type validation so tabs/newlines aren’t accepted:
- rule: self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$') + rule: self.matches('^unix:///(@[^/\\s]+|[^@\\s][^\\s]*)$')config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
171-195: Alignkms.endpointCEL regex across all CRD variants (featuregated/generated/payload).
This manifest’srule: self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')excludes only literal spaces; earlier review feedback noted other CRDs use\s-style exclusion—please make them identical by updating the generator source and regenerating outputs.#!/bin/bash set -euo pipefail # Compare all CEL rules for endpoint validation across the repo rg -n "endpoint must follow the format|self\.matches\('\^unix:///" -S echo echo "Unique regex rules found:" rg -n "self\.matches\('\^unix:///[^\n]*'\)" -S | sed -E "s/.*self\.matches\('([^']+)'\).*/\1/" | sort -uconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-195: KMS endpoint/type schema update is consistent here; keep it generator-driven.
This matches the featuregated manifest (endpoint required, type default/enumExternal, same CEL regex). If you adjust the endpoint regex for consistency, ensure it’s done in the generator source and regen updates this file too.openapi/openapi.json (1)
9854-9858: Remove invalidpolicyTypedefault""and model allowed values explicitly.
This still contradicts the description (“required” + allowed values) and can lead to invalid objects being considered “default-valid” by generators."policyType": { "description": "policyType is a required field specifies the type of the policy for verification. This field must correspond to how the policy was generated. Allowed values are \"PublicKey\", \"FulcioCAWithRekor\", and \"PKI\". When set to \"PublicKey\", the policy relies on a sigstore publicKey and may optionally use a Rekor verification. When set to \"FulcioCAWithRekor\", the policy is based on the Fulcio certification and incorporates a Rekor verification. When set to \"PKI\", the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI).", "type": "string", - "default": "" + "enum": ["PublicKey", "FulcioCAWithRekor", "PKI"] },Also verify the enclosing schema’s
"required"includes"policyType"(outside this hunk).
🧹 Nitpick comments (1)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
8-143: Test updates cover External+endpoint and key failure modes well.Optional: add one negative case for
endpointcontaining\t/\nif you adopt the\\s-based regex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/types_kmsencryption.goopenapi/openapi.jsonconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.go
🔇 Additional comments (8)
config/v1/types_kmsencryption.go (1)
6-13: External-only provider typing/defaulting looks consistent.Also applies to: 26-34
config/v1/zz_generated.deepcopy.go (1)
43-50: DeepCopyInto KMSConfig shallow-copy is correct with current value-only fields.config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2198: KMSConfig SwaggerDoc looks aligned with the new External+endpoint API surface.
Docs now correctly describetype=Externaland the unix socketendpointformats. Since this is generated, just ensure the source-of-truth type comments/validation inconfig/v1/types_kmsencryption.gomatch this wording before regen.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
170-195: KMS schema update looks consistent (endpoint + External-only type); keep validation in sync after source change.
This matches the TechPreview manifest, including the endpoint validation rule; if you adjust the regex in the Go source, regenerate this file too to keep DevPreview/TechPreview identical.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
170-195: Payload CRD aligns with generated schema; ensure regeneration after any regex tweak.
Given this is a payload manifest, please regenerate from the same source change (if you move to\s) to avoid subtle drift between payload vs zz_generated manifests.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
170-195: Same endpoint validation consideration applies here (space-only exclusion).
If the intent is “no whitespace”, prefer a source-level\sregex and regenerate to update this payload file as well.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
170-195: Endpoint regex blocks spaces but not tabs; however, this is not a practical concern.The regex
^unix:///(@[^/ ]+|[^@ ][^ ]*)$does match only literal spaces and not other whitespace. However:
- Filesystem paths cannot contain newlines (kernel restriction), and tabs in socket paths are extraordinarily rare in practice.
- The suggested
\sfix is problematic: it would match newlines, which could break YAML multiline string parsing that occurs before validation.- The test suite (in
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml) explicitly validates rejection of spaces in paths, confirming the intent is deliberate.- The codebase pattern: No other CEL validations use
\s; explicit character classes like[0-9]are preferred, suggesting intentional specificity.The current regex is fit for purpose. If stricter whitespace handling is desired, consider documenting the intentional choice in the source file comment rather than introducing a regex that could have unintended side effects.
openapi/openapi.json (1)
7971-7986: ConstrainKMSConfig.typeto the documented value (and ensure endpoint validation exists).
Right now"type"is an unconstrained string while the description says only"External"is valid and defaulted when omitted. Consider enforcing that withenum(and optionallydefault) so generated clients/validation can’t accept arbitrary values. Also,endpointis required but shown without anypattern/length constraints—ensure those validations are actually present in this schema if they’re intended."properties": { "endpoint": { "description": "endpoint specifies the unix domain socket endpoint for communicating with the external KMS plugin. The endpoint must follow the format \"unix:///path/to/socket\" for filesystem-based sockets or \"unix:///@abstractname\" for abstract sockets.", "type": "string" }, "type": { "description": "type defines the kind of KMS provider. Valid values are \"External\". When set to External, the KMS provider is an external plugin accessed via unix socket. When not specified, defaults to External.", - "type": "string" + "type": "string", + "enum": ["External"], + "default": "External" } }
|
/cc @flavianmissi |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
openapi/generated_openapi/zz_generated.openapi.go (1)
15140-15146: [Duplicate] OpenAPI schema still missing Enum and Default fortypefieldThe generated schema lacks
Enum: ["External"]andDefault: "External"despite the description stating "defaults to External". This is the same issue flagged in the previous review.The source markers (
+kubebuilder:default=Externaland+kubebuilder:validation:Enum=External) are not propagating to the generated OpenAPI schema, weakening the published API contract. Clients cannot rely on schema-level validation or defaulting.Verify that the source markers are correctly applied and regenerate the schema. If the type-level Enum marker is not being recognized, consider applying it directly to the field.
config/v1/types_kmsencryption.go (1)
16-24: TightenendpointCEL regex to match documentedunix:///path/unix:///@namemore precisely. The current pattern (a) allowsunix:////...via the filesystem branch and (b) only blocks literal spaces, not all whitespace.- // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'" + // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/\\s]+|[^@/\\s][^\\s]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'"After updating the Go type, regenerate the CRD/payload manifests so all three stay in sync. (CEL regex escape semantics vary by engine/version, so please double-check this against the apiserver’s CEL implementation.)
openapi/openapi.json (1)
9854-9858: policyType:default: ""contradicts “required field” and allowed valuesThis should either have no default (since it’s required) or be constrained to the allowed values (and default to one of them only if you truly want a default).
"policyType": { "description": "policyType is a required field specifies the type of the policy for verification. This field must correspond to how the policy was generated. Allowed values are \"PublicKey\", \"FulcioCAWithRekor\", and \"PKI\". When set to \"PublicKey\", the policy relies on a sigstore publicKey and may optionally use a Rekor verification. When set to \"FulcioCAWithRekor\", the policy is based on the Fulcio certification and incorporates a Rekor verification. When set to \"PKI\", the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI).", - "type": "string", - "default": "" + "type": "string", + "enum": ["PublicKey", "FulcioCAWithRekor", "PKI"] },Also ensure the parent schema’s
"required"includes"policyType"if it’s truly mandatory.
🧹 Nitpick comments (3)
config/v1/zz_generated.deepcopy.go (1)
43-51: Keep DeepCopy semantics forAPIServerEncryption.KMS(avoid**out = **in). The current code is equivalent today, but it’s easy to accidentally lose deep-copy semantics ifKMSConfiglater gains reference fields. Prefer the standard pattern(*in).DeepCopyInto(*out)(via generator source), even if it compiles to the same thing now.func (in *APIServerEncryption) DeepCopyInto(out *APIServerEncryption) { *out = *in if in.KMS != nil { in, out := &in.KMS, &out.KMS *out = new(KMSConfig) - **out = **in + (*in).DeepCopyInto(*out) } return }config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2198: Align Swagger text with schema constraints (required/wording).
Swagger doc says “unix socket” and doesn’t stateendpointis required; CRD/schema typically uses “unix domain socket” and makesendpointrequired + validated. Consider tweaking the strings for consistency.var map_KMSConfig = map[string]string{ "": "KMSConfig defines the configuration for the KMS instance that will be used with KMSEncryptionProvider encryption", - "type": "type defines the kind of KMS provider. Valid values are \"External\". When set to External, the KMS provider is an external plugin accessed via unix socket. When not specified, defaults to External.", - "endpoint": "endpoint specifies the unix domain socket endpoint for communicating with the external KMS plugin. The endpoint must follow the format \"unix:///path/to/socket\" for filesystem-based sockets or \"unix:///@abstractname\" for abstract sockets.", + "type": "type defines the kind of KMS provider. Valid values are \"External\". When set to External, the KMS provider is an external plugin accessed via a unix domain socket. When not specified, defaults to External.", + "endpoint": "endpoint specifies the unix domain socket endpoint for communicating with the external KMS plugin (required). The endpoint must follow the format \"unix:///path/to/socket\" for filesystem-based sockets or \"unix:///@abstractname\" for abstract sockets.", }openapi/generated_openapi/zz_generated.openapi.go (1)
15147-15155: Consider adding Pattern validation forendpointformatThe description specifies that the endpoint must follow the format
"unix:///path/to/socket"or"unix:///@abstractname", but the schema lacks aPatternfield to enforce this at validation time.Without a pattern constraint, invalid endpoints (e.g.,
"http://example.com"or"/path/to/socket") could pass schema validation, deferring errors to runtime.Consider adding a pattern in the source type definition, such as:
+kubebuilder:validation:Pattern=`^unix://(/|@).+$`Then regenerate the OpenAPI schema to include the
Patternfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.deepcopy.goconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.swagger_doc_generated.go
🔇 Additional comments (8)
config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
161-195: KMS schema update looks consistent (External-only + endpoint required).config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
8-143: CRD validation tests are solid, but error-message matching may be brittle across apiserver versions. If this test harness expects exact strings, consider using regexp/contains (if supported) or include the full field path prefix that kube-apiserver typically returns (e.g.,spec.encryption.kms.endpoint: Invalid value: ...).config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-195: KMS endpoint validation looks coherent; confirm regex matches intended formats.
endpointis required and validated with^unix:///(@[^/ ]+|[^@ ][^ ]*)$, which appears to coverunix:///pathandunix:///@abstractnameas documented. Please sanity-check edge cases you want to allow/forbid (e.g., abstract names with/, or any whitespace) since this is user-facing API validation.config/v1/types_kmsencryption.go (1)
6-15: External-only provider defaulting/enum looks consistent.Also applies to: 31-34
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-195: Generated CRD changes reflect the newkms.endpoint+ External-only provider model. Please keep this file regenerated fromconfig/v1/types_kmsencryption.go(don’t hand-edit).config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-195: Looks consistent with the TechPreviewNoUpgrade CRD:endpointrequired + External-only type/default (generated output).payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-195: Payload-manifest CRD matches the new schema (endpoint required; External-only). If you adjust the Go regex, re-run generation so payload + zz_generated stay aligned.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-195: CustomNoUpgrade payload CRD update is consistent with the other variants (endpoint required + External-only).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
8-143: Tests appear to usespec.encryption.kms.type, but the API/CRDs usespec.encryption.kms.managementModel.
Unless there’s anothertypefield elsewhere (not shown in the provided CRDs), these tests look out of sync with the new field name and defaulting expectations.- kms: - type: External - endpoint: unix:///var/run/kmsplugin/socket.sock + kms: + managementModel: External + endpoint: unix:///var/run/kmsplugin/socket.sock @@ - kms: - endpoint: unix:///var/run/kmsplugin/socket.sock + kms: + endpoint: unix:///var/run/kmsplugin/socket.sock @@ - kms: - type: External - endpoint: unix:///var/run/kmsplugin/socket.sock + kms: + managementModel: External + endpoint: unix:///var/run/kmsplugin/socket.sock @@ - kms: - type: External - endpoint: unix:///@abstractsocket + kms: + managementModel: External + endpoint: unix:///@abstractsocket(Apply similarly throughout, and rename “default type” wording to “default managementModel”.)
♻️ Duplicate comments (4)
config/v1/types_kmsencryption.go (1)
6-24: TightenEndpointXValidation to reject all whitespace (not just spaces).Current regex (
[^ ]) blocks only literal spaces; tabs, newlines, and other whitespace would still pass. Use\s-based exclusion to reject all whitespace:- // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'" + // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///(@[^/\\s]+|[^@\\s][^\\s]*)$')",message="endpoint must follow the format 'unix:///path' or 'unix:///@abstractname'"openapi/generated_openapi/zz_generated.openapi.go (1)
15140-15156: OpenAPI contract is weaker than the described constraints (missing enum/default + endpoint validation).
managementModelis described as only allowing"External", but the schema shown doesn’t enforce it (noEnum/Default).endpointis required but has noPattern/MinLength, so invalid values can still pass OpenAPI validation. Please ensure the kubebuilder markers are applied at the field level (not only the underlying type) and then regenerate this file (don’t patch generated output directly).#!/bin/bash set -euo pipefail echo "=== Generated KMSConfig schema (context) ===" sed -n '15130,15170p' openapi/generated_openapi/zz_generated.openapi.go echo -e "\n=== Source markers for KMSConfig fields (locate file(s)) ===" rg -n --type go 'type\s+KMSConfig\b|KMSConfig\s+struct' . echo -e "\n=== Check for field-level markers (Enum/default/pattern/minlength) ===" rg -n --type go '\+kubebuilder:validation:Enum=|\+kubebuilder:default=|\+kubebuilder:validation:Pattern=|\+kubebuilder:validation:MinLength=' .openapi/openapi.json (2)
7971-7985: Add OpenAPI validation forendpointand constrainmanagementModeltoExternal.Right now
endpointis required but can still be""or non-unix:///...strings, andmanagementModelis documented as External-only but unconstrained."properties": { "endpoint": { "description": "endpoint specifies the unix domain socket endpoint for communicating with the external KMS plugin. The endpoint must follow the format \"unix:///path/to/socket\" for filesystem-based sockets or \"unix:///@abstractname\" for abstract sockets.", - "type": "string" + "type": "string", + "minLength": 1, + "pattern": "^unix:///((@[^/\\x00\\n\\r]+)|([^/\\x00\\n\\r]+(/[^/\\x00\\n\\r]+)*))$" }, "managementModel": { "description": "type defines how KMS plugins are managed. Valid values are \"External\". When set to External, encryption keys are managed by a user-deployed KMS plugin that communicates via unix domain socket using KMS V2 API.", - "type": "string" + "type": "string", + "enum": ["External"], + "default": "External" } }
9854-9858: Remove invalidpolicyTypedefault""and enforce allowed values.The schema currently defaults
policyTypeto"", which contradicts the documented allowed values."policyType": { "description": "policyType is a required field specifies the type of the policy for verification. This field must correspond to how the policy was generated. Allowed values are \"PublicKey\", \"FulcioCAWithRekor\", and \"PKI\". When set to \"PublicKey\", the policy relies on a sigstore publicKey and may optionally use a Rekor verification. When set to \"FulcioCAWithRekor\", the policy is based on the Fulcio certification and incorporates a Rekor verification. When set to \"PKI\", the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI).", "type": "string", - "default": "" + "enum": ["PublicKey", "FulcioCAWithRekor", "PKI"] },Also ensure the parent schema’s
"required": [...]includes"policyType"if it’s truly required.
🧹 Nitpick comments (2)
config/v1/types_kmsencryption.go (1)
6-14: Fix field docs: description says “type defines …” but the field ismanagementModel.
This wording propagates into the CRD schema descriptions (seen in the provided CRDs). Consider updating the comment to avoid user confusion.Also applies to: 27-28
config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2198: SwaggerDoc text:managementModeldescription incorrectly starts with “type defines …”In
map_KMSConfig, themanagementModelfield’s description begins with “type defines…”, which is confusing now thattypeis not that field. Suggest updating the wording to “managementModel defines…” for clarity/accuracy (in the generator source).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/zz_generated.deepcopy.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/openapi.jsonconfig/v1/zz_generated.crd-manifests/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.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
🔇 Additional comments (6)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-195: Generated CRD: ensure it’s regenerated after tighteningendpointvalidation / docs in Go types.
If you adopt\s-based whitespace exclusion (recommended), this file should update via regeneration rather than manual edits.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
161-195: Same as CustomNoUpgrade: treat as generated output; regen after anyendpointregex/doc adjustments inconfig/v1/types_kmsencryption.go.config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
171-195: Generated manifest; no additional concerns beyond theendpointwhitespace validation already raised inconfig/v1/types_kmsencryption.go.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-195: Payload artifact: if you changeEndpointvalidation/docs in the Go type, regenerate payload-manifests to keep them byte-identical with generated CRDs.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-195: KMS endpoint configuration is consistent and correct.The
endpointfield is properly configured as a required string with unix socket format validation. The regex pattern^unix:///(@[^/ ]+|[^@ ][^ ]*)$correctly validates both filesystem paths (unix:///path/to/socket) and abstract sockets (unix:///@socketname) while rejecting any paths containing whitespace. Length constraints (minLength 9, maxLength 120) and themanagementModeldefault to External align with the shift from AWS-centric to External KMS via unix socket.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-195: Regex patterns now consistent across both CRD manifests.Both the TechPreviewNoUpgrade and DevPreviewNoUpgrade CRD manifests now use the identical regex pattern
^unix:///(@[^/ ]+|[^@ ][^ ]*)$with literal space characters. This resolves the prior inconsistency flagged in past reviews and ensures both feature-gated variants enforce identical endpoint validation. The pattern correctly prevents whitespace in socket paths while supporting both filesystem-based and abstract socket formats.
..._generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
161-196: Tightenkms.endpointregex if you want to forbidunix:////...Current CEL rule allows a path that begins with
/afterunix:///(e.g.,unix:////tmp/sock), which is arguably outside the documented formats. If you intend to only allowunix:///path...(where path starts immediately after the 3rd slash) orunix:///@abstract..., consider adjusting the regex accordingly.
♻️ Duplicate comments (5)
openapi/openapi.json (1)
9854-9858: policyType: empty-string default conflicts with “required” + allowed-values contractThis still has
"default": ""while the description says it’s required and constrained toPublicKey|FulcioCAWithRekor|PKI. Prefer removing the empty default and adding anenum(and ensure the parent schema actually requirespolicyType).payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-196: Samekms.endpointregex permissiveness as the DevPreviewNoUpgrade CRD
See the earlier comment aboutunix:////...being accepted by the current regex.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-182: Endpoint validation still allows non-space whitespace characters despite partial fix.The validation regex
self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')uses literal space characters instead of the\swhitespace class, which blocks spaces but still permits tabs, newlines, and other whitespace. Unix socket paths should reject all whitespace. The pattern[^@ ][^ ]*permits paths likeunix:///var/run/kms\t.sock(containing a tab).This is a continuation of the earlier whitespace validation issue. While the developer changed from
\sto literal spaces, the fundamental problem persists.Recommend updating the regex to properly exclude all whitespace:
-rule: self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$') +rule: self.matches('^unix:///(@[^/\s]+|[^@\s][^\s]*)$')This ensures the validation uses the proper
\scharacter class (or equivalent escaping appropriate for CEL in CRD validation rules) to reject all whitespace characters, not just literal spaces.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-182: Endpoint validation still allows non-space whitespace characters (payload manifest).Same issue as identified in the generated CRD: the regex
self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')only blocks literal spaces but permits tabs and newlines.Apply the same fix to ensure consistency across manifests:
-rule: self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$') +rule: self.matches('^unix:///(@[^/\s]+|[^@\s][^\s]*)$')config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-182: Endpoint validation whitespace issue present in CustomNoUpgrade variant.The endpoint validation regex
self.matches('^unix:///(@[^/ ]+|[^@ ][^ ]*)$')has the same limitation as the TechPreviewNoUpgrade variants: it only blocks literal spaces, permitting tabs and newlines. Since this is a generated CRD, fix the issue at the source inconfig/v1/types_kmsencryption.goto propagate the correct validation pattern across all generated CRD manifests.The source-level fix should use proper whitespace character classes:
// In types_kmsencryption.go, update the CEL validation rule rule="self.matches('^unix:///(@[^/\\s]+|[^@\\s][^\\s]*)$')"This single fix will regenerate all three CRD variants (TechPreviewNoUpgrade, DevPreviewNoUpgrade, CustomNoUpgrade) with the corrected validation rule.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.deepcopy.goconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlopenapi/openapi.json
🔇 Additional comments (5)
config/v1/types_kmsencryption.go (1)
5-34: LGTM! Clean refactoring to External-only KMS model.The restructuring from AWS-specific configuration to a streamlined External endpoint-based model is well-executed:
- ManagementModel: Properly constrained to
Externalwith appropriate enum validation and defaulting- Endpoint: Comprehensive validation with regex pattern that correctly handles both filesystem (
unix:///path) and abstract socket (unix:///@abstractname) formats, rejecting spaces and slashes in abstract sockets- Length constraints: MinLength=9 and MaxLength=120 are appropriate for unix socket paths
- Documentation: Clear and concise field descriptions
The use of
omitemptywith+requiredis correct per the project's linter configuration (as confirmed in past review discussions).config/v1/zz_generated.deepcopy.go (1)
43-51: LGTM! Deepcopy logic correctly simplified.The change from deep copy to shallow copy (
**out = **in) for the KMS field is appropriate. SinceKMSConfignow only contains simple string fields (ManagementModelandEndpoint) with no pointers or nested structures, a shallow copy is both correct and more efficient.config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
7-143: Excellent test coverage for the new KMS configuration.The test suite comprehensively validates the External endpoint-based KMS configuration across 10 scenarios:
Positive cases:
- Valid filesystem socket paths (
unix:///var/run/kmsplugin/socket.sock)- Valid abstract sockets (
unix:///@abstractsocket)- Defaulting behavior (External management model when only endpoint is provided)
Negative cases:
- Format violations (missing
unix:///prefix, spaces in paths, slashes in abstract sockets)- Missing required fields (empty config, missing endpoint)
- Length constraints (empty endpoint string triggers MinLength validation)
- Type mismatch (KMS config with aescbc encryption type)
The tests align perfectly with the validation rules in
types_kmsencryption.goand provide strong confidence in the API behavior.config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2202: SwaggerDoc updates forKMSConfiglook consistent with the new External endpoint modelconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-195: Approve KMS configuration restructuring.The migration from AWS-specific fields (
aws.keyARN,aws.region) to the endpoint-based external KMS model is well-structured. Theendpointfield is properly marked as required, validations are in place with reasonable length constraints (minLength 9 forunix:///a, maxLength 120), and default values are sensible. The overall schema change aligns with the PR objective to support only external KMS plugins.
..._generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
|
I ran Claude |
|
Looking great, thanks Arda! /lgtm |
|
Scheduling tests matching the |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
openapi/generated_openapi/zz_generated.openapi.go (1)
15140-15155: Validation constraints still missing from generated schema.The field names have been correctly updated (
type→managementModel,aws→endpoint), but the validation gaps identified in previous reviews persist:
managementModel(Lines 15140-15146): Description states "Valid values are "External"" but lacks:
Enum: []interface{}{"External"}to enforce the single valid valueDefault: "External"to provide the default
endpoint(Lines 15147-15153): Description specifies unix socket format patterns but lacks:
Pattern: "^unix:///.*$|^unix:///@.*$"to enforce the formatSince this is a generated file, add the missing kubebuilder markers to the source type definition in
config/v1/types_kmsencryption.go:
+kubebuilder:validation:Enum=Externalon themanagementModelfield+kubebuilder:default=Externalon themanagementModelfield+kubebuilder:validation:Pattern=^unix:///.*$|^unix:///@.*$on theendpointfieldThen regenerate the OpenAPI schema.
openapi/openapi.json (1)
7977-7984: Missing validation constraints issue previously raised.The prior review comment at lines 7973-7985 flagged that the OpenAPI schema lacks the validation constraints present in the source code (
config/v1/types_kmsencryption.go):
endpoint: missingminLength,maxLength, andpatternconstraintsmanagementModel: missingenumanddefaultvaluesEnsure the OpenAPI generation properly reflects the kubebuilder validation markers.
🧹 Nitpick comments (1)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
161-195: KMS endpoint + managementModel schema looks solidThe unix-domain-socket
endpointvalidation andmanagementModelenum/default cleanly enforce the external KMS plugin model, and the CEL regex correctly rejects abstract sockets and obvious bad formats. One possible (non-blocking) refinement would be to alignmaxLengthwith the underlying unix socket path limit to fail clearly at validation time rather than later in the KMS client, but the current bound is acceptable as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(4 hunks)config/v1/types_kmsencryption.go(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 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-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/types_kmsencryption.gopayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.deepcopy.goopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlconfig/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlopenapi/openapi.json
🔇 Additional comments (10)
config/v1/zz_generated.deepcopy.go (1)
42-51: DeepCopy change forKMSConfigis correctUsing
new(KMSConfig)plus**out = **inis appropriate now thatKMSConfigonly has value fields, and it avoids pointer aliasing. Generated code remains consistent with other simple structs.config/v1/types_kmsencryption.go (1)
5-25: KMSConfig shape and validation look consistent and sound
ManagementModel+ requiredEndpointwith the unix‑socket regex, length bounds, default, and docs all line up with the CRDs and tests. Theomitemptyon required fields matches this repo’s linter conventions.config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
171-194: Feature‑gated CRD kms schema correctly mirrors the Go type
endpointandmanagementModeldefinitions (including requiredness, defaults, and CEL regex) are in sync withKMSConfigand the tests, so CRD and server‑side behavior stay aligned.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-194: TechPreview CRD kms block is consistent with source and other CRDsThe kms
endpointandmanagementModelfields, validations, and required list match the Go type and the feature‑gated CRD, keeping the API coherent across feature sets.config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
8-133: KMS tests comprehensively exercise the new endpoint semanticsThe added cases cover valid configuration, defaulting of
managementModel, and the main failure modes (scheme, missing/empty endpoint, spaces, abstract sockets, and invalid pairing with non‑KMS type), which should give good confidence in the new validation logic.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
161-195: Consistent KMS schema across feature setsThe
kmsblock (endpoint + managementModel + required:endpoint and validations) matches the DevPreview CRD, which keeps behavior consistent across feature sets and avoids drift. No additional issues spotted here.config/v1/zz_generated.swagger_doc_generated.go (1)
2194-2198: KMSConfig Swagger docs aligned with new External KMS modelThe updated
map_KMSConfigentries correctly describe the newmanagementModelandendpointfields and match the CRD semantics (external plugin viaunix:///path, no abstract sockets). This keeps generated Swagger/OpenAPI in sync with the refactored API.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (3)
171-181: Endpoint validation correctly implements UNIX socket format requirements.The regex validation
^unix:///[^@ ][^ ]*$properly:
- Enforces the
unix:///prefix format- Prevents abstract Linux sockets (starting with
@) as documented- Rejects spaces in paths to avoid parsing ambiguities
- Works with the length constraints (minLength 9 ensures at least one character after the prefix)
This correctly transitions from AWS-specific fields to a generic External KMS endpoint model.
182-191: LGTM! Management model correctly restricted to External.The default value of
Externaland the enum constraint align with the transition away from AWS-specific KMS configurations. The description accurately reflects the External model using user-deployed KMS plugins with UNIX domain sockets and KMS V2 API.
192-194: Required field correctly updated for External KMS model.Making
endpointrequired (instead of the removed AWS-specific fields) is appropriate for the External KMS configuration. This ensures users must specify the UNIX socket endpoint when configuring KMS encryption.
|
@flavianmissi I've excluded abstract Linux paths from the validations (as we agreed upon). Could you PTAL?. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: flavianmissi 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 |
|
/test e2e-aws-ovn |
|
/lgtm cancel @ardaguclu @flavianmissi While I appreciate the team review and LGTM, please don't Will get you a reviewer assigned |
Thank you for the comment. This doesn't look like the flow in other OCP repositories but the motivation makes sense to me. As prerequisite we need to merge our EP openshift/enhancements#1900 first (@JoelSpeed I've added you as api-approver, but we need to complete our reviews within the team first). |
|
No worries @ardaguclu, it's a different process that we are trialing and I expect more repos to take on in the future. I'm hoping we can make it more obvious later @yuqi-zhang is one of our API reviewers who's training up, he will take a first review pass here and on the EP, and once he's happy I'll take a look |
|
@ardaguclu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // | ||
| // +unionDiscriminator | ||
| // +required | ||
| Type KMSProviderType `json:"type"` |
There was a problem hiding this comment.
For serialization safety and to adhere to v1 API guarantees, we do not allow removal of any fields directly, even if it was only accessible in tech preview. Instead, we ask to tombstone them (removal with a comment), see some examples #2256 and #2576
This also means that the field name may not be used again, so we should be certain that we're ok with the removal.
There was a problem hiding this comment.
Thank you for review. That makes sense. I'll update this PR based on your comments, once we finalize the design.
| // | ||
| // +unionMember | ||
| // +kubebuilder:validation:Enum=External | ||
| // +kubebuilder:default=External |
There was a problem hiding this comment.
Could you help me understand why you'd like a default to be set here? We generally try not to set defaults via the API but instead through controllers and document the default behaviour via godocs, allowing for flexibility in the future. Since we only have one optional for now, is anything depending on this field to be explicitly set?
For additional context, see: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#defaulting
| // `<region-prefix>-<region-name>-<number>`, e.g.: `us-east-1`. | ||
| // Only lowercase letters and hyphens followed by numbers are allowed. | ||
| // endpoint specifies the UNIX domain socket endpoint for communicating with the external KMS plugin. | ||
| // The endpoint must follow the format "unix:///path". |
There was a problem hiding this comment.
| // The endpoint must follow the format "unix:///path". | |
| // The endpoint must follow the format "unix:///path" and be between 9 and 120 characters in length. |
| // type defines the kind of platform for the KMS provider. | ||
| // Available provider types are AWS only. | ||
| // managementModel defines how KMS plugins are managed. | ||
| // Valid values are "External". |
There was a problem hiding this comment.
Out of curiosity, what would potentially be other possible values in the future? Would there be any cloud-provided specific plugins supported here since you mentioned that AWS is obsolete? Just trying to understand what would be the api changes would look like in the future.
5 participants
KMSConfigis gated behindKMSEncryptionProviderfeature gate which is in tech preview. However, currently there isn't any controller listeningKMSConfigandKMSEncryptionProvider. So that gives us an opportunity to change the API fields in a breaking way, according to the newest updates in this feature (AWS related kms configurations are obsolete, since we won't support it internally)Note: We'll have to backport these changes into 4.21.