Fill out the tests around coordinate/node functionality

agent/consul/coordinate_endpoint.go

@@ -139,9 +139,6 @@ func (c *Coordinate) Update(args *structs.CoordinateUpdateRequest, reply *struct
 		return err
 	}
 	if rule != nil && c.srv.config.ACLEnforceVersion8 {
-		// We don't enforce the sentinel policy here, since at this time
-		// sentinel only applies to creating or updating node or service
-		// info, not updating coordinates.
 		if !rule.NodeWrite(args.Node, nil) {
 			return acl.ErrPermissionDenied
 		}
@@ -213,10 +210,7 @@ func (c *Coordinate) Node(args *structs.NodeSpecificRequest, reply *structs.Inde
 		return err
 	}
 	if rule != nil && c.srv.config.ACLEnforceVersion8 {
-		// We don't enforce the sentinel policy here, since at this time
+		if !rule.NodeRead(args.Node) {
-		// sentinel only applies to creating or updating node or service
-		// info, not updating coordinates.
-		if !rule.NodeWrite(args.Node, nil) {
 			return acl.ErrPermissionDenied
 		}
 	}

agent/consul/coordinate_endpoint_test.go

@@ -555,27 +555,44 @@ func TestCoordinate_Node_ACLDeny(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Send an update for the first node. This should go through since we
+	coord := generateRandomCoordinate()
-	// don't have version 8 ACLs enforced yet.
 	req := structs.CoordinateUpdateRequest{
 		Datacenter: "dc1",
 		Node:       "node1",
-		Coord:      generateRandomCoordinate(),
+		Coord:      coord,
 	}
 	var out struct{}
 	if err := msgpackrpc.CallWithCodec(codec, "Coordinate.Update", &req, &out); err != nil {
 		t.Fatalf("err: %v", err)
 	}
 
+	// Try a read for the first node. This should go through since we
+	// don't have version 8 ACLs enforced yet.
+	arg := structs.NodeSpecificRequest{
+		Node:       "node1",
+		Datacenter: "dc1",
+	}
+	resp := structs.IndexedCoordinates{}
+	retry.Run(t, func(r *retry.R) {
+		if err := msgpackrpc.CallWithCodec(codec, "Coordinate.Node", &arg, &resp); err != nil {
+			r.Fatalf("err: %v", err)
+		}
+		if len(resp.Coordinates) != 1 ||
+			resp.Coordinates[0].Node != "node1" {
+			r.Fatalf("bad: %v", resp.Coordinates)
+		}
+		verify.Values(t, "", resp.Coordinates[0].Coord, coord)
+	})
+
 	// Now turn on version 8 enforcement and try again.
 	s1.config.ACLEnforceVersion8 = true
-	err := msgpackrpc.CallWithCodec(codec, "Coordinate.Update", &req, &out)
+	err := msgpackrpc.CallWithCodec(codec, "Coordinate.Node", &arg, &resp)
 	if !acl.IsErrPermissionDenied(err) {
 		t.Fatalf("err: %v", err)
 	}
 
-	// Create an ACL that can write to the node.
+	// Create an ACL that can read from the node.
-	arg := structs.ACLRequest{
+	aclReq := structs.ACLRequest{
 		Datacenter: "dc1",
 		Op:         structs.ACLSet,
 		ACL: structs.ACL{
@@ -583,26 +600,26 @@ func TestCoordinate_Node_ACLDeny(t *testing.T) {
 			Type: structs.ACLTypeClient,
 			Rules: `
 node "node1" {
-	policy = "write"
+	policy = "read"
 }
 `,
 		},
 		WriteRequest: structs.WriteRequest{Token: "root"},
 	}
 	var id string
-	if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &id); err != nil {
+	if err := msgpackrpc.CallWithCodec(codec, "ACL.Apply", &aclReq, &id); err != nil {
 		t.Fatalf("err: %v", err)
 	}
 
 	// With the token, it should now go through.
-	req.Token = id
+	arg.Token = id
-	if err := msgpackrpc.CallWithCodec(codec, "Coordinate.Update", &req, &out); err != nil {
+	if err := msgpackrpc.CallWithCodec(codec, "Coordinate.Node", &arg, &resp); err != nil {
 		t.Fatalf("err: %v", err)
 	}
 
 	// But it should be blocked for the other node.
-	req.Node = "node2"
+	arg.Node = "node2"
-	err = msgpackrpc.CallWithCodec(codec, "Coordinate.Update", &req, &out)
+	err = msgpackrpc.CallWithCodec(codec, "Coordinate.Node", &arg, &resp)
 	if !acl.IsErrPermissionDenied(err) {
 		t.Fatalf("err: %v", err)
 	}

agent/consul/state/coordinate_test.go

@@ -42,7 +42,8 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	}
 	verify.Values(t, "", all, structs.Coordinates{})
 
-	_, coords, err := s.Coordinate("nope", nil)
+	coordinateWs := memdb.NewWatchSet()
+	_, coords, err := s.Coordinate("nope", coordinateWs)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -63,7 +64,7 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	if err := s.CoordinateBatchUpdate(1, updates); err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	if watchFired(ws) {
+	if watchFired(ws) || watchFired(coordinateWs) {
 		t.Fatalf("bad")
 	}
 
@@ -79,13 +80,22 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	}
 	verify.Values(t, "", all, structs.Coordinates{})
 
+	coordinateWs = memdb.NewWatchSet()
+	idx, coords, err = s.Coordinate("node1", coordinateWs)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if idx != 1 {
+		t.Fatalf("bad index: %d", idx)
+	}
+
 	// Register the nodes then do the update again.
 	testRegisterNode(t, s, 1, "node1")
 	testRegisterNode(t, s, 2, "node2")
 	if err := s.CoordinateBatchUpdate(3, updates); err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	if !watchFired(ws) {
+	if !watchFired(ws) || !watchFired(coordinateWs) {
 		t.Fatalf("bad")
 	}
 
@@ -101,11 +111,16 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	verify.Values(t, "", all, updates)
 
 	// Also verify the per-node coordinate interface.
-	for _, update := range updates {
+	nodeWs := make([]memdb.WatchSet, len(updates))
-		_, coords, err := s.Coordinate(update.Node, nil)
+	for i, update := range updates {
+		nodeWs[i] = memdb.NewWatchSet()
+		idx, coords, err := s.Coordinate(update.Node, nodeWs[i])
 		if err != nil {
 			t.Fatalf("err: %s", err)
 		}
+		if idx != 3 {
+			t.Fatalf("bad index: %d", idx)
+		}
 		expected := lib.CoordinateSet{
 			"": update.Coord,
 		}
@@ -120,6 +135,11 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	if !watchFired(ws) {
 		t.Fatalf("bad")
 	}
+	for _, ws := range nodeWs {
+		if !watchFired(ws) {
+			t.Fatalf("bad")
+		}
+	}
 
 	// Verify it got applied.
 	idx, all, err = s.Coordinates(nil)
@@ -133,10 +153,13 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 
 	// And check the per-node coordinate version of the same thing.
 	for _, update := range updates {
-		_, coords, err := s.Coordinate(update.Node, nil)
+		idx, coords, err := s.Coordinate(update.Node, nil)
 		if err != nil {
 			t.Fatalf("err: %s", err)
 		}
+		if idx != 4 {
+			t.Fatalf("bad index: %d", idx)
+		}
 		expected := lib.CoordinateSet{
 			"": update.Coord,
 		}

agent/coordinate_endpoint.go

@@ -86,7 +86,7 @@ func (s *HTTPServer) CoordinateNodes(resp http.ResponseWriter, req *http.Request
 		return nil, err
 	}
 
-	return filterCoordinates(req, "", out.Coordinates), nil
+	return filterCoordinates(req, out.Coordinates), nil
 }
 
 // CoordinateNode returns the LAN node in the given datacenter, along with
@@ -109,10 +109,16 @@ func (s *HTTPServer) CoordinateNode(resp http.ResponseWriter, req *http.Request)
 		return nil, err
 	}
 
-	return filterCoordinates(req, node, out.Coordinates), nil
+	result := filterCoordinates(req, out.Coordinates)
+	if len(result) == 0 {
+		resp.WriteHeader(http.StatusNotFound)
+		return nil, nil
+	}
+
+	return result, nil
 }
 
-func filterCoordinates(req *http.Request, node string, in structs.Coordinates) structs.Coordinates {
+func filterCoordinates(req *http.Request, in structs.Coordinates) structs.Coordinates {
 	out := structs.Coordinates{}
 
 	if in == nil {
@@ -126,9 +132,6 @@ func filterCoordinates(req *http.Request, node string, in structs.Coordinates) s
 	}
 
 	for _, c := range in {
-		if node != "" && c.Node != node {
-			continue
-		}
 		if filterBySegment && c.Segment != segment {
 			continue
 		}

agent/coordinate_endpoint_test.go

@@ -146,17 +146,15 @@ func TestCoordinate_Node(t *testing.T) {
 	a := NewTestAgent(t.Name(), "")
 	defer a.Shutdown()
 
-	// Make sure an empty list is non-nil.
+	// Make sure we get a 404 with no coordinates.
 	req, _ := http.NewRequest("GET", "/v1/coordinate/node/foo?dc=dc1", nil)
 	resp := httptest.NewRecorder()
 	obj, err := a.srv.CoordinateNode(resp, req)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
-
+	if resp.Code != http.StatusNotFound {
-	coordinates := obj.(structs.Coordinates)
+		t.Fatalf("bad: %v", resp.Code)
-	if coordinates == nil || len(coordinates) != 0 {
-		t.Fatalf("bad: %v", coordinates)
 	}
 
 	// Register the nodes.
@@ -204,7 +202,7 @@ func TestCoordinate_Node(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	coordinates = obj.(structs.Coordinates)
+	coordinates := obj.(structs.Coordinates)
 	if len(coordinates) != 1 ||
 		coordinates[0].Node != "foo" {
 		t.Fatalf("bad: %v", coordinates)
@@ -217,10 +215,8 @@ func TestCoordinate_Node(t *testing.T) {
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
-
+	if resp.Code != http.StatusNotFound {
-	coordinates = obj.(structs.Coordinates)
+		t.Fatalf("bad: %v", resp.Code)
-	if len(coordinates) != 0 {
-		t.Fatalf("bad: %v", coordinates)
 	}
 
 	// Filter on a real node segment
@@ -243,9 +239,7 @@ func TestCoordinate_Node(t *testing.T) {
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
-
+	if resp.Code != http.StatusNotFound {
-	coordinates = obj.(structs.Coordinates)
+		t.Fatalf("bad: %v", resp.Code)
-	if len(coordinates) != 0 {
-		t.Fatalf("bad: %v", coordinates)
 	}
 }

api/coordinate_test.go

@@ -3,6 +3,8 @@ package api
 import (
 	"testing"
 
+	"strings"
+
 	"github.com/hashicorp/consul/testutil/retry"
 )
 
@@ -51,7 +53,7 @@ func TestAPI_CoordinateNode(t *testing.T) {
 	coordinate := c.Coordinate()
 	retry.Run(t, func(r *retry.R) {
 		_, _, err := coordinate.Node(s.Config.NodeName, nil)
-		if err != nil {
+		if err != nil && !strings.Contains(err.Error(), "Unexpected response code: 404") {
 			r.Fatal(err)
 		}
 

website/source/api/coordinate.html.md

@@ -94,6 +94,11 @@ The table below shows this endpoint's support for
 - `dc` `(string: "")` - Specifies the datacenter to query. This will default to
   the datacenter of the agent being queried. This is specified as part of the
   URL as a query parameter.
+- `segment` `(string: "")` - (Enterprise-only) Specifies the segment to list members for.
+  If left blank, this will query for the default segment when connecting to a server and
+  the agent's own segment when connecting to a client (clients can only be part of one
+  network segment). When querying a server, setting this to the special string `_all`
+  will show members in all segments.
 
 ### Sample Request
 
@@ -125,8 +130,7 @@ segment.
 
 ## Read LAN Coordinates for a node
 
-This endpoint returns the LAN network coordinates for all nodes in a given
+This endpoint returns the LAN network coordinates for the given node.
-datacenter.
 
 | Method | Path                         | Produces                   |
 | ------ | ---------------------------- | -------------------------- |
@@ -146,6 +150,11 @@ The table below shows this endpoint's support for
 - `dc` `(string: "")` - Specifies the datacenter to query. This will default to
   the datacenter of the agent being queried. This is specified as part of the
   URL as a query parameter.
+- `segment` `(string: "")` - (Enterprise-only) Specifies the segment to list members for.
+  If left blank, this will query for the default segment when connecting to a server and
+  the agent's own segment when connecting to a client (clients can only be part of one
+  network segment). When querying a server, setting this to the special string `_all`
+  will show members in all segments.
 
 ### Sample Request