region forwarding; prevent recursive forwards for impossible requests

prevent region forwarding loop, backfill tests

api/agent_test.go

@@ -389,9 +389,21 @@ func TestAgentCPUProfile(t *testing.T) {
 		AuthToken: token.SecretID,
 	}
 
-	resp, err := agent.CPUProfile("", "", 1, q)
-	require.NoError(t, err)
-	require.NotNil(t, resp)
+	// Valid local request
+	{
+		resp, err := agent.CPUProfile("", "", 1, q)
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+	}
+
+	// Invalid server request
+	{
+		resp, err := agent.CPUProfile("unknown.global", "", 1, q)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "500 (unknown nomad server unknown.global)")
+		require.Nil(t, resp)
+	}
+
 }
 
 func TestAgentTrace(t *testing.T) {

client/agent_endpoint.go

@@ -32,9 +32,9 @@ func NewAgentEndpoint(c *Client) *Agent {
 func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPprofResponse) error {
 	// Check ACL for agent write
 	if aclObj, err := a.c.ResolveToken(args.AuthToken); err != nil {
-		return structs.NewErrRPCCoded(500, err.Error())
+		return err
 	} else if aclObj != nil && !aclObj.AllowAgentWrite() {
-		return structs.NewErrRPCCoded(403, structs.ErrPermissionDenied.Error())
+		return structs.ErrPermissionDenied
 	}
 
 	var resp []byte

client/agent_endpoint_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hashicorp/nomad/client/config"
 	sframer "github.com/hashicorp/nomad/client/lib/streamframer"
 	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/command/agent/profile"
 	"github.com/hashicorp/nomad/nomad"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
@@ -213,3 +214,115 @@ func TestMonitor_Monitor_ACL(t *testing.T) {
 		})
 	}
 }
+
+func TestAgentProfile(t *testing.T) {
+	t.Parallel()
+	require := require.New(t)
+
+	// start server and client
+	s1, cleanup := nomad.TestServer(t, nil)
+	defer cleanup()
+
+	testutil.WaitForLeader(t, s1.RPC)
+
+	c, cleanupC := TestClient(t, func(c *config.Config) {
+		c.Servers = []string{s1.GetConfig().RPCAddr.String()}
+	})
+	defer cleanupC()
+
+	// Successful request
+	{
+		req := structs.AgentPprofRequest{
+			ReqType: profile.CPUReq,
+			NodeID:  c.NodeID(),
+		}
+
+		reply := structs.AgentPprofResponse{}
+
+		err := c.ClientRPC("Agent.Profile", &req, &reply)
+		require.NoError(err)
+
+		require.NotNil(reply.Payload)
+		require.Equal(c.NodeID(), reply.AgentID)
+	}
+
+	// Unknown profile request
+	{
+		req := structs.AgentPprofRequest{
+			ReqType: profile.LookupReq,
+			Profile: "unknown",
+			NodeID:  c.NodeID(),
+		}
+
+		reply := structs.AgentPprofResponse{}
+
+		err := c.ClientRPC("Agent.Profile", &req, &reply)
+		require.EqualError(err, "RPC Error:: 404,Pprof profile not found profile: unknown")
+	}
+}
+
+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()
+	testutil.WaitForLeader(t, s.RPC)
+
+	c, cleanupC := TestClient(t, func(c *config.Config) {
+		c.ACLEnabled = true
+		c.Servers = []string{s.GetConfig().RPCAddr.String()}
+	})
+	defer cleanupC()
+
+	policyBad := mock.AgentPolicy(acl.PolicyRead)
+	tokenBad := mock.CreatePolicyAndToken(t, s.State(), 1005, "invalid", policyBad)
+
+	policyGood := mock.AgentPolicy(acl.PolicyWrite)
+	tokenGood := mock.CreatePolicyAndToken(t, s.State(), 1009, "valid", policyGood)
+
+	cases := []struct {
+		Name    string
+		Token   string
+		authErr bool
+	}{
+		{
+			Name:    "bad token",
+			Token:   tokenBad.SecretID,
+			authErr: true,
+		},
+		{
+			Name:  "good token",
+			Token: tokenGood.SecretID,
+		},
+		{
+			Name:  "root token",
+			Token: root.SecretID,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			req := &structs.AgentPprofRequest{
+				ReqType: profile.CmdReq,
+				QueryOptions: structs.QueryOptions{
+					Namespace: structs.DefaultNamespace,
+					Region:    "global",
+					AuthToken: tc.Token,
+				},
+			}
+
+			reply := &structs.AgentPprofResponse{}
+
+			err := c.ClientRPC("Agent.Profile", req, reply)
+			if tc.authErr {
+				require.EqualError(err, structs.ErrPermissionDenied.Error())
+			} else {
+				require.NoError(err)
+				require.NotNil(reply.Payload)
+			}
+		})
+	}
+}

command/agent/agent_endpoint.go

@@ -426,12 +426,7 @@ func (s *HTTPServer) agentPprof(reqType profile.ReqType, resp http.ResponseWrite
 	}
 
 	if rpcErr != nil {
-		code, msg, ok := structs.CodeFromRPCCodedErr(rpcErr)
-		if !ok {
-			return nil, CodedError(500, rpcErr.Error())
-		}
-		// Return CodedError
-		return nil, CodedError(code, msg)
+		return nil, rpcErr
 	}
 
 	// Set headers from profile request

command/agent/agent_endpoint_test.go

