Draft
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:
WalkthroughAdds a new KMS sidecar injector and wiring: a new file providing functions to read a Changes
Sequence Diagram(s)sequenceDiagram
participant Main as cmd/main
participant Installer as Installer (with PodMutationFn)
participant Mutator as kms_sidecar.AddKMSPluginToPodSpecFn
participant Secrets as SecretLister (k8s API)
participant Pod as PodSpec
Main->>Installer: create installer with PodMutationFn (Mutator)
Installer->>Mutator: invoke PodMutationFn for PodSpec
Mutator->>Secrets: Get(secret: <targetNamespace>/vault-kms-credentials)
alt secret found
Secrets-->>Mutator: return secret
Mutator->>Pod: append kms-plugin sidecar, add hostPath volume & mounts
Mutator-->>Installer: return mutated PodSpec
else secret missing or error
Secrets-->>Mutator: not found / error
Mutator-->>Installer: return original PodSpec (no KMS)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bertinatto The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/operator/targetconfigcontroller/kms_sidecar.go (4)
112-117: Misleading variable name:foundVolumeMountshould befoundVolume.The variable
foundVolumeMountiterates overpodSpec.Volumes, notVolumeMounts. This is confusing and could lead to maintenance errors.✏️ Suggested fix
- foundVolumeMount := false - for _, volumeMount := range podSpec.Volumes { - if volumeMount.Name == "kms-plugin-socket" { - foundVolumeMount = true + foundVolume := false + for _, volume := range podSpec.Volumes { + if volume.Name == "kms-plugin-socket" { + foundVolume = true } } - if !foundVolumeMount { + if !foundVolume {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 112 - 117, Rename the misleading variable foundVolumeMount to foundVolume (or similar) in the kms sidecar logic that iterates podSpec.Volumes: update the declaration and all references to foundVolumeMount so the loop that checks for volume name "kms-plugin-socket" correctly reads and conveys that it's checking Volumes (not VolumeMounts), e.g., change foundVolumeMount := false and any subsequent uses to foundVolume.
20-32: Remove or enable commented-out feature gate logic.This commented-out block should either be removed (if not needed for TP v1) or enabled. Leaving large commented-out blocks makes the code harder to maintain and unclear about intent. If this is intentionally disabled for the tech preview, add a TODO comment explaining when it should be re-enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 20 - 32, The commented-out feature gate block around featureGateAccessor.AreInitialFeatureGatesObserved(), featureGateAccessor.CurrentFeatureGates(), and the check for features.FeatureGateKMSEncryption should be addressed: either delete the entire commented block if not needed for TP v1, or restore it (uncomment and handle errors from CurrentFeatureGates() and short-circuit when the gate is disabled, using klog.Infof as shown) so feature-gate behavior is enforced; if the code is intentionally disabled for tech preview, leave a single-line TODO above the block referencing the intent and when to re-enable (e.g., "TODO: re-enable KMS feature-gate checks for GA") and remove the rest of the commented lines.
112-117: Loop continues after finding the volume.The loop doesn't break after finding a match, which is inefficient.
✏️ Suggested fix
foundVolume := false for _, volume := range podSpec.Volumes { if volume.Name == "kms-plugin-socket" { foundVolume = true + break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 112 - 117, The loop scanning podSpec.Volumes to set foundVolumeMount should exit once the "kms-plugin-socket" volume is found to avoid unnecessary iterations; modify the loop in kms_sidecar.go to break immediately after setting foundVolumeMount = true when volumeMount.Name == "kms-plugin-socket" (search for the foundVolumeMount variable and the loop over podSpec.Volumes).
15-18: Unused parameter:featureGateAccessorThe
featureGateAccessorparameter is passed but never used since the feature gate check code is commented out (lines 20-32). Either remove the parameter or enable the feature gate logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 15 - 18, The AddKMSPluginToPodSpec function currently accepts an unused parameter featureGateAccessor; remove featureGateAccessor from the function signature and from all callers to avoid dead API surface, and update any tests or call sites that pass it. Specifically, edit the AddKMSPluginToPodSpec declaration to drop the featureGateAccessor parameter and remove it from every invocation, and if there was intended feature-gate behavior instead, restore the commented feature gate check inside AddKMSPluginToPodSpec (using featureGateAccessor.IsFeatureEnabled or the appropriate method) rather than keeping the parameter unused. Ensure imports and any affected compile-time references are adjusted accordingly.pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
312-312: Function signature is getting long.The
managePodsfunction now has 10 parameters. Consider grouping related parameters into a struct to improve readability and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go` at line 312, The managePods function signature is too long; refactor by creating a single struct (e.g., ManagePodsOptions or PodManagerConfig) that groups related parameters (client coreclientv1.ConfigMapsGetter, featureGateAccessor featuregates.FeatureGateAccess, isStartupMonitorEnabledFn func() (bool, error), recorder events.Recorder, operatorSpec *operatorv1.StaticPodOperatorSpec, imagePullSpec, operatorImagePullSpec, operatorImageVersion string, secretLister corev1listers.SecretLister) and replace the current parameter list with a single options parameter in the managePods declaration; update all callers of managePods to construct and pass the new struct, adjust any unit tests or mock call sites to use the struct, and keep the internal logic unchanged except for accessing fields from the new struct.
🤖 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/targetconfigcontroller/kms_sidecar.go`:
- Around line 66-81: Add explicit validation to ensure the secret contains
VAULT_SECRET_ID, VAULT_ADDR, VAULT_NAMESPACE, VAULT_KEY_NAME, and VAULT_ROLE_ID
before building the Args for the sidecar; implement a helper like
validateCredentials(creds *corev1.Secret) that checks each required key and
returns an error if any are missing, and call it from the code path that
constructs the fmt.Sprintf command (the block creating Args in kms_sidecar.go)
so you abort or surface a clear error instead of producing empty values in the
command.
- Around line 61-86: The container currently embeds VAULT_SECRET_ID into the
shell command and sets SecurityContext.Privileged=true; instead, stop echoing
secrets via the command and mount the existing Kubernetes secret
(vault-kms-credentials) as a secret volume into the pod; update the container
args for vault-kube-kms to reference a file path (e.g., /etc/vault/secret-id)
for -approle-secret-id-path and remove the echo/command wrapper that injects
string(creds.Data["VAULT_SECRET_ID"]); also remove Privileged: ptrBool(true)
from the SecurityContext and replace it with the minimal required security
settings/capabilities (or drop SecurityContext if none are needed) to avoid full
host privileges.
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Around line 333-337: Replace the hardcoded personal image string assigned to
kmsPluginImage with a configurable value: read the KMS plugin image from an
operator configuration or environment variable (e.g., KMS_PLUGIN_IMAGE) with a
sensible organizational default (not quay.io/bertinatto), ensure the value is
validated (non-empty and contains an explicit tag or digest) before calling
AddKMSPluginToPodSpec(&required.Spec, ...), and fail-fast with a clear error if
invalid; update the TODO comment to reflect the new configuration source and
ensure any references to operatorclient.TargetNamespace remain unchanged.
---
Nitpick comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 112-117: Rename the misleading variable foundVolumeMount to
foundVolume (or similar) in the kms sidecar logic that iterates podSpec.Volumes:
update the declaration and all references to foundVolumeMount so the loop that
checks for volume name "kms-plugin-socket" correctly reads and conveys that it's
checking Volumes (not VolumeMounts), e.g., change foundVolumeMount := false and
any subsequent uses to foundVolume.
- Around line 20-32: The commented-out feature gate block around
featureGateAccessor.AreInitialFeatureGatesObserved(),
featureGateAccessor.CurrentFeatureGates(), and the check for
features.FeatureGateKMSEncryption should be addressed: either delete the entire
commented block if not needed for TP v1, or restore it (uncomment and handle
errors from CurrentFeatureGates() and short-circuit when the gate is disabled,
using klog.Infof as shown) so feature-gate behavior is enforced; if the code is
intentionally disabled for tech preview, leave a single-line TODO above the
block referencing the intent and when to re-enable (e.g., "TODO: re-enable KMS
feature-gate checks for GA") and remove the rest of the commented lines.
- Around line 112-117: The loop scanning podSpec.Volumes to set foundVolumeMount
should exit once the "kms-plugin-socket" volume is found to avoid unnecessary
iterations; modify the loop in kms_sidecar.go to break immediately after setting
foundVolumeMount = true when volumeMount.Name == "kms-plugin-socket" (search for
the foundVolumeMount variable and the loop over podSpec.Volumes).
- Around line 15-18: The AddKMSPluginToPodSpec function currently accepts an
unused parameter featureGateAccessor; remove featureGateAccessor from the
function signature and from all callers to avoid dead API surface, and update
any tests or call sites that pass it. Specifically, edit the
AddKMSPluginToPodSpec declaration to drop the featureGateAccessor parameter and
remove it from every invocation, and if there was intended feature-gate behavior
instead, restore the commented feature gate check inside AddKMSPluginToPodSpec
(using featureGateAccessor.IsFeatureEnabled or the appropriate method) rather
than keeping the parameter unused. Ensure imports and any affected compile-time
references are adjusted accordingly.
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Line 312: The managePods function signature is too long; refactor by creating
a single struct (e.g., ManagePodsOptions or PodManagerConfig) that groups
related parameters (client coreclientv1.ConfigMapsGetter, featureGateAccessor
featuregates.FeatureGateAccess, isStartupMonitorEnabledFn func() (bool, error),
recorder events.Recorder, operatorSpec *operatorv1.StaticPodOperatorSpec,
imagePullSpec, operatorImagePullSpec, operatorImageVersion string, secretLister
corev1listers.SecretLister) and replace the current parameter list with a single
options parameter in the managePods declaration; update all callers of
managePods to construct and pass the new struct, adjust any unit tests or mock
call sites to use the struct, and keep the internal logic unchanged except for
accessing fields from the new struct.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
pkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
Comment on lines
+66
to
+81
| Args: []string{fmt.Sprintf(` | ||
| echo "%s" > /tmp/secret-id | ||
| exec /vault-kube-kms \ | ||
| -listen-address=unix:///var/run/kmsplugin/kms.sock \ | ||
| -vault-address=%s \ | ||
| -vault-namespace=%s \ | ||
| -transit-mount=transit \ | ||
| -transit-key=%s \ | ||
| -log-level=debug-extended \ | ||
| -approle-role-id=%s \ | ||
| -approle-secret-id-path=/tmp/secret-id`, | ||
| string(creds.Data["VAULT_SECRET_ID"]), | ||
| string(creds.Data["VAULT_ADDR"]), | ||
| string(creds.Data["VAULT_NAMESPACE"]), | ||
| string(creds.Data["VAULT_KEY_NAME"]), | ||
| string(creds.Data["VAULT_ROLE_ID"])), |
There was a problem hiding this comment.
Missing validation for required secret keys.
The code assumes all required keys (VAULT_SECRET_ID, VAULT_ADDR, VAULT_NAMESPACE, VAULT_KEY_NAME, VAULT_ROLE_ID) exist in the secret. If any key is missing, the resulting command will have empty values, leading to runtime failures that are hard to diagnose.
🛡️ Suggested validation
func validateCredentials(creds *corev1.Secret) error {
requiredKeys := []string{"VAULT_SECRET_ID", "VAULT_ADDR", "VAULT_NAMESPACE", "VAULT_KEY_NAME", "VAULT_ROLE_ID"}
for _, key := range requiredKeys {
if _, ok := creds.Data[key]; !ok {
return fmt.Errorf("missing required key %q in vault-kms-credentials secret", key)
}
}
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 66 - 81, Add
explicit validation to ensure the secret contains VAULT_SECRET_ID, VAULT_ADDR,
VAULT_NAMESPACE, VAULT_KEY_NAME, and VAULT_ROLE_ID before building the Args for
the sidecar; implement a helper like validateCredentials(creds *corev1.Secret)
that checks each required key and returns an error if any are missing, and call
it from the code path that constructs the fmt.Sprintf command (the block
creating Args in kms_sidecar.go) so you abort or surface a clear error instead
of producing empty values in the command.
Comment on lines
333
to
337
| // TODO: placeholder. I (fbertina) need to grant you read permissions. Please ask | ||
| kmsPluginImage := "quay.io/bertinatto/vault:v1" | ||
| if err := AddKMSPluginToPodSpec(&required.Spec, featureGateAccessor, secretLister, operatorclient.TargetNamespace, kmsPluginImage); err != nil { | ||
| return nil, false, fmt.Errorf("failed to add KMS plugin sidecar: %w", err) | ||
| } |
There was a problem hiding this comment.
Hardcoded personal container image should be addressed before merge.
The KMS plugin image is hardcoded to a personal Quay.io repository (quay.io/bertinatto/vault:v1). The TODO comment indicates this is intentional for WIP, but this needs to be resolved:
- The image should come from an official/organizational registry
- Consider making this configurable via environment variable or operator config
- The version tag
:v1is vague; use a more specific version or digest
Since this is a draft PR, flagging for tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go` around lines
333 - 337, Replace the hardcoded personal image string assigned to
kmsPluginImage with a configurable value: read the KMS plugin image from an
operator configuration or environment variable (e.g., KMS_PLUGIN_IMAGE) with a
sensible organizational default (not quay.io/bertinatto), ensure the value is
validated (non-empty and contains an explicit tag or digest) before calling
AddKMSPluginToPodSpec(&required.Spec, ...), and fail-fast with a clear error if
invalid; update the TODO comment to reflect the new configuration source and
ensure any references to operatorclient.TargetNamespace remain unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
312-312: Unused parameter acceptable for WIP, but ensure cleanup before merge.The
secretListerparameter is added tomanagePodsbut is currently unused since the KMS plugin integration code is commented out. This is fine for a draft PR, but before merging:
- Either uncomment and finalize the KMS integration code
- Or remove the unused
secretListerparameter to avoid dead codeThe placeholder comment addresses the previously flagged hardcoded image concern.
Also applies to: 333-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go` at line 312, The secretLister parameter in managePods is currently unused; either remove it from the managePods signature and from all call sites (and drop any now-unused imports) or re-enable the KMS/plugin code that consumes secretLister so it is actually used—update the managePods function (and matching callers) to match the chosen approach and ensure any referenced symbols (secretLister, managePods) are consistently changed across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cluster-kube-apiserver-operator/main.go`:
- Around line 52-53: Remove the dead helper ptrBool function from this file: it
is unused and duplicates the real ptrBool in kms_sidecar.go; leave the installer
wiring as-is (the call to
installerpod.NewInstallOptions().WithPodMutationFactory(targetconfigcontroller.AddKMSPluginToPodSpecFn)
and cmd.AddCommand(installerpod.NewInstallerWithInstallOptions(ctx, opts))
should remain unchanged), so delete the ptrBool declaration (the function named
ptrBool) and run a quick build to confirm no references remain.
- Around line 72-75: Remove the unused helper function ptrBool from the package
main entry point: delete the function definition func ptrBool(b bool) *bool {
return &b } (it is not referenced anywhere in main.go and package main files
should not keep orphan helpers); ensure no other code relies on ptrBool before
removing it.
---
Nitpick comments:
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Line 312: The secretLister parameter in managePods is currently unused; either
remove it from the managePods signature and from all call sites (and drop any
now-unused imports) or re-enable the KMS/plugin code that consumes secretLister
so it is actually used—update the managePods function (and matching callers) to
match the chosen approach and ensure any referenced symbols (secretLister,
managePods) are consistently changed across the codebase.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/library-go/pkg/operator/staticpod/installerpod/cmd.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
cmd/cluster-kube-apiserver-operator/main.gopkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/targetconfigcontroller/kms_sidecar.go
Comment on lines
+52
to
+53
| opts := installerpod.NewInstallOptions().WithPodMutationFactory(targetconfigcontroller.AddKMSPluginToPodSpecFn) | ||
| cmd.AddCommand(installerpod.NewInstallerWithInstallOptions(ctx, opts)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for AddKMSPluginToPodSpecFn definition and usage
rg -n "AddKMSPluginToPodSpecFn" --type go -A 5Repository: openshift/cluster-kube-apiserver-operator
Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Check the main.go file to see the full context including ptrBool
cat -n cmd/cluster-kube-apiserver-operator/main.go | head -80Repository: openshift/cluster-kube-apiserver-operator
Length of output: 3423
🏁 Script executed:
#!/bin/bash
# Search for ptrBool usage across the codebase
rg -n "ptrBool" --type goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 414
Remove unused ptrBool function and fix installer wiring.
The installer command wiring at lines 52-53 is correct—AddKMSPluginToPodSpecFn is properly defined in the targetconfigcontroller package with the correct signature and integrates the KMS sidecar injection pathway as expected.
However, the ptrBool function at lines 73-75 is dead code and should be removed. It is not called anywhere in this file. (Note: a separate ptrBool function exists in kms_sidecar.go and is used there, but the one in main.go serves no purpose.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cluster-kube-apiserver-operator/main.go` around lines 52 - 53, Remove the
dead helper ptrBool function from this file: it is unused and duplicates the
real ptrBool in kms_sidecar.go; leave the installer wiring as-is (the call to
installerpod.NewInstallOptions().WithPodMutationFactory(targetconfigcontroller.AddKMSPluginToPodSpecFn)
and cmd.AddCommand(installerpod.NewInstallerWithInstallOptions(ctx, opts))
should remain unchanged), so delete the ptrBool declaration (the function named
ptrBool) and run a quick build to confirm no references remain.
Comment on lines
+72
to
+75
|
|
||
| func ptrBool(b bool) *bool { | ||
| return &b | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "ptrBool" --type goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 414
🏁 Script executed:
head -20 cmd/cluster-kube-apiserver-operator/main.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 833
Remove unused ptrBool function.
This helper function is defined but never called in this file. Since main.go is a package main entry point, it cannot be imported elsewhere. Remove the unused function at lines 73-75.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/cluster-kube-apiserver-operator/main.go` around lines 72 - 75, Remove the
unused helper function ptrBool from the package main entry point: delete the
function definition func ptrBool(b bool) *bool { return &b } (it is not
referenced anywhere in main.go and package main files should not keep orphan
helpers); ensure no other code relies on ptrBool before removing it.
bertinatto
force-pushed
the
kms-plugin-pod-mutation
branch
from
ec1c752 to
96cd753
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/operator/targetconfigcontroller/kms_sidecar.go (2)
69-84:⚠️ Potential issue | 🟡 MinorValidate required Secret keys before building sidecar args.
If any required key is missing, this generates empty runtime flags and fails late in a hard-to-debug way.
🛡️ Proposed validation helper
+func validateKMSCredentials(creds *corev1.Secret) error { + required := []string{ + "VAULT_SECRET_ID", + "VAULT_ADDR", + "VAULT_NAMESPACE", + "VAULT_KEY_NAME", + "VAULT_ROLE_ID", + } + for _, key := range required { + if _, ok := creds.Data[key]; !ok { + return fmt.Errorf("missing required key %q in vault-kms-credentials secret", key) + } + } + return nil +} + func addKMSPluginSidecarToPodSpec(podSpec *corev1.PodSpec, containerName string, image string, creds *corev1.Secret) error { if podSpec == nil { return fmt.Errorf("pod spec cannot be nil") } + if err := validateKMSCredentials(creds); err != nil { + return err + } ... }Also applies to: 159-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 69 - 84, Before assembling the sidecar Args string, validate that creds.Data contains all required keys (VAULT_SECRET_ID, VAULT_ADDR, VAULT_NAMESPACE, VAULT_KEY_NAME, VAULT_ROLE_ID); if any are missing return an error instead of building empty flags. Add a small helper (e.g., validateSecretKeys or ensureVaultSecretKeys) called from the code path that builds Args (the block using creds.Data[...] and setting Args) to check presence and non-empty values for those keys and surface a clear error; apply the same validation to the other Args construction block referenced (lines ~159-173) so both sidecar builder sites fail fast with a descriptive error rather than producing empty runtime flags.
69-88:⚠️ Potential issue | 🟠 MajorAvoid shell-injected secrets and privileged sidecars.
Both sidecar definitions embed sensitive values into a shell command and run with
Privileged: true. That increases secret exposure risk and host compromise blast radius.🔒 Proposed hardening
- Command: []string{"/bin/sh", "-c"}, - Args: []string{fmt.Sprintf(` - echo "%s" > /tmp/secret-id - exec /vault-kube-kms \ - -listen-address=unix:///var/run/kmsplugin/kms.sock \ - -vault-address=%s \ - -vault-namespace=%s \ - -transit-mount=transit \ - -transit-key=%s \ - -log-level=debug-extended \ - -approle-role-id=%s \ - -approle-secret-id-path=/tmp/secret-id`, - string(creds.Data["VAULT_SECRET_ID"]), - string(creds.Data["VAULT_ADDR"]), - string(creds.Data["VAULT_NAMESPACE"]), - string(creds.Data["VAULT_KEY_NAME"]), - string(creds.Data["VAULT_ROLE_ID"])), - }, - SecurityContext: &corev1.SecurityContext{ - Privileged: ptrBool(true), - }, + Command: []string{"/vault-kube-kms"}, + Args: []string{ + "-listen-address=unix:///var/run/kmsplugin/kms.sock", + fmt.Sprintf("-vault-address=%s", string(creds.Data["VAULT_ADDR"])), + fmt.Sprintf("-vault-namespace=%s", string(creds.Data["VAULT_NAMESPACE"])), + "-transit-mount=transit", + fmt.Sprintf("-transit-key=%s", string(creds.Data["VAULT_KEY_NAME"])), + "-log-level=debug-extended", + fmt.Sprintf("-approle-role-id=%s", string(creds.Data["VAULT_ROLE_ID"])), + "-approle-secret-id-path=/etc/vault/VAULT_SECRET_ID", + }, + VolumeMounts: []corev1.VolumeMount{ + { + Name: "vault-kms-credentials", + MountPath: "/etc/vault", + ReadOnly: true, + }, + },Also applies to: 159-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 69 - 88, The sidecar currently injects secrets into a shell heredoc via Args (the fmt.Sprintf block referencing creds.Data["VAULT_SECRET_ID"], "VAULT_ADDR", "VAULT_NAMESPACE", "VAULT_KEY_NAME", "VAULT_ROLE_ID"]) and sets SecurityContext.Privileged via ptrBool(true); change this to avoid shell-injected secrets and privilege: pass static executable and args as an argv slice (no embedded heredoc), deliver secret material via a mounted Secret volume or an env var/file path (keep approle-secret-id-path pointing to a mounted file like /etc/creds/secret-id rather than inlining creds.Data into the command), and tighten SecurityContext by removing Privileged=true, setting runAsNonRoot, allowPrivilegeEscalation=false, and dropping all Linux capabilities (or apply the minimal required capabilities only). Update the Args construction and the SecurityContext block (and any code referencing approle-secret-id-path and VAULT_SECRET_ID) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 144-180: Add the missing shared socket wiring and avoid hardcoding
the image: in AddKMSPluginToPodSpecFn create a pod.Spec.Volumes entry (e.g.,
name "kms-socket") using an EmptyDir, add a VolumeMount to the kms-plugin
container mounting that volume at /var/run/kmsplugin (so the -listen-address
unix path is actually backed by a shared FS), and locate the kube-apiserver
container in pod.Spec.Containers (match by Name "kube-apiserver") and add the
same VolumeMount to it so both containers share the socket; also stop hardcoding
the image string and use a configurable field from InstallOptions (e.g.,
o.KMSPluginImage) with a sensible fallback. Ensure all referenced symbols
(AddKMSPluginToPodSpecFn, kms-plugin container Name, kube-apiserver container
Name, and the volume name "kms-socket") are updated consistently.
- Around line 37-40: The code calling
secretLister.Secrets(targetNamespace).Get("vault-kms-credentials") treats any
error as “KMS disabled”; change the logic to only swallow NotFound errors and
treat other errors as hard failures: use the k8s API error helper
(apierrors.IsNotFound(err)) to detect missing secret and keep the current
log/return-nil path for that case, but for all other errors log an error
(klog.Errorf) with the error details and return the error instead of nil so
transient API/cache failures surface and stop startup; update the block around
creds, err to reference secretLister, targetNamespace and the
"vault-kms-credentials" call accordingly.
---
Duplicate comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 69-84: Before assembling the sidecar Args string, validate that
creds.Data contains all required keys (VAULT_SECRET_ID, VAULT_ADDR,
VAULT_NAMESPACE, VAULT_KEY_NAME, VAULT_ROLE_ID); if any are missing return an
error instead of building empty flags. Add a small helper (e.g.,
validateSecretKeys or ensureVaultSecretKeys) called from the code path that
builds Args (the block using creds.Data[...] and setting Args) to check presence
and non-empty values for those keys and surface a clear error; apply the same
validation to the other Args construction block referenced (lines ~159-173) so
both sidecar builder sites fail fast with a descriptive error rather than
producing empty runtime flags.
- Around line 69-88: The sidecar currently injects secrets into a shell heredoc
via Args (the fmt.Sprintf block referencing creds.Data["VAULT_SECRET_ID"],
"VAULT_ADDR", "VAULT_NAMESPACE", "VAULT_KEY_NAME", "VAULT_ROLE_ID"]) and sets
SecurityContext.Privileged via ptrBool(true); change this to avoid
shell-injected secrets and privilege: pass static executable and args as an argv
slice (no embedded heredoc), deliver secret material via a mounted Secret volume
or an env var/file path (keep approle-secret-id-path pointing to a mounted file
like /etc/creds/secret-id rather than inlining creds.Data into the command), and
tighten SecurityContext by removing Privileged=true, setting runAsNonRoot,
allowPrivilegeEscalation=false, and dropping all Linux capabilities (or apply
the minimal required capabilities only). Update the Args construction and the
SecurityContext block (and any code referencing approle-secret-id-path and
VAULT_SECRET_ID) accordingly.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/operator/targetconfigcontroller/kms_sidecar.go
Comment on lines
+37
to
+40
| creds, err := secretLister.Secrets(targetNamespace).Get("vault-kms-credentials") | ||
| if err != nil { | ||
| klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err) | ||
| return nil |
There was a problem hiding this comment.
Handle non-NotFound secret retrieval errors explicitly.
This currently treats every secret retrieval error as “KMS disabled,” which can silently bypass KMS on transient API/cache failures. Only “not found” should be treated as disabled; other errors should fail fast.
🔧 Proposed fix
+import apierrors "k8s.io/apimachinery/pkg/api/errors"
...
creds, err := secretLister.Secrets(targetNamespace).Get("vault-kms-credentials")
if err != nil {
- klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err)
- return nil
+ if apierrors.IsNotFound(err) {
+ klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err)
+ return nil
+ }
+ return fmt.Errorf("failed to get vault-kms-credentials secret: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 37 - 40, The
code calling secretLister.Secrets(targetNamespace).Get("vault-kms-credentials")
treats any error as “KMS disabled”; change the logic to only swallow NotFound
errors and treat other errors as hard failures: use the k8s API error helper
(apierrors.IsNotFound(err)) to detect missing secret and keep the current
log/return-nil path for that case, but for all other errors log an error
(klog.Errorf) with the error details and return the error instead of nil so
transient API/cache failures surface and stop startup; update the block around
creds, err to reference secretLister, targetNamespace and the
"vault-kms-credentials" call accordingly.
Comment on lines
+144
to
+180
| func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions) installerpod.PodMutationFunc { | ||
| return func(pod *corev1.Pod) error { | ||
| creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{}) | ||
| if err != nil { | ||
| klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err) | ||
| return nil | ||
| } | ||
| klog.Infof("kms is ENABLED") | ||
|
|
||
| pod.Spec.Containers = append(pod.Spec.Containers, corev1.Container{ | ||
| Name: "kms-plugin", | ||
| Image: "quay.io/bertinatto/vault:v1", | ||
| ImagePullPolicy: corev1.PullAlways, | ||
| Command: []string{"/bin/sh", "-c"}, | ||
| Args: []string{fmt.Sprintf(` | ||
| echo "%s" > /tmp/secret-id | ||
| exec /vault-kube-kms \ | ||
| -listen-address=unix:///var/run/kmsplugin/kms.sock \ | ||
| -vault-address=%s \ | ||
| -vault-namespace=%s \ | ||
| -transit-mount=transit \ | ||
| -transit-key=%s \ | ||
| -log-level=debug-extended \ | ||
| -approle-role-id=%s \ | ||
| -approle-secret-id-path=/tmp/secret-id`, | ||
| string(creds.Data["VAULT_SECRET_ID"]), | ||
| string(creds.Data["VAULT_ADDR"]), | ||
| string(creds.Data["VAULT_NAMESPACE"]), | ||
| string(creds.Data["VAULT_KEY_NAME"]), | ||
| string(creds.Data["VAULT_ROLE_ID"])), | ||
| }, | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| Privileged: ptrBool(true), | ||
| }, | ||
| }) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Pod mutation path is incomplete and can produce a non-functional KMS setup.
AddKMSPluginToPodSpecFn appends only the sidecar and hardcodes the image, but it never wires shared socket mounts for both kms-plugin and kube-apiserver. This diverges from the main helper path and breaks socket sharing.
♻️ Proposed consolidation/fix
-func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions) installerpod.PodMutationFunc {
+func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions, kmsPluginImage string) installerpod.PodMutationFunc {
return func(pod *corev1.Pod) error {
+ if pod == nil {
+ return fmt.Errorf("pod cannot be nil")
+ }
creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{})
if err != nil {
klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err)
return nil
}
- klog.Infof("kms is ENABLED")
-
- pod.Spec.Containers = append(pod.Spec.Containers, corev1.Container{
- Name: "kms-plugin",
- Image: "quay.io/bertinatto/vault:v1",
- ImagePullPolicy: corev1.PullAlways,
- Command: []string{"/bin/sh", "-c"},
- Args: []string{...},
- SecurityContext: &corev1.SecurityContext{
- Privileged: ptrBool(true),
- },
- })
+ if err := addKMSPluginSidecarToPodSpec(&pod.Spec, "kms-plugin", kmsPluginImage, creds); err != nil {
+ return err
+ }
+ if err := addKMSPluginVolumeAndMountToPodSpec(&pod.Spec, "kms-plugin"); err != nil {
+ return err
+ }
+ if err := addKMSPluginVolumeAndMountToPodSpec(&pod.Spec, "kube-apiserver"); err != nil {
+ return err
+ }
return nil
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 144 - 180,
Add the missing shared socket wiring and avoid hardcoding the image: in
AddKMSPluginToPodSpecFn create a pod.Spec.Volumes entry (e.g., name
"kms-socket") using an EmptyDir, add a VolumeMount to the kms-plugin container
mounting that volume at /var/run/kmsplugin (so the -listen-address unix path is
actually backed by a shared FS), and locate the kube-apiserver container in
pod.Spec.Containers (match by Name "kube-apiserver") and add the same
VolumeMount to it so both containers share the socket; also stop hardcoding the
image string and use a configurable field from InstallOptions (e.g.,
o.KMSPluginImage) with a sensible fallback. Ensure all referenced symbols
(AddKMSPluginToPodSpecFn, kms-plugin container Name, kube-apiserver container
Name, and the volume name "kms-socket") are updated consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/operator/targetconfigcontroller/kms_sidecar.go (2)
149-153:⚠️ Potential issue | 🟠 MajorHandle non-NotFound secret retrieval errors explicitly.
Same issue as in
AddKMSPluginToPodSpec: this treats every secret retrieval error as "KMS disabled," which silently bypasses KMS on transient API failures. Consider usingapierrors.IsNotFound(err)to distinguish missing secrets from other errors.🔧 Proposed fix
+import apierrors "k8s.io/apimachinery/pkg/api/errors" ... creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{}) if err != nil { - klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err) - return nil + if apierrors.IsNotFound(err) { + klog.Infof("kms is disabled: vault-kms-credentials secret not found") + return nil + } + return fmt.Errorf("failed to get vault-kms-credentials secret: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 149 - 153, The secret retrieval in the call to o.KubeClient.CoreV1().Secrets(...).Get currently treats any error as "KMS disabled"; change the error handling in the function containing this Get call so that you call apierrors.IsNotFound(err) (from k8s.io/apimachinery/pkg/api/errors) and only treat NotFound as "KMS disabled" (log and return nil), while for any other error you return or propagate the actual error (and log it as a transient API error) so transient failures are not silently ignored; update the klog message to reflect the different cases.
147-147:⚠️ Potential issue | 🟡 MinorAdd nil check for pod parameter.
The mutation function should validate that the
podparameter is not nil before accessingpod.Specto prevent potential nil pointer dereference.🛡️ Proposed fix
return func(pod *corev1.Pod) error { + if pod == nil { + return fmt.Errorf("pod cannot be nil") + } creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` at line 147, The returned mutation function in kms_sidecar.go should guard against a nil pod parameter before accessing pod.Spec; update the anonymous func(pod *corev1.Pod) error to begin with a nil check (e.g., if pod == nil { return fmt.Errorf("pod is nil") } or an appropriate error) so that subsequent uses of pod.Spec and other fields are safe and avoid a nil pointer dereference.
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/kms_sidecar.go (1)
146-146: Hardcoded image should be configurable.The image
"quay.io/bertinatto/vault:v1"is hardcoded. This should be passed as a parameter or read from configuration to allow for environment-specific images and version updates without code changes.♻️ Suggested approach
-func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions) installerpod.PodMutationFunc { - kmsPluginImage := "quay.io/bertinatto/vault:v1" +func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions, kmsPluginImage string) installerpod.PodMutationFunc { + if kmsPluginImage == "" { + kmsPluginImage = "quay.io/bertinatto/vault:v1" // default fallback + } return func(pod *corev1.Pod) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` at line 146, The hardcoded image assignment to kmsPluginImage ("quay.io/bertinatto/vault:v1") should be made configurable; update the code where kmsPluginImage is set (in kms_sidecar.go) to read the image value from configuration or a function parameter (e.g., an injected Config struct, environment variable, or controller options) and fall back to a sensible default if not provided, and propagate that configurable value into the code paths that use kmsPluginImage (e.g., the sidecar creation function or struct) so deployments can override image and tag without modifying code.
🤖 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/targetconfigcontroller/kms_sidecar.go`:
- Around line 145-148: Remove the temporary developer debug logs in
AddKMSPluginToPodSpecFn: eliminate the two klog.Infof calls that start with
"fjb:" (the ones logged just before kmPluginImage assignment and inside the
returned func(pod *corev1.Pod) error) or replace them with appropriate
structured/production logging (e.g., use klog.V(...) or include contextual
fields) so no developer-initialized debug text remains in the final code; ensure
any retained log references the function name and pod metadata rather than the
"fjb:" tag.
---
Duplicate comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 149-153: The secret retrieval in the call to
o.KubeClient.CoreV1().Secrets(...).Get currently treats any error as "KMS
disabled"; change the error handling in the function containing this Get call so
that you call apierrors.IsNotFound(err) (from
k8s.io/apimachinery/pkg/api/errors) and only treat NotFound as "KMS disabled"
(log and return nil), while for any other error you return or propagate the
actual error (and log it as a transient API error) so transient failures are not
silently ignored; update the klog message to reflect the different cases.
- Line 147: The returned mutation function in kms_sidecar.go should guard
against a nil pod parameter before accessing pod.Spec; update the anonymous
func(pod *corev1.Pod) error to begin with a nil check (e.g., if pod == nil {
return fmt.Errorf("pod is nil") } or an appropriate error) so that subsequent
uses of pod.Spec and other fields are safe and avoid a nil pointer dereference.
---
Nitpick comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Line 146: The hardcoded image assignment to kmsPluginImage
("quay.io/bertinatto/vault:v1") should be made configurable; update the code
where kmsPluginImage is set (in kms_sidecar.go) to read the image value from
configuration or a function parameter (e.g., an injected Config struct,
environment variable, or controller options) and fall back to a sensible default
if not provided, and propagate that configurable value into the code paths that
use kmsPluginImage (e.g., the sidecar creation function or struct) so
deployments can override image and tag without modifying code.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/library-go/pkg/operator/staticpod/installerpod/cmd.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (1)
pkg/operator/targetconfigcontroller/kms_sidecar.go
Comment on lines
+145
to
+148
| klog.Infof("fjb: in AddKMSPluginToPodSpecFn") | ||
| kmsPluginImage := "quay.io/bertinatto/vault:v1" | ||
| return func(pod *corev1.Pod) error { | ||
| klog.Infof("fjb: running func in AddKMSPluginToPodSpecFn") |
There was a problem hiding this comment.
Remove debug log statements before merge.
Lines 145 and 148 contain debug log statements with developer initials ("fjb:"). These should be removed or converted to proper structured logging before merging.
🧹 Proposed fix
func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions) installerpod.PodMutationFunc {
- klog.Infof("fjb: in AddKMSPluginToPodSpecFn")
kmsPluginImage := "quay.io/bertinatto/vault:v1"
return func(pod *corev1.Pod) error {
- klog.Infof("fjb: running func in AddKMSPluginToPodSpecFn")
creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{})📝 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
| klog.Infof("fjb: in AddKMSPluginToPodSpecFn") | |
| kmsPluginImage := "quay.io/bertinatto/vault:v1" | |
| return func(pod *corev1.Pod) error { | |
| klog.Infof("fjb: running func in AddKMSPluginToPodSpecFn") | |
| kmsPluginImage := "quay.io/bertinatto/vault:v1" | |
| return func(pod *corev1.Pod) error { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go` around lines 145 - 148,
Remove the temporary developer debug logs in AddKMSPluginToPodSpecFn: eliminate
the two klog.Infof calls that start with "fjb:" (the ones logged just before
kmPluginImage assignment and inside the returned func(pod *corev1.Pod) error) or
replace them with appropriate structured/production logging (e.g., use
klog.V(...) or include contextual fields) so no developer-initialized debug text
remains in the final code; ensure any retained log references the function name
and pod metadata rather than the "fjb:" tag.
Contributor
|
@bertinatto: 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. |
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
1 participant
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.
Summary by CodeRabbit
New Features
Refactor