From 4fb564abbcb02206e5266b34361d027ec1f5b023 Mon Sep 17 00:00:00 2001
From: Aestek <thib.gilles@gmail.com>
Date: Tue, 13 Nov 2018 15:44:36 +0100
Subject: [PATCH] Fix catalog tag filter backward compat (#4944)

Fix catalog service node filtering (ex /v1/catalog/service/srv?tag=tag1)
between agent version <=v1.2.3 and server >=v1.3.0.
New server version did not account for the old field when filtering
hence request made from old agent were not tag-filtered.
---
 agent/consul/catalog_endpoint.go      | 10 +++-
 agent/consul/catalog_endpoint_test.go | 73 +++++++++++++++++++++++++++
 agent/consul/health_endpoint.go       |  2 +-
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go
index a95e6782d..da13abf6c 100644
--- a/agent/consul/catalog_endpoint.go
+++ b/agent/consul/catalog_endpoint.go
@@ -274,7 +274,15 @@ func (c *Catalog) ServiceNodes(args *structs.ServiceSpecificRequest, reply *stru
 			}
 
 			if args.TagFilter {
-				return s.ServiceTagNodes(ws, args.ServiceName, args.ServiceTags)
+				tags := args.ServiceTags
+
+				// Agents < v1.3.0 and DNS service lookups populate the ServiceTag field. In this case,
+				// use ServiceTag instead of the ServiceTags field.
+				if args.ServiceTag != "" {
+					tags = []string{args.ServiceTag}
+				}
+
+				return s.ServiceTagNodes(ws, args.ServiceName, tags)
 			}
 
 			return s.ServiceNodes(ws, args.ServiceName)
diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go
index ddb07628c..259d1cb18 100644
--- a/agent/consul/catalog_endpoint_test.go
+++ b/agent/consul/catalog_endpoint_test.go
@@ -1634,6 +1634,79 @@ func TestCatalog_ListServiceNodes(t *testing.T) {
 	}
 }
 
+// TestCatalog_ListServiceNodes_ServiceTags_V1_2_3Compat asserts the compatibility between <=v1.2.3 agents and >=v1.3.0 servers
+// see https://github.com/hashicorp/consul/issues/4922
+func TestCatalog_ListServiceNodes_ServiceTags_V1_2_3Compat(t *testing.T) {
+	t.Parallel()
+	dir1, s1 := testServer(t)
+	defer os.RemoveAll(dir1)
+	defer s1.Shutdown()
+	codec := rpcClient(t, s1)
+	defer codec.Close()
+
+	testrpc.WaitForTestAgent(t, s1.RPC, "dc1")
+
+	err := s1.fsm.State().EnsureNode(1, &structs.Node{Node: "foo", Address: "127.0.0.1"})
+	require.NoError(t, err)
+
+	// register two service instances with different tags
+	err = s1.fsm.State().EnsureService(2, "foo", &structs.NodeService{ID: "db1", Service: "db", Tags: []string{"primary"}, Address: "127.0.0.1", Port: 5000})
+	require.NoError(t, err)
+	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)
+
+	// make a request with the <=1.2.3 ServiceTag tag field (vs ServiceTags)
+	args := structs.ServiceSpecificRequest{
+		Datacenter:  "dc1",
+		ServiceName: "db",
+		ServiceTag:  "primary",
+		TagFilter:   true,
+	}
+	var out structs.IndexedServiceNodes
+	err = msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)
+	require.NoError(t, err)
+
+	// nodes should be filtered, even when using the old ServiceTag field
+	require.Equal(t, 1, len(out.ServiceNodes))
+	require.Equal(t, "db1", out.ServiceNodes[0].ServiceID)
+
+	// test with the other tag
+	args = structs.ServiceSpecificRequest{
+		Datacenter:  "dc1",
+		ServiceName: "db",
+		ServiceTag:  "secondary",
+		TagFilter:   true,
+	}
+	out = structs.IndexedServiceNodes{}
+	err = msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)
+	require.NoError(t, err)
+	require.Equal(t, 1, len(out.ServiceNodes))
+	require.Equal(t, "db2", out.ServiceNodes[0].ServiceID)
+
+	// no tag, both instances
+	args = structs.ServiceSpecificRequest{
+		Datacenter:  "dc1",
+		ServiceName: "db",
+	}
+	out = structs.IndexedServiceNodes{}
+	err = msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)
+	require.NoError(t, err)
+	require.Equal(t, 2, len(out.ServiceNodes))
+
+	// when both ServiceTag and ServiceTags fields are populated, use ServiceTag
+	args = structs.ServiceSpecificRequest{
+		Datacenter:  "dc1",
+		ServiceName: "db",
+		ServiceTag:  "primary",
+		ServiceTags: []string{"secondary"},
+		TagFilter:   true,
+	}
+	out = structs.IndexedServiceNodes{}
+	err = msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &args, &out)
+	require.NoError(t, err)
+	require.Equal(t, "db1", out.ServiceNodes[0].ServiceID)
+}
+
 func TestCatalog_ListServiceNodes_NodeMetaFilter(t *testing.T) {
 	t.Parallel()
 	dir1, s1 := testServer(t)
diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go
index a3c29b7b6..103fb2faf 100644
--- a/agent/consul/health_endpoint.go
+++ b/agent/consul/health_endpoint.go
@@ -198,7 +198,7 @@ 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) {
-	// DNS service lookups populate the ServiceTag field. In this case,
+	// Agents < v1.3.0 and 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})