From d80774cb3fe6abcd0fea9dd0c68812ee94c7cf2a Mon Sep 17 00:00:00 2001 From: Br1an <932039080@qq.com> Date: Tue, 12 May 2026 07:27:03 +0800 Subject: [PATCH] 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 | 51 +++++++++++++-------------------------------------- admin_test.go | 24 +++++++++--------------- caddy.go | 7 ------- 3 files changed, 22 insertions(+), 60 deletions(-) diff --git a/admin.go b/admin.go index 08465a923..97af846ef 100644 --- a/admin.go +++ b/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 } diff --git a/admin_test.go b/admin_test.go index 19cc6ae7c..dda06a9e9 100644 --- a/admin_test.go +++ b/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)) - } }) } } diff --git a/caddy.go b/caddy.go index b3144299d..8799594a9 100644 --- a/caddy.go +++ b/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))