WIP: compute encryption-config for preflight - #2373
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 centralizes encryption-key planning and Secret construction. Key creation uses these helpers. KMS preflight now computes and deploys the next-key encryption configuration. Desired encryption-state computation is exported, with expanded preflight tests. ChangesEncryption rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KMSPreflightController
participant EncryptionDeployer
participant SecretInformer
participant EncryptionKeyHelpers
participant GetDesiredEncryptionState
KMSPreflightController->>EncryptionDeployer: check encryption-state convergence
EncryptionDeployer-->>KMSPreflightController: deployed encryption configuration
KMSPreflightController->>SecretInformer: read existing key Secrets
SecretInformer-->>KMSPreflightController: key Secret data
KMSPreflightController->>EncryptionKeyHelpers: plan and build next KMS key
EncryptionKeyHelpers-->>KMSPreflightController: simulated key Secret
KMSPreflightController->>GetDesiredEncryptionState: derive desired encryption state
GetDesiredEncryptionState-->>KMSPreflightController: desired encryption state
KMSPreflightController->>EncryptionDeployer: deploy encryption-config Secret
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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/key_controller.go`:
- Around line 214-216: Update the EncryptionKeyCreateFailed warning in the
createErr handling branch to log createErr instead of err, while preserving the
existing event message, return createErr behavior, and surrounding
generateKeySecret flow.
🪄 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: 3dabf247-0dc6-4ff5-9a83-9c4d3be9df94
📒 Files selected for processing (8)
pkg/operator/encryption/controllers/encryption_key_helpers.gopkg/operator/encryption/controllers/encryption_key_helpers_test.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/kms/constants.gopkg/operator/encryption/statemachine/transition.gopkg/operator/encryption/statemachine/transition_test.go
| if createErr != nil { | ||
| syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, err) | ||
| return createErr |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Warning event logs err (nil) instead of createErr.
At this point err is the (nil) result from generateKeySecret that already passed the Line 207 guard, so the EncryptionKeyCreateFailed event will render <nil> instead of the real create failure. Return value is fine; only the diagnostic is lost.
🐛 Proposed fix
if createErr != nil {
- syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, err)
+ syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, createErr)
return createErr
}📝 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.
| if createErr != nil { | |
| syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, err) | |
| return createErr | |
| if createErr != nil { | |
| syncContext.Recorder().Warningf("EncryptionKeyCreateFailed", "Secret %q failed to create: %v", keySecret.Name, createErr) | |
| return createErr |
🤖 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 214 -
216, Update the EncryptionKeyCreateFailed warning in the createErr handling
branch to log createErr instead of err, while preserving the existing event
message, return createErr behavior, and surrounding generateKeySecret flow.
| // requeue is true when the API server revisions have not converged yet (mirrors | ||
| // statemachine.GetEncryptionConfigAndState's "APIServerRevisionNotConverged" case); | ||
| // callers should requeue and retry later rather than treat this as an error. | ||
| func (c *kmsPreflightController) computeEncryptionConfigSecret(ctx context.Context) (requeue bool, secret *corev1.Secret, err error) { |
There was a problem hiding this comment.
I'm pretty sure this went past you, but I've added a small helper for testing the drift and preflight deployer:
https://github.com/openshift/library-go/blob/master/test/library/encryption/preflight_deploy.go#L147
do you mind to refactor a function that we can reuse there?
There was a problem hiding this comment.
oh, sorry, I missed this comment. Do you still need this?
97da54e to
3882161
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/kms/preflight/deployer_test.go (1)
430-432: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the failing action in the assertion diagnostic.
This assertion checks
actions[10], but the failure message prints the type ofactions[4], which can mislead debugging. Useactions[10]instead.Proposed fix
- t.Fatalf("expected CreateAction, got %T", actions[4]) + t.Fatalf("expected CreateAction, got %T", actions[10])🤖 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/kms/preflight/deployer_test.go` around lines 430 - 432, Update the CreateAction type assertion diagnostic in the relevant test to report the type of actions[10], matching the action being validated, instead of actions[4].
🧹 Nitpick comments (2)
pkg/operator/encryption/controllers/encryption_key_helpers.go (2)
45-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the reason-collapsing rationale.
The logic that collapses per-resource reasons into a single shared reason when they all match (vs. keeping resource-prefixed reasons otherwise) is subtle and easy to break during future edits to this newly-shared helper. A short comment explaining the intent (e.g., "avoid repeating the same reason once per resource when it's the same event affecting all GRs") would help.
🤖 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_key_helpers.go` around lines 45 - 74, Add a concise comment immediately above the reason-collapsing condition in the shared helper, explaining that identical reasons are reduced to one shared reason to avoid repeating the same event for every resource, while differing reasons remain resource-prefixed.
82-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider bundling the trailing string params into a struct.
Both
buildEncryptionKeyStateandbuildEncryptionKeySecrettake 10 positional parameters, including three consecutivestringargs (internalReason,externalReason,kmsEndpointOverride). Since Go has no named arguments and all three are the same type, it's easy for a future call site to transpose them silently — the compiler won't catch it. These helpers are already called from at least two different controllers (key_controller.go and kms_preflight_controller.go per the provided context), so the risk compounds with each new caller.♻️ Suggested direction
+type keySecretOptions struct { + InternalReason string + ExternalReason string + KMSEndpointOverride string +} + func buildEncryptionKeyState( ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, desiredProviderCfg kmsProviderConfig, secretClient corev1client.SecretsGetter, configMapClient corev1client.ConfigMapsGetter, - internalReason string, - externalReason string, - kmsEndpointOverride string, + opts keySecretOptions, ) (state.KeyState, error) {🤖 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_key_helpers.go` around lines 82 - 191, Bundle the trailing string parameters into a named options or metadata struct and update both buildEncryptionKeyState and buildEncryptionKeySecret to accept that struct instead of separate internalReason, externalReason, and kmsEndpointOverride arguments. Update every caller, including the key and KMS preflight controller paths, to populate the named fields explicitly while preserving the existing values and 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_key_helpers.go`:
- Around line 123-157: Wrap errors returned by
desiredProviderCfg.referencedSecretName() and referencedConfigMapName() with
context identifying the referenced secret or ConfigMap resolution, and wrap
failures from ks.KMS.PluginSecretData.Set and PluginConfigMapData.Set with the
corresponding resource name and key. Preserve the existing error propagation
while using %w so callers retain the underlying errors.
---
Outside diff comments:
In `@pkg/operator/encryption/kms/preflight/deployer_test.go`:
- Around line 430-432: Update the CreateAction type assertion diagnostic in the
relevant test to report the type of actions[10], matching the action being
validated, instead of actions[4].
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/encryption_key_helpers.go`:
- Around line 45-74: Add a concise comment immediately above the
reason-collapsing condition in the shared helper, explaining that identical
reasons are reduced to one shared reason to avoid repeating the same event for
every resource, while differing reasons remain resource-prefixed.
- Around line 82-191: Bundle the trailing string parameters into a named options
or metadata struct and update both buildEncryptionKeyState and
buildEncryptionKeySecret to accept that struct instead of separate
internalReason, externalReason, and kmsEndpointOverride arguments. Update every
caller, including the key and KMS preflight controller paths, to populate the
named fields explicitly while preserving the existing values and 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: 17872bf1-c780-446c-a7e7-12cb9d5aecd3
📒 Files selected for processing (9)
pkg/operator/encryption/controllers/encryption_key_helpers.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/kms/preflight/cmd.gopkg/operator/encryption/kms/preflight/deployer_test.gopkg/operator/encryption/kms/preflight_endpoint.gopkg/operator/encryption/statemachine/transition.gopkg/operator/encryption/statemachine/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/encryption/statemachine/transition_test.go
- pkg/operator/encryption/statemachine/transition.go
- pkg/operator/encryption/controllers/kms_preflight_controller.go
- pkg/operator/encryption/controllers/kms_preflight_controller_test.go
- pkg/operator/encryption/controllers/key_controller.go
| if secretName, expectedKeys, err := desiredProviderCfg.referencedSecretName(); err != nil { | ||
| return state.KeyState{}, err | ||
| } else if len(secretName) > 0 { | ||
| refSecret, err := secretClient.Secrets(openshiftConfigNS).Get(ctx, secretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return state.KeyState{}, fmt.Errorf("failed to get secret %s in %s: %w", secretName, openshiftConfigNS, err) | ||
| } | ||
| for _, key := range expectedKeys { | ||
| v, ok := refSecret.Data[key] | ||
| if !ok { | ||
| return state.KeyState{}, fmt.Errorf("secret %s in %s is missing required key %q", secretName, openshiftConfigNS, key) | ||
| } | ||
| if err := ks.KMS.PluginSecretData.Set(secretName, key, v); err != nil { | ||
| return state.KeyState{}, err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if cmName, expectedKeys, err := desiredProviderCfg.referencedConfigMapName(); err != nil { | ||
| return state.KeyState{}, err | ||
| } else if len(cmName) > 0 { | ||
| refCM, err := configMapClient.ConfigMaps(openshiftConfigNS).Get(ctx, cmName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return state.KeyState{}, fmt.Errorf("failed to get configmap %s in %s: %w", cmName, openshiftConfigNS, err) | ||
| } | ||
| for _, key := range expectedKeys { | ||
| v, ok := refCM.Data[key] | ||
| if !ok { | ||
| return state.KeyState{}, fmt.Errorf("configmap %s in %s is missing required key %q", cmName, openshiftConfigNS, key) | ||
| } | ||
| if err := ks.KMS.PluginConfigMapData.Set(cmName, key, []byte(v)); err != nil { | ||
| return state.KeyState{}, err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add context to bubbled-up errors.
Errors from desiredProviderCfg.referencedSecretName()/referencedConfigMapName() (Lines 123-125, 141-143) and from PluginSecretData.Set/PluginConfigMapData.Set (Lines 135-137, 153-155) are returned bare, unlike the sibling Get() calls a few lines above/below that wrap errors with fmt.Errorf("failed to get secret %s in %s: %w", ...). This makes failures in these paths harder to diagnose in logs.
🐛 Proposed fix
- if secretName, expectedKeys, err := desiredProviderCfg.referencedSecretName(); err != nil {
- return state.KeyState{}, err
+ if secretName, expectedKeys, err := desiredProviderCfg.referencedSecretName(); err != nil {
+ return state.KeyState{}, fmt.Errorf("failed to resolve referenced secret name: %w", err)
} else if len(secretName) > 0 {
...
- if err := ks.KMS.PluginSecretData.Set(secretName, key, v); err != nil {
- return state.KeyState{}, err
+ if err := ks.KMS.PluginSecretData.Set(secretName, key, v); err != nil {
+ return state.KeyState{}, fmt.Errorf("failed to set plugin secret data %s/%s: %w", secretName, key, err)
}(similarly for the configmap branch)
🤖 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_key_helpers.go` around lines
123 - 157, Wrap errors returned by desiredProviderCfg.referencedSecretName() and
referencedConfigMapName() with context identifying the referenced secret or
ConfigMap resolution, and wrap failures from ks.KMS.PluginSecretData.Set and
PluginConfigMapData.Set with the corresponding resource name and key. Preserve
the existing error propagation while using %w so callers retain the underlying
errors.
3882161 to
1d4924b
Compare
…hine Export the desired encryption state helper so other encryption controllers can reuse the same state transition logic instead of reimplementing it.
1d4924b to
51a718f
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. |
Move KMS key planning and key secret construction into reusable helpers so the key controller and preflight flow can build the same next-key shape from one implementation.
Build the candidate encryption config that would result from the next KMS key before launching preflight so the checker validates the exact secret and plugin data that rollout will use.
51a718f to
96361f8
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.
🧹 Nitpick comments (3)
pkg/operator/encryption/controllers/encryption_key_helpers_test.go (2)
15-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
planNextEncryptionKey.The new file adds
planNextEncryptionKey, which aggregates per-resource reasons, collapses a common reason, and picks the maximum next key ID. No test in this file exercises it. Add cases for a single resource, multiple resources with the same reason, and multiple resources with different reasons and different latest key IDs.
Do you want me to draft these tests?🤖 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_key_helpers_test.go` around lines 15 - 99, Add table-driven coverage for planNextEncryptionKey, covering one resource, multiple resources sharing a reason, and multiple resources with distinct reasons and latest key IDs. Assert the aggregated reason output, common-reason collapse behavior, and selection of the maximum next key ID.
72-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test or split the success subtest.
TestBuildEncryptionKeyStateMissingRefsnow contains a success-path subtest that asserts returned refs and the endpoint override. Move that subtest to a separate test function, for exampleTestBuildEncryptionKeyStateReturnsRefs, so the test name matches the behavior under test.🤖 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_key_helpers_test.go` around lines 72 - 98, Move the “returns fetched refs for hasher reuse” success subtest out of TestBuildEncryptionKeyStateMissingRefs into a separate test function such as TestBuildEncryptionKeyStateReturnsRefs. Preserve its existing assertions for refSecret, refCM, and the KMS endpoint override, while leaving only missing-reference behavior in the original test.pkg/operator/encryption/controllers/key_controller.go (1)
238-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the reasons list for the event message.
keyPlan.reasonsis a[]string. With%qthe event message renders as["a" "b"]. The previous message used a joined string. UsekeyPlan.internalReason, or join the slice, to keep the message readable.♻️ Proposed change
- syncContext.Recorder().Eventf("EncryptionKeyCreated", "Secret %q successfully created: %q", keySecret.Name, keyPlan.reasons) + syncContext.Recorder().Eventf("EncryptionKeyCreated", "Secret %q successfully created: %q", keySecret.Name, keyPlan.internalReason)🤖 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` at line 238, Update the Eventf call in the key creation flow to format the reasons as a readable string rather than passing the []string keyPlan.reasons with %q; use keyPlan.internalReason or join keyPlan.reasons before interpolation, while preserving the existing event message context.
🤖 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/encryption_key_helpers_test.go`:
- Around line 15-99: Add table-driven coverage for planNextEncryptionKey,
covering one resource, multiple resources sharing a reason, and multiple
resources with distinct reasons and latest key IDs. Assert the aggregated reason
output, common-reason collapse behavior, and selection of the maximum next key
ID.
- Around line 72-98: Move the “returns fetched refs for hasher reuse” success
subtest out of TestBuildEncryptionKeyStateMissingRefs into a separate test
function such as TestBuildEncryptionKeyStateReturnsRefs. Preserve its existing
assertions for refSecret, refCM, and the KMS endpoint override, while leaving
only missing-reference behavior in the original test.
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Line 238: Update the Eventf call in the key creation flow to format the
reasons as a readable string rather than passing the []string keyPlan.reasons
with %q; use keyPlan.internalReason or join keyPlan.reasons before
interpolation, while preserving the existing event message context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: acdfc66b-d5f8-4e43-b6eb-f75d957894c5
📒 Files selected for processing (8)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_key_helpers.gopkg/operator/encryption/controllers/encryption_key_helpers_test.gopkg/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.gopkg/operator/encryption/statemachine/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/encryption/statemachine/transition.go
- pkg/operator/encryption/statemachine/transition_test.go
- pkg/operator/encryption/controllers.go
- pkg/operator/encryption/controllers/kms_preflight_controller.go
- pkg/operator/encryption/controllers/kms_preflight_controller_test.go
|
@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
Tests