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:
WalkthroughThe changes implement KMS plugin sidecar injection into the static pod operator, replacing a prior volume-based approach. A new file contains logic to detect KMS encryption configuration from secrets, extract KMS endpoints, and inject sidecar containers into the kube-apiserver pod spec. The controller is updated to use this new mechanism and pass required dependencies. Module dependencies are adjusted, a revision precondition is added to the startup sequence, and a migration test is disabled. Changes
Sequence DiagramsequenceDiagram
actor TargetConfigController as TargetConfigController
participant FeatureGates as Feature Gates
participant SecretStore as Secret Store
participant EncConfig as Encryption Config Parser
participant PodSpecMutator as Pod Spec Mutator
participant KubernetesPod as kube-apiserver Pod
TargetConfigController->>FeatureGates: Check KMS feature gate enabled?
alt KMS Feature Gate Disabled
TargetConfigController-->>TargetConfigController: Return early (no mutations)
else KMS Feature Gate Enabled
TargetConfigController->>SecretStore: Read encryption-config-openshift-kube-apiserver Secret
SecretStore-->>TargetConfigController: Return encryption config payload
TargetConfigController->>EncConfig: Decode & scan for KMS providers
EncConfig-->>TargetConfigController: Extract unique KMS endpoints & key IDs
alt KMS Providers Found
TargetConfigController->>PodSpecMutator: Inject EmptyDir volume (socket)
TargetConfigController->>PodSpecMutator: Inject hostPath volume (SoftHSM token)
TargetConfigController->>PodSpecMutator: Inject init container (bootstrap token)
TargetConfigController->>PodSpecMutator: Mount socket volume in kube-apiserver container
TargetConfigController->>PodSpecMutator: Inject sidecar container per KMS key ID
PodSpecMutator->>KubernetesPod: Apply mutations to pod spec
KubernetesPod-->>TargetConfigController: Pod spec updated
else No KMS Providers Found
TargetConfigController-->>TargetConfigController: No mutations (early return)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ardaguclu 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: 4
🧹 Nitpick comments (5)
cmd/cluster-kube-apiserver-operator/main.go (1)
19-19: Minor: Import ordering.The
targetconfigcontrollerimport is placed betweencertregenerationcontrollerandcheckendpoints, breaking alphabetical order within the local package imports group. Consider moving it afterstartupmonitorreadinessfor consistency.🤖 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` at line 19, Move the import "github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/targetconfigcontroller" so it follows the other local package imports and restores alphabetical ordering within that group (place it after "startupmonitorreadiness"); update the import block so local imports are alphabetized and keep third-party/std imports groups unchanged.test/e2e-encryption-kms/encryption_kms_test.go (1)
57-76: Uset.Skip()instead of commenting out entire test functions.Commenting out
TestKMSEncryptionProvidersMigrationmakes it invisible to test runners and CI reports. Usingt.Skip("reason")at the start of the function preserves test visibility and documents why it's skipped, making it easier to track and re-enable later.func TestKMSEncryptionProvidersMigration(t *testing.T) { t.Skip("Temporarily disabled while refactoring KMS sidecar injection - see PR `#2103`") // ... rest of test }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-kms/encryption_kms_test.go` around lines 57 - 76, Uncomment the TestKMSEncryptionProvidersMigration function and, at the top of that function (before any setup), call t.Skip with a brief reason string so the test remains visible to runners; locate the commented block for TestKMSEncryptionProvidersMigration and replace the leading comment with a real function body that begins by invoking t.Skip("reason") while leaving the rest of the original test body (librarykms.DeployUpstreamMockKMSPlugin, library.TestEncryptionProvidersMigration call, etc.) unchanged so it can be re-enabled later.pkg/operator/targetconfigcontroller/kms_sidecar.go (3)
134-148: Missing idempotency check for sidecar container.
addKMSSidecarContainerunconditionally appends a new container without checking if one with the same name already exists. This could result in duplicate containers if the mutation is invoked multiple times.Proposed fix
func addKMSSidecarContainer(podSpec *corev1.PodSpec, keyID, endpoint string) { containerName := fmt.Sprintf("kms-plugin-%s", keyID) + // Check if container already exists + for _, c := range podSpec.Containers { + if c.Name == containerName { + return + } + } + podSpec.Containers = append(podSpec.Containers, corev1.Container{🤖 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 134 - 148, addKMSSidecarContainer currently always appends a new container which can create duplicates; modify addKMSSidecarContainer to compute containerName (fmt.Sprintf("kms-plugin-%s", keyID)), iterate podSpec.Containers to check for an existing container with that Name, and if found update its Image, Command, Args and VolumeMounts (or simply return) instead of appending; only append the new corev1.Container when no matching containerName exists, referencing podSpec.Containers, kmsPluginImage, kmsSocketVolumeName and kmsSocketMountPath to keep behavior consistent.
4-4: Use passed context instead ofcontext.TODO().The function creates a closure but uses
context.TODO()on line 33. ThePodMutationFuncsignature doesn't include context, but if cancellation semantics are needed, consider whether the factory function should capture a context or ifcontext.Background()is more appropriate thanTODO()for production code.🤖 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 4, The closure creating the PodMutationFunc currently calls context.TODO() (in kms_sidecar.go) which is inappropriate; modify the factory that returns PodMutationFunc to accept a context.Context parameter (e.g., add ctx to the factory signature) and capture that ctx inside the closure to replace context.TODO(); if the design intentionally does not support cancellation, explicitly use context.Background() instead of TODO() so intent is clear (update the factory function and the closure where PodMutationFunc is constructed and referenced).
119-132: Missing idempotency check for volume mount.Unlike
addKMSSocketVolumewhich checks for existing volume,addKMSSocketVolumeMountunconditionally appends the mount. If the mutation function runs multiple times or the mount already exists, duplicates will be created.Proposed fix
func addKMSSocketVolumeMount(podSpec *corev1.PodSpec, containerName string) error { for i, container := range podSpec.Containers { if container.Name == containerName { + // Check if mount already exists + for _, vm := range container.VolumeMounts { + if vm.Name == kmsSocketVolumeName { + return nil + } + } podSpec.Containers[i].VolumeMounts = append(podSpec.Containers[i].VolumeMounts, corev1.VolumeMount{ Name: kmsSocketVolumeName, MountPath: kmsSocketMountPath, }, ) return nil } } return fmt.Errorf("container %s not found", containerName) }🤖 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 119 - 132, addKMSSocketVolumeMount unconditionally appends a VolumeMount causing duplicates on repeated mutations; modify addKMSSocketVolumeMount to first iterate podSpec.Containers to find container by containerName, then check the container's VolumeMounts for an existing mount matching kmsSocketVolumeName or kmsSocketMountPath and return nil if found, otherwise append the new corev1.VolumeMount (using kmsSocketVolumeName and kmsSocketMountPath) and return nil; keep the existing error return fmt.Errorf("container %s not found", containerName) if the container is missing.
🤖 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 15-21: The kmsPluginImage constant currently hardcodes a test
image ("quay.io/rhn_support_rgangwar/mock-kms-plugin-vault:latest") which must
not be used in production; change the implementation so that kmsPluginImage is
not a hardcoded constant but instead is sourced from a configuration (e.g., the
kms-provider-config field on the encryption-key secret or an operator
ConfigMap/flag/environment variable) and ensure the image tag is fixed (no
":latest") for reproducibility; update any code paths that reference
kmsPluginImage (and keep kmsSocketVolumeName and kmsSocketMountPath as-is) to
read the configured image and validate it exists before deploying.
- Around line 33-37: The code is returning nil for any error from
o.KubeClient.CoreV1().Secrets(...).Get which hides real failures; change the
error handling to treat only "not found" as benign and surface all other errors.
Specifically, after calling o.KubeClient.CoreV1().Secrets(o.Namespace).Get(...),
use apierrors.IsNotFound(err) to detect a missing secret (log and return nil)
and for any other error log the failure with context (including err) and return
the error (or wrap it) instead of swallowing it; add the import for
k8s.io/apimachinery/pkg/api/errors as apierrors and update the klog call
locations around secretName and o.Namespace accordingly.
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Line 312: The managePods function signature includes an unused parameter
secretLister which should be removed to avoid unused parameter warnings or
confusion; update the managePods declaration (and all its callers) to drop the
secretLister parameter, or if it's intentionally reserved for future use, keep
the parameter and add a clear TODO comment referencing secretLister inside the
managePods function body explaining the future purpose; ensure you update any
references to managePods so signatures match (search for managePods(...,
secretLister) and adjust accordingly).
---
Nitpick comments:
In `@cmd/cluster-kube-apiserver-operator/main.go`:
- Line 19: Move the import
"github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/targetconfigcontroller"
so it follows the other local package imports and restores alphabetical ordering
within that group (place it after "startupmonitorreadiness"); update the import
block so local imports are alphabetized and keep third-party/std imports groups
unchanged.
In `@pkg/operator/targetconfigcontroller/kms_sidecar.go`:
- Around line 134-148: addKMSSidecarContainer currently always appends a new
container which can create duplicates; modify addKMSSidecarContainer to compute
containerName (fmt.Sprintf("kms-plugin-%s", keyID)), iterate podSpec.Containers
to check for an existing container with that Name, and if found update its
Image, Command, Args and VolumeMounts (or simply return) instead of appending;
only append the new corev1.Container when no matching containerName exists,
referencing podSpec.Containers, kmsPluginImage, kmsSocketVolumeName and
kmsSocketMountPath to keep behavior consistent.
- Line 4: The closure creating the PodMutationFunc currently calls
context.TODO() (in kms_sidecar.go) which is inappropriate; modify the factory
that returns PodMutationFunc to accept a context.Context parameter (e.g., add
ctx to the factory signature) and capture that ctx inside the closure to replace
context.TODO(); if the design intentionally does not support cancellation,
explicitly use context.Background() instead of TODO() so intent is clear (update
the factory function and the closure where PodMutationFunc is constructed and
referenced).
- Around line 119-132: addKMSSocketVolumeMount unconditionally appends a
VolumeMount causing duplicates on repeated mutations; modify
addKMSSocketVolumeMount to first iterate podSpec.Containers to find container by
containerName, then check the container's VolumeMounts for an existing mount
matching kmsSocketVolumeName or kmsSocketMountPath and return nil if found,
otherwise append the new corev1.VolumeMount (using kmsSocketVolumeName and
kmsSocketMountPath) and return nil; keep the existing error return
fmt.Errorf("container %s not found", containerName) if the container is missing.
In `@test/e2e-encryption-kms/encryption_kms_test.go`:
- Around line 57-76: Uncomment the TestKMSEncryptionProvidersMigration function
and, at the top of that function (before any setup), call t.Skip with a brief
reason string so the test remains visible to runners; locate the commented block
for TestKMSEncryptionProvidersMigration and replace the leading comment with a
real function body that begins by invoking t.Skip("reason") while leaving the
rest of the original test body (librarykms.DeployUpstreamMockKMSPlugin,
library.TestEncryptionProvidersMigration call, etc.) unchanged so it can be
re-enabled 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: 12388e07-5397-4384-b698-46ea4b332a47
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/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/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/staticpod/installerpod/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
cmd/cluster-kube-apiserver-operator/main.gogo.modpkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/targetconfigcontroller.gotest/e2e-encryption-kms/encryption_kms_test.go
Comment on lines
+15
to
+21
| const ( | ||
| // TODO: this will be replaced by the kms-provider-config data field from the encryption-key secret | ||
| // once the library-go encryption controllers support it. | ||
| kmsPluginImage = "quay.io/rhn_support_rgangwar/mock-kms-plugin-vault:latest" | ||
| kmsSocketVolumeName = "kms-plugin-socket" | ||
| kmsSocketMountPath = "/var/run/kmsplugin" | ||
| ) |
There was a problem hiding this comment.
Hardcoded mock image must be replaced before production use.
The kmsPluginImage constant points to a test image (quay.io/rhn_support_rgangwar/mock-kms-plugin-vault:latest). The TODO comment indicates this will be replaced, but ensure this is tracked and addressed before merging. Using :latest tag is also problematic for reproducibility.
🤖 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 - 21, The
kmsPluginImage constant currently hardcodes a test image
("quay.io/rhn_support_rgangwar/mock-kms-plugin-vault:latest") which must not be
used in production; change the implementation so that kmsPluginImage is not a
hardcoded constant but instead is sourced from a configuration (e.g., the
kms-provider-config field on the encryption-key secret or an operator
ConfigMap/flag/environment variable) and ensure the image tag is fixed (no
":latest") for reproducibility; update any code paths that reference
kmsPluginImage (and keep kmsSocketVolumeName and kmsSocketMountPath as-is) to
read the configured image and validate it exists before deploying.
Comment on lines
+33
to
+37
| secret, err := o.KubeClient.CoreV1().Secrets(o.Namespace).Get(context.TODO(), secretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| klog.V(4).Infof("No encryption config secret %s/%s: %v", o.Namespace, secretName, err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Swallowing all secret fetch errors may hide real problems.
Returning nil on any error (including permission denied, network issues, etc.) makes debugging difficult. Consider distinguishing between "secret not found" (expected when encryption is disabled) and other errors that indicate actual problems.
Proposed fix
secret, err := o.KubeClient.CoreV1().Secrets(o.Namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err != nil {
- klog.V(4).Infof("No encryption config secret %s/%s: %v", o.Namespace, secretName, err)
- return nil
+ if apierrors.IsNotFound(err) {
+ klog.V(4).Infof("No encryption config secret %s/%s, skipping KMS injection", o.Namespace, secretName)
+ return nil
+ }
+ return fmt.Errorf("failed to get encryption config secret %s/%s: %w", o.Namespace, secretName, 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
| secret, err := o.KubeClient.CoreV1().Secrets(o.Namespace).Get(context.TODO(), secretName, metav1.GetOptions{}) | |
| if err != nil { | |
| klog.V(4).Infof("No encryption config secret %s/%s: %v", o.Namespace, secretName, err) | |
| return nil | |
| } | |
| secret, err := o.KubeClient.CoreV1().Secrets(o.Namespace).Get(context.TODO(), secretName, metav1.GetOptions{}) | |
| if err != nil { | |
| if apierrors.IsNotFound(err) { | |
| klog.V(4).Infof("No encryption config secret %s/%s, skipping KMS injection", o.Namespace, secretName) | |
| return nil | |
| } | |
| return fmt.Errorf("failed to get encryption config secret %s/%s: %w", o.Namespace, secretName, 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 33 - 37, The
code is returning nil for any error from o.KubeClient.CoreV1().Secrets(...).Get
which hides real failures; change the error handling to treat only "not found"
as benign and surface all other errors. Specifically, after calling
o.KubeClient.CoreV1().Secrets(o.Namespace).Get(...), use
apierrors.IsNotFound(err) to detect a missing secret (log and return nil) and
for any other error log the failure with context (including err) and return the
error (or wrap it) instead of swallowing it; add the import for
k8s.io/apimachinery/pkg/api/errors as apierrors and update the klog call
locations around secretName and o.Namespace accordingly.
Comment on lines
+71
to
+91
| func extractUniqueKMSEndpoints(config *apiserverconfigv1.EncryptionConfiguration) map[string]string { | ||
| endpoints := make(map[string]string) | ||
| seenEndpoints := make(map[string]bool) | ||
|
|
||
| for _, rc := range config.Resources { | ||
| for _, provider := range rc.Providers { | ||
| if provider.KMS == nil { | ||
| continue | ||
| } | ||
| endpoint := provider.KMS.Endpoint | ||
| if seenEndpoints[endpoint] { | ||
| continue | ||
| } | ||
| seenEndpoints[endpoint] = true | ||
|
|
||
| keyID := extractKeyIDFromProviderName(provider.KMS.Name) | ||
| endpoints[keyID] = endpoint | ||
| } | ||
| } | ||
| return endpoints | ||
| } |
There was a problem hiding this comment.
Potential keyID collision overwrites earlier endpoints.
If two KMS providers have the same keyID prefix (e.g., 1_secrets and 1_configmaps both yield keyID 1), the endpoints[keyID] = endpoint assignment on line 87 will overwrite the previous entry. While same-keyID providers likely share an endpoint, this assumption isn't validated and could silently lose configuration.
Consider either:
- Validating that same-keyID providers have the same endpoint
- Using endpoint as the primary key instead of keyID
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-encryption-kms/encryption_kms_test.go (1)
59-78: Prefert.Skip(...)over block-commenting the test.The test
TestKMSEncryptionProvidersMigrationis block-commented on line 59, which prevents compile-time type checking and makes the test invisible to test discovery tools. Instead, uncomment the function and uset.Skip(...)with a reason to keep the test compiled while explicitly disabling it.Suggested change
-/*func TestKMSEncryptionProvidersMigration(t *testing.T) { +func TestKMSEncryptionProvidersMigration(t *testing.T) { + t.Skip("temporarily disabled for plugin lifecycle PoC; re-enable after library-go plugin lifecycle follow-up") librarykms.DeployUpstreamMockKMSPlugin(context.Background(), t, library.GetClients(t).Kube, librarykms.WellKnownUpstreamMockKMSPluginNamespace, librarykms.WellKnownUpstreamMockKMSPluginImage) library.TestEncryptionProvidersMigration(t, library.ProvidersMigrationScenario{ BasicScenario: library.BasicScenario{ Namespace: operatorclient.GlobalMachineSpecifiedConfigNamespace, LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + operatorclient.TargetNamespace, EncryptionConfigSecretName: fmt.Sprintf("encryption-config-%s", operatorclient.TargetNamespace), EncryptionConfigSecretNamespace: operatorclient.GlobalMachineSpecifiedConfigNamespace, OperatorNamespace: operatorclient.OperatorNamespace, TargetGRs: operatorencryption.DefaultTargetGRs, AssertFunc: operatorencryption.AssertSecretsAndConfigMaps, }, CreateResourceFunc: operatorencryption.CreateAndStoreSecretOfLife, AssertResourceEncryptedFunc: operatorencryption.AssertSecretOfLifeEncrypted, AssertResourceNotEncryptedFunc: operatorencryption.AssertSecretOfLifeNotEncrypted, ResourceFunc: operatorencryption.SecretOfLife, ResourceName: "SecretOfLife", EncryptionProviders: library.ShuffleEncryptionProviders([]configv1.EncryptionType{configv1.EncryptionTypeKMS, library.SupportedStaticEncryptionProviders[rand.IntN(len(library.SupportedStaticEncryptionProviders))]}), }) -}*/ +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-kms/encryption_kms_test.go` around lines 59 - 78, Unblock the commented-out test function TestKMSEncryptionProvidersMigration by restoring its func declaration and body, and at the start of the test call t.Skip("reason") with a short reason string so the test remains compiled and visible to test discovery while disabled; locate the TestKMSEncryptionProvidersMigration function block in encryption_kms_test.go and replace the block comment with the original function body beginning with t.Skip(...) before any test setup code (preserve the existing calls such as librarykms.DeployUpstreamMockKMSPlugin and library.TestEncryptionProvidersMigration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e-encryption-kms/encryption_kms_test.go`:
- Around line 59-78: Unblock the commented-out test function
TestKMSEncryptionProvidersMigration by restoring its func declaration and body,
and at the start of the test call t.Skip("reason") with a short reason string so
the test remains compiled and visible to test discovery while disabled; locate
the TestKMSEncryptionProvidersMigration function block in encryption_kms_test.go
and replace the block comment with the original function body beginning with
t.Skip(...) before any test setup code (preserve the existing calls such as
librarykms.DeployUpstreamMockKMSPlugin and
library.TestEncryptionProvidersMigration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a3a6837c-38d6-4d3c-a9ad-fa6e4de62a64
📒 Files selected for processing (1)
test/e2e-encryption-kms/encryption_kms_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (1)
312-312:operatorStatusparameter is unused inmanagePods.The
operatorStatusparameter is threaded through fromsynctocreateTargetConfigtomanagePods, but it's not used within themanagePodsfunction body. If this is preparation for future functionality, consider adding a brief TODO comment. Otherwise, remove the unused parameter.🤖 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 operatorStatus parameter in managePods is unused; either remove it from the managePods signature and all call sites (e.g., callers sync and createTargetConfig) or, if reserved for future use, add a single-line TODO comment above the managePods declaration noting why it remains (e.g., "TODO: reserve operatorStatus for future status updates"). Update the function signature references to match (remove operatorStatus from managePods(...) and its callers, or leave parameter and add the TODO) and run the compiler to fix any resulting call-site signature mismatches.
🤖 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 57-65: Update the error handling in kms_sidecar.go where
encryptionConfig is fetched: correct the log text to reference the actual secret
name "openshift-config-managed/encryption-config-openshift-kube-apiserver"
(instead of "openshift-config/encryption-config") and stop swallowing
non-NotFound errors; for apierrors.IsNotFound(err) keep an Info log and return
nil, but for other errors log at Warning/Error using klog (including the error)
and return the error (do not return nil) so transient failures surface to the
caller; use the existing encryptionConfig variable and
secretLister.Secrets(...).Get(...) call context when making these changes.
---
Nitpick comments:
In `@pkg/operator/targetconfigcontroller/targetconfigcontroller.go`:
- Line 312: The operatorStatus parameter in managePods is unused; either remove
it from the managePods signature and all call sites (e.g., callers sync and
createTargetConfig) or, if reserved for future use, add a single-line TODO
comment above the managePods declaration noting why it remains (e.g., "TODO:
reserve operatorStatus for future status updates"). Update the function
signature references to match (remove operatorStatus from managePods(...) and
its callers, or leave parameter and add the TODO) and run the compiler to fix
any resulting call-site signature mismatches.
🪄 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: 8dd8f040-4971-4173-9022-98e5929a41c3
📒 Files selected for processing (3)
pkg/operator/starter.gopkg/operator/targetconfigcontroller/kms_sidecar.gopkg/operator/targetconfigcontroller/targetconfigcontroller.go
Comment on lines
+57
to
+65
| 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/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 | ||
| } |
There was a problem hiding this comment.
Inconsistent log message and partial error swallowing.
-
Line 59 log message refers to
openshift-config/encryption-configbut the code actually fetches fromopenshift-config-managed/encryption-config-openshift-kube-apiserver. -
Lines 62-65: Non-NotFound errors are logged at Info level and return
nil, which silently masks transient failures (network issues, permission errors). Consider returning the error or distinguishing between expected and unexpected failures.
Proposed 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/encryption-config not found: %v", err)
+ klog.Infof("kms is disabled: secret openshift-config-managed/encryption-config-openshift-kube-apiserver not found")
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 57 - 65,
Update the error handling in kms_sidecar.go where encryptionConfig is fetched:
correct the log text to reference the actual secret name
"openshift-config-managed/encryption-config-openshift-kube-apiserver" (instead
of "openshift-config/encryption-config") and stop swallowing non-NotFound
errors; for apierrors.IsNotFound(err) keep an Info log and return nil, but for
other errors log at Warning/Error using klog (including the error) and return
the error (do not return nil) so transient failures surface to the caller; use
the existing encryptionConfig variable and secretLister.Secrets(...).Get(...)
call context when making these changes.
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Contributor
|
@ardaguclu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Member
Author
|
/close |
Contributor
|
@ardaguclu: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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.
Trying to test openshift/library-go#2161 with adapted version of #2056 on KMS On/Off scenario.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests