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 1c5ddb7c2c..fa75a1cd1c 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 9d8f72c2b8..c465b687a8 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 108f970f9f..a2e27feaf4 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 9d65231ead..8e0bb37ef9 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 91617d67b5..1a8dc13794 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 d872d8808c..cc5451804d 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 7a45542731..1d1ca8e48d 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 1ad5823913..5ba36320f6 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 212d8ef7e1..99100c5fbc 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(), `