Add encryption config Secret generation with in-memory Key Secret in preflight controller - #2376
Add encryption config Secret generation with in-memory Key Secret in preflight controller#2376ardaguclu wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughKMS preflight now computes an encryption-config Secret from the current KMS configuration and a simulated additional key before deploying the preflight workload. Encryption lookup uses explicit dependencies, and tests add matching deployer wiring. ChangesKMS preflight encryption configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant runPreflightChecks
participant computeEncryptionConfigSecret
participant ComputeDesiredEncryptionStateWithAdditionalKey
participant encryptionDeployer
runPreflightChecks->>computeEncryptionConfigSecret: compute encryption-config Secret
computeEncryptionConfigSecret->>encryptionDeployer: read deployed encryption config
computeEncryptionConfigSecret->>ComputeDesiredEncryptionStateWithAdditionalKey: provide simulated key Secret
ComputeDesiredEncryptionStateWithAdditionalKey-->>computeEncryptionConfigSecret: desired encryption state
computeEncryptionConfigSecret-->>runPreflightChecks: encryption-config Secret
runPreflightChecks->>encryptionDeployer: deploy preflight workload with Secret
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu 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
| return nil, fmt.Errorf("preflight is only supported in KMS mode, current mode is %q", currentMode) | ||
| } | ||
|
|
||
| providerCfg, err := newKMSProviderConfig(apiEncryptionConfiguration.KMS) |
There was a problem hiding this comment.
We need to calculate the hash and compare it with what we have. If it doesn't match, we should requeue or return error.
![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/controllers/kms_preflight_controller_test.go (1)
864-875: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the encryption Secret passed to
Deploy.The fake deployer discards this argument, so the new no-pod path only proves it did not error. Capture and validate the generated Secret, including the candidate KMS plugin and referenced data; include an existing key to verify preservation.
🤖 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 864 - 875, Update the test around NewKMSPreflightController and fakeEncryptionDeployer so the Deploy call captures the encryption Secret instead of discarding it. Assert the generated Secret contains the candidate KMS plugin and its referenced data, and seed an existing key before reconciliation to verify it remains preserved.
🤖 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_controller.go`:
- Line 482: Update the generateKeySecret call in the preflight flow to use the
next unused KMS key ID instead of the hard-coded 0, ensuring the simulated
secret always sorts as newest. Determine that ID from existing keys and add
coverage for a case where keys already exist, verifying preflight validates the
newly simulated key’s plugin.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 864-875: Update the test around NewKMSPreflightController and
fakeEncryptionDeployer so the Deploy call captures the encryption Secret instead
of discarding it. Assert the generated Secret contains the candidate KMS plugin
and its referenced data, and seed an existing key before reconciliation to
verify it remains preserved.
🪄 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: 20e22517-de2f-4e8f-b04f-3d63e16b2946
📒 Files selected for processing (4)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/statemachine/transition.go
f86d558 to
b6631bb
Compare
…preflight controller
b6631bb to
fc7a4f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/operator/encryption/controllers/kms_preflight_controller_test.go (1)
864-877: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test asserts on the computed encryption-config Secret content.
fakeDeployer.Deploydiscards the*corev1.Secretargument, so none of these scenarios verify thatcomputeEncryptionConfigSecret/buildSimulatedKeySecretactually produce a correct Secret (simulated key, KMS plugin config, referenced secret/configmap data). Given this is new, non-trivial logic, consider capturing the deployed Secret infakeDeployerand adding assertions for at least one scenario (e.g. that it contains the expected KMS plugin config), plus a scenario wherecomputeEncryptionConfigSecretitself fails (e.g. mode mismatch).🤖 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 864 - 877, Extend the preflight controller tests around NewKMSPreflightController to have fakeDeployer capture the *corev1.Secret passed to Deploy instead of discarding it. Add assertions for at least one successful scenario covering the expected simulated key, KMS plugin configuration, and referenced Secret/ConfigMap data, and add a failure scenario where computeEncryptionConfigSecret returns an error such as a mode mismatch.pkg/operator/encryption/statemachine/transition.go (1)
64-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared logic with
GetEncryptionConfigAndState.
ComputeDesiredEncryptionStateWithAdditionalKeyduplicates the "load deployed config → parse → list key secrets" sequence already implemented inGetEncryptionConfigAndState(Lines 38-58), differing only in the converged check and the extra key. Consider factoring the shared steps into a helper so future changes (e.g., error wrapping) don't need to be kept in sync across both call paths.♻️ Sketch of a shared helper
func loadCurrentAndKeySecrets(ctx context.Context, deployer Deployer, secretClient corev1client.SecretsGetter, encryptionSecretSelector metav1.ListOptions) (*encryptiondata.Config, []*corev1.Secret, error) { encryptionConfigSecret, _, err := deployer.DeployedEncryptionConfigSecret(ctx) if err != nil { return nil, nil, err } var current *encryptiondata.Config if encryptionConfigSecret != nil { current, err = encryptiondata.FromSecret(encryptionConfigSecret) if err != nil { return nil, nil, fmt.Errorf("invalid encryption config %s/%s: %v", encryptionConfigSecret.Namespace, encryptionConfigSecret.Name, err) } } encryptionSecrets, err := secrets.ListKeySecrets(ctx, secretClient, encryptionSecretSelector) if err != nil { return nil, nil, err } return current, encryptionSecrets, nil }🤖 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/statemachine/transition.go` around lines 64 - 101, Extract the shared deployed-config loading, parsing, and key-secret listing logic from GetEncryptionConfigAndState and ComputeDesiredEncryptionStateWithAdditionalKey into a helper such as loadCurrentAndKeySecrets. Update both callers to use it, preserving GetEncryptionConfigAndState’s converged check and appending additionalKeySecret only in ComputeDesiredEncryptionStateWithAdditionalKey.
🤖 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_controller.go`:
- Around line 472-505: Reuse the APIServer encryption configuration already
fetched by preflightRequired when generating the Secret, instead of calling
getCurrentModeReasonAndEncryptionConfig again inside
computeEncryptionConfigSecret. Thread the fetched configuration through
runPreflightChecks and computeEncryptionConfigSecret, while preserving the
existing KMS-mode validation and error handling.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 864-877: Extend the preflight controller tests around
NewKMSPreflightController to have fakeDeployer capture the *corev1.Secret passed
to Deploy instead of discarding it. Add assertions for at least one successful
scenario covering the expected simulated key, KMS plugin configuration, and
referenced Secret/ConfigMap data, and add a failure scenario where
computeEncryptionConfigSecret returns an error such as a mode mismatch.
In `@pkg/operator/encryption/statemachine/transition.go`:
- Around line 64-101: Extract the shared deployed-config loading, parsing, and
key-secret listing logic from GetEncryptionConfigAndState and
ComputeDesiredEncryptionStateWithAdditionalKey into a helper such as
loadCurrentAndKeySecrets. Update both callers to use it, preserving
GetEncryptionConfigAndState’s converged check and appending additionalKeySecret
only in ComputeDesiredEncryptionStateWithAdditionalKey.
🪄 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: e5668c4a-241e-410f-b95a-25aff4b967c1
📒 Files selected for processing (4)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/statemachine/transition.go
| func (c *kmsPreflightController) computeEncryptionConfigSecret(ctx context.Context) (*corev1.Secret, error) { | ||
| currentMode, _, apiEncryptionConfiguration, err := getCurrentModeReasonAndEncryptionConfig(ctx, c.apiServerClient, c.operatorClient, c.unsupportedConfigPrefix) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if currentMode != state.KMS { | ||
| return nil, fmt.Errorf("preflight is only supported in KMS mode, current mode is %q", currentMode) | ||
| } | ||
|
|
||
| providerCfg, err := newKMSProviderConfig(apiEncryptionConfiguration.KMS) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create KMS provider config: %w", err) | ||
| } | ||
|
|
||
| simulatedKeySecret, err := c.buildSimulatedKeySecret(ctx, apiEncryptionConfiguration.KMS, providerCfg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build simulated key secret: %w", err) | ||
| } | ||
|
|
||
| desiredState, err := statemachine.ComputeDesiredEncryptionStateWithAdditionalKey( | ||
| ctx, c.encryptionDeployer, c.coreClient, c.encryptionSecretSelector, | ||
| c.provider.EncryptedGRs(), simulatedKeySecret, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cfg, err := encryptiondata.FromEncryptionState(desiredState) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build encryption config: %w", err) | ||
| } | ||
| secretName := fmt.Sprintf("%s-%s", encryptiondata.EncryptionConfSecretName, c.instanceName) | ||
| return encryptiondata.ToSecret("openshift-config-managed", secretName, cfg) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Duplicate APIServer fetch between preflightRequired and computeEncryptionConfigSecret.
preflightRequired (Line 673) and computeEncryptionConfigSecret (via getCurrentModeReasonAndEncryptionConfig, Line 473) independently re-fetch the "cluster" APIServer object within the same runPreflightChecks invocation. If the spec changes between the two fetches, the deployed encryption-config Secret could reflect a different plugin config than the one requiredHash was computed from — the pod's reported hash condition would no longer accurately describe the deployed Secret's content. This self-corrects on the next resync (stale-hash cleanup), but threading the already-fetched config from preflightRequired into computeEncryptionConfigSecret would remove the extra round trip and the inconsistency window.
🤖 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.go` around lines
472 - 505, Reuse the APIServer encryption configuration already fetched by
preflightRequired when generating the Secret, instead of calling
getCurrentModeReasonAndEncryptionConfig again inside
computeEncryptionConfigSecret. Thread the fetched configuration through
runPreflightChecks and computeEncryptionConfigSecret, while preserving the
existing KMS-mode validation and error handling.
|
This was experimentation, closing |
|
@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. |
1 participant
This PR experiments the encryption controllers to find an elegant way for the preflight controller to reuse the encryption config Secret generation mechanism.
Summary by CodeRabbit
Summary
Improvements
encryption-configSecret and provide it to the preflight workload, including when no preflight pod exists.Tests