address pr feedback

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

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

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

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?**  |

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

command/agent/profile/pprof.go

@@ -11,12 +11,6 @@ import (
 	"time"
 )
 
-// goroutine
-// threadcreate
-// heap
-// allocs
-// block
-// mutex
 type ReqType string
 
 const (

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

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 {

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