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
| // 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
9054524 to
e096fcc
Compare
| // 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.
| // 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.
88be3e3 to
6f8557c
Compare
| // - (secret, true, nil) — preflight passed; caller should persist the key. | ||
| // - (nil, false, nil) — preflight still in progress; caller should back off. | ||
| // - (nil, false, err) — preflight failed or transient error; caller should surface it. | ||
| func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, desiredProviderCfg kmsProviderConfig, internalReason, externalReason string) (*corev1.Secret, bool, error) { |
There was a problem hiding this comment.
buildEncryptionKeyState accesses refSecret, refCM already. If it returns those values as well, this function will be simplified (and we don't need round-tripping);
func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, desiredProviderCfg kmsProviderConfig, internalReason, externalReason
string) (*corev1.Secret, bool, error) {
ks, refSecret, refCM, err := buildEncryptionKeyState(ctx, keyID, currentMode, apiServerEncryption, desiredProviderCfg, c.secretClient, c.configMapClient, internalReason, externalReason, "")
if err != nil {
return nil, false, err
}
if currentMode == state.KMS {
resources := &prefetchedKMSConfigHasherResourceProvider{secret: refSecret, configMap: refCM}
hasher, err := newKMSConfigHasher(desiredProviderCfg, resources, openshiftConfigNS)
if err != nil {
return nil, false, fmt.Errorf("failed to create KMS config hasher: %w", err)
}
configHash, err := hasher.hash(ctx)
if err != nil {
return nil, false, fmt.Errorf("failed to compute KMS config hash: %w", err)
}
preflightPassed, err := c.ensureKMSPreflightPassed(ctx, configHash)
if err != nil {
return nil, false, err
}
if !preflightPassed {
return nil, false, nil
}
}
secret, err := secrets.FromKeyState(c.instanceName, ks)
if err != nil {
return nil, false, err
}
return secret, true, nil
}There was a problem hiding this comment.
preflight can ignore refCM and refSecret.
There was a problem hiding this comment.
I think that's a great suggestion
There was a problem hiding this comment.
Would it make sense having this second commit in separate pr, and if it looks good to me, merging it?
openshift-ci
Bot
added
the
needs-rebase
6f8557c to
4e8ce2c
Compare
openshift-ci
Bot
removed
the
needs-rebase
4e8ce2c to
271e0d3
Compare
271e0d3 to
7764bcc
Compare
| if snap == nil { | ||
| return nil, fmt.Errorf("snapshot is required") | ||
| } | ||
| if len(snap.ProgressingReason) > 0 { |
There was a problem hiding this comment.
We have decided that preflight controller should continue progress even if there isn't any convergence between apiservers (i.e. progressingreason > 0). So we should remove this. Key controller and state controller should continue requeueing separately in their logic..
There was a problem hiding this comment.
good point, I attempted to fix this in a separate commit: 88188ee. If we're happy with it, I can incorporate the changes into the second commit
| snap.KeySecrets = keySecrets | ||
| snap.ProgressingReason = progressingReason | ||
|
|
||
| if snap.modeResolved && len(progressingReason) == 0 && snap.CurrentMode == state.KMS { |
There was a problem hiding this comment.
Logic about progressionReason varies on each caller. So we can update this;
if snap.CurrentMode == state.KMS {
snap.desiredProviderCfg, err = newKMSProviderConfig(snap.APIEncryption.KMS)
if err != nil {
return nil, err
}
}
| @@ -0,0 +1,114 @@ | |||
| package controllers | |||
There was a problem hiding this comment.
Do we need this abstraction (encryption_computer)?. Preflight and key controller can directly call: planner.Load -> PlanNextKey -> MaterializeKey -> ComputeConfig. It would be easier to read the logic from each controller.
Moreover, we wouldn't maintain another layer which may not be needed.
There was a problem hiding this comment.
I agree, I think we don't need it; this can be simplified into a method in kmsPreflightController. I'll leave it as is for now and until we're happy with the planner
| } | ||
|
|
||
| keyPlan, err := planNextEncryptionKey(desiredEncryptionState, currentMode, externalReason, encryptedGRs, desiredProviderCfg) | ||
| plan, err := planner.PlanNextKey(snap) |
There was a problem hiding this comment.
this is essentially the same thing: planner.PlanNextKey(snap) == planNextEncryptionKey(desiredEncryptionState, currentMode, externalReason, encryptedGRs, desiredProviderCfg). The only difference is that the former returns an exported type
| if snap == nil { | ||
| return nil, fmt.Errorf("snapshot is required") | ||
| } | ||
| if len(snap.ProgressingReason) > 0 { |
There was a problem hiding this comment.
good point, I attempted to fix this in a separate commit: 88188ee. If we're happy with it, I can incorporate the changes into the second commit
| @@ -0,0 +1,114 @@ | |||
| package controllers | |||
There was a problem hiding this comment.
I agree, I think we don't need it; this can be simplified into a method in kmsPreflightController. I'll leave it as is for now and until we're happy with the planner
| // LoadForPreflight resolves encryption mode and key-planning state for KMS preflight. | ||
| // Unlike Load, when deployer convergence is still in progress it still loads persisted | ||
| // key secrets and computes DesiredBeforePlan so PlanNextKey and ComputeConfig can proceed. | ||
| func (p *EncryptionPlanner) LoadForPreflight(ctx context.Context, encryptedGRs []schema.GroupResource) (*KeyPlanningSnapshot, error) { |
There was a problem hiding this comment.
I think, this may contradict our goal that key controller and preflight use the exact same flow to generate the same outcome. Now preflight diverges from the key controller.
Maybe we can have a flag in Snapshot like ignore progress reason, etc.. This flag can be used to what to do in Load function (short cut or proceed) instead of extending it to newer LoadForPreflight.
There was a problem hiding this comment.
this is a good point. I dropped that additional commit and implemented your suggestion of adding a boolean in KeyPlanningSnapshot, PTAL.
as we dicussed offline, after implementing it, I found it a bit clumsy because that type was supposed to be an output (snapshot), but now it contains a field that's not part of the snapshot and is used as an input.
I experimented with adding a ListOptions to the Load() method and I think it looks OK. I left it in a separate commit to make it easier to delete, PTAL: e4817cd
There was a problem hiding this comment.
I experimented with adding a ListOptions to the Load() method and I think it looks OK.
I think, that looks good.
| func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext factory.SyncContext, encryptedGRs []schema.GroupResource) error { | ||
| currentMode, externalReason, apiEncryptionConfiguration, err := getCurrentModeReasonAndEncryptionConfig(ctx, c.apiServerClient, c.operatorClient, c.unsupportedConfigPrefix) | ||
| planner := NewEncryptionPlanner(c.instanceName, c.unsupportedConfigPrefix, c.deployer, c.secretClient, c.configMapClient, c.apiServerClient, c.operatorClient, c.encryptionSecretSelector) | ||
| snap, err := planner.Load(ctx, encryptedGRs) |
There was a problem hiding this comment.
nit: Would it make sense NewEncryptionPlanner returns an interface with 5 functions Load, PlanNextKey, ComputeConfig, LoadState, MaterializeKey instead of concrete type?. It is totally up to your taste.
88188ee to
91220d6
Compare
| desiredProviderCfg: noopKMSProviderConfig{}, | ||
| } | ||
|
|
||
| if currentMode == state.KMS { |
There was a problem hiding this comment.
Current code does not populate desiredProviderCfg, if progressingReason is non-empty. But this is benign in-memory operation. So we should be fine. Also, we will likely use these populated data in in-place field updates.
There was a problem hiding this comment.
ack, thanks for pointing that out
| } | ||
|
|
||
| if len(stateSnap.ProgressingReason) > 0 && opts.ListKeysWhileProgressing { | ||
| keySecrets, err := secrets.ListKeySecrets(ctx, p.secretClient, p.encryptionSecretSelector) |
There was a problem hiding this comment.
GetEncryptionConfigAndState function in LoadState short cuts, when there is no convergence. Therefore, we are doing this extra operation for preflight controller to have the necessary data. I think, it would be great if we document the reasons why we do this.
There was a problem hiding this comment.
agreed; addded a comment
| // deployer has not converged. Default false so the key controller does | ||
| // not List during API-server rollouts. Preflight sets this so PlanNextKey | ||
| // can run. ProgressingReason is still populated in either case. | ||
| ListKeysWhileProgressing bool |
There was a problem hiding this comment.
I think, this is the way of idiomatic k8s, just as we use from client-go. So, this looks good to me.
| return out, nil | ||
| } | ||
|
|
||
| desiredState := statemachine.GetDesiredEncryptionState(state.CurrentConfig, keySecrets, state.EncryptedGRs) |
There was a problem hiding this comment.
We don't need to call that function, if there is no plannedKey. What about something like this:
out.DesiredState = snap.DesiredBeforePlan
if opts.PlannedKey != nil && opts.PlannedKey.Secret != nil {
desiredState := statemachine.GetDesiredEncryptionState(state.CurrentConfig, keySecrets, state.EncryptedGRs)
out.DesiredState = desiredState
}
cfg, err := encryptiondata.FromEncryptionState(out.DesiredState)
if err != nil {
return nil, fmt.Errorf("failed to build encryption config: %w", err)
}
out.EncryptionConfig = cfgThere was a problem hiding this comment.
took the suggestion, ComputeConfig now reuses snap.DesiredBeforePlan that was computed before
e4817cd to
bd3857f
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.
Preflight now builds the encryption-config secret itself (Load, PlanNextKey, MaterializeKey, ComputeConfig) instead of only delegating to EncryptionConfigurationComputer. The computer is still accepted for existing operators and tests until that API is removed.
2601dca to
f085f00
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