From 58638deeb3ac16b6556bba6a2b24034d7f886cce Mon Sep 17 00:00:00 2001 From: absolutelightning Date: Wed, 21 Jun 2023 18:35:47 +0530 Subject: [PATCH] parent 10f500e895d92cc3691ade7b74a33db755d22039 author absolutelightning 1687352587 +0530 committer absolutelightning 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 --- agent/agent.go | 8 +++++-- agent/agent_endpoint_test.go | 5 ++--- agent/agent_test.go | 4 ++-- agent/config/builder.go | 18 +--------------- agent/config/runtime.go | 3 +-- agent/config/runtime_test.go | 10 +++------ agent/http.go | 9 ++++++-- agent/http_oss_test.go | 8 +++---- agent/http_test.go | 40 +++++++++++++++++------------------ agent/ui_endpoint_oss_test.go | 4 ++-- agent/ui_endpoint_test.go | 4 ++-- 11 files changed, 48 insertions(+), 65 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 1c5ddb7c2cc4..fa75a1cd1cf4 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,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) } @@ -4291,8 +4295,8 @@ 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()) + 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 9d8f72c2b8bb..c465b687a880 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,8 @@ 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.enableDebug.Store(true) + resp := httptest.NewRecorder() handler := a.srv.handler() go handler.ServeHTTP(resp, req) diff --git a/agent/agent_test.go b/agent/agent_test.go index 108f970f9fec..a2e27feaf4fd 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -4212,7 +4212,7 @@ func TestAgent_ReloadConfig_EnableDebug(t *testing.T) { }, ) require.NoError(t, a.reloadConfigInternal(c)) - require.Equal(t, true, a.config.EnableDebug.Load()) + require.Equal(t, true, a.enableDebug.Load()) c = TestConfig( testutil.Logger(t), @@ -4223,7 +4223,7 @@ func TestAgent_ReloadConfig_EnableDebug(t *testing.T) { }, ) require.NoError(t, a.reloadConfigInternal(c)) - require.Equal(t, false, a.config.EnableDebug.Load()) + require.Equal(t, false, a.enableDebug.Load()) } func TestAgent_consulConfig_AutoEncryptAllowTLS(t *testing.T) { diff --git a/agent/config/builder.go b/agent/config/builder.go index 9d65231eadb8..8e0bb37ef999 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: boolVal(c.EnableDebug), EnableRemoteScriptChecks: enableRemoteScriptChecks, EnableLocalScriptChecks: enableLocalScriptChecks, EncryptKey: stringVal(c.EncryptKey), @@ -1943,21 +1942,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 d872d8808ca4..cc5451804dd7 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" @@ -326,7 +325,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.DisableAnonymousSignature = true rt.DisableKeyringFile = true rt.Experiments = []string{"resource-apis"} - rt.EnableDebug.Store(true) + rt.EnableDebug = true rt.UIConfig.Enabled = true rt.LeaveOnTerm = false rt.Logging.LogLevel = "DEBUG" @@ -5977,8 +5976,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 }, }) } @@ -6081,8 +6079,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() @@ -6370,7 +6366,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..1d1ca8e48d58 100644 --- a/agent/http.go +++ b/agent/http.go @@ -213,8 +213,13 @@ 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.checkACLDisabled() { + resp.WriteHeader(http.StatusForbidden) + return + } + + // If enableDebug register wrapped pprof handlers + if !s.agent.enableDebug.Load() { resp.WriteHeader(http.StatusNotFound) return } diff --git a/agent/http_oss_test.go b/agent/http_oss_test.go index 1ad58239135e..5ba36320f628 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.enableDebug.Store(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.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 212d8ef7e18e..99100c5fbc8e 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,8 @@ 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.enableDebug.Store(true) + srvHandler := a.srv.handler() mux, ok := srvHandler.(*wrappedMux) require.True(t, ok, "expected a *wrappedMux, got %T", handler) @@ -486,8 +485,8 @@ 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.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) @@ -511,8 +510,8 @@ 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.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 { @@ -652,8 +651,8 @@ 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.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) hdrs := resp.Header() @@ -715,8 +714,8 @@ 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.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, 200, resp.Code) require.Equal(t, "", resp.Header().Get("Content-Encoding")) @@ -724,8 +723,8 @@ 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.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, 200, resp.Code) require.Equal(t, "gzip", resp.Header().Get("Content-Encoding")) @@ -1081,8 +1080,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.enableDebug.Store(true) httpServer := &HTTPHandlers{agent: a.Agent} httpServer.handler().ServeHTTP(resp, req) @@ -1183,8 +1181,8 @@ 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.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) assert.Equal(t, c.code, resp.Code) }) @@ -1495,8 +1493,8 @@ 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.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) @@ -1526,8 +1524,8 @@ 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.enableDebug.Store(true) + a.srv.handler().ServeHTTP(resp, req) require.Equal(t, http.StatusOK, resp.Code) require.Contains(t, resp.Body.String(), `