From fea07f272b69561065549652bfc25af6f6316d44 Mon Sep 17 00:00:00 2001 From: Rebecca Zanzig Date: Mon, 24 Sep 2018 15:53:52 -0700 Subject: [PATCH 1/4] Support multiple tags for health and catalog api endpoints Fixes #1781. Adds a `ServiceTags` field to the ServiceSpecificRequest to support multiple tags, updates the filter logic in the catalog store, and propagates these change through to the health and catalog endpoints. Note: Leaves `ServiceTag` in the struct, since it is being used as part of the DNS lookup, which in turn uses the health check. --- agent/catalog_endpoint.go | 2 +- agent/consul/catalog_endpoint.go | 10 +++- agent/consul/catalog_endpoint_test.go | 33 ++++++++---- agent/consul/health_endpoint.go | 14 ++++- agent/consul/health_endpoint_test.go | 78 +++++++++++++++++++++++++++ agent/consul/state/catalog.go | 24 +++++++-- agent/consul/state/catalog_test.go | 36 ++++++++++--- agent/health_endpoint.go | 4 +- agent/structs/structs.go | 1 + 9 files changed, 177 insertions(+), 25 deletions(-) diff --git a/agent/catalog_endpoint.go b/agent/catalog_endpoint.go index 10dc555e45ac..7381cdf6420c 100644 --- a/agent/catalog_endpoint.go +++ b/agent/catalog_endpoint.go @@ -188,7 +188,7 @@ func (s *HTTPServer) catalogServiceNodes(resp http.ResponseWriter, req *http.Req // Check for a tag params := req.URL.Query() if _, ok := params["tag"]; ok { - args.ServiceTag = params.Get("tag") + args.ServiceTags = params["tag"] args.TagFilter = true } diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 6cebd4500578..47faeffb8638 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -273,7 +273,7 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru } if args.TagFilter { - return s.ServiceTagNodes(ws, args.ServiceName, args.ServiceTag) + return s.ServiceTagNodes(ws, args.ServiceName, args.ServiceTags) } return s.ServiceNodes(ws, args.ServiceName) @@ -334,6 +334,14 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru metrics.IncrCounterWithLabels([]string{"catalog", key, "query-tag"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}, {Name: "tag", Value: args.ServiceTag}}) } + if len(args.ServiceTags) > 0 { + // Build metric labels + labels := []metrics.Label{{Name: "service", Value: args.ServiceName}} + for _, tag := range args.ServiceTags { + labels = append(labels, metrics.Label{Name: "tag", Value: tag}) + } + metrics.IncrCounterWithLabels([]string{"catalog", key, "query-tags"}, 1, labels) + } if len(reply.ServiceNodes) == 0 { metrics.IncrCounterWithLabels([]string{"catalog", key, "not-found"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}}) diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 79560ce808e0..2fa87fa604f9 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -1589,7 +1589,7 @@ func TestCatalog_ListServiceNodes(t *testing.T) { args := structs.ServiceSpecificRequest{ Datacenter: "dc1", ServiceName: "db", - ServiceTag: "slave", + ServiceTags: []string{"slave"}, TagFilter: false, } var out structs.IndexedServiceNodes @@ -1647,16 +1647,16 @@ func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) { if err := s1.fsm.State().EnsureNode(2, node2); err != nil { t.Fatalf("err: %v", err) } - if err := s1.fsm.State().EnsureService(3, "foo", &structs.NodeService{ID: "db", Service: "db", Tags: []string{"primary"}, Address: "127.0.0.1", Port: 5000}); err != nil { + if err := s1.fsm.State().EnsureService(3, "foo", &structs.NodeService{ID: "db", Service: "db", Tags: []string{"primary", "v2"}, Address: "127.0.0.1", Port: 5000}); err != nil { t.Fatalf("err: %v", err) } - if err := s1.fsm.State().EnsureService(4, "bar", &structs.NodeService{ID: "db2", Service: "db", Tags: []string{"secondary"}, Address: "127.0.0.2", Port: 5000}); err != nil { + if err := s1.fsm.State().EnsureService(4, "bar", &structs.NodeService{ID: "db2", Service: "db", Tags: []string{"secondary", "v2"}, Address: "127.0.0.2", Port: 5000}); err != nil { t.Fatalf("err: %v", err) } cases := []struct { filters map[string]string - tag string + tags []string services structs.ServiceNodes }{ // Basic meta filter @@ -1667,7 +1667,7 @@ func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) { // Basic meta filter, tag { filters: map[string]string{"somekey": "somevalue"}, - tag: "primary", + tags: []string{"primary"}, services: structs.ServiceNodes{&structs.ServiceNode{Node: "foo", ServiceID: "db"}}, }, // Common meta filter @@ -1681,7 +1681,7 @@ func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) { // Common meta filter, tag { filters: map[string]string{"common": "1"}, - tag: "secondary", + tags: []string{"secondary"}, services: structs.ServiceNodes{ &structs.ServiceNode{Node: "bar", ServiceID: "db2"}, }, @@ -1699,7 +1699,22 @@ func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) { // Multiple filter values, tag { filters: map[string]string{"somekey": "somevalue", "common": "1"}, - tag: "primary", + tags: []string{"primary"}, + services: structs.ServiceNodes{&structs.ServiceNode{Node: "foo", ServiceID: "db"}}, + }, + // Common meta filter, single tag + { + filters: map[string]string{"common": "1"}, + tags: []string{"v2"}, + services: structs.ServiceNodes{ + &structs.ServiceNode{Node: "bar", ServiceID: "db2"}, + &structs.ServiceNode{Node: "foo", ServiceID: "db"}, + }, + }, + // Common meta filter, multiple tags + { + filters: map[string]string{"common": "1"}, + tags: []string{"v2", "primary"}, services: structs.ServiceNodes{&structs.ServiceNode{Node: "foo", ServiceID: "db"}}, }, } @@ -1709,8 +1724,8 @@ func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) { Datacenter: "dc1", NodeMetaFilters: tc.filters, ServiceName: "db", - ServiceTag: tc.tag, - TagFilter: tc.tag != "", + ServiceTags: tc.tags, + TagFilter: len(tc.tags) > 0, } var out structs.IndexedServiceNodes if err := msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out); err != nil { diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index 38b7a9c0a538..e82f3e8e2b20 100644 --- a/agent/consul/health_endpoint.go +++ b/agent/consul/health_endpoint.go @@ -170,6 +170,13 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc metrics.IncrCounterWithLabels([]string{"health", key, "query-tag"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}, {Name: "tag", Value: args.ServiceTag}}) } + if len(args.ServiceTags) > 0 { + labels := []metrics.Label{{Name: "service", Value: args.ServiceName}} + for _, tag := range args.ServiceTags { + labels = append(labels, metrics.Label{Name: "tag", Value: tag}) + } + metrics.IncrCounterWithLabels([]string{"health", key, "query-tags"}, 1, labels) + } if len(reply.Nodes) == 0 { metrics.IncrCounterWithLabels([]string{"health", key, "not-found"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}}) @@ -186,7 +193,12 @@ func (h *Health) serviceNodesConnect(ws memdb.WatchSet, s *state.Store, args *st } func (h *Health) serviceNodesTagFilter(ws memdb.WatchSet, s *state.Store, args *structs.ServiceSpecificRequest) (uint64, structs.CheckServiceNodes, error) { - return s.CheckServiceTagNodes(ws, args.ServiceName, args.ServiceTag) + // DNS service lookups populate the ServiceTag field. In this case, + // use ServiceTag instead of the ServiceTags field. + if args.ServiceTag != "" { + return s.CheckServiceTagNodes(ws, args.ServiceName, []string{args.ServiceTag}) + } + return s.CheckServiceTagNodes(ws, args.ServiceName, args.ServiceTags) } func (h *Health) serviceNodesDefault(ws memdb.WatchSet, s *state.Store, args *structs.ServiceSpecificRequest) (uint64, structs.CheckServiceNodes, error) { diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index 0524617c6f0d..e62fbb1f5a0e 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -602,6 +602,84 @@ func TestHealth_ServiceNodes(t *testing.T) { } } +func TestHealth_ServiceNodes_MultipleServiceTags(t *testing.T) { + t.Parallel() + dir1, s1 := testServer(t) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + arg := structs.RegisterRequest{ + Datacenter: "dc1", + Node: "foo", + Address: "127.0.0.1", + Service: &structs.NodeService{ + ID: "db", + Service: "db", + Tags: []string{"master", "v2"}, + }, + Check: &structs.HealthCheck{ + Name: "db connect", + Status: api.HealthPassing, + ServiceID: "db", + }, + } + var out struct{} + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + + arg = structs.RegisterRequest{ + Datacenter: "dc1", + Node: "bar", + Address: "127.0.0.2", + Service: &structs.NodeService{ + ID: "db", + Service: "db", + Tags: []string{"slave", "v2"}, + }, + Check: &structs.HealthCheck{ + Name: "db connect", + Status: api.HealthWarning, + ServiceID: "db", + }, + } + if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out); err != nil { + t.Fatalf("err: %v", err) + } + + var out2 structs.IndexedCheckServiceNodes + req := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "db", + ServiceTags: []string{"master", "v2"}, + TagFilter: true, + } + if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2); err != nil { + t.Fatalf("err: %v", err) + } + + nodes := out2.Nodes + if len(nodes) != 1 { + t.Fatalf("Bad: %v", nodes) + } + if nodes[0].Node.Node != "foo" { + t.Fatalf("Bad: %v", nodes[0]) + } + if !lib.StrContains(nodes[0].Service.Tags, "v2") { + t.Fatalf("Bad: %v", nodes[0]) + } + if !lib.StrContains(nodes[0].Service.Tags, "master") { + t.Fatalf("Bad: %v", nodes[0]) + } + if nodes[0].Checks[0].Status != api.HealthPassing { + t.Fatalf("Bad: %v", nodes[0]) + } +} + func TestHealth_ServiceNodes_NodeMetaFilter(t *testing.T) { t.Parallel() dir1, s1 := testServer(t) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index e4685bc20b1d..d551a47e6b45 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -897,8 +897,8 @@ func (s *Store) serviceNodes(ws memdb.WatchSet, serviceName string, connect bool } // ServiceTagNodes returns the nodes associated with a given service, filtering -// out services that don't contain the given tag. -func (s *Store) ServiceTagNodes(ws memdb.WatchSet, service string, tag string) (uint64, structs.ServiceNodes, error) { +// out services that don't contain the given tags. +func (s *Store) ServiceTagNodes(ws memdb.WatchSet, service string, tags []string) (uint64, structs.ServiceNodes, error) { tx := s.db.Txn(false) defer tx.Abort() @@ -916,7 +916,7 @@ func (s *Store) ServiceTagNodes(ws memdb.WatchSet, service string, tag string) ( var results structs.ServiceNodes for service := services.Next(); service != nil; service = services.Next() { svc := service.(*structs.ServiceNode) - if !serviceTagFilter(svc, tag) { + if !serviceTagsFilter(svc, tags) { results = append(results, svc) } } @@ -945,6 +945,20 @@ func serviceTagFilter(sn *structs.ServiceNode, tag string) bool { return true } +// serviceTagsFilter returns true (should filter) if the given service node +// doesn't contain the given set of tags. +func serviceTagsFilter(sn *structs.ServiceNode, tags []string) bool { + for _, tag := range tags { + if serviceTagFilter(sn, tag) { + // If any one of the expected tags was not found, filter the service + return true + } + } + + // If all tags were found, don't filter the service + return false +} + // ServiceAddressNodes returns the nodes associated with a given service, filtering // out services that don't match the given serviceAddress func (s *Store) ServiceAddressNodes(ws memdb.WatchSet, address string) (uint64, structs.ServiceNodes, error) { @@ -1614,7 +1628,7 @@ func (s *Store) checkServiceNodes(ws memdb.WatchSet, serviceName string, connect // CheckServiceTagNodes is used to query all nodes and checks for a given // service, filtering out services that don't contain the given tag. -func (s *Store) CheckServiceTagNodes(ws memdb.WatchSet, serviceName, tag string) (uint64, structs.CheckServiceNodes, error) { +func (s *Store) CheckServiceTagNodes(ws memdb.WatchSet, serviceName string, tags []string) (uint64, structs.CheckServiceNodes, error) { tx := s.db.Txn(false) defer tx.Abort() @@ -1632,7 +1646,7 @@ func (s *Store) CheckServiceTagNodes(ws memdb.WatchSet, serviceName, tag string) var results structs.ServiceNodes for service := iter.Next(); service != nil; service = iter.Next() { svc := service.(*structs.ServiceNode) - if !serviceTagFilter(svc, tag) { + if !serviceTagsFilter(svc, tags) { results = append(results, svc) } } diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 61d8d55b747e..f4f49da98792 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -1809,7 +1809,7 @@ func TestStateStore_ServiceTagNodes(t *testing.T) { // Listing with no results returns an empty list. ws := memdb.NewWatchSet() - idx, nodes, err := s.ServiceTagNodes(ws, "db", "master") + idx, nodes, err := s.ServiceTagNodes(ws, "db", []string{"master"}) if err != nil { t.Fatalf("err: %s", err) } @@ -1842,7 +1842,7 @@ func TestStateStore_ServiceTagNodes(t *testing.T) { // Read everything back. ws = memdb.NewWatchSet() - idx, nodes, err = s.ServiceTagNodes(ws, "db", "master") + idx, nodes, err = s.ServiceTagNodes(ws, "db", []string{"master"}) if err != nil { t.Fatalf("err: %s", err) } @@ -1903,7 +1903,7 @@ func TestStateStore_ServiceTagNodes_MultipleTags(t *testing.T) { t.Fatalf("err: %v", err) } - idx, nodes, err := s.ServiceTagNodes(nil, "db", "master") + idx, nodes, err := s.ServiceTagNodes(nil, "db", []string{"master"}) if err != nil { t.Fatalf("err: %s", err) } @@ -1926,7 +1926,7 @@ func TestStateStore_ServiceTagNodes_MultipleTags(t *testing.T) { t.Fatalf("bad: %v", nodes) } - idx, nodes, err = s.ServiceTagNodes(nil, "db", "v2") + idx, nodes, err = s.ServiceTagNodes(nil, "db", []string{"v2"}) if err != nil { t.Fatalf("err: %s", err) } @@ -1937,7 +1937,31 @@ func TestStateStore_ServiceTagNodes_MultipleTags(t *testing.T) { t.Fatalf("bad: %v", nodes) } - idx, nodes, err = s.ServiceTagNodes(nil, "db", "dev") + // Test filtering on multiple tags + idx, nodes, err = s.ServiceTagNodes(nil, "db", []string{"v2", "slave"}) + if err != nil { + t.Fatalf("err: %s", err) + } + if idx != 19 { + t.Fatalf("bad: %v", idx) + } + if len(nodes) != 2 { + t.Fatalf("bad: %v", nodes) + } + if !lib.StrContains(nodes[0].ServiceTags, "v2") { + t.Fatalf("bad: %v", nodes) + } + if !lib.StrContains(nodes[0].ServiceTags, "slave") { + t.Fatalf("bad: %v", nodes) + } + if !lib.StrContains(nodes[1].ServiceTags, "v2") { + t.Fatalf("bad: %v", nodes) + } + if !lib.StrContains(nodes[1].ServiceTags, "slave") { + t.Fatalf("bad: %v", nodes) + } + + idx, nodes, err = s.ServiceTagNodes(nil, "db", []string{"dev"}) if err != nil { t.Fatalf("err: %s", err) } @@ -3088,7 +3112,7 @@ func TestStateStore_CheckServiceTagNodes(t *testing.T) { } ws := memdb.NewWatchSet() - idx, nodes, err := s.CheckServiceTagNodes(ws, "db", "master") + idx, nodes, err := s.CheckServiceTagNodes(ws, "db", []string{"master"}) if err != nil { t.Fatalf("err: %s", err) } diff --git a/agent/health_endpoint.go b/agent/health_endpoint.go index f87b89261efb..5348eac44f0b 100644 --- a/agent/health_endpoint.go +++ b/agent/health_endpoint.go @@ -161,10 +161,10 @@ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Requ return nil, nil } - // Check for a tag + // Check for tags params := req.URL.Query() if _, ok := params["tag"]; ok { - args.ServiceTag = params.Get("tag") + args.ServiceTags = params["tag"] args.TagFilter = true } diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 00fb21047054..919bbd8580f1 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -347,6 +347,7 @@ type ServiceSpecificRequest struct { NodeMetaFilters map[string]string ServiceName string ServiceTag string + ServiceTags []string ServiceAddress string TagFilter bool // Controls tag filtering Source QuerySource From 1977293d7ebd82976332b6528d3f0df352fe1697 Mon Sep 17 00:00:00 2001 From: Rebecca Zanzig Date: Thu, 27 Sep 2018 17:55:58 -0700 Subject: [PATCH 2/4] Update the api package to support multiple tags Includes additional tests. --- api/catalog.go | 30 ++++++++-- api/catalog_test.go | 81 +++++++++++++++++++++++++- api/health.go | 28 +++++++-- api/health_test.go | 135 ++++++++++++++++++++++++++++++++------------ 4 files changed, 226 insertions(+), 48 deletions(-) diff --git a/api/catalog.go b/api/catalog.go index a311a59d4c3e..3ca89a472a76 100644 --- a/api/catalog.go +++ b/api/catalog.go @@ -165,23 +165,43 @@ func (c *Catalog) Services(q *QueryOptions) (map[string][]string, *QueryMeta, er // Service is used to query catalog entries for a given service func (c *Catalog) Service(service, tag string, q *QueryOptions) ([]*CatalogService, *QueryMeta, error) { - return c.service(service, tag, q, false) + var tags []string + if tag != "" { + tags = []string{tag} + } + return c.service(service, tags, q, false) +} + +// Supports multiple tags for filtering +func (c *Catalog) ServiceMultipleTags(service string, tags []string, q *QueryOptions) ([]*CatalogService, *QueryMeta, error) { + return c.service(service, tags, q, false) } // Connect is used to query catalog entries for a given Connect-enabled service func (c *Catalog) Connect(service, tag string, q *QueryOptions) ([]*CatalogService, *QueryMeta, error) { - return c.service(service, tag, q, true) + var tags []string + if tag != "" { + tags = []string{tag} + } + return c.service(service, tags, q, true) +} + +// Supports multiple tags for filtering +func (c *Catalog) ConnectMultipleTags(service string, tags []string, q *QueryOptions) ([]*CatalogService, *QueryMeta, error) { + return c.service(service, tags, q, true) } -func (c *Catalog) service(service, tag string, q *QueryOptions, connect bool) ([]*CatalogService, *QueryMeta, error) { +func (c *Catalog) service(service string, tags []string, q *QueryOptions, connect bool) ([]*CatalogService, *QueryMeta, error) { path := "/v1/catalog/service/" + service if connect { path = "/v1/catalog/connect/" + service } r := c.c.newRequest("GET", path) r.setQueryOptions(q) - if tag != "" { - r.params.Set("tag", tag) + if len(tags) > 0 { + for _, tag := range tags { + r.params.Add("tag", tag) + } } rtt, resp, err := requireOK(c.c.doRequest(r)) if err != nil { diff --git a/api/catalog_test.go b/api/catalog_test.go index 9f68786acf02..f583f9ec3954 100644 --- a/api/catalog_test.go +++ b/api/catalog_test.go @@ -5,11 +5,10 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" - "github.com/hashicorp/consul/testutil" "github.com/hashicorp/consul/testutil/retry" "github.com/pascaldekloe/goe/verify" + "github.com/stretchr/testify/require" ) func TestAPI_CatalogDatacenters(t *testing.T) { @@ -295,6 +294,84 @@ func TestAPI_CatalogServiceCached(t *testing.T) { require.Equal(time.Duration(0), meta.CacheAge) } +func TestAPI_CatalogService_SingleTag(t *testing.T) { + t.Parallel() + c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { + conf.NodeName = "node123" + }) + defer s.Stop() + + agent := c.Agent() + catalog := c.Catalog() + + reg := &AgentServiceRegistration{ + Name: "foo", + ID: "foo1", + Tags: []string{"bar"}, + } + require.NoError(t, agent.ServiceRegister(reg)) + defer agent.ServiceDeregister("foo1") + + retry.Run(t, func(r *retry.R) { + services, meta, err := catalog.Service("foo", "bar", nil) + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + require.Len(t, services, 1) + require.Equal(t, services[0].ServiceID, "foo1") + }) +} + +func TestAPI_CatalogService_MultipleTags(t *testing.T) { + t.Parallel() + c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { + conf.NodeName = "node123" + }) + defer s.Stop() + + agent := c.Agent() + catalog := c.Catalog() + + // Make two services with a check + reg := &AgentServiceRegistration{ + Name: "foo", + ID: "foo1", + Tags: []string{"bar"}, + } + require.NoError(t, agent.ServiceRegister(reg)) + defer agent.ServiceDeregister("foo1") + + reg2 := &AgentServiceRegistration{ + Name: "foo", + ID: "foo2", + Tags: []string{"bar", "v2"}, + } + require.NoError(t, agent.ServiceRegister(reg2)) + defer agent.ServiceDeregister("foo2") + + // Test searching with one tag (two results) + retry.Run(t, func(r *retry.R) { + services, meta, err := catalog.ServiceMultipleTags("foo", []string{"bar"}, nil) + + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + + // Should be 2 services with the `bar` tag + require.Len(t, services, 2) + }) + + // Test searching with two tags (one result) + retry.Run(t, func(r *retry.R) { + services, meta, err := catalog.ServiceMultipleTags("foo", []string{"bar", "v2"}, nil) + + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + + // Should be exactly 1 service, named "foo2" + require.Len(t, services, 1) + require.Equal(t, services[0].ServiceID, "foo2") + }) +} + func TestAPI_CatalogService_NodeMetaFilter(t *testing.T) { t.Parallel() meta := map[string]string{"somekey": "somevalue"} diff --git a/api/health.go b/api/health.go index 1835da559f08..eae6a01a8682 100644 --- a/api/health.go +++ b/api/health.go @@ -159,7 +159,15 @@ func (h *Health) Checks(service string, q *QueryOptions) (HealthChecks, *QueryMe // for a given service. It can optionally do server-side filtering on a tag // or nodes with passing health checks only. func (h *Health) Service(service, tag string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { - return h.service(service, tag, passingOnly, q, false) + var tags []string + if tag != "" { + tags = []string{tag} + } + return h.service(service, tags, passingOnly, q, false) +} + +func (h *Health) ServiceMultipleTags(service string, tags []string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { + return h.service(service, tags, passingOnly, q, false) } // Connect is equivalent to Service except that it will only return services @@ -168,18 +176,28 @@ func (h *Health) Service(service, tag string, passingOnly bool, q *QueryOptions) // passingOnly is true only instances where both the service and any proxy are // healthy will be returned. func (h *Health) Connect(service, tag string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { - return h.service(service, tag, passingOnly, q, true) + var tags []string + if tag != "" { + tags = []string{tag} + } + return h.service(service, tags, passingOnly, q, true) +} + +func (h *Health) ConnectMultipleTags(service string, tags []string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { + return h.service(service, tags, passingOnly, q, true) } -func (h *Health) service(service, tag string, passingOnly bool, q *QueryOptions, connect bool) ([]*ServiceEntry, *QueryMeta, error) { +func (h *Health) service(service string, tags []string, passingOnly bool, q *QueryOptions, connect bool) ([]*ServiceEntry, *QueryMeta, error) { path := "/v1/health/service/" + service if connect { path = "/v1/health/connect/" + service } r := h.c.newRequest("GET", path) r.setQueryOptions(q) - if tag != "" { - r.params.Set("tag", tag) + if len(tags) > 0 { + for _, tag := range tags { + r.params.Add("tag", tag) + } } if passingOnly { r.params.Set(HealthPassing, "1") diff --git a/api/health_test.go b/api/health_test.go index 18974acdc498..1ecd3cfc85dd 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -283,6 +283,101 @@ func TestAPI_HealthService(t *testing.T) { }) } +func TestAPI_HealthService_SingleTag(t *testing.T) { + t.Parallel() + c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { + conf.NodeName = "node123" + }) + defer s.Stop() + agent := c.Agent() + health := c.Health() + reg := &AgentServiceRegistration{ + Name: "foo", + ID: "foo1", + Tags: []string{"bar"}, + Check: &AgentServiceCheck{ + Status: HealthPassing, + TTL: "15s", + }, + } + require.NoError(t, agent.ServiceRegister(reg)) + defer agent.ServiceDeregister("foo1") + retry.Run(t, func(r *retry.R) { + services, meta, err := health.Service("foo", "bar", true, nil) + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + require.Len(t, services, 1) + require.Equal(t, services[0].Service.ID, "foo1") + }) +} +func TestAPI_HealthService_MultipleTags(t *testing.T) { + t.Parallel() + c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { + conf.NodeName = "node123" + }) + defer s.Stop() + agent := c.Agent() + health := c.Health() + // Make two services with a check + reg := &AgentServiceRegistration{ + Name: "foo", + ID: "foo1", + Tags: []string{"bar"}, + Check: &AgentServiceCheck{ + Status: HealthPassing, + TTL: "15s", + }, + } + require.NoError(t, agent.ServiceRegister(reg)) + defer agent.ServiceDeregister("foo1") + reg2 := &AgentServiceRegistration{ + Name: "foo", + ID: "foo2", + Tags: []string{"bar", "v2"}, + Check: &AgentServiceCheck{ + Status: HealthPassing, + TTL: "15s", + }, + } + require.NoError(t, agent.ServiceRegister(reg2)) + defer agent.ServiceDeregister("foo2") + // Test searching with one tag (two results) + retry.Run(t, func(r *retry.R) { + services, meta, err := health.ServiceMultipleTags("foo", []string{"bar"}, true, nil) + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + require.Len(t, services, 2) + }) + // Test searching with two tags (one result) + retry.Run(t, func(r *retry.R) { + services, meta, err := health.ServiceMultipleTags("foo", []string{"bar", "v2"}, true, nil) + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + require.Len(t, services, 1) + require.Equal(t, services[0].Service.ID, "foo2") + }) +} + +func TestAPI_HealthService_NodeMetaFilter(t *testing.T) { + t.Parallel() + meta := map[string]string{"somekey": "somevalue"} + c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { + conf.NodeMeta = meta + }) + defer s.Stop() + + health := c.Health() + retry.Run(t, func(r *retry.R) { + // consul service should always exist... + checks, meta, err := health.Service("consul", "", true, &QueryOptions{NodeMeta: meta}) + require.NoError(t, err) + require.NotEqual(t, meta.LastIndex, 0) + require.NotEqual(t, len(checks), 0) + require.Equal(t, checks[0].Node.Datacenter, "dc1") + require.Contains(t, checks[0].Node.TaggedAddresses, "wan") + }) +} + func TestAPI_HealthConnect(t *testing.T) { t.Parallel() c, s := makeClient(t) @@ -302,12 +397,10 @@ func TestAPI_HealthConnect(t *testing.T) { // Register the proxy proxyReg := &AgentServiceRegistration{ - Name: "foo-proxy", - Port: 8001, - Kind: ServiceKindConnectProxy, - Proxy: &AgentServiceConnectProxyConfig{ - DestinationServiceName: "foo", - }, + Name: "foo-proxy", + Port: 8001, + Kind: ServiceKindConnectProxy, + ProxyDestination: "foo", } err = agent.ServiceRegister(proxyReg) require.NoError(t, err) @@ -335,36 +428,6 @@ func TestAPI_HealthConnect(t *testing.T) { }) } -func TestAPI_HealthService_NodeMetaFilter(t *testing.T) { - t.Parallel() - meta := map[string]string{"somekey": "somevalue"} - c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { - conf.NodeMeta = meta - }) - defer s.Stop() - - health := c.Health() - retry.Run(t, func(r *retry.R) { - // consul service should always exist... - checks, meta, err := health.Service("consul", "", true, &QueryOptions{NodeMeta: meta}) - if err != nil { - r.Fatal(err) - } - if meta.LastIndex == 0 { - r.Fatalf("bad: %v", meta) - } - if len(checks) == 0 { - r.Fatalf("Bad: %v", checks) - } - if _, ok := checks[0].Node.TaggedAddresses["wan"]; !ok { - r.Fatalf("Bad: %v", checks[0].Node) - } - if checks[0].Node.Datacenter != "dc1" { - r.Fatalf("Bad datacenter: %v", checks[0].Node) - } - }) -} - func TestAPI_HealthState(t *testing.T) { t.Parallel() c, s := makeClient(t) From 98cdcaa55083eed4ee800c47a3988ddd28ca9bae Mon Sep 17 00:00:00 2001 From: Rebecca Zanzig Date: Sun, 7 Oct 2018 22:03:22 -0700 Subject: [PATCH 3/4] Update new tests to use the `require` library --- agent/consul/catalog_endpoint_test.go | 9 +-- agent/consul/health_endpoint_test.go | 33 +++------ agent/consul/state/catalog_test.go | 97 +++++++-------------------- api/health_test.go | 7 ++ 4 files changed, 43 insertions(+), 103 deletions(-) diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 2fa87fa604f9..d7569fa9961d 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -1728,13 +1728,8 @@ func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) { TagFilter: len(tc.tags) > 0, } var out structs.IndexedServiceNodes - if err := msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out); err != nil { - t.Fatalf("err: %v", err) - } - - if len(out.ServiceNodes) != len(tc.services) { - t.Fatalf("bad: %v", out) - } + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)) + require.Len(t, out.ServiceNodes, len(tc.services)) for i, serviceNode := range out.ServiceNodes { if serviceNode.Node != tc.services[i].Node || serviceNode.ServiceID != tc.services[i].ServiceID { diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index e62fbb1f5a0e..cbe1141f2e90 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestHealth_ChecksInState(t *testing.T) { @@ -628,9 +629,7 @@ func TestHealth_ServiceNodes_MultipleServiceTags(t *testing.T) { }, } var out struct{} - if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out)) arg = structs.RegisterRequest{ Datacenter: "dc1", @@ -647,9 +646,7 @@ func TestHealth_ServiceNodes_MultipleServiceTags(t *testing.T) { ServiceID: "db", }, } - if err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out)) var out2 structs.IndexedCheckServiceNodes req := structs.ServiceSpecificRequest{ @@ -658,26 +655,14 @@ func TestHealth_ServiceNodes_MultipleServiceTags(t *testing.T) { ServiceTags: []string{"master", "v2"}, TagFilter: true, } - if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2)) nodes := out2.Nodes - if len(nodes) != 1 { - t.Fatalf("Bad: %v", nodes) - } - if nodes[0].Node.Node != "foo" { - t.Fatalf("Bad: %v", nodes[0]) - } - if !lib.StrContains(nodes[0].Service.Tags, "v2") { - t.Fatalf("Bad: %v", nodes[0]) - } - if !lib.StrContains(nodes[0].Service.Tags, "master") { - t.Fatalf("Bad: %v", nodes[0]) - } - if nodes[0].Checks[0].Status != api.HealthPassing { - t.Fatalf("Bad: %v", nodes[0]) - } + require.Len(t, nodes, 1) + require.Equal(t, nodes[0].Node.Node, "foo") + require.Contains(t, nodes[0].Service.Tags, "v2") + require.Contains(t, nodes[0].Service.Tags, "master") + require.Equal(t, nodes[0].Checks[0].Status, api.HealthPassing) } func TestHealth_ServiceNodes_NodeMetaFilter(t *testing.T) { diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index f4f49da98792..f27b9c118e0a 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -15,6 +15,7 @@ import ( uuid "github.com/hashicorp/go-uuid" "github.com/pascaldekloe/goe/verify" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func makeRandomNodeID(t *testing.T) types.NodeID { @@ -1904,85 +1905,37 @@ func TestStateStore_ServiceTagNodes_MultipleTags(t *testing.T) { } idx, nodes, err := s.ServiceTagNodes(nil, "db", []string{"master"}) - if err != nil { - t.Fatalf("err: %s", err) - } - if idx != 19 { - t.Fatalf("bad: %v", idx) - } - if len(nodes) != 1 { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].Node != "foo" { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].Address != "127.0.0.1" { - t.Fatalf("bad: %v", nodes) - } - if !lib.StrContains(nodes[0].ServiceTags, "master") { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].ServicePort != 8000 { - t.Fatalf("bad: %v", nodes) - } + require.NoError(t, err) + require.Equal(t, int(idx), 19) + require.Len(t, nodes, 1) + require.Equal(t, nodes[0].Node, "foo") + require.Equal(t, nodes[0].Address, "127.0.0.1") + require.Contains(t, nodes[0].ServiceTags, "master") + require.Equal(t, nodes[0].ServicePort, 8000) idx, nodes, err = s.ServiceTagNodes(nil, "db", []string{"v2"}) - if err != nil { - t.Fatalf("err: %s", err) - } - if idx != 19 { - t.Fatalf("bad: %v", idx) - } - if len(nodes) != 3 { - t.Fatalf("bad: %v", nodes) - } + require.NoError(t, err) + require.Equal(t, int(idx), 19) + require.Len(t, nodes, 3) // Test filtering on multiple tags idx, nodes, err = s.ServiceTagNodes(nil, "db", []string{"v2", "slave"}) - if err != nil { - t.Fatalf("err: %s", err) - } - if idx != 19 { - t.Fatalf("bad: %v", idx) - } - if len(nodes) != 2 { - t.Fatalf("bad: %v", nodes) - } - if !lib.StrContains(nodes[0].ServiceTags, "v2") { - t.Fatalf("bad: %v", nodes) - } - if !lib.StrContains(nodes[0].ServiceTags, "slave") { - t.Fatalf("bad: %v", nodes) - } - if !lib.StrContains(nodes[1].ServiceTags, "v2") { - t.Fatalf("bad: %v", nodes) - } - if !lib.StrContains(nodes[1].ServiceTags, "slave") { - t.Fatalf("bad: %v", nodes) - } + require.NoError(t, err) + require.Equal(t, int(idx), 19) + require.Len(t, nodes, 2) + require.Contains(t, nodes[0].ServiceTags, "v2") + require.Contains(t, nodes[0].ServiceTags, "slave") + require.Contains(t, nodes[1].ServiceTags, "v2") + require.Contains(t, nodes[1].ServiceTags, "slave") idx, nodes, err = s.ServiceTagNodes(nil, "db", []string{"dev"}) - if err != nil { - t.Fatalf("err: %s", err) - } - if idx != 19 { - t.Fatalf("bad: %v", idx) - } - if len(nodes) != 1 { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].Node != "foo" { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].Address != "127.0.0.1" { - t.Fatalf("bad: %v", nodes) - } - if !lib.StrContains(nodes[0].ServiceTags, "dev") { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].ServicePort != 8001 { - t.Fatalf("bad: %v", nodes) - } + require.NoError(t, err) + require.Equal(t, int(idx), 19) + require.Len(t, nodes, 1) + require.Equal(t, nodes[0].Node, "foo") + require.Equal(t, nodes[0].Address, "127.0.0.1") + require.Contains(t, nodes[0].ServiceTags, "dev") + require.Equal(t, nodes[0].ServicePort, 8001) } func TestStateStore_DeleteService(t *testing.T) { diff --git a/api/health_test.go b/api/health_test.go index 1ecd3cfc85dd..2663e59f7075 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -316,8 +316,10 @@ func TestAPI_HealthService_MultipleTags(t *testing.T) { conf.NodeName = "node123" }) defer s.Stop() + agent := c.Agent() health := c.Health() + // Make two services with a check reg := &AgentServiceRegistration{ Name: "foo", @@ -330,6 +332,7 @@ func TestAPI_HealthService_MultipleTags(t *testing.T) { } require.NoError(t, agent.ServiceRegister(reg)) defer agent.ServiceDeregister("foo1") + reg2 := &AgentServiceRegistration{ Name: "foo", ID: "foo2", @@ -341,16 +344,20 @@ func TestAPI_HealthService_MultipleTags(t *testing.T) { } require.NoError(t, agent.ServiceRegister(reg2)) defer agent.ServiceDeregister("foo2") + // Test searching with one tag (two results) retry.Run(t, func(r *retry.R) { services, meta, err := health.ServiceMultipleTags("foo", []string{"bar"}, true, nil) + require.NoError(t, err) require.NotEqual(t, meta.LastIndex, 0) require.Len(t, services, 2) }) + // Test searching with two tags (one result) retry.Run(t, func(r *retry.R) { services, meta, err := health.ServiceMultipleTags("foo", []string{"bar", "v2"}, true, nil) + require.NoError(t, err) require.NotEqual(t, meta.LastIndex, 0) require.Len(t, services, 1) From f39d51a0108d4d39202357d2bbbde156c69d9bbf Mon Sep 17 00:00:00 2001 From: Rebecca Zanzig Date: Wed, 10 Oct 2018 16:09:50 -0700 Subject: [PATCH 4/4] Update HealthConnect check after a bad merge --- api/health_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/api/health_test.go b/api/health_test.go index 2663e59f7075..7a9147350e8e 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -404,10 +404,12 @@ func TestAPI_HealthConnect(t *testing.T) { // Register the proxy proxyReg := &AgentServiceRegistration{ - Name: "foo-proxy", - Port: 8001, - Kind: ServiceKindConnectProxy, - ProxyDestination: "foo", + Name: "foo-proxy", + Port: 8001, + Kind: ServiceKindConnectProxy, + Proxy: &AgentServiceConnectProxyConfig{ + DestinationServiceName: "foo", + }, } err = agent.ServiceRegister(proxyReg) require.NoError(t, err)