From 5e3ad77fc092058d48a3e3d242d76921ade1a56a Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Fri, 30 Jun 2023 08:10:20 -0500 Subject: [PATCH] Backport of feature - [NET - 4005] - [Supportability] Reloadable Configuration - enable_debug into release/1.16.x (#17969) * backport of commit 10f500e895d92cc3691ade7b74a33db755d22039 * backport of commit e08c30910167fa2c6dd5a639b92338d2389457e4 * backport of commit 58638deeb3ac16b6556bba6a2b24034d7f886cce * merge conf resolve --------- Co-authored-by: Ashesh Vidyut Co-authored-by: Ashesh Vidyut <134911583+absolutelightning@users.noreply.github.com> --- .changelog/17565.txt | 3 +++ agent/agent.go | 12 ++++++++-- agent/agent_endpoint_test.go | 8 ++++--- agent/agent_test.go | 33 ++++++++++++++++++++++++++++ agent/config/runtime_test.go | 2 +- agent/http.go | 23 ++++++++++++-------- agent/http_oss_test.go | 7 ++++-- agent/http_test.go | 41 +++++++++++++++++++++++++---------- agent/ui_endpoint_oss_test.go | 4 +++- agent/ui_endpoint_test.go | 4 +++- 10 files changed, 107 insertions(+), 30 deletions(-) create mode 100644 .changelog/17565.txt diff --git a/.changelog/17565.txt b/.changelog/17565.txt new file mode 100644 index 000000000..f7cf46c38 --- /dev/null +++ b/.changelog/17565.txt @@ -0,0 +1,3 @@ +```release-note:feature +reloadable config: Made enable_debug config reloadable and enable pprof command to work when config toggles to true +``` \ No newline at end of file diff --git a/agent/agent.go b/agent/agent.go index 90bfffc1a..fa75a1cd1 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -19,6 +19,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "github.com/armon/go-metrics" @@ -415,6 +416,8 @@ type Agent struct { // enterpriseAgent embeds fields that we only access in consul-enterprise builds enterpriseAgent + + enableDebug atomic.Bool } // New process the desired options and creates a new Agent. @@ -597,6 +600,8 @@ func (a *Agent) Start(ctx context.Context) error { // Overwrite the configuration. a.config = c + a.enableDebug.Store(c.EnableDebug) + if err := a.tlsConfigurator.Update(a.config.TLS); err != nil { return fmt.Errorf("Failed to load TLS configurations after applying auto-config settings: %w", err) } @@ -1126,13 +1131,13 @@ func (a *Agent) listenHTTP() ([]apiServer, error) { httpServer := &http.Server{ Addr: l.Addr().String(), TLSConfig: tlscfg, - Handler: srv.handler(a.config.EnableDebug), + Handler: srv.handler(), MaxHeaderBytes: a.config.HTTPMaxHeaderBytes, } if scada.IsCapability(l.Addr()) { // wrap in http2 server handler - httpServer.Handler = h2c.NewHandler(srv.handler(a.config.EnableDebug), &http2.Server{}) + httpServer.Handler = h2c.NewHandler(srv.handler(), &http2.Server{}) } // Load the connlimit helper into the server @@ -4290,6 +4295,9 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { a.proxyConfig.SetUpdateRateLimit(newCfg.XDSUpdateRateLimit) + a.enableDebug.Store(newCfg.EnableDebug) + a.config.EnableDebug = newCfg.EnableDebug + return nil } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 367a998a2..9f4210ac8 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -1625,7 +1625,7 @@ func TestHTTPHandlers_AgentMetricsStream_ACLDeny(t *testing.T) { resp := httptest.NewRecorder() req, err := http.NewRequestWithContext(ctx, http.MethodGet, "/v1/agent/metrics/stream", nil) require.NoError(t, err) - handle := h.handler(false) + handle := h.handler() handle.ServeHTTP(resp, req) require.Equal(t, http.StatusForbidden, resp.Code) require.Contains(t, resp.Body.String(), "Permission denied") @@ -1662,7 +1662,7 @@ func TestHTTPHandlers_AgentMetricsStream(t *testing.T) { resp := httptest.NewRecorder() req, err := http.NewRequestWithContext(ctx, http.MethodGet, "/v1/agent/metrics/stream", nil) require.NoError(t, err) - handle := h.handler(false) + handle := h.handler() handle.ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) @@ -6010,8 +6010,10 @@ func TestAgent_Monitor(t *testing.T) { cancelCtx, cancelFunc := context.WithCancel(context.Background()) req = req.WithContext(cancelCtx) + a.enableDebug.Store(true) + resp := httptest.NewRecorder() - handler := a.srv.handler(true) + handler := a.srv.handler() go handler.ServeHTTP(resp, req) args := &structs.ServiceDefinition{ diff --git a/agent/agent_test.go b/agent/agent_test.go index b234573f3..a2e27feaf 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -4193,6 +4193,39 @@ func TestAgent_ReloadConfig_XDSUpdateRateLimit(t *testing.T) { require.Equal(t, rate.Limit(1000), a.proxyConfig.UpdateRateLimit()) } +func TestAgent_ReloadConfig_EnableDebug(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + cfg := fmt.Sprintf(`data_dir = %q`, testutil.TempDir(t, "agent")) + + a := NewTestAgent(t, cfg) + defer a.Shutdown() + + c := TestConfig( + testutil.Logger(t), + config.FileSource{ + Name: t.Name(), + Format: "hcl", + Data: cfg + ` enable_debug = true`, + }, + ) + require.NoError(t, a.reloadConfigInternal(c)) + require.Equal(t, true, a.enableDebug.Load()) + + c = TestConfig( + testutil.Logger(t), + config.FileSource{ + Name: t.Name(), + Format: "hcl", + Data: cfg + ` enable_debug = false`, + }, + ) + require.NoError(t, a.reloadConfigInternal(c)) + require.Equal(t, false, a.enableDebug.Load()) +} + func TestAgent_consulConfig_AutoEncryptAllowTLS(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index f868ea964..c4d598c10 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -324,8 +324,8 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.DevMode = true rt.DisableAnonymousSignature = true rt.DisableKeyringFile = true - rt.EnableDebug = true rt.Experiments = []string{"resource-apis"} + rt.EnableDebug = true rt.UIConfig.Enabled = true rt.LeaveOnTerm = false rt.Logging.LogLevel = "DEBUG" diff --git a/agent/http.go b/agent/http.go index 1706794ad..32010c343 100644 --- a/agent/http.go +++ b/agent/http.go @@ -167,7 +167,7 @@ func (s *HTTPHandlers) ReloadConfig(newCfg *config.RuntimeConfig) error { // // The first call must not be concurrent with any other call. Subsequent calls // may be concurrent with HTTP requests since no state is modified. -func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { +func (s *HTTPHandlers) handler() http.Handler { // Memoize multiple calls. if s.h != nil { return s.h @@ -210,7 +210,15 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { // handlePProf takes the given pattern and pprof handler // and wraps it to add authorization and metrics handlePProf := func(pattern string, handler http.HandlerFunc) { + wrapper := func(resp http.ResponseWriter, req *http.Request) { + + // If enableDebug register wrapped pprof handlers + if !s.agent.enableDebug.Load() && s.checkACLDisabled() { + resp.WriteHeader(http.StatusNotFound) + return + } + var token string s.parseToken(req, &token) @@ -245,14 +253,11 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { handleFuncMetrics(pattern, s.wrap(bound, methods)) } - // If enableDebug or ACL enabled, register wrapped pprof handlers - if enableDebug || !s.checkACLDisabled() { - handlePProf("/debug/pprof/", pprof.Index) - handlePProf("/debug/pprof/cmdline", pprof.Cmdline) - handlePProf("/debug/pprof/profile", pprof.Profile) - handlePProf("/debug/pprof/symbol", pprof.Symbol) - handlePProf("/debug/pprof/trace", pprof.Trace) - } + handlePProf("/debug/pprof/", pprof.Index) + handlePProf("/debug/pprof/cmdline", pprof.Cmdline) + handlePProf("/debug/pprof/profile", pprof.Profile) + handlePProf("/debug/pprof/symbol", pprof.Symbol) + handlePProf("/debug/pprof/trace", pprof.Trace) if s.IsUIEnabled() { // Note that we _don't_ support reloading ui_config.{enabled, content_dir, diff --git a/agent/http_oss_test.go b/agent/http_oss_test.go index 60a956797..5ba36320f 100644 --- a/agent/http_oss_test.go +++ b/agent/http_oss_test.go @@ -144,7 +144,8 @@ func TestHTTPAPI_OptionMethod_OSS(t *testing.T) { uri := fmt.Sprintf("http://%s%s", a.HTTPAddr(), path) req, _ := http.NewRequest("OPTIONS", uri, nil) resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) allMethods := append([]string{"OPTIONS"}, methods...) if resp.Code != http.StatusOK { @@ -190,7 +191,9 @@ func TestHTTPAPI_AllowedNets_OSS(t *testing.T) { req, _ := http.NewRequest(method, uri, nil) req.RemoteAddr = "192.168.1.2:5555" resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusForbidden, resp.Code, "%s %s", method, path) }) diff --git a/agent/http_test.go b/agent/http_test.go index 967b1b0b4..99100c5fb 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -288,7 +288,9 @@ func TestSetupHTTPServer_HTTP2(t *testing.T) { err = setupHTTPS(httpServer, noopConnState, time.Second) require.NoError(t, err) - srvHandler := a.srv.handler(true) + a.enableDebug.Store(true) + + srvHandler := a.srv.handler() mux, ok := srvHandler.(*wrappedMux) require.True(t, ok, "expected a *wrappedMux, got %T", handler) mux.mux.HandleFunc("/echo", handler) @@ -483,7 +485,9 @@ func TestHTTPAPI_Ban_Nonprintable_Characters(t *testing.T) { t.Fatal(err) } resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) if got, want := resp.Code, http.StatusBadRequest; got != want { t.Fatalf("bad response code got %d want %d", got, want) } @@ -506,7 +510,9 @@ func TestHTTPAPI_Allow_Nonprintable_Characters_With_Flag(t *testing.T) { t.Fatal(err) } resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) // Key doesn't actually exist so we should get 404 if got, want := resp.Code, http.StatusNotFound; got != want { t.Fatalf("bad response code got %d want %d", got, want) @@ -645,7 +651,9 @@ func requireHasHeadersSet(t *testing.T, a *TestAgent, path string) { resp := httptest.NewRecorder() req, _ := http.NewRequest("GET", path, nil) - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) hdrs := resp.Header() require.Equal(t, "*", hdrs.Get("Access-Control-Allow-Origin"), @@ -706,14 +714,18 @@ func TestAcceptEncodingGzip(t *testing.T) { // negotiation, but since this call doesn't go through a real // transport, the header has to be set manually req.Header["Accept-Encoding"] = []string{"gzip"} - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, 200, resp.Code) require.Equal(t, "", resp.Header().Get("Content-Encoding")) resp = httptest.NewRecorder() req, _ = http.NewRequest("GET", "/v1/kv/long", nil) req.Header["Accept-Encoding"] = []string{"gzip"} - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, 200, resp.Code) require.Equal(t, "gzip", resp.Header().Get("Content-Encoding")) } @@ -1068,8 +1080,9 @@ func TestHTTPServer_PProfHandlers_EnableDebug(t *testing.T) { resp := httptest.NewRecorder() req, _ := http.NewRequest("GET", "/debug/pprof/profile?seconds=1", nil) + a.enableDebug.Store(true) httpServer := &HTTPHandlers{agent: a.Agent} - httpServer.handler(true).ServeHTTP(resp, req) + httpServer.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) } @@ -1087,7 +1100,7 @@ func TestHTTPServer_PProfHandlers_DisableDebugNoACLs(t *testing.T) { req, _ := http.NewRequest("GET", "/debug/pprof/profile", nil) httpServer := &HTTPHandlers{agent: a.Agent} - httpServer.handler(false).ServeHTTP(resp, req) + httpServer.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusNotFound, resp.Code) } @@ -1168,7 +1181,9 @@ func TestHTTPServer_PProfHandlers_ACLs(t *testing.T) { t.Run(fmt.Sprintf("case %d (%#v)", i, c), func(t *testing.T) { req, _ := http.NewRequest("GET", fmt.Sprintf("%s?token=%s", c.endpoint, c.token), nil) resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) assert.Equal(t, c.code, resp.Code) }) } @@ -1478,7 +1493,9 @@ func TestEnableWebUI(t *testing.T) { req, _ := http.NewRequest("GET", "/ui/", nil) resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) // Validate that it actually sent the index page we expect since an error @@ -1507,7 +1524,9 @@ func TestEnableWebUI(t *testing.T) { { req, _ := http.NewRequest("GET", "/ui/", nil) resp := httptest.NewRecorder() - a.srv.handler(true).ServeHTTP(resp, req) + a.enableDebug.Store(true) + + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) require.Contains(t, resp.Body.String(), `