@@ -489,43 +489,46 @@ func TestAgent_PprofRequest_Permissions(t *testing.T) {
 
 func TestAgent_PprofRequest(t *testing.T) {
 	cases := []struct {
-		desc           string
-		url            string
-		addNodeID      bool
-		addServerID    bool
-		expectedErr    string
-		expectedStatus int
+		desc        string
+		url         string
+		addNodeID   bool
+		addServerID bool
+		expectedErr string
 	}{
 		{
-			desc:           "cmdline request",
-			url:            "/v1/agent/pprof/cmdline",
-			addNodeID:      true,
-			expectedStatus: 200,
+			desc: "cmdline local request",
+			url:  "/v1/agent/pprof/cmdline",
 		},
 		{
-			desc:           "cpu profile request",
-			url:            "/v1/agent/pprof/profile",
-			addNodeID:      true,
-			expectedStatus: 200,
+			desc:      "cmdline node request",
+			url:       "/v1/agent/pprof/cmdline",
+			addNodeID: true,
 		},
 		{
-			desc:           "trace request",
-			url:            "/v1/agent/pprof/trace",
-			addNodeID:      true,
-			expectedStatus: 200,
+			desc:        "cmdline server request",
+			url:         "/v1/agent/pprof/cmdline",
+			addServerID: true,
 		},
 		{
-			desc:           "pprof lookup request",
-			url:            "/v1/agent/pprof/goroutine",
-			addNodeID:      true,
-			expectedStatus: 200,
+			desc:      "cpu profile request",
+			url:       "/v1/agent/pprof/profile",
+			addNodeID: true,
 		},
 		{
-			desc:           "unknown pprof lookup request",
-			url:            "/v1/agent/pprof/latency",
-			addNodeID:      true,
-			expectedStatus: 404,
-			expectedErr:    "Unknown profile: latency",
+			desc:      "trace request",
+			url:       "/v1/agent/pprof/trace",
+			addNodeID: true,
+		},
+		{
+			desc:      "pprof lookup request",
+			url:       "/v1/agent/pprof/goroutine",
+			addNodeID: true,
+		},
+		{
+			desc:        "unknown pprof lookup request",
+			url:         "/v1/agent/pprof/latency",
+			addNodeID:   true,
+			expectedErr: "RPC Error:: 404,Pprof profile not found profile: latency",
 		},
 	}
 
@@ -549,10 +552,7 @@ func TestAgent_PprofRequest(t *testing.T) {
 
 				if tc.expectedErr != "" {
 					require.Error(t, err)
-
-					httpErr, ok := err.(HTTPCodedError)
-					require.True(t, ok)
-					require.Equal(t, httpErr.Code(), tc.expectedStatus)
+					require.EqualError(t, err, tc.expectedErr)
 				} else {
 					require.NoError(t, err)
 					require.NotNil(t, resp)

command/agent/profile/pprof.go

@@ -25,7 +25,7 @@ const (
 	TraceReq  ReqType = "trace"
 	LookupReq ReqType = "profile"
 
-	ErrProfileNotFoundPrefix = "Pprof profile not found"
+	ErrProfileNotFoundPrefix = "Pprof profile not found profile:"
 )
 
 // NewErrProfileNotFound returns a new error caused by a pprof.Lookup

nomad/client_agent_endpoint.go

@@ -29,14 +29,27 @@ func (a *Agent) register() {
 }
 
 func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPprofResponse) error {
+	// handle when serverID does not exist for requested region
+	region := args.RequestRegion()
+	if region == "" {
+		return fmt.Errorf("missing target RPC")
+	}
+
+	// Handle region forwarding
+	if region != a.srv.config.Region {
+		// Mark that we are forwarding
+		args.SetForwarded()
+		return a.srv.forwardRegion(region, "Agent.Profile", args, reply)
+	}
+
 	// Targeting a node, forward request to node
 	if args.NodeID != "" {
 		return a.forwardProfileClient(args, reply)
 	}
 
 	// Handle serverID not equal to ours
 	if args.ServerID != "" {
-		serverToFwd, err := a.serverFor(args.ServerID)
+		serverToFwd, err := a.serverFor(args.ServerID, region)
 		if err != nil {
 			return err
 		}
@@ -47,9 +60,9 @@ func (a *Agent) Profile(args *structs.AgentPprofRequest, reply *structs.AgentPpr
 
 	// Check ACL for agent write
 	if aclObj, err := a.srv.ResolveToken(args.AuthToken); err != nil {
-		return structs.NewErrRPCCoded(500, err.Error())
+		return err
 	} else if aclObj != nil && !aclObj.AllowAgentWrite() {
-		return structs.NewErrRPCCoded(403, structs.ErrPermissionDenied.Error())
+		return structs.ErrPermissionDenied
 	}
 
 	// Process the request on this server
@@ -247,7 +260,7 @@ OUTER:
 	}
 }
 
-func (a *Agent) serverFor(serverID string) (*serverParts, error) {
+func (a *Agent) serverFor(serverID, region string) (*serverParts, error) {
 	var target *serverParts
 
 	if serverID == "leader" {
@@ -267,6 +280,12 @@ func (a *Agent) serverFor(serverID string) (*serverParts, error) {
 			// with a serf member
 			if mem.Name == serverID || mem.Tags["id"] == serverID {
 				if ok, srv := isNomadServer(mem); ok {
+					if srv.Region != region {
+						return nil,
+							fmt.Errorf(
+								"Requested server:%s region:%s does not exist in requested region: %s",
+								serverID, srv.Region, region)
+					}
 					target = srv
 				}
 
@@ -281,7 +300,7 @@ func (a *Agent) serverFor(serverID string) (*serverParts, error) {
 
 	// ServerID is this current server,
 	// No need to forward request
-	if target.ID == a.srv.GetConfig().NodeID {
+	if target.Name == a.srv.LocalMember().Name {
 		return nil, nil
 	}