[WIP] kms: compute dry-run encryption config secret for preflight pod - #2377
[WIP] kms: compute dry-run encryption config secret for preflight pod#2377bertinatto wants to merge 3 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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
|
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 preflight controller now computes an encryption configuration Secret through sandboxed key and state reconciliation, rewrites its KMS endpoint for preflight, supports convergence requeues, and passes the resulting Secret to the preflight deployer. Key and state controllers now expose concrete controller types. ChangesKMS preflight encryption flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KMSPreflightController
participant DryRunSandbox
participant KeyController
participant StateController
participant PreflightDeployer
KMSPreflightController->>DryRunSandbox: compute encryption-config Secret
DryRunSandbox->>KeyController: reconcile key Secret
KeyController-->>DryRunSandbox: capture generated key Secret
DryRunSandbox->>StateController: reconcile encryption-config Secret
StateController-->>DryRunSandbox: return generated configuration Secret
DryRunSandbox-->>KMSPreflightController: return rewritten Secret or requeue
KMSPreflightController->>PreflightDeployer: deploy with encryption-config Secret
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 108-156: Update seedPreflightSandboxFS to reuse the
already-fetched liveDeployed encryption-config secret instead of performing a
second live Secrets.Get for managedEncryptionConfigName. Pass that shared value
into the function or otherwise connect it to the existing source-of-truth flow,
while preserving handling for an absent secret and the existing putSecretYAML
behavior.
🪄 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: 05fc02de-036a-4b69-aeb1-d99be8615bba
📒 Files selected for processing (4)
pkg/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.go
2634b8b to
cf6b6ed
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/kms_preflight_sandbox.go (1)
352-394: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent fallback could preflight-check the wrong KMS provider.
When the write key can't be precisely matched (lines 376-391), the function silently rewrites the first KMS provider's endpoint instead of failing or surfacing the ambiguity. In a multi-provider config this can validate the wrong provider, giving a false-positive preflight result for exactly the scenario this controller exists to catch.
🤖 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 352 - 394, Remove the silent first-provider fallback in rewritePreflightWriteKeyEndpoint. When no KMS provider matches the requested keyID or expected endpoint, return an explicit error instead of rewriting any provider; preserve the existing successful rewrite and secret serialization behavior for precise matches.
🤖 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.
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox.go`:
- Around line 352-394: Remove the silent first-provider fallback in
rewritePreflightWriteKeyEndpoint. When no KMS provider matches the requested
keyID or expected endpoint, return an explicit error instead of rewriting any
provider; preserve the existing successful rewrite and secret serialization
behavior for precise matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 968ce6eb-6617-4a11-85c0-f5e2ae1e6d4e
📒 Files selected for processing (7)
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.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/encryption/controllers/key_controller_test.go
- pkg/operator/encryption/controllers/kms_preflight_sandbox_test.go
- pkg/operator/encryption/controllers/kms_preflight_controller_test.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/controllers/kms_preflight_controller.go
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
This sandboxing looks brilliant to me. I just commented about a few items that logically differ between preflight and key controller.
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/encryption/controllers/kms_preflight_sandbox.go (1)
92-95: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReuse one operator snapshot for both dry-runs. Each call to
operatorClientForDryRun()reads live state independently, soKeyControllerandStateControllercan observe different spec/status if the operator changes between calls. Capture the fake client once and share it across both paths.🤖 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 92 - 95, Update the dry-run setup in the surrounding preflight flow to create one fake operator client snapshot and reuse it for both KeyController and StateController paths. Replace separate operatorClientForDryRun calls with the shared client, while preserving the existing error propagation when snapshot creation fails.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/kms_preflight_sandbox.go (1)
117-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the underlying finder error for easier debugging.
The original error from
findWrittenWithPrefixis discarded in favor of a fixed message. Minor, but wrapping it would preserve useful detail (e.g., which prefix/namespace was searched) if this ever fires in practice.♻️ Proposed fix
created, err := overlay.findWrittenWithPrefix(openshiftConfigManagedNS, prefix) if err != nil { - return "", fmt.Errorf("preflight required but key controller dry-run did not create a new encryption key secret") + return "", fmt.Errorf("preflight required but key controller dry-run did not create a new encryption key secret: %w", err) }🤖 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 117 - 122, Update the error handling after overlay.findWrittenWithPrefix in the preflight key lookup to wrap and retain the underlying err while preserving the existing contextual message; include the original error as the cause rather than discarding it.
🤖 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.
Outside diff comments:
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox.go`:
- Around line 92-95: Update the dry-run setup in the surrounding preflight flow
to create one fake operator client snapshot and reuse it for both KeyController
and StateController paths. Replace separate operatorClientForDryRun calls with
the shared client, while preserving the existing error propagation when snapshot
creation fails.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox.go`:
- Around line 117-122: Update the error handling after
overlay.findWrittenWithPrefix in the preflight key lookup to wrap and retain the
underlying err while preserving the existing contextual message; include the
original error as the cause rather than discarding it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 10885456-5564-4850-9edb-c25485bb872c
📒 Files selected for processing (4)
pkg/operator/encryption/controllers/kms_preflight_overlay.gopkg/operator/encryption/controllers/kms_preflight_overlay_test.gopkg/operator/encryption/controllers/kms_preflight_sandbox.gopkg/operator/encryption/controllers/kms_preflight_sandbox_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/encryption/controllers/kms_preflight_sandbox_test.go
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Since we agree upon pursuing this mechanism, I dropped some comments about the code.
| if err != nil { | ||
| return false, fmt.Errorf("failed to compute encryption config for preflight: %w", err) | ||
| } | ||
| if requeueForConvergence { |
There was a problem hiding this comment.
It is better to document why we requeue when there is no convergence for future readers.
There was a problem hiding this comment.
@p0lyn0mial since in-place field updates work even if there is no convergence, preflight should work too.
aa75446 to
1edb9fb
Compare
openshift-ci
Bot
removed
the
needs-rebase
bertinatto
changed the title
openshift-ci
Bot
added
do-not-merge/work-in-progress
1edb9fb to
54ac90b
Compare
e20ad1a to
820dda3
Compare
820dda3 to
33f19d7
Compare
|
/retest |
| // encryptionDeployer is the same statemachine.Deployer used by the key and state | ||
| // controllers. It is consulted once when seeding the dry-run that produces the | ||
| // encryption-config Secret for the preflight pod. | ||
| encryptionDeployer statemachine.Deployer, |
There was a problem hiding this comment.
nit: Naming is confusing for me especially looking with the one line above.
There was a problem hiding this comment.
would it be better if we renamed deployer to preflightDeployet in a separate PR?
| if err != nil { | ||
| return false, fmt.Errorf("failed to compute encryption config for preflight: %w", err) | ||
| } | ||
| if requeueForConvergence { |
There was a problem hiding this comment.
@p0lyn0mial since in-place field updates work even if there is no convergence, preflight should work too.
| if s, ok := c.parent.written[c.ns][name]; ok { | ||
| return s.DeepCopy(), nil | ||
| } | ||
| return c.parent.real.Secrets(c.ns).Get(ctx, name, opts) |
There was a problem hiding this comment.
result should not be written to c.parent.written back, because this does not work as cache
There was a problem hiding this comment.
agreed; the object we get from the apiserver should not be stored in written (which should only contain objects that were tried to be 'written' [created/updated]). At least that's the idea, are you suggesting it's not currently doing that?
There was a problem hiding this comment.
Current mechanism looks good to me
| if err != nil { | ||
| return nil, err | ||
| } | ||
| list, err := c.parent.real.Secrets(c.ns).List(ctx, opts) |
There was a problem hiding this comment.
Would make sense adding nil checks for c.parent.real and return error
There was a problem hiding this comment.
I can add a nil check in the constructor (rather than checking it on every Get/List call. PTAL
| for i := range cfg.Encryption.Resources { | ||
| for j := range cfg.Encryption.Resources[i].Providers { | ||
| kms := cfg.Encryption.Resources[i].Providers[j].KMS | ||
| if kms == nil || kms.Endpoint != wantOld { |
There was a problem hiding this comment.
it is better to compare against providerName instead of endpoint.
| } | ||
|
|
||
| // recordedKeySecret returns the single encryption-key secret recorded for instanceName. | ||
| func recordedKeySecret(overlay *dryRunSecretsGetter, instanceName string) (*corev1.Secret, error) { |
There was a problem hiding this comment.
This sounds like leaking the knowledge of encryption controllers. Instead of this function, can't we get keyID like this;
keySecrets, err := secrets.ListKeySecrets(ctx, overlay, encryptionSecretSelector)
if err != nil {
return false, nil, err
}
var latestKeyID uint64
for _, s := range keySecrets {
if id, ok := state.NameToKeyID(s.Name); ok && id > latestKeyID {
latestKeyID = id
}
}| // | ||
| // TODO: once preflight dials the write-key socket (kms-{id}.sock) directly, | ||
| // this rewrite can be removed and the dry-run secret can be used as-is. | ||
| func rewritePreflightWriteKeyEndpoint(secret *corev1.Secret, keyID uint64) (*corev1.Secret, error) { |
There was a problem hiding this comment.
Instead of having this function, can't we call this in the caller;
target := fmt.Sprintf("%d_", latestKeyID)
for i := range cfg.Encryption.Resources {
for j := range cfg.Encryption.Resources[i].Providers {
if kms := cfg.Encryption.Resources[i].Providers[j].KMS; kms != nil && strings.HasPrefix(kms.Name, target) {
kms.Endpoint = preflightKMSSocketEndpoint
}
}
}There was a problem hiding this comment.
if I understand correctly, that's just part of what this function do, the caller still needs to call encryptiondata.FromSecret. Inlining this in computeEncryptionConfigSecretDryRun() would make it even bigger. Also, the idea is that this would be dropped in the future, so a helpers makes that easier. WDYT? Am I misunderstanding it?
There was a problem hiding this comment.
oh, and the caller would also need encryptiondata.ToSecret
There was a problem hiding this comment.
Yeah, it would be better to leave this func as is
bef1557 to
8483711
Compare
8483711 to
fa25e8f
Compare
|
@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. |
3 participants
When deploying a new KMS preflight pod, the preflight controller now computes the required encryption configuration Secret using a dry-run sandbox.
Summary by CodeRabbit
encryption-configvia a dry-run and deploys the preflight workload using the computed, validated result.nilencryption-configwhen no preflight pod exists.encryption-configgeneration and for the dry-run secrets overlay.encryption-configis passed to the deployer.