NET-4135 - Fix NodeMeta filtering Catalog List Services API by asheshvidyut · Pull Request #18322 · hashicorp/consul

asheshvidyut · 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

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

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

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

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

asheshvidyut · 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.

asheshvidyut · 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"

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

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

asheshvidyut · 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

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

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

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

asheshvidyut requested review from a team, JadhavPoonam, github-team-consul-core-pr-approver, hc-github-team-consul-core and lkysow 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

Comment thread agent/catalog_endpoint.go Outdated

david-yu added backport/1.14 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

asheshvidyut 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

asheshvidyut 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

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

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

asheshvidyut merged commit a30ccdf into main Oct 8, 2023

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

asheshvidyut mentioned this pull request Oct 9, 2023

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

Merged

4 tasks

asheshvidyut mentioned this pull request Oct 9, 2023

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

Merged

4 tasks

atlassian 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

Conversation

asheshvidyut commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing & Reproduction steps

PR Checklist

Uh oh!

asheshvidyut commented Jul 28, 2023

Uh oh!

lkysow Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

rboyer Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

asheshvidyut Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

asheshvidyut Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

lkysow commented Jul 28, 2023

Uh oh!

asheshvidyut commented Jul 28, 2023

Uh oh!

lkysow commented Jul 28, 2023

Uh oh!

asheshvidyut commented Jul 28, 2023

Uh oh!

Uh oh!

david-yu commented Jul 28, 2023

Uh oh!

asheshvidyut commented Aug 2, 2023

Uh oh!

david-yu commented Oct 6, 2023

Uh oh!

missylbytes commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

asheshvidyut commented Jul 28, 2023 •

edited

Loading