feature - [NET - 4005] - [Supportability] Reloadable Configuration - enable_debug (#17565)

* # This is a combination of 9 commits.
# This is the 1st commit message:

init without tests

# This is the commit message #2:

change log

# This is the commit message #3:

fix tests

# This is the commit message #4:

fix tests

# This is the commit message #5:

added tests

# This is the commit message #6:

change log breaking change

# This is the commit message #7:

removed breaking change

# This is the commit message #8:

fix test

# This is the commit message #9:

keeping the test behaviour same

* # This is a combination of 12 commits.
# This is the 1st commit message:

init without tests

# This is the commit message #2:

change log

# This is the commit message #3:

fix tests

# This is the commit message #4:

fix tests

# This is the commit message #5:

added tests

# This is the commit message #6:

change log breaking change

# This is the commit message #7:

removed breaking change

# This is the commit message #8:

fix test

# This is the commit message #9:

keeping the test behaviour same

# This is the commit message #10:

made enable debug atomic bool

# This is the commit message #11:

fix lint

# This is the commit message #12:

fix test true enable debug

* parent 10f500e895
author absolutelightning <ashesh.vidyut@hashicorp.com> 1687352587 +0530
committer absolutelightning <ashesh.vidyut@hashicorp.com> 1687352592 +0530

init without tests

change log

fix tests

fix tests

added tests

change log breaking change

removed breaking change

fix test

keeping the test behaviour same

made enable debug atomic bool

fix lint

fix test true enable debug

using enable debug in agent as atomic bool

test fixes

fix tests

fix tests

added update on correct locaiton

fix tests

fix reloadable config enable debug

fix tests

fix init and acl 403

* revert commit

.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
+```

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
 }
 

agent/agent_endpoint_test.go

@@ -1623,7 +1623,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")
@@ -1660,7 +1660,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)
 
@@ -6008,8 +6008,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{

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

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"

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
+	handlePProf("/debug/pprof/", pprof.Index)
-	if enableDebug || !s.checkACLDisabled() {
+	handlePProf("/debug/pprof/cmdline", pprof.Cmdline)
-		handlePProf("/debug/pprof/", pprof.Index)
+	handlePProf("/debug/pprof/profile", pprof.Profile)
-		handlePProf("/debug/pprof/cmdline", pprof.Cmdline)
+	handlePProf("/debug/pprof/symbol", pprof.Symbol)
-		handlePProf("/debug/pprof/profile", pprof.Profile)
+	handlePProf("/debug/pprof/trace", pprof.Trace)
-		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,

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

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`)

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

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()