metrics: Add nil check for metricsHandler in AdminMetrics.serveHTTP (#7553)

* metrics: Add nil check for metricsHandler in AdminMetrics.serveHTTP

Prevents panic when the admin metrics endpoint is accessed before
the module is fully provisioned. Returns a proper API error instead
of crashing.

* admin: provision router modules before registering routes

Instead of adding a nil check for metricsHandler, address the root
cause by provisioning admin router modules before calling Routes().
This ensures all handler state is initialized before routes are
registered on the mux.

Merge newAdminHandler and provisionAdminRouters into a single step,
removing the two-phase setup where routes were registered first and
modules provisioned later. The AdminConfig.routers field is no longer
needed since provisioning happens inline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: go fmt admin.go

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

admin.go

@@ -120,10 +120,6 @@ type AdminConfig struct {
 	//
 	// EXPERIMENTAL: This feature is subject to change.
 	Remote *RemoteAdmin `json:"remote,omitempty"`
-
-	// Holds onto the routers so that we can later provision them
-	// if they require provisioning.
-	routers []AdminRouter
 }
 
 // ConfigSettings configures the management of configuration.
@@ -222,7 +218,7 @@ type AdminPermissions struct {
 
 // newAdminHandler reads admin's config and returns an http.Handler suitable
 // for use in an admin endpoint server, which will be listening on listenAddr.
-func (admin *AdminConfig) newAdminHandler(addr NetworkAddress, remote bool, _ Context) adminHandler {
+func (admin *AdminConfig) newAdminHandler(addr NetworkAddress, remote bool, ctx Context) (adminHandler, error) {
 	muxWrap := adminHandler{mux: http.NewServeMux()}
 
 	// secure the local or remote endpoint respectively
@@ -279,34 +275,21 @@ func (admin *AdminConfig) newAdminHandler(addr NetworkAddress, remote bool, _ Co
 	// register third-party module endpoints
 	for _, m := range GetModules("admin.api") {
 		router := m.New().(AdminRouter)
+
+		// provision the router before registering its routes, so
+		// handlers have access to all provisioned state
+		if provisioner, ok := router.(Provisioner); ok {
+			if err := provisioner.Provision(ctx); err != nil {
+				return adminHandler{}, fmt.Errorf("provisioning admin router module %s: %v", m.ID, err)
+			}
+		}
+
 		for _, route := range router.Routes() {
 			addRoute(route.Pattern, handlerLabel, route.Handler)
 		}
-		admin.routers = append(admin.routers, router)
 	}
 
-	return muxWrap
-}
-
-// provisionAdminRouters provisions all the router modules
-// in the admin.api namespace that need provisioning.
-func (admin *AdminConfig) provisionAdminRouters(ctx Context) error {
-	for _, router := range admin.routers {
-		provisioner, ok := router.(Provisioner)
-		if !ok {
-			continue
-		}
-
-		err := provisioner.Provision(ctx)
-		if err != nil {
-			return err
-		}
-	}
-
-	// We no longer need the routers once provisioned, allow for GC
-	admin.routers = nil
-
-	return nil
+	return muxWrap, nil
 }
 
 // allowedOrigins returns a list of origins that are allowed.
@@ -430,11 +413,7 @@ func replaceLocalAdminServer(cfg *Config, ctx Context) error {
 		return err
 	}
 
-	handler := cfg.Admin.newAdminHandler(addr, false, ctx)
-
-	// run the provisioners for loaded modules to make sure local
-	// state is properly re-initialized in the new admin server
-	err = cfg.Admin.provisionAdminRouters(ctx)
+	handler, err := cfg.Admin.newAdminHandler(addr, false, ctx)
 	if err != nil {
 		return err
 	}
@@ -558,11 +537,7 @@ func replaceRemoteAdminServer(ctx Context, cfg *Config) error {
 
 	// make the HTTP handler but disable Host/Origin enforcement
 	// because we are using TLS authentication instead
-	handler := cfg.Admin.newAdminHandler(addr, true, ctx)
-
-	// run the provisioners for loaded modules to make sure local
-	// state is properly re-initialized in the new admin server
-	err = cfg.Admin.provisionAdminRouters(ctx)
+	handler, err := cfg.Admin.newAdminHandler(addr, true, ctx)
 	if err != nil {
 		return err
 	}

admin_test.go

@@ -340,7 +340,10 @@ func TestAdminHandlerBuiltinRouteErrors(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Failed to parse address: %v", err)
 	}
-	handler := cfg.Admin.newAdminHandler(addr, false, Context{})
+	handler, err := cfg.Admin.newAdminHandler(addr, false, Context{})
+	if err != nil {
+		t.Fatalf("Failed to create admin handler: %v", err)
+	}
 
 	tests := []struct {
 		name           string
@@ -461,7 +464,10 @@ func TestNewAdminHandlerRouterRegistration(t *testing.T) {
 	admin := &AdminConfig{
 		EnforceOrigin: false,
 	}
-	handler := admin.newAdminHandler(addr, false, Context{})
+	handler, err := admin.newAdminHandler(addr, false, Context{})
+	if err != nil {
+		t.Fatalf("Failed to create admin handler: %v", err)
+	}
 
 	req := httptest.NewRequest("GET", "/mock", nil)
 	req.Host = "localhost:2019"
@@ -473,10 +479,6 @@ func TestNewAdminHandlerRouterRegistration(t *testing.T) {
 		t.Errorf("Expected status code %d but got %d", http.StatusOK, rr.Code)
 		t.Logf("Response body: %s", rr.Body.String())
 	}
-
-	if len(admin.routers) != 1 {
-		t.Errorf("Expected 1 router to be stored, got %d", len(admin.routers))
-	}
 }
 
 type mockProvisionableRouter struct {
@@ -514,19 +516,16 @@ func TestAdminRouterProvisioning(t *testing.T) {
 		name         string
 		provisionErr error
 		wantErr      bool
-		routersAfter int // expected number of routers after provisioning
 	}{
 		{
 			name:         "successful provisioning",
 			provisionErr: nil,
 			wantErr:      false,
-			routersAfter: 0,
 		},
 		{
 			name:         "provisioning error",
 			provisionErr: fmt.Errorf("provision failed"),
 			wantErr:      true,
-			routersAfter: 1,
 		},
 	}
 
@@ -562,8 +561,7 @@ func TestAdminRouterProvisioning(t *testing.T) {
 				t.Fatalf("Failed to parse address: %v", err)
 			}
 
-			_ = admin.newAdminHandler(addr, false, Context{})
-			err = admin.provisionAdminRouters(Context{})
+			_, err = admin.newAdminHandler(addr, false, Context{})
 
 			if test.wantErr {
 				if err == nil {
@@ -574,10 +572,6 @@ func TestAdminRouterProvisioning(t *testing.T) {
 					t.Errorf("Expected no error but got: %v", err)
 				}
 			}
-
-			if len(admin.routers) != test.routersAfter {
-				t.Errorf("Expected %d routers after provisioning, got %d", test.routersAfter, len(admin.routers))
-			}
 		})
 	}
 }

caddy.go

@@ -440,13 +440,6 @@ func run(newCfg *Config, start bool) (Context, error) {
 		}
 	}()
 
-	// Provision any admin routers which may need to access
-	// some of the other apps at runtime
-	err = ctx.cfg.Admin.provisionAdminRouters(ctx)
-	if err != nil {
-		return ctx, err
-	}
-
 	// Start
 	err = func() error {
 		started := make([]string, 0, len(ctx.cfg.apps))