feat(encryption): Stage 6D-6b - elastickv-admin enable-storage-envelope CLI subcommand#820
Conversation
…pe CLI subcommand Stage 6D-6b adds the operator-facing CLI surface for the EnableStorageEnvelope RPC that landed in 6D-6a. The remaining 6D-6c slice will wire the cipher + capability-fanout closure in main.go and add the end-to-end integration test. ## CLI wiring (cmd/elastickv-admin/) - encryption.go dispatch picks up `enable-storage-envelope`; the help string in the -h/--help/help branches lists the new subcommand. - encryption_mutators.go ships runEncryptionEnableStorageEnvelope + parseEnableStorageEnvelopeArgs + printEnableStorageEnvelopeResult. - CLI-side validation duplicates the §6.1 proposer_node_id != 0 sentinel and §4.1 16-bit proposer_local_epoch bound so an operator with a misconfigured shell variable fails fast before the round-trip; the server re-validates as the source of truth. ## Output shape (shell-friendly) Stable line prefixes so automation can switch on column 1 of the result line: - Fresh success: `enabled applied_index=N` followed by a `capability summary:` block listing each probed member. - Idempotent retry (§6.4 was_already_active=true): `already-active applied_index=N` where N is the ORIGINAL StorageEnvelopeCutoverIndex (NOT this call's Raft index). Capability summary intentionally omitted per §3.1. - Defensive fallback (§6.4 cutover_index_unknown=true): a `warning: cutover_index_unknown=true (sidecar may have been hand-edited or rolled back)` line BEFORE the already-active result so operators grepping on the warning substring can flag clusters that need investigation. ## Tests (cmd/elastickv-admin/encryption_test.go) - stubMutatorServer extended with EnableStorageEnvelope: new enableEnvelopeCalls slice captures the proto requests for wire-level assertions; new enableEnvelopeResp field lets tests pin the exact response shape (fresh-success vs idempotent vs defensive). - TestRunEncryptionEnableStorageEnvelope_HappyPath - fresh success: asserts the `enabled` prefix, the per-member verdict rows in the capability summary, and the wire-level proto request matches the flag inputs. - TestRunEncryptionEnableStorageEnvelope_IdempotentRetry - was_already_active=true response: asserts the `already-active` prefix, the absence of the capability summary header, and the absence of any warning. - TestRunEncryptionEnableStorageEnvelope_DefensiveCutoverIndexUnknown - was_already_active=true + cutover_index_unknown=true: asserts the warning line precedes the already-active result. - TestRunEncryptionEnableStorageEnvelope_RejectsZeroProposerNodeID - §6.1 sentinel rejection at the CLI side before any RPC. - TestRunEncryptionEnableStorageEnvelope_RejectsBadEpoch - §4.1 16-bit bound at the CLI side. - TestEncryptionMain_EnableStorageEnvelopeSubcommand - dispatch routing pin: a typo in encryptionMain's switch statement would route to the default branch and the user would see "unknown subcommand" rather than the runner's --proposer-node-id error. ## Caller audit (semantic change) This PR is purely additive: - The new subcommand is reachable only via `elastickv-admin encryption enable-storage-envelope` — no existing dispatch path is touched. - stubMutatorServer gains two new fields with zero-value defaults; existing tests (rotate-dek, register-writer, bootstrap) continue to use the same fixture unchanged. - main.go does NOT wire the EnableStorageEnvelope server method's capability-fanout closure yet (lands in 6D-6c), so the new subcommand will refuse with FailedPrecondition until 6D-6c ships. Operator-inert at this stage. ## Verification - go test -race -timeout 60s ./cmd/elastickv-admin/... - all green - golangci-lint --new-from-rev=origin/main - 0 issues Refs: design doc §3.1 (Stage 6D-6b slice of the §7 decomposition table; 6D-6c remains).
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 42 minutes and 18 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements the ChangesEnable Storage Envelope CLI Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the enable-storage-envelope subcommand to the elastickv-admin encryption CLI, facilitating the transition to storage envelope encryption. The changes include a refactored subcommand dispatch mechanism, the implementation of the new command's logic with robust flag validation, and comprehensive unit tests covering various operational outcomes. Documentation was also updated to reflect the completion of this milestone. Feedback was provided to align the docstring examples in encryption_mutators.go with the actual output format implemented in the code.
| // fresh: "enabled storage envelope applied_index=N | ||
| // capability summary: ..." | ||
| // already-on: "storage envelope already active (idempotent | ||
| // retry) applied_index=N" | ||
| // defensive: "warning: cutover_index_unknown=true" |
There was a problem hiding this comment.
The docstring for runEncryptionEnableStorageEnvelope contains example output strings that do not match the actual implementation in printEnableStorageEnvelopeResult. Specifically, the implementation uses more concise prefixes (enabled and already-active) and includes additional detail in the warning message. Updating the docstring to match the implementation will prevent confusion for future maintainers.
| // fresh: "enabled storage envelope applied_index=N | |
| // capability summary: ..." | |
| // already-on: "storage envelope already active (idempotent | |
| // retry) applied_index=N" | |
| // defensive: "warning: cutover_index_unknown=true" | |
| // fresh: "enabled applied_index=N | |
| // capability summary: ..." | |
| // already-on: "already-active applied_index=N" | |
| // defensive: "warning: cutover_index_unknown=true (sidecar may have been hand-edited or rolled back)" |
![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 (2)
cmd/elastickv-admin/encryption_mutators.go (1)
145-148: ⚡ Quick winUse structured
slogfor the close-error path in this new subcommand.This newly-added logging path emits unstructured stderr text. Please switch to
slogwith stable keys to match repo logging policy.♻️ Proposed change
import ( "context" "encoding/base64" "flag" "fmt" "io" + "log/slog" "math" "os" "strconv" "strings" @@ defer func() { if err := closeFn(); err != nil { - fmt.Fprintf(os.Stderr, "encryption: close connection: %v\n", err) + slog.Error("encryption close connection", + "subcommand", "enable-storage-envelope", + "endpoint", *endpoint.endpoint, + "error", err, + ) } }()As per coding guidelines, "Use structured
sloglogging with stable keys (key,commit_ts,route_id, etc.) for logging throughout the codebase."🤖 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 `@cmd/elastickv-admin/encryption_mutators.go` around lines 145 - 148, Replace the unstructured fmt.Fprintf call inside the defer that invokes closeFn() with structured slog logging: import log/slog if not already present, and in the defer's error branch call slog.Error (or slog.Log with LevelError) with a clear message and stable keys such as "component":"encryption", "action":"close_connection" and "error":err (or "err":err) so the close error is emitted as structured logs; update the defer that calls closeFn() (reference closeFn in encryption_mutators.go) accordingly.cmd/elastickv-admin/encryption.go (1)
46-53: ⚡ Quick winDerive the supported-subcommand list from the dispatch table.
The help text and unknown-subcommand error each hardcode command names, separate from
encryptionSubcommands(). This can drift on the next subcommand addition/removal.♻️ Proposed refactor
import ( "context" "flag" "fmt" "io" "os" "slices" "strconv" + "strings" "time" @@ +func encryptionSupportedSubcommands() []string { + keys := make([]string, 0, len(encryptionSubcommands())) + for k := range encryptionSubcommands() { + keys = append(keys, k) + } + slices.Sort(keys) + return keys +} + func encryptionMain(args []string) error { @@ if sub == "-h" || sub == "--help" || sub == "help" { + supported := strings.Join(encryptionSupportedSubcommands(), "\n ") - _, err := fmt.Fprintln(os.Stdout, "usage: elastickv-admin encryption <subcommand> [flags]\n\nsubcommands:\n status\n rotate-dek\n register-writer\n bootstrap\n enable-storage-envelope\n probe-node-id") + _, err := fmt.Fprintf(os.Stdout, "usage: elastickv-admin encryption <subcommand> [flags]\n\nsubcommands:\n %s\n", supported) if err != nil { return errors.Wrap(err, "write usage") } return nil } - return errors.Errorf("encryption: unknown subcommand %q (supported: status, rotate-dek, register-writer, bootstrap, enable-storage-envelope, probe-node-id)", sub) + return errors.Errorf("encryption: unknown subcommand %q (supported: %s)", sub, strings.Join(encryptionSupportedSubcommands(), ", ")) }🤖 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 `@cmd/elastickv-admin/encryption.go` around lines 46 - 53, The usage text and unknown-subcommand error should be generated from the dispatch table returned by encryptionSubcommands() instead of hardcoding names; query encryptionSubcommands(), collect its keys into a slice (sort them for stable output), join them with ", " (or format into the multiline usage string) and use that generated string in both the fmt.Fprintln usage and the errors.Errorf unknown-subcommand message (referencing the existing local variable sub for the unknown value). This ensures the displayed supported-subcommand list is always derived from encryptionSubcommands().
🤖 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 `@cmd/elastickv-admin/encryption.go`:
- Around line 30-32: The Stage-6 comment is out of sync: it mentions
`enable-raft-envelope` but this PR actually wires the `probe-node-id` CLI flag;
update the comment in encryption.go to describe that Stage-6 adds
`probe-node-id` (and retains `enable-storage-envelope`/ResyncSidecar behavior)
so the text matches the implemented CLI surface and references the
`probe-node-id` symbol instead of `enable-raft-envelope`.
---
Nitpick comments:
In `@cmd/elastickv-admin/encryption_mutators.go`:
- Around line 145-148: Replace the unstructured fmt.Fprintf call inside the
defer that invokes closeFn() with structured slog logging: import log/slog if
not already present, and in the defer's error branch call slog.Error (or
slog.Log with LevelError) with a clear message and stable keys such as
"component":"encryption", "action":"close_connection" and "error":err (or
"err":err) so the close error is emitted as structured logs; update the defer
that calls closeFn() (reference closeFn in encryption_mutators.go) accordingly.
In `@cmd/elastickv-admin/encryption.go`:
- Around line 46-53: The usage text and unknown-subcommand error should be
generated from the dispatch table returned by encryptionSubcommands() instead of
hardcoding names; query encryptionSubcommands(), collect its keys into a slice
(sort them for stable output), join them with ", " (or format into the multiline
usage string) and use that generated string in both the fmt.Fprintln usage and
the errors.Errorf unknown-subcommand message (referencing the existing local
variable sub for the unknown value). This ensures the displayed
supported-subcommand list is always derived from encryptionSubcommands().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: add537ea-28f2-478e-9342-fd6e3640a094
📒 Files selected for processing (4)
cmd/elastickv-admin/encryption.gocmd/elastickv-admin/encryption_mutators.gocmd/elastickv-admin/encryption_test.godocs/design/2026_05_18_partial_6d_enable_storage_envelope.md
…string drift Round-1 review surfaced two documentation-accuracy findings plus one quick-win style suggestion. Addressing the docs; skipping the slog suggestion because the other mutator runners (runEncryptionRotateDEK, runEncryptionRegisterWriter, runEncryptionBootstrap) all use the same fmt.Fprintf(os.Stderr, ...) close-error pattern - changing only the new runner would introduce local style drift. ## gemini medium - runEncryptionEnableStorageEnvelope docstring drift Pre-fix: the docstring example output strings referenced 'enabled storage envelope applied_index=N' / 'storage envelope already active (idempotent retry)' but the actual printEnableStorageEnvelopeResult implementation uses the more concise 'enabled' / 'already-active' prefixes that the CLI tests pin as the wire-level shell-script contract. Fix: rewrite the docstring's example block to match the implementation verbatim. The new block names all three outcomes (fresh / already-on / defensive) with the exact prefixes a script will see, including the parenthetical '(sidecar may have been hand-edited or rolled back)' suffix on the warning line. ## coderabbit minor - encryption.go Stage-6 comment drift Pre-fix: the dispatch comment said 'Stage 6 adds enable-storage-envelope and enable-raft-envelope' but this PR wires only enable-storage-envelope; enable-raft-envelope is a §7.1 Phase 2 surface that lands in Stage 6E. Fix: rewrite the dispatch comment's lifecycle history to match shipped reality. Calls out 6D-2 (probe-node-id) which was missing from the original comment, names 6D-6b as the PR that wires enable-storage-envelope, and defers enable-raft-envelope to Stage 6E. ## coderabbit quick-win (skipped) - slog vs fmt.Fprintf in close-err path Pre-fix and current state: the new runEncryptionEnableStorageEnvelope uses the same fmt.Fprintf(os.Stderr, ...) close-error path as runEncryptionRotateDEK / runEncryptionRegisterWriter / runEncryptionBootstrap. Coderabbit suggested swapping in slog.Error with structured keys. Skipped for this PR: changing only this one site creates local style drift across the four mutator runners. The other three runners would need the same conversion, and that scope is better landed as a separate cleanup PR that touches all four together. Recording the suggestion here so a future 'CLI logging consistency' pass can pick it up. ## Caller audit Docstring-only changes; no behaviour change, no test impact. ## Verification - go test -race -timeout 60s ./cmd/elastickv-admin/... - all green - golangci-lint --new-from-rev=origin/main - 0 issues
|
@claude review Round-1 (commit gemini medium — runEncryptionEnableStorageEnvelope docstring driftPre-fix: docstring example output strings referenced 'enabled storage envelope applied_index=N' / 'storage envelope already active (idempotent retry)' but the actual Fix: rewrote the docstring's example block to match the implementation verbatim. New block names all three outcomes (fresh / already-on / defensive) with the exact prefixes a script will see, including the coderabbit minor — encryption.go Stage-6 dispatch comment driftPre-fix: dispatch comment said 'Stage 6 adds enable-storage-envelope and enable-raft-envelope' but this PR wires only Fix: rewrote the lifecycle history to match shipped reality. Now calls out 6D-2 ( coderabbit quick-win (skipped) — slog vs fmt.Fprintf in close-err pathThe new runner uses the same Caller auditDocstring-only changes; no behaviour change, no test impact. Verification
|
|
Claude finished @bootjp's task in 2m 1s —— View job Review — Round-1 fixes verified, no new blockers
Round-1 fixesBoth Round-1 fixes land correctly. Gemini docstring fix ( CodeRabbit comment drift ( slog quick-win (deferred): the rationale — changing only the new site while the existing three runners use Findings from this review passNo correctness blockers. Two minor observations worth flagging: 1. Missing help-flag test for the new subcommand
2. Stale
|
1 participant
Summary
EnableStorageEnvelopeRPC that landed in 6D-6a. 6D-6c will wire the cipher + capability-fanout closure inmain.goand add the end-to-end integration test.elastickv-admin encryption enable-storage-envelopesubcommand. The runner dispatches the RPC and renders the §3.1 response with shell-friendly stable line prefixes:enabled applied_index=N+ per-membercapability summary:blockwas_already_active=true):already-active applied_index=N(where N is the ORIGINALStorageEnvelopeCutoverIndex, not this call's Raft index); summary intentionally omitted per §3.1cutover_index_unknown=true): precedingwarning: cutover_index_unknown=true …line so operators grepping on the warning can flag clusters that need investigationproposer_node_id != 0sentinel and §4.1 16-bitproposer_local_epochbound so an operator with a misconfigured shell variable fails fast before the round-trip; the server re-validates as the source of truth.encryptionMaindispatch refactored to a name→handler map so adding the new subcommand keeps the function under the cyclop budget.Test plan
go test -race -timeout 60s ./cmd/elastickv-admin/...— all greengolangci-lint run(full repo) — 0 issuesTestRunEncryptionEnableStorageEnvelope_HappyPath— fresh-success prefix + capability summary rows + wire-level proto request matches flag inputsTestRunEncryptionEnableStorageEnvelope_IdempotentRetry—already-activeprefix; no summary header; no warningTestRunEncryptionEnableStorageEnvelope_DefensiveCutoverIndexUnknown— warning line precedes the already-active resultTestRunEncryptionEnableStorageEnvelope_RejectsZeroProposerNodeID— §6.1 sentinel rejection at the CLI side before any RPCTestRunEncryptionEnableStorageEnvelope_RejectsBadEpoch— §4.1 16-bit bound at the CLI sideTestEncryptionMain_EnableStorageEnvelopeSubcommand— dispatch routing pinCaller audit (semantic change)
This PR is purely additive:
elastickv-admin encryption enable-storage-envelope— no existing dispatch path is touched.stubMutatorServergains two new fields with zero-value defaults; existing rotate / register / bootstrap test fixtures continue to use the same struct unchanged.main.godoes NOT wire the server method'sWithEncryptionAdminCapabilityFanoutclosure yet (lands in 6D-6c), so the new subcommand will refuse withFailedPreconditionuntil 6D-6c ships. Operator-inert at this stage.Refs: design doc §3.1 (Stage 6D-6b slice; 6D-6c remains).
Summary by CodeRabbit
New Features
elastickv-admin encryption enable-storage-envelopesubcommand to enable storage envelope with specified proposer identity parameters. Output displays success status, applied index, and capability information with warnings when applicable.Documentation