From 3adcafd4c1120f071e1c1b1407566780144e7fc9 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Tue, 17 Feb 2026 15:14:06 -0500 Subject: [PATCH] admin: Fix tests locally, properly isolate storage (#7486) * admin: Fix tests locally, properly isolate storage * Fix flaky pki_test * Drop testdata dir logic * Safer temp dir * Test handlers without a full server --- admin.go | 19 +++++++++++- admin_test.go | 49 +++++++++++++++++++++++++------ caddytest/integration/pki_test.go | 4 +-- 3 files changed, 60 insertions(+), 12 deletions(-) diff --git a/admin.go b/admin.go index 46f1bbda3..2eb9c3b04 100644 --- a/admin.go +++ b/admin.go @@ -47,6 +47,12 @@ import ( "go.uber.org/zap/zapcore" ) +// testCertMagicStorageOverride is a package-level test hook. Tests may set +// this variable to provide a temporary certmagic.Storage so that cert +// management in tests does not hit the real default storage on disk. +// This must NOT be set in production code. +var testCertMagicStorageOverride certmagic.Storage + func init() { // The hard-coded default `DefaultAdminListen` can be overridden // by setting the `CADDY_ADMIN` environment variable. @@ -633,8 +639,19 @@ func (ident *IdentityConfig) certmagicConfig(logger *zap.Logger, makeCache bool) // certmagic config, although it'll be mostly useless for remote management ident = new(IdentityConfig) } + // Choose storage: prefer the package-level test override when present, + // otherwise use the configured DefaultStorage. Tests may set an override + // to divert storage into a temporary location. Otherwise, in production + // we use the DefaultStorage since we don't want to act as part of a + // cluster; this storage is for the server's local identity only. + var storage certmagic.Storage + if testCertMagicStorageOverride != nil { + storage = testCertMagicStorageOverride + } else { + storage = DefaultStorage + } template := certmagic.Config{ - Storage: DefaultStorage, // do not act as part of a cluster (this is for the server's local identity) + Storage: storage, Logger: logger, Issuers: ident.issuers, } diff --git a/admin_test.go b/admin_test.go index 92dd43a5c..97dc76f4d 100644 --- a/admin_test.go +++ b/admin_test.go @@ -22,9 +22,11 @@ import ( "maps" "net/http" "net/http/httptest" + "os" "reflect" "sync" "testing" + "time" "github.com/caddyserver/certmagic" "github.com/prometheus/client_golang/prometheus" @@ -275,13 +277,12 @@ func TestAdminHandlerBuiltinRouteErrors(t *testing.T) { }, } - err := replaceLocalAdminServer(cfg, Context{}) + // Build the admin handler directly (no listener active) + addr, err := ParseNetworkAddress("localhost:2019") if err != nil { - t.Fatalf("setting up admin server: %v", err) + t.Fatalf("Failed to parse address: %v", err) } - defer func() { - stopAdminServer(localAdminServer) - }() + handler := cfg.Admin.newAdminHandler(addr, false, Context{}) tests := []struct { name string @@ -314,7 +315,7 @@ func TestAdminHandlerBuiltinRouteErrors(t *testing.T) { req := httptest.NewRequest(test.method, fmt.Sprintf("http://localhost:2019%s", test.path), nil) rr := httptest.NewRecorder() - localAdminServer.Handler.ServeHTTP(rr, req) + handler.ServeHTTP(rr, req) if rr.Code != test.expectedStatus { t.Errorf("expected status %d but got %d", test.expectedStatus, rr.Code) @@ -799,8 +800,24 @@ MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDRS0LmTwUT0iwP ... -----END PRIVATE KEY-----`) - testStorage := certmagic.FileStorage{Path: t.TempDir()} - err := testStorage.Store(context.Background(), "localhost/localhost.crt", certPEM) + tmpDir, err := os.MkdirTemp("", "TestManageIdentity-") + if err != nil { + t.Fatal(err) + } + testStorage := certmagic.FileStorage{Path: tmpDir} + // Clean up the temp dir after the test finishes. Ensure any background + // certificate maintenance is stopped first to avoid RemoveAll races. + t.Cleanup(func() { + if identityCertCache != nil { + identityCertCache.Stop() + identityCertCache = nil + } + // Give goroutines a moment to exit and release file handles. + time.Sleep(50 * time.Millisecond) + _ = os.RemoveAll(tmpDir) + }) + + err = testStorage.Store(context.Background(), "localhost/localhost.crt", certPEM) if err != nil { t.Fatal(err) } @@ -862,7 +879,7 @@ MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDRS0LmTwUT0iwP }, }, }, - storage: &certmagic.FileStorage{Path: "testdata"}, + storage: &testStorage, }, checkState: func(t *testing.T, cfg *Config) { if len(cfg.Admin.Identity.issuers) != 1 { @@ -900,6 +917,13 @@ MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDRS0LmTwUT0iwP identityCertCache.Stop() identityCertCache = nil } + // Ensure any cache started by manageIdentity is stopped at the end + defer func() { + if identityCertCache != nil { + identityCertCache.Stop() + identityCertCache = nil + } + }() ctx := Context{ Context: context.Background(), @@ -907,6 +931,13 @@ MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDRS0LmTwUT0iwP moduleInstances: make(map[string][]Module), } + // If this test provided a FileStorage, set the package-level + // testCertMagicStorageOverride so certmagicConfig will use it. + if test.cfg != nil && test.cfg.storage != nil { + testCertMagicStorageOverride = test.cfg.storage + defer func() { testCertMagicStorageOverride = nil }() + } + err := manageIdentity(ctx, test.cfg) if test.wantErr { diff --git a/caddytest/integration/pki_test.go b/caddytest/integration/pki_test.go index 846798209..3f1491e7e 100644 --- a/caddytest/integration/pki_test.go +++ b/caddytest/integration/pki_test.go @@ -53,7 +53,7 @@ func TestLeafCertLifetimeLessThanIntermediate(t *testing.T) { } } } - `, "json", "certificate lifetime (168h0m0s) should be less than intermediate certificate lifetime (168h0m0s)") + `, "json", "should be less than intermediate certificate lifetime") } func TestIntermediateLifetimeLessThanRoot(t *testing.T) { @@ -103,5 +103,5 @@ func TestIntermediateLifetimeLessThanRoot(t *testing.T) { } } } - `, "json", "intermediate certificate lifetime must be less than root certificate lifetime (86400h0m0s)") + `, "json", "intermediate certificate lifetime must be less than actual root certificate lifetime") }