NET-4135 - Fix NodeMeta filtering Catalog List Services API (#18322)

* logs for debugging * Init * white spaces fix * added change log * Fix tests * fix typo * using queryoptionfilter to populate args.filter * tests * fix test * fix tests * fix tests * fix tests * fix tests * fix variable name * fix tests * fix tests * fix tests * Update .changelog/18322.txt Co-authored-by: Ganesh S <ganesh.seetharaman@hashicorp.com> * fix change log * address nits * removed unused line * doing join only when filter has nodemeta * fix tests * fix tests * Update agent/consul/catalog_endpoint.go Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com> * fix tests * removed unwanted code --------- Co-authored-by: Ganesh S <ganesh.seetharaman@hashicorp.com> Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
hashicorp · Oct 12, 2023 · def5920 · def5920
1 parent 71ed289
commit def5920
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 5 deletions.
diff --git a/.changelog/18322.txt b/.changelog/18322.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+catalog api: fixes a bug with catalog api where filter query parameter was not working correctly for the `/v1/catalog/services` endpoint
+```
diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go
@@ -596,7 +596,7 @@ func (c *Catalog) ListServices(args *structs.DCSpecificRequest, reply *structs.I
 			if len(args.NodeMetaFilters) > 0 {
 				reply.Index, serviceNodes, err = state.ServicesByNodeMeta(ws, args.NodeMetaFilters, &args.EnterpriseMeta, args.PeerName)
 			} else {
-				reply.Index, serviceNodes, err = state.Services(ws, &args.EnterpriseMeta, args.PeerName)
+				reply.Index, serviceNodes, err = state.Services(ws, &args.EnterpriseMeta, args.PeerName, args.Filter != "")
 			}
 			if err != nil {
 				return err

diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go
@@ -1218,7 +1218,7 @@ func terminatingGatewayVirtualIPsSupported(tx ReadTxn, ws memdb.WatchSet) (bool,
 }
 
 // Services returns all services along with a list of associated tags.
-func (s *Store) Services(ws memdb.WatchSet, entMeta *acl.EnterpriseMeta, peerName string) (uint64, []*structs.ServiceNode, error) {
+func (s *Store) Services(ws memdb.WatchSet, entMeta *acl.EnterpriseMeta, peerName string, joinServiceNodes bool) (uint64, structs.ServiceNodes, error) {
 	tx := s.db.Txn(false)
 	defer tx.Abort()
 
@@ -1236,6 +1236,13 @@ func (s *Store) Services(ws memdb.WatchSet, entMeta *acl.EnterpriseMeta, peerNam
 	for service := services.Next(); service != nil; service = services.Next() {
 		result = append(result, service.(*structs.ServiceNode))
 	}
+	if joinServiceNodes {
+		parsedResult, err := parseServiceNodes(tx, ws, result, entMeta, peerName)
+		if err != nil {
+			return 0, nil, fmt.Errorf("failed querying and parsing services :%s", err)
+		}
+		return idx, parsedResult, nil
+	}
 	return idx, result, nil
 }
 

diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go
@@ -2178,7 +2178,7 @@ func TestStateStore_Services(t *testing.T) {
 
 	// Listing with no results returns an empty list.
 	ws := memdb.NewWatchSet()
-	idx, services, err := s.Services(ws, nil, "")
+	idx, services, err := s.Services(ws, nil, "", false)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -2223,7 +2223,7 @@ func TestStateStore_Services(t *testing.T) {
 
 	// Pull all the services.
 	ws = memdb.NewWatchSet()
-	idx, services, err = s.Services(ws, nil, "")
+	idx, services, err = s.Services(ws, nil, "", false)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -2232,7 +2232,7 @@ func TestStateStore_Services(t *testing.T) {
 	}
 
 	// Verify the result.
-	expected := []*structs.ServiceNode{
+	expected := structs.ServiceNodes{
 		ns1Dogs.ToServiceNode("node1"),
 		ns1.ToServiceNode("node1"),
 		ns2.ToServiceNode("node2"),

diff --git a/api/catalog_test.go b/api/catalog_test.go
@@ -220,6 +220,73 @@ func TestAPI_CatalogServices_NodeMetaFilter(t *testing.T) {
 	})
 }
 
+func TestAPI_CatalogServices_FilterExpr_NodeMeta(t *testing.T) {
+	t.Parallel()
+	meta := map[string]string{"somekey": "somevalue", "synthetic": "true"}
+	c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) {
+		conf.NodeMeta = meta
+	})
+	defer s.Stop()
+
+	catalog := c.Catalog()
+	// Make sure we get the service back when filtering by filter expression
+	retry.Run(t, func(r *retry.R) {
+		services, meta, err := catalog.Services(&QueryOptions{Filter: "NodeMeta[\"synthetic\"] == true and NodeMeta[\"somekey\"] == somevalue"})
+		if err != nil {
+			r.Fatal(err)
+		}
+
+		if meta.LastIndex == 0 {
+			r.Fatalf("Bad: %v", meta)
+		}
+		if len(services) == 0 {
+			r.Fatalf("Bad: %v", services)
+		}
+	})
+	retry.Run(t, func(r *retry.R) {
+		services, meta, err := catalog.Services(&QueryOptions{Filter: "NodeMeta.synthetic == true"})
+		if err != nil {
+			r.Fatal(err)
+		}
+
+		if meta.LastIndex == 0 {
+			r.Fatalf("Bad: %v", meta)
+		}
+
+		if len(services) == 0 {
+			r.Fatalf("Bad: %v", services)
+		}
+	})
+	retry.Run(t, func(r *retry.R) {
+		services, meta, err := catalog.Services(&QueryOptions{Filter: "NodeMeta.somekey == somevalue"})
+		if err != nil {
+			r.Fatal(err)
+		}
+
+		if meta.LastIndex == 0 {
+			r.Fatalf("Bad: %v", meta)
+		}
+
+		if len(services) == 0 {
+			r.Fatalf("Bad: %v", services)
+		}
+	})
+	retry.Run(t, func(r *retry.R) {
+		services, meta, err := catalog.Services(&QueryOptions{Filter: "NodeMeta.nope == nope"})
+		if err != nil {
+			r.Fatal(err)
+		}
+
+		if meta.LastIndex == 0 {
+			r.Fatalf("Bad: %v", meta)
+		}
+
+		if len(services) != 0 {
+			r.Fatalf("Bad: %v", services)
+		}
+	})
+}
+
 func TestAPI_CatalogService(t *testing.T) {
 	t.Parallel()
 	c, s := makeClient(t)