Backport of feature - [NET - 4005] - [Supportability] Reloadable Conf…

…iguration - enable_debug into release/1.16.x (#17969) * backport of commit 10f500e * backport of commit e08c309 * backport of commit 58638de * merge conf resolve --------- Co-authored-by: Ashesh Vidyut <[email protected]> Co-authored-by: Ashesh Vidyut <[email protected]>
hashicorp · Jun 30, 2023 · f434ab5 · f434ab5
1 parent c11d89d
commit f434ab5
Show file tree

Hide file tree

Showing 10 changed files with 107 additions and 30 deletions.
diff --git a/.changelog/17565.txt 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
+```
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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(), `<!-- CONSUL_VERSION:`)
 		require.Contains(t, resp.Body.String(), `valid-but-unlikely-metrics-provider-name`)

diff --git a/agent/ui_endpoint_oss_test.go b/agent/ui_endpoint_oss_test.go
@@ -58,7 +58,9 @@ func TestUIEndpoint_MetricsProxy_ACLDeny(t *testing.T) {
 	`, backendURL))
 	defer a.Shutdown()
 
-	h := a.srv.handler(true)
+	a.enableDebug.Store(true)
+
+	h := a.srv.handler()
 
 	testrpc.WaitForLeader(t, a.RPC, "dc1")
 

diff --git a/agent/ui_endpoint_test.go b/agent/ui_endpoint_test.go
@@ -2620,7 +2620,9 @@ func TestUIEndpoint_MetricsProxy(t *testing.T) {
 			require.NoError(t, a.Agent.reloadConfigInternal(&cfg))
 
 			// Now fetch the API handler to run requests against
-			h := a.srv.handler(true)
+			a.enableDebug.Store(true)
+
+			h := a.srv.handler()
 
 			req := httptest.NewRequest("GET", tc.path, nil)
 			rec := httptest.NewRecorder()