NO-JIRA: rotation controller prototype#2237
Open
tjungblu wants to merge 1 commit into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tjungblu: This pull request explicitly references no jira issue. 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. |
|
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:
WalkthroughThis PR adds KMS KEK rotation orchestration: a new rotation controller, status types and helpers for convergence and rotation history, migration-completion checks from secret annotations, config/secrets helpers, operator-condition persistence, and unit tests. ChangesKMS encryption key rotation
Sequence Diagram(s)sequenceDiagram
participant RotationController
participant OperatorStatus
participant HealthReports
participant EncryptionSecret
participant Migrator
RotationController->>OperatorStatus: load operator status
RotationController->>HealthReports: extract health reports from conditions
RotationController->>HealthReports: check KEK convergence per plugin
RotationController->>RotationController: upsert open rotation entry
RotationController->>EncryptionSecret: read migration finish annotation
RotationController->>RotationController: mirror timestamp to rotation status
RotationController->>RotationController: enforce post-discovery delay
RotationController->>Migrator: prune storage migration state
RotationController->>EncryptionSecret: clear migration annotations
RotationController->>OperatorStatus: persist rotation status to conditions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 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 NOT APPROVED This pull-request has been approved by: tjungblu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 (1)
pkg/operator/encryption/encryptionstatus/rotation_test.go (1)
23-35: ⚡ Quick winAdd a regression test for
UpsertOpenRotationusing a fixednowvalue.Current tests miss the existing-open-entry path, which is where timestamp-source regressions occur.
Proposed test addition
+func TestUpsertOpenRotationUsesProvidedNowForMissingDiscoveryTime(t *testing.T) { + now := metav1.Now() + rotations := []KMSPluginRotationStatus{ + {KeyID: "1", KEKID: "kek-a", DiscoveryTime: nil}, + } + + updated, idx := UpsertOpenRotation(rotations, "1", "kek-a", now) + if idx != 0 { + t.Fatalf("expected index 0, got %d", idx) + } + if updated[0].DiscoveryTime == nil || !updated[0].DiscoveryTime.Equal(&now) { + t.Fatalf("expected discoveryTime to equal provided now") + } +}🤖 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/encryptionstatus/rotation_test.go` around lines 23 - 35, Add a regression test for UpsertOpenRotation that uses a fixed metav1.Time value to ensure the code path that updates an existing open entry uses the supplied now value as the timestamp source. In rotation_test.go create rotations containing an open KMSPluginRotationStatus (MigrationFinishTime == nil) for a given KeyID, call UpsertOpenRotation(rotations, <KeyID>, <KEKID>, now) where now is a deterministic metav1.NewTime(fixedTime), and assert that the returned/open KMSPluginRotationStatus for that KEK has its timestamp field set to exactly that now (not a freshly generated time) and that the entry identity (KeyID/KEKID) remains correct. Reference UpsertOpenRotation and KMSPluginRotationStatus to locate where to add this test.
🤖 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_rotation_controller.go`:
- Around line 254-257: The current code treats a failed time.Parse as a
successful completion by returning metav1.Now(); instead, when time.Parse(raw)
fails you must not signal completion—log or record the parse error and return a
zero metav1.Time (metav1.Time{}) and false (or propagate an error to requeue) so
the rotation is not marked finished; update the block around time.Parse(...) to
avoid using metav1.Now() on parse failure and reference the time.Parse/raw
parsing logic and the metav1.Now() return to locate and fix the code.
In `@pkg/operator/encryption/encryptiondata/config.go`:
- Around line 50-55: The loop in
pkg/operator/encryption/encryptiondata/config.go currently ignores entries where
resourceConfig.Resources has length != 1, dropping multi-resource entries and
underreporting encrypted GR coverage; update the loop over
c.Encryption.Resources to iterate over each entry in resourceConfig.Resources
(instead of only handling index 0 when len==1) and call
schema.ParseGroupResource for every resource string, appending each parsed value
to grs so all resources in resourceConfig.Resources are included; adjust any
variable names (resourceConfig, grs) accordingly to preserve existing semantics.
In `@pkg/operator/encryption/encryptionstatus/rotation.go`:
- Around line 35-42: The UpsertOpenRotation function inconsistently uses
metav1.Now() instead of the injected now parameter; locate UpsertOpenRotation
and replace the metav1.Now() call in the SetDiscoveryTime branch with the now
argument so the function uses the provided timestamp consistently (references:
UpsertOpenRotation, indexRotation, prependRotation, newOpenRotation,
SetDiscoveryTime).
---
Nitpick comments:
In `@pkg/operator/encryption/encryptionstatus/rotation_test.go`:
- Around line 23-35: Add a regression test for UpsertOpenRotation that uses a
fixed metav1.Time value to ensure the code path that updates an existing open
entry uses the supplied now value as the timestamp source. In rotation_test.go
create rotations containing an open KMSPluginRotationStatus (MigrationFinishTime
== nil) for a given KeyID, call UpsertOpenRotation(rotations, <KeyID>, <KEKID>,
now) where now is a deterministic metav1.NewTime(fixedTime), and assert that the
returned/open KMSPluginRotationStatus for that KEK has its timestamp field set
to exactly that now (not a freshly generated time) and that the entry identity
(KeyID/KEKID) remains correct. Reference UpsertOpenRotation and
KMSPluginRotationStatus to locate where to add this test.
🪄 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: f8872af3-c7bd-42da-918f-d4f3c2c38997
📒 Files selected for processing (13)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_rotation_controller.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptionstatus/convergence.gopkg/operator/encryption/encryptionstatus/convergence_test.gopkg/operator/encryption/encryptionstatus/migration.gopkg/operator/encryption/encryptionstatus/migration_test.gopkg/operator/encryption/encryptionstatus/operator.gopkg/operator/encryption/encryptionstatus/operator_test.gopkg/operator/encryption/encryptionstatus/rotation.gopkg/operator/encryption/encryptionstatus/rotation_test.gopkg/operator/encryption/encryptionstatus/types.gopkg/operator/encryption/secrets/secrets.go
tjungblu
force-pushed
the
rotation_controller
branch
from
9958b19 to
b8c8866
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/operator/encryption/encryptiondata/config.go (1)
50-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not drop multi-resource encryption entries when deriving encrypted GRs.
Skipping entries where
len(resourceConfig.Resources) != 1can omit encrypted resources and make migration/rotation decisions incorrect.✅ Include every listed resource
for _, resourceConfig := range c.Encryption.Resources { - if len(resourceConfig.Resources) != 1 { - continue - } - grs = append(grs, schema.ParseGroupResource(resourceConfig.Resources[0])) + for _, resource := range resourceConfig.Resources { + grs = append(grs, schema.ParseGroupResource(resource)) + } }🤖 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/encryptiondata/config.go` around lines 50 - 55, The loop over c.Encryption.Resources currently skips entries with multiple resources by checking len(resourceConfig.Resources) != 1; update the logic in the block that builds grs so that for each resourceConfig in c.Encryption.Resources you iterate over every entry in resourceConfig.Resources and call schema.ParseGroupResource for each item (instead of only handling single-item lists), appending each parsed GroupResource to grs; ensure you still handle empty lists safely and preserve existing variable names (resourceConfig.Resources, schema.ParseGroupResource, grs) so all listed resources are included when deriving encrypted GRs.
🧹 Nitpick comments (1)
pkg/operator/encryption/encryptionstatus/migration_test.go (1)
30-30: ⚡ Quick winCheck marshal errors in test setup.
Ignoring the error here can mask setup failures and make assertions misleading.
Proposed fix
- raw, _ = json.Marshal(partial) + raw, err = json.Marshal(partial) + if err != nil { + t.Fatal(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/encryptionstatus/migration_test.go` at line 30, The test currently ignores the error from json.Marshal(partial) when setting raw; capture the error (err := json.Marshal(partial) or similar) and assert it succeeded before proceeding (e.g., use require.NoError(t, err) or t.Fatalf("marshal failed: %v", err)); ensure you still assign raw only after a successful marshal so test setup failures are surfaced early (refer to the variables raw and partial and the call to json.Marshal).
🤖 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_rotation_controller.go`:
- Around line 150-160: UpsertOpenRotation is being called before we know whether
the converged kekID already matches the latest completed rotation, which causes
churn; call AllEncryptedGRsMigrated(...) first to get migrated, then inspect the
existing rotations slice to find the latest completed rotation's kekID (e.g.,
scan rotations for State==Completed and take its KekID) and only call
encryptionstatus.UpsertOpenRotation(...) if that latestCompletedKekID != kekID
or migration is not completed; ensure mirrorMigrationFinish and
maybeStartRotation still receive the correct rotations/idx values (create idx
only when you actually upsert) so we don't create new rotation entries during
steady-state convergence.
---
Duplicate comments:
In `@pkg/operator/encryption/encryptiondata/config.go`:
- Around line 50-55: The loop over c.Encryption.Resources currently skips
entries with multiple resources by checking len(resourceConfig.Resources) != 1;
update the logic in the block that builds grs so that for each resourceConfig in
c.Encryption.Resources you iterate over every entry in resourceConfig.Resources
and call schema.ParseGroupResource for each item (instead of only handling
single-item lists), appending each parsed GroupResource to grs; ensure you still
handle empty lists safely and preserve existing variable names
(resourceConfig.Resources, schema.ParseGroupResource, grs) so all listed
resources are included when deriving encrypted GRs.
---
Nitpick comments:
In `@pkg/operator/encryption/encryptionstatus/migration_test.go`:
- Line 30: The test currently ignores the error from json.Marshal(partial) when
setting raw; capture the error (err := json.Marshal(partial) or similar) and
assert it succeeded before proceeding (e.g., use require.NoError(t, err) or
t.Fatalf("marshal failed: %v", err)); ensure you still assign raw only after a
successful marshal so test setup failures are surfaced early (refer to the
variables raw and partial and the call to json.Marshal).
🪄 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: 3c07d496-feb3-4fd0-bfe1-149deb69b1ba
📒 Files selected for processing (13)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_rotation_controller.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptionstatus/convergence.gopkg/operator/encryption/encryptionstatus/convergence_test.gopkg/operator/encryption/encryptionstatus/migration.gopkg/operator/encryption/encryptionstatus/migration_test.gopkg/operator/encryption/encryptionstatus/operator.gopkg/operator/encryption/encryptionstatus/operator_test.gopkg/operator/encryption/encryptionstatus/rotation.gopkg/operator/encryption/encryptionstatus/rotation_test.gopkg/operator/encryption/encryptionstatus/types.gopkg/operator/encryption/secrets/secrets.go
tjungblu
force-pushed
the
rotation_controller
branch
from
b8c8866 to
d0f58b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/encryptionstatus/convergence.go`:
- Around line 9-17: KEKByKeyID currently only checks isHealthyReport but does
not skip reports with empty KeyID or KEKID as the contract requires; update
KEKByKeyID to additionally guard that report.KeyID != "" and report.KEKID != ""
before appending to result (keep the existing isHealthyReport(report) check), so
only reports with non-empty keyId and kekId are included and downstream callers
simplify.
In `@pkg/operator/encryption/encryptionstatus/migration_test.go`:
- Around line 30-31: The test setup currently ignores the error from
json.Marshal when creating raw from partial and assigning it to
secret.Annotations[secrets.EncryptionSecretMigratedResources]; update the setup
to check the error return from json.Marshal(partial), fail the test (or use
t.Fatalf/t.Helper with a clear message) when marshal returns an error, and
include a descriptive failure message that identifies the marshal of partial and
the target annotation key so test failures show the root cause.
🪄 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: 730cfbd0-b2d2-4d99-9d33-ec8cfb74a78f
📒 Files selected for processing (13)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_rotation_controller.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptionstatus/convergence.gopkg/operator/encryption/encryptionstatus/convergence_test.gopkg/operator/encryption/encryptionstatus/migration.gopkg/operator/encryption/encryptionstatus/migration_test.gopkg/operator/encryption/encryptionstatus/operator.gopkg/operator/encryption/encryptionstatus/operator_test.gopkg/operator/encryption/encryptionstatus/rotation.gopkg/operator/encryption/encryptionstatus/rotation_test.gopkg/operator/encryption/encryptionstatus/types.gopkg/operator/encryption/secrets/secrets.go
Comment on lines
+9
to
+17
| func KEKByKeyID(reports []KMSPluginHealthReport) map[string][]string { | ||
| result := map[string][]string{} | ||
| for _, report := range reports { | ||
| if !isHealthyReport(report) { | ||
| continue | ||
| } | ||
| result[report.KeyID] = append(result[report.KeyID], report.KEKID) | ||
| } | ||
| return result |
There was a problem hiding this comment.
Filter empty KeyID/KEKID to match documented contract.
The comment on line 8 states that only reports with non-empty keyId and kekId are included, but the code does not enforce this. Although ConvergedKEKForKeyID handles empty values downstream, filtering here would match the documented contract and simplify caller logic.
Proposed fix
func KEKByKeyID(reports []KMSPluginHealthReport) map[string][]string {
result := map[string][]string{}
for _, report := range reports {
if !isHealthyReport(report) {
continue
}
+ if report.KeyID == "" || report.KEKID == "" {
+ continue
+ }
result[report.KeyID] = append(result[report.KeyID], report.KEKID)
}
return result
}🤖 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/encryptionstatus/convergence.go` around lines 9 - 17,
KEKByKeyID currently only checks isHealthyReport but does not skip reports with
empty KeyID or KEKID as the contract requires; update KEKByKeyID to additionally
guard that report.KeyID != "" and report.KEKID != "" before appending to result
(keep the existing isHealthyReport(report) check), so only reports with
non-empty keyId and kekId are included and downstream callers simplify.
Comment on lines
+30
to
+31
| raw, _ = json.Marshal(partial) | ||
| secret.Annotations[secrets.EncryptionSecretMigratedResources] = string(raw) |
There was a problem hiding this comment.
Handle marshal failure in the partial-case setup.
Ignoring the second json.Marshal error can hide setup failures behind assertion noise.
Proposed fix
partial := secrets.MigratedGroupResources{Resources: grs[:1]}
- raw, _ = json.Marshal(partial)
+ raw, err = json.Marshal(partial)
+ if err != nil {
+ t.Fatal(err)
+ }
secret.Annotations[secrets.EncryptionSecretMigratedResources] = string(raw)As per coding guidelines, "Assertions should include meaningful failure messages that help diagnose what went wrong."
🤖 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/encryptionstatus/migration_test.go` around lines 30 -
31, The test setup currently ignores the error from json.Marshal when creating
raw from partial and assigning it to
secret.Annotations[secrets.EncryptionSecretMigratedResources]; update the setup
to check the error return from json.Marshal(partial), fail the test (or use
t.Fatalf/t.Helper with a clear message) when marshal returns an error, and
include a descriptive failure message that identifies the marshal of partial and
the target annotation key so test failures show the root cause.
d0f58b3 to
e3b702a
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/operator/encryption/encryptionstatus/migration_test.go (1)
29-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle marshal error in partial fixture setup.
Line 30 ignores
json.Marshal(partial)error, which can hide setup breakage behind assertion failures. Please fail fast with a descriptive message.Proposed fix
partial := secrets.MigratedGroupResources{Resources: grs[:1]} - raw, _ = json.Marshal(partial) + raw, err = json.Marshal(partial) + if err != nil { + t.Fatalf("failed to marshal partial migrated resources for annotation %q: %v", secrets.EncryptionSecretMigratedResources, err) + } secret.Annotations[secrets.EncryptionSecretMigratedResources] = string(raw)🤖 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/encryptionstatus/migration_test.go` around lines 29 - 31, The test currently ignores the error returned by json.Marshal(partial); update the fixture setup in migration_test.go to check the error from json.Marshal(partial) and fail fast with a descriptive message (e.g., using t.Fatalf or require.NoError) before assigning to secret.Annotations[secrets.EncryptionSecretMigratedResources]; reference the variables/functions partial, secrets.MigratedGroupResources, json.Marshal, and secrets.EncryptionSecretMigratedResources so the change is applied where the partial fixture is prepared.pkg/operator/encryption/encryptionstatus/convergence.go (1)
7-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the documented non-empty KeyID/KEKID filter in
KEKByKeyID.Line 8 says empty
keyId/kekIdare excluded, but Line 15 appends without that guard. Please skip reports wherereport.KeyID == "" || report.KEKID == ""before appending.Proposed fix
func KEKByKeyID(reports []KMSPluginHealthReport) map[string][]string { result := map[string][]string{} for _, report := range reports { if !isHealthyReport(report) { continue } + if report.KeyID == "" || report.KEKID == "" { + continue + } result[report.KeyID] = append(result[report.KeyID], report.KEKID) } return result }🤖 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/encryptionstatus/convergence.go` around lines 7 - 16, KEKByKeyID currently appends report.KEKID even when KeyID or KEKID are empty; update the loop in KEKByKeyID to, after confirming isHealthyReport(report), skip the report if report.KeyID == "" || report.KEKID == "" so only healthy reports with non-empty KeyID and KEKID are added; keep using the same map result and append into result[report.KeyID] for valid reports.
🤖 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_rotation_controller.go`:
- Around line 151-162: The controller is copying stale migration finish
annotations into a newly discovered KEK because migrated is read from
writeKeySecret before the secret is cleared; update the logic in the block that
creates/opens a rotation (where rotations, idx =
encryptionstatus.GetOrCreateOpenRotation(...) is called) so you only call
mirrorMigrationFinish(c.instanceName, rotations, idx, migrated, writeKeySecret)
when the migration annotations actually belong to the same KEK (i.e.,
hasCompleted is true and latest.KEKID == kekID), otherwise pass migrated=false
(or skip mirroring entirely). This ensures mirrorMigrationFinish is only applied
when the secret’s migration finish timestamp corresponds to the same
KEK/rotation.
In `@pkg/operator/encryption/encryptionstatus/operator.go`:
- Around line 29-33: The code treats an explicitly empty structured field as
absent by using len(encryptionStatus.HealthReports) > 0; change this to a nil
check so "present but empty" is preserved: replace the len(...) > 0 check with
encryptionStatus.HealthReports != nil and return that slice when non-nil,
otherwise call healthReportsFromConditions(status); apply the same nil-check fix
for the analogous block around lines 78-83 (same pattern) so other structured
report fields use presence (nil vs non-nil) rather than emptiness to decide
fallback.
---
Duplicate comments:
In `@pkg/operator/encryption/encryptionstatus/convergence.go`:
- Around line 7-16: KEKByKeyID currently appends report.KEKID even when KeyID or
KEKID are empty; update the loop in KEKByKeyID to, after confirming
isHealthyReport(report), skip the report if report.KeyID == "" || report.KEKID
== "" so only healthy reports with non-empty KeyID and KEKID are added; keep
using the same map result and append into result[report.KeyID] for valid
reports.
In `@pkg/operator/encryption/encryptionstatus/migration_test.go`:
- Around line 29-31: The test currently ignores the error returned by
json.Marshal(partial); update the fixture setup in migration_test.go to check
the error from json.Marshal(partial) and fail fast with a descriptive message
(e.g., using t.Fatalf or require.NoError) before assigning to
secret.Annotations[secrets.EncryptionSecretMigratedResources]; reference the
variables/functions partial, secrets.MigratedGroupResources, json.Marshal, and
secrets.EncryptionSecretMigratedResources so the change is applied where the
partial fixture is prepared.
🪄 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: e6b37a35-61e5-41cc-8dc2-6ba31fc8e0e4
📒 Files selected for processing (13)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_rotation_controller.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptionstatus/convergence.gopkg/operator/encryption/encryptionstatus/convergence_test.gopkg/operator/encryption/encryptionstatus/migration.gopkg/operator/encryption/encryptionstatus/migration_test.gopkg/operator/encryption/encryptionstatus/operator.gopkg/operator/encryption/encryptionstatus/operator_test.gopkg/operator/encryption/encryptionstatus/rotation.gopkg/operator/encryption/encryptionstatus/rotation_test.gopkg/operator/encryption/encryptionstatus/types.gopkg/operator/encryption/secrets/secrets.go
Comment on lines
+29
to
+33
| encryptionStatus := encryptionStatusFromOperator(status) | ||
| if len(encryptionStatus.HealthReports) > 0 { | ||
| return encryptionStatus.HealthReports | ||
| } | ||
| return healthReportsFromConditions(status) |
There was a problem hiding this comment.
Honor explicit empty structured status instead of falling back to interim conditions.
The current len(...) > 0 checks treat an explicitly empty structured field as “not present,” which can incorrectly fall back to old condition data. Prefer nil checks to preserve “present but empty” semantics.
Proposed fix
func HealthReportsFromOperatorStatus(status *operatorv1.OperatorStatus) []KMSPluginHealthReport {
encryptionStatus := encryptionStatusFromOperator(status)
- if len(encryptionStatus.HealthReports) > 0 {
+ if encryptionStatus.HealthReports != nil {
return encryptionStatus.HealthReports
}
return healthReportsFromConditions(status)
}
@@
func KeyRotationStatusFromOperatorStatus(status *operatorv1.OperatorStatus) ([]KMSPluginRotationStatus, error) {
encryptionStatus := encryptionStatusFromOperator(status)
- if len(encryptionStatus.KeyRotationStatus) > 0 {
+ if encryptionStatus.KeyRotationStatus != nil {
return encryptionStatus.KeyRotationStatus, nil
}
return keyRotationStatusFromCondition(status)
}Also applies to: 78-83
🤖 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/encryptionstatus/operator.go` around lines 29 - 33,
The code treats an explicitly empty structured field as absent by using
len(encryptionStatus.HealthReports) > 0; change this to a nil check so "present
but empty" is preserved: replace the len(...) > 0 check with
encryptionStatus.HealthReports != nil and return that slice when non-nil,
otherwise call healthReportsFromConditions(status); apply the same nil-check fix
for the analogous block around lines 78-83 (same pattern) so other structured
report fields use presence (nil vs non-nil) rather than emptiness to decide
fallback.
Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu
force-pushed
the
rotation_controller
branch
from
e3b702a to
876547e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
pkg/operator/encryption/controllers/encryption_rotation_controller.go (1)
163-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mirror the previous rotation's finish timestamp onto a newly discovered KEK.
Line 169 mirrors the write-secret migration timestamp before the new rotation has cleared the old annotations, so a fresh
convergedKEKIDcan be recorded as finished before it has even started. That corrupts rotation history and can make status look complete too early.💡 Guard mirroring until bootstrap or after this rotation has started
migrated, err := encryptionstatus.AllEncryptedGRsMigrated(writeKeySecret, encryptedGRs) if err != nil { return rotations, err } - rotations = mirrorMigrationFinish(c.instanceName, rotations, openIdx, migrated, writeKeySecret) + if !hasCompleted || rotations[openIdx].MigrationStartTime != nil { + rotations = mirrorMigrationFinish(c.instanceName, rotations, openIdx, migrated, writeKeySecret) + }🤖 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_rotation_controller.go` around lines 163 - 169, The code calls mirrorMigrationFinish(...) unconditionally and can copy the previous rotation's migration-finished timestamp onto a newly discovered KEK; change this by guarding the mirrorMigrationFinish call so it only runs when this rotation has actually started or when bootstrapping is allowed: after computing migrated via encryptionstatus.AllEncryptedGRsMigrated(writeKeySecret, encryptedGRs), check the rotation state in rotations[openIdx] (e.g., whether the rotation has a start marker/flag or is a bootstrap rotation) and only invoke mirrorMigrationFinish(c.instanceName, rotations, openIdx, migrated, writeKeySecret) when that check confirms the rotation has begun (or bootstrap path applies), otherwise skip mirroring to avoid tainting the new convergedKEKID.pkg/operator/encryption/encryptionstatus/migration_test.go (1)
30-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle the
json.Marshalerror in test setup.Ignoring the marshal error can hide setup failures and produce confusing assertion errors downstream if the struct becomes unmarshalable.
🧪 Proposed fix
partial := secrets.MigratedGroupResources{Resources: grs[:1]} - raw, _ = json.Marshal(partial) + raw, err = json.Marshal(partial) + if err != nil { + t.Fatal(err) + } secret.Annotations[secrets.EncryptionSecretMigratedResources] = string(raw)As per coding guidelines, "Assertions should include meaningful failure messages that help diagnose what went wrong."
🤖 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/encryptionstatus/migration_test.go` around lines 30 - 31, The test currently ignores json.Marshal error when building the annotation (raw, _ = json.Marshal(partial)) which can mask setup failures; change this to capture the error (raw, err := json.Marshal(partial)) and fail the test on error (e.g., require.NoError(t, err, "failed to marshal partial migration annotation") or t.Fatalf("failed to marshal partial migration annotation: %v", err)), then assign secret.Annotations[secrets.EncryptionSecretMigratedResources] = string(raw) as before.pkg/operator/encryption/encryptionstatus/convergence.go (1)
9-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFilter empty KeyID/KEKID to match documented contract.
The past review comment remains valid: the function comment states "non-empty keyId and kekId are included," but the implementation only filters by health status without checking for empty identifiers.
🤖 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/encryptionstatus/convergence.go` around lines 9 - 17, KEKByKeyID currently only filters by health via isHealthyReport but must also skip entries with empty KeyID or KEKID per its contract; update KEKByKeyID to check report.KeyID != "" and report.KEKID != "" (in addition to isHealthyReport(report)) before appending so only non-empty keyId and kekId values are included, leaving the rest of the logic unchanged.pkg/operator/encryption/encryptionstatus/operator.go (2)
29-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor explicit empty structured status instead of falling back to interim conditions.
The past review comment remains valid: using
len(encryptionStatus.HealthReports) > 0treats an explicitly empty slice as "not present" and falls back to condition parsing. A nil check would preserve "present but empty" semantics.🤖 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/encryptionstatus/operator.go` around lines 29 - 33, The current check uses len(encryptionStatus.HealthReports) > 0 which treats an explicitly present but empty slice as "absent" and incorrectly falls back to healthReportsFromConditions; change the logic to detect presence (nil vs non-nil) instead: use a nil-check on encryptionStatus.HealthReports (e.g., encryptionStatus.HealthReports != nil) to return the structured HealthReports when it is present (even if empty), otherwise fall back to healthReportsFromConditions(status); reference encryptionStatus, encryptionStatusFromOperator, encryptionStatus.HealthReports, and healthReportsFromConditions in the change.
78-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor explicit empty structured status instead of falling back to interim conditions.
The past review comment remains valid: using
len(encryptionStatus.KeyRotationStatus) > 0treats an explicitly empty slice as "not present." A nil check would correctly distinguish between absent and explicitly empty.🤖 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/encryptionstatus/operator.go` around lines 78 - 83, Replace the current presence check that uses len(encryptionStatus.KeyRotationStatus) > 0 with a nil check so an explicitly empty slice is honored: call encryptionStatus := encryptionStatusFromOperator(status) and if encryptionStatus.KeyRotationStatus != nil { return encryptionStatus.KeyRotationStatus, nil } else return keyRotationStatusFromCondition(status); update the branch around encryptionStatusFromOperator and keyRotationStatusFromCondition to rely on nil vs absent rather than length.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/encryption_rotation_controller.go (1)
112-125: ⚡ Quick winSort plugin keys before reconciling.
Iterating
currentConfig.KMSPluginsdirectly makes newly created rotation entries order-dependent on Go's randomized map iteration, which can churn the serialized operator status for the same logical state. Sorting thekeyIDs before this loop keeps status updates stable.♻️ Minimal change
- for keyID := range currentConfig.KMSPlugins { + keyIDs := make([]string, 0, len(currentConfig.KMSPlugins)) + for keyID := range currentConfig.KMSPlugins { + keyIDs = append(keyIDs, keyID) + } + sort.Strings(keyIDs) + + for _, keyID := range keyIDs { writeKeySecret := secrets.FindKeySecret(encryptionSecrets, c.instanceName, keyID) if writeKeySecret == nil { klog.Infof("%s: no write key secret for KMS plugin keyID %q, skipping", c.instanceName, keyID) continue }🤖 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_rotation_controller.go` around lines 112 - 125, Iteration over currentConfig.KMSPlugins is non-deterministic causing churn; collect the map keys into a slice, sort the slice, and iterate the sorted keys instead of ranging the map directly so that calls to c.reconcileKMSPlugin(ctx, keyID, ...) happen in a stable order; locate the loop that ranges currentConfig.KMSPlugins and replace it with iterating over the sorted keys slice (use the existing variable names keyID, rotations, writeKeySecret and call reconcileKMSPlugin unchanged).
🤖 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.
Duplicate comments:
In `@pkg/operator/encryption/controllers/encryption_rotation_controller.go`:
- Around line 163-169: The code calls mirrorMigrationFinish(...) unconditionally
and can copy the previous rotation's migration-finished timestamp onto a newly
discovered KEK; change this by guarding the mirrorMigrationFinish call so it
only runs when this rotation has actually started or when bootstrapping is
allowed: after computing migrated via
encryptionstatus.AllEncryptedGRsMigrated(writeKeySecret, encryptedGRs), check
the rotation state in rotations[openIdx] (e.g., whether the rotation has a start
marker/flag or is a bootstrap rotation) and only invoke
mirrorMigrationFinish(c.instanceName, rotations, openIdx, migrated,
writeKeySecret) when that check confirms the rotation has begun (or bootstrap
path applies), otherwise skip mirroring to avoid tainting the new
convergedKEKID.
In `@pkg/operator/encryption/encryptionstatus/convergence.go`:
- Around line 9-17: KEKByKeyID currently only filters by health via
isHealthyReport but must also skip entries with empty KeyID or KEKID per its
contract; update KEKByKeyID to check report.KeyID != "" and report.KEKID != ""
(in addition to isHealthyReport(report)) before appending so only non-empty
keyId and kekId values are included, leaving the rest of the logic unchanged.
In `@pkg/operator/encryption/encryptionstatus/migration_test.go`:
- Around line 30-31: The test currently ignores json.Marshal error when building
the annotation (raw, _ = json.Marshal(partial)) which can mask setup failures;
change this to capture the error (raw, err := json.Marshal(partial)) and fail
the test on error (e.g., require.NoError(t, err, "failed to marshal partial
migration annotation") or t.Fatalf("failed to marshal partial migration
annotation: %v", err)), then assign
secret.Annotations[secrets.EncryptionSecretMigratedResources] = string(raw) as
before.
In `@pkg/operator/encryption/encryptionstatus/operator.go`:
- Around line 29-33: The current check uses len(encryptionStatus.HealthReports)
> 0 which treats an explicitly present but empty slice as "absent" and
incorrectly falls back to healthReportsFromConditions; change the logic to
detect presence (nil vs non-nil) instead: use a nil-check on
encryptionStatus.HealthReports (e.g., encryptionStatus.HealthReports != nil) to
return the structured HealthReports when it is present (even if empty),
otherwise fall back to healthReportsFromConditions(status); reference
encryptionStatus, encryptionStatusFromOperator, encryptionStatus.HealthReports,
and healthReportsFromConditions in the change.
- Around line 78-83: Replace the current presence check that uses
len(encryptionStatus.KeyRotationStatus) > 0 with a nil check so an explicitly
empty slice is honored: call encryptionStatus :=
encryptionStatusFromOperator(status) and if encryptionStatus.KeyRotationStatus
!= nil { return encryptionStatus.KeyRotationStatus, nil } else return
keyRotationStatusFromCondition(status); update the branch around
encryptionStatusFromOperator and keyRotationStatusFromCondition to rely on nil
vs absent rather than length.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/encryption_rotation_controller.go`:
- Around line 112-125: Iteration over currentConfig.KMSPlugins is
non-deterministic causing churn; collect the map keys into a slice, sort the
slice, and iterate the sorted keys instead of ranging the map directly so that
calls to c.reconcileKMSPlugin(ctx, keyID, ...) happen in a stable order; locate
the loop that ranges currentConfig.KMSPlugins and replace it with iterating over
the sorted keys slice (use the existing variable names keyID, rotations,
writeKeySecret and call reconcileKMSPlugin unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 84d39536-856c-47bc-a4e3-fe8a7b413d96
📒 Files selected for processing (13)
pkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/encryption_rotation_controller.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptionstatus/convergence.gopkg/operator/encryption/encryptionstatus/convergence_test.gopkg/operator/encryption/encryptionstatus/migration.gopkg/operator/encryption/encryptionstatus/migration_test.gopkg/operator/encryption/encryptionstatus/operator.gopkg/operator/encryption/encryptionstatus/operator_test.gopkg/operator/encryption/encryptionstatus/rotation.gopkg/operator/encryption/encryptionstatus/rotation_test.gopkg/operator/encryption/encryptionstatus/types.gopkg/operator/encryption/secrets/secrets.go
Contributor
|
@tjungblu: 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. |
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
New Features
Bug Fixes / Enhancements
Tests