From c7e3db2c609a7db4f345baa6fc5804539a7633aa Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Mon, 9 Oct 2023 01:05:58 -0400 Subject: [PATCH] Backport of NET-4135 - Fix NodeMeta filtering Catalog List Services API into release/1.16.x (#19113) * backport of commit cef8e3d27b2bab062d44e8d55f40a2e97c8efe3b * 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 * 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 Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com> * removed unwanted log lines * removed unwanted log lines --------- Co-authored-by: absolutelightning Co-authored-by: Ashesh Vidyut <134911583+absolutelightning@users.noreply.github.com> Co-authored-by: Ganesh S Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com> --- .changelog/18322.txt | 3 ++ agent/consul/catalog_endpoint.go | 2 +- agent/consul/state/catalog.go | 11 ++++- agent/consul/state/catalog_test.go | 6 +-- api/catalog_test.go | 67 ++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 .changelog/18322.txt diff --git a/.changelog/18322.txt b/.changelog/18322.txt new file mode 100644 index 000000000..9be8c2cfa --- /dev/null +++ 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 +``` \ No newline at end of file diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 446037a31..c53c41356 100644 --- 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 index 4c2770a37..ceb63fab3 100644 --- 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 } @@ -1311,6 +1318,7 @@ func (s *Store) ServicesByNodeMeta(ws memdb.WatchSet, filters map[string]string, EnterpriseMeta: *entMeta, PeerName: peerName, }) + if err != nil { return 0, nil, fmt.Errorf("failed nodes lookup: %s", err) } @@ -1330,7 +1338,6 @@ func (s *Store) ServicesByNodeMeta(ws memdb.WatchSet, filters map[string]string, if len(filters) > 1 && !structs.SatisfiesMetaFilters(n.Meta, filters) { continue } - // List all the services on the node services, err := catalogServiceListByNode(tx, n.Node, entMeta, n.PeerName, false) if err != nil { diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 763d29c8c..7ca578307 100644 --- 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 index 2b0a4097b..708392c42 100644 --- 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)