WIP: kms: reconcile sidecar containers and volumes to match encryption config - #2334
WIP: kms: reconcile sidecar containers and volumes to match encryption config#2334bertinatto wants to merge 3 commits into
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 KMS plugin lifecycle now removes managed static-pod resources before rebuilding the sidecar, supports alternate disk Secret naming, and uses the shared health-reporter container name constant. Tests cover stale-provider cleanup and complete KMS removal. ChangesKMS static-pod reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EnsureKMSPluginSidecarInStaticPodSpec
participant PodSpec
participant KMSPluginBuilder
EnsureKMSPluginSidecarInStaticPodSpec->>PodSpec: remove KMS-managed resources
EnsureKMSPluginSidecarInStaticPodSpec->>KMSPluginBuilder: configure disk Secret and sidecar
KMSPluginBuilder->>PodSpec: apply desired containers, volumes, and mounts
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
c573895 to
049d9ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go (1)
614-827: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSolid coverage for pruning behavior; consider adding a disabled-feature-gate subtest.
The two scenarios (stale provider pruned, full removal) validate the filtering logic well. Given the doc/behavior mismatch flagged in
sidecar.go(removal happening even when the feature gate is disabled), a subtest asserting the podSpec is left untouched whenFeatureGateKMSEncryptionis disabled would catch regressions here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go` around lines 614 - 827, Add a new subtest in TestEnsureKMSPluginSidecarInStaticPodSpec that covers FeatureGateKMSEncryption being disabled, using EnsureKMSPluginSidecarInStaticPodSpec with a hardcoded feature gate access that omits the gate. The test should verify the input PodSpec is left unchanged, unlike the current pruning cases, so the behavior around disabled gates is covered and regression-proof.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/sidecar.go`:
- Around line 83-93: Ensure EnsureKMSPluginSidecarInStaticPodSpec does not
mutate podSpec before confirming the KMS sidecar should be reconciled. Move or
duplicate the same feature-gate and encryption-config existence checks used by
AddKMSPluginSidecarToStaticPodSpec so removeAllKMSManagedResources only runs
when those preconditions pass, and avoid leaving podSpec partially stripped on
any error path. If needed, extract the shared guard logic into a helper used by
both EnsureKMSPluginSidecarInStaticPodSpec and
AddKMSPluginSidecarToStaticPodSpec to keep the behavior consistent.
---
Nitpick comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go`:
- Around line 614-827: Add a new subtest in
TestEnsureKMSPluginSidecarInStaticPodSpec that covers FeatureGateKMSEncryption
being disabled, using EnsureKMSPluginSidecarInStaticPodSpec with a hardcoded
feature gate access that omits the gate. The test should verify the input
PodSpec is left unchanged, unlike the current pruning cases, so the behavior
around disabled gates is covered and regression-proof.
🪄 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: Enterprise
Run ID: bf748370-6b95-4e25-9e43-dfd8d893e261
📒 Files selected for processing (3)
pkg/operator/encryption/kms/pluginlifecycle/builder.gopkg/operator/encryption/kms/pluginlifecycle/sidecar.gopkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/encryption/kms/pluginlifecycle/builder.go
| // EnsureKMSPluginSidecarInStaticPodSpec reconciles KMS plugin sidecar containers in a kube-apiserver static pod spec. | ||
| // It removes all KMS-managed resources (sidecars, volumes, volume mounts) and then re-adds exactly what the | ||
| // current encryption config requires, ensuring stale resources from a previous configuration are pruned. | ||
| // | ||
| // It is a no-op when the KMSEncryption feature gate is not enabled or the encryption-config secret does not exist. | ||
| // The secretClient should be uncached to avoid injecting sidecars based on a stale encryption configuration. | ||
| func EnsureKMSPluginSidecarInStaticPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, operatorBinary string, operatorImage string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess) error { | ||
| removeAllKMSManagedResources(podSpec, containerName) | ||
| return AddKMSPluginSidecarToStaticPodSpec(ctx, podSpec, containerName, encryptionConfigNamespace, encryptionConfigSecretName, operatorBinary, operatorImage, secretClient, featureGateAccessor) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Doc comment contradicts actual behavior; destructive removal happens even on no-op/error paths.
The comment says this is "a no-op when the KMSEncryption feature gate is not enabled or the encryption-config secret does not exist," but removeAllKMSManagedResources (Line 90) runs unconditionally before AddKMSPluginSidecarToStaticPodSpec checks the feature gate (Lines 57-66) or config presence (Lines 72-74). So when those "no-op" conditions are true, the function still strips all KMS-managed init containers, volumes, and socket mounts from the podSpec — it's not a no-op at all.
Worse, if AddKMSPluginSidecarToStaticPodSpec returns an error (feature-gate fetch failure Line 61-63, or fetchEncryptionConfig error Line 69-71), the podSpec has already been destructively mutated by the time the error propagates to the caller, leaving it in a half-cleaned state that may still get applied.
🩹 Possible fix: gate the removal on the same preconditions
func EnsureKMSPluginSidecarInStaticPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, operatorBinary string, operatorImage string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess) error {
- removeAllKMSManagedResources(podSpec, containerName)
- return AddKMSPluginSidecarToStaticPodSpec(ctx, podSpec, containerName, encryptionConfigNamespace, encryptionConfigSecretName, operatorBinary, operatorImage, secretClient, featureGateAccessor)
+ if !featureGateAccessor.AreInitialFeatureGatesObserved() {
+ return nil
+ }
+ featureGates, err := featureGateAccessor.CurrentFeatureGates()
+ if err != nil {
+ return fmt.Errorf("failed to get feature gates: %w", err)
+ }
+ if !featureGates.Enabled(features.FeatureGateKMSEncryption) {
+ return nil
+ }
+
+ removeAllKMSManagedResources(podSpec, containerName)
+ return AddKMSPluginSidecarToStaticPodSpec(ctx, podSpec, containerName, encryptionConfigNamespace, encryptionConfigSecretName, operatorBinary, operatorImage, secretClient, featureGateAccessor)
}This duplicates the guard rather than restructuring AddKMSPluginSidecarToStaticPodSpec; consider extracting the guard checks into a shared helper instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/encryption/kms/pluginlifecycle/sidecar.go` around lines 83 - 93,
Ensure EnsureKMSPluginSidecarInStaticPodSpec does not mutate podSpec before
confirming the KMS sidecar should be reconciled. Move or duplicate the same
feature-gate and encryption-config existence checks used by
AddKMSPluginSidecarToStaticPodSpec so removeAllKMSManagedResources only runs
when those preconditions pass, and avoid leaving podSpec partially stripped on
any error path. If needed, extract the shared guard logic into a helper used by
both EnsureKMSPluginSidecarInStaticPodSpec and
AddKMSPluginSidecarToStaticPodSpec to keep the behavior consistent.
8a69ee8 to
4210bf5
Compare
The existing Add* functions use upsert semantics, which leaves orphaned KMS sidecar containers, health reporters, and volumes when a provider is removed from the encryption configuration. Add EnsureKMSPluginSidecarInStaticPodSpec, which removes all KMS-managed resources before re-adding exactly what the current config requires.
…ret and mount point
4210bf5 to
fed6abe
Compare
507ef12 to
91ba23e
Compare
|
@bertinatto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests