From 402a65d288ffc8ae00573735bb5526c8a47bcac6 Mon Sep 17 00:00:00 2001 From: Ashesh Vidyut Date: Sat, 17 Jun 2023 09:38:44 +0530 Subject: [PATCH] using enable debug in agent as atomic bool --- agent/agent.go | 10 +++++++--- agent/agent_endpoint_test.go | 4 +--- agent/config/builder.go | 18 +----------------- agent/config/runtime.go | 3 +-- agent/config/runtime_test.go | 11 +++-------- agent/http.go | 2 +- agent/http_oss_test.go | 8 +++----- agent/http_test.go | 31 ++++++++++--------------------- agent/ui_endpoint_oss_test.go | 3 +-- agent/ui_endpoint_test.go | 3 +-- 10 files changed, 29 insertions(+), 64 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 102b47b056e4..6259daa67066 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -416,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. @@ -598,6 +600,9 @@ func (a *Agent) Start(ctx context.Context) error { // Overwrite the configuration. a.config = c + a.enableDebug = atomic.Bool{} + 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) } @@ -4157,6 +4162,8 @@ func (a *Agent) reloadConfig(autoReload bool) error { } } + a.enableDebug.Store(newCfg.EnableDebug) + return a.reloadConfigInternal(newCfg) } @@ -4291,9 +4298,6 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { a.proxyConfig.SetUpdateRateLimit(newCfg.XDSUpdateRateLimit) - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(newCfg.EnableDebug.Load()) - return nil } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 9d8f72c2b8bb..23829cd2c16c 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -17,7 +17,6 @@ import ( "os" "strconv" "strings" - "sync/atomic" "testing" "time" @@ -6009,8 +6008,7 @@ func TestAgent_Monitor(t *testing.T) { cancelCtx, cancelFunc := context.WithCancel(context.Background()) req = req.WithContext(cancelCtx) - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true resp := httptest.NewRecorder() handler := a.srv.handler() go handler.ServeHTTP(resp, req) diff --git a/agent/config/builder.go b/agent/config/builder.go index 0979c1bf2cc8..0f87ff51a578 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -18,7 +18,6 @@ import ( "sort" "strconv" "strings" - "sync/atomic" "time" "github.com/armon/go-metrics/prometheus" @@ -1011,7 +1010,7 @@ func (b *builder) build() (rt RuntimeConfig, err error) { DiscoveryMaxStale: b.durationVal("discovery_max_stale", c.DiscoveryMaxStale), EnableAgentTLSForChecks: boolVal(c.EnableAgentTLSForChecks), EnableCentralServiceConfig: boolVal(c.EnableCentralServiceConfig), - EnableDebug: *atomicBoolVal(c.EnableDebug), + EnableDebug: c.EnableDebug, EnableRemoteScriptChecks: enableRemoteScriptChecks, EnableLocalScriptChecks: enableLocalScriptChecks, EncryptKey: stringVal(c.EncryptKey), @@ -1939,21 +1938,6 @@ func boolValWithDefault(v *bool, defaultVal bool) bool { return *v } -func atomicBool(v bool) *atomic.Bool { - atomicBool := atomic.Bool{} - atomicBool.Store(v) - return &atomicBool -} - -func atomicBoolVal(v *bool) *atomic.Bool { - if v == nil { - return &atomic.Bool{} - } - atomicBool := atomic.Bool{} - atomicBool.Store(*v) - return &atomicBool -} - func boolVal(v *bool) bool { if v == nil { return false diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 91617d67b5b9..1a8dc13794d3 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -8,7 +8,6 @@ import ( "net" "reflect" "strings" - "sync/atomic" "time" "github.com/hashicorp/go-uuid" @@ -652,7 +651,7 @@ type RuntimeConfig struct { // EnableDebug is used to enable various debugging features. // // hcl: enable_debug = (true|false) - EnableDebug atomic.Bool + EnableDebug bool // EnableLocalScriptChecks controls whether health checks declared from the local // config file which execute scripts are enabled. This includes regular script diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 935fe6e4ab61..c1cd85ac502f 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -17,7 +17,6 @@ import ( "reflect" "strconv" "strings" - "sync/atomic" "testing" "time" @@ -325,8 +324,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.DevMode = true rt.DisableAnonymousSignature = true rt.DisableKeyringFile = true - rt.EnableDebug = atomic.Bool{} - rt.EnableDebug.Store(true) + rt.EnableDebug = true rt.Experiments = []string{"resource-apis"} rt.UIConfig.Enabled = true rt.LeaveOnTerm = false @@ -5966,8 +5964,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { // The logstore settings from first file should not be overridden by a // later file with nothing to say about logstores! rt.RaftLogStoreConfig.Backend = consul.LogStoreBackendWAL - rt.EnableDebug = atomic.Bool{} - rt.EnableDebug.Store(true) + rt.EnableDebug = true }, }) } @@ -6070,8 +6067,6 @@ func TestLoad_FullConfig(t *testing.T) { _, n, _ := net.ParseCIDR(s) return n } - atomicBoolTrue := atomic.Bool{} - atomicBoolTrue.Store(true) defaultEntMeta := structs.DefaultEnterpriseMetaInDefaultPartition() nodeEntMeta := structs.NodeEnterpriseMetaInDefaultPartition() @@ -6357,7 +6352,7 @@ func TestLoad_FullConfig(t *testing.T) { DiscoveryMaxStale: 5 * time.Second, EnableAgentTLSForChecks: true, EnableCentralServiceConfig: false, - EnableDebug: *atomicBool(true), + EnableDebug: true, EnableRemoteScriptChecks: true, EnableLocalScriptChecks: true, EncryptKey: "A4wELWqH", diff --git a/agent/http.go b/agent/http.go index 7a4554273111..9363d3fcc6d9 100644 --- a/agent/http.go +++ b/agent/http.go @@ -214,7 +214,7 @@ func (s *HTTPHandlers) handler() http.Handler { wrapper := func(resp http.ResponseWriter, req *http.Request) { // If enableDebug or ACL enabled, register wrapped pprof handlers - if !s.agent.config.EnableDebug.Load() && s.checkACLDisabled() { + if !s.agent.enableDebug.Load() && s.checkACLDisabled() { resp.WriteHeader(http.StatusNotFound) return } diff --git a/agent/http_oss_test.go b/agent/http_oss_test.go index 1ad58239135e..1c468c7b7e08 100644 --- a/agent/http_oss_test.go +++ b/agent/http_oss_test.go @@ -9,7 +9,6 @@ import ( "net/http" "net/http/httptest" "strings" - "sync/atomic" "testing" "time" @@ -145,8 +144,7 @@ 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.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) allMethods := append([]string{"OPTIONS"}, methods...) @@ -193,8 +191,8 @@ func TestHTTPAPI_AllowedNets_OSS(t *testing.T) { req, _ := http.NewRequest(method, uri, nil) req.RemoteAddr = "192.168.1.2:5555" resp := httptest.NewRecorder() - a.config.EnableDebug = atomic.Bool{} - + a.config.EnableDebug = 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 212d8ef7e18e..0fd866f6edf1 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -20,7 +20,6 @@ import ( "runtime" "strconv" "strings" - "sync/atomic" "testing" "time" @@ -289,8 +288,7 @@ func TestSetupHTTPServer_HTTP2(t *testing.T) { err = setupHTTPS(httpServer, noopConnState, time.Second) require.NoError(t, err) - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true srvHandler := a.srv.handler() mux, ok := srvHandler.(*wrappedMux) require.True(t, ok, "expected a *wrappedMux, got %T", handler) @@ -486,8 +484,7 @@ func TestHTTPAPI_Ban_Nonprintable_Characters(t *testing.T) { t.Fatal(err) } resp := httptest.NewRecorder() - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = 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) @@ -511,8 +508,7 @@ func TestHTTPAPI_Allow_Nonprintable_Characters_With_Flag(t *testing.T) { t.Fatal(err) } resp := httptest.NewRecorder() - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = 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 { @@ -652,8 +648,7 @@ func requireHasHeadersSet(t *testing.T, a *TestAgent, path string) { resp := httptest.NewRecorder() req, _ := http.NewRequest("GET", path, nil) - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) hdrs := resp.Header() @@ -715,8 +710,7 @@ 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.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) require.Equal(t, 200, resp.Code) require.Equal(t, "", resp.Header().Get("Content-Encoding")) @@ -724,8 +718,7 @@ func TestAcceptEncodingGzip(t *testing.T) { resp = httptest.NewRecorder() req, _ = http.NewRequest("GET", "/v1/kv/long", nil) req.Header["Accept-Encoding"] = []string{"gzip"} - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) require.Equal(t, 200, resp.Code) require.Equal(t, "gzip", resp.Header().Get("Content-Encoding")) @@ -1081,8 +1074,7 @@ func TestHTTPServer_PProfHandlers_EnableDebug(t *testing.T) { resp := httptest.NewRecorder() req, _ := http.NewRequest("GET", "/debug/pprof/profile?seconds=1", nil) - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true httpServer := &HTTPHandlers{agent: a.Agent} httpServer.handler().ServeHTTP(resp, req) @@ -1183,8 +1175,7 @@ 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.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) assert.Equal(t, c.code, resp.Code) }) @@ -1495,8 +1486,7 @@ func TestEnableWebUI(t *testing.T) { req, _ := http.NewRequest("GET", "/ui/", nil) resp := httptest.NewRecorder() - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) @@ -1526,8 +1516,7 @@ func TestEnableWebUI(t *testing.T) { { req, _ := http.NewRequest("GET", "/ui/", nil) resp := httptest.NewRecorder() - a.config.EnableDebug = atomic.Bool{} - a.config.EnableDebug.Store(true) + a.config.EnableDebug = true a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) require.Contains(t, resp.Body.String(), `