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

absolutelightning · 2023-07-28T17:57:06Z

Description

Fixed the working of filter query parameter in /v1/catalog/services API. Fixes - #17422

Testing & Reproduction steps

Followed this tutorial and setup workloads in K8s.
Did curl -

asheshvidyut@asheshvidyut-H2GX766V9T ~/consul (NET-4135) » curl -ks -G http://localhost:8500/v1/catalog/services --data-urlencode filter="NodeMeta[\"consul-network-segment\"] == \"\" and NodeMeta[\"consul-version\"] == \"1.17.0\"" | jq
{
  "consul": []
}
asheshvidyut@asheshvidyut-H2GX766V9T ~/consul (NET-4135) » curl -ks -G http://localhost:8500/v1/catalog/services --data-urlencode filter="NodeMeta[\"synthetic-node\"] == true" | jq
{
  "frontend": [],
  "frontend-sidecar-proxy": [],
  "nginx": [],
  "nginx-sidecar-proxy": [],
  "payments": [],
  "payments-sidecar-proxy": [],
  "postgres": [],
  "postgres-sidecar-proxy": [],
  "products-api": [],
  "products-api-sidecar-proxy": [],
  "public-api": [],
  "public-api-sidecar-proxy": []
}

PR Checklist

updated test coverage
external facing docs updated
appropriate backport labels added
not a security concern

absolutelightning · 2023-07-28T18:18:21Z

@david-yu - please help me with the backports.

lkysow · 2023-07-28T18:20:59Z

agent/consul/state/catalog.go

+	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


@rboyer I saw you were the one that added parseServiceNodes to a bunch of other endpoints back in 2022. Is there any performance implications we need to think about for adding this to this endpoint as well? This change is needed to fill in nodemeta so it can be filtered on.

Currently we only do the expensive join against the nodes table when the user provides the NodeMeta parameter on the original API query. When they do this we call a completely different function link.

If we were concerned like that, we could instead alter the Store.Services method to take an extra boolean parameter to conditionally join the NodeMeta as in this PR.

Then the calling code in agent/consul/catalog_endpoint.go could set the flag to true when filter != "" OR just when:

strings.Contains(strings.ToLower(filter), "nodemeta")

@rboyer I have updated the PR can I merge this now? please approve.

lkysow · 2023-07-28T18:22:06Z

Can we simplify this if/else now:

consul/agent/consul/catalog_endpoint.go

Lines 596 to 600 in 7f4fd27

    
           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) 
        
           }

absolutelightning · 2023-07-28T18:29:13Z

Can we simplify this if/else now:

consul/agent/consul/catalog_endpoint.go

Lines 596 to 600 in 7f4fd27

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)

}

I think we want to node-meta also. What do you think?

lkysow · 2023-07-28T18:38:49Z

I was wondering if NodeMetaFilters can be encoded as a bexpr expression.

absolutelightning · 2023-07-28T18:50:28Z

I was wondering if NodeMetaFilters can be encoded as a bexpr expression.

I think no, because node-meta is a json like structure but filter takes expression with == , "and" or "or"

agent/catalog_endpoint.go

david-yu · 2023-07-28T22:57:27Z

Added backports to 1.16.x, 1.15.x, and 1.14.x.

absolutelightning · 2023-08-02T10:45:41Z

@rboyer @lkysow please let me know the next steps.

…-4135

david-yu · 2023-10-06T19:10:09Z

Thank you for the review. @absolutelightning could you go ahead and merge and follow up with the backports?

* 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>

