From 17764589566692ef4f9268a88ca3ffff239e4bb0 Mon Sep 17 00:00:00 2001 From: Drew Bailey <2614075+drewbailey@users.noreply.github.com> Date: Mon, 16 Dec 2019 13:42:18 -0500 Subject: [PATCH] address pr feedback --- api/agent.go | 4 ---- client/agent_endpoint_test.go | 1 - command/agent/agent_endpoint.go | 10 +++++----- command/agent/agent_endpoint_test.go | 2 +- command/agent/http.go | 6 ++++-- command/agent/profile/pprof.go | 6 ------ helper/pool/pool.go | 1 + nomad/client_agent_endpoint.go | 13 ++++++++----- website/source/api/agent.html.md | 26 ++++++++++++++++---------- 9 files changed, 35 insertions(+), 34 deletions(-) diff --git a/api/agent.go b/api/agent.go index fbcbd04ff..884d44688 100644 --- a/api/agent.go +++ b/api/agent.go @@ -379,10 +379,6 @@ func (a *Agent) Trace(serverID, nodeID string, seconds int, q *QueryOptions) ([] // The call blocks until the profile finishes, and returns the raw bytes of the // profile. func (a *Agent) Profile(serverID, nodeID, profile string, debug int, q *QueryOptions) ([]byte, error) { - if q == nil { - q = &QueryOptions{} - } - if q == nil { q = &QueryOptions{} } diff --git a/client/agent_endpoint_test.go b/client/agent_endpoint_test.go index 52e1aa100..20010ff99 100644 --- a/client/agent_endpoint_test.go +++ b/client/agent_endpoint_test.go @@ -293,7 +293,6 @@ func TestAgentProfile_ACL(t *testing.T) { t.Parallel() require := require.New(t) - // start server // start server s, root, cleanupS := nomad.TestACLServer(t, nil) defer cleanupS() diff --git a/command/agent/agent_endpoint.go b/command/agent/agent_endpoint.go index 1b96c1372..b1e266dd6 100644 --- a/command/agent/agent_endpoint.go +++ b/command/agent/agent_endpoint.go @@ -336,15 +336,15 @@ func (s *HTTPServer) AgentForceLeaveRequest(resp http.ResponseWriter, req *http. func (s *HTTPServer) AgentPprofRequest(resp http.ResponseWriter, req *http.Request) ([]byte, error) { path := strings.TrimPrefix(req.URL.Path, "/v1/agent/pprof/") - switch { - case path == "": + switch path { + case "": // no root index route return nil, CodedError(404, ErrInvalidMethod) - case path == "cmdline": + case "cmdline": return s.agentPprof(profile.CmdReq, resp, req) - case path == "profile": + case "profile": return s.agentPprof(profile.CPUReq, resp, req) - case path == "trace": + case "trace": return s.agentPprof(profile.TraceReq, resp, req) default: // Add profile to request diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index db89e1d38..b98b67e17 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -389,7 +389,7 @@ func TestHTTP_AgentMonitor(t *testing.T) { }) } -// Scenerios when Pprof requests should be available +// Scenarios when Pprof requests should be available // see https://github.com/hashicorp/nomad/issues/6496 // +---------------+------------------+--------+------------------+ // | Endpoint | `enable_debug` | ACLs | **Available?** | diff --git a/command/agent/http.go b/command/agent/http.go index f62b4b7ba..4000c8292 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -219,7 +219,9 @@ func (s *HTTPServer) registerHandlers(enableDebug bool) { s.mux.Handle("/", handleRootFallthrough()) if enableDebug { - s.logger.Warn("enable_debug is set to true. This is insecure and should not be enabled in production") + if !s.agent.config.DevMode { + s.logger.Warn("enable_debug is set to true. This is insecure and should not be enabled in production") + } s.mux.HandleFunc("/debug/pprof/", pprof.Index) s.mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) s.mux.HandleFunc("/debug/pprof/profile", pprof.Profile) @@ -358,7 +360,7 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque return f } -// wrapNonJON is used to wrap functions returning non JSON +// wrapNonJSON is used to wrap functions returning non JSON // serializeable data to make them more convenient. It is primarily // responsible for setting nomad headers and logging. func (s *HTTPServer) wrapNonJSON(handler func(resp http.ResponseWriter, req *http.Request) ([]byte, error)) func(resp http.ResponseWriter, req *http.Request) { diff --git a/command/agent/profile/pprof.go b/command/agent/profile/pprof.go index e3d4fe0ac..51bbc81ff 100644 --- a/command/agent/profile/pprof.go +++ b/command/agent/profile/pprof.go @@ -11,12 +11,6 @@ import ( "time" ) -// goroutine -// threadcreate -// heap -// allocs -// block -// mutex type ReqType string const ( diff --git a/helper/pool/pool.go b/helper/pool/pool.go index 9833382dd..d02251faf 100644 --- a/helper/pool/pool.go +++ b/helper/pool/pool.go @@ -433,6 +433,7 @@ func (p *ConnPool) RPC(region string, addr net.Addr, version int, method string, return err } + // TODO wrap with RPCCoded error instead return fmt.Errorf("rpc error: %v", err) } diff --git a/nomad/client_agent_endpoint.go b/nomad/client_agent_endpoint.go index 8a3a666da..d27b57226 100644 --- a/nomad/client_agent_endpoint.go +++ b/nomad/client_agent_endpoint.go @@ -82,16 +82,17 @@ func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPpr // Determine which profile to run and generate profile. // Blocks for args.Seconds // Our RPC endpoints currently don't support context - // or request cancellation so stubbing with TODO + // or request cancellation so using server shutdownCtx as a + // best effort. switch args.ReqType { case profile.CPUReq: - resp, headers, err = profile.CPUProfile(context.TODO(), args.Seconds) + resp, headers, err = profile.CPUProfile(a.srv.shutdownCtx, args.Seconds) case profile.CmdReq: resp, headers, err = profile.Cmdline() case profile.LookupReq: resp, headers, err = profile.Profile(args.Profile, args.Debug) case profile.TraceReq: - resp, headers, err = profile.Trace(context.TODO(), args.Seconds) + resp, headers, err = profile.Trace(a.srv.shutdownCtx, args.Seconds) default: err = structs.NewErrRPCCoded(404, "Unknown profile request type") } @@ -269,6 +270,9 @@ OUTER: } } +// forwardFor returns a serverParts for a request to be forwarded to. +// A response of nil, nil indicates that the current server is equal to the +// serverID and region so the request should not be forwarded. func (a *Agent) forwardFor(serverID, region string) (*serverParts, error) { var target *serverParts @@ -295,7 +299,6 @@ func (a *Agent) forwardFor(serverID, region string) (*serverParts, error) { } target = srv } - } } } @@ -451,7 +454,7 @@ func (a *Agent) forwardProfileClient(args *structs.AgentPprofRequest, reply *str if node == nil { err := fmt.Errorf("Unknown node %q", nodeID) - return structs.NewErrRPCCoded(400, err.Error()) + return structs.NewErrRPCCoded(404, err.Error()) } if err := nodeSupportsRpc(node); err != nil { diff --git a/website/source/api/agent.html.md b/website/source/api/agent.html.md index d5a3ff185..7cb0d2556 100644 --- a/website/source/api/agent.html.md +++ b/website/source/api/agent.html.md @@ -589,10 +589,13 @@ The table below shows this endpoint's support for ### Default Behavior -The default behavior of this endpoint complex but seeks to maximize security, -backward compatibility, and still allow debug access by default when possible. -The table below outlines the different scenarios which will enable or disable -the endpoint. +This endpoint is enabled whenever ACLs are enabled. Due to the potentially +sensitive nature of data contained in profiles, as well as their significant +performance impact, the agent/pprof endpoint is protected by a high level ACL: +`agent:write`. For these reasons its recommended to leave [`enable_debug`](/docs/configuration/index.html#enable_debug) +unset and only use the ACL-protected endpoints. + +The following table explains when each endpoint is available: | Endpoint | `enable_debug` | ACLs | **Available?** | |------------------|------------------|--------|------------------| @@ -602,39 +605,42 @@ the endpoint. | /v1/agent/pprof | unset | off | no | | /v1/agent/pprof | unset | on | **yes** | | /v1/agent/pprof | `true` | off | yes | -| /v1/agent/pprof | `false` | n/a | **no** | +| /v1/agent/pprof | `false` | on | **yes** | ### Parameters - `node_id` `(string: "a57b2adb-1a30-2dda-8df0-25abb0881952")` - Specifies a text - string containing a node-id to target for streaming. + string containing a Node ID to target for profiling. - `server_id` `(string: "server1.global")` - Specifies a text - string containing a server id, name or "leader" to target a specific remote - server or leader for streaming. + string containing a Server ID, name, or `leader` to target a specific remote + server or leader for profiling. - `seconds` `(int: 3)` - Specifies the amount of time to run a profile or trace request for. - `debug` `(int: 1)` - Specifies if a given pprof profile should be returned as - text/plain instead of application/octet-stream. Defaults to 0, setting to 1 - enables. + human readable plain text instead of the pprof binary format. Defaults to 0, + setting to 1 enables human readable plain text. ### Sample Request ```text $ curl -O -J \ + --header "X-Nomad-Token: 8176afd3-772d-0b71-8f85-7fa5d903e9d4" \ https://localhost:4646/v1/agent/pprof/goroutine?server_id=leader $ go tool pprof goroutine $ curl -O -J \ + --header "X-Nomad-Token: 8176afd3-772d-0b71-8f85-7fa5d903e9d4" \ https://localhost:4646/v1/agent/profile?seconds=5&node_id=a57b2adb-1a30-2dda-8df0-25abb0881952 $ go tool pprof profile $ curl -O -J \ + --header "X-Nomad-Token: 8176afd3-772d-0b71-8f85-7fa5d903e9d4" \ https://localhost:4646/v1/agent/trace?&seconds=5&server_id=server1.global go tool trace trace