WIP: compute encryption-config secret (isolated namespace approach) - #2399
WIP: compute encryption-config secret (isolated namespace approach)#2399bertinatto wants to merge 4 commits into
Conversation
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughChangesKMS preflight encryption configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant KMSPreflightController
participant TemporaryNamespace
participant KeyController
participant StateController
participant EncryptionDeployer
KMSPreflightController->>TemporaryNamespace: create labeled namespace
KMSPreflightController->>KeyController: reconcile keys without status writes
KeyController->>TemporaryNamespace: seed and create key Secrets
KMSPreflightController->>StateController: generate encryption configuration
StateController->>EncryptionDeployer: retrieve deployed encryption Secret
StateController->>TemporaryNamespace: store generated encryption-config Secret
KMSPreflightController->>TemporaryNamespace: rewrite selected KMS endpoint
KMSPreflightController->>EncryptionDeployer: deploy generated encryption Secret
KMSPreflightController->>TemporaryNamespace: delete temporary namespace
🚥 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
78c1fd4 to
f5aa865
Compare
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/operator/encryption/controllers/kms_preflight_sandbox.go (3)
313-314: 📐 Maintainability & Code Quality | 🔵 TrivialTODO tracked: remove the endpoint rewrite once preflight dials the per-key socket.
The rationale is documented, so no change is needed now. Do you want me to open an issue to track the removal of
rewritePreflightWriteKeyEndpoint?🤖 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/controllers/kms_preflight_sandbox.go` around lines 313 - 314, Keep the existing TODO documenting removal of rewritePreflightWriteKeyEndpoint; no code changes are required for this review comment.
133-207: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAvoid constructing
keyControllerandstateControllerfield by field.
runKeyControllerInNamespaceandrunStateControllerInNamespacebuild the two controller structs directly instead of usingNewKeyControllerandNewStateController. Every required field must be repeated here.When someone adds a field to either struct and sets it only in the public constructor, this file still compiles. The field stays nil, and the preflight run fails at runtime inside the controller. The compiler gives no warning.
Extract the struct assembly used by
NewKeyControllerandNewStateControllerinto an internal helper, then call that helper from both the public constructor and this file.🤖 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/controllers/kms_preflight_sandbox.go` around lines 133 - 207, Extract the shared keyController and stateController initialization from NewKeyController and NewStateController into internal helpers, then update runKeyControllerInNamespace and runStateControllerInNamespace to use those helpers while preserving their preflight-specific fields and options. Ensure both public constructors and these namespace runners use the same assembly path so newly added constructor fields cannot be omitted.
243-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
errors.Joinfor the aggregated cleanup errors.Lines 249-251 flatten the collected errors with
%v, which drops wrapping and preventserrors.Isanderrors.Ason the result.PodPreflightDeployer.Cleanupinpkg/operator/encryption/kms/preflight/deployer.goalready useserrors.Joinfor the same pattern.♻️ Proposed change
- if len(errs) > 0 { - return fmt.Errorf("failed to delete some preflight temp namespaces: %v", errs) - } - return nil + return errors.Join(errs...)Add the
errorsimport.🤖 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/controllers/kms_preflight_sandbox.go` around lines 243 - 252, Update the cleanup aggregation in the preflight namespace deletion flow to import and use errors.Join(errs) when returning collected failures, preserving error wrapping so errors.Is and errors.As continue to work. Keep the existing success path and contextual failure message in the surrounding cleanup function unchanged.pkg/operator/encryption/secrets/secrets.go (1)
134-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing the namespace into
FromKeyState.
FromKeyStatealways setsEncryptionKeysNamespace. The key controller then overwrites the field withkeysNamespaceinpkg/operator/encryption/controllers/key_controller.goat Line 281. The namespace decision is therefore split across two files. A namespace parameter (or aFromKeyStateInNamespacevariant, matching theListKeySecretsInNamespacepattern in this file) would keep the choice in one place and prevent a future caller from forgetting the override.🤖 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/secrets/secrets.go` at line 134, Update FromKeyState to accept a namespace parameter, or add a FromKeyStateInNamespace variant matching ListKeySecretsInNamespace, and use it from the key controller with keysNamespace so namespace selection is centralized and the controller no longer overwrites the returned field.pkg/operator/encryption/controllers/key_controller_test.go (1)
889-894: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not discard the encode error.
Line 891 ignores the error from
encoding.EncodeKMSPluginConfig. If the call fails,expectedDatais empty and the comparison can pass or fail for the wrong reason.♻️ Proposed change
- expectedData, _ := encoding.EncodeKMSPluginConfig(*updated) + expectedData, err := encoding.EncodeKMSPluginConfig(*updated) + if err != nil { + ts.Fatalf("failed to encode expected KMS plugin config: %v", err) + }As per path instructions: "Never ignore error returns".
🤖 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/controllers/key_controller_test.go` around lines 889 - 894, Handle the error returned by encoding.EncodeKMSPluginConfig in the test around updated and expectedData: assert or report the error and stop the test path before comparing pluginData. Do not discard the encode error, while preserving the existing comparison of successfully encoded expectedData.Source: Path instructions
pkg/operator/encryption/controllers/kms_preflight_controller_test.go (1)
1025-1032: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the helper and the exported data-key constant.
Lines 1026-1031 repeat the first two checks of
assertDeployedPreflightEncryptionConfigat Lines 1164-1169. Both places also hardcode the data key"encryption-config", whileencryptiondata.EncryptionConfSecretNamenames that key.pkg/operator/encryption/controllers/state_controller_test.goLine 698 already indexes Secret data with that constant.♻️ Proposed change
if fakeDeployerInstance.deployed { - if fakeDeployerInstance.lastEncryptionConfigSecret == nil { - t.Fatalf("expected Deploy to receive a non-nil encryption config secret") - } - if fakeDeployerInstance.lastEncryptionConfigSecret.Data["encryption-config"] == nil { - t.Fatalf("expected encryption config secret to contain encryption-config data") - } + assertDeployedPreflightEncryptionConfig(t, fakeDeployerInstance.lastEncryptionConfigSecret) }And in the helper:
- if secret.Data["encryption-config"] == nil { + if secret.Data[encryptiondata.EncryptionConfSecretName] == nil { t.Fatal("expected encryption config secret to contain encryption-config data") }🤖 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/controllers/kms_preflight_controller_test.go` around lines 1025 - 1032, Update the deployed-secret assertions in the test to reuse assertDeployedPreflightEncryptionConfig instead of duplicating its nil and data checks, and replace the hardcoded "encryption-config" key within that helper with encryptiondata.EncryptionConfSecretName. Preserve the existing validation behavior while using the exported constant for Secret.Data access.pkg/operator/encryption/controllers/key_controller.go (2)
371-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not hardcode the plugin-config data key.
Line 374 repeats the literal
"encryption.apiserver.operator.openshift.io-kms-plugin-config". Thesecretspackage owns this key through its unexportedencryptionSecretKMSPluginConfigconstant, andsecrets.ToKeyStatereads the value through that constant.If the key ever changes in the
secretspackage, this write targets a stale key.ToKeyStatethen keeps decoding the old value, and the in-place update becomes a silent no-op instead of a build failure.Export the constant from the
secretspackage and use it here.♻️ Proposed change
In
pkg/operator/encryption/secrets/types.go, export the key:// EncryptionSecretKMSPluginConfig is the Secret data key holding the KMS plugin config. EncryptionSecretKMSPluginConfig = "encryption.apiserver.operator.openshift.io-kms-plugin-config"Then apply this diff:
- s.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"] = pluginData + s.Data[secrets.EncryptionSecretKMSPluginConfig] = pluginData🤖 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/controllers/key_controller.go` around lines 371 - 375, Export the existing encryptionSecretKMSPluginConfig constant from the secrets package as EncryptionSecretKMSPluginConfig, preserving its value and documenting it as the Secret data key for the KMS plugin configuration. In the key controller’s Secret update flow, replace the hardcoded data-key literal with secrets.EncryptionSecretKMSPluginConfig so writes stay aligned with secrets.ToKeyState.
394-399: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable type comparison.
Line 394 returns an error unless both
latest.Typeandcurrent.Typeequalconfigv1.VaultKMSProvider. Line 397 then compares the two types. After Line 394 both values areVaultKMSProvider, so Line 397 can never be true.Also, the error message on Line 395 reports only
latest.Type. Whencurrent.Typeholds the invalid value, the message names the wrong field.♻️ Proposed change
if latest.Type != configv1.VaultKMSProvider || current.Type != configv1.VaultKMSProvider { - return false, fmt.Errorf("KMS plugin config has an invalid type: %q", latest.Type) + return false, fmt.Errorf("KMS plugin config has an invalid type: latest=%q, current=%q", latest.Type, current.Type) } - if latest.Type != current.Type { - return true, nil - } if latest.Vault.VaultAddress != current.Vault.VaultAddress ||🤖 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/controllers/key_controller.go` around lines 394 - 399, In the type validation logic surrounding the latest/current KMS configuration comparison, remove the unreachable latest.Type != current.Type branch after validating both types are VaultKMSProvider. Update the invalid-type error in the enclosing comparison to report the actual invalid value, including current.Type when latest.Type is valid but current.Type is not.
🤖 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/controllers/key_controller.go`:
- Around line 317-383: Update kmsMigrationRequired and the in-place update flow
around maybeUpdateKMSPluginConfigInPlace so changes to
Vault.Authentication.AppRole.Secret.Name or Vault.TLS.CABundle.Name require key
migration instead of an in-place update. Preserve in-place updates for fields
that do not alter referenced resource names, and extend the existing
Authentication and TLS tests with assertions covering carried Secret and
ConfigMap data.
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox.go`:
- Around line 65-69: Update the deferred cleanup around
deletePreflightTempNamespace to use a separate cleanup context derived without
cancellation or deadline propagation from the sync ctx. Pass this cleanup
context to deletePreflightTempNamespace so namespace deletion still runs after
the caller’s ctx is cancelled, while preserving the existing warning behavior.
- Around line 255-280: Update seedEncryptionKeySecrets at
pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L255-L280 to omit
existingSecret.Finalizers when constructing temporary Secret copies. Update
ensurePheightTempNamespace at
pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L209-L223 to
inspect an existing namespace’s Status.Phase and return a distinct
terminating-namespace error when it is NamespaceTerminating, rather than
treating IsAlreadyExists as success.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 889-894: Handle the error returned by
encoding.EncodeKMSPluginConfig in the test around updated and expectedData:
assert or report the error and stop the test path before comparing pluginData.
Do not discard the encode error, while preserving the existing comparison of
successfully encoded expectedData.
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 371-375: Export the existing encryptionSecretKMSPluginConfig
constant from the secrets package as EncryptionSecretKMSPluginConfig, preserving
its value and documenting it as the Secret data key for the KMS plugin
configuration. In the key controller’s Secret update flow, replace the hardcoded
data-key literal with secrets.EncryptionSecretKMSPluginConfig so writes stay
aligned with secrets.ToKeyState.
- Around line 394-399: In the type validation logic surrounding the
latest/current KMS configuration comparison, remove the unreachable latest.Type
!= current.Type branch after validating both types are VaultKMSProvider. Update
the invalid-type error in the enclosing comparison to report the actual invalid
value, including current.Type when latest.Type is valid but current.Type is not.
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 1025-1032: Update the deployed-secret assertions in the test to
reuse assertDeployedPreflightEncryptionConfig instead of duplicating its nil and
data checks, and replace the hardcoded "encryption-config" key within that
helper with encryptiondata.EncryptionConfSecretName. Preserve the existing
validation behavior while using the exported constant for Secret.Data access.
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox.go`:
- Around line 313-314: Keep the existing TODO documenting removal of
rewritePreflightWriteKeyEndpoint; no code changes are required for this review
comment.
- Around line 133-207: Extract the shared keyController and stateController
initialization from NewKeyController and NewStateController into internal
helpers, then update runKeyControllerInNamespace and
runStateControllerInNamespace to use those helpers while preserving their
preflight-specific fields and options. Ensure both public constructors and these
namespace runners use the same assembly path so newly added constructor fields
cannot be omitted.
- Around line 243-252: Update the cleanup aggregation in the preflight namespace
deletion flow to import and use errors.Join(errs) when returning collected
failures, preserving error wrapping so errors.Is and errors.As continue to work.
Keep the existing success path and contextual failure message in the surrounding
cleanup function unchanged.
In `@pkg/operator/encryption/secrets/secrets.go`:
- Line 134: Update FromKeyState to accept a namespace parameter, or add a
FromKeyStateInNamespace variant matching ListKeySecretsInNamespace, and use it
from the key controller with keysNamespace so namespace selection is centralized
and the controller no longer overwrites the returned field.
🪄 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: 4396964d-69ec-4783-a627-691bdef4710e
📒 Files selected for processing (11)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/controllers/kms_preflight_sandbox.gopkg/operator/encryption/controllers/kms_preflight_sandbox_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/statemachine/transition.go
| func (c *keyController) maybeUpdateKMSPluginConfigInPlace(ctx context.Context, syncContext factory.SyncContext, apiServerEncryption configv1.APIServerEncryption, keysNamespace string) error { | ||
| keySecrets, err := secrets.ListKeySecretsInNamespace(ctx, c.secretClient, keysNamespace, c.encryptionSecretSelector) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(keySecrets) == 0 { | ||
| return nil | ||
| } | ||
| // Sort by key ID descending so [0] is the newest. | ||
| // Parse the newest secret directly — fail fast if it is malformed | ||
| // instead of silently falling back to an older key. | ||
| sort.Slice(keySecrets, func(i, j int) bool { | ||
| iKeyID, _ := state.NameToKeyID(keySecrets[i].Name) | ||
| jKeyID, _ := state.NameToKeyID(keySecrets[j].Name) | ||
| return iKeyID > jKeyID | ||
| }) | ||
| // We only focus on the latest backed key. | ||
| latest, err := secrets.ToKeyState(keySecrets[0]) | ||
| if err != nil { | ||
| return fmt.Errorf("latest key secret %s is invalid: %w", keySecrets[0].Name, err) | ||
| } | ||
|
|
||
| // Any mode mismatch (e.g. KMS <-> AESCBC) requires a migration, not an in-place | ||
| // update. The normal needsNewKey path handles this after convergence. | ||
| if latest.Mode != state.KMS { | ||
| return nil | ||
| } | ||
| // This should never happen under normal operation because ToKeyState enforces | ||
| // that KMS mode keys have a plugin config. This can only occur if someone | ||
| // manually edited the key secret and removed the kms-plugin-config data field. | ||
| if !latest.HasKMSPlugin() { | ||
| return fmt.Errorf("latest KMS key %s is missing plugin config", latest.Key.Name) | ||
| } | ||
| if equality.Semantic.DeepEqual(latest.KMS.Plugin, apiServerEncryption.KMS) { | ||
| return nil | ||
| } | ||
|
|
||
| migrationRequired, err := kmsMigrationRequired(latest.KMS.Plugin, apiServerEncryption.KMS) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // migration-triggering fields changed (needs a new key, not an in-place update). | ||
| if migrationRequired { | ||
| return nil | ||
| } | ||
|
|
||
| s, err := c.secretClient.Secrets(keysNamespace).Get(ctx, keySecrets[0].Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get key secret %s/%s: %v", keysNamespace, keySecrets[0].Name, err) | ||
| } | ||
| pluginData, err := encoding.EncodeKMSPluginConfig(apiServerEncryption.KMS) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to encode KMS plugin config: %v", err) | ||
| } | ||
| if s.Data == nil { | ||
| s.Data = map[string][]byte{} | ||
| } | ||
| s.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"] = pluginData | ||
| _, updateErr := c.secretClient.Secrets(keysNamespace).Update(ctx, s, metav1.UpdateOptions{}) | ||
| if errors.IsConflict(updateErr) { | ||
| return nil | ||
| } | ||
| if updateErr == nil { | ||
| syncContext.Recorder().Eventf("EncryptionKeyKMSPluginConfigUpdated", "Updated KMS plugin config on key secret %q in-place", s.Name) | ||
| } | ||
| return updateErr | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The in-place update does not refresh carried Secret and ConfigMap data.
maybeUpdateKMSPluginConfigInPlace writes only the kms-plugin-config data field. kmsMigrationRequired classifies changes to Vault.Authentication and Vault.TLS as in-place-safe. Those fields carry resource references (Authentication.AppRole.Secret.Name, TLS.CABundle.Name).
When an administrator points Authentication.AppRole.Secret at a different Secret, this code updates the plugin config on the key Secret but keeps the old encryption.apiserver.operator.openshift.io-kms-plugin-secret-<old-name>_* entries. generateKeySecret populates those entries only when it mints a new key. Result: the key Secret, and therefore the encryption-config Secret built by the state controller, declares the new reference name while carrying credential data copied from the old Secret. The stale entries are also never removed. The KMS plugin sidecar then receives wrong or missing credentials.
The same gap applies to TLS.CABundle and the ...-kms-plugin-configmap-* entries.
Either re-resolve and replace the referenced Secret and ConfigMap payloads in this function (reusing the referencedSecretName and referencedConfigMapName logic from generateKeySecret), or exclude reference-name changes from the in-place-safe class in kmsMigrationRequired.
The existing tests "in-place update when only Authentication changes (non-migration field)" and "in-place update when only TLS changes (non-migration field)" in pkg/operator/encryption/controllers/key_controller_test.go assert only the action verbs, so they do not cover this case. Add data assertions for the carried entries.
🤖 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/controllers/key_controller.go` around lines 317 -
383, Update kmsMigrationRequired and the in-place update flow around
maybeUpdateKMSPluginConfigInPlace so changes to
Vault.Authentication.AppRole.Secret.Name or Vault.TLS.CABundle.Name require key
migration instead of an in-place update. Preserve in-place updates for fields
that do not alter referenced resource names, and extend the existing
Authentication and TLS tests with assertions covering carried Secret and
ConfigMap data.
| defer func() { | ||
| if cleanupErr := deletePreflightTempNamespace(ctx, coreClient, tempNamespace); cleanupErr != nil { | ||
| klog.Warningf("failed to delete preflight temp namespace %q: %v", tempNamespace, cleanupErr) | ||
| } | ||
| }() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use a cleanup context that survives cancellation of ctx.
The deferred cleanup calls deletePreflightTempNamespace with the same ctx that the sync uses. When ctx is cancelled or its deadline expires, the delete call fails immediately and the code only logs a warning.
The temp namespace holds copies of the encryption key Secrets. For AESCBC and AESGCM keys those copies contain the raw key material in EncryptionSecretKeyDataKey; for KMS keys they contain the carried plugin credential payloads. A failed cleanup therefore leaves cluster encryption key material in an extra namespace.
cleanupPreflightTempNamespaces does not recover this case: it runs only when preflight is no longer required. While the same configHash stays required, the leaked namespace persists.
Derive a separate context for cleanup so it is not bound to the caller's cancellation.
🔒️ Proposed fix
defer func() {
- if cleanupErr := deletePreflightTempNamespace(ctx, coreClient, tempNamespace); cleanupErr != nil {
+ // Use a context detached from ctx so cleanup still runs after the sync
+ // context is cancelled. The namespace holds copies of encryption key material.
+ cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)
+ defer cancel()
+ if cleanupErr := deletePreflightTempNamespace(cleanupCtx, coreClient, tempNamespace); cleanupErr != nil {
klog.Warningf("failed to delete preflight temp namespace %q: %v", tempNamespace, cleanupErr)
}
}()📝 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.
| defer func() { | |
| if cleanupErr := deletePreflightTempNamespace(ctx, coreClient, tempNamespace); cleanupErr != nil { | |
| klog.Warningf("failed to delete preflight temp namespace %q: %v", tempNamespace, cleanupErr) | |
| } | |
| }() | |
| defer func() { | |
| // Use a context detached from ctx so cleanup still runs after the sync | |
| // context is cancelled. The namespace holds copies of encryption key material. | |
| cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) | |
| defer cancel() | |
| if cleanupErr := deletePreflightTempNamespace(cleanupCtx, coreClient, tempNamespace); cleanupErr != nil { | |
| klog.Warningf("failed to delete preflight temp namespace %q: %v", tempNamespace, cleanupErr) | |
| } | |
| }() |
🤖 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/controllers/kms_preflight_sandbox.go` around lines 65
- 69, Update the deferred cleanup around deletePreflightTempNamespace to use a
separate cleanup context derived without cancellation or deadline propagation
from the sync ctx. Pass this cleanup context to deletePreflightTempNamespace so
namespace deletion still runs after the caller’s ctx is cancelled, while
preserving the existing warning behavior.
| // seedEncryptionKeySecrets copies existing encryption key secrets from sourceNamespace into | ||
| // destNamespace so the key/state controllers can build on the live key set without mutating it. | ||
| func seedEncryptionKeySecrets(ctx context.Context, coreClient corev1client.CoreV1Interface, sourceNamespace, destNamespace string, encryptionSecretSelector metav1.ListOptions) error { | ||
| existing, err := secrets.ListKeySecretsInNamespace(ctx, coreClient, sourceNamespace, encryptionSecretSelector) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list encryption key secrets in %s: %w", sourceNamespace, err) | ||
| } | ||
| for _, existingSecret := range existing { | ||
| copySecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: existingSecret.Name, | ||
| Namespace: destNamespace, | ||
| Labels: existingSecret.Labels, | ||
| Annotations: existingSecret.Annotations, | ||
| Finalizers: existingSecret.Finalizers, | ||
| }, | ||
| Type: existingSecret.Type, | ||
| Data: existingSecret.Data, | ||
| } | ||
| _, err := coreClient.Secrets(destNamespace).Create(ctx, copySecret, metav1.CreateOptions{}) | ||
| if err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return fmt.Errorf("failed to seed encryption key secret %s/%s: %w", destNamespace, copySecret.Name, err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The temp namespace can never finish terminating, because the seeded Secrets carry the deletion-protection finalizer.
secrets.FromKeyState sets Finalizers: []string{EncryptionSecretFinalizer} on every encryption key Secret, so every live key Secret carries encryption.apiserver.operator.openshift.io/deletion-protection. seedEncryptionKeySecrets copies existingSecret.Finalizers verbatim into the temp namespace.
Namespace deletion cannot complete while a contained Secret has a finalizer. Nothing removes the finalizer from these copies: the key deletion and pruning controllers act on secrets.EncryptionKeysNamespace, not on the temp namespace. The temp namespace therefore stays in Terminating forever and keeps the copied encryption key material.
The second site turns this into a repeating failure. ensurePheightTempNamespace treats IsAlreadyExists as success, and the API server returns AlreadyExists for a namespace in Terminating. The following seedEncryptionKeySecrets call then fails because the namespace is being terminated, and the controller reports a confusing degraded message on every retry with the same configHash.
The unit tests do not catch this. The fake clientset enforces neither Secret finalizers nor namespace termination, so TestComputeEncryptionConfigSecretInTempNamespace_FirstKMSKey observes the temp namespace as fully deleted.
pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L255-L280: do not copyFinalizersinto the temp namespace copy. The copies are throwaway and need no deletion protection.pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L209-L223: if the namespace exists, verify that itsStatus.Phaseis notNamespaceTerminatingbefore seeding. If it is terminating, return a distinct error so the caller requeues with a clear reason instead of reporting a seeding failure.
🐛 Proposed fix for the seeded Secrets
for _, existingSecret := range existing {
copySecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: existingSecret.Name,
Namespace: destNamespace,
Labels: existingSecret.Labels,
Annotations: existingSecret.Annotations,
- Finalizers: existingSecret.Finalizers,
},
Type: existingSecret.Type,
Data: existingSecret.Data,
}📍 Affects 1 file
pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L255-L280(this comment)pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L209-L223
🤖 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/controllers/kms_preflight_sandbox.go` around lines
255 - 280, Update seedEncryptionKeySecrets at
pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L255-L280 to omit
existingSecret.Finalizers when constructing temporary Secret copies. Update
ensurePheightTempNamespace at
pkg/operator/encryption/controllers/kms_preflight_sandbox.go#L209-L223 to
inspect an existing namespace’s Status.Phase and return a distinct
terminating-namespace error when it is NamespaceTerminating, rather than
treating IsAlreadyExists as success.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/controllers/kms_preflight_sandbox.go`:
- Around line 88-107: Use the state controller’s selected write-key ID instead
of deriving latestKeyID by scanning keySecrets before calling
rewritePreflightWriteKeyEndpoint. Preserve the existing no-key error handling,
and pass the controller-selected write-key value so the rewrite targets the
active write-key provider.
🪄 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: ebbdc8d0-8b38-4f98-b2d3-2030028cf6ae
📒 Files selected for processing (11)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/controllers/kms_preflight_sandbox.gopkg/operator/encryption/controllers/kms_preflight_sandbox_test.gopkg/operator/encryption/controllers/state_controller.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/statemachine/transition.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/operator/encryption/secrets/secrets.go
- pkg/operator/encryption/controllers/state_controller_test.go
- pkg/operator/encryption/controllers/kms_preflight_controller_test.go
- pkg/operator/encryption/controllers/key_controller.go
- pkg/operator/encryption/controllers/kms_preflight_controller.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/controllers/kms_preflight_sandbox_test.go
- pkg/operator/encryption/statemachine/transition.go
- pkg/operator/encryption/secrets/types.go
- pkg/operator/encryption/controllers/key_controller_test.go
| var latestKeyID uint64 | ||
| foundKey := false | ||
| for _, s := range keySecrets { | ||
| id, ok := state.NameToKeyID(s.Name) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if !foundKey || id > latestKeyID { | ||
| latestKeyID = id | ||
| foundKey = true | ||
| } | ||
| } | ||
| if !foundKey { | ||
| return nil, fmt.Errorf("no encryption key secrets found after key controller run in temp namespace %q", tempNamespace) | ||
| } | ||
|
|
||
| rewritten, err := rewritePreflightWriteKeyEndpoint(encryptionSecret, latestKeyID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to rewrite preflight KMS endpoint: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect write-key selection logic relative to key IDs.
rg -n -C10 'func getDesiredEncryptionState' pkg/operator/encryption/statemachine/transition.go
rg -n -C5 'func NameToKeyID' pkg/operator/encryption/stateRepository: openshift/library-go
Length of output: 2189
🏁 Script executed:
#!/bin/bash
sed -n '89,230p' pkg/operator/encryption/statemachine/transition.go
sed -n '66,95p' pkg/operator/encryption/state/helpers.go
sed -n '300,345p' pkg/operator/encryption/controllers/kms_preflight_sandbox.go
rg -n -C8 'latestKeyID|rewritePreflightWriteKeyEndpoint|write.?key|WriteKey|KeyID' pkg/operator/encryption/controllers pkg/operator/encryption/statemachine pkg/operator/encryption/stateRepository: openshift/library-go
Length of output: 50377
🏁 Script executed:
#!/bin/bash
rg -n -C12 'func ToEncryptionState|ToEncryptionState\(' pkg/operator/encryption
rg -n -C10 'func KeysWithPotentiallyPersistedDataAndNextReadKey|func SortRecentFirst|backedKeys' pkg/operator/encryption
sed -n '1,120p' pkg/operator/encryption/statemachine/transition.go
sed -n '1,115p' pkg/operator/encryption/state/helpers.goRepository: openshift/library-go
Length of output: 47442
🏁 Script executed:
#!/bin/bash
sed -n '45,115p' pkg/operator/encryption/controllers/kms_preflight_sandbox.go
sed -n '282,350p' pkg/operator/encryption/encryptiondata/config.go
rg -n -C10 'GetEncryptionConfigAndStateInNamespace|FromEncryptionState|prepare.*sandbox|sandbox' pkg/operator/encryption/controllers/kms_preflight_sandbox.go pkg/operator/encryption/controllers/*test.goRepository: openshift/library-go
Length of output: 5713
🏁 Script executed:
#!/bin/bash
rg -n -C18 'func runStateControllerInNamespace|runStateControllerInNamespace\(' pkg/operator/encryption/controllers
rg -n -C12 'func FromEncryptionState|createKMSPluginName' pkg/operator/encryption/encryptiondata
sed -n '1,55p' pkg/operator/encryption/controllers/kms_preflight_sandbox.go
sed -n '350,430p' pkg/operator/encryption/encryptiondata/config.goRepository: openshift/library-go
Length of output: 18092
🏁 Script executed:
#!/bin/bash
sed -n '170,245p' pkg/operator/encryption/controllers/kms_preflight_sandbox.go
rg -n -C15 'func \(c \*stateController\).*sync|syncInternal\(' pkg/operator/encryption/controllers/state_controller.goRepository: openshift/library-go
Length of output: 5335
🏁 Script executed:
#!/bin/bash
sed -n '120,185p' pkg/operator/encryption/controllers/state_controller.go
python3 - <<'PY'
# Read-only model of the relevant state-machine transition.
# Existing config writes key 1. A newly backed key 2 is present, but its
# migration/read-key requirements are not yet complete, so STEP 2 returns
# before STEP 3 assigns key 2 as WriteKey.
old_write_key = 1
backed_key_ids = sorted([1, 2], reverse=True)
read_keys = [old_write_key]
expected_read_keys = [2, 1]
read_keys = sorted(set(read_keys + expected_read_keys), reverse=True)
all_read_secrets_as_expected = True
# The newly added read key is backed, but adding it changes the state.
changed = 2 not in [old_write_key]
if changed:
all_read_secrets_as_expected = False
desired_write_key = old_write_key
if all_read_secrets_as_expected:
desired_write_key = backed_key_ids[0] # STEP 3
print({
"backed_key_ids": backed_key_ids,
"desired_read_keys": read_keys,
"desired_write_key": desired_write_key,
"latest_key_id": backed_key_ids[0],
"latest_is_desired_write_key": desired_write_key == backed_key_ids[0],
})
assert desired_write_key != backed_key_ids[0]
PYRepository: openshift/library-go
Length of output: 2929
🏁 Script executed:
#!/bin/bash
sed -n '120,185p' pkg/operator/encryption/controllers/state_controller.go
python3 - <<'PY'
# Read-only model of the relevant state-machine transition.
old_write_key = 1
backed_key_ids = sorted([1, 2], reverse=True)
read_keys = sorted(set([old_write_key, 2, 1]), reverse=True)
all_read_secrets_as_expected = False
desired_write_key = old_write_key
if all_read_secrets_as_expected:
desired_write_key = backed_key_ids[0]
print({
"backed_key_ids": backed_key_ids,
"desired_read_keys": read_keys,
"desired_write_key": desired_write_key,
"latest_key_id": backed_key_ids[0],
"latest_is_desired_write_key": desired_write_key == backed_key_ids[0],
})
assert desired_write_key != backed_key_ids[0]
PYRepository: openshift/library-go
Length of output: 2929
Pass the state controller’s selected write-key ID to rewritePreflightWriteKeyEndpoint.
When a new key is added, STEP 2 can return before STEP 3 promotes it to the write key. The highest key ID can therefore remain a read key. Using latestKeyID can rewrite the wrong KMS provider or fail to find the provider.
🤖 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/controllers/kms_preflight_sandbox.go` around lines 88
- 107, Use the state controller’s selected write-key ID instead of deriving
latestKeyID by scanning keySecrets before calling
rewritePreflightWriteKeyEndpoint. Preserve the existing no-key error handling,
and pass the controller-selected write-key value so the rewrite targets the
active write-key provider.
f5aa865 to
e35c7dc
Compare
Allow key secret creation in a custom namespace and optionally skip operator degraded status writes so preflight can reuse the core logic.
Allow encryption-config writes in a custom namespace and optionally skip operator degraded status writes for preflight reuse.
Run key/state controller cores against an ephemeral namespace so preflight can obtain a real encryption-config secret without mutating production keys or writing operator degraded conditions.
Best-effort delete of labeled temp namespaces on idle/succeeded preflight paths so failed mid-compute runs do not leak namespaces.
| } | ||
| }() | ||
|
|
||
| if err := seedEncryptionKeySecrets(ctx, coreClient, secrets.EncryptionKeysNamespace, tempNamespace, encryptionSecretSelector); err != nil { |
There was a problem hiding this comment.
this is necessary to copy existing key secrets. We need to copy those for cases when we need an in-place upgrade for instance
| defer func() { | ||
| if cleanupErr := deletePreflightTempNamespace(ctx, coreClient, tempNamespace); cleanupErr != nil { | ||
| klog.Warningf("failed to delete preflight temp namespace %q: %v", tempNamespace, cleanupErr) | ||
| } |
There was a problem hiding this comment.
should we worry about namespace churn (too many namespaces being created/deleted)?
| return nil, fmt.Errorf("no encryption key secrets found after key controller run in temp namespace %q", tempNamespace) | ||
| } | ||
|
|
||
| rewritten, err := rewritePreflightWriteKeyEndpoint(encryptionSecret, latestKeyID) |
There was a problem hiding this comment.
need to confirm with @p0lyn0mial, but IIUC we'll be able to remove this workaround
e35c7dc to
73ca988
Compare
| @@ -0,0 +1,318 @@ | |||
| package controllers | |||
There was a problem hiding this comment.
this isn't really a "sandbox" because it's not possible to control where the key/state controllers will store data (i.e., an change in one of those controllers might introduce a resource somewhere outside the designated namespace and the preflight controller won't know it)
|
@bertinatto: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit