From 4c7f5f31c718ea1ee00ef93545177c951376b51f Mon Sep 17 00:00:00 2001 From: Dhia Ayachi Date: Thu, 27 May 2021 09:41:53 -0400 Subject: [PATCH] debug: remove the CLI check for debug_enabled (#10273) * debug: remove the CLI check for debug_enabled The API allows collecting profiles even debug_enabled=false as long as ACLs are enabled. Remove this check from the CLI so that users do not need to set debug_enabled=true for no reason. Also: - fix the API client to return errors on non-200 status codes for debug endpoints - improve the failure messages when pprof data can not be collected Co-Authored-By: Dhia Ayachi * remove parallel test runs parallel runs create a race condition that fail the debug tests * Add changelog Co-authored-by: Daniel Nephin --- .changelog/10273.txt | 5 +++++ agent/http.go | 14 ++++++-------- api/debug.go | 16 ++++++++++++++++ command/debug/debug.go | 31 ++++++++----------------------- command/debug/debug_test.go | 30 ++++++++++++------------------ 5 files changed, 47 insertions(+), 49 deletions(-) create mode 100644 .changelog/10273.txt diff --git a/.changelog/10273.txt b/.changelog/10273.txt new file mode 100644 index 0000000000..74800c4f7a --- /dev/null +++ b/.changelog/10273.txt @@ -0,0 +1,5 @@ +```release-note:bug +cli: removes the need to set debug_enabled=true to collect debug data from the CLI. Now +the CLI behaves the same way as the API and accepts either an ACL token with operator:read, or +debug_enabled=true. +``` diff --git a/agent/http.go b/agent/http.go index d5215a2524..f6a8b448e4 100644 --- a/agent/http.go +++ b/agent/http.go @@ -238,20 +238,18 @@ func (s *HTTPHandlers) handler(enableDebug bool) http.Handler { var token string s.parseToken(req, &token) + // If enableDebug is not set, and ACLs are disabled, write + // an unauthorized response + if !enableDebug && s.checkACLDisabled(resp, req) { + return + } + rule, err := s.agent.delegate.ResolveTokenAndDefaultMeta(token, nil, nil) if err != nil { resp.WriteHeader(http.StatusForbidden) return } - // If enableDebug is not set, and ACLs are disabled, write - // an unauthorized response - if !enableDebug { - if s.checkACLDisabled(resp, req) { - return - } - } - // If the token provided does not have the necessary permissions, // write a forbidden response if rule != nil && rule.OperatorRead(nil) != acl.Allow { diff --git a/api/debug.go b/api/debug.go index 238046853a..7ab1a93b5c 100644 --- a/api/debug.go +++ b/api/debug.go @@ -29,6 +29,10 @@ func (d *Debug) Heap() ([]byte, error) { } defer resp.Body.Close() + if resp.StatusCode != 200 { + return nil, generateUnexpectedResponseCodeError(resp) + } + // We return a raw response because we're just passing through a response // from the pprof handlers body, err := ioutil.ReadAll(resp.Body) @@ -52,6 +56,10 @@ func (d *Debug) Profile(seconds int) ([]byte, error) { } defer resp.Body.Close() + if resp.StatusCode != 200 { + return nil, generateUnexpectedResponseCodeError(resp) + } + // We return a raw response because we're just passing through a response // from the pprof handlers body, err := ioutil.ReadAll(resp.Body) @@ -75,6 +83,10 @@ func (d *Debug) Trace(seconds int) ([]byte, error) { } defer resp.Body.Close() + if resp.StatusCode != 200 { + return nil, generateUnexpectedResponseCodeError(resp) + } + // We return a raw response because we're just passing through a response // from the pprof handlers body, err := ioutil.ReadAll(resp.Body) @@ -95,6 +107,10 @@ func (d *Debug) Goroutine() ([]byte, error) { } defer resp.Body.Close() + if resp.StatusCode != 200 { + return nil, generateUnexpectedResponseCodeError(resp) + } + // We return a raw response because we're just passing through a response // from the pprof handlers body, err := ioutil.ReadAll(resp.Body) diff --git a/command/debug/debug.go b/command/debug/debug.go index c6642faf68..ddf98422f6 100644 --- a/command/debug/debug.go +++ b/command/debug/debug.go @@ -15,10 +15,11 @@ import ( "sync" "time" - "github.com/hashicorp/consul/api" - "github.com/hashicorp/consul/command/flags" multierror "github.com/hashicorp/go-multierror" "github.com/mitchellh/cli" + + "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/command/flags" ) const ( @@ -254,28 +255,12 @@ func (c *cmd) prepare() (version string, err error) { return "", fmt.Errorf("agent response did not contain version key") } - debugEnabled, ok := self["DebugConfig"]["EnableDebug"].(bool) - if !ok { - return version, fmt.Errorf("agent response did not contain debug key") - } - // If none are specified we will collect information from // all by default if len(c.capture) == 0 { c.capture = c.defaultTargets() } - if !debugEnabled && c.configuredTarget("pprof") { - cs := c.capture - for i := 0; i < len(cs); i++ { - if cs[i] == "pprof" { - c.capture = append(cs[:i], cs[i+1:]...) - i-- - } - } - c.UI.Warn("[WARN] Unable to capture pprof. Set enable_debug to true on target agent to enable profiling.") - } - for _, t := range c.capture { if !c.allowedTarget(t) { return version, fmt.Errorf("target not found: %s", t) @@ -414,7 +399,7 @@ func (c *cmd) captureDynamic() error { heap, err := c.client.Debug().Heap() if err != nil { - errCh <- err + errCh <- fmt.Errorf("failed to collect heap profile: %w", err) } err = ioutil.WriteFile(fmt.Sprintf("%s/heap.prof", timestampDir), heap, 0644) @@ -432,7 +417,7 @@ func (c *cmd) captureDynamic() error { go func() { prof, err := c.client.Debug().Profile(int(s)) if err != nil { - errCh <- err + errCh <- fmt.Errorf("failed to collect cpu profile: %w", err) } err = ioutil.WriteFile(fmt.Sprintf("%s/profile.prof", timestampDir), prof, 0644) @@ -447,7 +432,7 @@ func (c *cmd) captureDynamic() error { go func() { trace, err := c.client.Debug().Trace(int(s)) if err != nil { - errCh <- err + errCh <- fmt.Errorf("failed to collect trace: %w", err) } err = ioutil.WriteFile(fmt.Sprintf("%s/trace.out", timestampDir), trace, 0644) @@ -460,7 +445,7 @@ func (c *cmd) captureDynamic() error { gr, err := c.client.Debug().Goroutine() if err != nil { - errCh <- err + errCh <- fmt.Errorf("failed to collect goroutine profile: %w", err) } err = ioutil.WriteFile(fmt.Sprintf("%s/goroutine.prof", timestampDir), gr, 0644) @@ -532,7 +517,7 @@ func (c *cmd) captureDynamic() error { c.UI.Output(fmt.Sprintf("Capture successful %s (%d)", time.Unix(t, 0).Local().String(), intervalCount)) go capture() case e := <-errCh: - c.UI.Error(fmt.Sprintf("Capture failure %s", e)) + c.UI.Error(fmt.Sprintf("Capture failure: %s", e)) case <-durationChn: return nil case <-c.shutdownCh: diff --git a/command/debug/debug_test.go b/command/debug/debug_test.go index 64509d7206..6485832bb9 100644 --- a/command/debug/debug_test.go +++ b/command/debug/debug_test.go @@ -5,15 +5,18 @@ import ( "compress/gzip" "fmt" "io" + "io/ioutil" "os" "path/filepath" "strings" "testing" + "github.com/mitchellh/cli" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/testrpc" - "github.com/mitchellh/cli" ) func TestDebugCommand_noTabs(t *testing.T) { @@ -29,8 +32,6 @@ func TestDebugCommand(t *testing.T) { t.Skip("too slow for testing.Short") } - t.Parallel() - testDir := testutil.TempDir(t, "debug") a := agent.NewTestAgent(t, ` @@ -69,8 +70,6 @@ func TestDebugCommand_Archive(t *testing.T) { t.Skip("too slow for testing.Short") } - t.Parallel() - testDir := testutil.TempDir(t, "debug") a := agent.NewTestAgent(t, ` @@ -187,8 +186,6 @@ func TestDebugCommand_OutputPathExists(t *testing.T) { t.Skip("too slow for testing.Short") } - t.Parallel() - testDir := testutil.TempDir(t, "debug") a := agent.NewTestAgent(t, "") @@ -228,8 +225,6 @@ func TestDebugCommand_CaptureTargets(t *testing.T) { t.Skip("too slow for testing.Short") } - t.Parallel() - cases := map[string]struct { // used in -target param targets []string @@ -340,8 +335,6 @@ func TestDebugCommand_ProfilesExist(t *testing.T) { t.Skip("too slow for testing.Short") } - t.Parallel() - testDir := testutil.TempDir(t, "debug") a := agent.NewTestAgent(t, ` @@ -390,8 +383,6 @@ func TestDebugCommand_ValidateTiming(t *testing.T) { t.Skip("too slow for testing.Short") } - t.Parallel() - cases := map[string]struct { duration string interval string @@ -486,13 +477,16 @@ func TestDebugCommand_DebugDisabled(t *testing.T) { // Glob ignores file system errors for _, v := range profiles { fs, _ := filepath.Glob(fmt.Sprintf("%s/*/%s", outputPath, v)) - if len(fs) > 0 { - t.Errorf("output data should not exist for %s", v) - } + // TODO: make this always one + require.True(t, len(fs) >= 1) + content, err := ioutil.ReadFile(fs[0]) + require.NoError(t, err) + require.Len(t, content, 0) } errOutput := ui.ErrorWriter.String() - if !strings.Contains(errOutput, "Unable to capture pprof") { - t.Errorf("expected warn output, got %s", errOutput) + for _, prof := range []string{"heap", "cpu", "goroutine", "trace"} { + expected := fmt.Sprintf("failed to collect %v", prof) + require.Contains(t, errOutput, expected) } }