WIP: Kms preflight compute converged 3 - #2410
Conversation
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 change adds ChangesEncryption planner and controller integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KMSPreflightController
participant EncryptionPlanner
participant statemachine.Deployer
participant PreflightWorkload
KMSPreflightController->>EncryptionPlanner: Compute candidate encryption Secret
EncryptionPlanner-->>KMSPreflightController: Return candidate configuration
KMSPreflightController->>statemachine.Deployer: Deploy candidate Secret
statemachine.Deployer->>PreflightWorkload: Apply KMS configuration
PreflightWorkload-->>KMSPreflightController: Report convergence
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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: 3
🧹 Nitpick comments (2)
pkg/operator/encryption/controllers/encryption_planner.go (2)
41-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional planner dependencies are expressed positionally with no guard on the
ComputeDesiredConfigpath.NewEncryptionPlanneraccepts nine positional parameters, three of which are optional, and the state controller passesnilfor all three.PlanKeyandComputeCandidateConfigreject nil clients, butComputeDesiredConfigdoes not, so the only thing that keeps the state controller from panicking is an undocumented convention.
pkg/operator/encryption/controllers/encryption_planner.go#L41-L63: replace the positional parameter list with a parameter struct, or add a second constructor that only accepts the dependenciesComputeDesiredConfigneeds.pkg/operator/encryption/controllers/state_controller.go#L130-L141: use that narrower constructor so this call site cannot passnilclients.🤖 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/encryption_planner.go` around lines 41 - 63, Replace the positional dependency list in NewEncryptionPlanner with a safer parameter structure or add a narrower constructor containing only the dependencies required by ComputeDesiredConfig; preserve the existing full constructor for PlanKey and ComputeCandidateConfig callers as needed. In pkg/operator/encryption/controllers/encryption_planner.go lines 41-63, implement the constructor change. In pkg/operator/encryption/controllers/state_controller.go lines 130-141, update the state controller to call the narrower constructor so it cannot supply nil clients.
228-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared planning steps to remove duplication between
PlanKeyandComputeCandidateConfig.Lines 229-275 repeat lines 113-156 almost exactly: the client nil guards,
resolveEncryptionModeAndConfig,GetEncryptionConfigAndState, thehasBeenOnBeforecheck, the KMS provider config construction, andplanNextEncryptionKey. Lines 303-321 repeat lines 203-224 for desired-state serialization.The stated purpose of this type is to keep the key controller, state controller, and preflight controller from drifting. Two parallel copies of the plan sequence inside the planner itself reintroduce that drift risk. Extract one internal helper that returns mode, current config, key Secrets, and key plan, then let
PlanKeyandComputeCandidateConfigbuild their own result types from it. Extract a second helper for the desired-state →Config→ Secret conversion.🤖 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/encryption_planner.go` around lines 228 - 324, Extract the duplicated planning flow from PlanKey and ComputeCandidateConfig into one internal helper returning the encryption mode, current config, key Secrets, and key plan, including shared guards, mode resolution, state loading, early identity handling, provider construction, and planNextEncryptionKey. Extract the desired-state serialization in ComputeCandidateConfig and its counterpart in PlanKey into a second helper that returns the Config and managed Secret, then have both callers construct their existing result types from these helpers without changing behavior.
🤖 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/encryption_planner.go`:
- Line 433: Update the lookup in the encryption planning flow around
ModeToNewKeyFunc to verify that currentMode has a registered, non-nil
constructor before invoking it. If the mode is missing, return a descriptive
error from the surrounding function; ensure both modes produced by
resolveEncryptionModeAndConfig, including state.Identity and state.KMS, are
handled safely.
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 1293-1295: Update the fixture setup around the
PluginSecretData.Set and PluginConfigMapData.Set calls in the affected test,
including the analogous calls near the later fixture block, to check each
returned error and call t.Fatalf with relevant context on failure. Follow the
existing error-handling pattern used by the fixtures around lines 1154-1162 and
1563-1571, ensuring no Set error is discarded.
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 669-680: Derive the rewrite target in the PlannedKey-nil branch
from result.DesiredState, using the candidate write key rather than
latestKeyIDFromSecrets over all secrets. Add or use a helper that validates the
desired state has a write key and parses its key ID, then pass that ID to
rewriteWriteKeyKMSEndpoint; also add coverage for a newer non-migrated read-key
Secret alongside an older deployed write key.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/encryption_planner.go`:
- Around line 41-63: Replace the positional dependency list in
NewEncryptionPlanner with a safer parameter structure or add a narrower
constructor containing only the dependencies required by ComputeDesiredConfig;
preserve the existing full constructor for PlanKey and ComputeCandidateConfig
callers as needed. In pkg/operator/encryption/controllers/encryption_planner.go
lines 41-63, implement the constructor change. In
pkg/operator/encryption/controllers/state_controller.go lines 130-141, update
the state controller to call the narrower constructor so it cannot supply nil
clients.
- Around line 228-324: Extract the duplicated planning flow from PlanKey and
ComputeCandidateConfig into one internal helper returning the encryption mode,
current config, key Secrets, and key plan, including shared guards, mode
resolution, state loading, early identity handling, provider construction, and
planNextEncryptionKey. Extract the desired-state serialization in
ComputeCandidateConfig and its counterpart in PlanKey into a second helper that
returns the Config and managed Secret, then have both callers construct their
existing result types from these helpers without changing behavior.
🪄 Autofix
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: acfed139-8105-4df9-bd01-782b6359fcad
📒 Files selected for processing (11)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_planner.gopkg/operator/encryption/controllers/encryption_planner_test.gopkg/operator/encryption/controllers/helpers_test.gopkg/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/state_controller.gopkg/operator/encryption/statemachine/transition.gopkg/operator/encryption/statemachine/transition_test.go
| externalReason string, | ||
| kmsEndpointOverride string, | ||
| ) (state.KeyState, error) { | ||
| bs := crypto.ModeToNewKeyFunc[currentMode]() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the crypto.ModeToNewKeyFunc lookup against a missing mode.
Line 433 calls the map value directly. If crypto.ModeToNewKeyFunc has no entry for the resolved mode, the value is a nil function and the call panics. resolveEncryptionModeAndConfig permits state.Identity and state.KMS, so both must be present in the map.
Add an existence check and return an error, or confirm the map covers every mode that resolveEncryptionModeAndConfig returns.
#!/bin/bash
# Check which modes ModeToNewKeyFunc defines.
rg -nP -C 20 'ModeToNewKeyFunc' --type=go
rg -nP -C 3 '^\s*(AESCBC|AESGCM|KMS|Identity|SecretBox|DefaultMode)\s' --type=go -g '**/state/**'🛡️ Proposed guard
- bs := crypto.ModeToNewKeyFunc[currentMode]()
+ newKeyFunc, ok := crypto.ModeToNewKeyFunc[currentMode]
+ if !ok {
+ return state.KeyState{}, fmt.Errorf("no key generation function for encryption mode %q", currentMode)
+ }
+ bs := newKeyFunc()🤖 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/encryption_planner.go` at line 433,
Update the lookup in the encryption planning flow around ModeToNewKeyFunc to
verify that currentMode has a registered, non-nil constructor before invoking
it. If the mode is missing, return a descriptive error from the surrounding
function; ensure both modes produced by resolveEncryptionModeAndConfig,
including state.Identity and state.KMS, are handled safely.
| // When reusing an existing write key, rewrite its endpoint so the preflight | ||
| // checker dials the fixed socket. New planned keys already use the override. | ||
| if result.PlannedKey == nil { | ||
| writeKeyID, err := latestKeyIDFromSecrets(result.KeySecrets) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
| secret, err = rewriteWriteKeyKMSEndpoint(secret, writeKeyID, preflightKMSSocketEndpoint) | ||
| if err != nil { | ||
| return false, nil, fmt.Errorf("failed to rewrite preflight KMS endpoint: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Derive the rewrite target from the candidate write key, not from the newest key Secret.
latestKeyIDFromSecrets returns max(key ID) over all key Secrets. The write key in the candidate configuration is chosen by statemachine.GetDesiredEncryptionState, which can keep an older write key while a newer key Secret exists. The tests in this PR show exactly that shape: key 3 stays the write key while key 4 is only a read key.
PlannedKey == nil does not exclude this case. If a key Secret newer than the current write key already exists and is not migrated yet, needsNewKey reports needed=false, so no key is planned, and max(key ID) is the new read key rather than the write key. rewriteWriteKeyKMSEndpoint then rewrites the read-key provider and leaves the write-key provider on its per-key production socket. The preflight pod dials the fixed socket, so it validates a provider that is not the write key.
Use result.DesiredState to read the write-key name instead. That value is authoritative for the configuration being deployed.
Please also add a test with a non-migrated newer key Secret plus an older write key in the deployed configuration.
🐛 Proposed fix
if result.PlannedKey == nil {
- writeKeyID, err := latestKeyIDFromSecrets(result.KeySecrets)
+ writeKeyID, err := candidateWriteKeyID(result.DesiredState)
if err != nil {
return false, nil, err
}
secret, err = rewriteWriteKeyKMSEndpoint(secret, writeKeyID, preflightKMSSocketEndpoint)
if err != nil {
return false, nil, fmt.Errorf("failed to rewrite preflight KMS endpoint: %w", err)
}
}Replacement helper for latestKeyIDFromSecrets:
// candidateWriteKeyID returns the key ID of the write key in the candidate
// encryption state. All group resources share one write key at this point.
func candidateWriteKeyID(desired map[schema.GroupResource]state.GroupResourceState) (uint64, error) {
for gr, grState := range desired {
if !grState.HasWriteKey() {
return 0, fmt.Errorf("resource %s has no write key in the candidate encryption state", gr)
}
id, ok := state.NameToKeyID(grState.WriteKey.Key.Name)
if !ok {
return 0, fmt.Errorf("write key %q for resource %s has an invalid name", grState.WriteKey.Key.Name, gr)
}
return id, nil
}
return 0, fmt.Errorf("no encryption key secrets available to compute preflight encryption config")
}📝 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.
| // When reusing an existing write key, rewrite its endpoint so the preflight | |
| // checker dials the fixed socket. New planned keys already use the override. | |
| if result.PlannedKey == nil { | |
| writeKeyID, err := latestKeyIDFromSecrets(result.KeySecrets) | |
| if err != nil { | |
| return false, nil, err | |
| } | |
| secret, err = rewriteWriteKeyKMSEndpoint(secret, writeKeyID, preflightKMSSocketEndpoint) | |
| if err != nil { | |
| return false, nil, fmt.Errorf("failed to rewrite preflight KMS endpoint: %w", err) | |
| } | |
| } | |
| // When reusing an existing write key, rewrite its endpoint so the preflight | |
| // checker dials the fixed socket. New planned keys already use the override. | |
| if result.PlannedKey == nil { | |
| writeKeyID, err := candidateWriteKeyID(result.DesiredState) | |
| if err != nil { | |
| return false, nil, err | |
| } | |
| secret, err = rewriteWriteKeyKMSEndpoint(secret, writeKeyID, preflightKMSSocketEndpoint) | |
| if err != nil { | |
| return false, nil, fmt.Errorf("failed to rewrite preflight KMS endpoint: %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_controller.go` around lines
669 - 680, Derive the rewrite target in the PlannedKey-nil branch from
result.DesiredState, using the candidate write key rather than
latestKeyIDFromSecrets over all secrets. Add or use a helper that validates the
desired state has a write key and parses its key ID, then pass that ID to
rewriteWriteKeyKMSEndpoint; also add coverage for a newer non-migrated read-key
Secret alongside an older deployed write key.
cae2cf2 to
7153a8b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
7153a8b to
5362595
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/key_controller.go`:
- Around line 293-301: Update newKMSConfigHasher.hash to use a cryptographic
hash such as SHA-256 instead of fnv.New32 when computing the KMS configuration
identity. Preserve the existing hash input and result flow, and update all
related hash fixtures to match the new digest.
🪄 Autofix
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: 04e194eb-a0ce-48dc-ad2d-9b2925c184b8
📒 Files selected for processing (11)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_planner.gopkg/operator/encryption/controllers/encryption_planner_test.gopkg/operator/encryption/controllers/helpers_test.gopkg/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/state_controller.gopkg/operator/encryption/statemachine/transition.gopkg/operator/encryption/statemachine/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/operator/encryption/statemachine/transition_test.go
- pkg/operator/encryption/controllers.go
- pkg/operator/encryption/controllers/key_controller_test.go
- pkg/operator/encryption/statemachine/transition.go
- pkg/operator/encryption/controllers/encryption_planner_test.go
- pkg/operator/encryption/controllers/helpers_test.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/controllers/encryption_planner.go
- pkg/operator/encryption/controllers/kms_preflight_controller_test.go
- pkg/operator/encryption/controllers/kms_preflight_controller.go
5362595 to
8fa0ee6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/operator/encryption/controllers/key_controller.go (1)
293-301: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse a cryptographic hash for the KMS configuration identity.
newKMSConfigHasher.hashstill uses non-cryptographicfnv.New32. This hash decides whether a preflight result matches the current KMS configuration. Replace it with SHA-256 or stronger, and update the related hash fixtures.🤖 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 293 - 301, Update newKMSConfigHasher.hash to use a cryptographic hash such as SHA-256 instead of fnv.New32, preserving the existing KMS configuration identity and error flow. Adjust all related hash fixtures and expected values to match the new digest output.Source: Coding guidelines
🤖 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 209-216: Update the error returns in the MaterializeKey
error-handling block to wrap both underlying errors with %w: the
plannedKeyBuildError path should wrap buildErr.err, and the fallback return
should wrap err. Preserve the existing messages and stderrors.As matching
behavior so callers can inspect the original causes.
---
Duplicate comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 293-301: Update newKMSConfigHasher.hash to use a cryptographic
hash such as SHA-256 instead of fnv.New32, preserving the existing KMS
configuration identity and error flow. Adjust all related hash fixtures and
expected values to match the new digest output.
🪄 Autofix
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: 01876c0b-f618-4c8c-9a89-e18aabf2d543
📒 Files selected for processing (11)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_planner.gopkg/operator/encryption/controllers/encryption_planner_test.gopkg/operator/encryption/controllers/helpers_test.gopkg/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/state_controller.gopkg/operator/encryption/statemachine/transition.gopkg/operator/encryption/statemachine/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/operator/encryption/controllers.go
- pkg/operator/encryption/statemachine/transition_test.go
- pkg/operator/encryption/statemachine/transition.go
- pkg/operator/encryption/controllers/key_controller_test.go
- pkg/operator/encryption/controllers/helpers_test.go
- pkg/operator/encryption/controllers/kms_preflight_controller_test.go
- pkg/operator/encryption/controllers/encryption_planner_test.go
- pkg/operator/encryption/controllers/state_controller.go
- pkg/operator/encryption/controllers/kms_preflight_controller.go
- pkg/operator/encryption/controllers/encryption_planner.go
| plannedKey, err := planner.MaterializeKey(ctx, snap, plan, "") | ||
| if err != nil { | ||
| var buildErr plannedKeyBuildError | ||
| if stderrors.As(err, &buildErr) { | ||
| return fmt.Errorf("failed to create key: %v", buildErr.err) | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C4 'plannedKeyBuildError' pkg/operator/encryption --glob '*.go'Repository: openshift/library-go
Length of output: 3340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- key_controller.go ---'
sed -n '190,230p' pkg/operator/encryption/controllers/key_controller.go
printf '%s\n' '--- encryption_planner.go ---'
sed -n '105,130p;220,248p' pkg/operator/encryption/controllers/encryption_planner.go
printf '%s\n' '--- plannedKeyBuildError usages ---'
rg -n -C3 'plannedKeyBuildError|failed to create key' pkg/operator/encryption --glob '*.go'
printf '%s\n' '--- error inspection around controller call sites ---'
rg -n -C4 'MaterializeKey|CreateKey|failed to create key' pkg/operator/encryption --glob '*.go'Repository: openshift/library-go
Length of output: 37723
Wrap both underlying errors with %w. stderrors.As correctly matches plannedKeyBuildError because its Error() method has a value receiver. Use %w at lines 213 and 220 so callers can inspect the underlying causes.
🤖 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 209 -
216, Update the error returns in the MaterializeKey error-handling block to wrap
both underlying errors with %w: the plannedKeyBuildError path should wrap
buildErr.err, and the fallback return should wrap err. Preserve the existing
messages and stderrors.As matching behavior so callers can inspect the original
causes.
9054524 to
e096fcc
Compare
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| refSecret, refCM, err := referencedResourcesFromKeyState(ks, desiredProviderCfg) |
There was a problem hiding this comment.
This could be a separate refactoring that we can merge it now.
There was a problem hiding this comment.
I meant we can have a generic function that fetches the configmap and secrets from the given provider config. This function can be used by everywhere automatically.
|
|
||
| // referencedResourcesFromKeyState rebuilds the Secret/ConfigMap objects the hasher | ||
| // expects from credentials already embedded in the planned key state. | ||
| func referencedResourcesFromKeyState(ks state.KeyState, desiredProviderCfg kmsProviderConfig) (*corev1.Secret, *corev1.ConfigMap, error) { |
There was a problem hiding this comment.
I think this function is better move independent from the refactoring (like we did exporting GetDesiredState.)
There was a problem hiding this comment.
I'm not sure I understand well; but if we extract this now, it wouldn't have any use right now, correct? Something like this? referencedResourcesFromKeyState
There was a problem hiding this comment.
Ignore my comment. I missed that we rebuild from KeyState.
There was a problem hiding this comment.
xref: #2410 (comment)
I think, we don't need this function.
There was a problem hiding this comment.
the problem of removing this function and using fetchReferencedResources in ensureKMSPreflightBeforeKeyCreate is that we end up calling the api twice for the same ref cm and secret
There was a problem hiding this comment.
Why is that?. Don't we need to call generateKeySecret once (which is the only function that calls ensureKMSPreflightpassed)?
| // - (true, nil) — proceed with key creation | ||
| // - (false, nil) — preflight in progress; caller should back off | ||
| // - (false, err) — preflight failed or transient error | ||
| func (c *keyController) ensureKMSPreflightBeforeKeyCreate(ctx context.Context, currentMode state.Mode, plannedKeySecret *corev1.Secret) (bool, error) { |
There was a problem hiding this comment.
This is likely coming from another PR whose already has merged.
There was a problem hiding this comment.
As far as I understand, this function is not a replacement of generateKeySecret?. Why do we need this?
There was a problem hiding this comment.
xref: #2410 (comment)
I think, we don't need this function
| // rewriteWriteKeyKMSEndpoint, and the PlannedKey==nil rewrite in computeEncryptionConfigSecret. | ||
| const preflightKMSSocketEndpoint = "unix:///var/run/kmsplugin/kms.sock" | ||
|
|
||
| func latestKeyIDFromSecrets(keySecrets []*corev1.Secret) (uint64, error) { |
There was a problem hiding this comment.
Why do we have this function?. Doesn't plannedKey give this us for free?
There was a problem hiding this comment.
this was supposed to be temporary because the preflight controller currently only works with a fixed UDS patch (unix:///var/run/kmsplugin/kms.sock)
There was a problem hiding this comment.
yes, but we should already have latestKeyID. We don't need to iterate over the keySecrets again.
| Plugin: apiServerEncryption.KMS, | ||
| } | ||
|
|
||
| if secretName, expectedKeys, err := desiredProviderCfg.referencedSecretName(); err != nil { |
There was a problem hiding this comment.
Can't we use referencedResourcesFromKeyState automatically instead of duplicating the code?
There was a problem hiding this comment.
We can use referenced Secret/Configmap fetcher function instead of duplicating.
| snap.desiredProviderCfg = noopKMSProviderConfig{} | ||
|
|
||
| if currentMode == state.KMS { | ||
| desiredProviderCfg, err := newKMSProviderConfig(apiEncryption.KMS) |
There was a problem hiding this comment.
Previously newKMSProviderConfig is called after the progressingReason check. I think, if progressingReason is not empty, we need to short cut first to not have any behavioral changes.
| // When reusing an existing write key, rewrite its endpoint so the preflight | ||
| // checker dials the fixed socket. New planned keys already use the override. | ||
| if plannedKey == nil { | ||
| writeKeyID, err := latestKeyIDFromSecrets(result.KeySecrets) |
There was a problem hiding this comment.
| writeKeyID, err := latestKeyIDFromSecrets(result.KeySecrets) | |
| for _, grState := range snap.DesiredBeforePlan { | |
| if grState.HasWriteKey() { | |
| writeKeyID, _ := state.NameToKeyID(grState.WriteKey.Key.Name) | |
| // use writeKeyID | |
| break | |
| } | |
| } | |
|
Changes, with this planner and computer approach, make encryption controllers more elegant. I dropped a few comments for some refactorings that will be even useful without the changes here. So I think, we can merge those refactorings first. So this will reduce the number of changed lines in this PR. |
e096fcc to
4af2450
Compare
| // For KMS mode it also computes the config hash (from the referenced Secret and | ||
| // ConfigMap it already reads) and gates on the KMS preflight check. The boolean | ||
| // return value signals the outcome: | ||
| // plannedKeySecret is the in-memory key secret from MaterializeKey; its embedded plugin credentials are reused for hashing so we do not re-fetch openshift-config resources. |
There was a problem hiding this comment.
I think, instead of rebuilding the credentials from KeyState (which is done #2420), we should always fetch them from API Server to work with up to date data. Because if the content of referenced Secret/Configmap changes, hash must mismatch in preflight (i.e. ensureKMSPrelightPassed). So that process needs to restart with the new hashes.
Rebuilding referenced data from KeyState may work on stale data which is not ideal.
So I think, this #2418 is the right refactoring. This function can call fetchReferencedResources. Am I missing something?. Please let me know your thoughts.
There was a problem hiding this comment.
We just need like this;
resources := newCoreClientKMSConfigHasherResourceProvider(c.secretsClient, c.configMapsClient)
hasher, _ := newKMSConfigHasher(snap.desiredProviderCfg, resources, openshiftConfigNS)
configHash, _ := hasher.hash(ctx)
return c.ensureKMSPreflightPassed(ctx, configHash)If hash mismatches, process will restart.
Extract a reusable helper that loads the Secret/ConfigMap named by a KMS provider config (with required-key validation).
ee67d95 to
88be3e3
Compare
Pull planNextEncryptionKey and buildEncryptionKey* out of the key-controller control flow so they can be reused in place by EncryptionPlanner without cross-file moves later.
Introduce Load, PlanNextKey, MaterializeKey, and ComputeConfig on top of the shared helpers already living in the key controller package. No controller call sites switch yet.
88be3e3 to
6f8557c
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. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes