DO NOT MERGE: test kms plugin lifecycle#2104
Open
bertinatto wants to merge 7 commits into
Open
Conversation
|
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:
WalkthroughThe pull request introduces KMS encryption plugin sidecar injection support for pod specifications. It adds a new module implementing KMS plugin attachment with Vault provider configuration, comprehensive test coverage, updates the target config controller to use the new logic, and replaces a library-go dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AddKMSPlugin as AddKMSPluginToPodSpec
participant SecretLister
participant ConfigParser as Config Parser
participant PodMutator as Pod Spec Mutator
Client->>AddKMSPlugin: AddKMSPluginToPodSpec(podSpec, featureGate, secretLister, image)
AddKMSPlugin->>AddKMSPlugin: Check KMSEncryption feature gate enabled
alt Feature gate disabled
AddKMSPlugin-->>Client: Return (no-op)
end
AddKMSPlugin->>SecretLister: Fetch encryption-config secret
alt Secret not found
AddKMSPlugin-->>Client: Return (no-op)
end
AddKMSPlugin->>ConfigParser: Decode EncryptionConfiguration
AddKMSPlugin->>ConfigParser: Extract KMS provider & keyID
AddKMSPlugin->>SecretLister: Read KMS provider config secret
AddKMSPlugin->>SecretLister: Read KMS credentials secret
AddKMSPlugin->>ConfigParser: Parse Vault configuration
alt Invalid Vault config
AddKMSPlugin-->>Client: Return error
end
AddKMSPlugin->>PodMutator: Append kms-plugin container with Vault args
AddKMSPlugin->>PodMutator: Append volume mount to kms-plugin & kube-apiserver
AddKMSPlugin->>PodMutator: Deduplicate/create kms-plugin-socket hostPath volume
AddKMSPlugin-->>Client: Return success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
pkg/operator/targetconfigcontroller/kms_sidecar_test.go (1)
21-78: Test coverage is limited to happy path only.Consider adding test cases for edge cases and error conditions:
- Feature gate disabled
- Encryption config secret not found
- Missing
encryption-configkey in secret- Missing
kms-provider-config-*keys- Invalid JSON in provider config
- Nil pod spec
This would improve confidence in the error handling paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/targetconfigcontroller/kms_sidecar_test.go` around lines 21 - 78, Add unit tests to TestAddKMSPluginToPodSpec covering negative and edge cases for AddKMSPluginToPodSpec: add subtests for feature gate disabled (use featuregates.NewHardcodedFeatureGateAccess without features), encryption config secret not found (no secrets added to fake.NewClientset), secret present but missing "encryption-config" key, secrets missing "kms-provider-config-*" keys, invalid JSON in provider config secret, and nil podSpec; each subtest should assert the expected error string or behavior using the existing secretLister helper and newEncryptionConfigSecret helper (or variants) to construct secrets and reuse the kmsPluginImage fixture.pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
333-340: Hardcoded image and commented-out code should be addressed before merge.The hardcoded
kmsPluginImageand the commented-out code (encryptionConfig, err := secretLister.Secrets(namespace string)) indicate incomplete implementation. Since this is marked "DO NOT MERGE" for testing, this is expected, but should be tracked.Recommendations
- The
kmsPluginImageshould be sourced from environment variables or operator configuration, similar toIMAGEandOPERATOR_IMAGE- Remove or complete the commented-out code on line 336
🤖 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 - 340, Replace the hardcoded kmsPluginImage and the commented-out encryptionConfig line: read the KMS plugin image from the operator config or environment (reuse the same pattern used for IMAGE/OPERATOR_IMAGE) and pass that value into AddKMSPluginToPodSpec instead of the literal "quay.io/bertinatto/vault:v1"; also remove or implement the commented-out `encryptionConfig, err := secretLister.Secrets(namespace string)` by either calling the correct secretLister method to retrieve encryptionConfig (and handling the error) or deleting the placeholder if not needed; keep references to kmsPluginImage, AddKMSPluginToPodSpec, and secretLister to locate where to apply the change.pkg/operator/kms/precondition.go (1)
98-102: Silent error handling may mask configuration issues.Decoding failures in
hasKMSEncryptionProvidersilently returnfalse, which could cause the precondition to incorrectly evaluate if the encryption configuration is malformed. Consider logging the error for debuggability.🔧 Proposed fix
obj, _, err := codecs.UniversalDeserializer().Decode(encryptionConfigBytes, &gvk, nil) if err != nil { + klog.Warningf("failed to decode encryption config: %v", err) return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/kms/precondition.go` around lines 98 - 102, In hasKMSEncryptionProvider, don't silently swallow decode errors: when codecs.UniversalDeserializer().Decode(encryptionConfigBytes, &gvk, nil) returns an error, log the error with context (e.g., include "failed to decode EncryptionConfiguration" and the error value) before returning false so malformed encryptionConfigBytes are visible in logs; update the error path that currently returns false to emit a clear log entry referencing gvk and encryptionConfigBytes (or a short identifier) to aid debugging.pkg/operator/targetconfigcontroller/kms_sidecar.go (3)
213-218: Volume mount is always appended without checking for duplicates.While the volume addition checks for existing volumes (line 220-225), the volume mount is unconditionally appended to the container. If this function were called multiple times for the same container, it would create duplicate mounts. Consider adding deduplication for consistency.
🔧 Add mount deduplication
container := &podSpec.Containers[containerIndex] + for _, vm := range container.VolumeMounts { + if vm.Name == "kms-plugin-socket" { + // Mount already exists + goto checkVolume + } + } container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ Name: "kms-plugin-socket", MountPath: "/var/run/kmsplugin", }, ) +checkVolume:🤖 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 213 - 218, The code appends a corev1.VolumeMount with Name "kms-plugin-socket" to container.VolumeMounts unconditionally, causing duplicate mounts if called repeatedly; update the logic around container.VolumeMounts (the append of corev1.VolumeMount{Name: "kms-plugin-socket", MountPath: "/var/run/kmsplugin"}) to first scan existing container.VolumeMounts for a mount with the same Name (or MountPath) and only append when not present, mirroring the existing deduplication used for volumes so the function (in kms_sidecar.go where container.VolumeMounts is modified) remains idempotent.
165-166:ImagePullPolicy: PullAlwaysmay cause issues in disconnected environments.Using
PullAlwayscan prevent pod startup in air-gapped or disconnected environments where the registry is not reachable. Consider usingIfNotPresentor making this configurable.🤖 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 165 - 166, The container spec in kms_sidecar.go currently hardcodes ImagePullPolicy: corev1.PullAlways which breaks air-gapped environments; change this to corev1.PullIfNotPresent or make it configurable (e.g., via a KMS sidecar config field or operator flag) and use that value when constructing the Container (the location where Image and ImagePullPolicy are set, referencing the Image variable and ImagePullPolicy property); ensure default is IfNotPresent and validate/propagate the configured policy into the container spec.
167-183: Shell command construction via string formatting is fragile.Embedding config values directly into a shell command string via
fmt.Sprintfis error-prone. If any config value contains shell metacharacters (spaces, quotes, etc.), the command could break or behave unexpectedly. Consider usingCommandwith explicit args instead ofsh -c.🔧 Safer approach using explicit args
podSpec.Containers = append(podSpec.Containers, corev1.Container{ Name: containerName, Image: image, ImagePullPolicy: corev1.PullAlways, - Command: []string{"/bin/sh", "-c"}, - Args: []string{fmt.Sprintf(` - exec /vault-kube-kms \ - -listen-address=unix:///var/run/kmsplugin/kms.sock \ - ... + Command: []string{"/vault-kube-kms"}, + Args: []string{ + "-listen-address=unix:///var/run/kmsplugin/kms.sock", + fmt.Sprintf("-vault-address=%s", config.Addr), + fmt.Sprintf("-vault-namespace=%s", config.Namespace), + "-transit-mount=transit", + fmt.Sprintf("-transit-key=%s", config.KeyName), + "-log-level=debug-extended", + fmt.Sprintf("-approle-role-id=%s", config.RoleID), + fmt.Sprintf("-approle-secret-id-path=/etc/kubernetes/static-pod-resources/%s", keySecretID), + },🤖 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 167 - 183, The current construction of the sidecar process line uses fmt.Sprintf to embed config.Addr, config.Namespace, config.KeyName, config.RoleID and keySecretID into a single shell string passed to Command with "/bin/sh -c", which is fragile and unsafe; change it to invoke the binary directly with explicit arguments (i.e., set Command to []string{"/vault-kube-kms"} and Args to the list of flag strings: "-listen-address=unix:///var/run/kmsplugin/kms.sock", "-vault-address="+config.Addr, "-vault-namespace="+config.Namespace, "-transit-mount=transit", "-transit-key="+config.KeyName, "-log-level=debug-extended", "-approle-role-id="+config.RoleID, "-approle-secret-id-path=/etc/kubernetes/static-pod-resources/"+keySecretID) so no shell parsing is required and shell metacharacters in config values cannot break the invocation.
🤖 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/kms/precondition.go`:
- Around line 66-67: The precondition check uses the wrong container name
string: update the call to podHasKMSSidecar in precondition.go (where kmsInPod
is set) to look for "kms-plugin" (matching the sidecar added by the operator)
instead of "km-plugin", so podHasKMSSidecar(podCM, "kms-plugin") will correctly
detect the sidecar and allow the precondition to pass; keep the FIXME comment or
remove it after fixing.
- Around line 76-85: The function podHasKMSSidecar currently uses json.Unmarshal
to parse pod.yaml which fails for non-JSON YAML content; update podHasKMSSidecar
to parse the ConfigMap's "pod.yaml" using a YAML-aware deserializer (e.g.,
sigs.k8s.io/yaml.Unmarshal) or the Kubernetes universal deserializer to decode
into corev1.Pod, handle and return on errors as before, and keep the rest of the
logic unchanged; reference the podHasKMSSidecar function and the pod variable
when making this change.
- Around line 28-30: init() is registering audit types via
install.Install(cfgScheme) while hasKMSEncryptionProvider decodes
apiserverv1.EncryptionConfiguration; replace the audit scheme registration with
the apiserver types registration (register the apiserver/v1 scheme into
cfgScheme such that apiserverv1.EncryptionConfiguration can be decoded) by
updating the call in init() to use the apiserver scheme installer (the apiserver
registration function for cfgScheme or apiserverv1.AddToScheme), and remove the
now-unused audit import that was introduced for install.Install.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 58-61: The current handling of secretLister.Get swallows all
errors by logging and returning nil; change it to distinguish NotFound from
other errors: call secretLister.Get(...) as before, and if
apierrors.IsNotFound(err) return nil, but for any other non-nil err return that
error (do not just klog.Infof and return nil). Update the block around
secretLister.Get (the code that currently logs "kms is disabled: failed to get
encryption-config-openshift-kube-apiserver secret: %v") to use
apierrors.IsNotFound for the NotFound path and propagate non-NotFound errors
upward so callers of this function see transient API/permission failures.
---
Nitpick comments:
In `@pkg/operator/kms/precondition.go`:
- Around line 98-102: In hasKMSEncryptionProvider, don't silently swallow decode
errors: when codecs.UniversalDeserializer().Decode(encryptionConfigBytes, &gvk,
nil) returns an error, log the error with context (e.g., include "failed to
decode EncryptionConfiguration" and the error value) before returning false so
malformed encryptionConfigBytes are visible in logs; update the error path that
currently returns false to emit a clear log entry referencing gvk and
encryptionConfigBytes (or a short identifier) to aid debugging.
In `@pkg/operator/targetconfigcontroller/kms_sidecar_test.go`:
- Around line 21-78: Add unit tests to TestAddKMSPluginToPodSpec covering
negative and edge cases for AddKMSPluginToPodSpec: add subtests for feature gate
disabled (use featuregates.NewHardcodedFeatureGateAccess without features),
encryption config secret not found (no secrets added to fake.NewClientset),
secret present but missing "encryption-config" key, secrets missing
"kms-provider-config-*" keys, invalid JSON in provider config secret, and nil
podSpec; each subtest should assert the expected error string or behavior using
the existing secretLister helper and newEncryptionConfigSecret helper (or
variants) to construct secrets and reuse the kmsPluginImage fixture.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 213-218: The code appends a corev1.VolumeMount with Name
"kms-plugin-socket" to container.VolumeMounts unconditionally, causing duplicate
mounts if called repeatedly; update the logic around container.VolumeMounts (the
append of corev1.VolumeMount{Name: "kms-plugin-socket", MountPath:
"/var/run/kmsplugin"}) to first scan existing container.VolumeMounts for a mount
with the same Name (or MountPath) and only append when not present, mirroring
the existing deduplication used for volumes so the function (in kms_sidecar.go
where container.VolumeMounts is modified) remains idempotent.
- Around line 165-166: The container spec in kms_sidecar.go currently hardcodes
ImagePullPolicy: corev1.PullAlways which breaks air-gapped environments; change
this to corev1.PullIfNotPresent or make it configurable (e.g., via a KMS sidecar
config field or operator flag) and use that value when constructing the
Container (the location where Image and ImagePullPolicy are set, referencing the
Image variable and ImagePullPolicy property); ensure default is IfNotPresent and
validate/propagate the configured policy into the container spec.
- Around line 167-183: The current construction of the sidecar process line uses
fmt.Sprintf to embed config.Addr, config.Namespace, config.KeyName,
config.RoleID and keySecretID into a single shell string passed to Command with
"/bin/sh -c", which is fragile and unsafe; change it to invoke the binary
directly with explicit arguments (i.e., set Command to
[]string{"/vault-kube-kms"} and Args to the list of flag strings:
"-listen-address=unix:///var/run/kmsplugin/kms.sock",
"-vault-address="+config.Addr, "-vault-namespace="+config.Namespace,
"-transit-mount=transit", "-transit-key="+config.KeyName,
"-log-level=debug-extended", "-approle-role-id="+config.RoleID,
"-approle-secret-id-path=/etc/kubernetes/static-pod-resources/"+keySecretID) so
no shell parsing is required and shell metacharacters in config values cannot
break the invocation.
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Around line 333-340: Replace the hardcoded kmsPluginImage and the
commented-out encryptionConfig line: read the KMS plugin image from the operator
config or environment (reuse the same pattern used for IMAGE/OPERATOR_IMAGE) and
pass that value into AddKMSPluginToPodSpec instead of the literal
"quay.io/bertinatto/vault:v1"; also remove or implement the commented-out
`encryptionConfig, err := secretLister.Secrets(namespace string)` by either
calling the correct secretLister method to retrieve encryptionConfig (and
handling the error) or deleting the placeholder if not needed; keep references
to kmsPluginImage, AddKMSPluginToPodSpec, and secretLister to locate where to
apply the change.
🪄 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: 8c1cb6bc-4ed4-4b73-bb9f-1c0143484fa2
📒 Files selected for processing (5)
pkg/operator/kms/precondition.gopkg/operator/starter.gopkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/kms_sidecar_test.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
Comment on lines
+28
to
+30
| func init() { | ||
| install.Install(cfgScheme) | ||
| } |
There was a problem hiding this comment.
Incorrect scheme registration: audit types registered instead of apiserver types.
The init() function registers audit types via install.Install(cfgScheme), but hasKMSEncryptionProvider decodes apiserverv1.EncryptionConfiguration. The scheme needs apiserver types registered, not audit types.
🐛 Proposed fix
+import (
+ utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+)
+
func init() {
- install.Install(cfgScheme)
+ utilruntime.Must(apiserverv1.AddToScheme(cfgScheme))
}Also remove the unused import:
- "k8s.io/apiserver/pkg/apis/audit/install"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/kms/precondition.go` around lines 28 - 30, init() is registering
audit types via install.Install(cfgScheme) while hasKMSEncryptionProvider
decodes apiserverv1.EncryptionConfiguration; replace the audit scheme
registration with the apiserver types registration (register the apiserver/v1
scheme into cfgScheme such that apiserverv1.EncryptionConfiguration can be
decoded) by updating the call in init() to use the apiserver scheme installer
(the apiserver registration function for cfgScheme or apiserverv1.AddToScheme),
and remove the now-unused audit import that was introduced for install.Install.
Comment on lines
+76
to
+85
| func podHasKMSSidecar(podCM *corev1.ConfigMap, sidecarName string) bool { | ||
| podYAML, ok := podCM.Data["pod.yaml"] | ||
| if !ok { | ||
| return false | ||
| } | ||
|
|
||
| var pod corev1.Pod | ||
| if err := json.Unmarshal([]byte(podYAML), &pod); err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Parsing YAML with json.Unmarshal is unreliable.
The pod.yaml data is likely in YAML format, but json.Unmarshal only handles JSON. While JSON is valid YAML, the reverse isn't true. Consider using yaml.Unmarshal or the universal deserializer for consistency with how the pod is serialized.
🔧 Proposed fix using yaml
+import (
+ "github.com/ghodss/yaml"
+)
+
func podHasKMSSidecar(podCM *corev1.ConfigMap, sidecarName string) bool {
podYAML, ok := podCM.Data["pod.yaml"]
if !ok {
return false
}
var pod corev1.Pod
- if err := json.Unmarshal([]byte(podYAML), &pod); err != nil {
+ if err := yaml.Unmarshal([]byte(podYAML), &pod); err != nil {
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/kms/precondition.go` around lines 76 - 85, The function
podHasKMSSidecar currently uses json.Unmarshal to parse pod.yaml which fails for
non-JSON YAML content; update podHasKMSSidecar to parse the ConfigMap's
"pod.yaml" using a YAML-aware deserializer (e.g., sigs.k8s.io/yaml.Unmarshal) or
the Kubernetes universal deserializer to decode into corev1.Pod, handle and
return on errors as before, and keep the rest of the logic unchanged; reference
the podHasKMSSidecar function and the pod variable when making this change.
Comment on lines
+58
to
+61
| if err != nil { | ||
| klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Non-NotFound errors are swallowed, masking potential issues.
When secretLister.Get() fails with an error other than NotFound, the function logs the error but returns nil. This could mask transient API errors or permission issues, causing silent failures.
🔧 Proposed fix
if err != nil {
- klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", err)
- return nil
+ return fmt.Errorf("failed to get encryption-config-openshift-kube-apiserver secret: %w", err)
}📝 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
| if err != nil { | |
| klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", err) | |
| return nil | |
| } | |
| if err != nil { | |
| if !apierrors.IsNotFound(err) { | |
| return fmt.Errorf("failed to get encryption-config-openshift-kube-apiserver secret: %w", err) | |
| } | |
| klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", 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 58 - 61, The
current handling of secretLister.Get swallows all errors by logging and
returning nil; change it to distinguish NotFound from other errors: call
secretLister.Get(...) as before, and if apierrors.IsNotFound(err) return nil,
but for any other non-nil err return that error (do not just klog.Infof and
return nil). Update the block around secretLister.Get (the code that currently
logs "kms is disabled: failed to get encryption-config-openshift-kube-apiserver
secret: %v") to use apierrors.IsNotFound for the NotFound path and propagate
non-NotFound errors upward so callers of this function see transient
API/permission failures.
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
2bbdfea to
96575b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/operator/targetconfigcontroller/kms_sidecar.go (1)
54-61:⚠️ Potential issue | 🟠 MajorPropagate non-
NotFoundsecret lookup failures.Lines 59-61 currently treat RBAC/cache/API errors as “KMS disabled”. That can silently render a revision without the sidecar even though encryption still requires it. Only
NotFoundshould short-circuit; other errors should fail reconciliation.Suggested fix
encryptionConfig, err := secretLister.Secrets("openshift-config-managed").Get("encryption-config-openshift-kube-apiserver") if apierrors.IsNotFound(err) { klog.Infof("kms is disabled: secret openshift-config-managed/encryption-config not found: %v", err) return nil } if err != nil { - klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", err) - return nil + return fmt.Errorf("failed to get openshift-config-managed/encryption-config-openshift-kube-apiserver: %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 54 - 61, The current secret lookup in kms_sidecar.go calls secretLister.Secrets("openshift-config-managed").Get("encryption-config-openshift-kube-apiserver") and treats any error other than IsNotFound as KMS-disabled; change the logic so only apierrors.IsNotFound(err) short-circuits with a benign log and nil return, while any other error is returned (propagated) from the reconciliation function instead of being logged as “kms is disabled” (i.e., replace the second klog.Infof(...) + return nil path with returning the error so callers can fail/retry).
🤖 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 137-148: The code parses
encryptionConfig.Data[keyKMSProviderConfig] but never checks
encryptionConfig.Data[keySecretID]; before calling addKMSPluginSidecarToPodSpec
(with podSpec, "kms-plugin", kmsPluginImage, vaultConfig, endpoint, keySecretID)
validate that encryptionConfig.Data contains keySecretID and return a clear
contextual error if it is missing so we fail fast rather than creating a sidecar
that references a non-existent secret path.
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Around line 333-339: The code currently hard-codes kmsPluginImage
("quay.io/bertinatto/vault:v2") before calling AddKMSPluginToPodSpec; remove
that literal and instead obtain the KMS sidecar image from the operator's
release/operand image resolution machinery used for other static-pod images (the
same resolver/helper used elsewhere in this controller), propagate any
resolution errors, and pass the resolved image string into
AddKMSPluginToPodSpec; ensure the resolved image is an operator-managed
reference (prefer digest or release-managed tag) and handle/log resolution
failure before returning.
---
Duplicate comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 54-61: The current secret lookup in kms_sidecar.go calls
secretLister.Secrets("openshift-config-managed").Get("encryption-config-openshift-kube-apiserver")
and treats any error other than IsNotFound as KMS-disabled; change the logic so
only apierrors.IsNotFound(err) short-circuits with a benign log and nil return,
while any other error is returned (propagated) from the reconciliation function
instead of being logged as “kms is disabled” (i.e., replace the second
klog.Infof(...) + return nil path with returning the error so callers can
fail/retry).
🪄 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: d469180e-df91-4844-8b49-770a00e2ccdd
📒 Files selected for processing (3)
pkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/kms_sidecar_test.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
96575b7 to
505cb19
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/operator/targetconfigcontroller/kms_sidecar.go (1)
54-62:⚠️ Potential issue | 🟠 MajorPropagate non-NotFound secret lookup failures.
Line 59 still treats API, RBAC, and transient lister failures as “KMS disabled” and returns
nil, which can silently render a pod without the sidecar. Return the error so the controller requeues instead of masking it.Suggested fix
if apierrors.IsNotFound(err) { klog.Infof("kms is disabled: secret openshift-config-managed/encryption-config not found: %v", err) return nil } if err != nil { - klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", err) - return nil + return fmt.Errorf("failed to get encryption-config-openshift-kube-apiserver 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 54 - 62, The current secret lookup treats all non-NotFound errors as "KMS disabled" and returns nil; change the handling after secretLister.Secrets("openshift-config-managed").Get("encryption-config-openshift-kube-apiserver") so that if apierrors.IsNotFound(err) you still log and return nil, but for any other non-nil err you return that err (not nil) so the controller requeues; update the branch that now calls klog.Infof("kms is disabled: failed to get ... %v", err) to instead return err (preserving or adjusting the log as desired) and keep the NotFound path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 140-144: Remove the checked-in replace block that redirects
github.com/openshift/api, github.com/openshift/client-go, and
github.com/openshift/library-go to personal forks (the replace (...) block);
instead revert go.mod to upstream module versions, delete those replace entries
from go.mod, run go mod tidy to update the module graph, and use a local go.work
(or local replace in your own workspace) for any temporary testing against forks
so CI/release builds do not consume unvetted overrides.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 141-178: The code uses credentials["roleID"] and the
credentialsKey without validating the decoded credentials map; update
addKMSPluginSidecarToPodSpec to validate required Vault AppRole fields (at
minimum "roleID" and the secret blob referenced by credentialsKey or "secretID")
and that they are non-empty before mutating podSpec. If any required key is
missing or empty, return a descriptive error (do not append the container or add
mounts). Use the existing function name addKMSPluginSidecarToPodSpec and the
credentials map/credentialsKey symbols to locate where to add the checks and
short-circuit on failure.
---
Duplicate comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 54-62: The current secret lookup treats all non-NotFound errors as
"KMS disabled" and returns nil; change the handling after
secretLister.Secrets("openshift-config-managed").Get("encryption-config-openshift-kube-apiserver")
so that if apierrors.IsNotFound(err) you still log and return nil, but for any
other non-nil err you return that err (not nil) so the controller requeues;
update the branch that now calls klog.Infof("kms is disabled: failed to get ...
%v", err) to instead return err (preserving or adjusting the log as desired) and
keep the NotFound path unchanged.
🪄 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: 8d9d0306-ba0d-4da5-9abf-792a3739bec3
⛔ Files ignored due to path filters (99)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_dns.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/awsdnsspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/infrastructurestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/kmsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/prefixedclaimmapping.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/usernameclaimmapping.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vaultkmsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/alertmanagercustomconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/containerresource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/metricsserverconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/openshiftstatemetricsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatoradmissionwebhookconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatorconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/telemeterclientconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/thanosquerierconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/awscsidriverconfigspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/bgpmanagedconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/ingresscontrollertuningoptions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/nooverlayconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/ovnkubernetesconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponent.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallercomponentsource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallerrevision.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapiinstallerrevisionmanifestsubstitution.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1/clusterapistatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/controller/controllercmd/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptionconfig/secret.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/vault.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modpkg/operator/starter.gopkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/kms_sidecar_test.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/targetconfigcontroller/kms_sidecar_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/targetconfigcontroller/targetconfigcontroller.go
Comment on lines
+140
to
+144
| replace ( | ||
| github.com/openshift/api => github.com/ardaguclu/openshift-api v0.0.0-20260416125923-e7b399e13aaf | ||
| github.com/openshift/client-go => github.com/ardaguclu/client-go v0.0.0-20260416132153-8efa801dc740 | ||
| github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260416132259-5e6285d10163 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if go.mod exists and examine the relevant lines
if [ -f "go.mod" ]; then
echo "=== go.mod file found ==="
wc -l go.mod
echo ""
echo "=== Lines 135-150 of go.mod ==="
sed -n '135,150p' go.mod
else
echo "go.mod not found in root"
find . -name "go.mod" -type f | head -10
fiRepository: openshift/cluster-kube-apiserver-operator
Length of output: 620
🏁 Script executed:
# Check for go.work file
if [ -f "go.work" ]; then
echo "=== go.work exists ==="
cat go.work
else
echo "No go.work file found in root"
fiRepository: openshift/cluster-kube-apiserver-operator
Length of output: 114
🏁 Script executed:
# Check git context to understand if this is test-only
git log --oneline -20 2>/dev/null || echo "Not a git repo or git unavailable"Repository: openshift/cluster-kube-apiserver-operator
Length of output: 138
🏁 Script executed:
# Look for any test or branch-related documentation
find . -maxdepth 2 -type f \( -name "README*" -o -name "TESTING*" -o -name ".github" \) 2>/dev/null | head -5Repository: openshift/cluster-kube-apiserver-operator
Length of output: 96
🏁 Script executed:
# Check the README for context about the project and testing
head -100 README.mdRepository: openshift/cluster-kube-apiserver-operator
Length of output: 3953
🏁 Script executed:
# Check if there are any comments or documentation about these fork dependencies
grep -n "ardaguclu" . -r --include="*.md" --include="*.txt" 2>/dev/null | head -20Repository: openshift/cluster-kube-apiserver-operator
Length of output: 954
🏁 Script executed:
# Look at the actual dependency to see when it was added and any related comments
git log --oneline --all | grep -i "ardaguclu\|fork\|replace" | head -20Repository: openshift/cluster-kube-apiserver-operator
Length of output: 1087
🏁 Script executed:
# Check if there's a PR or branch context in git
git branch -a 2>/dev/null | head -20
git status 2>/dev/null | head -10Repository: openshift/cluster-kube-apiserver-operator
Length of output: 248
Keep fork overrides out of the checked-in module graph.
The replace block in lines 140-144 redirects every build to personal forks. If this leaks into a merged commit, all CI and release builds would consume these unvetted OpenShift modules. Use a local go.work file for testing instead of committing these overrides to go.mod.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 140 - 144, Remove the checked-in replace block that
redirects github.com/openshift/api, github.com/openshift/client-go, and
github.com/openshift/library-go to personal forks (the replace (...) block);
instead revert go.mod to upstream module versions, delete those replace entries
from go.mod, run go mod tidy to update the module graph, and use a local go.work
(or local replace in your own workspace) for any temporary testing against forks
so CI/release builds do not consume unvetted overrides.
Comment on lines
+141
to
+178
| var credentials map[string][]byte | ||
| if err := json.Unmarshal(credentialsData, &credentials); err != nil { | ||
| return fmt.Errorf("failed to unmarshal KMS credentials: %w", err) | ||
| } | ||
|
|
||
| klog.Infof("kms is enabled: found config, now patching kube-apiserver pod") | ||
| if err := addKMSPluginSidecarToPodSpec(podSpec, "kms-plugin", kmsPluginImage, kmsProviderConfig.Vault, kmsConfig.Endpoint, credentials, credentialsKey); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kms-plugin"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kube-apiserver"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func addKMSPluginSidecarToPodSpec(podSpec *corev1.PodSpec, containerName, image string, vaultConfig *configv1.VaultKMSConfig, endpoint string, credentials map[string][]byte, credentialsKey string) error { | ||
| if podSpec == nil { | ||
| return fmt.Errorf("pod spec cannot be nil") | ||
| } | ||
|
|
||
| podSpec.Containers = append(podSpec.Containers, corev1.Container{ | ||
| Name: containerName, | ||
| Image: image, | ||
| Args: []string{ | ||
| "--log-level=debug-extended", | ||
| fmt.Sprintf("--listen-address=%s", endpoint), | ||
| fmt.Sprintf("--vault-address=%s", vaultConfig.VaultAddress), | ||
| fmt.Sprintf("--vault-namespace=%s", vaultConfig.VaultNamespace), | ||
| fmt.Sprintf("--transit-key=%s", vaultConfig.TransitKey), | ||
| fmt.Sprintf("--transit-mount=%s", vaultConfig.TransitMount), | ||
| fmt.Sprintf("--approle-role-id=%s", string(credentials["roleID"])), | ||
| fmt.Sprintf("--approle-secret-id-path=/etc/kubernetes/static-pod-resources/%s", credentialsKey), |
There was a problem hiding this comment.
Validate required Vault credential fields before mutating the pod.
Line 177 reads credentials["roleID"] without checking that the decoded blob actually contains roleID, and the sidecar is wired up even if secretID is absent. A malformed Secret should fail fast here instead of rolling out a pod with empty or unusable AppRole inputs.
Suggested fix
var credentials map[string][]byte
if err := json.Unmarshal(credentialsData, &credentials); err != nil {
return fmt.Errorf("failed to unmarshal KMS credentials: %w", err)
}
+ if len(credentials["roleID"]) == 0 {
+ return fmt.Errorf("missing roleID in credentials key %s", credentialsKey)
+ }
+ if len(credentials["secretID"]) == 0 {
+ return fmt.Errorf("missing secretID in credentials key %s", credentialsKey)
+ }
klog.Infof("kms is enabled: found config, now patching kube-apiserver pod")📝 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
| var credentials map[string][]byte | |
| if err := json.Unmarshal(credentialsData, &credentials); err != nil { | |
| return fmt.Errorf("failed to unmarshal KMS credentials: %w", err) | |
| } | |
| klog.Infof("kms is enabled: found config, now patching kube-apiserver pod") | |
| if err := addKMSPluginSidecarToPodSpec(podSpec, "kms-plugin", kmsPluginImage, kmsProviderConfig.Vault, kmsConfig.Endpoint, credentials, credentialsKey); err != nil { | |
| return err | |
| } | |
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kms-plugin"); err != nil { | |
| return err | |
| } | |
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kube-apiserver"); err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func addKMSPluginSidecarToPodSpec(podSpec *corev1.PodSpec, containerName, image string, vaultConfig *configv1.VaultKMSConfig, endpoint string, credentials map[string][]byte, credentialsKey string) error { | |
| if podSpec == nil { | |
| return fmt.Errorf("pod spec cannot be nil") | |
| } | |
| podSpec.Containers = append(podSpec.Containers, corev1.Container{ | |
| Name: containerName, | |
| Image: image, | |
| Args: []string{ | |
| "--log-level=debug-extended", | |
| fmt.Sprintf("--listen-address=%s", endpoint), | |
| fmt.Sprintf("--vault-address=%s", vaultConfig.VaultAddress), | |
| fmt.Sprintf("--vault-namespace=%s", vaultConfig.VaultNamespace), | |
| fmt.Sprintf("--transit-key=%s", vaultConfig.TransitKey), | |
| fmt.Sprintf("--transit-mount=%s", vaultConfig.TransitMount), | |
| fmt.Sprintf("--approle-role-id=%s", string(credentials["roleID"])), | |
| fmt.Sprintf("--approle-secret-id-path=/etc/kubernetes/static-pod-resources/%s", credentialsKey), | |
| var credentials map[string][]byte | |
| if err := json.Unmarshal(credentialsData, &credentials); err != nil { | |
| return fmt.Errorf("failed to unmarshal KMS credentials: %w", err) | |
| } | |
| if len(credentials["roleID"]) == 0 { | |
| return fmt.Errorf("missing roleID in credentials key %s", credentialsKey) | |
| } | |
| if len(credentials["secretID"]) == 0 { | |
| return fmt.Errorf("missing secretID in credentials key %s", credentialsKey) | |
| } | |
| klog.Infof("kms is enabled: found config, now patching kube-apiserver pod") | |
| if err := addKMSPluginSidecarToPodSpec(podSpec, "kms-plugin", kmsPluginImage, kmsProviderConfig.Vault, kmsConfig.Endpoint, credentials, credentialsKey); err != nil { | |
| return err | |
| } | |
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kms-plugin"); err != nil { | |
| return err | |
| } | |
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kube-apiserver"); err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func addKMSPluginSidecarToPodSpec(podSpec *corev1.PodSpec, containerName, image string, vaultConfig *configv1.VaultKMSConfig, endpoint string, credentials map[string][]byte, credentialsKey string) error { | |
| if podSpec == nil { | |
| return fmt.Errorf("pod spec cannot be nil") | |
| } | |
| podSpec.Containers = append(podSpec.Containers, corev1.Container{ | |
| Name: containerName, | |
| Image: image, | |
| Args: []string{ | |
| "--log-level=debug-extended", | |
| fmt.Sprintf("--listen-address=%s", endpoint), | |
| fmt.Sprintf("--vault-address=%s", vaultConfig.VaultAddress), | |
| fmt.Sprintf("--vault-namespace=%s", vaultConfig.VaultNamespace), | |
| fmt.Sprintf("--transit-key=%s", vaultConfig.TransitKey), | |
| fmt.Sprintf("--transit-mount=%s", vaultConfig.TransitMount), | |
| fmt.Sprintf("--approle-role-id=%s", string(credentials["roleID"])), | |
| fmt.Sprintf("--approle-secret-id-path=/etc/kubernetes/static-pod-resources/%s", credentialsKey), |
🤖 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 141 - 178,
The code uses credentials["roleID"] and the credentialsKey without validating
the decoded credentials map; update addKMSPluginSidecarToPodSpec to validate
required Vault AppRole fields (at minimum "roleID" and the secret blob
referenced by credentialsKey or "secretID") and that they are non-empty before
mutating podSpec. If any required key is missing or empty, return a descriptive
error (do not append the container or add mounts). Use the existing function
name addKMSPluginSidecarToPodSpec and the credentials map/credentialsKey symbols
to locate where to add the checks and short-circuit on failure.
ac05cf6 to
8241914
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/operator/targetconfigcontroller/kms_sidecar.go (2)
146-153:⚠️ Potential issue | 🟠 MajorMissing validation for required credential fields.
The
credentialsmap is unmarshaled butroleIDandsecretIDare not validated before use. Line 183 usescredentials["roleID"]directly—if the key is missing,string(nil)yields an empty string, silently producing an unusable AppRole configuration.🛡️ Proposed fix to validate credentials
var credentials map[string][]byte if err := json.Unmarshal(credentialsData, &credentials); err != nil { return fmt.Errorf("failed to unmarshal KMS credentials: %w", err) } + if len(credentials["roleID"]) == 0 { + return fmt.Errorf("missing or empty roleID in credentials key %s", credentialsKey) + } + if len(credentials["secretID"]) == 0 { + return fmt.Errorf("missing or empty secretID in credentials key %s", credentialsKey) + } klog.Infof("kms is enabled: found config, now patching kube-apiserver pod")🤖 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 146 - 153, credentials map is unmarshaled but roleID and secretID are not validated; before calling addKMSPluginSidecarToPodSpec check that credentials contains non-empty values for "roleID" and "secretID" (e.g., val, ok := credentials["roleID"]; if !ok || len(val)==0 => return error) and return a descriptive fmt.Errorf if either is missing/empty so addKMSPluginSidecarToPodSpec receives valid AppRole data; reference the credentials variable and the "roleID"/"secretID" keys in your validation.
60-63:⚠️ Potential issue | 🟠 MajorNon-NotFound errors are swallowed, masking transient failures.
When
secretLister.Get()fails with an error other than NotFound, the function logs and returnsnil. This masks transient API errors or permission issues, potentially causing silent failures in KMS setup.🔧 Proposed fix
if err != nil { - klog.Infof("kms is disabled: failed to get encryption-config-openshift-kube-apiserver secret: %v", err) - return nil + return fmt.Errorf("failed to get encryption-config-openshift-kube-apiserver 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 60 - 63, The current handler swallows all errors from secretLister.Get() by logging and returning nil; change it to treat NotFound specially but surface other errors: call secretLister.Get(...), and if apierrors.IsNotFound(err) then log ("kms is disabled...") and return nil, but if err != nil and not NotFound, log the error with context and return the error (or wrap it) instead of nil; use k8s.io/apimachinery/pkg/api/errors.IsNotFound to detect NotFound and update the surrounding function (the call site using secretLister.Get() and the klog.Infof return path) accordingly.
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/kms_sidecar.go (1)
167-198: Consider adding resource requests/limits for the KMS sidecar.The TODO at line 172 notes that resource requests/limits should be set. For a production-ready sidecar running alongside kube-apiserver, this is important to prevent resource starvation and ensure proper scheduling.
Additionally,
Privileged: new(bool)sets privileged tofalseexplicitly, which is good security practice.Do you want me to propose reasonable default resource requests/limits for the KMS plugin sidecar, or open an issue to track this?
🤖 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 167 - 198, In addKMSPluginSidecarToPodSpec, populate the container's Resources (corev1.ResourceRequirements) instead of leaving the TODO: add a reasonable default Requests and Limits (e.g., cpu and memory) for both scheduling and resource isolation, and attach it to the appended corev1.Container for the KMS sidecar; keep the existing SecurityContext with Privileged set to false (Privileged: new(bool)) but consider wiring the resource values to configuration or constants so they can be tuned later.
🤖 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_test.go`:
- Around line 41-70: The test and implementation reference missing symbols
(configv1.VaultKMSConfig, configv1.VaultKMSProvider,
secrets.EncryptionSecretKMSSecretData) which don't exist in the vendored deps;
replace them with test-local equivalents or existing vendored types: define a
small test-only VaultKMSConfig struct and a provider constant (e.g. type
VaultKMSConfig struct { VaultAddress, VaultNamespace, TransitKey, TransitMount
string } and const VaultKMSProvider = "vault") and define the secret-data key
string constant (e.g. EncryptionSecretKMSSecretData :=
"encryption-kms-secret-data") in the test package, then update usages in
kms_sidecar_test.go and kms_sidecar.go to use these local symbols so the code
compiles without relying on missing external packages.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 130-137: The code references unreleased types
(configv1.VaultKMSProvider, kmsProviderConfig.Vault, configv1.VaultKMSConfig)
causing build failures; replace these direct references by unmarshalling
providerConfigData into a generic structure (e.g., map[string]interface{} or a
small local struct with a Type string and a raw Provider field) and then check
the "type" value for the expected vault provider string and validate the
presence/shape of the vault-specific subkey instead of using
configv1.VaultKMSConfig; update uses of kmsProviderConfig, providerConfigData,
configv1.KMSConfig and configv1.VaultKMSConfig accordingly, or alternatively
revert to the previous API usage or update the github.com/openshift/api
dependency once the Vault types are officially released.
- Around line 139-144: The build fails because the code references the undefined
constant secrets.EncryptionSecretKMSSecretData in kms_sidecar.go (used to build
credentialsKey); fix by defining that constant either upstream in the vendored
library-go or locally in this package. Add a constant named
EncryptionSecretKMSSecretData (matching the key name expected in
encryptionConfig.Data) in the same package that imports secrets or in a local
constants file, and update usages in kms_sidecar.go and tests to use this new
constant instead of the missing symbol so credentialsKey = fmt.Sprintf("%s-%s",
secrets.EncryptionSecretKMSSecretData, keyID) resolves.
---
Duplicate comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 146-153: credentials map is unmarshaled but roleID and secretID
are not validated; before calling addKMSPluginSidecarToPodSpec check that
credentials contains non-empty values for "roleID" and "secretID" (e.g., val, ok
:= credentials["roleID"]; if !ok || len(val)==0 => return error) and return a
descriptive fmt.Errorf if either is missing/empty so
addKMSPluginSidecarToPodSpec receives valid AppRole data; reference the
credentials variable and the "roleID"/"secretID" keys in your validation.
- Around line 60-63: The current handler swallows all errors from
secretLister.Get() by logging and returning nil; change it to treat NotFound
specially but surface other errors: call secretLister.Get(...), and if
apierrors.IsNotFound(err) then log ("kms is disabled...") and return nil, but if
err != nil and not NotFound, log the error with context and return the error (or
wrap it) instead of nil; use k8s.io/apimachinery/pkg/api/errors.IsNotFound to
detect NotFound and update the surrounding function (the call site using
secretLister.Get() and the klog.Infof return path) accordingly.
---
Nitpick comments:
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 167-198: In addKMSPluginSidecarToPodSpec, populate the container's
Resources (corev1.ResourceRequirements) instead of leaving the TODO: add a
reasonable default Requests and Limits (e.g., cpu and memory) for both
scheduling and resource isolation, and attach it to the appended
corev1.Container for the KMS sidecar; keep the existing SecurityContext with
Privileged set to false (Privileged: new(bool)) but consider wiring the resource
values to configuration or constants so they can be tuned later.
🪄 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: 0b85c883-e5ba-4c45-b5c5-42c67765dabc
⛔ Files ignored due to path filters (12)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/controller/controllercmd/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/state_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptionconfig/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/encryptionconfig/secret.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/secrets.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/encryption/state/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/kms/assets/k8s_mock_kms_plugin_daemonset.yamlis excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/kms_sidecar_test.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Comment on lines
+139
to
+144
| // Read the credentials (map[string][]byte with "roleID" and "secretID" keys) | ||
| credentialsKey := fmt.Sprintf("%s-%s", secrets.EncryptionSecretKMSSecretData, keyID) | ||
| credentialsData, ok := encryptionConfig.Data[credentialsKey] | ||
| if !ok { | ||
| return fmt.Errorf("missing credentials key %s in encryption-config secret", credentialsKey) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all EncryptionSecret* constants in the secrets package
echo "=== Constants in encryption/secrets/types.go ==="
cat vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go 2>/dev/null || echo "File not found"Repository: openshift/cluster-kube-apiserver-operator
Length of output: 4566
🏁 Script executed:
#!/bin/bash
# Search for usages of EncryptionSecretKMSSecretData in the codebase
echo "=== Searching for EncryptionSecretKMSSecretData usage ==="
rg "EncryptionSecretKMSSecretData" -A 2 -B 2
echo -e "\n=== Checking if constant is defined anywhere ==="
rg "EncryptionSecretKMSSecretData\s*="
echo -e "\n=== Searching for similar constants ==="
rg "EncryptionSecretKMS" vendor/github.com/openshift/library-go/Repository: openshift/cluster-kube-apiserver-operator
Length of output: 4299
🏁 Script executed:
#!/bin/bash
# Check the broader context of kms_sidecar.go around the usage
echo "=== Context in kms_sidecar.go (lines 130-160) ==="
sed -n '130,160p' pkg/operator/targetconfigcontroller/kms_sidecar.go
echo -e "\n=== Check imports in kms_sidecar.go ==="
head -30 pkg/operator/targetconfigcontroller/kms_sidecar.go | grep -A 30 "^import"Repository: openshift/cluster-kube-apiserver-operator
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Double-check if there are any other undefined constants being used
echo "=== All EncryptionSecret* constants used in code ==="
rg "secrets\.EncryptionSecret" --no-heading | sort -u
echo -e "\n=== All EncryptionSecret* constants defined in library-go ==="
rg "EncryptionSecret[A-Z]" vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go | grep "=" | sort -uRepository: openshift/cluster-kube-apiserver-operator
Length of output: 2981
Build failure: undefined constant secrets.EncryptionSecretKMSSecretData.
The constant secrets.EncryptionSecretKMSSecretData is not defined in the vendored library-go package. Per the code in vendor/github.com/openshift/library-go/pkg/operator/encryption/secrets/types.go, only EncryptionSecretKMSProviderConfig and EncryptionSecretKMSEncryptionConfig are defined.
This constant is used in production code at line 140 and in test code, and will prevent the build from succeeding. Add it to the library-go fork or define it locally.
🤖 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 139 - 144,
The build fails because the code references the undefined constant
secrets.EncryptionSecretKMSSecretData in kms_sidecar.go (used to build
credentialsKey); fix by defining that constant either upstream in the vendored
library-go or locally in this package. Add a constant named
EncryptionSecretKMSSecretData (matching the key name expected in
encryptionConfig.Data) in the same package that imports secrets or in a local
constants file, and update usages in kms_sidecar.go and tests to use this new
constant instead of the missing symbol so credentialsKey = fmt.Sprintf("%s-%s",
secrets.EncryptionSecretKMSSecretData, keyID) resolves.
fb889ee to
1eb6c5d
Compare
openshift-ci
Bot
added
the
needs-rebase
bertinatto
force-pushed
the
kms-plugin-lifecycle
branch
from
1eb6c5d to
940215b
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Contributor
|
@bertinatto: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
This PR is used only for testing and to create clusters from.
Please ignore.
Summary by CodeRabbit
New Features
Chores