CNTRLPLANE-3237: Introduce getter for KMS SecretData#2260
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: 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. |
WalkthroughKMSPluginsSecretData and KMSSecretData types are refactored to provide encapsulated read access through new ChangesKMS Secret Data Encapsulation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| if d.Entries == nil { | ||
| return nil, false | ||
| } | ||
| secretEntries, ok := d.Entries[secretName] |
There was a problem hiding this comment.
maybe in a follow-up would it make sense to hide d.Entries ?
|
/lgtm |
|
[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 |
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: 3
🤖 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/encryptiondata/config_test.go`:
- Around line 846-850: The test fixture is discarding errors returned by
encryptiondata.KMSPluginsSecretData.SetFromRawKey (used inside expectedData),
which can mask invalid keys; update the fixture to capture and assert the error
(e.g., use require.NoError or call a helper that panics on error) for each
SetFromRawKey call so the test fails fast if a key is invalid; apply the same
fix to the other occurrences of SetFromRawKey in this file (around the blocks
noted: lines ~1009-1014, 1050-1057, 1144-1149, 1157-1162, 1236-1241, 1261-1268)
so no SetFromRawKey error is ignored.
In `@pkg/operator/encryption/encryptiondata/config.go`:
- Around line 45-52: Get currently returns the internal state.KMSSecretData
value directly, allowing callers to mutate its exported Entries map and thus
mutate KMSPluginsSecretData.byKeyID; change KMSPluginsSecretData.Get to return a
defensive copy: look up sd := d.byKeyID[keyID], if ok then deep-copy sd by
allocating a new state.KMSSecretData value and, if sd.Entries is non-nil,
allocate a new map and copy each key/value into it, then return the copied
struct and true; keep the nil-map check for d.byKeyID and return zero
value/false when missing.
In `@pkg/operator/encryption/state/types.go`:
- Around line 85-99: KMSSecretData.Get currently returns the internal []byte
slice from d.Entries allowing callers to mutate internal state; change
KMSSecretData.Get (the function named Get) to return a copy of the found value:
after retrieving value := secretEntries[dataKey], allocate a new byte slice of
len(value), copy the bytes into it, and return that copy with the same bool;
keep all existing nil/empty checks and the same return semantics when key not
found.
🪄 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: d624dcc6-04e5-4ee7-9be4-16483157907a
📒 Files selected for processing (5)
pkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/encryptiondata/config.gopkg/operator/encryption/encryptiondata/config_test.gopkg/operator/encryption/state/types.gotest/e2e-encryption/encryption_test.go
| expectedData: func() encryptiondata.KMSPluginsSecretData { | ||
| var sd encryptiondata.KMSPluginsSecretData | ||
| sd.SetFromRawKey("1", "vault-approle-secret_role-id", []byte("test-role-id")) | ||
| sd.SetFromRawKey("1", "vault-approle-secret_secret-id", []byte("test-secret-id")) | ||
| return sd |
There was a problem hiding this comment.
Stop discarding SetFromRawKey errors in test fixtures.
If one of these raw keys becomes invalid, the fixture quietly degrades and the test starts asserting on the wrong expected state. Please fail fast here with require.NoError or a helper that panics in fixture construction.
As per coding guidelines, **/*.go: Never ignore error returns.
Also applies to: 1009-1014, 1050-1057, 1144-1149, 1157-1162, 1236-1241, 1261-1268
🤖 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_test.go` around lines 846 -
850, The test fixture is discarding errors returned by
encryptiondata.KMSPluginsSecretData.SetFromRawKey (used inside expectedData),
which can mask invalid keys; update the fixture to capture and assert the error
(e.g., use require.NoError or call a helper that panics on error) for each
SetFromRawKey call so the test fails fast if a key is invalid; apply the same
fix to the other occurrences of SetFromRawKey in this file (around the blocks
noted: lines ~1009-1014, 1050-1057, 1144-1149, 1157-1162, 1236-1241, 1261-1268)
so no SetFromRawKey error is ignored.
| // Get returns the KMSSecretData for the given keyID and true if it exists, | ||
| // or a zero value and false otherwise. It is safe to call on a nil map. | ||
| func (d *KMSPluginsSecretData) Get(keyID string) (state.KMSSecretData, bool) { | ||
| if d.byKeyID == nil { | ||
| return state.KMSSecretData{}, false | ||
| } | ||
| sd, ok := d.byKeyID[keyID] | ||
| return sd, ok |
There was a problem hiding this comment.
This getter still leaks mutable internal state.
Get returns a state.KMSSecretData value, but that struct still carries the exported Entries map. A caller can mutate the returned maps and silently modify KMSPluginsSecretData.byKeyID, so this doesn't actually enforce read-only access.
🤖 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 45 - 52, Get
currently returns the internal state.KMSSecretData value directly, allowing
callers to mutate its exported Entries map and thus mutate
KMSPluginsSecretData.byKeyID; change KMSPluginsSecretData.Get to return a
defensive copy: look up sd := d.byKeyID[keyID], if ok then deep-copy sd by
allocating a new state.KMSSecretData value and, if sd.Entries is non-nil,
allocate a new map and copy each key/value into it, then return the copied
struct and true; keep the nil-map check for d.byKeyID and return zero
value/false when missing.
There was a problem hiding this comment.
I'll unexport Entries in followup PR
There was a problem hiding this comment.
@ardaguclu, sounds good — unexporting Entries in a follow-up PR is a clean way to enforce read-only access at the type level. Would you like me to open a GitHub issue to track that work?
| // Get returns the value for the given secretName and dataKey. It returns false if | ||
| // secretName or dataKey is empty, if Entries is nil, or if the key does not exist. | ||
| func (d *KMSSecretData) Get(secretName, dataKey string) ([]byte, bool) { | ||
| if len(secretName) == 0 || len(dataKey) == 0 { | ||
| return nil, false | ||
| } | ||
| if d.Entries == nil { | ||
| return nil, false | ||
| } | ||
| secretEntries, ok := d.Entries[secretName] | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| value, ok := secretEntries[dataKey] | ||
| return value, ok |
There was a problem hiding this comment.
Return a copy of the stored secret bytes.
Get currently returns the original []byte from Entries, so callers can mutate internal secret data through the accessor. Copy the slice before returning it.
💡 Localized fix
value, ok := secretEntries[dataKey]
- return value, ok
+ if !ok {
+ return nil, false
+ }
+ return append([]byte(nil), value...), true🤖 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/state/types.go` around lines 85 - 99,
KMSSecretData.Get currently returns the internal []byte slice from d.Entries
allowing callers to mutate internal state; change KMSSecretData.Get (the
function named Get) to return a copy of the found value: after retrieving value
:= secretEntries[dataKey], allocate a new byte slice of len(value), copy the
bytes into it, and return that copy with the same bool; keep all existing
nil/empty checks and the same return semantics when key not found.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think this is a nice catch. I'll fix this in followup PR
There was a problem hiding this comment.
I disagree with coderabbitai :). The convention in go is to return the bytes without copying, same principle as returning a pointer. IMO copying the bytes here would be unusual and would require a comment explaining why
There was a problem hiding this comment.
yeah, additionally even if plugin lifecycle mutates the object, this won't be an issue. Because source of truth is stored in Secret. I decided to not adopt the suggested approach.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
|
@ardaguclu: 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
This PR introduces getter functionality, so that plugin lifecycle can safely query data in #2252
Summary by CodeRabbit
Release Notes