Adding key identifier in KMS to KMS cases#2346
Conversation
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds exported test helpers for deriving operator key IDs from encryption metadata and asserting KMS v2-encrypted etcd payload prefixes, including diagnostic reporting for malformed or mismatched payload prefixes. ChangesKMS v2 Assertion Helpers
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
gangwgr
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
gangwgr
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
|
||
| pluginName, ok := kmsPluginNameFromEtcdValue(raw) | ||
| if !ok { | ||
| t.Errorf("etcd data for resource %q is not KMS-encrypted (expected operator keyID %q)", resource, expectedKeyID) |
There was a problem hiding this comment.
are you able to add the first 30'ish bytes here from raw? it will be pretty difficult to reconstruct this otherwise from CI. Also applies for the above places where the assertions fail.
There was a problem hiding this comment.
it is in operator repo caller
| if _, err := strconv.ParseUint(keyID, 10, 64); err != nil { | ||
| return "", fmt.Errorf("invalid operator key ID %q in KMS plugin name %q: %w", keyID, pluginName, err) | ||
| } |
There was a problem hiding this comment.
that's nitpicky, but there's no need to parse this into an integer here when you're returning a string anyway. The assertion (that's currently missing) is checking whether the format is what we expect anyway.
| t.Errorf("etcd data for resource %q has an invalid KMS encryption prefix (expected operator keyID %q)", resource, expectedKeyID) | ||
| return | ||
| } | ||
| t.Errorf("etcd data for resource %q is encrypted with operator keyID %q, expected %q", resource, gotKeyID, expectedKeyID) |
There was a problem hiding this comment.
where's the assertion for that? :)
There was a problem hiding this comment.
I'm checking in caller
There was a problem hiding this comment.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/library/encryption/assertion.go (1)
231-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo direct unit tests for the exported assertion functions.
assertion_kms_test.goonly covers the internal parsing helpers (kmsPluginNameFromEtcdValue,operatorKeyIDFromKMSPluginName,expectedKMSPrefix,etcdRawHeadForDiagnostics). The exportedAssertEtcdValueEncryptedWithOperatorKeyIDandAssertKMSEncryptedWithWriteKeybranch logic (not-KMS-encrypted, invalid prefix, keyID mismatch) is untested directly, only indirectly via the helper tests.Consider adding a lightweight fake
testing.TB(or usingt.Runwith a sub-test assertingt.Failed()) to cover the mismatch/invalid-prefix branches ofAssertEtcdValueEncryptedWithOperatorKeyIDdirectly.🤖 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 `@test/library/encryption/assertion.go` around lines 231 - 271, The exported assertion helpers are missing direct coverage for their failure branches, so add tests around AssertEtcdValueEncryptedWithOperatorKeyID and AssertKMSEncryptedWithWriteKey rather than only the parsing helpers. Use a lightweight fake testing.TB or subtests with t.Run and t.Failed() to exercise the non-KMS-encrypted, invalid-prefix, and keyID-mismatch paths in AssertEtcdValueEncryptedWithOperatorKeyID, and verify AssertKMSEncryptedWithWriteKey reaches that logic via GetLastKeyMeta/OperatorKeyIDFromEncryptionKeyMeta.
🤖 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 `@test/library/encryption/assertion.go`:
- Around line 231-271: The exported assertion helpers are missing direct
coverage for their failure branches, so add tests around
AssertEtcdValueEncryptedWithOperatorKeyID and AssertKMSEncryptedWithWriteKey
rather than only the parsing helpers. Use a lightweight fake testing.TB or
subtests with t.Run and t.Failed() to exercise the non-KMS-encrypted,
invalid-prefix, and keyID-mismatch paths in
AssertEtcdValueEncryptedWithOperatorKeyID, and verify
AssertKMSEncryptedWithWriteKey reaches that logic via
GetLastKeyMeta/OperatorKeyIDFromEncryptionKeyMeta.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a5f83aef-4417-43cd-af8a-026fbcad7326
📒 Files selected for processing (2)
test/library/encryption/assertion.gotest/library/encryption/assertion_kms_test.go
| resource, pluginName, expectedKeyID, etcdRawHeadForDiagnostics(raw)) | ||
| return | ||
| } | ||
| t.Errorf("etcd data for resource %q is encrypted with operator keyID %q, expected %q, raw head: %s", |
There was a problem hiding this comment.
okay let me try this again, what do you assert here? This will always fail, won't it?
There was a problem hiding this comment.
| t.Errorf("etcd data for resource %q is encrypted with operator keyID %q, expected %q, raw head: %s", | |
| if want != gotKeyId { | |
| t.Errorf("etcd data for resource %q is encrypted with operator keyID %q, expected %q, raw head: %s", |
maybe?
There was a problem hiding this comment.
okay let me try this again, what do you assert here? This will always fail, won't it?
The last t.Errorf is only reached when the prefix check at the top fails — it's a diagnostic failure path, not the assertion itself. On success we return at line 242 (see Auth-O CI - https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-authentication-operator/941/pull-ci-openshift-cluster-authentication-operator-master-e2e-aws-operator-encryption-kms-2/2072983447919398912/artifacts/e2e-aws-operator-encryption-kms-2/openshift-e2e-test/artifacts/junit/extension_test_result_e2e__20260703-105523-everything.html, keyID "1" then "2" both pass there). We assert the full prefix k8s:enc:kms:v2:{keyID}_{resource}.
| if len(raw) > max { | ||
| return hex.EncodeToString(raw[:max]) + "..." | ||
| } | ||
| return hex.EncodeToString(raw) |
There was a problem hiding this comment.
is there a better way to display it as a string without decoding hex? sorry I just realized that this is stupid, nobody is going to de-hex this when seeing this in CI...
gangwgr
force-pushed
the
kmstoks
branch
6 times, most recently
from
77c641e to
02433a0
Compare
| return | ||
| } | ||
| t.Logf("etcd data for resource %q matches operator keyID %q", resource, expectedKeyID) |
There was a problem hiding this comment.
| return | |
| } | |
| t.Logf("etcd data for resource %q matches operator keyID %q", resource, expectedKeyID) | |
| } else { | |
| t.Logf("etcd data for resource %q matches operator keyID %q", resource, expectedKeyID) | |
| } |
save yourself the return path, we're not developing in C
|
/lgtm |
|
@gangwgr: 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. |
|
/approve |
|
/hold in case you need more testing |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr, 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 |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
2 participants
Adding key identifier in KMS to KMS cases
Summary by CodeRabbit
{keyID}_{resource}pattern.