Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix panic when monitoring a local client node #7053

Merged
merged 3 commits into from
Feb 3, 2020
Merged

Fix panic when monitoring a local client node #7053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39a6c63
Fix panic when monitoring a local client node
drewbailey Feb 3, 2020
4e9dc03
agent Profile req nil check s.agent.Server()
drewbailey Feb 3, 2020
173ad83
update changelog
drewbailey Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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