diff --git a/agent/cache-types/catalog_services_test.go b/agent/cache-types/catalog_services_test.go index 7889f8bdfa8a..7eeff023d5bc 100644 --- a/agent/cache-types/catalog_services_test.go +++ b/agent/cache-types/catalog_services_test.go @@ -25,28 +25,30 @@ func TestCatalogServices(t *testing.T) { require.Equal(uint64(24), req.QueryOptions.MinQueryIndex) require.Equal(1*time.Second, req.QueryOptions.MaxQueryTime) require.Equal("web", req.ServiceName) - require.Equal("canary", req.ServiceTag) require.True(req.AllowStale) reply := args.Get(2).(*structs.IndexedServiceNodes) + reply.ServiceNodes = []*structs.ServiceNode{ + &structs.ServiceNode{ServiceTags: req.ServiceTags}, + } reply.QueryMeta.Index = 48 resp = reply }) // Fetch - result, err := typ.Fetch(cache.FetchOptions{ + resultA, err := typ.Fetch(cache.FetchOptions{ MinIndex: 24, Timeout: 1 * time.Second, }, &structs.ServiceSpecificRequest{ Datacenter: "dc1", ServiceName: "web", - ServiceTag: "canary", + ServiceTags: []string{"tag1", "tag2"}, }) require.NoError(err) require.Equal(cache.FetchResult{ Value: resp, Index: 48, - }, result) + }, resultA) } func TestCatalogServices_badReqType(t *testing.T) { diff --git a/agent/cache-types/health_services_test.go b/agent/cache-types/health_services_test.go index 7b00fbb7b11f..4b368c29d438 100644 --- a/agent/cache-types/health_services_test.go +++ b/agent/cache-types/health_services_test.go @@ -25,28 +25,30 @@ func TestHealthServices(t *testing.T) { require.Equal(uint64(24), req.QueryOptions.MinQueryIndex) require.Equal(1*time.Second, req.QueryOptions.MaxQueryTime) require.Equal("web", req.ServiceName) - require.Equal("canary", req.ServiceTag) require.True(req.AllowStale) reply := args.Get(2).(*structs.IndexedCheckServiceNodes) + reply.Nodes = []structs.CheckServiceNode{ + {Service: &structs.NodeService{Tags: req.ServiceTags}}, + } reply.QueryMeta.Index = 48 resp = reply }) // Fetch - result, err := typ.Fetch(cache.FetchOptions{ + resultA, err := typ.Fetch(cache.FetchOptions{ MinIndex: 24, Timeout: 1 * time.Second, }, &structs.ServiceSpecificRequest{ Datacenter: "dc1", ServiceName: "web", - ServiceTag: "canary", + ServiceTags: []string{"tag1", "tag2"}, }) require.NoError(err) require.Equal(cache.FetchResult{ Value: resp, Index: 48, - }, result) + }, resultA) } func TestHealthServices_badReqType(t *testing.T) { diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index da13abf6cb74..614984ccda47 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -275,8 +275,9 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru if args.TagFilter { tags := args.ServiceTags - - // Agents < v1.3.0 and DNS service lookups populate the ServiceTag field. In this case, + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. + // Agents < v1.3.0 populate the ServiceTag field. In this case, // use ServiceTag instead of the ServiceTags field. if args.ServiceTag != "" { tags = []string{args.ServiceTag} @@ -339,6 +340,8 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru metrics.IncrCounterWithLabels([]string{"catalog", key, "query"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}}) + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. if args.ServiceTag != "" { metrics.IncrCounterWithLabels([]string{"catalog", key, "query-tag"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}, {Name: "tag", Value: args.ServiceTag}}) diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 259d1cb182b3..70e0fa95d18d 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -1655,6 +1655,8 @@ func TestCatalog_ListServiceNodes_ServiceTags_V1_2_3Compat(t *testing.T) { err = s1.fsm.State().EnsureService(2, "foo", &structs.NodeService{ID: "db2", Service: "db", Tags: []string{"secondary"}, Address: "127.0.0.1", Port: 5001}) require.NoError(t, err) + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. // make a request with the <=1.2.3 ServiceTag tag field (vs ServiceTags) args := structs.ServiceSpecificRequest{ Datacenter: "dc1", @@ -1670,6 +1672,8 @@ func TestCatalog_ListServiceNodes_ServiceTags_V1_2_3Compat(t *testing.T) { require.Equal(t, 1, len(out.ServiceNodes)) require.Equal(t, "db1", out.ServiceNodes[0].ServiceID) + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. // test with the other tag args = structs.ServiceSpecificRequest{ Datacenter: "dc1", @@ -1693,6 +1697,8 @@ func TestCatalog_ListServiceNodes_ServiceTags_V1_2_3Compat(t *testing.T) { require.NoError(t, err) require.Equal(t, 2, len(out.ServiceNodes)) + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. // when both ServiceTag and ServiceTags fields are populated, use ServiceTag args = structs.ServiceSpecificRequest{ Datacenter: "dc1", diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index 103fb2faf666..0f2f44c27cfa 100644 --- a/agent/consul/health_endpoint.go +++ b/agent/consul/health_endpoint.go @@ -167,6 +167,8 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc metrics.IncrCounterWithLabels([]string{"health", key, "query"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}}) + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. if args.ServiceTag != "" { metrics.IncrCounterWithLabels([]string{"health", key, "query-tag"}, 1, []metrics.Label{{Name: "service", Value: args.ServiceName}, {Name: "tag", Value: args.ServiceTag}}) @@ -198,7 +200,9 @@ 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) { - // Agents < v1.3.0 and DNS service lookups populate the ServiceTag field. In this case, + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. + // Agents < v1.3.0 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}) diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index 5a937ec5da08..b02e30e9986f 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -572,7 +572,7 @@ func TestHealth_ServiceNodes(t *testing.T) { req := structs.ServiceSpecificRequest{ Datacenter: "dc1", ServiceName: "db", - ServiceTag: "master", + ServiceTags: []string{"master"}, TagFilter: false, } if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2); err != nil { @@ -601,6 +601,45 @@ func TestHealth_ServiceNodes(t *testing.T) { if nodes[1].Checks[0].Status != api.HealthPassing { t.Fatalf("Bad: %v", nodes[1]) } + + // Same should still work for <1.3 RPCs with singular tags + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. + { + var out2 structs.IndexedCheckServiceNodes + req := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: "db", + ServiceTag: "master", + TagFilter: false, + } + if err := msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2); err != nil { + t.Fatalf("err: %v", err) + } + + nodes := out2.Nodes + if len(nodes) != 2 { + t.Fatalf("Bad: %v", nodes) + } + if nodes[0].Node.Node != "bar" { + t.Fatalf("Bad: %v", nodes[0]) + } + if nodes[1].Node.Node != "foo" { + t.Fatalf("Bad: %v", nodes[1]) + } + if !lib.StrContains(nodes[0].Service.Tags, "slave") { + t.Fatalf("Bad: %v", nodes[0]) + } + if !lib.StrContains(nodes[1].Service.Tags, "master") { + t.Fatalf("Bad: %v", nodes[1]) + } + if nodes[0].Checks[0].Status != api.HealthWarning { + t.Fatalf("Bad: %v", nodes[0]) + } + if nodes[1].Checks[0].Status != api.HealthPassing { + t.Fatalf("Bad: %v", nodes[1]) + } + } } func TestHealth_ServiceNodes_MultipleServiceTags(t *testing.T) { diff --git a/agent/dns.go b/agent/dns.go index 3e7213b2a733..5775e15a52ca 100644 --- a/agent/dns.go +++ b/agent/dns.go @@ -1009,7 +1009,7 @@ func (d *DNSServer) lookupServiceNodes(datacenter, service, tag string, connect Connect: connect, Datacenter: datacenter, ServiceName: service, - ServiceTag: tag, + ServiceTags: []string{tag}, TagFilter: tag != "", QueryOptions: structs.QueryOptions{ Token: d.agent.tokens.UserToken(), diff --git a/agent/dns_test.go b/agent/dns_test.go index 00edf257d423..95d941e1bfe7 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -2571,11 +2571,24 @@ func TestDNS_ServiceLookup_TagPeriod(t *testing.T) { t.Fatalf("err: %v", err) } + m1 := new(dns.Msg) + m1.SetQuestion("v1.master2.db.service.consul.", dns.TypeSRV) + + c1 := new(dns.Client) + in, _, err := c1.Exchange(m1, a.DNSAddr()) + if err != nil { + t.Fatalf("err: %v", err) + } + + if len(in.Answer) != 0 { + t.Fatalf("Bad: %#v", in) + } + m := new(dns.Msg) m.SetQuestion("v1.master.db.service.consul.", dns.TypeSRV) c := new(dns.Client) - in, _, err := c.Exchange(m, a.DNSAddr()) + in, _, err = c.Exchange(m, a.DNSAddr()) if err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/structs/structs.go b/agent/structs/structs.go index c1bd34a9f95a..4e11e97339dc 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -6,6 +6,7 @@ import ( "math/rand" "reflect" "regexp" + "sort" "strconv" "strings" "time" @@ -351,11 +352,13 @@ type ServiceSpecificRequest struct { Datacenter string NodeMetaFilters map[string]string ServiceName string - ServiceTag string - ServiceTags []string - ServiceAddress string - TagFilter bool // Controls tag filtering - Source QuerySource + // DEPRECATED (singular-service-tag) - remove this when backwards RPC compat + // with 1.2.x is not required. + ServiceTag string + ServiceTags []string + ServiceAddress string + TagFilter bool // Controls tag filtering + Source QuerySource // Connect if true will only search for Connect-compatible services. Connect bool @@ -384,10 +387,19 @@ func (r *ServiceSpecificRequest) CacheInfo() cache.RequestInfo { // cached results, we need to be careful we maintain the same order of fields // here. We could alternatively use `hash:set` struct tag on an anonymous // struct to make it more robust if it becomes significant. + sort.Strings(r.ServiceTags) v, err := hashstructure.Hash([]interface{}{ r.NodeMetaFilters, r.ServiceName, + // DEPRECATED (singular-service-tag) - remove this when upgrade RPC compat + // with 1.2.x is not required. We still need this in because <1.3 agents + // might still send RPCs with singular tag set. In fact the only place we + // use this method is in agent cache so if the agent is new enough to have + // this code this should never be set, but it's safer to include it until we + // completely remove this field just in case it's erroneously used anywhere + // (e.g. until this change DNS still used it). r.ServiceTag, + r.ServiceTags, r.ServiceAddress, r.TagFilter, r.Connect, diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index 473e71bbe0c1..0c943b90371b 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/types" "github.com/stretchr/testify/assert" @@ -899,3 +900,139 @@ func TestStructs_validateMetaPair(t *testing.T) { } } } + +func TestSpecificServiceRequest_CacheInfo(t *testing.T) { + tests := []struct { + name string + req ServiceSpecificRequest + mutate func(req *ServiceSpecificRequest) + want *cache.RequestInfo + wantSame bool + }{ + { + name: "basic params", + req: ServiceSpecificRequest{ + QueryOptions: QueryOptions{Token: "foo"}, + Datacenter: "dc1", + }, + want: &cache.RequestInfo{ + Token: "foo", + Datacenter: "dc1", + }, + wantSame: true, + }, + { + name: "name should be considered", + req: ServiceSpecificRequest{ + ServiceName: "web", + }, + mutate: func(req *ServiceSpecificRequest) { + req.ServiceName = "db" + }, + wantSame: false, + }, + { + name: "node meta should be considered", + req: ServiceSpecificRequest{ + NodeMetaFilters: map[string]string{ + "foo": "bar", + }, + }, + mutate: func(req *ServiceSpecificRequest) { + req.NodeMetaFilters = map[string]string{ + "foo": "qux", + } + }, + wantSame: false, + }, + { + name: "address should be considered", + req: ServiceSpecificRequest{ + ServiceAddress: "1.2.3.4", + }, + mutate: func(req *ServiceSpecificRequest) { + req.ServiceAddress = "4.3.2.1" + }, + wantSame: false, + }, + { + name: "tag filter should be considered", + req: ServiceSpecificRequest{ + TagFilter: true, + }, + mutate: func(req *ServiceSpecificRequest) { + req.TagFilter = false + }, + wantSame: false, + }, + { + name: "connect should be considered", + req: ServiceSpecificRequest{ + Connect: true, + }, + mutate: func(req *ServiceSpecificRequest) { + req.Connect = false + }, + wantSame: false, + }, + { + name: "tags should be different", + req: ServiceSpecificRequest{ + ServiceName: "web", + ServiceTags: []string{"foo"}, + }, + mutate: func(req *ServiceSpecificRequest) { + req.ServiceTags = []string{"foo", "bar"} + }, + wantSame: false, + }, + { + name: "tags should not depend on order", + req: ServiceSpecificRequest{ + ServiceName: "web", + ServiceTags: []string{"bar", "foo"}, + }, + mutate: func(req *ServiceSpecificRequest) { + req.ServiceTags = []string{"foo", "bar"} + }, + wantSame: true, + }, + // DEPRECATED (singular-service-tag) - remove this when upgrade RPC compat + // with 1.2.x is not required. + { + name: "legacy requests with singular tag should be different", + req: ServiceSpecificRequest{ + ServiceName: "web", + ServiceTag: "foo", + }, + mutate: func(req *ServiceSpecificRequest) { + req.ServiceTag = "bar" + }, + wantSame: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + info := tc.req.CacheInfo() + if tc.mutate != nil { + tc.mutate(&tc.req) + } + afterInfo := tc.req.CacheInfo() + + // Check key matches or not + if tc.wantSame { + require.Equal(t, info, afterInfo) + } else { + require.NotEqual(t, info, afterInfo) + } + + if tc.want != nil { + // Reset key since we don't care about the actual hash value as long as + // it does/doesn't change appropriately (asserted with wantSame above). + info.Key = "" + require.Equal(t, *tc.want, info) + } + }) + } +}