CNTRLPLANE-3364: added vault key rotation func#2229
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
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:
WalkthroughAdds a VaultKeyRotator test utility with defaults, constructors, methods to read transit key metadata via ChangesVault Key Rotation Utility
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sandeepknd 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 |
|
/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
🧹 Nitpick comments (1)
test/library/encryption/kms/vault_key_rotation.go (1)
63-132: ⚡ Quick winExtract shared rotation flow to reduce drift between
ForceKeyRotationandRotateKeyOnly.Both methods duplicate the same 5-step sequence (pre-check, rotate, post-check, increment validation, final validation). A single helper with a
forceMinDecryptionflag will reduce maintenance risk.♻️ Refactor sketch
func (v *VaultKeyRotator) ForceKeyRotation(ctx context.Context) (int, error) { - // duplicated flow... - ... + return v.rotateAndValidate(ctx, true) } func (v *VaultKeyRotator) RotateKeyOnly(ctx context.Context) (int, error) { - // duplicated flow... - ... + return v.rotateAndValidate(ctx, false) } + +func (v *VaultKeyRotator) rotateAndValidate(ctx context.Context, forceMinDecryption bool) (int, error) { + initialVersion, err := v.getCurrentKeyVersion(ctx) + if err != nil { + return 0, fmt.Errorf("failed to get initial key version: %w", err) + } + if err := v.rotateKey(ctx); err != nil { + return 0, fmt.Errorf("failed to rotate key: %w", err) + } + newVersion, err := v.getCurrentKeyVersion(ctx) + if err != nil { + return 0, fmt.Errorf("failed to get new key version: %w", err) + } + if newVersion <= initialVersion { + return 0, fmt.Errorf("rotation failed: version did not increase (before=%d, after=%d)", initialVersion, newVersion) + } + if forceMinDecryption { + if err := v.updateMinDecryptionVersion(ctx, newVersion); err != nil { + return 0, fmt.Errorf("failed to update min_decryption_version: %w", err) + } + } + if err := v.ValidateKeyRotation(ctx, newVersion); err != nil { + return 0, fmt.Errorf("validation failed: %w", err) + } + return newVersion, nil +}🤖 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/kms/vault_key_rotation.go` around lines 63 - 132, Both ForceKeyRotation and RotateKeyOnly duplicate the same 5-step flow (getCurrentKeyVersion, rotateKey, getCurrentKeyVersion, check version increase, optional updateMinDecryptionVersion, ValidateKeyRotation); extract that shared flow into a single helper (e.g., performRotation(ctx context.Context, forceMinDecryption bool) (int, error)) that calls getCurrentKeyVersion, rotateKey, re-checks version, enforces the version-increase check, calls updateMinDecryptionVersion only when forceMinDecryption is true, and finally calls ValidateKeyRotation; then implement ForceKeyRotation to call performRotation(ctx, true) and RotateKeyOnly to call performRotation(ctx, false) to remove duplication while keeping existing helper method names (getCurrentKeyVersion, rotateKey, updateMinDecryptionVersion, ValidateKeyRotation).
🤖 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 `@test/library/encryption/kms/vault_key_rotation.go`:
- Around line 86-94: The code updates min_decryption_version via
v.updateMinDecryptionVersion(ctx, newVersion) but then only calls
v.ValidateKeyRotation(ctx, newVersion) which checks latest_version; change the
validation to assert that min_decryption_version actually equals newVersion and
enforces decryption gating: either extend ValidateKeyRotation to also fetch and
compare the stored min_decryption_version to newVersion or add a new check after
updateMinDecryptionVersion that calls a method (e.g., v.getKeyMetadata or
v.fetchMinDecryptionVersion) and verifies the persisted min_decryption_version
== newVersion and that decryption of data encrypted with older versions fails;
update ForceKeyRotation to use this stronger validation path (referencing
updateMinDecryptionVersion, ValidateKeyRotation, and ForceKeyRotation).
---
Nitpick comments:
In `@test/library/encryption/kms/vault_key_rotation.go`:
- Around line 63-132: Both ForceKeyRotation and RotateKeyOnly duplicate the same
5-step flow (getCurrentKeyVersion, rotateKey, getCurrentKeyVersion, check
version increase, optional updateMinDecryptionVersion, ValidateKeyRotation);
extract that shared flow into a single helper (e.g., performRotation(ctx
context.Context, forceMinDecryption bool) (int, error)) that calls
getCurrentKeyVersion, rotateKey, re-checks version, enforces the
version-increase check, calls updateMinDecryptionVersion only when
forceMinDecryption is true, and finally calls ValidateKeyRotation; then
implement ForceKeyRotation to call performRotation(ctx, true) and RotateKeyOnly
to call performRotation(ctx, false) to remove duplication while keeping existing
helper method names (getCurrentKeyVersion, rotateKey,
updateMinDecryptionVersion, ValidateKeyRotation).
🪄 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: 257059ea-a5b6-4d2e-a6c0-dac867470c7a
📒 Files selected for processing (1)
test/library/encryption/kms/vault_key_rotation.go
sandeepknd
force-pushed
the
vault-key-rotate
branch
from
e6d8357 to
0532af7
Compare
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 `@test/library/encryption/kms/vault_key_rotation.go`:
- Around line 114-116: The oc exec invocations use the caller ctx which may lack
a deadline and hang; wrap ctx with a bounded fallback timeout (e.g.,
defaultTimeout := 30*time.Second) when ctx.Err()==nil and ctx has no deadline by
creating ctxWithTimeout, using context.WithTimeout(ctx, defaultTimeout),
deferring cancel(), and pass ctxWithTimeout into exec.CommandContext for the
calls that build commands using v.podName, v.namespace, v.transitKeyName (the
"vault read -format=json transit/keys/..." call) and the subsequent oc exec
invocation around lines 135-137; ensure you only create the wrapper when
context.Deadline() returns false so existing deadlines are preserved.
🪄 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: 77ec3172-f5ef-43ae-9d3c-ceb6ef36325c
📒 Files selected for processing (1)
test/library/encryption/kms/vault_key_rotation.go
sandeepknd
force-pushed
the
vault-key-rotate
branch
from
0532af7 to
d586f5c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/kms/vault_key_rotation.go (1)
107-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a fallback timeout for
oc execto prevent hanging e2e flows.At Line 109 and Line 130,
exec.CommandContextuses the incoming context directly. If upstream passes a context without a deadline, these calls can hang indefinitely and stall tests.Proposed patch
import ( "context" "encoding/json" "fmt" "os/exec" + "time" "k8s.io/client-go/kubernetes" ) +const defaultOCExecTimeout = 2 * time.Minute + +func withDefaultTimeout(ctx context.Context) (context.Context, context.CancelFunc) { + if _, hasDeadline := ctx.Deadline(); hasDeadline { + return ctx, func() {} + } + return context.WithTimeout(ctx, defaultOCExecTimeout) +} + // GetKeyInfo returns information about the transit key including current version and configuration func (v *VaultKeyRotator) GetKeyInfo(ctx context.Context) (map[string]interface{}, error) { // Read key information: vault read transit/keys/<key-name> - cmd := exec.CommandContext(ctx, "oc", "exec", v.podName, "-n", v.namespace, "--", + cmdCtx, cancel := withDefaultTimeout(ctx) + defer cancel() + cmd := exec.CommandContext(cmdCtx, "oc", "exec", v.podName, "-n", v.namespace, "--", "vault", "read", "-format=json", fmt.Sprintf("transit/keys/%s", v.transitKeyName)) @@ func (v *VaultKeyRotator) rotateKey(ctx context.Context) error { @@ - cmd := exec.CommandContext(ctx, "oc", "exec", v.podName, "-n", v.namespace, "--", + cmdCtx, cancel := withDefaultTimeout(ctx) + defer cancel() + cmd := exec.CommandContext(cmdCtx, "oc", "exec", v.podName, "-n", v.namespace, "--", "vault", "write", "-f", fmt.Sprintf("transit/keys/%s/rotate", v.transitKeyName))Also applies to: 127-136
🤖 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/kms/vault_key_rotation.go` around lines 107 - 115, The oc exec calls in VaultKeyRotator.GetKeyInfo (and the similar exec.CommandContext usage around lines 127-136) can hang if the incoming ctx has no deadline; wrap the provided ctx with a child context that enforces a short timeout (e.g., 30s) only when ctx.Deadline() returns false, use ctxWithTimeout, defer cancel(), and pass that context into exec.CommandContext so the command is bounded; update both GetKeyInfo and the other exec call sites (referencing VaultKeyRotator, v.podName, v.namespace, v.transitKeyName and the other method where exec.CommandContext is used) accordingly.
🤖 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 `@test/library/encryption/kms/vault_key_rotation.go`:
- Around line 107-115: The oc exec calls in VaultKeyRotator.GetKeyInfo (and the
similar exec.CommandContext usage around lines 127-136) can hang if the incoming
ctx has no deadline; wrap the provided ctx with a child context that enforces a
short timeout (e.g., 30s) only when ctx.Deadline() returns false, use
ctxWithTimeout, defer cancel(), and pass that context into exec.CommandContext
so the command is bounded; update both GetKeyInfo and the other exec call sites
(referencing VaultKeyRotator, v.podName, v.namespace, v.transitKeyName and the
other method where exec.CommandContext is used) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0de628d-6a3b-4b93-870f-82ebf25a377f
📒 Files selected for processing (1)
test/library/encryption/kms/vault_key_rotation.go
sandeepknd
force-pushed
the
vault-key-rotate
branch
from
d586f5c to
52cbf55
Compare
sandeepknd
force-pushed
the
vault-key-rotate
branch
from
52cbf55 to
24fdd44
Compare
|
@sandeepknd: 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. |
sandeepknd
changed the title
|
@sandeepknd: This pull request references CNTRLPLANE-3364 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-robot
added
the
jira/valid-reference
3 participants
Added the vault kms key rotation function.
Currently two functions are defined :
first one only rotates the key - vault creates multiple versions after rotation; keeping the min_decryption_version same.
second one rotates the key and also updates the min_decryption_version field to the latest version resulting in only 1 key available at at time for decryption.
Let me know your thoughts if both functions are needed.
Summary by CodeRabbit