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 <dhia@hashicorp.com>

* remove parallel test runs

parallel runs create a race condition that fail the debug tests

* Add changelog

Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>

.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.
+```

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 {

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)

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:

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