From 39a6c6374afd3265b65efdbfd4e1fb894640a13d Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 3 Feb 2020 10:17:59 -0500 Subject: [PATCH 1/3] Fix panic when monitoring a local client node Fixes a panic when accessing a.agent.Server() when agent is a client instead. This pr removes a redundant ACL check since ACLs are validated at the RPC layer. It also nil checks the agent server and uses Client() when appropriate. --- command/agent/agent_endpoint.go | 18 +++++-------- command/agent/agent_endpoint_test.go | 40 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/command/agent/agent_endpoint.go b/command/agent/agent_endpoint.go index 4e76a8ec8541..416f403b5cb3 100644 --- a/command/agent/agent_endpoint.go +++ b/command/agent/agent_endpoint.go @@ -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 == "" { @@ -229,8 +219,12 @@ func (s *HTTPServer) AgentMonitor(resp http.ResponseWriter, req *http.Request) ( handlerErr = CodedError(400, "No local Node and node_id not provided") } } else { - // No node id monitor server - handler, handlerErr = s.agent.Server().StreamingRpcHandler("Agent.Monitor") + // No node id monitor current server/client + if srv := s.agent.Server(); srv != nil { + handler, handlerErr = srv.StreamingRpcHandler("Agent.Monitor") + } else { + handler, handlerErr = s.agent.Client().StreamingRpcHandler("Agent.Monitor") + } } if handlerErr != nil { diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index 54f86cff74d5..32acc6698f61 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -386,6 +386,46 @@ func TestHTTP_AgentMonitor(t *testing.T) { require.Fail(t, err.Error()) }) } + + // stream logs for a local client + { + 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) + require.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()) + }) + } }) } From 4e9dc03d7dd329157522721652502a5440f14a17 Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 3 Feb 2020 10:27:29 -0500 Subject: [PATCH 2/3] agent Profile req nil check s.agent.Server() clean up logic and tests --- command/agent/agent_endpoint.go | 16 +++--- command/agent/agent_endpoint_test.go | 73 ++++++++++++++++------------ 2 files changed, 51 insertions(+), 38 deletions(-) diff --git a/command/agent/agent_endpoint.go b/command/agent/agent_endpoint.go index 416f403b5cb3..c7cc6c5d123b 100644 --- a/command/agent/agent_endpoint.go +++ b/command/agent/agent_endpoint.go @@ -218,13 +218,11 @@ func (s *HTTPServer) AgentMonitor(resp http.ResponseWriter, req *http.Request) ( } else { handlerErr = CodedError(400, "No local Node and node_id not provided") } - } else { // No node id monitor current server/client - if srv := s.agent.Server(); srv != nil { - handler, handlerErr = srv.StreamingRpcHandler("Agent.Monitor") - } else { - handler, handlerErr = s.agent.Client().StreamingRpcHandler("Agent.Monitor") - } + } else if srv := s.agent.Server(); srv != nil { + handler, handlerErr = srv.StreamingRpcHandler("Agent.Monitor") + } else { + handler, handlerErr = s.agent.Client().StreamingRpcHandler("Agent.Monitor") } if handlerErr != nil { @@ -395,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 { diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index 32acc6698f61..01457ad4fb74 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -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" ) @@ -255,35 +256,34 @@ 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() @@ -291,7 +291,7 @@ func TestHTTP_AgentMonitor(t *testing.T) { 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 @@ -313,10 +313,11 @@ 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() @@ -324,7 +325,7 @@ func TestHTTP_AgentMonitor(t *testing.T) { 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 @@ -346,10 +347,11 @@ 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() @@ -357,7 +359,7 @@ func TestHTTP_AgentMonitor(t *testing.T) { 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 @@ -385,10 +387,11 @@ func TestHTTP_AgentMonitor(t *testing.T) { }, func(err error) { require.Fail(t, err.Error()) }) - } + }) + }) - // stream logs for a local client - { + 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() @@ -398,7 +401,7 @@ func TestHTTP_AgentMonitor(t *testing.T) { // set server to nil to monitor as client s.Agent.server = nil _, err = s.Server.AgentMonitor(resp, req) - require.NoError(t, err) + assert.NoError(t, err) }() // send the same log until monitor sink is set up @@ -425,7 +428,7 @@ func TestHTTP_AgentMonitor(t *testing.T) { }, func(err error) { require.Fail(t, err.Error()) }) - } + }) }) } @@ -521,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", @@ -577,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() From 173ad8315fad2c095bf3f127a83026b70244df98 Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 3 Feb 2020 12:06:29 -0500 Subject: [PATCH 3/3] update changelog --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0117640331bd..b782677e02a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)]