From c248b0017a22fb2fcb4f9cb566e32ad07fff2ec9 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Sat, 14 Nov 2015 21:05:37 -0800 Subject: [PATCH] Fixes nil slices from HTTP endpoints. These would manifest in the HTTP output as Javascript nulls instead of empty lists, so we had unintentionally changed the interface while porting to the new state store. We added code to each HTTP endpoint to convert nil slices to empty ones so they JSON-ify properly, and we added tests to catch this in the future. --- command/agent/acl_endpoint.go | 10 ++ command/agent/acl_endpoint_test.go | 35 +++++- command/agent/catalog_endpoint.go | 10 ++ command/agent/catalog_endpoint_test.go | 21 ++++ command/agent/coordinate_endpoint.go | 17 +++ command/agent/coordinate_endpoint_test.go | 25 ++++- command/agent/health_endpoint.go | 28 +++++ command/agent/health_endpoint_test.go | 130 ++++++++++++++++++++-- command/agent/session_endpoint.go | 15 +++ command/agent/session_endpoint_test.go | 49 ++++++++ command/agent/ui_endpoint.go | 22 +++- command/agent/ui_endpoint_test.go | 53 ++++++++- 12 files changed, 398 insertions(+), 17 deletions(-) diff --git a/command/agent/acl_endpoint.go b/command/agent/acl_endpoint.go index 72ae4c20af..3bc054611b 100644 --- a/command/agent/acl_endpoint.go +++ b/command/agent/acl_endpoint.go @@ -176,6 +176,11 @@ func (s *HTTPServer) ACLGet(resp http.ResponseWriter, req *http.Request) (interf if err := s.agent.RPC("ACL.Get", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.ACLs == nil { + out.ACLs = make(structs.ACLs, 0) + } return out.ACLs, nil } @@ -193,5 +198,10 @@ func (s *HTTPServer) ACLList(resp http.ResponseWriter, req *http.Request) (inter if err := s.agent.RPC("ACL.List", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.ACLs == nil { + out.ACLs = make(structs.ACLs, 0) + } return out.ACLs, nil } diff --git a/command/agent/acl_endpoint_test.go b/command/agent/acl_endpoint_test.go index b0dea0c9ba..361d661ac6 100644 --- a/command/agent/acl_endpoint_test.go +++ b/command/agent/acl_endpoint_test.go @@ -94,15 +94,30 @@ func TestACLUpdate_Upsert(t *testing.T) { func TestACLDestroy(t *testing.T) { httpTest(t, func(srv *HTTPServer) { id := makeTestACL(t, srv) - req, err := http.NewRequest("PUT", "/v1/session/destroy/"+id+"?token=root", nil) + req, err := http.NewRequest("PUT", "/v1/acl/destroy/"+id+"?token=root", nil) resp := httptest.NewRecorder() obj, err := srv.ACLDestroy(resp, req) if err != nil { t.Fatalf("err: %v", err) } - if resp := obj.(bool); !resp { + if resp, ok := obj.(bool); !ok || !resp { t.Fatalf("should work") } + + req, err = http.NewRequest("GET", + "/v1/acl/info/"+id, nil) + resp = httptest.NewRecorder() + obj, err = srv.ACLGet(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + respObj, ok := obj.(structs.ACLs) + if !ok { + t.Fatalf("should work") + } + if len(respObj) != 0 { + t.Fatalf("bad: %v", respObj) + } }) } @@ -143,6 +158,22 @@ func TestACLClone(t *testing.T) { } func TestACLGet(t *testing.T) { + httpTest(t, func(srv *HTTPServer) { + req, err := http.NewRequest("GET", "/v1/acl/info/nope", nil) + resp := httptest.NewRecorder() + obj, err := srv.ACLGet(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + respObj, ok := obj.(structs.ACLs) + if !ok { + t.Fatalf("should work") + } + if respObj == nil || len(respObj) != 0 { + t.Fatalf("bad: %v", respObj) + } + }) + httpTest(t, func(srv *HTTPServer) { id := makeTestACL(t, srv) diff --git a/command/agent/catalog_endpoint.go b/command/agent/catalog_endpoint.go index 447822a875..3a5bd98134 100644 --- a/command/agent/catalog_endpoint.go +++ b/command/agent/catalog_endpoint.go @@ -70,6 +70,11 @@ func (s *HTTPServer) CatalogNodes(resp http.ResponseWriter, req *http.Request) ( if err := s.agent.RPC("Catalog.ListNodes", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.Nodes == nil { + out.Nodes = make(structs.Nodes, 0) + } return out.Nodes, nil } @@ -117,6 +122,11 @@ func (s *HTTPServer) CatalogServiceNodes(resp http.ResponseWriter, req *http.Req if err := s.agent.RPC("Catalog.ServiceNodes", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.ServiceNodes == nil { + out.ServiceNodes = make(structs.ServiceNodes, 0) + } return out.ServiceNodes, nil } diff --git a/command/agent/catalog_endpoint_test.go b/command/agent/catalog_endpoint_test.go index 5e4d75aab3..251d288df0 100644 --- a/command/agent/catalog_endpoint_test.go +++ b/command/agent/catalog_endpoint_test.go @@ -351,6 +351,27 @@ func TestCatalogServiceNodes(t *testing.T) { testutil.WaitForLeader(t, srv.agent.RPC, "dc1") + // Make sure an empty list is returned, not a nil + { + req, err := http.NewRequest("GET", "/v1/catalog/service/api?tag=a", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp := httptest.NewRecorder() + obj, err := srv.CatalogServiceNodes(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + + assertIndex(t, resp) + + nodes := obj.(structs.ServiceNodes) + if nodes == nil || len(nodes) != 0 { + t.Fatalf("bad: %v", obj) + } + } + // Register node args := &structs.RegisterRequest{ Datacenter: "dc1", diff --git a/command/agent/coordinate_endpoint.go b/command/agent/coordinate_endpoint.go index 0cc5824cec..92453225d7 100644 --- a/command/agent/coordinate_endpoint.go +++ b/command/agent/coordinate_endpoint.go @@ -45,6 +45,18 @@ func (s *HTTPServer) CoordinateDatacenters(resp http.ResponseWriter, req *http.R } return nil, err } + + // Use empty list instead of nil (these aren't really possible because + // Serf will give back a default coordinate and there's always one DC, + // but it's better to be explicit about what we want here). + for i, _ := range out { + if out[i].Coordinates == nil { + out[i].Coordinates = make(structs.Coordinates, 0) + } + } + if out == nil { + out = make([]structs.DatacenterMap, 0) + } return out, nil } @@ -62,5 +74,10 @@ func (s *HTTPServer) CoordinateNodes(resp http.ResponseWriter, req *http.Request sort.Sort(&sorter{out.Coordinates}) return nil, err } + + // Use empty list instead of nil. + if out.Coordinates == nil { + out.Coordinates = make(structs.Coordinates, 0) + } return out.Coordinates, nil } diff --git a/command/agent/coordinate_endpoint_test.go b/command/agent/coordinate_endpoint_test.go index aee7bc18cd..8da0a5b397 100644 --- a/command/agent/coordinate_endpoint_test.go +++ b/command/agent/coordinate_endpoint_test.go @@ -48,6 +48,23 @@ func TestCoordinate_Nodes(t *testing.T) { testutil.WaitForLeader(t, srv.agent.RPC, "dc1") + // Make sure an empty list is non-nil. + req, err := http.NewRequest("GET", "/v1/coordinate/nodes?dc=dc1", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp := httptest.NewRecorder() + obj, err := srv.CoordinateNodes(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) + } + // Register the nodes. nodes := []string{"foo", "bar"} for _, node := range nodes { @@ -85,18 +102,18 @@ func TestCoordinate_Nodes(t *testing.T) { time.Sleep(200 * time.Millisecond) // Query back and check the nodes are present and sorted correctly. - req, err := http.NewRequest("GET", "/v1/coordinate/nodes?dc=dc1", nil) + req, err = http.NewRequest("GET", "/v1/coordinate/nodes?dc=dc1", nil) if err != nil { t.Fatalf("err: %v", err) } - resp := httptest.NewRecorder() - obj, err := srv.CoordinateNodes(resp, req) + resp = httptest.NewRecorder() + obj, err = srv.CoordinateNodes(resp, req) if err != nil { t.Fatalf("err: %v", err) } - coordinates := obj.(structs.Coordinates) + coordinates = obj.(structs.Coordinates) if len(coordinates) != 2 || coordinates[0].Node != "bar" || coordinates[1].Node != "foo" { diff --git a/command/agent/health_endpoint.go b/command/agent/health_endpoint.go index f95e4c5e72..e4a36f6a6a 100644 --- a/command/agent/health_endpoint.go +++ b/command/agent/health_endpoint.go @@ -28,6 +28,11 @@ func (s *HTTPServer) HealthChecksInState(resp http.ResponseWriter, req *http.Req if err := s.agent.RPC("Health.ChecksInState", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.HealthChecks == nil { + out.HealthChecks = make(structs.HealthChecks, 0) + } return out.HealthChecks, nil } @@ -52,6 +57,11 @@ func (s *HTTPServer) HealthNodeChecks(resp http.ResponseWriter, req *http.Reques if err := s.agent.RPC("Health.NodeChecks", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.HealthChecks == nil { + out.HealthChecks = make(structs.HealthChecks, 0) + } return out.HealthChecks, nil } @@ -77,6 +87,11 @@ func (s *HTTPServer) HealthServiceChecks(resp http.ResponseWriter, req *http.Req if err := s.agent.RPC("Health.ServiceChecks", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.HealthChecks == nil { + out.HealthChecks = make(structs.HealthChecks, 0) + } return out.HealthChecks, nil } @@ -114,6 +129,19 @@ func (s *HTTPServer) HealthServiceNodes(resp http.ResponseWriter, req *http.Requ if _, ok := params["passing"]; ok { out.Nodes = filterNonPassing(out.Nodes) } + + // Use empty list instead of nil + for i, _ := range out.Nodes { + // TODO (slackpad) It's lame that this isn't a slice of pointers + // but it's not a well-scoped change to fix this. We should + // change this at the next opportunity. + if out.Nodes[i].Checks == nil { + out.Nodes[i].Checks = make(structs.HealthChecks, 0) + } + } + if out.Nodes == nil { + out.Nodes = make(structs.CheckServiceNodes, 0) + } return out.Nodes, nil } diff --git a/command/agent/health_endpoint_test.go b/command/agent/health_endpoint_test.go index 317203065b..02330a37c0 100644 --- a/command/agent/health_endpoint_test.go +++ b/command/agent/health_endpoint_test.go @@ -15,6 +15,31 @@ import ( ) func TestHealthChecksInState(t *testing.T) { + httpTest(t, func(srv *HTTPServer) { + req, err := http.NewRequest("GET", "/v1/health/state/warning?dc=dc1", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + testutil.WaitForResult(func() (bool, error) { + resp := httptest.NewRecorder() + obj, err := srv.HealthChecksInState(resp, req) + if err != nil { + return false, err + } + if err := checkIndex(resp); err != nil { + return false, err + } + + // Should be a non-nil empty list + nodes := obj.(structs.HealthChecks) + if nodes == nil || len(nodes) != 0 { + return false, fmt.Errorf("bad: %v", obj) + } + return true, nil + }, func(err error) { t.Fatalf("err: %v", err) }) + }) + httpTest(t, func(srv *HTTPServer) { req, err := http.NewRequest("GET", "/v1/health/state/passing?dc=dc1", nil) if err != nil { @@ -130,8 +155,7 @@ func TestHealthNodeChecks(t *testing.T) { testutil.WaitForLeader(t, srv.agent.RPC, "dc1") - req, err := http.NewRequest("GET", - fmt.Sprintf("/v1/health/node/%s?dc=dc1", srv.agent.config.NodeName), nil) + req, err := http.NewRequest("GET", "/v1/health/node/nope?dc=dc1", nil) if err != nil { t.Fatalf("err: %v", err) } @@ -143,8 +167,27 @@ func TestHealthNodeChecks(t *testing.T) { } assertIndex(t, resp) - // Should be 1 health check for the server + // Should be a non-nil empty list nodes := obj.(structs.HealthChecks) + if nodes == nil || len(nodes) != 0 { + t.Fatalf("bad: %v", obj) + } + + req, err = http.NewRequest("GET", + fmt.Sprintf("/v1/health/node/%s?dc=dc1", srv.agent.config.NodeName), nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp = httptest.NewRecorder() + obj, err = srv.HealthNodeChecks(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + assertIndex(t, resp) + + // Should be 1 health check for the server + nodes = obj.(structs.HealthChecks) if len(nodes) != 1 { t.Fatalf("bad: %v", obj) } @@ -158,6 +201,24 @@ func TestHealthServiceChecks(t *testing.T) { testutil.WaitForLeader(t, srv.agent.RPC, "dc1") + req, err := http.NewRequest("GET", "/v1/health/checks/consul?dc=dc1", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp := httptest.NewRecorder() + obj, err := srv.HealthServiceChecks(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + assertIndex(t, resp) + + // Should be a non-nil empty list + nodes := obj.(structs.HealthChecks) + if nodes == nil || len(nodes) != 0 { + t.Fatalf("bad: %v", obj) + } + // Create a service check args := &structs.RegisterRequest{ Datacenter: "dc1", @@ -171,24 +232,24 @@ func TestHealthServiceChecks(t *testing.T) { } var out struct{} - if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil { + if err = srv.agent.RPC("Catalog.Register", args, &out); err != nil { t.Fatalf("err: %v", err) } - req, err := http.NewRequest("GET", "/v1/health/checks/consul?dc=dc1", nil) + req, err = http.NewRequest("GET", "/v1/health/checks/consul?dc=dc1", nil) if err != nil { t.Fatalf("err: %v", err) } - resp := httptest.NewRecorder() - obj, err := srv.HealthServiceChecks(resp, req) + resp = httptest.NewRecorder() + obj, err = srv.HealthServiceChecks(resp, req) if err != nil { t.Fatalf("err: %v", err) } assertIndex(t, resp) // Should be 1 health check for consul - nodes := obj.(structs.HealthChecks) + nodes = obj.(structs.HealthChecks) if len(nodes) != 1 { t.Fatalf("bad: %v", obj) } @@ -306,6 +367,59 @@ func TestHealthServiceNodes(t *testing.T) { if len(nodes) != 1 { t.Fatalf("bad: %v", obj) } + + req, err = http.NewRequest("GET", "/v1/health/service/nope?dc=dc1", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp = httptest.NewRecorder() + obj, err = srv.HealthServiceNodes(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + + assertIndex(t, resp) + + // Should be a non-nil empty list + nodes = obj.(structs.CheckServiceNodes) + if nodes == nil || len(nodes) != 0 { + t.Fatalf("bad: %v", obj) + } + + args := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "bar", + Address: "127.0.0.1", + Service: &structs.NodeService{ + ID: "test", + Service: "test", + }, + } + + var out struct{} + if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil { + t.Fatalf("err: %v", err) + } + + req, err = http.NewRequest("GET", "/v1/health/service/test?dc=dc1", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp = httptest.NewRecorder() + obj, err = srv.HealthServiceNodes(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + + assertIndex(t, resp) + + // Should be a non-nil empty list for checks + nodes = obj.(structs.CheckServiceNodes) + if len(nodes) != 1 || nodes[0].Checks == nil || len(nodes[0].Checks) != 0 { + t.Fatalf("bad: %v", obj) + } } func TestHealthServiceNodes_DistanceSort(t *testing.T) { diff --git a/command/agent/session_endpoint.go b/command/agent/session_endpoint.go index f3d8db22bc..0f2685465f 100644 --- a/command/agent/session_endpoint.go +++ b/command/agent/session_endpoint.go @@ -185,6 +185,11 @@ func (s *HTTPServer) SessionGet(resp http.ResponseWriter, req *http.Request) (in if err := s.agent.RPC("Session.Get", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.Sessions == nil { + out.Sessions = make(structs.Sessions, 0) + } return out.Sessions, nil } @@ -200,6 +205,11 @@ func (s *HTTPServer) SessionList(resp http.ResponseWriter, req *http.Request) (i if err := s.agent.RPC("Session.List", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.Sessions == nil { + out.Sessions = make(structs.Sessions, 0) + } return out.Sessions, nil } @@ -223,5 +233,10 @@ func (s *HTTPServer) SessionsForNode(resp http.ResponseWriter, req *http.Request if err := s.agent.RPC("Session.NodeSessions", &args, &out); err != nil { return nil, err } + + // Use empty list instead of nil + if out.Sessions == nil { + out.Sessions = make(structs.Sessions, 0) + } return out.Sessions, nil } diff --git a/command/agent/session_endpoint_test.go b/command/agent/session_endpoint_test.go index 960fdeec18..c7f744c84a 100644 --- a/command/agent/session_endpoint_test.go +++ b/command/agent/session_endpoint_test.go @@ -349,6 +349,22 @@ func TestSessionTTLRenew(t *testing.T) { } func TestSessionGet(t *testing.T) { + httpTest(t, func(srv *HTTPServer) { + req, err := http.NewRequest("GET", "/v1/session/info/adf4238a-882b-9ddc-4a9d-5b6758e4159e", nil) + resp := httptest.NewRecorder() + obj, err := srv.SessionGet(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + respObj, ok := obj.(structs.Sessions) + if !ok { + t.Fatalf("should work") + } + if respObj == nil || len(respObj) != 0 { + t.Fatalf("bad: %v", respObj) + } + }) + httpTest(t, func(srv *HTTPServer) { id := makeTestSession(t, srv) @@ -370,6 +386,22 @@ func TestSessionGet(t *testing.T) { } func TestSessionList(t *testing.T) { + httpTest(t, func(srv *HTTPServer) { + req, err := http.NewRequest("GET", "/v1/session/list", nil) + resp := httptest.NewRecorder() + obj, err := srv.SessionList(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + respObj, ok := obj.(structs.Sessions) + if !ok { + t.Fatalf("should work") + } + if respObj == nil || len(respObj) != 0 { + t.Fatalf("bad: %v", respObj) + } + }) + httpTest(t, func(srv *HTTPServer) { var ids []string for i := 0; i < 10; i++ { @@ -393,6 +425,23 @@ func TestSessionList(t *testing.T) { } func TestSessionsForNode(t *testing.T) { + httpTest(t, func(srv *HTTPServer) { + req, err := http.NewRequest("GET", + "/v1/session/node/"+srv.agent.config.NodeName, nil) + resp := httptest.NewRecorder() + obj, err := srv.SessionsForNode(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + respObj, ok := obj.(structs.Sessions) + if !ok { + t.Fatalf("should work") + } + if respObj == nil || len(respObj) != 0 { + t.Fatalf("bad: %v", respObj) + } + }) + httpTest(t, func(srv *HTTPServer) { var ids []string for i := 0; i < 10; i++ { diff --git a/command/agent/ui_endpoint.go b/command/agent/ui_endpoint.go index 425e392ce9..c240879876 100644 --- a/command/agent/ui_endpoint.go +++ b/command/agent/ui_endpoint.go @@ -38,6 +38,19 @@ RPC: } return nil, err } + + // Use empty list instead of nil + for _, info := range out.Dump { + if info.Services == nil { + info.Services = make([]*structs.NodeService, 0) + } + if info.Checks == nil { + info.Checks = make([]*structs.HealthCheck, 0) + } + } + if out.Dump == nil { + out.Dump = make(structs.NodeDump, 0) + } return out.Dump, nil } @@ -73,7 +86,14 @@ RPC: // Return only the first entry if len(out.Dump) > 0 { - return out.Dump[0], nil + info := out.Dump[0] + if info.Services == nil { + info.Services = make([]*structs.NodeService, 0) + } + if info.Checks == nil { + info.Checks = make([]*structs.HealthCheck, 0) + } + return info, nil } return nil, nil } diff --git a/command/agent/ui_endpoint_test.go b/command/agent/ui_endpoint_test.go index 76e3f379ba..c37d7c0214 100644 --- a/command/agent/ui_endpoint_test.go +++ b/command/agent/ui_endpoint_test.go @@ -65,6 +65,17 @@ func TestUiNodes(t *testing.T) { testutil.WaitForLeader(t, srv.agent.RPC, "dc1") + args := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "test", + Address: "127.0.0.1", + } + + var out struct{} + if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil { + t.Fatalf("err: %v", err) + } + req, err := http.NewRequest("GET", "/v1/internal/ui/nodes/dc1", nil) if err != nil { t.Fatalf("err: %v", err) @@ -77,9 +88,15 @@ func TestUiNodes(t *testing.T) { } assertIndex(t, resp) - // Should be 1 node for the server + // Should be 2 nodes, and all the empty lists should be non-nil nodes := obj.(structs.NodeDump) - if len(nodes) != 1 { + if len(nodes) != 2 || + nodes[0].Node != srv.agent.config.NodeName || + nodes[0].Services == nil || len(nodes[0].Services) != 1 || + nodes[0].Checks == nil || len(nodes[0].Checks) != 1 || + nodes[1].Node != "test" || + nodes[1].Services == nil || len(nodes[1].Services) != 0 || + nodes[1].Checks == nil || len(nodes[1].Checks) != 0 { t.Fatalf("bad: %v", obj) } } @@ -111,6 +128,38 @@ func TestUiNodeInfo(t *testing.T) { if node.Node != srv.agent.config.NodeName { t.Fatalf("bad: %v", node) } + + args := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "test", + Address: "127.0.0.1", + } + + var out struct{} + if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil { + t.Fatalf("err: %v", err) + } + + req, err = http.NewRequest("GET", "/v1/internal/ui/node/test", nil) + if err != nil { + t.Fatalf("err: %v", err) + } + + resp = httptest.NewRecorder() + obj, err = srv.UINodeInfo(resp, req) + if err != nil { + t.Fatalf("err: %v", err) + } + + assertIndex(t, resp) + + // Should be non-nil empty lists for services and checks + node = obj.(*structs.NodeInfo) + if node.Node != "test" || + node.Services == nil || len(node.Services) != 0 || + node.Checks == nil || len(node.Checks) != 0 { + t.Fatalf("bad: %v", node) + } } func TestSummarizeServices(t *testing.T) {