CNTRLPLANE-3237: kms: report preflight results via KMSEncryptionStatus - #2375
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@p0lyn0mial: This pull request references CNTRLPLANE-3237 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe KMS preflight controller now coordinates with an encryption status provider using observed configuration hashes and persisted preflight results. It records remote key IDs, reuses stored outcomes, cleans up successful checks immediately, and updates pod assertions and controller tests. ChangesKMS preflight coordination
Estimated code review effort: 4 (Complex) | ~45 minutes 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.
🧹 Nitpick comments (3)
pkg/operator/encryption/kms/preflight/pod_status_test.go (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover "KEK" naming after the rename to
KMSPreflightRemoteKeyIDPodCondition. Two local variables still use the old terminology even though the condition type/constant they reference was renamed.
pkg/operator/encryption/kms/preflight/pod_status_test.go#L41-L44: renamekekIdto something likeremoteKeyIDCond.test/library/encryption/preflight_deploy.go#L116-L117: renamekekCondto something likeremoteKeyIDCond.🤖 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/pod_status_test.go` around lines 41 - 44, Rename the local variable kekId to remoteKeyIDCond in pod_status_test.go and update its references; likewise rename kekCond to remoteKeyIDCond in preflight_deploy.go and update all references there to use the current KMSPreflightRemoteKeyIDPodCondition terminology.pkg/operator/encryption/controllers/kms_preflight_controller.go (1)
466-514: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo-op guard in
ensurePreflightResultignoresStatus, only comparesConfigHash.If a config hash's result is ever re-evaluated for a different
StatuswhileConfigHashstays the same (e.g. once retries for failed preflights are implemented per the TODO above), this write would silently be dropped since onlyConfigHashequality is checked. Not an issue today since nothing currently re-runs a check for the same hash, but worth keeping in mind when retry support lands.🤖 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 466 - 514, The no-op guard in ensurePreflightResult currently skips updates when only Status changes; require both ConfigHash and Status to match before returning early, so a result with the same hash but a new status is persisted.pkg/operator/encryption/controllers/kms_preflight_controller_test.go (1)
335-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
updateErrfield onfakeEncryptionStatusProvideris never exercised.No scenario sets
updateErr, so the write-failure branches in production code (ensurePreflightResultreturning an error on scenario 3e, and the wrapped-error formatting on scenario 3f, inkms_preflight_controller.go) have no test coverage.♻️ Suggested scenario to add
{ name: "UpdateKMSEncryptionStatus fails when writing succeeded result", deployer: &fakeDeployer{podStatus: corev1.PodStatus{ Conditions: []corev1.PodCondition{ {Type: KMSPreflightConfigHashPodCondition, Message: wellKnownMatchingHashForBaseVaultConfig}, {Type: KMSPreflightResultPodCondition, Status: corev1.ConditionTrue}, }, }}, encryptionStatusProvider: &fakeEncryptionStatusProvider{ observedConfigHash: wellKnownMatchingHashForBaseVaultConfig, updateErr: fmt.Errorf("conflict"), }, apiServerObjects: []runtime.Object{apiServerWithKMS}, coreObjects: []runtime.Object{&wellKnownBaseSecret, &wellKnownBaseConfigMap}, preconditionsMet: true, expectedError: "conflict", expectedEncryptionStatusProviderUpdateCalls: 1, expectedConditions: []operatorv1.OperatorCondition{ {Type: "EncryptionKMSPreflightControllerDegraded", Status: "True", Reason: "Error", Message: "conflict"}, }, },🤖 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_test.go` around lines 335 - 368, Add a controller test scenario that configures fakeEncryptionStatusProvider.updateErr and exercises the successful preflight-result write path in ensurePreflightResult. Assert the returned error contains the underlying “conflict” message, exactly one status update is attempted, and the expected degraded OperatorCondition is recorded, covering both the direct write failure and wrapped-error 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.
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 335-368: Add a controller test scenario that configures
fakeEncryptionStatusProvider.updateErr and exercises the successful
preflight-result write path in ensurePreflightResult. Assert the returned error
contains the underlying “conflict” message, exactly one status update is
attempted, and the expected degraded OperatorCondition is recorded, covering
both the direct write failure and wrapped-error behavior.
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 466-514: The no-op guard in ensurePreflightResult currently skips
updates when only Status changes; require both ConfigHash and Status to match
before returning early, so a result with the same hash but a new status is
persisted.
In `@pkg/operator/encryption/kms/preflight/pod_status_test.go`:
- Around line 41-44: Rename the local variable kekId to remoteKeyIDCond in
pod_status_test.go and update its references; likewise rename kekCond to
remoteKeyIDCond in preflight_deploy.go and update all references there to use
the current KMSPreflightRemoteKeyIDPodCondition terminology.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c961b24c-ca2d-40ca-a25e-09e72aece819
⛔ Files ignored due to path filters (14)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmsencryptionstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspreflightcheck.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/kmspreflightresult.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modpkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/kms/preflight/pod_status.gopkg/operator/encryption/kms/preflight/pod_status_test.gotest/library/encryption/preflight_deploy.go
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
With respect to states, only a comment about the removal of cleanup retention. The rest is about code.
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(scenario.encryptionStatusProvider.writtenStatus, scenario.expectedKMSPreflightResult) { |
There was a problem hiding this comment.
Can equality.semantics be used?
| } | ||
| requiredCondition := operatorv1helpers.FindOperatorCondition(operatorStatus.Conditions, "EncryptionKMSPreflightRequired") | ||
| if requiredCondition == nil || requiredCondition.Status != operatorv1.ConditionTrue { | ||
| requiredHash := encryptionStatus.Preflight.ObservedConfigHash |
There was a problem hiding this comment.
Can encryptionStatus.Preflight be nil?
There was a problem hiding this comment.
encryptionStatus.Preflight is a value type
| return false, err | ||
| } | ||
| return false, c.cleanupAfterRetention(ctx, resultCondition.LastTransitionTime.Time) | ||
| return false, c.deployer.Cleanup(ctx) |
There was a problem hiding this comment.
I'm just thinking about the benefits of keeping this pod 1 hour, even in success cases. But probably there is no scenario cluster admin checks it. So this change makes sense to me.
There was a problem hiding this comment.
must-gathers really, but for that we should maybe keep them around for 12h or so
| }) | ||
| } | ||
|
|
||
| func (c *kmsPreflightController) cleanupAfterRetention(ctx context.Context, completedAt time.Time) error { |
There was a problem hiding this comment.
Why do we remove this. Don't we retain the pod 1 hour in failed cases?
There was a problem hiding this comment.
The failed pod is retained forever this was about pods that passed. I think it didn't make sense. Thus I decided to simply the code. The failure case is more important for which the pod will be kept until the "next" preflight.
| // existingResult is non-nil and its ConfigHash already matches result.ConfigHash, | ||
| // making it safe to call on every sync without overwriting a previously stored outcome. | ||
| func (c *kmsPreflightController) ensurePreflightResult(ctx context.Context, existingResult *operatorv1.KMSPreflightResult, result operatorv1.KMSPreflightResult) error { | ||
| if existingResult != nil && existingResult.ConfigHash == result.ConfigHash { |
There was a problem hiding this comment.
Idempotency is only for configHash. So status field can be updated with the given hash?. Should we document this or should we add status in the idempotency?
There was a problem hiding this comment.
the logic here was that for a given hash status should not "flap"(on/off). does it make sense ?
// Idempotency is keyed on ConfigHash only, not Status, because for a given hash
// the result status is stable: a pod posts its result condition once and does not
// change it. Success pods are cleaned up immediately; failure pods are retained
// until the config changes (new hash), so the same hash cannot produce both
// Succeeded and Failed.
| observedConfigHash string | ||
| writtenStatus *operatorv1.KMSPreflightResult | ||
| updateCallCount int | ||
| updateErr error |
| @@ -39,8 +39,8 @@ func TestPodCheckConditions(t *testing.T) { | |||
| } | |||
|
|
|||
| kekId := conditions[2] | |||
There was a problem hiding this comment.
| kekId := conditions[2] | |
| remoteKeyID := conditions[2] |
|
|
||
| return requiredHash, nil | ||
| if encryptionStatus.Preflight.Result.ConfigHash == requiredHash { | ||
| return requiredHash, &encryptionStatus.Preflight.Result, nil |
There was a problem hiding this comment.
Do we have to return as reference type?
There was a problem hiding this comment.
we don't have to. I can change it.
in general it was ok because the client returns a new struct on each invocation.
| // Result already recorded as Failed and the pod is gone: surface the error | ||
| // without re-deploying. The admin must fix the config (new hash) before a | ||
| // new check can run. | ||
| if isPreflightResult(existingResult, operatorv1.KMSPreflightResultFailed) { |
There was a problem hiding this comment.
can you add two functions helpers where this is more explicit in the name PreflightResultSuceeded / PreflightResultFailed
openshift-ci
Bot
added
the
needs-rebase
2b394b9 to
aba173d
Compare
openshift-ci
Bot
removed
the
needs-rebase
aba173d to
ed70b63
Compare
|
Thank you |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, 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 |
|
/hold cancel All good! Thanks @p0lyn0mial |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
|
@p0lyn0mial: 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. |
4 participants
Summary by CodeRabbit