API-1844: Update KMSConfig to allow manualy managed 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 |
📝 WalkthroughWalkthroughThe PR adds a Manual KMS provider (type and ManualKMSConfig) and an External endpoint-based KMS model, while marking AWS KMS fields/types deprecated. KMSConfig gains a 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. |
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
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
Outdated
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).
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 |
| // | ||
| // +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.
There was a problem hiding this comment.
Instead of directly removing it, I've marked fields as deprecated for now. In next release I'm planning to remove them entirely (by tombstoning as suggested ^^)
openshift-ci
bot
added
size/L
|
[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 |
ardaguclu
changed the title
|
@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 story to target the "4.22.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: 16
🤖 Fix all issues with AI agents
In @openapi/generated_openapi/zz_generated.openapi.go:
- Line 9177: The Description string for AWSKMSConfig contains a grammatical
error ("listening this resource type"); update the source that generates this
Description (the AWSKMSConfig description definition) to either "listening to
this resource type" or better: "No controller watches this resource type; we
plan to remove it in the next release." Change the Description for AWSKMSConfig
accordingly in the authoritative source so regenerated openapi code includes the
corrected deprecation notice.
- Around line 16176-16183: The discriminator mapping for the KMS encryption type
is incorrect: in the generated OpenAPI map inside zz_generated.openapi.go the
discriminator entry maps "name" to "Name" but the valid type enum values are
"AWS" and "Manual"; update the discriminator map (the map with key
"discriminator": "type" and "fields-to-discriminateBy") to map "aws" -> "AWS"
and change "name" -> "Manual" so the "name" field discriminates to the Manual
type.
In @openapi/openapi.json:
- Around line 4456-4457: Fix the minor grammar in the deprecation sentence for
the schema "com.github.openshift.api.config.v1.AWSKMSConfig": change "listening
this resource type" to "listening to this resource type" (or "watching this
resource type") in the description string so the deprecation note reads
correctly.
- Around line 4582-4585: The OpenAPI schema for the "name" property documents
that it must be a non-empty string and <=256 chars but lacks enforcement; update
the "name" property in the schema to include validation constraints by adding
"minLength": 1 and "maxLength": 256 to the existing "name" object so the spec
enforces the documented rules.
- Around line 6129-6140: The "conditions" array schema lacks the declared item
limit: add "maxItems": 1 to the "conditions" property (the array that references
"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition") so the schema
enforces the description "conditions must not contain more than one entry."
Ensure the "conditions" object keeps its existing "items",
"x-kubernetes-list-map-keys", and "x-kubernetes-list-type" entries and simply
adds the top-level "maxItems": 1 constraint.
- Around line 6094-6102: The OpenAPI schema for the "riskNames" property is
missing the documented validation constraints; update the "riskNames" array
schema (property name: riskNames) to include "maxItems: 500" on the array and
add "maxLength: 256" to the array "items" schema (the string item definition) so
each entry is limited to 256 characters and the list to 500 entries, preserving
the existing "type": "array", "items": { "type": "string" } and
"x-kubernetes-list-type": "set".
- Around line 11644-11655: The schema for the acceptRisks array is missing the
maxItems constraint declared in its description; add "maxItems": 1000 to the
acceptRisks definition (the same object that contains "description", "type",
"items", "x-kubernetes-list-map-keys", and "x-kubernetes-list-type") so the
schema enforces the documented limit of no more than 1000 entries for
acceptRisks.
- Around line 8547-8559: Add JSON Schema length constraints to the "name"
property by adding "minLength": 1 and "maxLength": 80 to enforce the documented
1–80 character limit; correct the grammar in the "aws" description by changing
"listening this resource type" to "listening to this resource type"; and
reconcile the "type" description with the discriminator entries by either
removing "aws" from the discriminator (if AWS support is being removed) or
updating the "type" description to note that "aws" is retained for backward
compatibility—ensure the discriminator and the "type" description are
consistent.
- Around line 5875-5886: The schema for the property conditionalUpdateRisks (in
the OpenAPI definition for com.github.openshift.api.config.v1) documents a
maximum of 500 entries but does not enforce it; add "maxItems: 500" to the
conditionalUpdateRisks array schema (the object containing "type": "array",
"items": { "$ref":
"#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" }) so
the schema validation matches the description.
In
@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml:
- Around line 220-227: The CRD's "type" field is inconsistent: its description
says "Manual only" while the enum lists "AWS" and "Manual"; update the "type"
field so schema and docs match—either remove "AWS" from the enum to enforce
Manual-only, or change the description to state both allowed values (e.g.,
"Available provider types are AWS (deprecated) and Manual") and mark AWS as
deprecated if applicable; modify the enum or the description in the "type"
definition to make them consistent (refer to the "type" field under the KMS
provider schema).
- Around line 231-235: The CEL rule currently only requires aws for self.type ==
'AWS' and name for self.type == 'Manual' but doesn't forbid the opposite field;
update the x-kubernetes-validations rule expression to both require the needed
field and forbid the other based on self.type (e.g., for self.type == 'AWS'
ensure has(self.aws) and not has(self.name) and for self.type == 'Manual' ensure
has(self.name) and not has(self.aws), also ensure name != '' and any required
subfields of aws are present); modify the rule under x-kubernetes-validations to
combine these clauses so that aws is forbidden when type == 'Manual' and name is
forbidden when type == 'AWS'.
In
@payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml:
- Around line 220-227: The CRD description for the KMS provider "type" is
misleading—update the description string under the `type` field (the block that
currently reads 'type defines the kind of platform for the KMS provider.
Available provider types are Manual only.') to accurately reflect that `enum`
contains both `AWS` and `Manual`, noting that `AWS` is deprecated/kept for
backward compatibility; reference the `type` field and the enum values `AWS` and
`Manual` and adjust the text to something like "Available provider types are
Manual and AWS (AWS is deprecated/kept for backward compatibility)."
In
@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml:
- Line 176: Update the deprecation description string for the CRD entry that
currently reads "There is no logic listening this resource type" to correct
grammar; change it to either "There is no logic listening to this resource type"
or "There is no controller watching this resource type" so the Deprecated field
reads clearly and correctly.
- Around line 220-227: The description for the KMS provider "type" is
inconsistent with the enum: it says "Manual only" while the enum lists both AWS
and Manual; update the description in the CRD (the "type" field description) to
accurately reflect the enum (e.g., "Available provider types are Manual and AWS
(AWS is deprecated)" or remove AWS from the enum if AWS is no longer supported)
so the documentation matches the allowed values.
🧹 Nitpick comments (5)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-176: Tighten/clarify the deprecation wording forkms.awsdocs.Current text (“There is no logic listening this resource type…”) is awkward and can be misread; suggest “no controller/watchers” + (if possible) an explicit release/version target for removal.
config/v1/zz_generated.swagger_doc_generated.go (1)
2328-2332: Swagger doc deprecation message: grammar + separation.
AWSKMSConfig’s""doc string would read better with punctuation and clearer phrasing (“no controller/watchers … planned removal …”). Since this file is generated, prefer fixing the Go type doc comments and regenerating.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (3)
176-176: Minor grammar issue in deprecation notice.Consider revising "listening this resource type" to "watching this resource type" or "for this resource". The phrase "this resource type" could also be clarified to explicitly reference "AWS KMS configuration" for better clarity.
231-235: Validation doesn't enforce mutual exclusivity as described.The
namefield description (line 216) states it is "forbidden when type is AWS", but the validation rule only ensures the required fields are present—it doesn't prevent settingnamewhentypeisAWS. A user could set bothawsconfig andnamewhen usingtype: AWS, and validation would pass.If enforcing mutual exclusivity is intended, consider adding explicit checks:
♻️ Suggested stricter validation
x-kubernetes-validations: - - message: aws config is required when kms provider type is AWS; - name is required when kms provider type is Manual - rule: (self.type == 'AWS' && has(self.aws)) || (self.type == - 'Manual' && has(self.name) && self.name != '') + - message: aws config is required when kms provider type is AWS, and name must not be set + rule: "self.type == 'AWS' ? (has(self.aws) && !has(self.name)) : true" + - message: name is required when kms provider type is Manual, and aws must not be set + rule: "self.type == 'Manual' ? (has(self.name) && self.name != '' && !has(self.aws)) : true"Alternatively, if mutual exclusivity is not critical (given AWS is deprecated), update the
namefield description to remove "forbidden when type is AWS" to match actual behavior.
210-219: Add pattern validation to the KMS plugin name field.The
namefield lacks character validation despite being templated into a Unix socket path. Without restrictions, users could provide values containing path separators (/), parent directory references (..), or other characters that could cause socket creation failures or path traversal issues.Other KMS fields in the same struct (AWS region and keyARN) already use pattern validation. Consider adding similar validation here:
Suggested pattern validation
x-kubernetes-validations: - message: name must contain only alphanumeric characters, hyphens, and underscores rule: self.matches('^[a-zA-Z0-9_-]+$')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/types_kmsencryption.go
🔇 Additional comments (9)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (4)
8-28: LGTM!Valid endpoint test case correctly validates the External KMS configuration with a properly formatted Unix socket path.
29-59: LGTM!Test cases correctly validate error scenarios:
- Missing
unix:///prefix triggers format error- KMS config with non-KMS encryption type is properly rejected
- Empty KMS config requires the endpoint field
60-79: LGTM!Good coverage for the defaulting behavior - when
endpointis provided withoutmanagementModel, it correctly defaults toExternal.
80-133: LGTM!Comprehensive coverage of edge cases:
- Missing endpoint when
managementModelis specified- Abstract socket patterns (both
@nameand@name/pathforms)- Empty string and whitespace in paths
The minimum length validation of 9 characters correctly ensures at least one character after the
unix:///prefix.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (2)
231-235: Validation rule doesn't enforce "forbidden" constraint described innamefield.The
namefield description (line 216) states it is "forbidden when type is AWS," but the validation rule only enforces that the required fields are present—it doesn't forbidnamefor AWS orawsfor Manual. This could allow ambiguous configurations.If this is intentional (e.g., for migration flexibility), consider updating the
namefield description to remove "forbidden when type is AWS." Otherwise, consider strengthening the validation:Suggested fix to enforce mutual exclusivity
x-kubernetes-validations: - - message: aws config is required when kms provider type is AWS; - name is required when kms provider type is Manual - rule: (self.type == 'AWS' && has(self.aws)) || (self.type == - 'Manual' && has(self.name) && self.name != '') + - message: aws config is required when kms provider type is AWS, and name must not be set; name is required when kms provider type is Manual, and aws must not be set + rule: (self.type == 'AWS' && has(self.aws) && !has(self.name)) || (self.type == 'Manual' && has(self.name) && self.name != '' && !has(self.aws))
210-219: LGTM on the newnamefield definition.The field constraints (minLength: 1, maxLength: 80) and the documentation explaining the socket path templating are clear and appropriate.
openapi/generated_openapi/zz_generated.openapi.go (1)
16148-16169: Schema additions look correct.The new
namefield is well-documented with clear constraints (1-80 characters, UNIX domain socket path templating). The type field description correctly reflects the transition to "Manual only" provider type.Note: The deprecation notice on line 16166 has the same grammatical issue as flagged earlier ("listening this" → "listening to this").
openapi/openapi.json (2)
8562-8569: LGTM - discriminator maintains backward compatibility.Keeping
awsin the discriminator alongside the newnamefield supports backward compatibility during the deprecation period.
11689-11691: LGTM!The updated description provides clearer documentation of the field's purpose.
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
ardaguclu
force-pushed
the
kms-api
branch
3 times, most recently
from
6bdb6ac to
94d4f28
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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.swagger_doc_generated.go (1)
2328-2343: Swagger doc text is inconsistent (“Manual only”, andnameforbidden for AWS).
typesays “Manual only” while AWS still exists (deprecated), andnameclaims it’s forbidden for AWS without an obvious corresponding validation.Suggested doc wording tweak
@@ - "type": "type defines the kind of platform for the KMS provider. Available provider types are Manual only.", + "type": "type defines the kind of platform for the KMS provider. Available provider types are AWS (deprecated) and Manual.", @@ - "name": "... This field is required when type is Manual, and forbidden when type is AWS. ...", + "name": "... This field is required when type is Manual. It must be omitted when type is AWS. ...",payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-236: CRD is missing (or not showing) thekms.namesafe-filename validation, and doesn’t forbid cross-fields.
Given the added negative tests, the CRD should enforce:
kms.namemust not contain/or..kms.namemust be omitted whenkms.type=AWSkms.awsmust be omitted whenkms.type=ManualSuggested CRD schema additions (conceptual; ideally comes from regen)
@@ name: @@ maxLength: 80 minLength: 1 type: string + x-kubernetes-validations: + - message: name must be a safe socket filename (must not contain '/' or '..') + rule: '!self.contains(''/'') && !self.contains(''..'')' @@ x-kubernetes-validations: - message: aws config is required when kms provider type is AWS; name is required when kms provider type is Manual rule: (self.type == 'AWS' && has(self.aws)) || (self.type == 'Manual' && has(self.name) && self.name != '') + - message: name is forbidden when kms provider type is AWS; aws config is forbidden when kms provider type is Manual + rule: (self.type == 'AWS' && !has(self.name)) || (self.type == 'Manual' && !has(self.aws))
🤖 Fix all issues with AI agents
In @config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml:
- Around line 29-50: Update the failing tests so they match the schema’s actual
cross-field validation: either (A) remove expectations that AWS+name or
Manual+aws are forbidden and instead assert success when the required field is
present (e.g., "Should be able to create KMS with AWS provider and valid aws
config" and the Manual case with aws present), or (B) if you introduce a new
forbidden-fields XValidation, add that XValidation to the schema and then keep
the tests but change the expectedError strings to match the combined xValidation
message (replace the exact "name is required when kms provider type is Manual"
expectations for the two Manual missing/empty name tests with the real combined
validation text produced by APIServer.spec.encryption.kms XValidation). Ensure
tests referencing the Manual provider name and AWS config (the tests named
"Should fail to create KMS Manual provider without name", "Should fail to create
KMS Manual provider with empty name", and "Should be able to create KMS with AWS
provider and valid aws config") are updated accordingly and apply the same fix
for the duplicated cases around the later block mentioned.
In @config/v1/types_kmsencryption.go:
- Around line 3-36: KMSConfig currently only requires the correct field per Type
but doesn't forbid the opposite field; update the kubebuilder XValidation rules
on KMSConfig to enforce exclusivity so that when Type == 'AWS' the Name field is
forbidden and AWS must be present, and when Type == 'Manual' the AWS field is
forbidden and Name must be present. Concretely, add XValidation rules
referencing the Type union discriminator and the Name/AWS fields (e.g., on type
KMSConfig: rule="(self.type == 'AWS' => has(self.aws) && !has(self.name)) &&
(self.type == 'Manual' => has(self.name) && !has(self.aws))", with appropriate
messages) so the schema forbids AWS+Name combinations and Manual+aws
combinations.
In
@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml:
- Around line 220-227: Update the field description for the `type` property (the
YAML `type` key with enum values `AWS` and `Manual`) so it accurately reflects
the enum; replace "Available provider types are Manual only" with a statement
that both `AWS` and `Manual` are valid values and note that `AWS` is deprecated
(or otherwise indicate its deprecated status) so the description matches the
enum.
- Around line 210-219: The CRD YAML's name field is missing the
x-kubernetes-validations that mirror the Go kubebuilder XValidation rule (in
config/v1/types_kmsencryption.go) preventing '/' and '..' in the KMS plugin
name; regenerate the CRD from the Go types so the name property in the CRD
includes the x-kubernetes-validations entry equivalent to the
+kubebuilder:validation:XValidation rule (ensuring the validation message and
rule are preserved), and update the build process to run the CRD generation step
so future edits to the types produce the CRD with the validation.
🧹 Nitpick comments (2)
config/v1/types_kmsencryption.go (1)
63-75: Minor: polish Manual provider godoc for grammar/clarity.
“represents a supported KMS provider is managed…” reads a bit off; worth rephrasing since it’s user-facing API docs.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
231-235: Validation rule doesn't enforce mutual exclusivity.The current rule ensures required fields are present for each type, but doesn't prevent setting both
awsandnamewhentype: Manual(ornamewhentype: AWS). The field description statesnameis "forbidden when type is AWS", but the validation doesn't enforce this.Consider strengthening the rule to enforce mutual exclusivity:
Suggested stricter validation
x-kubernetes-validations: - - message: aws config is required when kms provider type is AWS; - name is required when kms provider type is Manual - rule: (self.type == 'AWS' && has(self.aws)) || (self.type == - 'Manual' && has(self.name) && self.name != '') + - message: aws config is required when kms provider type is AWS and name must not be set; name is required when kms provider type is Manual and aws must not be set + rule: (self.type == 'AWS' && has(self.aws) && !has(self.name)) || (self.type == 'Manual' && has(self.name) && self.name != '' && !has(self.aws))Alternatively, if mutual exclusivity is intentionally not enforced (e.g., to allow gradual migration), the field description at line 216 should be updated to remove "forbidden when type is AWS".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml
🔇 Additional comments (12)
openapi/generated_openapi/zz_generated.openapi.go (3)
9174-9180: LGTM!The deprecation notice for
AWSKMSConfigis clear and aligns with the PR objective to phase out AWS-specific KMS configuration in favor of the new Manual provider approach.
16148-16169: LGTM!The new
namefield schema is well-documented with clear constraints (1-80 chars, safe filename rules, conditional requirement based on type). The deprecation notice on theawsfield is consistent with the overall deprecation approach.Since this is a generated file, ensure the source type definitions in
config/v1/types_kmsencryption.gohave the corresponding CEL validation markers to enforce the documented constraints (length, forbidden characters, conditional requirements).
16176-16183: LGTM!The discriminator mapping correctly handles the union type pattern:
awsfield for the deprecated AWS type andnamefield for the new Manual type. This follows OpenShift API conventions for type-discriminated unions.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-209: Deprecation notice is clear and appropriate.The AWS KMS configuration is properly marked as deprecated with a clear message about planned removal. Retaining the fields while deprecating ensures backward compatibility during the transition period.
openapi/openapi.json (8)
4456-4460: Deprecation notice added for AWSKMSConfig.The deprecation description clearly communicates that this type will be removed in the next release. This aligns with the PR objective to remove AWS-specific KMS configuration.
4575-4587: New AcceptRisk type looks correct.The type definition is straightforward with a required
namefield. The description mentions a 256-character limit; verify that this constraint is enforced via CEL validation or kubebuilder markers in the source Go types, since OpenAPI schema validation alone won't enforce it.
5875-5886: New conditionalUpdateRisks field with map semantics.The field uses
x-kubernetes-list-type: mapkeyed byname, which provides proper merge semantics. The description mentions a 500-entry limit that should be enforced via CEL validation in the source types.
6094-6102: New riskNames field with set semantics.Using
x-kubernetes-list-type: setcorrectly enforces uniqueness. The description mentions 256-character and 500-entry limits that should be enforced via source type validation.
6129-6140: New conditions field with map semantics.The field is correctly defined as a map keyed by
type. The description states "must not contain more than one entry" which is an unusually restrictive limit—verify this is intentional for the design.
8551-8554: New name field for Manual KMS provider.The
namefield description clearly explains the UNIX domain socket path templating and the validation requirements (1-80 chars, safe filename, no '/' or '..'). This aligns with the PR objective of supporting manually managed KMS plugins.Verify that the length and character constraints are enforced via CEL validation in the source Go types.
11644-11655: New acceptRisks field on Update.The field uses map semantics keyed by
namewith a 1000-entry limit mentioned in the description. This provides a mechanism for administrators to accept conditional update risks.
11689-11692: Minor description update for acceptedRisks.The updated description provides clearer examples of what risks might be accepted (Upgradeable=False, missing signature override, non-recommended updates). This is a documentation improvement.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
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)
140-167: Tests will fail: validation rule doesn't enforce mutual exclusivity.The current validation rule is:
(self.type == 'AWS' && has(self.aws)) || (self.type == 'Manual' && has(self.name) && self.name != '')
Lines 140-153: With
type: AWS,awspresent, andname: my-plugin, the first condition(self.type == 'AWS' && has(self.aws))is TRUE, so validation passes. The test expects an error, but won't get one.Lines 154-167: With
type: Manual,name: my-plugin, andawspresent, the second condition is TRUE, so validation passes. The test expects an error, but won't get one.The description states
nameis "forbidden when type is AWS" andawsshould be forbidden when type is Manual, but the validation rule doesn't enforce this mutual exclusivity.🐛 Proposed fix for the validation rule in CRDs and Go types
Update the validation rule to enforce mutual exclusivity:
- rule: (self.type == 'AWS' && has(self.aws)) || (self.type == 'Manual' && has(self.name) && self.name != '') + rule: (self.type == 'AWS' && has(self.aws) && !has(self.name)) || (self.type == 'Manual' && has(self.name) && self.name != '' && !has(self.aws))And update the error message to be more specific:
- message: aws config is required when kms provider type is AWS; name is required when kms provider type is Manual + message: aws config is required when kms provider type is AWS, and name must not be set; name is required when kms provider type is Manual, and aws must not be set
🤖 Fix all issues with AI agents
In @config/v1/zz_generated.swagger_doc_generated.go:
- Around line 2338-2343: The swagger doc for KMSConfig is incorrect: update the
"type" entry in map_KMSConfig (the "type" key in map_KMSConfig) to state that
both Manual and AWS are accepted and that AWS is deprecated (e.g., "type defines
the kind of platform for the KMS provider. Available provider types are Manual
and AWS (AWS is deprecated)."), then regenerate the swagger output so the change
propagates into zz_generated.swagger_doc_generated.go to match the
+kubebuilder:validation:Enum and the deprecation on AWSKMSProvider.
In @openapi/generated_openapi/zz_generated.openapi.go:
- Around line 16176-16183: The discriminator mapping for KMSConfig is wrong: in
the generated map for "fields-to-discriminateBy" change the entry that maps
"name" from "Name" to the actual enum value "Manual" (so the map becomes "name":
"Manual"), ensuring it matches the KMSProviderType enum and the XValidation rule
that ties the name field to type == 'Manual'.
In @openapi/openapi.json:
- Around line 6094-6102: The OpenAPI schema for the riskNames property is
missing the kubebuilder validation constraints; update the riskNames array
schema so the array object includes "maxItems: 500" and the nested "items"
object includes "maxLength: 256" (i.e., add maxItems: 500 to the riskNames
definition and add maxLength: 256 to the items definition for the string) so the
generated API enforces the same constraints as the Go tags referenced by
spec.desiredUpdate.acceptRisks.
- Around line 6129-6140: The OpenAPI "conditions" array schema is missing the
array bounds and has an inaccurate description; add "maxItems": 8 and
"minItems": 1 to the "conditions" property in the OpenAPI schema (the array
under the "conditions" key that references
io.k8s.apimachinery.pkg.apis.meta.v1.Condition) and update the description to
reflect the true validation (e.g., state the list must contain between 1 and 8
unique-type entries). Also update the source comment in
config/v1/types_cluster_version.go that documents the "conditions" field so it
no longer says "must not contain more than one entry" but accurately documents
the allowed 1–8 entries, then regenerate the OpenAPI JSON so the schema and
comment remain consistent.
In
@payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml:
- Around line 236-240: The x-kubernetes-validations rule must enforce mutual
exclusivity between aws and name based on type: update the validation rule for
the CustomNoUpgrade CRD so that when self.type == 'AWS' it requires
has(self.aws) and not has(self.name), and when self.type == 'Manual' it requires
has(self.name) and not has(self.aws) and self.name != ''; also update the
accompanying message to state that name is forbidden when type is AWS and aws is
forbidden when type is Manual to reflect the enforced exclusivity.
🧹 Nitpick comments (2)
config/v1/types_kmsencryption.go (1)
8-9: Comment is misleading - AWS is still a valid enum value.The comment states "Available provider types are Manual only" but line 64 shows
+kubebuilder:validation:Enum=AWS;Manual. While AWS is deprecated, it's still a valid value and the comment could confuse users.✏️ Suggested clarification
// type defines the kind of platform for the KMS provider. -// Available provider types are Manual only. +// Available provider types are AWS (deprecated) and Manual.config/v1/zz_generated.swagger_doc_generated.go (1)
2328-2332: Tighten/standardize the deprecation blurb formatting for AWSKMSConfig docs.Current
""doc string reads like two sentences run together and is missing punctuation (“provider Deprecated: …”). Consider making it a proper sentence and/or puttingDeprecated:on its own sentence for swagger consumers. Since this is generated, please update the source godoc and re-run the generator.Proposed doc string tweak (illustrative; prefer fixing at the source + regen)
- "": "AWSKMSConfig defines the KMS config specific to AWS KMS provider Deprecated: There is no logic listening to this resource type, we plan to remove it in next release.", + "": "AWSKMSConfig defines the KMS config specific to the AWS KMS provider. Deprecated: There is no logic listening to this resource type; it is planned for removal in a future release.",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
config/v1/types_kmsencryption.go (1)
config/v1/zz_generated.swagger_doc_generated.go (1)
AWSKMSConfig(2334-2336)
🔇 Additional comments (18)
openapi/generated_openapi/zz_generated.openapi.go (2)
9174-9180: LGTM!The deprecation notice for
AWSKMSConfigis clear and appropriately warns consumers about the planned removal.
16148-16169: LGTM!The new
namefield is well-documented with clear validation rules:
- Socket path templating is clearly explained
- Length constraints (1-80 chars) are specified
- Security-conscious validation (no '/' or '..') prevents path traversal
- Conditional requirements (required for Manual, forbidden for AWS) are explicit
The deprecation notices on the
awsfield appropriately mirror theAWSKMSConfigdeprecation.config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (4)
8-49: LGTM for Manual provider test cases.These tests correctly validate the Manual provider's
namefield requirements: valid names, missing names, and empty names are all properly covered.
50-84: LGTM for basic AWS provider test cases.The valid AWS config and missing AWS config scenarios are correctly tested.
85-139: LGTM for validation constraint tests.Good coverage for invalid provider types, name length limits, and path traversal prevention (
/and..).
168-213: LGTM for encryption type and AWS format validation tests.The tests correctly validate that KMS config is forbidden with non-KMS encryption types, empty KMS config requires type, and AWS-specific validations for keyARN and region formats.
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (2)
176-176: Deprecation note is appropriate.Clear communication that AWS KMS config is deprecated and planned for removal.
210-224: Name field schema looks correct.The
namefield has appropriate constraints: minLength 1, maxLength 80, and validation to prevent path traversal with'/'and'..'.config/v1/types_kmsencryption.go (4)
15-26: Name field definition looks correct.The field has appropriate validation constraints (1-80 chars, no path traversal characters) and clear documentation about socket path templating.
31-35: Deprecation note for AWS field is appropriate.Clear indication that the AWS config path is deprecated.
38-61: AWSKMSConfig type properly marked as deprecated.The struct and its validation rules remain intact for backward compatibility during the deprecation period.
63-75: Provider type enum and constants are well-defined.The enum includes both AWS (deprecated) and Manual types with appropriate constant definitions and documentation.
openapi/openapi.json (6)
4456-4460: LGTM!Clear deprecation notice for
AWSKMSConfigwith appropriate rationale. This aligns with the PR objective to phase out AWS-specific KMS configuration.
8562-8569: LGTM!The discriminator correctly maps the
typefield to the appropriate configuration fields:awsfor AWS type andnamefor Manual type. This enables proper union type validation.
11689-11692: LGTM!The updated description provides helpful examples (Upgradeable=False, missing signature) clarifying what risks may be recorded in
acceptedRisks.
8547-8559: Path-safety validations for thenamefield are properly enforced.The critical constraints mentioned in the description are implemented via kubebuilder markers in the Go type (
config/v1/types_kmsencryption.go):
- Length:
MinLength=1,MaxLength=80- Path traversal prevention: CEL validation rule
!self.contains('/') && !self.contains('..')with explicit error messageThese validations are correctly configured and will be enforced at the Kubernetes API server level, ensuring safe socket path templating for
unix:///var/run/kmsplugin/<name>.sock.
5875-5886: AddmaxItems: 500to the OpenAPI schema to match the Go validation.The Go type in
config/v1/types_cluster_version.go(line 213) enforces this constraint via+kubebuilder:validation:MaxItems=500, but the OpenAPI schema lacks the correspondingmaxItemsproperty. Update the schema to reflect the validation rule already present in the source types.
4575-4587: Verify that validation constraints are enforced.The description states the
namefield "must be a non-empty string and must not exceed 256 characters," but the schema lacks correspondingminLength: 1andmaxLength: 256constraints. If these constraints are enforced via kubebuilder validation markers or CEL validations in the Go types, this is fine. Otherwise, add schema-level validation.
|
@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 story to target the "4.22.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 story to target the "4.22.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: 7
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)
140-153: The CEL validation rule does not enforce the documented constraint that thenamefield is forbidden whentype=AWS.The rule
(self.type == 'AWS' && has(self.aws)) || (self.type == 'Manual' && has(self.name) && self.name != '')only requiresawsconfig for the AWS type but does not forbid thenamefield. Since the test provides bothnameand validawsconfig withtype: AWS, the rule evaluates to TRUE and validation passes—contradicting the test's expected error.To match the documented constraint (line 19: "forbidden when type is AWS"), the AWS branch should be:
(self.type == 'AWS' && has(self.aws) && !has(self.name)).
🤖 Fix all issues with AI agents
In @config/v1/types_kmsencryption.go:
- Line 9: The top-of-file comment is wrong: it says "Manual only" while the enum
(the provider types declaration including AWS and Manual) and the XValidation
rule still allow AWS; either update the comment to state that AWS is deprecated
but still supported (mentioning both AWS and Manual provider types) or, if AWS
should be disallowed, remove AWS from the enum and update the XValidation rule
accordingly; locate the provider types enum (the line containing "AWS;Manual")
and the XValidation rule at the file header and make the comment and schema
definitions consistent.
- Around line 15-26: The Name field in the KMS encryption struct is missing the
+unionMember marker; add the kubebuilder comment tag "+unionMember" immediately
above the Name string declaration (the same way AWS has it) so Name is treated
as a union member of the Type discriminator and matches the existing union
pattern used for AWS.
In @config/v1/zz_generated.swagger_doc_generated.go:
- Around line 2328-2332: The swagger entry for map_AWSKMSConfig contains awkward
wording and a poor deprecation phrasing; update the original Go doc comment on
the AWSKMSConfig type (likely in config/v1/types_kmsencryption.go) to use clear
grammar and a concise deprecation note (e.g., "Deprecated: AWSKMSConfig is
deprecated and will be removed in a future release.") and clarify the
keyARN/region sentences for grammar and format constraints, then re-run the
swagger generation to regenerate zz_generated.swagger_doc_generated.go so
map_AWSKMSConfig reflects the corrected text.
- Around line 2338-2343: The map_KMSConfig swagger docs are inconsistent: the
"type" entry claims "Manual only" yet "name" mentions behavior "forbidden when
type is AWS" and an "aws" field is documented; choose one approach and make docs
consistent. Either (A) restore AWS as a valid/deprecated provider by updating
the "type" description to list both "Manual" and "AWS" (mark AWS deprecated) and
keep the "aws" key with its deprecation note, or (B) remove AWS references by
deleting the "aws" key and updating the "type" and "name" strings to remove any
mention of AWS; apply the chosen change inside map_KMSConfig so the "type",
"name", and "aws" entries no longer contradict one another (refer to
map_KMSConfig and the "type"/"name"/"aws" map keys).
In @openapi/generated_openapi/zz_generated.openapi.go:
- Around line 16176-16183: The discriminator mapping inside the union
discriminator map (the map[string]interface{} literal containing "discriminator"
and "fields-to-discriminateBy") is incorrect: change the value for the "name"
key from "Name" to "Manual" so it matches the KMSProviderType enum ("AWS" and
"Manual"); locate the map with keys "discriminator" and
"fields-to-discriminateBy" and update the "name" -> "Name" entry to "name" ->
"Manual".
🧹 Nitpick comments (2)
openapi/generated_openapi/zz_generated.openapi.go (1)
16151-16166: Minor inconsistency in type description.The
typefield description states "Available provider types are Manual only," but the discriminator (lines 16179-16180) still includes bothAWSandNamemappings. While AWS is deprecated, the description could be slightly misleading for API consumers who may still encounter existing AWS configurations.Consider updating the source to say something like "Available provider types are AWS (deprecated) and Manual" for accuracy, or keep it as-is if the intent is to actively discourage AWS usage.
The new
namefield documentation is comprehensive and clearly explains the socket path templating, length constraints, and type-based requirements.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
227-231: Minor inconsistency: Description says "Manual only" but enum includes AWS.The description states "Available provider types are Manual only" while the enum includes both
AWSandManual. This is likely intentional during the deprecation period, but consider updating the description to reflect reality, e.g., "Available provider types are AWS (deprecated) and Manual."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
config/v1/types_kmsencryption.go (1)
config/v1/zz_generated.swagger_doc_generated.go (1)
AWSKMSConfig(2334-2336)
🔇 Additional comments (19)
config/v1/types_kmsencryption.go (2)
3-36: LGTM on the overall KMSConfig structure and validation logic.The XValidation rule correctly enforces the union semantics: requiring
awswhen type isAWSandnamewhen type isManual. The deprecation notes for AWS-related fields are clear, and the Name field validation appropriately prevents path traversal attacks for socket filenames.
38-61: Deprecation annotations and new ManualKMSProvider constant look good.The deprecation messages are clear and consistent across
AWSKMSConfig,AWSKMSProvider, and the inline field documentation. The newManualKMSProviderconstant with its documentation explaining that the KMS plugin should run as static pods on control plane nodes aligns with the enhancement proposal.Also applies to: 63-74
openapi/generated_openapi/zz_generated.openapi.go (1)
9177-9177: LGTM!The deprecation notice is clear and appropriately warns consumers that this type will be removed in a future release.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (3)
8-28: LGTM!Good test coverage for the new Manual provider type with a valid name. The expected output correctly includes the default audit profile.
29-49: LGTM!Proper negative test cases for missing and empty name when using Manual provider type.
96-139: LGTM!Comprehensive test coverage for the
namefield validation:
- Maximum length (80 characters)
- Forward slash prohibition
- Double dots prohibition (start, middle positions)
These tests properly verify the socket filename safety constraints.
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (2)
176-176: LGTM!Clear deprecation notice indicating this field will be removed in the next release.
210-224: LGTM!The
namefield is well-defined with:
- Clear description of the socket path templating
- Proper length constraints (1-80 characters)
- CEL validation preventing path traversal (
/and..)payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
176-240: Consistent with CustomNoUpgrade CRD.The KMS configuration changes (deprecation notice,
namefield, enum, validation rule) are identical to the CustomNoUpgrade manifest. The same validation rule issue noted in the CustomNoUpgrade file applies here as well - the rule doesn't enforce mutual exclusivity betweennameandawsfields.payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
176-240: Consistent with other CRD manifests.The KMS configuration changes are identical to CustomNoUpgrade and TechPreviewNoUpgrade manifests. The same validation rule limitation applies here.
Note: The AI summary suggested this file would have different changes (endpoint-based External KMS model), but the actual file content shows the same AWS/Manual pattern. This appears to be correct per the PR objectives which focus on Manual provider support.
openapi/openapi.json (9)
4456-4460: LGTM!The deprecation notice for
AWSKMSConfigis clear and follows conventions. The explanation that no controller listens to this resource provides context for the deprecation decision.
4575-4587: LGTM!The
AcceptRisktype is well-defined with a clear description. The 256-character constraint mentioned in the description should be enforced via validation markers in the Go source types.
5875-5886: LGTM!The
conditionalUpdateRisksfield is well-structured as a map-like array keyed byname. The description clearly explains the risk acceptance mechanism for conditional updates.
6094-6102: LGTM!The
riskNamesfield correctly uses set semantics to ensure unique entries. The description clearly explains the relationship between risk names, conditional updates, and the acceptance mechanism.
6129-6140: LGTM!The
conditionsfield follows the standard Kubernetes condition pattern with map semantics keyed bytype. The description clearly indicates theAppliescondition type for determining if a risk applies to the current cluster.
8547-8559: LGTM!The changes align well with the PR objectives:
awsfield properly marked deprecated with clear messaging.- New
namefield has comprehensive documentation: socket path template (unix:///var/run/kmsplugin/<name>.sock), length constraints (1-80 chars), type requirements (required for Manual, forbidden for AWS), and safety constraints (no/or..).- The
typefield description correctly reflects that only Manual is the available provider type going forward.
8562-8569: LGTM!The discriminator mapping correctly associates
awsfield with theAWStype andnamefield with theName(Manual) type. This enforces the mutual exclusivity described in the field documentation.
11644-11655: LGTM!The
acceptRisksfield is well-designed with map semantics keyed byname. The description clearly explains that accepted risks may reference current, previous, or future updates, providing flexibility for administrators.
11689-11692: LGTM!The
acceptedRisksdescription clearly explains the field's purpose with helpful examples of what types of overrides are recorded.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
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.swagger_doc_generated.go (1)
2328-2343: Swagger docs inherit the same “Manual only” vsAWSenum mismatch.
Fix the source-of-truth comments/validation first (likelyconfig/v1/types_kmsencryption.go), then regenerate swagger/CRD outputs to keep everything aligned.
🤖 Fix all issues with AI agents
In @config/v1/types_kmsencryption.go:
- Around line 5-36: KMSConfig's union is inconsistent: add explicit union
members and stricter CEL checks—mark Name as a union member (or, if +unionMember
doesn't support scalar strings, change Name to a nested struct like
ManualKMSConfig and add +unionMember to that) and ensure AWS has +unionMember
(AWS already uses AWSKMSConfig). Update the CEL XValidation to both require the
appropriate field and forbid the other (e.g. "(self.type == 'AWS' &&
has(self.aws) && !has(self.name)) || (self.type == 'Manual' && has(self.name) &&
self.name != '' && !has(self.aws))"). Also update the Type field comment to
"Available provider types are AWS (deprecated) and Manual." Ensure referenced
symbols: KMSConfig, Type (KMSProviderType), Name (or ManualKMSConfig), AWS
(AWSKMSConfig), and the XValidation rule are changed accordingly.
In @openapi/generated_openapi/zz_generated.openapi.go:
- Around line 16176-16183: The Name field in the KMSConfig union is missing a
union member marker so it maps to the "Manual" KMSProviderType; update the
KMSConfig definition in config/v1/types_kmsencryption.go by adding the
appropriate kubebuilder/unions marker (e.g. +unionMember or the project’s union
annotation) on the Name field to map it to "Manual", ensuring the discriminator
mapping aligns with the KMSProviderType enum that contains "AWS" and "Manual"
and matches how the AWS field already uses +unionMember for "AWS".
In
@payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml:
- Around line 226-232: The CRD docs for kms.type are inconsistent: the
description for kms.type says “Manual only” while the enum includes both "AWS"
and "Manual"; update the kms.type documentation and enum to match by either (A)
keeping both enum values and revising the description to mention "AWS"
(deprecated) and "Manual" and any deprecation note, or (B) removing "AWS" from
the enum so it truly is Manual-only—modify the enum list and the description
together (referencing kms.type, enum values "AWS" and "Manual") so they stay
consistent.
- Around line 210-241: The current x-kubernetes-validations CEL rule on the
object (referencing fields type, aws, and name) only enforces requiredness but
allows both aws and name to coexist; update that CEL rule to enforce exclusivity
so that when self.type == 'AWS' it requires has(self.aws) and forbids
has(self.name), and when self.type == 'Manual' it requires has(self.name) &&
self.name != '' and forbids has(self.aws); modify the validation expression on
the CRD where the x-kubernetes-validations rule is defined to reflect these
mutually exclusive conditions.
🧹 Nitpick comments (3)
config/v1/types_kmsencryption.go (1)
63-75: Minor: tighten wording onManualKMSProvidercomment.
Current phrasing is a bit awkward (“represents a supported KMS provider is managed…”).config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
1-199: Consider adding test for Manual provider withawsconfig.The tests validate that AWS provider with
namefield fails, but there's no reciprocal test verifying that Manual provider withawsconfig also fails. This would ensure symmetric validation coverage.Suggested additional test case
- name: Should fail to create KMS Manual provider with aws config initial: | apiVersion: config.openshift.io/v1 kind: APIServer spec: encryption: type: KMS kms: type: Manual name: my-plugin aws: keyARN: arn:aws:kms:us-east-1:123456789012:key/abcd1234-ab12-cd34-ef56-abcdef123456 region: us-east-1 expectedError: "name is required when kms provider type is Manual"openapi/openapi.json (1)
8555-8559: Type description may be misleading given AWS deprecation.The description states "Available provider types are Manual only," but the AWS type still exists in the schema (though deprecated). This could confuse API consumers who might wonder why
awsfield is present if only Manual is available.Consider clarifying the description to indicate AWS is deprecated but still present for backward compatibility:
Suggested improvement
-"description": "type defines the kind of platform for the KMS provider. Available provider types are Manual only.", +"description": "type defines the kind of platform for the KMS provider. Available provider types are Manual. The AWS type is deprecated and will be removed in a future release.",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🚧 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
🧰 Additional context used
🧬 Code graph analysis (1)
config/v1/types_kmsencryption.go (1)
config/v1/zz_generated.swagger_doc_generated.go (1)
AWSKMSConfig(2334-2336)
🔇 Additional comments (14)
openapi/generated_openapi/zz_generated.openapi.go (2)
9174-9180: LGTM - Deprecation notice added correctly.The deprecation notice clearly communicates the planned removal and lack of active listener logic.
16148-16169: LGTM - Newnamefield and deprecation notices are well-documented.The
namefield description comprehensively covers the socket path templating, length constraints (1-80 characters), required/forbidden conditions based on type, and safety requirements (no/or..).Minor note: Line 16151 states "Available provider types are Manual only" while AWS is deprecated but technically still valid. This is acceptable as it discourages new usage, but verify this aligns with the intended user guidance in the source type definition (
config/v1/types_kmsencryption.go).config/v1/types_kmsencryption.go (1)
63-75: Implementation correctly follows kubebuilder union discriminator and Kubernetes CRD CEL standards.Verification confirms: the discriminator field (
Type) as a required scalar string is valid; union members (AWS) properly use optional pointers with+unionMember; and CEL validation correctly useshas()semantics for optional field presence detection. No issues found.config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (7)
8-49: LGTM!Good coverage of Manual provider validation scenarios: valid name, missing name, and empty name cases are all properly tested with appropriate expected errors.
50-84: LGTM!AWS provider tests for valid configuration and missing aws config are correctly structured.
85-106: LGTM!Good coverage of invalid provider type and name length constraints.
107-139: LGTM!Comprehensive path-safety validation tests for the name field, covering forward slash, leading double dots, and embedded double dots.
154-173: LGTM!Good coverage of mutual exclusion between KMS config and non-KMS encryption types, plus empty KMS config validation.
174-199: LGTM!AWS-specific format validations for keyARN and region are properly tested.
140-153: Update test expected error to match actual validation issue.The test name indicates the intent is to verify that the
namefield is forbidden for AWS type providers. However, the expected error message refers to missingawsconfig, which is inconsistent because the test provides validawsconfig (keyARN and region).Per
config/v1/types_kmsencryption.go, thenamefield has an explicit comment stating: "This field is required when type is Manual, and forbidden when type is AWS." The validation rule should enforce this prohibition with an appropriate error message, or if the current rule is correct, the test should be updated to not include thenamefield and expect a different error scenario.openapi/openapi.json (4)
4456-4460: LGTM on the deprecation notice.The deprecation message clearly indicates that
AWSKMSConfigis deprecated and planned for removal. This aligns with the PR objective to deprecate AWS KMS in favor of the Manual type.
5875-5886: LGTM on conditional update risk fields.The
conditionalUpdateRisks,riskNames, andconditionsfields are well-documented with clear semantics:
- List-map with appropriate keys for merge semantics
- Clear descriptions of cardinality limits (500 entries, 256 char limit, single condition)
Ensure the cardinality constraints documented in descriptions are enforced via validation markers or CEL in the source types.
Also applies to: 6094-6102, 6129-6140
11644-11655: LGTM on acceptRisks field.The
acceptRisksfield structure is well-designed:
- Uses list-map with
namekey for efficient merge semantics- References the
AcceptRisktype correctly- Clear description of the relationship with
status.conditionalUpdateRisks
11689-11692: LGTM on acceptedRisks description update.The clarified description better explains the field's purpose in recording risks that were accepted during update initiation.
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
237-240: Consider: Validation allows extraneous fields when switching provider types.The validation rule ensures required fields are present but doesn't enforce that forbidden fields are absent. For example:
- A user could set
type: Manualwith bothnameandawsconfig present- A user could set
type: AWSwith bothawsconfig andnamepresentThe documentation states
nameis "forbidden when type is AWS", but this isn't enforced by the validation.This may be intentional to allow flexibility during deprecation, but consider if stricter validation would prevent configuration drift:
rule: (self.type == 'AWS' && has(self.aws) && !has(self.name)) || (self.type == 'Manual' && has(self.name) && self.name != '' && !has(self.aws))config/v1/types_kmsencryption.go (1)
8-13: Minor: Description says "Manual only" but enum includes AWS.The type description on line 9 states "Available provider types are Manual only", but the enum on line 64 includes both
AWSandManual. While AWS is deprecated, it's still a valid enum value.Consider updating the description to clarify the deprecation:
// type defines the kind of platform for the KMS provider. -// Available provider types are Manual only. +// Available provider types are AWS (deprecated) and Manual. +// AWS support is deprecated and will be removed in a future release.This would make the documentation consistent with the actual allowed values.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
50-95: Consider: Add boundary test cases for name length.The tests cover the error case for names exceeding 80 characters but don't include boundary success cases. Consider adding tests for:
- Name with exactly 1 character (minimum valid)
- Name with exactly 80 characters (maximum valid)
This would verify the boundary conditions are handled correctly.
openapi/openapi.json (1)
8547-8559: Consider clarifying the type description.Line 8556 states "Available provider types are Manual only" while the
awsfield still exists (though deprecated). This could confuse API consumers who see both options in the schema.Consider updating to: "Available provider types are Manual. AWS is deprecated and will be removed in a future release."
config/v1/zz_generated.swagger_doc_generated.go (1)
2328-2332: Fix deprecation sentence structure + timeline clarity in SwaggerDoc.Line 2329 reads like two sentences without punctuation (“provider Deprecated: …”) and “remove it in next release” is ambiguous/possibly inconsistent with “to be tombstoned in a future release” in the PR context.
Proposed wording (apply in source comments + regenerate)
- "": "AWSKMSConfig defines the KMS config specific to AWS KMS provider Deprecated: There is no logic listening to this resource type, we plan to remove it in next release.", + "": "AWSKMSConfig defines the KMS config specific to AWS KMS provider. Deprecated: there is currently no logic listening to this resource type; it is planned for removal in a future release.",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- openapi/generated_openapi/zz_generated.openapi.go
🧰 Additional context used
🧬 Code graph analysis (1)
config/v1/types_kmsencryption.go (1)
config/v1/zz_generated.swagger_doc_generated.go (1)
AWSKMSConfig(2334-2336)
🔇 Additional comments (15)
payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
210-240: LGTM! Newnamefield and validation rules are well-structured.The
namefield validation correctly enforces:
- Length constraints (1-80 characters)
- Safe socket filename rules (no
/or..)- Required when type is
Manual, forbidden implicitly forAWSvia the cross-field validationThe cross-field validation rule at lines 237-240 properly ensures mutual exclusivity between AWS and Manual provider configurations.
config/v1/types_kmsencryption.go (2)
15-26: LGTM! Name field validation is well-designed.The validation comprehensively addresses socket filename safety:
- Length bounds (1-80 chars) are reasonable for a UNIX socket path component
- The
!self.contains('/') && !self.contains('..')rule prevents path traversalNote: The
..check will reject names likemy..plugin(two consecutive dots anywhere), which is stricter than just preventing parent directory traversal (../). This is documented and acceptable as a conservative security measure.
67-75: LGTM! Provider type constants are properly defined.The
ManualKMSProviderconstant is well-documented, explaining that it represents user-managed KMS plugins running as static pods on control plane nodes. The deprecation notice onAWSKMSProvideris clear and consistent with other deprecation messages in this PR.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
210-240: LGTM! CustomNoUpgrade CRD is consistent with TechPreviewNoUpgrade.The KMS configuration changes (name field, Manual provider type, validation rules, deprecation notices) are identical to the TechPreviewNoUpgrade manifest, ensuring consistency across feature sets.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (2)
8-49: LGTM! Comprehensive test coverage for Manual provider.The tests thoroughly cover:
- Valid Manual provider configuration with name
- Missing
namefield (line 29-38)- Empty
namefield (line 39-49)These tests correctly validate the new validation rules for the Manual provider type.
107-139: LGTM! Good security validation test coverage.Tests correctly verify that the name field rejects:
- Forward slashes (
invalid/path)- Path traversal sequences (
../escape)- Double dots anywhere in name (
some..name)All tests expect the appropriate error message about safe socket filename requirements.
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
210-240: LGTM! DevPreviewNoUpgrade CRD is consistent with other manifests.All KMS-related changes match the TechPreviewNoUpgrade and CustomNoUpgrade manifests, ensuring consistent behavior across all feature gate variants.
openapi/openapi.json (8)
4456-4460: Deprecation notice properly added.The deprecation message clearly communicates the intent to remove this type in a future release, which aligns with the enhancement proposal goals.
4575-4587: LGTM.The AcceptRisk type is correctly defined with name as required. The 256-character limit mentioned in the description should be enforced via validation markers in the source Go types.
5875-5886: LGTM.The
conditionalUpdateRisksfield is properly defined as a map-type list keyed byname, following Kubernetes list conventions.
6094-6102: LGTM.The
riskNamesfield correctly usesx-kubernetes-list-type: setto enforce uniqueness of entries.
6129-6140: LGTM.The
conditionsfield follows the standard Kubernetes conditions pattern with map-type list keyed bytype.
8562-8569: LGTM.The discriminator mappings correctly associate
awswith the AWS provider type andnamewith the Manual provider type, following the union type pattern for conditional field requirements.
11644-11655: LGTM.The
acceptRisksfield is properly defined as a map-type list keyed byname, referencing the AcceptRisk type. The max 1000 entries constraint is documented in the description.
11689-11691: LGTM.Minor description refinement for the
acceptedRisksfield.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml:
- Around line 88-99: The test "Should fail to create KMS with invalid provider
type" currently asserts the full enum error string which is brittle; update the
assertion for the expectedError to match a stable fragment instead (e.g., assert
that the error contains "supported values" and at least one expected enum such
as "AWS") so that quoting/order/extra enums won't break the test—locate the test
block with name "Should fail to create KMS with invalid provider type" and
replace the full expectedError string with a shorter, stable fragment check.
In @config/v1/types_kmsencryption.go:
- Around line 23-31: Update the deprecation wording in the Go doc comments from
“next release” to “a future release” for the KMS encryption types; specifically
edit the comment block above the AWS field (AWS *AWSKMSConfig) and the other
similar comment blocks (the other occurrences in this file) so the text reads
“Deprecated: There is no logic listening to this resource type, we plan to
remove it in a future release.” and ensure the same phrasing is used for
generated CRDs by updating any doc/comment strings passed to codegen or
kubebuilder markers in the same file.
🧹 Nitpick comments (3)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (1)
100-147: Name validation tests are strong; consider adding a “dotfile” edge case if relevant.
MaxLength (Lines 100-111) and safe-socket rule coverage (Lines 112-147) is excellent, especiallysome..name(Line 146) which proves thecontains('..')rule isn’t just prefix-based. Optional: if “hidden” filenames (like.sockname) are undesired, you might add a test + validation now; otherwise current behavior allows it.config/v1/zz_generated.swagger_doc_generated.go (1)
2328-2356: Swagger docs: generated changes are fine; consider softening “next release” deprecation wording at the source.
The addedManualKMSConfigdocs (Lines 2349-2356) and updatedKMSConfigdocs (Lines 2338-2343) look correct and match theManualKMSConfigsemantics shown inconfig/v1/types_kmsencryption.go. The AWS deprecation blurb inmap_AWSKMSConfig/map_KMSConfigsays “remove it in next release” (Line 2329 / Line 2342); if that timeline isn’t guaranteed (especially with backports), updating the source Go doc comment to “a future release” and regenerating may be safer.config/v1/types_kmsencryption.go (1)
33-47: Consider tightening thenamefield to a safe filename character set.The current validation (blocking
/and.., with MaxLength=80) is functional but allows whitespace-only values and other characters that, while not creating traversal vulnerabilities, may cause issues. A stricter pattern—e.g.,^[a-zA-Z0-9._-]+$—would enforce more portable socket names and prevent ambiguous names. The MaxLength=80 is appropriate for sun_path buffer limits.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (9)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
config/v1/zz_generated.deepcopy.go (2)
config/v1/types_kmsencryption.go (1)
ManualKMSConfig(34-47)config/v1/zz_generated.swagger_doc_generated.go (1)
ManualKMSConfig(2354-2356)
config/v1/zz_generated.swagger_doc_generated.go (1)
config/v1/types_kmsencryption.go (2)
KMSConfig(8-31)ManualKMSConfig(34-47)
config/v1/types_kmsencryption.go (1)
config/v1/zz_generated.swagger_doc_generated.go (2)
ManualKMSConfig(2354-2356)AWSKMSConfig(2334-2336)
🔇 Additional comments (17)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (4)
148-168: Good negative coverage forencryption.type != KMSand emptykmsobject.
These two cases (Lines 148-168) are exactly the sort of regression guards that prevent accidental schema loosening.
169-194: AWS field format validations look consistent.
Invalid ARN and invalid region cases (Lines 169-194) provide good targeted coverage.
53-87: AWS provider tests: good keep-alive, but be mindful of deprecation + error wording drift.
The updatedkeyARNsample looks valid (Lines 63-77). Similar to Manual, the"aws config is required when kms provider type is AWS"assertion (Line 87) might diverge from actual validation error formatting—worth aligning with the harness' "stable substring" convention if needed.
8-53: Manual provider coverage looks good; ensureexpectedErroruses substring matching for stability.The positive case (Lines 8-31) and negative cases for missing/empty name (Lines 31-53) provide solid coverage. However, the
expectedErrorstrings like"manual config is required when kms provider type is Manual"and"spec.encryption.kms.manual.name: Invalid value"can be brittle if the apiserver message format changes slightly.OpenShift's CRD test harness supports both exact string matching and substring matching via the
containsMessagefield in the expected error specification. Since the KMSEncryptionProvider validation uses XValidation rules, consider using substring matching (e.g.,containsMessage: "spec.encryption.kms.manual.name: Invalid value") to match only the stable field path and error code, rather than the full error message, which may be wrapped or augmented by the validation layer.openapi/generated_openapi/zz_generated.openapi.go (1)
325-331: Generated OpenAPI definitions look correct.This is a generated file (
zz_generated.openapi.go) and the changes correctly reflect:
- New
ManualKMSConfigtype registration and schema definition- Deprecation notices on
AWSKMSConfigand theawsfield- Updated discriminator mappings for the
Manualprovider type- Required
namefield with appropriate validation description (1-80 chars, safe socket filename)The generated output is consistent with the PR objectives. Any schema adjustments should be made in the source type definitions (
config/v1/types_kmsencryption.go).Also applies to: 9175-9181, 16149-16190, 16579-16599
config/v1/zz_generated.deepcopy.go (2)
3833-3846: KMSConfig deepcopy updated correctly to include Manual.
Thein.Manual != nilbranch mirrors existing patterns and is safe givenManualKMSConfigcurrently contains only value fields.
4067-4082: ManualKMSConfig deepcopy methods look fine (generated).
No concerns.config/v1/types_kmsencryption.go (2)
74-86: Enum/const additions for Manual look consistent.
+kubebuilder:validation:Enum=AWS;Manualaligns with the newManualKMSProviderconstant.
5-7: CEL validation rules are correct and properly supported in Kubernetes 1.34.1.The
has(self.manual)andhas(self.aws)checks on optional pointer fields are fully supported. The rules correctly enforce the discriminated union semantics:has()returns true when a field is set, false when null/omitted. The ternary operator pattern you've used is the standard approach for this validation pattern in Kubernetes CRDs and matches best practices documented in the official CEL validation guidance.payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-252: Schema additions look coherent; please verify CEL/YAML quoting via schema validation.
Themanualobject schema, enum expansion (Manual), andx-kubernetes-validationsrules look consistent with the Go types, but this is brittle to quoting/escaping—worth double-checking withverify-crd-schemaon the target branch/tooling. Also consider aligning the “next release” deprecation phrasing (same note as in Go).payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-252: Same as TechPreviewNoUpgrade: looks good; confirm CEL/YAML escaping and deprecation phrasing consistency.payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-252: Same as the other CRDs: good update; verify CEL rules compile and behave as intended.openapi/openapi.json (5)
4456-4460: LGTM!The deprecation notice for
AWSKMSConfigis clear and aligns with the PR objectives to deprecate AWS KMS support in favor of the new Manual KMS provider.
4575-4587: Verify: AcceptRisk changes appear unrelated to KMS objectives.The
AcceptRisktype and associated fields (conditionalUpdateRisks,riskNames,conditions,acceptRisks) added throughout this file relate to conditional update risk management, not KMS configuration. The PR title and objectives focus specifically on KMS changes.Please confirm whether these changes are intentionally bundled in this PR or if
openapi.jsonwas regenerated and inadvertently picked up other pending API changes.
8545-8559: LGTM!The KMSConfig updates correctly:
- Deprecate the
awsfield with a clear notice- Add the new
manualfield with proper reference toManualKMSConfig- Update the
typedescription to reflect both available provider types
8562-8569: LGTM!The discriminator mappings are correctly configured to support the union type pattern for KMS configuration.
8803-8816: LGTM!The
ManualKMSConfigtype is well-defined with a clear description explaining:
- The socket path templating pattern (
unix:///var/run/kmsplugin/<name>.sock)- Character constraints (1-80 chars, no
/or..)- Cross-field validation requirements with the
typediscriminatorThe length and character restrictions mentioned in the description are presumably enforced via CEL validation markers in the Go types, which is the appropriate approach for Kubernetes APIs.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
Show resolved
Hide resolved
|
@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. |
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 (2)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
171-176: Avoid “next release” in deprecation text (will become stale).
Line 176: consider changing “remove it in next release” to “remove it in a future release” (or name the intended OCP version) so the CRD stays accurate over time.config/v1/zz_generated.swagger_doc_generated.go (1)
2328-2333: Generated doc string: fix punctuation + avoid “next release” wording at the source.
Line 2329 reads as a run-on (“provider Deprecated: …”) and “next release” will drift. Suggest updating the Go doc comment onAWSKMSConfig/KMSConfig.AWSto be grammatically separated and to say “a future release” (then regenerate).
🤖 Fix all issues with AI agents
In @config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml:
- Around line 31-41: The test's expectedError string for the case "Should fail
to create KMS Manual provider without manual config" is missing the trailing
clause from the CRD xValidation message (`, and forbidden otherwise`); update
the expectedError value to exactly match the XValidation message from
types_kmsencryption.go (i.e., include ", and forbidden otherwise") or change the
test to assert a containment/substring match against the canonical message so it
won't fail on exact string comparison.
🧹 Nitpick comments (6)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (2)
210-229: Consider tighteningmanual.nameto a safe-character regex (current rule is permissive).
The current validation only forbids/and.., which still allows whitespace and other odd filename characters. If downstream components ever interpolate this into paths/commands/logs, a stricter pattern (e.g.^[A-Za-z0-9][A-Za-z0-9_.-]{0,79}$) reduces ambiguity.
241-249: Cross-field CEL validation forawsvsmanualis solid; consider also makingmanual.nameschema-required.
The CEL rule correctly enforcesmanual+ non-emptymanual.namewhentype=Manualand forbids it otherwise. For better UX in non-CEL-aware tooling, you may also wantmanual.required: ["name"](and align the Go type tags accordingly) sincenameis effectively required in the Manual branch.config/v1/zz_generated.swagger_doc_generated.go (1)
2349-2356: ManualKMSConfig swagger doc looks good; consider documenting the “required when type=Manual” constraint.
Right now it describesname, but doesn’t state it’s required in the Manual branch (enforced via CEL/union logic). Adding that note in the Go comments would help API consumers (then regenerate).payload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml (1)
171-250: CRD wiring forkms.manual+ cross-field validations looks consistent.
- Line 210-Line 229:
manual.namelength + “no/or..” validation matches the Go type annotations.- Line 242-Line 249: mutual exclusivity and requiredness for
manualvsawsaligns with the union discriminator behavior.Doc tweak suggestion (optional)
- Deprecated: There is no logic listening to this resource type, we plan to remove it in next release. + Deprecated: There is no logic listening to this resource type. Planned for removal in a future release.config/v1/types_kmsencryption.go (2)
33-46: Consider makingManualKMSConfig.Namerequired in the schema (instead of “optional but required by XValidation”).Right now Line 44-Line 46 (
+optional+omitempty) makesnameappear optional in OpenAPI, even though it’s required wheneverkms.type == Manual. If there’s no intended valid state withmanualpresent butnameabsent, prefer+requiredand dropomitemptyto better reflect the true API contract.
73-85: Minor doc/wording nits onManualKMSProvidercomment.Line 82-Line 84 reads a bit ungrammatical (“represents a supported KMS provider is managed…”). Consider tightening wording since this is public API surface.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (9)
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yamlconfig/v1/types_kmsencryption.goconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
config/v1/zz_generated.swagger_doc_generated.go (1)
config/v1/types_kmsencryption.go (2)
KMSConfig(8-31)ManualKMSConfig(34-46)
config/v1/types_kmsencryption.go (1)
config/v1/zz_generated.swagger_doc_generated.go (2)
ManualKMSConfig(2354-2356)AWSKMSConfig(2334-2336)
config/v1/zz_generated.deepcopy.go (2)
config/v1/types_kmsencryption.go (1)
ManualKMSConfig(34-46)config/v1/zz_generated.swagger_doc_generated.go (1)
ManualKMSConfig(2354-2356)
🔇 Additional comments (17)
payload-manifests/crds/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml (1)
230-237: KMS provider enum update looks consistent with the Go union discriminator.
typenow allowsManual, matchingconfig/v1/types_kmsencryption.go’sKMSProviderTypeusage.config/v1/zz_generated.swagger_doc_generated.go (1)
2338-2347: KMSConfig swagger doc updates are coherent (Manual added + AWS marked deprecated).
Thetype/manual/awsfield descriptions line up with the intent inconfig/v1/types_kmsencryption.go.openapi/generated_openapi/zz_generated.openapi.go (5)
328-328: LGTM!The new
ManualKMSConfigtype is correctly registered in the OpenAPI definitions map.
9175-9181: LGTM!Deprecation notice clearly communicates the intended removal of AWS KMS support.
16149-16169: LGTM!The
manualfield is correctly added with proper reference, and theawsfield includes appropriate deprecation notice consistent with the overall deprecation strategy.
16176-16190: LGTM!Discriminator mapping and dependencies are correctly updated to include the new
Manualtype alongside the deprecatedAWStype.
16579-16597: Verifynamefield constraints are enforced in source type.The
namefield description states it's "between 1 and 80 characters in length," but the generated OpenAPI schema lacks:
- A
Requiredarray markingnameas mandatoryMinLength/MaxLengthconstraintsSince this is generated code, ensure the source type definition (
types_kmsencryption.go) has appropriate kubebuilder markers or CEL validation rules. Thenamefield appears essential for constructing the socket path (unix:///var/run/kmsplugin/<name>.sock).config/v1/zz_generated.deepcopy.go (2)
3833-3846: DeepCopy forKMSConfig.Manuallooks correct (given currentManualKMSConfigshape).Line 3835-Line 3839 correctly allocates and copies
Manual. SinceManualKMSConfigcurrently contains only value fields,**out = **inis fine; ifManualKMSConfiglater gains slices/maps/pointers, ensure codegen switches to(*in).DeepCopyInto(*out)to avoid shallow copies.
4067-4081:ManualKMSConfigdeepcopy methods are fine.Line 4068-Line 4081 is a standard generated deepcopy for a value-only struct.
config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml (2)
8-31: Good coverage for Manual provider happy-path.Line 8-Line 31 adds a clear create-success case that exercises the new
kms.type: Manual+kms.manual.namepath.
148-169: Potential brittleness: multiple validations may fire; ensure stable asserted error.Line 148-Line 169 asserts the top-level “kms config is required when encryption type is KMS, and forbidden otherwise”. Depending on admission validation ordering, you might also get provider-specific violations (e.g., manual-vs-type) in some implementations. If you see flakes, consider relaxing the expectation (if the framework supports partial match/regex) to the most stable portion.
payload-manifests/crds/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml (1)
171-250: DevPreviewNoUpgrade CRD changes mirror TechPreviewNoUpgrade appropriately.Looks consistent with the Go API annotations (manual schema, enum update, and
aws/manualexclusivity validations).config/v1/types_kmsencryption.go (1)
3-31: XValidation + union wiring for Manual provider looks correct.Line 5-Line 30 properly enforces that
manualis present (with non-emptyname) ifftype == Manual, and forbidsmanualotherwise; same pattern foraws.openapi/openapi.json (4)
4456-4460: LGTM - Clear deprecation notice for AWSKMSConfig.The deprecation message clearly communicates the planned removal to API consumers.
4575-4587: Unrelated changes included.These conditional update risk additions (AcceptRisk, conditionalUpdateRisks, riskNames, etc.) appear unrelated to the KMS API changes described in the PR objectives. If these came from a base branch merge, this is expected. Otherwise, consider splitting unrelated changes into separate PRs for clearer review scope.
8547-8569: LGTM - KMSConfig properly extended with manual provider.The discriminated union is correctly updated:
awsfield marked deprecatedmanualfield added with proper reference- Discriminator mapping includes both providers
8803-8812: The source Go type forManualKMSConfigalready contains the required validation markers.The original review assumption is incorrect. Examining
config/v1/types_kmsencryption.go(lines 34–46), thenamefield already has the proper kubebuilder validation annotations:
+kubebuilder:validation:MaxLength=80+kubebuilder:validation:MinLength=1+kubebuilder:validation:XValidation:rule="!self.contains('/') && !self.contains('..')"...The real issue is in the code generation pipeline: the generated OpenAPI schema function in
openapi/generated_openapi/zz_generated.openapi.go(lines 16579–16597) does not translate these kubebuilder markers into the OpenAPI schema properties. The generated schema only containsType: []string{"string"}without the correspondingminLength,maxLength, or validation constraints.This is a code generation/tooling issue, not a source code deficiency. The generated
zz_generated.openapi.gofile needs to be regenerated or the code generation configuration must be fixed to properly extract and map validation markers to the OpenAPI schema.Likely an incorrect or invalid review comment.
| - name: Should fail to create KMS Manual provider without manual config | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: APIServer | ||
| spec: | ||
| encryption: | ||
| type: KMS | ||
| kms: | ||
| type: Manual | ||
| expectedError: "manual config with non-empty name is required when kms provider type is Manual" | ||
| - name: Should fail to create KMS Manual provider with empty name |
There was a problem hiding this comment.
Fix likely expectedError mismatch for missing manual config.
Line 40 expects manual config with non-empty name is required when kms provider type is Manual, but the API/CRD XValidation message includes , and forbidden otherwise (see config/v1/types_kmsencryption.go, Line 5). If the test harness does exact string matching, this will fail.
Proposed fix
- expectedError: "manual config with non-empty name is required when kms provider type is Manual"
+ expectedError: "manual config with non-empty name is required when kms provider type is Manual, and forbidden otherwise"🤖 Prompt for AI Agents
In @config/v1/tests/apiservers.config.openshift.io/KMSEncryptionProvider.yaml
around lines 31 - 41, The test's expectedError string for the case "Should fail
to create KMS Manual provider without manual config" is missing the trailing
clause from the CRD xValidation message (`, and forbidden otherwise`); update
the expectedError value to exactly match the XValidation message from
types_kmsencryption.go (i.e., include ", and forbidden otherwise") or change the
test to assert a containment/substring match against the canonical message so it
won't fail on exact string comparison.
5 participants
KMSConfigis gated behindKMSEncryptionProviderfeature gate which is in tech preview. This PR tries to add the changes proposed in openshift/enhancements#1900 EP.This PR;
AWSKMSConfigfield andAWSKMSProviderType, since they are not used by anywhere already (we'll tombstone them in next release)ManualKMSProviderType andNamefield for manually managed KMS pluginsNote: We'll have to backport these changes into 4.21.