…PI into release/1.16.x (#19113) * backport of commit cef8e3d * 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> * removed unwanted log lines * removed unwanted log lines --------- Co-authored-by: absolutelightning <ashesh.vidyut@hashicorp.com> Co-authored-by: Ashesh Vidyut <134911583+absolutelightning@users.noreply.github.com> Co-authored-by: Ganesh S <ganesh.seetharaman@hashicorp.com> Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>

* 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>

#18322) (#19115) 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 * 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 * 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>

…18322) (#19116) 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 * 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 * 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>

missylbytes · 2023-10-10T14:19:43Z

FYI this PR broke the enterprise tests.
One example OSS->ENT PR: https://github.com/hashicorp/consul-enterprise/pull/7268
Failed tests: https://github.com/hashicorp/consul-enterprise/actions/runs/6459991615/job/17536847333?pr=7268
It was the changes made to the Services function: https://github.com/hashicorp/consul/pull/18322/files#diff-3e1e8da6c0e163e7b582def0d554a491c3a72ba9e02c1bfdce269bfe6cea50fdR1221

* 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>

absolutelightning and others added 5 commits July 28, 2023 17:58

logs for debugging

cef8e3d

Init

79fdf19

Merge branch 'main' into NET-4135

91efd2c

white spaces fix

4fa91a6

added change log

92ed62d

absolutelightning marked this pull request as ready for review July 28, 2023 18:11

absolutelightning changed the title ~~NET-4135~~ NET-4135 - Fix NodeMeta filtering Catalog List Services API Jul 28, 2023

absolutelightning requested review from lkysow, hc-github-team-consul-core, github-team-consul-core-pr-approver, a team and JadhavPoonam and removed request for a team July 28, 2023 18:13

lkysow reviewed Jul 28, 2023

View reviewed changes

absolutelightning added 2 commits July 29, 2023 00:09

Fix tests

1a53944

fix typo

c7c5876

lkysow reviewed Jul 28, 2023

View reviewed changes

agent/catalog_endpoint.go Outdated Show resolved Hide resolved

david-yu added backport/1.14 Changes are backported to 1.14 backport/1.15 Changes are backported to 1.15 backport/1.16 Changes are backported to 1.16 labels Jul 28, 2023

using queryoptionfilter to populate args.filter

db1039a

david-yu linked an issue Jul 29, 2023 that may be closed by this pull request

NodeMeta filtering Catalog List Services API is not working #17422

Closed

absolutelightning requested a review from lkysow July 29, 2023 03:53

absolutelightning added 2 commits September 26, 2023 09:54

removed unwanted code

1141444

Merge branch 'NET-4135' of ssh://github.com/hashicorp/consul into NET…

af363ff

…-4135

vercel bot deployed to Preview – consul September 26, 2023 04:29 View deployment

absolutelightning requested a review from rboyer September 26, 2023 15:28

Merge branch 'main' into NET-4135

ebcf3c3

vercel bot deployed to Preview – consul September 27, 2023 05:20 View deployment

rboyer approved these changes Oct 6, 2023

View reviewed changes

Merge branch 'main' into NET-4135

04aae7e

absolutelightning enabled auto-merge (squash) October 8, 2023 12:34

vercel bot deployed to Preview – consul October 8, 2023 12:37 View deployment

absolutelightning merged commit a30ccdf into main Oct 8, 2023
87 checks passed

absolutelightning deleted the NET-4135 branch October 8, 2023 12:48

absolutelightning mentioned this pull request Oct 9, 2023

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

Merged

4 tasks

absolutelightning mentioned this pull request Oct 9, 2023

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

Merged

4 tasks

jira bot mentioned this pull request Dec 1, 2023

copywrite EOL proto headers debugging #19799

Closed

zalimeni mentioned this pull request Dec 15, 2023

[NET-6581] perf: Fetch services once rather than per-node on deregister hashicorp/consul-k8s#3322

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

absolutelightning commented Jul 28, 2023 •

edited

absolutelightning commented Jul 28, 2023

lkysow Jul 28, 2023

rboyer Sep 19, 2023

absolutelightning Sep 21, 2023

absolutelightning Sep 21, 2023

lkysow commented Jul 28, 2023

absolutelightning commented Jul 28, 2023

lkysow commented Jul 28, 2023

absolutelightning commented Jul 28, 2023

david-yu commented Jul 28, 2023

absolutelightning commented Aug 2, 2023

david-yu commented Oct 6, 2023

missylbytes commented Oct 10, 2023

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

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

Conversation

absolutelightning commented Jul 28, 2023 • edited

Description

Testing & Reproduction steps

PR Checklist

absolutelightning commented Jul 28, 2023

lkysow Jul 28, 2023

Choose a reason for hiding this comment

rboyer Sep 19, 2023

Choose a reason for hiding this comment

absolutelightning Sep 21, 2023

Choose a reason for hiding this comment

absolutelightning Sep 21, 2023

Choose a reason for hiding this comment

lkysow commented Jul 28, 2023

absolutelightning commented Jul 28, 2023

lkysow commented Jul 28, 2023

absolutelightning commented Jul 28, 2023

david-yu commented Jul 28, 2023

absolutelightning commented Aug 2, 2023

david-yu commented Oct 6, 2023

missylbytes commented Oct 10, 2023

absolutelightning commented Jul 28, 2023 •

edited