luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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>
This commit is contained in:
Dhia Ayachi 2021-05-27 09:41:53 -04:00 committed by GitHub
parent c2dc8cf56b
commit 00f7e0772a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 47 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
.changelog/10273.txt Normal file
View File

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

14
agent/http.go
View File

@ -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 {

16
api/debug.go
View File

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

31
command/debug/debug.go
View File

@ -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:

30
command/debug/debug_test.go
View File

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