Skip to content

Commit

Permalink
Merge pull request #7053 from hashicorp/b-client-monitor-acl-panic
Browse files Browse the repository at this point in the history 
Fix panic when monitoring a local client node
  • Loading branch information
@drewbailey
drewbailey committed Feb 3, 2020
2 parents a1fee69 + 173ad83 commit d28898b
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 42 deletions.
5 changes: 3 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ IMPROVEMENTS:

BUG FIXES:

* core: Addressed an inconsistency where allocations created prior to 0.9 had missing fields [[GH-6922](https://github.com/hashicorp/nomad/issues/6922)]
* agent: Fixed race condition in logging when using `nomad monitor` command [[GH-6872](https://github.com/hashicorp/nomad/issues/6872)]
* agent: Fixed a panic when using `nomad monitor` on a client node. [[GH-7053](https://github.com/hashicorp/nomad/issues/7053)]
* agent: Fixed race condition in logging when using `nomad monitor` command. [[GH-6872](https://github.com/hashicorp/nomad/issues/6872)]
* agent: Fixed a bug where `nomad monitor -server-id` only work for a server's name instead of uuid or name. [[GH-7015](https://github.com/hashicorp/nomad/issues/7015)]
* core: Addressed an inconsistency where allocations created prior to 0.9 had missing fields [[GH-6922](https://github.com/hashicorp/nomad/issues/6922)]
* cli: Fixed a bug where `nomad monitor -node-id` would cause a cli panic when no nodes where found. [[GH-6828](https://github.com/hashicorp/nomad/issues/6828)]
* cli: Fixed a bug where error messages appeared interweived with help text inconsistently [[GH-6865](https://github.com/hashicorp/nomad/issues/6865)]
* config: Fixed a bug where agent startup would fail if the `consul.timeout` configuration was set. [[GH-6907](https://github.com/hashicorp/nomad/issues/6907)]
Expand Down
22 changes: 8 additions & 14 deletions command/agent/agent_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,6 @@ func (s *HTTPServer) AgentMembersRequest(resp http.ResponseWriter, req *http.Req
}

func (s *HTTPServer) AgentMonitor(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
var secret string
s.parseToken(req, &secret)

// Check agent read permissions
if aclObj, err := s.agent.Server().ResolveToken(secret); err != nil {
return nil, err
} else if aclObj != nil && !aclObj.AllowAgentRead() {
return nil, structs.ErrPermissionDenied
}

// Get the provided loglevel.
logLevel := req.URL.Query().Get("log_level")
if logLevel == "" {
Expand Down Expand Up @@ -228,9 +218,11 @@ func (s *HTTPServer) AgentMonitor(resp http.ResponseWriter, req *http.Request) (
} else {
handlerErr = CodedError(400, "No local Node and node_id not provided")
}
// No node id monitor current server/client
} else if srv := s.agent.Server(); srv != nil {
handler, handlerErr = srv.StreamingRpcHandler("Agent.Monitor")
} else {
// No node id monitor server
handler, handlerErr = s.agent.Server().StreamingRpcHandler("Agent.Monitor")
handler, handlerErr = s.agent.Client().StreamingRpcHandler("Agent.Monitor")
}

if handlerErr != nil {
Expand Down Expand Up @@ -401,9 +393,11 @@ func (s *HTTPServer) agentPprof(reqType pprof.ReqType, resp http.ResponseWriter,
} else if localServer {
rpcErr = s.agent.Server().RPC("Agent.Profile", &args, &reply)
}
// No node id, profile current server/client
} else if srv := s.agent.Server(); srv != nil {
rpcErr = srv.RPC("Agent.Profile", &args, &reply)
} else {
// No node id target server
rpcErr = s.agent.Server().RPC("Agent.Profile", &args, &reply)
rpcErr = s.agent.Client().RPC("Agent.Profile", &args, &reply)
}

if rpcErr != nil {
Expand Down
105 changes: 79 additions & 26 deletions command/agent/agent_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/hashicorp/nomad/nomad/mock"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -255,43 +256,42 @@ func TestHTTP_AgentMembers_ACL(t *testing.T) {
func TestHTTP_AgentMonitor(t *testing.T) {
t.Parallel()

httpTest(t, nil, func(s *TestAgent) {
// invalid log_json
{
t.Run("invalid log_json parameter", func(t *testing.T) {
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("GET", "/v1/agent/monitor?log_json=no", nil)
require.Nil(t, err)
resp := newClosableRecorder()

// Make the request
_, err = s.Server.AgentMonitor(resp, req)
if err.(HTTPCodedError).Code() != 400 {
t.Fatalf("expected 400 response, got: %v", resp.Code)
}
}
httpErr := err.(HTTPCodedError).Code()
require.Equal(t, 400, httpErr)
})
})

// unknown log_level
{
t.Run("unknown log_level", func(t *testing.T) {
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("GET", "/v1/agent/monitor?log_level=unknown", nil)
require.Nil(t, err)
resp := newClosableRecorder()

// Make the request
_, err = s.Server.AgentMonitor(resp, req)
if err.(HTTPCodedError).Code() != 400 {
t.Fatalf("expected 400 response, got: %v", resp.Code)
}
}
httpErr := err.(HTTPCodedError).Code()
require.Equal(t, 400, httpErr)
})
})

// check for a specific log
{
t.Run("check for specific log level", func(t *testing.T) {
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("GET", "/v1/agent/monitor?log_level=warn", nil)
require.Nil(t, err)
resp := newClosableRecorder()
defer resp.Close()

go func() {
_, err = s.Server.AgentMonitor(resp, req)
require.NoError(t, err)
assert.NoError(t, err)
}()

// send the same log until monitor sink is set up
Expand All @@ -313,18 +313,19 @@ func TestHTTP_AgentMonitor(t *testing.T) {
}, func(err error) {
require.Fail(t, err.Error())
})
}
})
})

// plain param set to true
{
t.Run("plain output", func(t *testing.T) {
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("GET", "/v1/agent/monitor?log_level=debug&plain=true", nil)
require.Nil(t, err)
resp := newClosableRecorder()
defer resp.Close()

go func() {
_, err = s.Server.AgentMonitor(resp, req)
require.NoError(t, err)
assert.NoError(t, err)
}()

// send the same log until monitor sink is set up
Expand All @@ -346,18 +347,19 @@ func TestHTTP_AgentMonitor(t *testing.T) {
}, func(err error) {
require.Fail(t, err.Error())
})
}
})
})

// stream logs for a given node
{
t.Run("logs for a specific node", func(t *testing.T) {
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("GET", "/v1/agent/monitor?log_level=warn&node_id="+s.client.NodeID(), nil)
require.Nil(t, err)
resp := newClosableRecorder()
defer resp.Close()

go func() {
_, err = s.Server.AgentMonitor(resp, req)
require.NoError(t, err)
assert.NoError(t, err)
}()

// send the same log until monitor sink is set up
Expand Down Expand Up @@ -385,7 +387,48 @@ func TestHTTP_AgentMonitor(t *testing.T) {
}, func(err error) {
require.Fail(t, err.Error())
})
}
})
})

t.Run("logs for a local client with no server running on agent", func(t *testing.T) {
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("GET", "/v1/agent/monitor?log_level=warn", nil)
require.Nil(t, err)
resp := newClosableRecorder()
defer resp.Close()

go func() {
// set server to nil to monitor as client
s.Agent.server = nil
_, err = s.Server.AgentMonitor(resp, req)
assert.NoError(t, err)
}()

// send the same log until monitor sink is set up
maxLogAttempts := 10
tried := 0
out := ""
testutil.WaitForResult(func() (bool, error) {
if tried < maxLogAttempts {
s.Agent.logger.Warn("log that should be sent")
tried++
}
output, err := ioutil.ReadAll(resp.Body)
if err != nil {
return false, err
}

out += string(output)
want := `{"Data":"`
if strings.Contains(out, want) {
return true, nil
}

return false, fmt.Errorf("missing expected log, got: %v, want: %v", out, want)
}, func(err error) {
require.Fail(t, err.Error())
})
})
})
}

Expand Down Expand Up @@ -481,11 +524,17 @@ func TestAgent_PprofRequest(t *testing.T) {
addNodeID bool
addServerID bool
expectedErr string
clientOnly bool
}{
{
desc: "cmdline local request",
desc: "cmdline local server request",
url: "/v1/agent/pprof/cmdline",
},
{
desc: "cmdline local node request",
url: "/v1/agent/pprof/cmdline",
clientOnly: true,
},
{
desc: "cmdline node request",
url: "/v1/agent/pprof/cmdline",
Expand Down Expand Up @@ -537,6 +586,10 @@ func TestAgent_PprofRequest(t *testing.T) {
url = url + "?server_id=" + s.server.LocalMember().Name
}

if tc.clientOnly {
s.Agent.server = nil
}

req, err := http.NewRequest("GET", url, nil)
require.Nil(t, err)
respW := httptest.NewRecorder()
Expand Down

0 comments on commit d28898b

Please sign in to comment.