[WIP] CNTRLPLANE-3236: Kms plugin lifecycle#2169
Open
bertinatto wants to merge 4 commits into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughPersist KMS provider configs into secrets, add types and helpers to parse/decode KMS endpoints and encryption configs, implement Vault sidecar provider and PodSpec injection, and update controllers and tests to collect, propagate, and validate KMS provider-config data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,200,255,0.5)
actor Operator
end
participant State as DesiredState
participant ToSecret as encryptionconfig.ToSecret
participant K8s as Kubernetes API (Secrets/Pods)
participant SidecarProv as SidecarProvider (Vault)
Operator->>State: read desired keys + KMSProviderConfig
Operator->>Operator: collectKMSProviderConfigs(desired)
Operator->>ToSecret: ToSecret(ns,name,ec,kmsProviderConfigs)
ToSecret->>K8s: create/update Secret (includes provider-config entries)
Operator->>K8s: read encryption-config secret & credentials secret
Operator->>SidecarProv: NewSidecarProvider(providerConfig, creds)
SidecarProv->>Operator: BuildSidecarContainer(...) -> Container
Operator->>K8s: patch PodSpec (append sidecar + socket volume)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
pkg/operator/encryption/testing/helpers.go (1)
28-28: Prefer reusing the canonical exported secret key constant.Using
secrets.EncryptionSecretKMSProviderConfigdirectly here avoids drift if the key string changes later.♻️ Suggested tweak
- encryptionSecretKMSProviderConfigForTest = "encryption.apiserver.operator.openshift.io-kms-provider-config" + encryptionSecretKMSProviderConfigForTest = secrets.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` at line 28, Replace the hard-coded constant encryptionSecretKMSProviderConfigForTest with the canonical exported constant secrets.EncryptionSecretKMSProviderConfig: update the declaration that currently sets encryptionSecretKMSProviderConfigForTest = "encryption.apiserver.operator.openshift.io-kms-provider-config" to reuse the exported symbol (secrets.EncryptionSecretKMSProviderConfig) so the test helper always references the single source of truth and avoids string drift.test/e2e-encryption/encryption_test.go (1)
429-437: Consider asserting provider-config payload equivalence, not just key presence.Right now this only checks existence. Adding a JSON-equivalence assertion between
keySecret.Data[secrets.EncryptionSecretKMSProviderConfig]andencCfgSecret.Data[providerConfigKey]would catch propagation mismatches.✅ Strengthen assertion
require.Contains(t, keySecret.Data, secrets.EncryptionSecretKMSProviderConfig, "key secret must contain kms-provider-config data") @@ providerConfigKey := fmt.Sprintf("%s-8", secrets.EncryptionSecretKMSProviderConfig) require.Contains(t, encCfgSecret.Data, providerConfigKey, "encryption-config secret must contain kms-provider-config-8 data") + require.JSONEq( + t, + string(keySecret.Data[secrets.EncryptionSecretKMSProviderConfig]), + string(encCfgSecret.Data[providerConfigKey]), + "kms-provider-config payload should match between key secret and encryption-config secret", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption/encryption_test.go` around lines 429 - 437, Add a JSON-equivalence assertion between the KMS provider config stored in keySecret and the one in encCfgSecret instead of only checking key presence: read keySecret.Data[secrets.EncryptionSecretKMSProviderConfig] and encCfgSecret.Data[providerConfigKey], and assert their JSON payloads are equivalent (e.g., with require.JSONEq(t, string(keyData), string(encCfgData)) or by unmarshaling both into map[string]interface{} and require.Equal), referencing the existing variables keySecret, encCfgSecret, providerConfigKey and constant secrets.EncryptionSecretKMSProviderConfig.pkg/operator/encryption/state/types.go (1)
49-61: Consider adding aValidate()method for provider configs at this boundary.
Image,VaultAddress, andTransitKeyappear required semantically. Early validation here would fail fast and avoid late runtime failures in sidecar generation.🤖 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 49 - 61, Add a Validate() method on VaultProviderConfig (and optionally on KMSProviderConfig to delegate) that checks required fields Image, VaultAddress, and TransitKey are non-empty and returns a descriptive error; call VaultProviderConfig.Validate() from KMSProviderConfig.Validate() if Vault is non-nil so callers can perform a single validation step. Ensure the error messages identify which field is missing (e.g., "VaultProviderConfig: missing Image") and update any callers that construct KMSProviderConfig to invoke KMSProviderConfig.Validate() before sidecar/template generation.pkg/operator/encryption/controllers/key_controller_test.go (1)
364-365: Use the shared provider-config key constant in these assertions.These hardcoded keys are repeated and can drift from production constants. Prefer
secrets.EncryptionSecretKMSProviderConfigfor stability.♻️ Suggested cleanup
+ "github.com/openshift/library-go/pkg/operator/encryption/secrets" @@ - kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"] + kmsProviderConfigData := actualSecret.Data[secrets.EncryptionSecretKMSProviderConfig] @@ - kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"] + kmsProviderConfigData := actualSecret.Data[secrets.EncryptionSecretKMSProviderConfig] @@ - if kmsProviderConfigData, exists := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"]; exists { + if kmsProviderConfigData, exists := actualSecret.Data[secrets.EncryptionSecretKMSProviderConfig]; exists {Also applies to: 430-431, 561-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/key_controller_test.go` around lines 364 - 365, Replace the hardcoded secret data key string used in assertions with the shared constant to avoid drift: where the test reads actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"] (and the two other similar occurrences), use secrets.EncryptionSecretKMSProviderConfig instead; keep the rest of the assertion (e.g., comparing kmsProviderConfigData to expectedProviderConfig) unchanged so the tests reference the canonical key constant rather than a literal string.pkg/operator/encryption/controllers/state_controller_test.go (1)
756-761: Prefer structured compare over raw JSON string equality here.Comparing serialized JSON bytes directly is brittle. Decode and compare
state.KMSProviderConfiginstead.🤖 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 JSON by raw string (using providerConfigKey / expectedProviderConfig) which is brittle; instead decode the JSON from actualSecret.Data[providerConfigKey] into the same struct type used by state.KMSProviderConfig and compare the resulting Go value to state.KMSProviderConfig (or a deep-equal of the deserialized struct) to assert equivalence; update the test to unmarshal the JSON bytes, handle/unset any non-deterministic fields if needed, and use reflect.DeepEqual or cmp.Equal to compare against state.KMSProviderConfig rather than comparing raw JSON strings.
🤖 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-290: The code in key_controller.go sets ks.KMSProviderConfig
with hardcoded VaultProviderConfig values (Image, VaultAddress, VaultNamespace,
TransitKey, TransitMount) which must not be persisted; instead modify the code
that assigns ks.KMSProviderConfig (where ks.KMSProviderConfig =
&state.KMSProviderConfig{ Vault: &state.VaultProviderConfig{...}}) to read these
fields from the operator/API configuration (or return a reconciliation
error/requeue if the configuration is missing) and only populate
ks.KMSProviderConfig with validated, real values; do not write placeholder or
:latest image tags into the generated key secret.
In `@pkg/operator/encryption/controllers/state_controller.go`:
- Around line 175-194: collectKMSProviderConfigs currently silently keeps the
first KMS config for a duplicate ks.Key.Name; change it to detect when a
duplicate keyID has a different ks.KMSProviderConfig and surface that as an
error instead of silently skipping: update
collectKMSProviderConfigs(desiredState
map[schema.GroupResource]state.GroupResourceState) to return
(map[string]*state.KMSProviderConfig, error), iterate as before and when you
find an existing entry for ks.Key.Name, compare the stored config with the new
ks.KMSProviderConfig (deep equal); if they differ, return a descriptive error
(including the conflicting keyID and identifiers from both
state.KMSProviderConfig values) so callers can handle the conflict; otherwise
continue and return the map and nil error.
In `@pkg/operator/encryption/encryptionconfig/secret.go`:
- Around line 78-84: The loop over kmsProviderConfigs currently marshals each
config without checking for nil, which allows a nil to be serialized as JSON
null; update the loop in the function handling kmsProviderConfigs to detect when
config == nil and return an error (or otherwise fail fast) instead of
marshaling, referencing the loop over kmsProviderConfigs and the key
construction using secrets.EncryptionSecretKMSProviderConfig and keyID so you
locate the exact spot to add the nil-check and error return.
In `@pkg/operator/encryption/kms/plugins/vault.go`:
- Around line 17-23: The code in BuildSidecarContainer dereferences
kmsConfig.Endpoint without checking kmsConfig for nil; add a nil check at the
start of VaultSidecarProvider.BuildSidecarContainer (same function) to return a
descriptive error (e.g., "kmsConfig cannot be nil") if kmsConfig is nil before
any access to kmsConfig.Endpoint, ensuring subsequent uses of kmsConfig.Endpoint
are safe.
- Around line 36-42: Validate the required secret keys and Vault config fields
before constructing the container: check that v.Credentials.Data contains
non-empty "VAULT_ROLE_ID" and "VAULT_SECRET_ID", and check that
v.Config.VaultAddress, v.Config.TransitMount, v.Config.TransitKey and
v.Config.Image (or the field used for the container image) are non-empty; if any
are missing return an explicit error immediately from the function that builds
the container (the method using variable v in vault.go) instead of proceeding to
use those values when creating the container.
- Around line 25-43: The current construction of args interpolates secrets and
config values (e.g., v.Credentials.Data["VAULT_SECRET_ID"],
v.Credentials.Data["VAULT_ROLE_ID"], kmsConfig.Endpoint, v.Config.VaultAddress/
VaultNamespace/TransitMount/TransitKey) into a shell string, which is unsafe;
change to use the exec form / argument slice instead of a single /bin/sh -c
string and move all secrets into environment variables or Kubernetes
SecretKeyRef (e.g., set VAULT_SECRET_ID and VAULT_ROLE_ID via Env/SecretKeyRef)
rather than embedding them in args, then build the command as an explicit argv
slice like ["/vault-kube-kms", "-listen-address", kmsConfig.Endpoint,
"-vault-address", v.Config.VaultAddress, ...] and remove the temporary echo to
/tmp/secret-id; ensure the Pod/Cmd construction that sets Privileged=true uses
these env refs and not interpolated values.
In `@pkg/operator/encryption/kms/sidecar.go`:
- Around line 27-35: Add nil-checks for the provider and kmsConfig at the start
of AddSidecarContainer: return an error if provider == nil or kmsConfig == nil
before calling provider.BuildSidecarContainer. Reference the function
AddSidecarContainer and the KMSSidecarProvider interface (and its
BuildSidecarContainer method) in your change so the nil validation clearly
precedes any invocation of provider.BuildSidecarContainer to avoid panics or
downstream nil dereferences.
- Around line 16-24: NewSidecarProvider currently dereferences config without a
nil check and constructs a VaultSidecarProvider without passing credentials,
causing a potential panic and later failure; update NewSidecarProvider to first
validate config is non-nil (return an error if nil), then when config.Vault !=
nil return &plugins.VaultSidecarProvider{Config: config.Vault, Credentials:
credentials} (or the actual field name used by VaultSidecarProvider) so
credentials are forwarded, and keep the default unsupported-provider error path
unchanged.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 364-365: Replace the hardcoded secret data key string used in
assertions with the shared constant to avoid drift: where the test reads
actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"]
(and the two other similar occurrences), use
secrets.EncryptionSecretKMSProviderConfig instead; keep the rest of the
assertion (e.g., comparing kmsProviderConfigData to expectedProviderConfig)
unchanged so the tests reference the canonical key constant rather than a
literal string.
In `@pkg/operator/encryption/controllers/state_controller_test.go`:
- Around line 756-761: The test currently compares the KMS provider JSON by raw
string (using providerConfigKey / expectedProviderConfig) which is brittle;
instead decode the JSON from actualSecret.Data[providerConfigKey] into the same
struct type used by state.KMSProviderConfig and compare the resulting Go value
to state.KMSProviderConfig (or a deep-equal of the deserialized struct) to
assert equivalence; update the test to unmarshal the JSON bytes, handle/unset
any non-deterministic fields if needed, and use reflect.DeepEqual or cmp.Equal
to compare against state.KMSProviderConfig rather than comparing raw JSON
strings.
In `@pkg/operator/encryption/state/types.go`:
- Around line 49-61: Add a Validate() method on VaultProviderConfig (and
optionally on KMSProviderConfig to delegate) that checks required fields Image,
VaultAddress, and TransitKey are non-empty and returns a descriptive error; call
VaultProviderConfig.Validate() from KMSProviderConfig.Validate() if Vault is
non-nil so callers can perform a single validation step. Ensure the error
messages identify which field is missing (e.g., "VaultProviderConfig: missing
Image") and update any callers that construct KMSProviderConfig to invoke
KMSProviderConfig.Validate() before sidecar/template generation.
In `@pkg/operator/encryption/testing/helpers.go`:
- Line 28: Replace the hard-coded constant
encryptionSecretKMSProviderConfigForTest with the canonical exported constant
secrets.EncryptionSecretKMSProviderConfig: update the declaration that currently
sets encryptionSecretKMSProviderConfigForTest =
"encryption.apiserver.operator.openshift.io-kms-provider-config" to reuse the
exported symbol (secrets.EncryptionSecretKMSProviderConfig) so the test helper
always references the single source of truth and avoids string drift.
In `@test/e2e-encryption/encryption_test.go`:
- Around line 429-437: Add a JSON-equivalence assertion between the KMS provider
config stored in keySecret and the one in encCfgSecret instead of only checking
key presence: read keySecret.Data[secrets.EncryptionSecretKMSProviderConfig] and
encCfgSecret.Data[providerConfigKey], and assert their JSON payloads are
equivalent (e.g., with require.JSONEq(t, string(keyData), string(encCfgData)) or
by unmarshaling both into map[string]interface{} and require.Equal), referencing
the existing variables keySecret, encCfgSecret, providerConfigKey and constant
secrets.EncryptionSecretKMSProviderConfig.
🪄 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: 5eb90795-9c46-4f8d-b9b2-3d434ec105c6
📒 Files selected for processing (19)
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/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/kms/plugins/vault.gopkg/operator/encryption/kms/plugins/vault_test.gopkg/operator/encryption/kms/sidecar.gopkg/operator/encryption/kms/sidecar_test.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
Comment on lines
+17
to
+23
| func (v *VaultSidecarProvider) BuildSidecarContainer(containerName string, kmsConfig *apiserverv1.KMSConfiguration) (corev1.Container, error) { | ||
| if v.Config == nil { | ||
| return corev1.Container{}, fmt.Errorf("vault config cannot be nil") | ||
| } | ||
| if v.Credentials == nil { | ||
| return corev1.Container{}, fmt.Errorf("vault credentials cannot be nil") | ||
| } |
There was a problem hiding this comment.
Add a nil check for kmsConfig before using Endpoint.
Line 37 dereferences kmsConfig.Endpoint without validating kmsConfig, which can panic.
Suggested fix
func (v *VaultSidecarProvider) BuildSidecarContainer(containerName string, kmsConfig *apiserverv1.KMSConfiguration) (corev1.Container, error) {
if v.Config == nil {
return corev1.Container{}, fmt.Errorf("vault config cannot be nil")
}
if v.Credentials == nil {
return corev1.Container{}, fmt.Errorf("vault credentials cannot be nil")
}
+ if kmsConfig == nil {
+ return corev1.Container{}, fmt.Errorf("kms configuration cannot be nil")
+ }Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 17 - 23, The code
in BuildSidecarContainer dereferences kmsConfig.Endpoint without checking
kmsConfig for nil; add a nil check at the start of
VaultSidecarProvider.BuildSidecarContainer (same function) to return a
descriptive error (e.g., "kmsConfig cannot be nil") if kmsConfig is nil before
any access to kmsConfig.Endpoint, ensuring subsequent uses of kmsConfig.Endpoint
are safe.
Comment on lines
+25
to
+43
| args := fmt.Sprintf(` | ||
| echo "%s" > /tmp/secret-id | ||
| exec /vault-kube-kms \ | ||
| -listen-address=%s \ | ||
| -vault-address=%s \ | ||
| -vault-namespace=%s \ | ||
| -transit-mount=%s \ | ||
| -transit-key=%s \ | ||
| -log-level=debug-extended \ | ||
| -approle-role-id=%s \ | ||
| -approle-secret-id-path=/tmp/secret-id`, | ||
| v.Credentials.Data["VAULT_SECRET_ID"], | ||
| kmsConfig.Endpoint, | ||
| v.Config.VaultAddress, | ||
| v.Config.VaultNamespace, | ||
| v.Config.TransitMount, | ||
| v.Config.TransitKey, | ||
| v.Credentials.Data["VAULT_ROLE_ID"], | ||
| ) |
There was a problem hiding this comment.
Avoid interpolating secret/config values into /bin/sh -c command text.
Lines 26-35 inject dynamic values directly into a shell script. A crafted secret/config value can alter command execution; this is especially risky with Line 52 setting Privileged=true.
Suggested direction
- args := fmt.Sprintf(`
- echo "%s" > /tmp/secret-id
- exec /vault-kube-kms \
- ...
- -approle-role-id=%s \
- -approle-secret-id-path=/tmp/secret-id`,
- v.Credentials.Data["VAULT_SECRET_ID"],
- ...
- v.Credentials.Data["VAULT_ROLE_ID"],
- )
+ args := `set -eu
+ printf '%s' "${VAULT_SECRET_ID}" > /tmp/secret-id
+ exec /vault-kube-kms \
+ -listen-address="${KMS_ENDPOINT}" \
+ -vault-address="${VAULT_ADDRESS}" \
+ -vault-namespace="${VAULT_NAMESPACE}" \
+ -transit-mount="${TRANSIT_MOUNT}" \
+ -transit-key="${TRANSIT_KEY}" \
+ -log-level=debug-extended \
+ -approle-role-id="${VAULT_ROLE_ID}" \
+ -approle-secret-id-path=/tmp/secret-id`Also move secret values to Env/SecretKeyRef instead of string interpolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 25 - 43, The
current construction of args interpolates secrets and config values (e.g.,
v.Credentials.Data["VAULT_SECRET_ID"], v.Credentials.Data["VAULT_ROLE_ID"],
kmsConfig.Endpoint, v.Config.VaultAddress/
VaultNamespace/TransitMount/TransitKey) into a shell string, which is unsafe;
change to use the exec form / argument slice instead of a single /bin/sh -c
string and move all secrets into environment variables or Kubernetes
SecretKeyRef (e.g., set VAULT_SECRET_ID and VAULT_ROLE_ID via Env/SecretKeyRef)
rather than embedding them in args, then build the command as an explicit argv
slice like ["/vault-kube-kms", "-listen-address", kmsConfig.Endpoint,
"-vault-address", v.Config.VaultAddress, ...] and remove the temporary echo to
/tmp/secret-id; ensure the Pod/Cmd construction that sets Privileged=true uses
these env refs and not interpolated values.
Comment on lines
+36
to
+42
| v.Credentials.Data["VAULT_SECRET_ID"], | ||
| kmsConfig.Endpoint, | ||
| v.Config.VaultAddress, | ||
| v.Config.VaultNamespace, | ||
| v.Config.TransitMount, | ||
| v.Config.TransitKey, | ||
| v.Credentials.Data["VAULT_ROLE_ID"], |
There was a problem hiding this comment.
Validate required secret keys and required Vault fields before building the container.
Lines 36/42 assume VAULT_ROLE_ID and VAULT_SECRET_ID exist; Line 48 assumes Image is non-empty. Failing fast here prevents opaque runtime failures.
Suggested fix
+ if len(v.Config.Image) == 0 {
+ return corev1.Container{}, fmt.Errorf("vault image cannot be empty")
+ }
+ roleID, ok := v.Credentials.Data["VAULT_ROLE_ID"]
+ if !ok || len(roleID) == 0 {
+ return corev1.Container{}, fmt.Errorf("missing VAULT_ROLE_ID in credentials")
+ }
+ secretID, ok := v.Credentials.Data["VAULT_SECRET_ID"]
+ if !ok || len(secretID) == 0 {
+ return corev1.Container{}, fmt.Errorf("missing VAULT_SECRET_ID in credentials")
+ }Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 36 - 42, Validate
the required secret keys and Vault config fields before constructing the
container: check that v.Credentials.Data contains non-empty "VAULT_ROLE_ID" and
"VAULT_SECRET_ID", and check that v.Config.VaultAddress, v.Config.TransitMount,
v.Config.TransitKey and v.Config.Image (or the field used for the container
image) are non-empty; if any are missing return an explicit error immediately
from the function that builds the container (the method using variable v in
vault.go) instead of proceeding to use those values when creating the container.
Comment on lines
+27
to
+35
| func AddSidecarContainer(podSpec *corev1.PodSpec, provider KMSSidecarProvider, containerName string, kmsConfig *apiserverv1.KMSConfiguration, | ||
| ) error { | ||
| if podSpec == nil { | ||
| return fmt.Errorf("pod spec cannot be nil") | ||
| } | ||
|
|
||
| container, err := provider.BuildSidecarContainer(containerName, kmsConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build sidecar container: %w", err) |
There was a problem hiding this comment.
Validate provider and kmsConfig inputs before invoking provider code.
AddSidecarContainer only checks podSpec; nil provider/kmsConfig can still trigger panics or downstream nil dereferences.
Suggested fix
func AddSidecarContainer(podSpec *corev1.PodSpec, provider KMSSidecarProvider, containerName string, kmsConfig *apiserverv1.KMSConfiguration,
) error {
if podSpec == nil {
return fmt.Errorf("pod spec cannot be nil")
}
+ if provider == nil {
+ return fmt.Errorf("kms sidecar provider cannot be nil")
+ }
+ if kmsConfig == nil {
+ return fmt.Errorf("kms configuration cannot be nil")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func AddSidecarContainer(podSpec *corev1.PodSpec, provider KMSSidecarProvider, containerName string, kmsConfig *apiserverv1.KMSConfiguration, | |
| ) error { | |
| if podSpec == nil { | |
| return fmt.Errorf("pod spec cannot be nil") | |
| } | |
| container, err := provider.BuildSidecarContainer(containerName, kmsConfig) | |
| if err != nil { | |
| return fmt.Errorf("failed to build sidecar container: %w", err) | |
| func AddSidecarContainer(podSpec *corev1.PodSpec, provider KMSSidecarProvider, containerName string, kmsConfig *apiserverv1.KMSConfiguration, | |
| ) error { | |
| if podSpec == nil { | |
| return fmt.Errorf("pod spec cannot be nil") | |
| } | |
| if provider == nil { | |
| return fmt.Errorf("kms sidecar provider cannot be nil") | |
| } | |
| if kmsConfig == nil { | |
| return fmt.Errorf("kms configuration cannot be nil") | |
| } | |
| container, err := provider.BuildSidecarContainer(containerName, kmsConfig) | |
| if err != nil { | |
| return fmt.Errorf("failed to build sidecar container: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/sidecar.go` around lines 27 - 35, Add nil-checks
for the provider and kmsConfig at the start of AddSidecarContainer: return an
error if provider == nil or kmsConfig == nil before calling
provider.BuildSidecarContainer. Reference the function AddSidecarContainer and
the KMSSidecarProvider interface (and its BuildSidecarContainer method) in your
change so the nil validation clearly precedes any invocation of
provider.BuildSidecarContainer to avoid panics or downstream nil dereferences.
ardaguclu
reviewed
Apr 21, 2026
Member
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
I dropped some comments but all of them are about mechanical changes. I think, core semantics is nice and aligns what we discussed.
| v.Credentials.Data["VAULT_ROLE_ID"], | ||
| ) | ||
|
|
||
| privileged := true |
Member
There was a problem hiding this comment.
Is there any particular reason to run the container privileged?
Member
Author
There was a problem hiding this comment.
I needed this to set up the UDS in the hostpath dir. Added a FIXME to remove this once I move to emptyDir
Member
Author
There was a problem hiding this comment.
Implemented emptyDir and removed this. I haven't tested it yet, let's see
| return corev1.Container{ | ||
| Name: containerName, | ||
| Image: v.Config.Image, | ||
| Command: []string{"/bin/sh", "-c"}, |
Member
There was a problem hiding this comment.
I think we shouldn't have this Command anymore
Member
Author
There was a problem hiding this comment.
I still need it because I'm temporarily doing echo "%s" > /tmp/secret-id before calling the plugin binary, so technically I need to run 2 commands, that's why I call /bin/sh -c instead of using the entrypoint. Once I can read the secret ID from the encryption configuration, then I'll be able to drop the echo command and the Command field.
| return nil | ||
| } | ||
|
|
||
| func ParseKeyIDFromEndpoint(endpoint string) (string, error) { |
Member
There was a problem hiding this comment.
Would it be better to directly extract key id from;
regexp.MustCompile(`^unix:///var/run/kmplugin/kms-(\d+)\.sock$`)
Member
There was a problem hiding this comment.
In my opinion, this commit is ready to merge whether before the API changes or ,ideally, after the API changes.
But it looks like this commit depends on #2163 to merge first.
| } | ||
| } | ||
|
|
||
| func AddSidecarContainer(podSpec *corev1.PodSpec, provider KMSSidecarProvider, containerName string, kmsConfig *apiserverv1.KMSConfiguration, |
Member
There was a problem hiding this comment.
Maybe this just returns Container and the caller uses its instead of implicit assignment.
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
9116ac8 to
57a97ab
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
pkg/operator/encryption/kms/sidecar.go (2)
16-24:⚠️ Potential issue | 🔴 CriticalGuard
configand forwardcredentialswhen creatingVaultSidecarProvider.
configcan panic when nil, andcredentialsis currently dropped, which breaks Vault sidecar construction later.Proposed fix
func NewSidecarProvider(config *state.KMSProviderConfig, credentials *corev1.Secret) (KMSSidecarProvider, error) { + if config == nil { + return nil, fmt.Errorf("kms provider configuration cannot be nil") + } switch { case config.Vault != nil: + if credentials == nil { + return nil, fmt.Errorf("vault credentials cannot be nil") + } return &plugins.VaultSidecarProvider{ - Config: config.Vault, + Config: config.Vault, + Credentials: credentials, }, nil default: return nil, fmt.Errorf("unsupported KMS provider configuration") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/sidecar.go` around lines 16 - 24, NewSidecarProvider should first nil-check the config before accessing config.Vault to avoid a panic; if config is nil return a clear error. When returning the Vault sidecar, pass the credentials through into the returned plugins.VaultSidecarProvider (i.e., set both Config: config.Vault and Credentials: credentials) so the Vault sidecar has the secret data it needs. Ensure the error paths remain unchanged and reference NewSidecarProvider and plugins.VaultSidecarProvider when making these changes.
27-34:⚠️ Potential issue | 🟠 MajorValidate
providerandkmsConfigbefore callingBuildSidecarContainer.The current call path can panic on nil
provideror nilkmsConfig(depending on provider implementation).Proposed fix
func AddSidecarContainer(podSpec *corev1.PodSpec, provider KMSSidecarProvider, containerName string, kmsConfig *apiserverv1.KMSConfiguration, ) error { if podSpec == nil { return fmt.Errorf("pod spec cannot be nil") } + if provider == nil { + return fmt.Errorf("kms sidecar provider cannot be nil") + } + if kmsConfig == nil { + return fmt.Errorf("kms configuration cannot be nil") + } container, err := provider.BuildSidecarContainer(containerName, kmsConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/sidecar.go` around lines 27 - 34, Check for nil inputs before calling provider.BuildSidecarContainer: in AddSidecarContainer validate that provider != nil and kmsConfig != nil and return a clear error (e.g., fmt.Errorf("provider cannot be nil") or fmt.Errorf("kmsConfig cannot be nil")) rather than proceeding; perform these checks before the existing call to provider.BuildSidecarContainer so you never dereference a nil KMSSidecarProvider or pass a nil kmsConfig into the provider implementation.pkg/operator/encryption/kms/plugins/vault.go (2)
17-23:⚠️ Potential issue | 🔴 CriticalAdd nil-check for
kmsConfigbefore using it.
kmsConfig.Endpointis dereferenced later without guardingkmsConfig.Proposed fix
func (v *VaultSidecarProvider) BuildSidecarContainer(containerName string, kmsConfig *apiserverv1.KMSConfiguration) (corev1.Container, error) { if v.Config == nil { return corev1.Container{}, fmt.Errorf("vault config cannot be nil") } if v.Credentials == nil { return corev1.Container{}, fmt.Errorf("vault credentials cannot be nil") } + if kmsConfig == nil { + return corev1.Container{}, fmt.Errorf("kms configuration cannot be nil") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 17 - 23, In BuildSidecarContainer, add a nil-check for the kmsConfig parameter before any use (e.g., before accessing kmsConfig.Endpoint) and return a clear error if it's nil; update the initial validation block that checks v.Config and v.Credentials to also check kmsConfig, returning something like fmt.Errorf("kmsConfig cannot be nil") from the BuildSidecarContainer function to avoid dereferencing a nil pointer.
25-43:⚠️ Potential issue | 🔴 CriticalAvoid interpolating untrusted values into
/bin/sh -c, and validate required inputs first.Secret/config values are injected into shell text, which is command-injection prone; also required keys/fields are not validated before use.
Safer pattern (validate first + use env expansion in script)
+ roleID, ok := v.Credentials.Data["VAULT_ROLE_ID"] + if !ok || len(roleID) == 0 { + return corev1.Container{}, fmt.Errorf("missing VAULT_ROLE_ID in credentials") + } + secretID, ok := v.Credentials.Data["VAULT_SECRET_ID"] + if !ok || len(secretID) == 0 { + return corev1.Container{}, fmt.Errorf("missing VAULT_SECRET_ID in credentials") + } + if len(v.Config.Image) == 0 || len(v.Config.VaultAddress) == 0 || len(v.Config.TransitKey) == 0 || len(v.Config.TransitMount) == 0 { + return corev1.Container{}, fmt.Errorf("vault provider config is incomplete") + } - args := fmt.Sprintf(` - echo "%s" > /tmp/secret-id + args := ` + set -eu + printf '%s' "$VAULT_SECRET_ID" > /tmp/secret-id exec /vault-kube-kms \ - -listen-address=%s \ - -vault-address=%s \ - -vault-namespace=%s \ - -transit-mount=%s \ - -transit-key=%s \ + -listen-address="$KMS_ENDPOINT" \ + -vault-address="$VAULT_ADDRESS" \ + -vault-namespace="$VAULT_NAMESPACE" \ + -transit-mount="$TRANSIT_MOUNT" \ + -transit-key="$TRANSIT_KEY" \ -log-level=debug-extended \ - -approle-role-id=%s \ - -approle-secret-id-path=/tmp/secret-id`, - v.Credentials.Data["VAULT_SECRET_ID"], - kmsConfig.Endpoint, - v.Config.VaultAddress, - v.Config.VaultNamespace, - v.Config.TransitMount, - v.Config.TransitKey, - v.Credentials.Data["VAULT_ROLE_ID"], - ) + -approle-role-id="$VAULT_ROLE_ID" \ + -approle-secret-id-path=/tmp/secret-id`And add matching
Enventries on the container forVAULT_SECRET_ID,VAULT_ROLE_ID,KMS_ENDPOINT,VAULT_ADDRESS,VAULT_NAMESPACE,TRANSIT_MOUNT,TRANSIT_KEY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 25 - 43, The current construction of args interpolates untrusted secret/config values (v.Credentials.Data["VAULT_SECRET_ID"], v.Credentials.Data["VAULT_ROLE_ID"], kmsConfig.Endpoint, v.Config.VaultAddress, v.Config.VaultNamespace, v.Config.TransitMount, v.Config.TransitKey) directly into a shell command, which is vulnerable to command injection and lacks validation; instead, first validate that each required key exists and is non-empty (check v.Credentials.Data for VAULT_SECRET_ID and VAULT_ROLE_ID and v.Config/* and kmsConfig.Endpoint), then stop if any are missing; change the invocation to avoid building a single /bin/sh -c string—use environment variables and the exec form or a small wrapper script that reads VAULT_SECRET_ID from the container Env (add Env entries: VAULT_SECRET_ID, VAULT_ROLE_ID, KMS_ENDPOINT, VAULT_ADDRESS, VAULT_NAMESPACE, TRANSIT_MOUNT, TRANSIT_KEY) and reference those env vars in the command (do not interpolate secret values into the args string), or write the secret to a file from the process environment in a safe controlled step rather than via shell interpolation.
🧹 Nitpick comments (1)
pkg/operator/encryption/kms/helpers_test.go (1)
50-53: Fix the “no digit key ID” test to isolate the intended condition.Current input fails because of missing
unix://, so it does not specifically validate non-digit key IDs.Proposed test fix
{ name: "no digit key ID", - endpoint: "/var/run/kmsplugin/kms-abc.sock", + endpoint: "unix:///var/run/kmsplugin/kms-abc.sock", wantErr: "unexpected KMS endpoint format", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/helpers_test.go` around lines 50 - 53, The "no digit key ID" unit test currently uses endpoint "/var/run/kmsplugin/kms-abc.sock" which fails early for missing scheme; update that test case (name: "no digit key ID") so the endpoint includes the expected "unix://" scheme (e.g. "unix:///var/run/kmsplugin/kms-abc.sock") and keep wantErr as "unexpected KMS endpoint format" (or the error string that validates non-digit key IDs) so the test isolates the non-digit key ID condition; modify the test case's endpoint field accordingly in helpers_test.go.
🤖 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/kms/plugins/vault.go`:
- Around line 17-23: In BuildSidecarContainer, add a nil-check for the kmsConfig
parameter before any use (e.g., before accessing kmsConfig.Endpoint) and return
a clear error if it's nil; update the initial validation block that checks
v.Config and v.Credentials to also check kmsConfig, returning something like
fmt.Errorf("kmsConfig cannot be nil") from the BuildSidecarContainer function to
avoid dereferencing a nil pointer.
- Around line 25-43: The current construction of args interpolates untrusted
secret/config values (v.Credentials.Data["VAULT_SECRET_ID"],
v.Credentials.Data["VAULT_ROLE_ID"], kmsConfig.Endpoint, v.Config.VaultAddress,
v.Config.VaultNamespace, v.Config.TransitMount, v.Config.TransitKey) directly
into a shell command, which is vulnerable to command injection and lacks
validation; instead, first validate that each required key exists and is
non-empty (check v.Credentials.Data for VAULT_SECRET_ID and VAULT_ROLE_ID and
v.Config/* and kmsConfig.Endpoint), then stop if any are missing; change the
invocation to avoid building a single /bin/sh -c string—use environment
variables and the exec form or a small wrapper script that reads VAULT_SECRET_ID
from the container Env (add Env entries: VAULT_SECRET_ID, VAULT_ROLE_ID,
KMS_ENDPOINT, VAULT_ADDRESS, VAULT_NAMESPACE, TRANSIT_MOUNT, TRANSIT_KEY) and
reference those env vars in the command (do not interpolate secret values into
the args string), or write the secret to a file from the process environment in
a safe controlled step rather than via shell interpolation.
In `@pkg/operator/encryption/kms/sidecar.go`:
- Around line 16-24: NewSidecarProvider should first nil-check the config before
accessing config.Vault to avoid a panic; if config is nil return a clear error.
When returning the Vault sidecar, pass the credentials through into the returned
plugins.VaultSidecarProvider (i.e., set both Config: config.Vault and
Credentials: credentials) so the Vault sidecar has the secret data it needs.
Ensure the error paths remain unchanged and reference NewSidecarProvider and
plugins.VaultSidecarProvider when making these changes.
- Around line 27-34: Check for nil inputs before calling
provider.BuildSidecarContainer: in AddSidecarContainer validate that provider !=
nil and kmsConfig != nil and return a clear error (e.g., fmt.Errorf("provider
cannot be nil") or fmt.Errorf("kmsConfig cannot be nil")) rather than
proceeding; perform these checks before the existing call to
provider.BuildSidecarContainer so you never dereference a nil KMSSidecarProvider
or pass a nil kmsConfig into the provider implementation.
---
Nitpick comments:
In `@pkg/operator/encryption/kms/helpers_test.go`:
- Around line 50-53: The "no digit key ID" unit test currently uses endpoint
"/var/run/kmsplugin/kms-abc.sock" which fails early for missing scheme; update
that test case (name: "no digit key ID") so the endpoint includes the expected
"unix://" scheme (e.g. "unix:///var/run/kmsplugin/kms-abc.sock") and keep
wantErr as "unexpected KMS endpoint format" (or the error string that validates
non-digit key IDs) so the test isolates the non-digit key ID condition; modify
the test case's endpoint field accordingly in helpers_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e81d4b57-bc0d-407c-88e7-ac6ff5e76760
📒 Files selected for processing (6)
pkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/kms/plugins/vault.gopkg/operator/encryption/kms/plugins/vault_test.gopkg/operator/encryption/kms/sidecar.gopkg/operator/encryption/kms/sidecar_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/operator/encryption/kms/plugins/vault_test.go
- pkg/operator/encryption/kms/sidecar_test.go
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
57a97ab to
81a4794
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/operator/encryption/kms/plugins/vault.go (2)
17-23:⚠️ Potential issue | 🔴 CriticalGuard
kmsConfigbefore accessingEndpoint.
kmsConfigis dereferenced without a nil check, which can panic.Suggested fix
func (v *VaultSidecarProvider) BuildSidecarContainer(containerName string, kmsConfig *apiserverv1.KMSConfiguration) (corev1.Container, error) { if v.Config == nil { return corev1.Container{}, fmt.Errorf("vault config cannot be nil") } if v.Credentials == nil { return corev1.Container{}, fmt.Errorf("vault credentials cannot be nil") } + if kmsConfig == nil { + return corev1.Container{}, fmt.Errorf("kms configuration cannot be nil") + }Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 17 - 23, The function VaultSidecarProvider.BuildSidecarContainer dereferences kmsConfig (accessing kmsConfig.Endpoint) without checking for nil; add a nil guard at the start of the function that returns an error if kmsConfig == nil, and ensure any subsequent uses of kmsConfig.Endpoint (and other fields) only occur after that check—update both the initial access and the later usage around where Endpoint is referenced.
25-43:⚠️ Potential issue | 🔴 CriticalAvoid embedding secrets/config into
/bin/sh -cscript text.This construction allows shell/script injection via dynamic values and exposes sensitive data in args. Prefer explicit argv + env/secret refs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 25 - 43, The current construction builds a shell script string in args (variable args) embedding secrets like v.Credentials.Data["VAULT_SECRET_ID"] and flags (kmsConfig.Endpoint, v.Config.*) which risks shell injection and leaks secrets; replace this by creating a secure temporary file for the secret using os.CreateTemp/os.WriteFile with restrictive permissions, then launch the /vault-kube-kms binary directly with exec.Command/CommandContext using an explicit argv slice (e.g., "/vault-kube-kms", "-listen-address", kmsConfig.Endpoint, "-vault-address", v.Config.VaultAddress, ... , "-approle-secret-id-path", tmpFilePath) and pass no secret content into a shell string, and avoid interpolating secrets into environment or log output; ensure you remove the temp file after use.
🧹 Nitpick comments (2)
pkg/operator/encryption/kms/plugins/vault_test.go (2)
57-67: Add anil kmsConfigtest case.Given
BuildSidecarContainerreadskmsConfig.Endpoint, this is an important failure-path test and will catch panic-prone regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault_test.go` around lines 57 - 67, Add a new table-driven test entry in vault_test.go that covers a nil kmsConfig to exercise the panic-prone path in BuildSidecarContainer: create a case similar to the "nil credentials" entry but set kmsConfig: nil, keep vaultConfig and credentials valid (e.g., same Image and VaultAddress and non-nil credentials), set containerName to "kms-plugin" and assert wantErr contains the expected error (e.g., an error indicating kmsConfig cannot be nil or a clear message matching how BuildSidecarContainer handles a nil kmsConfig when accessing kmsConfig.Endpoint) so the test fails if BuildSidecarContainer reads kmsConfig.Endpoint without a nil check.
90-111: Avoid locking tests to exact unsafe shell script content.Asserting full script equality makes future hardening (argv/env-based execution) noisy. Prefer asserting required flags/fields semantically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault_test.go` around lines 90 - 111, The test currently asserts exact equality of expectedArgs against container.Args[0], which locks the test to a specific shell script; instead, update the assertion to verify the presence of required flags and values semantically: for container.Args (and specifically container.Args[0]) assert that it contains the -listen-address, -vault-address, -vault-namespace, -transit-mount, -transit-key, -approle-role-id (tt.credentials.Data["VAULT_ROLE_ID"]) and -approle-secret-id-path flags and that the values match tt.kmsConfig.Endpoint and the tt.vaultConfig.* fields and the secret id (tt.credentials.Data["VAULT_SECRET_ID"]); also make the check for the secret-id write command optional or assert it with strings.Contains rather than full-script equality so future argv/env hardening won’t break the test.
🤖 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/kms/helpers_test.go`:
- Around line 50-53: The "no digit key ID" test fixture is currently missing the
"unix://" prefix so it conflicts with the "missing protocol" failure; update the
test case named "no digit key ID" in helpers_test.go to set endpoint to
"unix:///var/run/kmsplugin/kms-abc.sock" (keeping wantErr as "unexpected KMS
endpoint format") so the test isolates the non-numeric key ID path when
exercising the code that parses KMS endpoints.
In `@pkg/operator/encryption/kms/helpers.go`:
- Line 29: The call sites that still pass three arguments to
AddKMSPluginVolumeAndMountToPodSpec must be updated to the new two-argument
signature: locate any calls that pass (podSpec, containerName, <thirdArg>) and
remove the third argument so they call
AddKMSPluginVolumeAndMountToPodSpec(podSpec, containerName); specifically update
the call in the KMS preflight pod template where the old three-arg invocation
occurs to match the helper function AddKMSPluginVolumeAndMountToPodSpec(podSpec
*corev1.PodSpec, containerName string) error.
In `@pkg/operator/encryption/kms/plugins/vault.go`:
- Around line 45-54: Remove the hardcoded privileged mode: delete the local
privileged variable and stop setting SecurityContext.Privileged when building
the corev1.Container (the block that returns the container with Name:
containerName, Image: v.Config.Image, Command, Args and SecurityContext:
&corev1.SecurityContext{Privileged: &privileged}). Either omit the
SecurityContext entirely or construct it without the Privileged field so the
container runs unprivileged now that the KMS socket uses an EmptyDir volume.
In `@pkg/operator/encryption/kms/sidecar_test.go`:
- Around line 47-54: In TestNewSidecarProvider, strengthen the success-path
assertions by asserting that the returned provider from NewSidecarProvider is a
*plugins.VaultSidecarProvider and that its credentials field or accessor
contains the same credentials passed into the call; locate the test case where
provider, err := NewSidecarProvider(tt.config, credentials) is used and replace
the single require.NotNil(t, provider) with type-asserting the provider to
(*plugins.VaultSidecarProvider) (or using a provided accessor method) and assert
equality/containment between the provider's stored credentials and the input
credentials so credential wiring failures fail fast in the test.
---
Duplicate comments:
In `@pkg/operator/encryption/kms/plugins/vault.go`:
- Around line 17-23: The function VaultSidecarProvider.BuildSidecarContainer
dereferences kmsConfig (accessing kmsConfig.Endpoint) without checking for nil;
add a nil guard at the start of the function that returns an error if kmsConfig
== nil, and ensure any subsequent uses of kmsConfig.Endpoint (and other fields)
only occur after that check—update both the initial access and the later usage
around where Endpoint is referenced.
- Around line 25-43: The current construction builds a shell script string in
args (variable args) embedding secrets like
v.Credentials.Data["VAULT_SECRET_ID"] and flags (kmsConfig.Endpoint, v.Config.*)
which risks shell injection and leaks secrets; replace this by creating a secure
temporary file for the secret using os.CreateTemp/os.WriteFile with restrictive
permissions, then launch the /vault-kube-kms binary directly with
exec.Command/CommandContext using an explicit argv slice (e.g.,
"/vault-kube-kms", "-listen-address", kmsConfig.Endpoint, "-vault-address",
v.Config.VaultAddress, ... , "-approle-secret-id-path", tmpFilePath) and pass no
secret content into a shell string, and avoid interpolating secrets into
environment or log output; ensure you remove the temp file after use.
---
Nitpick comments:
In `@pkg/operator/encryption/kms/plugins/vault_test.go`:
- Around line 57-67: Add a new table-driven test entry in vault_test.go that
covers a nil kmsConfig to exercise the panic-prone path in
BuildSidecarContainer: create a case similar to the "nil credentials" entry but
set kmsConfig: nil, keep vaultConfig and credentials valid (e.g., same Image and
VaultAddress and non-nil credentials), set containerName to "kms-plugin" and
assert wantErr contains the expected error (e.g., an error indicating kmsConfig
cannot be nil or a clear message matching how BuildSidecarContainer handles a
nil kmsConfig when accessing kmsConfig.Endpoint) so the test fails if
BuildSidecarContainer reads kmsConfig.Endpoint without a nil check.
- Around line 90-111: The test currently asserts exact equality of expectedArgs
against container.Args[0], which locks the test to a specific shell script;
instead, update the assertion to verify the presence of required flags and
values semantically: for container.Args (and specifically container.Args[0])
assert that it contains the -listen-address, -vault-address, -vault-namespace,
-transit-mount, -transit-key, -approle-role-id
(tt.credentials.Data["VAULT_ROLE_ID"]) and -approle-secret-id-path flags and
that the values match tt.kmsConfig.Endpoint and the tt.vaultConfig.* fields and
the secret id (tt.credentials.Data["VAULT_SECRET_ID"]); also make the check for
the secret-id write command optional or assert it with strings.Contains rather
than full-script equality so future argv/env hardening won’t break the test.
🪄 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: d7c03785-2a43-480c-b0ff-5eba4b8171f1
📒 Files selected for processing (6)
pkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/kms/plugins/vault.gopkg/operator/encryption/kms/plugins/vault_test.gopkg/operator/encryption/kms/sidecar.gopkg/operator/encryption/kms/sidecar_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/encryption/kms/sidecar.go
| var kmsEndpointRegexp = regexp.MustCompile(`^unix:///var/run/kmsplugin/kms-(\d+)\.sock$`) | ||
|
|
||
| // AddKMSPluginVolumeAndMountToPodSpec adds an emptyDir volume for the KMS plugin socket and mounts it into the specified container. | ||
| func AddKMSPluginVolumeAndMountToPodSpec(podSpec *corev1.PodSpec, containerName string) error { |
There was a problem hiding this comment.
Update call sites for the new AddKMSPluginVolumeAndMountToPodSpec signature.
The function now takes 2 args, but Line 58 in pkg/operator/encryption/kms/preflight/pod_template.go still passes 3 args, which currently breaks typecheck/build.
Suggested fix
- if err = encryptionkms.AddKMSPluginVolumeAndMountToPodSpec(&pod.Spec, "kms-preflight-check", featureGateAccessor); err != nil {
+ if err = encryptionkms.AddKMSPluginVolumeAndMountToPodSpec(&pod.Spec, "kms-preflight-check"); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/helpers.go` at line 29, The call sites that still
pass three arguments to AddKMSPluginVolumeAndMountToPodSpec must be updated to
the new two-argument signature: locate any calls that pass (podSpec,
containerName, <thirdArg>) and remove the third argument so they call
AddKMSPluginVolumeAndMountToPodSpec(podSpec, containerName); specifically update
the call in the KMS preflight pod template where the old three-arg invocation
occurs to match the helper function AddKMSPluginVolumeAndMountToPodSpec(podSpec
*corev1.PodSpec, containerName string) error.
Comment on lines
+47
to
+54
| provider, err := NewSidecarProvider(tt.config, credentials) | ||
| if tt.wantErr != "" { | ||
| require.ErrorContains(t, err, tt.wantErr) | ||
| return | ||
| } | ||
| require.NoError(t, err) | ||
| require.NotNil(t, provider) | ||
| }) |
There was a problem hiding this comment.
Strengthen success-path assertions in TestNewSidecarProvider.
This test should assert that returned *plugins.VaultSidecarProvider includes the passed credentials, not just provider != nil; otherwise a broken wiring (credentials dropped) still passes and fails later at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/kms/sidecar_test.go` around lines 47 - 54, In
TestNewSidecarProvider, strengthen the success-path assertions by asserting that
the returned provider from NewSidecarProvider is a *plugins.VaultSidecarProvider
and that its credentials field or accessor contains the same credentials passed
into the call; locate the test case where provider, err :=
NewSidecarProvider(tt.config, credentials) is used and replace the single
require.NotNil(t, provider) with type-asserting the provider to
(*plugins.VaultSidecarProvider) (or using a provided accessor method) and assert
equality/containment between the provider's stored credentials and the input
credentials so credential wiring failures fail fast in the test.
81a4794 to
f940b14
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/operator/encryption/kms/helpers_test.go (1)
50-53:⚠️ Potential issue | 🟡 MinorFix the non-numeric key-ID fixture to isolate the intended failure mode.
At Line 52, the endpoint is missing
unix://, so this case duplicates the “no unix prefix” path instead of validating non-numeric key IDs.Proposed fix
{ name: "no digit key ID", - endpoint: "/var/run/kmsplugin/kms-abc.sock", + endpoint: "unix:///var/run/kmsplugin/kms-abc.sock", wantErr: "unexpected KMS endpoint format", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/helpers_test.go` around lines 50 - 53, The "no digit key ID" test case (the table entry with name "no digit key ID" in helpers_test.go) is missing the required "unix://" prefix so it currently duplicates the "no unix prefix" failure; update its endpoint to include the unix:// prefix and embed a non-numeric key ID in the socket path/query (e.g., a socket path that contains a non-digit key identifier) so the test exercises the non-numeric key-ID validation logic in the helper being tested.
🤖 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/kms/helpers_test.go`:
- Around line 50-53: The "no digit key ID" test case (the table entry with name
"no digit key ID" in helpers_test.go) is missing the required "unix://" prefix
so it currently duplicates the "no unix prefix" failure; update its endpoint to
include the unix:// prefix and embed a non-numeric key ID in the socket
path/query (e.g., a socket path that contains a non-digit key identifier) so the
test exercises the non-numeric key-ID validation logic in the helper being
tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e788cce-2877-4215-ab0e-7af5aa55e521
📒 Files selected for processing (6)
pkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/kms/plugins/vault.gopkg/operator/encryption/kms/plugins/vault_test.gopkg/operator/encryption/kms/sidecar.gopkg/operator/encryption/kms/sidecar_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/operator/encryption/kms/plugins/vault_test.go
- pkg/operator/encryption/kms/sidecar.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/encryption/kms/plugins/vault.go
- pkg/operator/encryption/kms/sidecar_test.go
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
0104816 to
1356633
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/operator/encryption/kms/plugins/vault.go (3)
17-23:⚠️ Potential issue | 🔴 CriticalGuard
kmsConfigbefore dereferencingEndpoint.Line 37 dereferences
kmsConfig.Endpointwithout checkingkmsConfigfor nil, which can panic.🛠️ Proposed fix
func (v *VaultSidecarProvider) BuildSidecarContainer(containerName string, kmsConfig *apiserverv1.KMSConfiguration) (corev1.Container, error) { if v.Config == nil { return corev1.Container{}, fmt.Errorf("vault config cannot be nil") } if v.Credentials == nil { return corev1.Container{}, fmt.Errorf("vault credentials cannot be nil") } + if kmsConfig == nil { + return corev1.Container{}, fmt.Errorf("kms configuration cannot be nil") + }Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 17 - 23, BuildSidecarContainer currently dereferences kmsConfig.Endpoint without validating kmsConfig; add a nil-check for the kmsConfig parameter at the start of VaultSidecarProvider.BuildSidecarContainer (alongside the existing v.Config and v.Credentials checks) and return a clear error (e.g., "kmsConfig cannot be nil") if it's nil, then proceed to use kmsConfig.Endpoint safely.
36-43:⚠️ Potential issue | 🟠 MajorValidate required secret keys and required Vault config fields before building the container.
Line 36/Line 42 and Line 47 assume required values exist. Missing keys/fields currently fail later and less clearly.
🧪 Proposed fail-fast validation
if v.Credentials == nil { return corev1.Container{}, fmt.Errorf("vault credentials cannot be nil") } + if len(v.Config.Image) == 0 { + return corev1.Container{}, fmt.Errorf("vault image cannot be empty") + } + if len(v.Config.VaultAddress) == 0 || len(v.Config.TransitMount) == 0 || len(v.Config.TransitKey) == 0 { + return corev1.Container{}, fmt.Errorf("vault config is missing required fields") + } + roleID, ok := v.Credentials.Data["VAULT_ROLE_ID"] + if !ok || len(roleID) == 0 { + return corev1.Container{}, fmt.Errorf("missing VAULT_ROLE_ID in credentials") + } + secretID, ok := v.Credentials.Data["VAULT_SECRET_ID"] + if !ok || len(secretID) == 0 { + return corev1.Container{}, fmt.Errorf("missing VAULT_SECRET_ID in credentials") + }Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 36 - 43, Validate presence of required secret keys and Vault config fields before constructing the container: check that v.Credentials.Data contains "VAULT_SECRET_ID" and "VAULT_ROLE_ID", and that kmsConfig.Endpoint and the fields on v.Config (VaultAddress, VaultNamespace, TransitMount, TransitKey) are non-empty; if any are missing return a clear error immediately from the function that builds the container (the scope using v.Credentials.Data and kmsConfig) rather than letting nil/empty values cause later failures.
25-43:⚠️ Potential issue | 🔴 CriticalDo not interpolate secrets/config into
/bin/sh -cscript text.Line 25–Line 43 directly inject dynamic values into shell script content. This is a command-injection risk and also leaks sensitive values into container args.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault.go` around lines 25 - 43, The current construction of the single string "args" in pkg/operator/encryption/kms/plugins/vault.go injects secrets (v.Credentials.Data["VAULT_SECRET_ID"], v.Credentials.Data["VAULT_ROLE_ID"]) and config values directly into a shell script which risks command injection and leaks secrets; instead stop building a /bin/sh -c payload and supply the executable and its arguments as a proper argv slice (or container args array) so values are passed as separate arguments, and handle secrets by mounting them as files or injecting them via secure env/secret volumes (write VAULT_SECRET_ID to a file inside the container via a secret volume with restrictive perms, reference the file path in the -approle-secret-id-path flag), i.e. replace the "args" string construction with explicit argument elements for /vault-kube-kms and use file-backed secret injection for v.Credentials.Data["VAULT_SECRET_ID"] and v.Credentials.Data["VAULT_ROLE_ID"] while keeping kmsConfig.Endpoint and v.Config.* values as normal non-interpolated args.
🧹 Nitpick comments (3)
pkg/operator/encryption/kms/helpers.go (2)
108-117: Include provider-config key in unmarshal failure context.Add the derived key to the unmarshal error to make multi-key secret failures easier to triage.
🛠️ Proposed refinement
var kmsProviderConfig state.KMSProviderConfig if err := json.Unmarshal(providerConfigData, &kmsProviderConfig); err != nil { - return nil, fmt.Errorf("failed to unmarshal KMS provider config: %w", err) + return nil, fmt.Errorf("failed to unmarshal KMS provider config for key %q: %w", providerConfigKey, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/helpers.go` around lines 108 - 117, GetKMSProviderConfig builds providerConfigKey and currently returns a generic error when json.Unmarshal(providerConfigData, &kmsProviderConfig) fails; update the error to include the derived providerConfigKey so you can identify which key's payload failed to unmarshal (e.g. change the error returned from json.Unmarshal to include providerConfigKey in the message and wrap the original err). Ensure the change references GetKMSProviderConfig, providerConfigKey, json.Unmarshal and kmsProviderConfig so the failure context is clear.
28-93: Avoid socket-path drift by centralizing constants.The socket directory is now part of parsing logic and pod mount logic in this file. Extract shared constants so path changes stay consistent.
♻️ Proposed refactor
+const ( + kmsPluginSocketDir = "/var/run/kmsplugin" + kmsEndpointPatternExpr = `^unix:///var/run/kmsplugin/kms-(\d+)\.sock$` +) + -var kmsEndpointRegexp = regexp.MustCompile(`^unix:///var/run/kmsplugin/kms-(\d+)\.sock$`) +var kmsEndpointRegexp = regexp.MustCompile(kmsEndpointPatternExpr)// Also reuse `kmsPluginSocketDir` in AddKMSPluginVolumeAndMountToPodSpec: // MountPath: kmsPluginSocketDir // Path: kmsPluginSocketDir🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/helpers.go` around lines 28 - 93, Create a single shared constant (e.g., kmsPluginSocketDir) for the socket directory and replace the hard-coded "/var/run/kmsplugin" occurrences in AddKMSPluginVolumeAndMountToPodSpec (MountPath and HostPath.Path) with that constant; also update kmsEndpointRegexp/ParseKeyIDFromEndpoint to build or reference the regexp using the same constant so the socket path in the regex stays in sync with the mount path (adjust regexp construction to use the constant safely, e.g., via fmt.Sprintf and regexp.QuoteMeta).pkg/operator/encryption/kms/plugins/vault_test.go (1)
70-112: Add a nilkmsConfigtest case to cover panic-prone input.Please add a table case where
kmsConfigisniland assert the expected error. This closes coverage on a critical dereference path inBuildSidecarContainer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/kms/plugins/vault_test.go` around lines 70 - 112, Add a table-driven test case to the existing tests loop that passes kmsConfig as nil to VaultSidecarProvider.BuildSidecarContainer (use the same VaultSidecarProvider, containerName, vaultConfig, and credentials symbols) and assert the function returns an error (use require.Error or require.ErrorContains with the expected message) instead of panicking; this ensures the nil dereference path in BuildSidecarContainer is exercised and the test verifies proper error handling when kmsConfig is nil.
🤖 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/kms/plugins/vault_test.go`:
- Around line 86-88: The test currently force-asserts that
container.SecurityContext and container.SecurityContext.Privileged are
non-nil/true but BuildSidecarContainer no longer sets a SecurityContext, causing
false failures; update the test in vault_test.go to handle a nil SecurityContext
by either (a) asserting that SecurityContext is nil when that's the expected
behavior from BuildSidecarContainer, or (b) checking for nil before
dereferencing and only asserting Privileged when SecurityContext is present (use
the local variable container returned from BuildSidecarContainer and the
SecurityContext.Privileged field in the conditional); ensure references to
BuildSidecarContainer, container, and SecurityContext.Privileged are used so the
change targets the correct assertions.
In `@pkg/operator/encryption/kms/plugins/vault.go`:
- Around line 21-23: The Vault provider validation in vault.go returns an error
if v.Credentials is nil, but the provider is constructed in
pkg/operator/encryption/kms/sidecar.go with only Config set; update the provider
construction in sidecar.go to also populate the Vault provider's Credentials
field (v.Credentials) with the appropriate Secret reference or parsed credential
object used by the sidecar so the nil check in vault.go passes; locate the
provider creation site in sidecar.go (where Config is assigned) and add the
credentials assignment there to ensure v.Credentials is non-nil before calling
into the Vault container creation logic.
---
Duplicate comments:
In `@pkg/operator/encryption/kms/plugins/vault.go`:
- Around line 17-23: BuildSidecarContainer currently dereferences
kmsConfig.Endpoint without validating kmsConfig; add a nil-check for the
kmsConfig parameter at the start of VaultSidecarProvider.BuildSidecarContainer
(alongside the existing v.Config and v.Credentials checks) and return a clear
error (e.g., "kmsConfig cannot be nil") if it's nil, then proceed to use
kmsConfig.Endpoint safely.
- Around line 36-43: Validate presence of required secret keys and Vault config
fields before constructing the container: check that v.Credentials.Data contains
"VAULT_SECRET_ID" and "VAULT_ROLE_ID", and that kmsConfig.Endpoint and the
fields on v.Config (VaultAddress, VaultNamespace, TransitMount, TransitKey) are
non-empty; if any are missing return a clear error immediately from the function
that builds the container (the scope using v.Credentials.Data and kmsConfig)
rather than letting nil/empty values cause later failures.
- Around line 25-43: The current construction of the single string "args" in
pkg/operator/encryption/kms/plugins/vault.go injects secrets
(v.Credentials.Data["VAULT_SECRET_ID"], v.Credentials.Data["VAULT_ROLE_ID"]) and
config values directly into a shell script which risks command injection and
leaks secrets; instead stop building a /bin/sh -c payload and supply the
executable and its arguments as a proper argv slice (or container args array) so
values are passed as separate arguments, and handle secrets by mounting them as
files or injecting them via secure env/secret volumes (write VAULT_SECRET_ID to
a file inside the container via a secret volume with restrictive perms,
reference the file path in the -approle-secret-id-path flag), i.e. replace the
"args" string construction with explicit argument elements for /vault-kube-kms
and use file-backed secret injection for v.Credentials.Data["VAULT_SECRET_ID"]
and v.Credentials.Data["VAULT_ROLE_ID"] while keeping kmsConfig.Endpoint and
v.Config.* values as normal non-interpolated args.
---
Nitpick comments:
In `@pkg/operator/encryption/kms/helpers.go`:
- Around line 108-117: GetKMSProviderConfig builds providerConfigKey and
currently returns a generic error when json.Unmarshal(providerConfigData,
&kmsProviderConfig) fails; update the error to include the derived
providerConfigKey so you can identify which key's payload failed to unmarshal
(e.g. change the error returned from json.Unmarshal to include providerConfigKey
in the message and wrap the original err). Ensure the change references
GetKMSProviderConfig, providerConfigKey, json.Unmarshal and kmsProviderConfig so
the failure context is clear.
- Around line 28-93: Create a single shared constant (e.g., kmsPluginSocketDir)
for the socket directory and replace the hard-coded "/var/run/kmsplugin"
occurrences in AddKMSPluginVolumeAndMountToPodSpec (MountPath and HostPath.Path)
with that constant; also update kmsEndpointRegexp/ParseKeyIDFromEndpoint to
build or reference the regexp using the same constant so the socket path in the
regex stays in sync with the mount path (adjust regexp construction to use the
constant safely, e.g., via fmt.Sprintf and regexp.QuoteMeta).
In `@pkg/operator/encryption/kms/plugins/vault_test.go`:
- Around line 70-112: Add a table-driven test case to the existing tests loop
that passes kmsConfig as nil to VaultSidecarProvider.BuildSidecarContainer (use
the same VaultSidecarProvider, containerName, vaultConfig, and credentials
symbols) and assert the function returns an error (use require.Error or
require.ErrorContains with the expected message) instead of panicking; this
ensures the nil dereference path in BuildSidecarContainer is exercised and the
test verifies proper error handling when kmsConfig is nil.
🪄 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: de75d96b-cacf-4abe-a902-f2a06cd8c170
📒 Files selected for processing (6)
pkg/operator/encryption/kms/helpers.gopkg/operator/encryption/kms/helpers_test.gopkg/operator/encryption/kms/plugins/vault.gopkg/operator/encryption/kms/plugins/vault_test.gopkg/operator/encryption/kms/sidecar.gopkg/operator/encryption/kms/sidecar_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/operator/encryption/kms/sidecar_test.go
- pkg/operator/encryption/kms/sidecar.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/encryption/kms/helpers_test.go
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
1356633 to
1c8254f
Compare
p0lyn0mial
reviewed
Apr 27, 2026
| -vault-namespace=%s \ | ||
| -transit-mount=%s \ | ||
| -transit-key=%s \ | ||
| -log-level=debug-extended \ |
Contributor
There was a problem hiding this comment.
probably we don't want to run the vault plugin with verbose logs by default in production.
maybe this could be set via unsupportedConfigOverrides ?
/cc @ardaguclu
Member
There was a problem hiding this comment.
Agree. Log-level can be updated just as timeout value via unsupportedConfigOverrides (this mechanism does not exist today for KMS and we'll add it separately)
bertinatto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bertinatto: This pull request references CNTRLPLANE-3236 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. |
openshift-ci
Bot
added
the
needs-rebase
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
1c8254f to
a4b51d9
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto 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 |
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
a4b51d9 to
f6b6a40
Compare
bertinatto
commented
May 6, 2026
| return matches[1], nil | ||
| } | ||
|
|
||
| func GetKMSProviderConfig(secretData map[string][]byte, keyID string) (*configv1.KMSConfig, error) { |
Member
Author
There was a problem hiding this comment.
GetKMSProviderConfig -> GetKMSConfigForKey
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
3 times, most recently
from
5d1aca5 to
5e5e7db
Compare
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
5e5e7db to
bcd8f9d
Compare
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
bcd8f9d to
07038d3
Compare
Member
Author
|
/retest |
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
2 times, most recently
from
d0842d1 to
0859dde
Compare
ardaguclu
reviewed
May 8, 2026
Member
There was a problem hiding this comment.
In my opinion, this PR works nice to inject multiple kms plugins as sidecars by reading from kms-plugin-config-* data from encryption config Secret. We can remove all the credentials dependencies. (of course, meanwhile I'll work on passing credentials)
| SECRET_ID=${SECRET_ID%%%%\"*} | ||
| ROLE_ID=${CREDS#*\"VAULT_ROLE_ID\":\"} | ||
| ROLE_ID=${ROLE_ID%%%%\"*} | ||
| printf '%%s' "$SECRET_ID" > /tmp/secret-id |
Member
There was a problem hiding this comment.
Vault mock plugin does not need any credentials. I think we can update this PR to generate a side-car plugin container spec with the data coming from kms-provider-config. This is testable because we'll set vault kms plugin image from API directly.
| apiserverv1 "k8s.io/apiserver/pkg/apis/apiserver/v1" | ||
| ) | ||
|
|
||
| var kmsEndpointRegexp = regexp.MustCompile(`^unix:///var/run/kmsplugin/kms-(\d+)\.sock$`) |
Member
There was a problem hiding this comment.
Agree that we should move this
to here.
Member
Author
There was a problem hiding this comment.
oh, I didn't recall this existed before. Are you suggesting I remove the one in key_controller.go and keep this one?
Member
There was a problem hiding this comment.
Yes we can remove the one from key_controller. I think kms/helpers.go is better place.
Member
Author
There was a problem hiding this comment.
hm, so that one is a string fmt, and this one is a regex
| return nil | ||
| } | ||
|
|
||
| func findFirstKMSConfiguration(config *apiserverv1.EncryptionConfiguration) *apiserverv1.KMSConfiguration { |
Member
There was a problem hiding this comment.
I think we don't need to wire the first config anymore. kms-plugin-configs are coming from encryption-config Secret already. So it is just a matter of a loop over the keys.
Member
Author
There was a problem hiding this comment.
ack, I can get the key ids from the secret's keys. The plugin also needs the endpoint path, do you suggest I reconstruct it using the information from the kms-plugin-config? Or is it better to plumb that information thru?
Member
There was a problem hiding this comment.
encryption-config key contains uds path keyed by secret_KEYID, oauth_KEYID, etc. format.
| BuildSidecarContainer(name string, kmsConfiguration *apiserverv1.KMSConfiguration) (corev1.Container, error) | ||
| } | ||
|
|
||
| func newSidecarProvider(providerConfig *configv1.KMSConfig, secretDataPath string) (SidecarProvider, error) { |
Member
There was a problem hiding this comment.
Suggested change
| func newSidecarProvider(providerConfig *configv1.KMSConfig, secretDataPath string) (SidecarProvider, error) { | |
| func newSidecarProvider(providerConfig *configv1.KMSConfig) (SidecarProvider, error) { |
We can wire secret when it is needed
tjungblu
reviewed
May 8, 2026
Comment on lines
+31
to
+35
| CREDS=$(cat %s) | ||
| SECRET_ID=${CREDS#*\"VAULT_SECRET_ID\":\"} | ||
| SECRET_ID=${SECRET_ID%%%%\"*} | ||
| ROLE_ID=${CREDS#*\"VAULT_ROLE_ID\":\"} | ||
| ROLE_ID=${ROLE_ID%%%%\"*} |
Contributor
There was a problem hiding this comment.
@bertinatto your demo got me thinking:
- you can just directly write the args string from the kmsConfig, without the indirection of the env vars
- the secret_id mount is obviously difficult to get rid off...
for the secret_id we may create a dedicated secret text file that can be mounted directly? I don't know how this will end up in the static pod though - I assume we want to avoid going to the API with this directly due to the catch-22 of the encryption/decryption of secrets 😄
Member
Author
There was a problem hiding this comment.
yeah, that'd be great, but unfortunately we can't mount the secret because this is a static pod; it doesn't work indeed :(
Add utility functions for parsing KMS endpoints, provider configs, and secret data paths from the encryption-config Secret.
Add InjectIntoPodSpec which reads the encryption-config Secret, parses the KMS configuration, and injects a sidecar container with the necessary volume mounts into the API server pod spec.
Implement the VaultSidecarProvider which builds a sidecar container for the vault-kube-kms binary with AppRole authentication. Wire it into the newSidecarProvider factory function.
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
0859dde to
793d171
Compare
Contributor
|
@bertinatto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
proof: openshift/cluster-kube-apiserver-operator#2104
Summary by CodeRabbit
Release Notes
New Features
Tests