feat(spire): enable persistent disk storage for keys#329
feat(spire): enable persistent disk storage for keys#329maishivamhoo123 wants to merge 5 commits intocontainer-registry:mainfrom
Conversation
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
📝 WalkthroughWalkthroughSwitch the embedded SPIRE server KeyManager from in-memory to disk-backed, add a keys_path template parameter, update config write call to include dataDir, and add unit and end-to-end tests that validate disk persistence of SPIRE keys across restarts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![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
🤖 Fix all issues with AI agents
In `@ground-control/internal/spiffe/persistence_test.go`:
- Around line 17-23: In TestSpireConfigUsesDiskPersistence replace the manual
temp dir creation (os.MkdirTemp + err check + defer os.RemoveAll) with
t.TempDir() — call tmpDir := t.TempDir() and remove the os.MkdirTemp error
handling and defer os.RemoveAll(tmpDir) lines so the test uses the testing
package’s automatic cleanup.
- Around line 97-102: Replace manual temporary directory creation using
os.MkdirTemp and explicit cleanup with the testing helper t.TempDir(): remove
the error handling and defer os.RemoveAll(tmpDir) around the tmpDir variable and
instead assign tmpDir := t.TempDir() inside the test (where tmpDir and err are
currently declared), so the test runtime handles cleanup automatically; update
references to tmpDir accordingly (look for the tmpDir variable in this test in
persistence_test.go).
- Around line 120-125: The goroutine calling t.Logf (inside the anonymous
goroutine invoking server1.Start(ctx1) and likewise for the server2 block around
lines 152-156) can race with the test and panic; instead either send the error
back to the test via a channel (or use a sync.WaitGroup) and call t.Logf from
the main test goroutine, or replace the in-goroutine t.Logf with a non-test
logger such as fmt.Fprintf(os.Stderr, ...) so logging does not use the testing.T
from a non-test goroutine; ensure the goroutine's lifetime is coordinated with
the test (via context cancellation or waiting) so Start(ctx1) cannot outlive the
test.
🧹 Nitpick comments (4)
ground-control/internal/spiffe/embedded_server.go (2)
195-199: Inconsistent indentation in HCL config template.Lines 197–198 use hard tabs while the rest of the template uses spaces. This produces misaligned output in the generated
server.conf, which hurts readability and could confuse HCL-aware tooling.🔧 Align indentation with the rest of the template
KeyManager "disk" { plugin_data { - keys_path = "%s/keys.json" - } + keys_path = "%s/keys.json" + } }
174-209: Consider HCL injection risk from unescapedDataDirinfmt.Sprintf.
DataDiris interpolated into the HCL config string six times (including the newkeys_path) without any escaping. IfDataDircontains a"or newline, the generated config will be malformed or could alter the config semantics. While unlikely in normal usage, a defensivestrings.ContainsAnycheck (or using an HCL library) would harden this.ground-control/internal/spiffe/persistence_test.go (2)
36-46: Callingserver.Start()just to triggerwriteConfig()is unnecessarily heavy.
Start()also launches a subprocess and blocks waiting for readiness. Since the test only cares about the generated config file, calling the unexportedwriteConfig()directly (or exporting it / extracting the config-generation logic) would make this test faster, deterministic, and free of goroutine/sleep hacks.As a simpler alternative within the current design, note that
writeConfigis an unexported method on the same package—you can call it directly from the test:♻️ Simplify by calling writeConfig directly
server := NewEmbeddedSpireServer(cfg) - // 3. Trigger config writing by attempting to start - // We run this in a goroutine because Start() might block or fail if binary is missing - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - - go func() { - _ = server.Start(ctx) - }() - - // Give it a tiny moment to write the file - time.Sleep(100 * time.Millisecond) + // Trigger config writing directly (same package, unexported method accessible) + if err := os.MkdirAll(cfg.DataDir, 0o700); err != nil { + t.Fatal(err) + } + if err := server.writeConfig(); err != nil { + t.Fatal(err) + }This also eliminates the retry loop (lines 52–60) since the file is guaranteed to exist after
writeConfigreturns.
88-94: This is an E2E/integration test—consider gating it with a build tag or test flag.The
TestEndToEndPersistencetest starts real SPIRE server subprocesses, sleeps for seconds, and only skips at runtime if the binary is absent. A build tag (e.g.,//go:build integration) or-shortflag check would let CI and localgo testskip it by default, keeping the fast unit-test path clean.
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ground-control/internal/spiffe/persistence_test.go`:
- Around line 73-96: The background goroutine calling server1.Start must
propagate errors and you must avoid blind sleeps: create an error channel
(errCh) and start server1 in a goroutine that sends any Start error to errCh;
wait (with a timeout context) for either Start to return successfully or an
error on errCh instead of sleeping; after the server is confirmed running, call
server1.Stop() explicitly (not just canceling exec context) to allow graceful
shutdown and ensure keys.json is flushed, then check keys.json existence;
reference server1.Start, server1.Stop, ctx1/cancel1 and keysPath when making
these changes.
🧹 Nitpick comments (4)
ground-control/internal/spiffe/embedded_server.go (1)
195-199: Minor indentation inconsistency in the config template.Line 195 uses 3-space indent (
KeyManager) while the sibling plugin blocks (DataStoreon Line 184,NodeAttestoron Line 191) use 4-space indent. Not functionally impactful (HCL is whitespace-tolerant), but worth aligning for readability.🔧 Suggested fix
- KeyManager "disk" { + KeyManager "disk" {ground-control/internal/spiffe/persistence_test.go (3)
32-35: Redundantos.MkdirAll—t.TempDir()already creates the directory.
setupTestConfigusest.TempDir()which returns an already-existing directory. Theos.MkdirAllon line 33 is a no-op.♻️ Suggested fix
cfg := setupTestConfig(t) - // Create DataDir explicitly - if err := os.MkdirAll(cfg.DataDir, 0700); err != nil { - t.Fatal(err) - } - server := NewEmbeddedSpireServer(cfg)
54-60: Consider also verifying the exactkeys_pathvalue, not just its presence.The test checks that
keys_path =exists but doesn't verify it points to the correct directory. A stronger assertion would be:expectedKeysPath := fmt.Sprintf(`keys_path = "%s/keys.json"`, cfg.DataDir) if !strings.Contains(content, expectedKeysPath) { t.Errorf("Config has wrong keys_path. Expected %q in:\n%s", expectedKeysPath, content) }
7-7: Remove the inline comment on the import.
// Added this for LookPathis commit-log noise that doesn't belong in the source.- "os/exec" // Added this for LookPath + "os/exec"
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ground-control/internal/spiffe/persistence_test.go`:
- Around line 94-98: The test currently uses os.Stat(keysPath) with
os.IsNotExist(err) which ignores other errors; change the check to call
os.Stat(keysPath), if err != nil then call t.Fatalf or t.Errorf with the actual
err to fail the test (instead of only checking os.IsNotExist), otherwise proceed
— update the block referencing keysPath, cfg.DataDir, os.Stat and t.Error so any
non-nil err (permission, IO, etc.) causes the test to fail and prints the error
details.
🧹 Nitpick comments (2)
ground-control/internal/spiffe/persistence_test.go (2)
73-76: E2E test should live intest/e2e/per project conventions.This is an end-to-end test (binary lookup, full server lifecycle) placed in the unit-test file. The project convention requires E2E tests in
test/e2e/with configurations intest/e2e/testconfig/. Thet.Skipguard mitigates CI breakage, but consider relocating this test to the correct directory.Based on learnings: "E2E tests must be located in test/e2e/ with test configurations in test/e2e/testconfig/"
100-109: Server 2 restart block doesn't assert anything beyond "no crash".After restarting the server with persisted keys, consider asserting that
keys.jsonstill exists (or hasn't changed) and potentially that the server is actually healthy — otherwise the restart test only proves the binary can start twice, not that it reused the persisted keys.
Signed-off-by: maishivamhoo123 <maishivamhoo@gmail.com>
1 participant
Description
This PR enables persistent storage for SPIRE Server keys, resolving an issue where keys were lost upon restart (previously stored in
memory).Changes
KeyManager: Switched from"memory"to"disk"ininternal/spiffe/embedded_server.go.keys_pathto<data_dir>/keys.json.internal/spiffe/persistence_test.goto verify:server.conf(checking for"disk"andkeys_path).spire-serverbinary is missing).Additional context
Verified locally with
go test ./internal/spiffe/....Summary by cubic
Enable persistent disk storage for SPIRE Server keys so they survive restarts, replacing the in-memory KeyManager. Addresses #288.
Written for commit fed3be5. Summary will update on new commits.
Summary by CodeRabbit