CNTRLPLANE-3237: Introduce KMSProviderConfig in encryption-config Secret - #2163
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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 "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigrates KMS config storage from secret annotations to secret Data fields, introduces per-key KMS provider configs, renames Changes
Sequence DiagramsequenceDiagram
participant KeyCtrl as Key Controller
participant StateCtrl as State Controller
participant Collector as collectKMSProviderConfigs
participant ToSecret as encryptionconfig.ToSecret
participant Secrets as secrets.FromKeyState
participant Kube as Kubernetes API
KeyCtrl->>KeyCtrl: generateKeySecret (KMS mode)
KeyCtrl->>KeyCtrl: set ks.KMSEncryptionConfig and ks.KMSProviderConfig
KeyCtrl->>Secrets: FromKeyState(ks)
Secrets-->>KeyCtrl: Secret with Data[KMS encryption/config, KMS provider/config]
StateCtrl->>Collector: collectKMSProviderConfigs(desiredState)
Collector-->>StateCtrl: map[keyID]KMSProviderConfig
StateCtrl->>ToSecret: ToSecret(ns,name,encryptionCfg,kmsProviderConfigs)
ToSecret->>ToSecret: marshal provider configs into secret.Data[...] per keyID
ToSecret-->>StateCtrl: Secret with provider configs in Data
StateCtrl->>Kube: apply Secret
Kube-->>StateCtrl: Secret created/updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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 "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml (2)
54-253: Consider templating to reduce duplication in the test asset.The 10 nearly-identical container definitions differ only in the container name and socket path number. While this is a test asset and correctness is not affected, this repetition is error-prone to maintain.
If the templating system supports it (Go templates are used based on
{{ .Image }}), consider generating the containers programmatically:{{- range $i := seq 1 10 }} - name: kms-plugin-{{ $i }} image: {{ $.Image }} ... args: - | rm -f /var/run/kmsplugin/kms-{{ $i }}.sock exec /usr/local/bin/mock-kms-plugin -listen-addr=unix:///var/run/kmsplugin/kms-{{ $i }}.sock -config-file-path=/etc/softhsm-config.json {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml` around lines 54 - 253, Replace the ten near-identical container blocks (names like kms-plugin-1..kms-plugin-10 and socket paths /var/run/kmsplugin/kms-#.sock) with a Go-template range loop that iterates from 1 to 10, uses the loop index for the container name and socket filename, and references the image as $.Image; keep the same securityContext, command, args (but with the index substituted), and volumeMounts (socket, softhsm-config with subPath, softhsm-tokens) inside the loop so the behavior remains identical while removing duplication.
57-58: Consider replacingprivileged: truewith specific capabilities or removing it if unnecessary.All 10 plugin containers and the init container use
privileged: true, but their operations—listening on Unix sockets, reading configuration files, and accessing mounted volumes—do not require privileged mode. For a test environment, replacing this with specific capabilities (e.g.,CAP_CHOWN) or a non-privileged security context would align with least-privilege principles. If socket creation and file access work without elevated privileges, remove thesecurityContextentirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml` around lines 57 - 58, The DaemonSet uses securityContext with privileged: true for the init container and all plugin containers; change this to follow least-privilege by removing the privileged setting or replacing it with narrow POSIX capabilities (e.g., add a securityContext.capabilities.add list with only needed capabilities like CAP_CHOWN) or a readOnlyRootFilesystem / runAsNonRoot setup if possible; update the initContainer and container specs (look for securityContext and privileged: true entries) to either remove the entire securityContext when not needed or explicitly list minimal capabilities and non-root settings so sockets and file access still work without full privileged mode.pkg/operator/encryption/testing/helpers.go (1)
24-29: Test constants duplicate production constants.These local constants duplicate values from
pkg/operator/encryption/secrets/types.go. While this avoids import cycles, it creates a maintenance burden if the values change.Consider adding a comment noting these must stay in sync with
secrets.EncryptionSecretKMSEncryptionConfigandsecrets.EncryptionSecretKMSProviderConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/testing/helpers.go` around lines 24 - 29, The test constants (encryptionSecretKeyDataForTest, encryptionSecretMigratedTimestampForTest, encryptionSecretMigratedResourcesForTest, encryptionSecretKMSEncryptionConfigForTest, encryptionSecretKMSProviderConfigForTest) duplicate production values; add a clear comment above these declarations stating they must remain in sync with the production symbols in pkg/operator/encryption/secrets (specifically secrets.EncryptionSecretKMSEncryptionConfig and secrets.EncryptionSecretKMSProviderConfig) to avoid drift and explain why the duplication exists (to avoid import cycles).pkg/operator/encryption/controllers/key_controller.go (1)
280-289: Hardcoded KMS provider configuration noted.The TODO comment indicates these values will be replaced by API when ready. For Tech Preview, this is acceptable, but consider:
- These hardcoded values will need to be parameterized before GA
- The image reference uses a personal/support account registry path which may not be suitable for production
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/key_controller.go` around lines 280 - 289, The code currently assigns hardcoded KMS values in ks.KMSProviderConfig using state.KMSProviderConfig and state.VaultProviderConfig (fields Image, VaultAddress, VaultNamespace, TransitKey, TransitMount); replace these hardcoded literals with configurable sources (e.g., read from controller config, CR spec, environment variables or a ConfigMap/Secret) and fall back to safe defaults only for Tech Preview, ensure the image string is not a personal registry (make it configurable and validate it) and update/remove the TODO accordingly so the values are injected rather than baked into the key_controller.go logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 280-289: The code currently assigns hardcoded KMS values in
ks.KMSProviderConfig using state.KMSProviderConfig and state.VaultProviderConfig
(fields Image, VaultAddress, VaultNamespace, TransitKey, TransitMount); replace
these hardcoded literals with configurable sources (e.g., read from controller
config, CR spec, environment variables or a ConfigMap/Secret) and fall back to
safe defaults only for Tech Preview, ensure the image string is not a personal
registry (make it configurable and validate it) and update/remove the TODO
accordingly so the values are injected rather than baked into the
key_controller.go logic.
In `@pkg/operator/encryption/testing/helpers.go`:
- Around line 24-29: The test constants (encryptionSecretKeyDataForTest,
encryptionSecretMigratedTimestampForTest,
encryptionSecretMigratedResourcesForTest,
encryptionSecretKMSEncryptionConfigForTest,
encryptionSecretKMSProviderConfigForTest) duplicate production values; add a
clear comment above these declarations stating they must remain in sync with the
production symbols in pkg/operator/encryption/secrets (specifically
secrets.EncryptionSecretKMSEncryptionConfig and
secrets.EncryptionSecretKMSProviderConfig) to avoid drift and explain why the
duplication exists (to avoid import cycles).
In `@test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml`:
- Around line 54-253: Replace the ten near-identical container blocks (names
like kms-plugin-1..kms-plugin-10 and socket paths /var/run/kmsplugin/kms-#.sock)
with a Go-template range loop that iterates from 1 to 10, uses the loop index
for the container name and socket filename, and references the image as $.Image;
keep the same securityContext, command, args (but with the index substituted),
and volumeMounts (socket, softhsm-config with subPath, softhsm-tokens) inside
the loop so the behavior remains identical while removing duplication.
- Around line 57-58: The DaemonSet uses securityContext with privileged: true
for the init container and all plugin containers; change this to follow
least-privilege by removing the privileged setting or replacing it with narrow
POSIX capabilities (e.g., add a securityContext.capabilities.add list with only
needed capabilities like CAP_CHOWN) or a readOnlyRootFilesystem / runAsNonRoot
setup if possible; update the initContainer and container specs (look for
securityContext and privileged: true entries) to either remove the entire
securityContext when not needed or explicitly list minimal capabilities and
non-root settings so sockets and file access still work without full privileged
mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74d7db0f-ae9d-4fe6-adb1-90b5bb8f25cc
📒 Files selected for processing (18)
pkg/operator/encryption/controllers/helpers_test.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/deployer/unionrevisionedpod_test.gopkg/operator/encryption/encryptionconfig/config.gopkg/operator/encryption/encryptionconfig/config_test.gopkg/operator/encryption/encryptionconfig/secret.gopkg/operator/encryption/observer/observe_encryption_config_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/statemachine/transition_test.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.gotest/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml
d9bafe1 to
585e22b
Compare
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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 "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/secrets/secrets.go`:
- Around line 66-75: The new code drops the legacy annotation-backed KMS secret
read path, causing migration breakage; update the ToKeyState logic in secrets.go
so that after attempting to unmarshal EncryptionSecretKMSEncryptionConfig from
s.Data it falls back to checking the legacy annotation (the previous KMS
encryption config stored on the Secret's annotations), unmarshal that into
apiserverconfigv1.KMSConfiguration and set key.KMSEncryptionConfig, and only
return the "KMSEncryptionConfig can not be nil" error if both the Data field and
the legacy annotation are absent or both fail to parse; reference the
EncryptionSecretKMSEncryptionConfig symbol, s.Data / s.Annotations,
key.KMSEncryptionConfig and state.KeyState when making the change.
- Around line 135-149: FromKeyState currently allows serializing a key secret
with ks.Mode == state.KMS even when ks.KMSEncryptionConfig is nil, which breaks
round-trip with ToKeyState; update FromKeyState to validate and reject invalid
KMS states before writing: if ks.Mode == state.KMS and ks.KMSEncryptionConfig ==
nil return a clear error instead of producing a secret, and keep the existing
serialization of KMSEncryptionConfig and KMSProviderConfig
(EncryptionSecretKMSEncryptionConfig, EncryptionSecretKMSProviderConfig) only
when those fields are non-nil; reference the FromKeyState function, ks.Mode,
KMSEncryptionConfig, KMSProviderConfig, and the constants used for secret keys
when implementing the check and error return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 446c51cf-2cc5-43fb-8ddc-4a15e21f75ff
📒 Files selected for processing (13)
pkg/operator/encryption/controllers/helpers_test.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/deployer/unionrevisionedpod_test.gopkg/operator/encryption/encryptionconfig/secret.gopkg/operator/encryption/observer/observe_encryption_config_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/operator/encryption/deployer/unionrevisionedpod_test.go
- pkg/operator/encryption/controllers/helpers_test.go
- test/e2e-encryption/encryption_test.go
- pkg/operator/encryption/controllers/key_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/operator/encryption/observer/observe_encryption_config_test.go
- pkg/operator/encryption/controllers/key_controller.go
- pkg/operator/encryption/encryptionconfig/secret.go
- pkg/operator/encryption/controllers/state_controller_test.go
- pkg/operator/encryption/state/types.go
- pkg/operator/encryption/testing/helpers.go
585e22b to
e143c1e
Compare
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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 "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/operator/encryption/secrets/secrets.go (1)
135-149:⚠️ Potential issue | 🟠 MajorReject invalid KMS key states before writing the secret.
ToKeyStaterejects KMS secrets withoutKMSEncryptionConfig(lines 73-75), butFromKeyStatestill serializesks.Mode == state.KMSwith that field potentially unset. This makes the conversion non-round-trippable and lets callers persist a secret this package cannot read back.🛡️ Proposed fix
+ if ks.Mode == state.KMS && ks.KMSEncryptionConfig == nil { + return nil, fmt.Errorf("KMSEncryptionConfig cannot be nil when mode is KMS") + } + if ks.KMSEncryptionConfig != nil { kmsEncCfgJSON, err := json.Marshal(ks.KMSEncryptionConfig) if err != nil { return nil, err } s.Data[EncryptionSecretKMSEncryptionConfig] = kmsEncCfgJSON }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/secrets/secrets.go` around lines 135 - 149, FromKeyState currently allows serializing ks with ks.Mode == state.KMS even when ks.KMSEncryptionConfig is nil, producing secrets that ToKeyState cannot read back; update FromKeyState to validate and reject invalid KMS states before writing the secret by returning an error when ks.Mode == state.KMS and ks.KMSEncryptionConfig == nil (and likewise validate any other required KMS fields such as KMSProviderConfig if your domain requires it) instead of proceeding to marshal and set EncryptionSecretKMSEncryptionConfig/EncryptionSecretKMSProviderConfig in s.Data.
🧹 Nitpick comments (1)
test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml (1)
54-253: Consider templating the repeated container definitions.The 10 container blocks are nearly identical, differing only in the container name and socket index. While acceptable for a test asset, this creates maintenance burden if you need to change shared parameters (image, volumeMounts, securityContext).
Since this YAML already uses Go templating (
{{ .Index }},{{ .Image }}), you could reduce duplication by templating the containers as well:♻️ Proposed refactor using Go range template
containers: - - name: kms-plugin-1 - image: {{ .Image }} - imagePullPolicy: IfNotPresent - securityContext: - privileged: true - command: - - /bin/sh - - -c - args: - - | - rm -f /var/run/kmsplugin/kms-1.sock - exec /usr/local/bin/mock-kms-plugin -listen-addr=unix:///var/run/kmsplugin/kms-1.sock -config-file-path=/etc/softhsm-config.json - volumeMounts: - - name: socket - mountPath: /var/run/kmsplugin - - name: softhsm-config - mountPath: /etc/softhsm-config.json - subPath: softhsm-config.json - - name: softhsm-tokens - mountPath: /var/lib/softhsm/tokens - - name: kms-plugin-2 - ... (repeat for 3-10) +{{- range $i := list 1 2 3 4 5 6 7 8 9 10 }} + - name: kms-plugin-{{ $i }} + image: {{ $.Image }} + imagePullPolicy: IfNotPresent + securityContext: + privileged: true + command: + - /bin/sh + - -c + args: + - | + rm -f /var/run/kmsplugin/kms-{{ $i }}.sock + exec /usr/local/bin/mock-kms-plugin -listen-addr=unix:///var/run/kmsplugin/kms-{{ $i }}.sock -config-file-path=/etc/softhsm-config.json + volumeMounts: + - name: socket + mountPath: /var/run/kmsplugin + - name: softhsm-config + mountPath: /etc/softhsm-config.json + subPath: softhsm-config.json + - name: softhsm-tokens + mountPath: /var/lib/softhsm/tokens +{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml` around lines 54 - 253, Refactor the repeated kms-plugin-* container blocks into a Go template range to eliminate duplication: create a slice (e.g., .Plugins or .NumPlugins) and iterate (range) to render each container using a template body that sets name as "kms-plugin-{{index}}" and socket paths like /var/run/kmsplugin/kms-{{index}}. Keep shared fields (image / Image, imagePullPolicy, securityContext, command, args, volumeMounts, softhsm-config subPath) inside the templated block so one change updates all; update any references to container names or sockets (e.g., kms-plugin-1..kms-plugin-10 and -listen-addr=unix:///var/run/kmsplugin/kms-<index>.sock) to use the template index variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/operator/encryption/secrets/secrets.go`:
- Around line 135-149: FromKeyState currently allows serializing ks with ks.Mode
== state.KMS even when ks.KMSEncryptionConfig is nil, producing secrets that
ToKeyState cannot read back; update FromKeyState to validate and reject invalid
KMS states before writing the secret by returning an error when ks.Mode ==
state.KMS and ks.KMSEncryptionConfig == nil (and likewise validate any other
required KMS fields such as KMSProviderConfig if your domain requires it)
instead of proceeding to marshal and set
EncryptionSecretKMSEncryptionConfig/EncryptionSecretKMSProviderConfig in s.Data.
---
Nitpick comments:
In `@test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml`:
- Around line 54-253: Refactor the repeated kms-plugin-* container blocks into a
Go template range to eliminate duplication: create a slice (e.g., .Plugins or
.NumPlugins) and iterate (range) to render each container using a template body
that sets name as "kms-plugin-{{index}}" and socket paths like
/var/run/kmsplugin/kms-{{index}}. Keep shared fields (image / Image,
imagePullPolicy, securityContext, command, args, volumeMounts, softhsm-config
subPath) inside the templated block so one change updates all; update any
references to container names or sockets (e.g., kms-plugin-1..kms-plugin-10 and
-listen-addr=unix:///var/run/kmsplugin/kms-<index>.sock) to use the template
index variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04563983-6d64-435c-b21a-0f1e31caf275
📒 Files selected for processing (18)
pkg/operator/encryption/controllers/helpers_test.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/deployer/unionrevisionedpod_test.gopkg/operator/encryption/encryptionconfig/config.gopkg/operator/encryption/encryptionconfig/config_test.gopkg/operator/encryption/encryptionconfig/secret.gopkg/operator/encryption/observer/observe_encryption_config_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/statemachine/transition_test.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.gotest/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yaml
✅ Files skipped from review due to trivial changes (3)
- pkg/operator/encryption/controllers/helpers_test.go
- pkg/operator/encryption/deployer/unionrevisionedpod_test.go
- pkg/operator/encryption/statemachine/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/operator/encryption/encryptionconfig/config_test.go
- pkg/operator/encryption/observer/observe_encryption_config_test.go
- pkg/operator/encryption/secrets/types.go
- pkg/operator/encryption/encryptionconfig/config.go
- pkg/operator/encryption/controllers/key_controller_test.go
- test/e2e-encryption/encryption_test.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/encryptionconfig/secret.go
- pkg/operator/encryption/testing/helpers.go
- pkg/operator/encryption/controllers/key_controller.go
|
/retest |
e143c1e to
66123fd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/encryption/state/types.go (1)
43-44: Clarify theKMSEncryptionConfigfield comment.On Line 43, “Encoded” is misleading: this field holds typed config, while encoding happens during secret serialization.
Proposed wording tweak
- // Encoded KMSEncryptionConfig that stores the KMS related fields + // KMSEncryptionConfig stores KMS encryption-related fields. KMSEncryptionConfig *apiserverconfigv1.KMSConfiguration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/state/types.go` around lines 43 - 44, The comment for the KMSEncryptionConfig field is misleading by saying "Encoded"; update the comment on KMSEncryptionConfig (type *apiserverconfigv1.KMSConfiguration) to state that it holds the typed KMS configuration (KMS-related fields) and that any encoding/serialization is performed later when storing in a Secret, e.g., "Typed KMS configuration (apiserverconfigv1.KMSConfiguration); encoding/serialization occurs when persisted to a Secret."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/encryption/state/types.go`:
- Around line 43-44: The comment for the KMSEncryptionConfig field is misleading
by saying "Encoded"; update the comment on KMSEncryptionConfig (type
*apiserverconfigv1.KMSConfiguration) to state that it holds the typed KMS
configuration (KMS-related fields) and that any encoding/serialization is
performed later when storing in a Secret, e.g., "Typed KMS configuration
(apiserverconfigv1.KMSConfiguration); encoding/serialization occurs when
persisted to a Secret."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 94e98970-9ef2-4cff-8b60-a432226cd703
📒 Files selected for processing (13)
pkg/operator/encryption/controllers/helpers_test.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/deployer/unionrevisionedpod_test.gopkg/operator/encryption/encryptionconfig/secret.gopkg/operator/encryption/observer/observe_encryption_config_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/operator/encryption/controllers/helpers_test.go
- pkg/operator/encryption/controllers/key_controller.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/operator/encryption/deployer/unionrevisionedpod_test.go
- pkg/operator/encryption/secrets/types.go
- test/e2e-encryption/encryption_test.go
- pkg/operator/encryption/encryptionconfig/secret.go
- pkg/operator/encryption/secrets/secrets.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/controllers/state_controller_test.go
- pkg/operator/encryption/controllers/key_controller_test.go
- pkg/operator/encryption/testing/helpers.go
66123fd to
03cb2c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/operator/encryption/controllers/state_controller_test.go (2)
756-761: Avoid brittle raw JSON string comparison for provider config.This assertion can fail on harmless JSON formatting/order changes. Prefer unmarshalling both expected/actual and comparing structured values.
Proposed test hardening
+import "encoding/json" ... - providerConfigKey := secrets.EncryptionSecretKMSProviderConfig + "-1" - expectedProviderConfig := `{"vault":{"image":"quay.io/org/vault-kms-plugin@sha256:abc123","vaultAddress":"https://vault.example.com:8200","transitKey":"my-transit-key","transitMount":"transit"}}` - if string(actualSecret.Data[providerConfigKey]) != expectedProviderConfig { - ts.Errorf("unexpected kms-provider-config-1 in encryption-config secret: %s", actualSecret.Data[providerConfigKey]) - } +providerConfigKey := secrets.EncryptionSecretKMSProviderConfig + "-1" +expected := &state.KMSProviderConfig{ + Vault: &state.VaultProviderConfig{ + Image: "quay.io/org/vault-kms-plugin@sha256:abc123", + VaultAddress: "https://vault.example.com:8200", + TransitKey: "my-transit-key", + TransitMount: "transit", + }, +} +actual := &state.KMSProviderConfig{} +if err := json.Unmarshal(actualSecret.Data[providerConfigKey], actual); err != nil { + ts.Fatalf("failed to unmarshal kms provider config: %v", err) +} +if diff := cmp.Diff(expected, actual); diff != "" { + ts.Errorf("unexpected kms provider config (-want,+got): %s", diff) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/state_controller_test.go` around lines 756 - 761, The test currently compares the KMS provider config by raw JSON string (expectedProviderConfig) which is brittle; change it to unmarshal both the expectedProviderConfig and the actualSecret.Data[providerConfigKey] into structured values (e.g., map[string]interface{} or a small struct) and compare them using reflect.DeepEqual or cmp.Diff. Locate the variables providerConfigKey, expectedProviderConfig and actualSecret.Data in the test function in state_controller_test.go, replace the string equality check with JSON unmarshalling of both sides, and update the ts.Errorf to print a clear diff or the mismatched structured values when the comparison fails.
1352-1354: StrengthenTestCollectKMSProviderConfigsby asserting full config equality.Current checks only verify
config.Vault != nil; they won’t catch wrong values being collected for a key. Add acmp.Diff(vaultConfig, config)assertion per key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/state_controller_test.go` around lines 1352 - 1354, In TestCollectKMSProviderConfigs, strengthen the per-key assertion by replacing the lone nil-check of config.Vault with a full equality check against the expected vaultConfig: call cmp.Diff(vaultConfig, config) and if the diff is non-empty call t.Errorf("unexpected Vault config for key %q: %s", key, diff). Ensure the test file imports github.com/google/go-cmp/cmp (add the import if missing) and keep the existing nil-check or fold it into the cmp-based assertion so mismatches (not just nil) are reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 280-289: The controller is writing hardcoded Vault provider config
into secrets via ks.KMSProviderConfig (state.KMSProviderConfig ->
VaultProviderConfig) inside generateKeySecret; remove the hardcoded struct
assignment and instead populate ks.KMSProviderConfig from the
operator/API-server observed configuration (or leave it nil/omitted until the
real config is available), adding a nil-check before writing secrets to avoid
persisting placeholder values like the mock image or ":latest" tag; update
generateKeySecret to accept or look up the real KMS config (e.g., via a
passed-in config object or operator config getter) and write only validated
fields.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/state_controller_test.go`:
- Around line 756-761: The test currently compares the KMS provider config by
raw JSON string (expectedProviderConfig) which is brittle; change it to
unmarshal both the expectedProviderConfig and the
actualSecret.Data[providerConfigKey] into structured values (e.g.,
map[string]interface{} or a small struct) and compare them using
reflect.DeepEqual or cmp.Diff. Locate the variables providerConfigKey,
expectedProviderConfig and actualSecret.Data in the test function in
state_controller_test.go, replace the string equality check with JSON
unmarshalling of both sides, and update the ts.Errorf to print a clear diff or
the mismatched structured values when the comparison fails.
- Around line 1352-1354: In TestCollectKMSProviderConfigs, strengthen the
per-key assertion by replacing the lone nil-check of config.Vault with a full
equality check against the expected vaultConfig: call cmp.Diff(vaultConfig,
config) and if the diff is non-empty call t.Errorf("unexpected Vault config for
key %q: %s", key, diff). Ensure the test file imports
github.com/google/go-cmp/cmp (add the import if missing) and keep the existing
nil-check or fold it into the cmp-based assertion so mismatches (not just nil)
are reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03e1083e-9634-45ba-bd9b-3956a8377abd
📒 Files selected for processing (13)
pkg/operator/encryption/controllers/helpers_test.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/deployer/unionrevisionedpod_test.gopkg/operator/encryption/encryptionconfig/secret.gopkg/operator/encryption/observer/observe_encryption_config_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/state/types.gopkg/operator/encryption/testing/helpers.gotest/e2e-encryption/encryption_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/encryption/controllers/key_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/operator/encryption/observer/observe_encryption_config_test.go
- pkg/operator/encryption/deployer/unionrevisionedpod_test.go
- pkg/operator/encryption/controllers/helpers_test.go
- test/e2e-encryption/encryption_test.go
- pkg/operator/encryption/secrets/types.go
- pkg/operator/encryption/secrets/secrets.go
- pkg/operator/encryption/testing/helpers.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/encryptionconfig/secret.go
353e782 to
572df00
Compare
572df00 to
b4ec83a
Compare
|
/retitle CNTRLPLANE-3237: Introduce KMSProviderConfig |
openshift-ci
Bot
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
6c8ddbb to
6292f49
Compare
openshift-ci
Bot
added
the
do-not-merge/hold
ed082b3 to
639dafc
Compare
|
/retest |
7d56954 to
0a01a8c
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
These are the fake bumps that all seems to be working; |
| }) | ||
|
|
||
| // Collect KMS provider configs from read keys (which already include the write key). | ||
| // The same keyID appears across multiple resources (e.g. secrets and configmaps), |
There was a problem hiding this comment.
please explain why we have duplicates.
There was a problem hiding this comment.
I've updated the documentation to better explain the duplication. Does it make sense?
|
|
||
| // ProviderConfigKeyID extracts the keyID from a kms-provider-config data key. | ||
| // Returns the keyID and true if the key matches the "kms-provider-config-<keyID>" pattern. | ||
| func ProviderConfigKeyID(dataKey string) (string, bool) { |
There was a problem hiding this comment.
KeyIDFromProviderConfigSecretDataKey ?
| var providerConfigKeyRegex = regexp.MustCompile(`^kms-provider-config-(\d+)$`) | ||
|
|
||
| // ProviderConfigDataKey constructs the data key for storing a KMS provider config in the encryption-config Secret. | ||
| func ProviderConfigDataKey(keyID string) string { |
There was a problem hiding this comment.
ToProviderConfigSecretDataKeFor ?
| corev1 "k8s.io/api/core/v1" | ||
| ) | ||
|
|
||
| const providerConfigDataKeyFormat = "kms-provider-config-%s" |
There was a problem hiding this comment.
here we accept any string.
There was a problem hiding this comment.
Validation is added by parsing keyID to integer.
|
|
||
| const providerConfigDataKeyFormat = "kms-provider-config-%s" | ||
|
|
||
| var providerConfigKeyRegex = regexp.MustCompile(`^kms-provider-config-(\d+)$`) |
There was a problem hiding this comment.
but here we say the keyID can a number
|
|
||
| // ProviderConfigDataKey constructs the data key for storing a KMS provider config in the encryption-config Secret. | ||
| func ProviderConfigDataKey(keyID string) string { | ||
| return fmt.Sprintf(providerConfigDataKeyFormat, keyID) |
There was a problem hiding this comment.
so maybe this function should validate the keyID ?
There was a problem hiding this comment.
Integer parsing logic is added.
| } | ||
|
|
||
| encryption := apiServer.Spec.Encryption | ||
| // TODO: we'll allow updating some values such as timeout via unsupportedconfig overrides. |
| }, | ||
| } | ||
|
|
||
| func TestRoundtrip(t *testing.T) { |
There was a problem hiding this comment.
does this test validate a RoundTrip from encryptiondata.ToSecret to encryptiondata.FromSecret?
if not maybe worth adding a test like that that would ensure the cfg is preserved.
|
|
||
| const providerConfigDataKeyFormat = "kms-provider-config-%s" | ||
|
|
||
| var providerConfigKeyRegex = regexp.MustCompile(`^kms-provider-config-(\d+)$`) |
There was a problem hiding this comment.
maybe using strings.CutPrefix would be simpler ?
There was a problem hiding this comment.
Yes, it would be simpler. Added.
| kmsProviders = map[string]*configv1.KMSConfig{} | ||
| } | ||
| if _, exists := kmsProviders[key.Key.Name]; !exists { | ||
| kmsProviders[key.Key.Name] = key.KMSConfig.Provider |
There was a problem hiding this comment.
should we compare if the cfg we already collected is the same as the new one ?
There was a problem hiding this comment.
I think this is a good idea. But that would change the signature of FromEncryptionState function, since we should return error if there is any discrepancy.
Would it make sense we move forward as is to unblock Fabio and I'll add this check in followup PR by updating the signature;
func FromEncryptionState(encryptionState map[schema.GroupResource]state.GroupResourceState) (*Config, error) {
There was a problem hiding this comment.
or if you prefer, I can fix it in this PR as a separate commit.
There was a problem hiding this comment.
i'm ok with a follow-up PR.
0a01a8c to
5d6162f
Compare
| return "", false | ||
| } | ||
| if _, err := strconv.ParseUint(keyID, 10, 64); err != nil { | ||
| return "", false |
There was a problem hiding this comment.
Maybe we should return error here
There was a problem hiding this comment.
we expect the keyID to be of specific format if present.
There was a problem hiding this comment.
Updated. We filter out the other unrelated data keys. If we detect kms-provider-config-keyid, we will extract key id. If key id is not uint64 integer, we error out.
|
unrelated |
| return &Config{Encryption: encryptionConfig}, nil | ||
| var kmsProviders map[string]*configv1.KMSConfig | ||
| for key, value := range encryptionConfigSecret.Data { | ||
| keyID, ok := kms.KeyIDFromProviderConfigSecretDataKey(key) |
There was a problem hiding this comment.
could we explain why not all data field will have keyID?
| } | ||
|
|
||
| for keyID, providerConfig := range secretData.KMSProviders { | ||
| providerJSON, err := encoding.EncodeKMSConfig(providerConfig) |
| for keyID, providerConfig := range secretData.KMSProviders { | ||
| providerJSON, err := encoding.EncodeKMSConfig(providerConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to encode KMS provider config for key %s: %v", keyID, err) |
| // ToProviderConfigSecretDataKeyFor constructs the data key for storing a KMS provider config in the encryption-config Secret. | ||
| // The keyID must be a valid non-negative integer string. | ||
| func ToProviderConfigSecretDataKeyFor(keyID string) (string, error) { | ||
| if _, err := strconv.ParseUint(keyID, 10, 64); err != nil { |
There was a problem hiding this comment.
I think this is ok because keyID is uint64, right ?
There was a problem hiding this comment.
Yes, keyID is uint64
5d6162f to
41146d7
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
unrelated |
|
@ardaguclu: all tests passed! 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. |
4 participants
This is continuation of #2186 for the work that is described openshift/enhancements#1960
This PR carries the kms-provider-config into encryption-config Secret.