From 763d5ea8a70404059864e4a3685ca208e02e7b41 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 72ae4c20a..3bc054611 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 b0dea0c9b..361d661ac 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 447822a87..3a5bd9813 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 5e4d75aab..251d288df 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 0cc5824ce..92453225d 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 aee7bc18c..8da0a5b39 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 f95e4c5e7..e4a36f6a6 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 317203065..02330a37c 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 f3d8db22b..0f2685465 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 960fdeec1..c7f744c84 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 425e392ce..c24087987 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 76e3f379b..c37d7c021 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) {