NET-4135 - Fix NodeMeta filtering Catalog List Services API

[absolutelightning]

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]

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

[lkysow]

@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.

[rboyer]

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

[absolutelightning]

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

[absolutelightning]

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

[lkysow]

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]

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]

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

[absolutelightning]

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]

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

[absolutelightning]

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

[david-yu]

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

[missylbytes]

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