From 9909b661aced5a4e8f1033d6967ed88bb2b14fcd Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Tue, 31 Oct 2017 15:08:14 -0700 Subject: [PATCH] Fill out the tests around coordinate/node functionality --- agent/consul/coordinate_endpoint.go | 8 +---- agent/consul/coordinate_endpoint_test.go | 41 +++++++++++++++++------- agent/consul/state/coordinate_test.go | 35 ++++++++++++++++---- agent/coordinate_endpoint.go | 15 +++++---- agent/coordinate_endpoint_test.go | 22 +++++-------- api/coordinate_test.go | 4 ++- website/source/api/coordinate.html.md | 13 ++++++-- 7 files changed, 90 insertions(+), 48 deletions(-) diff --git a/agent/consul/coordinate_endpoint.go b/agent/consul/coordinate_endpoint.go index 559112221..f683e4874 100644 --- a/agent/consul/coordinate_endpoint.go +++ b/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 - // sentinel only applies to creating or updating node or service - // info, not updating coordinates. - if !rule.NodeWrite(args.Node, nil) { + if !rule.NodeRead(args.Node) { return acl.ErrPermissionDenied } } diff --git a/agent/consul/coordinate_endpoint_test.go b/agent/consul/coordinate_endpoint_test.go index d697f9234..58eb13fab 100644 --- a/agent/consul/coordinate_endpoint_test.go +++ b/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 - // don't have version 8 ACLs enforced yet. + coord := generateRandomCoordinate() 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. - arg := structs.ACLRequest{ + // Create an ACL that can read from the node. + 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 - if err := msgpackrpc.CallWithCodec(codec, "Coordinate.Update", &req, &out); err != nil { + arg.Token = id + 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" - err = msgpackrpc.CallWithCodec(codec, "Coordinate.Update", &req, &out) + arg.Node = "node2" + err = msgpackrpc.CallWithCodec(codec, "Coordinate.Node", &arg, &resp) if !acl.IsErrPermissionDenied(err) { t.Fatalf("err: %v", err) } diff --git a/agent/consul/state/coordinate_test.go b/agent/consul/state/coordinate_test.go index dcc59c864..7bd962521 100644 --- a/agent/consul/state/coordinate_test.go +++ b/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 { - _, coords, err := s.Coordinate(update.Node, nil) + nodeWs := make([]memdb.WatchSet, len(updates)) + 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, } diff --git a/agent/coordinate_endpoint.go b/agent/coordinate_endpoint.go index ade8b582a..deaad933e 100644 --- a/agent/coordinate_endpoint.go +++ b/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 } diff --git a/agent/coordinate_endpoint_test.go b/agent/coordinate_endpoint_test.go index 74da244c0..c7005e926 100644 --- a/agent/coordinate_endpoint_test.go +++ b/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) } - - coordinates := obj.(structs.Coordinates) - if coordinates == nil || len(coordinates) != 0 { - t.Fatalf("bad: %v", coordinates) + if resp.Code != http.StatusNotFound { + t.Fatalf("bad: %v", resp.Code) } // 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) } - - coordinates = obj.(structs.Coordinates) - if len(coordinates) != 0 { - t.Fatalf("bad: %v", coordinates) + if resp.Code != http.StatusNotFound { + t.Fatalf("bad: %v", resp.Code) } // Filter on a real node segment @@ -243,9 +239,7 @@ func TestCoordinate_Node(t *testing.T) { if err != nil { t.Fatalf("err: %v", err) } - - coordinates = obj.(structs.Coordinates) - if len(coordinates) != 0 { - t.Fatalf("bad: %v", coordinates) + if resp.Code != http.StatusNotFound { + t.Fatalf("bad: %v", resp.Code) } } diff --git a/api/coordinate_test.go b/api/coordinate_test.go index a91d1869b..aae1ddf1a 100644 --- a/api/coordinate_test.go +++ b/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) } diff --git a/website/source/api/coordinate.html.md b/website/source/api/coordinate.html.md index e3d87fcab..a1bfecb60 100644 --- a/website/source/api/coordinate.html.md +++ b/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 -datacenter. +This endpoint returns the LAN network coordinates for the given node. | 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