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 10f500e author absolutelightning <[email protected]> 1687352587 +0530 committer absolutelightning <[email protected]> 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
hashicorp · Jun 30, 2023 · 2af6bc4 · 2af6bc4
1 parent 2736e64
commit 2af6bc4
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
@@ -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{

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