[WIP] pkg/operator/encryption/controllers: NewKMSPreflightController scaffolding#2170
Draft
p0lyn0mial wants to merge 3 commits into
Draft
[WIP] pkg/operator/encryption/controllers: NewKMSPreflightController scaffolding#2170p0lyn0mial wants to merge 3 commits into
p0lyn0mial wants to merge 3 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds a new KMS preflight controller constructor and implementation that wires APIServer and operator informers, evaluates operator preconditions and provider readiness, executes preflight checks (stubbed), and updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller (kmsPreflightController)
participant Preconditions as PreconditionsFn
participant Provider as Provider
participant Preflight as runPreflightChecks
participant Operator as OperatorClient/StatusApply
Controller->>Preconditions: preconditionsFulfilledFn(ctx)
Controller->>Provider: ShouldRunEncryptionControllers(ctx)
alt preconditions/provider not ready or error
Controller->>Operator: apply degraded = False / clear or return
else ready
Controller->>Preflight: runPreflightChecks(ctx)
alt preflight fails
Controller->>Operator: apply degraded = True (Error, message)
else preflight succeeds
Controller->>Operator: apply degraded = False
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/kms_preflight_controller.go (1)
27-28: Replace TODO comment with exported API doc before merge.On Line 27, convert the placeholder into a real GoDoc comment for
NewKMSPreflightControllerto document intent and inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/kms_preflight_controller.go` around lines 27 - 28, Replace the TODO comment above NewKMSPreflightController with a proper GoDoc: write an exported comment that starts with "NewKMSPreflightController" and briefly describes what the constructor returns, its purpose, and the meaning of its input parameters (e.g., the controller's responsibilities and any important preconditions for the arguments); ensure the comment follows GoDoc style (complete sentence, starts with the function name) so tools and consumers can discover the API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 91-94: The method kmsPreflightController.runPreflightChecks
currently always returns nil which masks missing validation; replace the
placeholder success with a failing error until real preflight logic is
implemented so the controller fails closed. Update runPreflightChecks to return
a clear sentinel error (e.g. errors.New("preflight checks not implemented")) and
ensure callers of runPreflightChecks (the kmsPreflightController
reconcile/health logic) treat any non-nil error as a failed preflight so the
controller reports unhealthy instead of appearing healthy.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 27-28: Replace the TODO comment above NewKMSPreflightController
with a proper GoDoc: write an exported comment that starts with
"NewKMSPreflightController" and briefly describes what the constructor returns,
its purpose, and the meaning of its input parameters (e.g., the controller's
responsibilities and any important preconditions for the arguments); ensure the
comment follows GoDoc style (complete sentence, starts with the function name)
so tools and consumers can discover the API.
🪄 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: Pro Plus
Run ID: a0c7ca5a-b59a-4036-bf42-1bee5bc65cd7
📒 Files selected for processing (1)
pkg/operator/encryption/controllers/kms_preflight_controller.go
p0lyn0mial
force-pushed
the
kms-preflight-checks-part2
branch
from
41bf1bc to
687510a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 65-67: The deferred status-write currently clobbers any earlier
reconcile error by assigning applyError to err; change this so we preserve both
errors (e.g., use errors.Join or wrap the original err with applyError) instead
of replacing it. Locate the operatorClient.ApplyOperatorStatus call in
kms_preflight_controller.go and, when applyError != nil, set err =
errors.Join(err, applyError) (or err = fmt.Errorf("%w; status apply failed: %v",
err, applyError) if errors.Join isn't available) so the original reconcile
failure and the status-write failure are both preserved.
- Around line 70-76: The current early-return on
shouldRunEncryptionController(...) errors drops updating degradedCondition
(setting it to nil) which leaves stale health; instead, when
shouldRunEncryptionController returns an error, set
KMSPreflightControllerDegraded to True on degradedCondition (use the existing
degradedCondition variable returned/managed in this scope), include the error
details in the condition message/reason (e.g., call
WithStatus(operatorv1.ConditionTrue) and attach err.Error() or a concise
reason), ensure that updated degradedCondition is written back to the operator
status before returning the err so the operator reflects the current failure of
shouldRunEncryptionController.
🪄 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: Pro Plus
Run ID: 1770302b-4a21-4ea2-8d61-896989389b70
📒 Files selected for processing (1)
pkg/operator/encryption/controllers/kms_preflight_controller.go
Comment on lines
+65
to
+67
| if applyError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status); applyError != nil { | ||
| err = applyError | ||
| } |
There was a problem hiding this comment.
Preserve the original reconcile error when status apply also fails.
The deferred write currently overwrites any earlier error. If preflight evaluation fails and ApplyOperatorStatus(...) also fails, callers only see the status-write error and lose the root cause. Prefer wrapping/joining instead of replacing err.
Suggested change
status := applyoperatorv1.OperatorStatus().WithConditions(degradedCondition)
if applyError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status); applyError != nil {
- err = applyError
+ if err == nil {
+ err = applyError
+ } else {
+ err = fmt.Errorf("%v; failed to apply operator status: %w", err, applyError)
+ }
}
}()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go` around lines
65 - 67, The deferred status-write currently clobbers any earlier reconcile
error by assigning applyError to err; change this so we preserve both errors
(e.g., use errors.Join or wrap the original err with applyError) instead of
replacing it. Locate the operatorClient.ApplyOperatorStatus call in
kms_preflight_controller.go and, when applyError != nil, set err =
errors.Join(err, applyError) (or err = fmt.Errorf("%w; status apply failed: %v",
err, applyError) if errors.Join isn't available) so the original reconcile
failure and the status-write failure are both preserved.
Comment on lines
+70
to
+76
| if ready, err := shouldRunEncryptionController(c.operatorClient, c.preconditionsFulfilledFn, c.provider.ShouldRunEncryptionControllers); err != nil || !ready { | ||
| if err != nil { | ||
| degradedCondition = nil | ||
| } else { | ||
| degradedCondition = degradedCondition.WithStatus(operatorv1.ConditionFalse) | ||
| } | ||
| return err // we will get re-kicked when the operator status updates |
There was a problem hiding this comment.
Report readiness-evaluation failures in KMSPreflightControllerDegraded.
On Line 72, dropping the condition update leaves the previous value in place. After a prior preflight failure/success, a later shouldRunEncryptionController(...) error will keep reporting stale health instead of the current controller failure. This path should write KMSPreflightControllerDegraded=True with the error details rather than setting degradedCondition to nil.
Suggested change
if ready, err := shouldRunEncryptionController(c.operatorClient, c.preconditionsFulfilledFn, c.provider.ShouldRunEncryptionControllers); err != nil || !ready {
if err != nil {
- degradedCondition = nil
+ degradedCondition = degradedCondition.
+ WithStatus(operatorv1.ConditionTrue).
+ WithReason("Error").
+ WithMessage(err.Error())
} else {
degradedCondition = degradedCondition.WithStatus(operatorv1.ConditionFalse)
}
return err // we will get re-kicked when the operator status updates
}📝 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.
Suggested change
| if ready, err := shouldRunEncryptionController(c.operatorClient, c.preconditionsFulfilledFn, c.provider.ShouldRunEncryptionControllers); err != nil || !ready { | |
| if err != nil { | |
| degradedCondition = nil | |
| } else { | |
| degradedCondition = degradedCondition.WithStatus(operatorv1.ConditionFalse) | |
| } | |
| return err // we will get re-kicked when the operator status updates | |
| if ready, err := shouldRunEncryptionController(c.operatorClient, c.preconditionsFulfilledFn, c.provider.ShouldRunEncryptionControllers); err != nil || !ready { | |
| if err != nil { | |
| degradedCondition = degradedCondition. | |
| WithStatus(operatorv1.ConditionTrue). | |
| WithReason("Error"). | |
| WithMessage(err.Error()) | |
| } else { | |
| degradedCondition = degradedCondition.WithStatus(operatorv1.ConditionFalse) | |
| } | |
| return err // we will get re-kicked when the operator status updates | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go` around lines
70 - 76, The current early-return on shouldRunEncryptionController(...) errors
drops updating degradedCondition (setting it to nil) which leaves stale health;
instead, when shouldRunEncryptionController returns an error, set
KMSPreflightControllerDegraded to True on degradedCondition (use the existing
degradedCondition variable returned/managed in this scope), include the error
details in the condition message/reason (e.g., call
WithStatus(operatorv1.ConditionTrue) and attach err.Error() or a concise
reason), ensure that updated degradedCondition is written back to the operator
status before returning the err so the operator reflects the current failure of
shouldRunEncryptionController.
…nt the design of the controller
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/operator/encryption/controllers/kms_preflight_controller.go (2)
126-132:⚠️ Potential issue | 🟠 MajorReport readiness-evaluation failures in
EncryptionKMSPreflightControllerDegraded.When
shouldRunEncryptionController(...)returns an error, settingdegradedCondition = nil(line 128) skips the status update entirely. This leaves stale state from a prior sync instead of reflecting the current failure. This path should writeEncryptionKMSPreflightControllerDegraded=Truewith the error details.Suggested fix
if ready, err := shouldRunEncryptionController(c.operatorClient, c.preconditionsFulfilledFn, c.provider.ShouldRunEncryptionControllers); err != nil || !ready { if err != nil { - degradedCondition = nil + degradedCondition = degradedCondition. + WithStatus(operatorv1.ConditionTrue). + WithReason("Error"). + WithMessage(err.Error()) } else { degradedCondition = degradedCondition.WithStatus(operatorv1.ConditionFalse) } return err // we will get re-kicked when the operator status updates }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/kms_preflight_controller.go` around lines 126 - 132, The current error path in shouldRunEncryptionController(...) clears degradedCondition (degradedCondition = nil) which prevents updating EncryptionKMSPreflightControllerDegraded; instead, when shouldRunEncryptionController returns an error you should set degradedCondition = degradedCondition.WithStatus(operatorv1.ConditionTrue).WithMessage(err.Error()) (or equivalent) so the controller status writes EncryptionKMSPreflightControllerDegraded=True with the error details before returning err; update the branch in the function containing shouldRunEncryptionController, degradedCondition and the return to set and retain the degradedCondition with the error info rather than nil so the failure is reflected in status.
121-123:⚠️ Potential issue | 🟡 MinorPreserve the original reconcile error when status apply also fails.
The deferred write currently overwrites any earlier error. If preflight evaluation fails and
ApplyOperatorStatus(...)also fails, callers only see the status-write error and lose the root cause.Suggested fix using error joining
status := applyoperatorv1.OperatorStatus().WithConditions(degradedCondition) if applyError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status); applyError != nil { - err = applyError + if err == nil { + err = applyError + } else { + err = fmt.Errorf("%v; failed to apply operator status: %w", err, applyError) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/encryption/controllers/kms_preflight_controller.go` around lines 121 - 123, The deferred status-write currently overwrites the original reconcile error stored in err when c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status) returns applyError; change the defer logic to preserve and combine errors instead of replacing them — e.g., if both err and applyError are non-nil set err = errors.Join(err, applyError), if only applyError is non-nil set err = applyError, otherwise leave err unchanged; update imports to include "errors" if needed so the ApplyOperatorStatus defer merges the errors rather than discarding the original reconcile failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 126-132: The current error path in
shouldRunEncryptionController(...) clears degradedCondition (degradedCondition =
nil) which prevents updating EncryptionKMSPreflightControllerDegraded; instead,
when shouldRunEncryptionController returns an error you should set
degradedCondition =
degradedCondition.WithStatus(operatorv1.ConditionTrue).WithMessage(err.Error())
(or equivalent) so the controller status writes
EncryptionKMSPreflightControllerDegraded=True with the error details before
returning err; update the branch in the function containing
shouldRunEncryptionController, degradedCondition and the return to set and
retain the degradedCondition with the error info rather than nil so the failure
is reflected in status.
- Around line 121-123: The deferred status-write currently overwrites the
original reconcile error stored in err when
c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
returns applyError; change the defer logic to preserve and combine errors
instead of replacing them — e.g., if both err and applyError are non-nil set err
= errors.Join(err, applyError), if only applyError is non-nil set err =
applyError, otherwise leave err unchanged; update imports to include "errors" if
needed so the ApplyOperatorStatus defer merges the errors rather than discarding
the original reconcile failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae271b2-e73a-4c97-bf82-d81491e0b573
📒 Files selected for processing (1)
pkg/operator/encryption/controllers/kms_preflight_controller.go
ardaguclu
reviewed
Apr 24, 2026
| // The controller creates a ServiceAccount, Role and RoleBinding so that the pod | ||
| // can update its own status. These resources are cleaned up when the pod is removed. | ||
| // The controller reads these enhanced pod statuses to update its own operator | ||
| // status, which is propagated to end users. |
Member
There was a problem hiding this comment.
This mechanism makes sense to me.
Just minor improvement;
Suggested change
| // status, which is propagated to end users. | |
| // status and propagates this per provider name to end users. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit