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

@@ -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,
 	}

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() {
+	handler := cfg.Admin.newAdminHandler(addr, false, Context{})
-		stopAdminServer(localAdminServer)
-	}()
 
 	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()}
+	tmpDir, err := os.MkdirTemp("", "TestManageIdentity-")
-	err := testStorage.Store(context.Background(), "localhost/localhost.crt", certPEM)
+	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 {

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")
 }