CNTRLPLANE-3237: KMS preflight checker part3#2313
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughExtends the ChangesKMS Config Hashing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/assign @tjungblu |
![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/kms_preflight_controller.go`:
- Around line 72-80: The hash stream is vulnerable to collisions because
key/value boundaries are not encoded when writing to the hasher. Currently, the
code at the hasher.Write call in the loop iterating over sorted keys only writes
the raw value bytes, which means different key-value combinations can produce
identical hashes. To fix this, encode the key and value boundaries into the hash
stream by writing both the key and value in a way that makes their boundaries
unambiguous (for example, by writing the key length, key, value length, and
value in sequence). Apply this same fix to all places where values are written
to the hasher, specifically in the loop handling secret data keys and in the
similar section handling ConfigMap data (which the comment indicates also exists
around lines 99-107).
🪄 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: b80c1c01-e2c1-490e-8f96-d483bae4c79b
📒 Files selected for processing (2)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/kms_preflight_controller.go
| return "", err | ||
| } | ||
|
|
||
| return base64.URLEncoding.EncodeToString(hasher.Sum(nil)), nil |
There was a problem hiding this comment.
how long is that going to be? 32 bit of FNV as base64 is 6 characters?
There was a problem hiding this comment.
yes, 6 chars + padding (check the unit tests)
There was a problem hiding this comment.
can we use this as a suffix in a deployment name?
DC20hA==
"==" wouldn't work. Maybe just hex the string instead?
Alternatively we can just generate a random name and have this as a label, that's also cool for me. Just adds another filtering step to the queries.
There was a problem hiding this comment.
yeah, I think we should have a stable pod name. There will be a single instance of the preflight pod per namespace at any given time. It will make cleanup easier.
We could add the hash as an annotation to keep track of the version/generation/configuration that the preflight pod is checking.
p0lyn0mial
force-pushed
the
kms-preflight-checks-part3
branch
from
176f74b to
ad336ec
Compare
p0lyn0mial
changed the title
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
removed
the
do-not-merge/work-in-progress
p0lyn0mial
force-pushed
the
kms-preflight-checks-part3
branch
from
ad336ec to
361eec0
Compare
| name: "same config and resources produce the same hash", | ||
| vaultConfig: baseVaultConfig, | ||
| resources: []runtime.Object{baseSecret, baseConfigMap}, | ||
| expectedHash: "k6dSVA==", |
There was a problem hiding this comment.
this is a maintenance nitpick for me, but imagine we're changing the structure in the future and the hashes change. Maybe we don't want to hardcode the hashes on every test instead of a "match/mismatch" flag.
There was a problem hiding this comment.
in an AI era the cost of updating the hashes is very low. I did that many time while I was developing the tests.
There was a problem hiding this comment.
also i wanted to avoid precomputing the hash in the test because i wanted to avoid using the same production code path or build a new function for the tests
There was a problem hiding this comment.
does it make sense ?
There was a problem hiding this comment.
not really, how does the AI generate the hashes when the struct changes? Let's say we add a new provider in KMSPluginConfig.
avoid precomputing the hash in the test
so how did you get the hashes in the first place?
There was a problem hiding this comment.
so how did you get the hashes in the first place?
directly from the hash method.
There was a problem hiding this comment.
not really, how does the AI generate the hashes when the struct changes?
this is a similar technique to
- https://github.com/openshift/openshift-apiserver/blob/main/pkg/bootstrappolicy/policy_test.go#L100
- https://github.com/openshift/machine-config-operator/blob/main/devex/cmd/mco-sanitize/sanitize_e2e_test.go#L150
just now you can ask ai to replace the hardcoded values.
There was a problem hiding this comment.
I added the following comment as a hint for agents:
// Each scenario changes exactly one field from the baseline and verifies the hash changes.
// If the hash algorithm or encoding changes, update the expectedHash values by running
// the test and copying the actual values from the error output.
There was a problem hiding this comment.
just now you can ask ai to replace the hardcoded values.
yeah, but the agent does the following (try it out and check the commands):
- runs the tests
- input the failed actual values into the expected values
You might as well just save yourself the tokens and directly generate the expected values.
…S config and referenced resources
p0lyn0mial
force-pushed
the
kms-preflight-checks-part3
branch
from
361eec0 to
8fc8f41
Compare
|
@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. |
|
LGTM |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, tjungblu 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 |
4 participants
Summary by CodeRabbit