Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 40 commits into from
Oct 8, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
cef8e3d
logs for debugging
absolutelightning Jul 28, 2023
79fdf19
Init
absolutelightning Jul 28, 2023
91efd2c
Merge branch 'main' into NET-4135
absolutelightning Jul 28, 2023
4fa91a6
white spaces fix
absolutelightning Jul 28, 2023
92ed62d
added change log
absolutelightning Jul 28, 2023
1a53944
Fix tests
absolutelightning Jul 28, 2023
c7c5876
fix typo
absolutelightning Jul 28, 2023
db1039a
using queryoptionfilter to populate args.filter
absolutelightning Jul 29, 2023
e475fa6
Merge branch 'main' of ssh://github.com/hashicorp/consul into NET-4135
absolutelightning Sep 15, 2023
dc4f9d1
Merge branch 'main' of ssh://github.com/hashicorp/consul into NET-4135
absolutelightning Sep 16, 2023
ecc8dbf
tests
absolutelightning Sep 16, 2023
b8dfd87
fix test
absolutelightning Sep 16, 2023
174f8b0
fix tests
absolutelightning Sep 16, 2023
6d1cee7
fix tests
absolutelightning Sep 16, 2023
8b21150
fix tests
absolutelightning Sep 16, 2023
6d8d396
fix tests
absolutelightning Sep 16, 2023
f63c251
fix variable name
absolutelightning Sep 18, 2023
7f6c537
fix tests
absolutelightning Sep 18, 2023
6c08f9c
fix tests
absolutelightning Sep 18, 2023
44f366b
fix tests
absolutelightning Sep 18, 2023
8c12cbb
Merge branch 'main' into NET-4135
absolutelightning Sep 18, 2023
961eeef
Update .changelog/18322.txt
absolutelightning Sep 18, 2023
5af1b1c
fix change log
absolutelightning Sep 18, 2023
1763589
address nits
absolutelightning Sep 18, 2023
f475356
removed unused line
absolutelightning Sep 18, 2023
86b1e28
Merge branch 'main' into NET-4135
absolutelightning Sep 18, 2023
aaf6e54
Merge branch 'main' into NET-4135
absolutelightning Sep 19, 2023
4ed56cc
doing join only when filter has nodemeta
absolutelightning Sep 20, 2023
eaed33c
Merge branch 'NET-4135' of ssh://github.com/hashicorp/consul into NET…
absolutelightning Sep 20, 2023
5d366e8
Merge branch 'main' into NET-4135
absolutelightning Sep 20, 2023
7a95115
fix tests
absolutelightning Sep 20, 2023
96421c2
Merge branch 'NET-4135' of ssh://github.com/hashicorp/consul into NET…
absolutelightning Sep 20, 2023
94e73db
fix tests
absolutelightning Sep 20, 2023
b7aa37e
Update agent/consul/catalog_endpoint.go
absolutelightning Sep 26, 2023
89ed772
fix tests
absolutelightning Sep 26, 2023
b25d920
Merge branch 'main' into NET-4135
absolutelightning Sep 26, 2023
1141444
removed unwanted code
absolutelightning Sep 26, 2023
af363ff
Merge branch 'NET-4135' of ssh://github.com/hashicorp/consul into NET…
absolutelightning Sep 26, 2023
ebcf3c3
Merge branch 'main' into NET-4135
absolutelightning Sep 27, 2023
04aae7e
Merge branch 'main' into NET-4135
absolutelightning Oct 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changelog/18322.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:bug
catalog api: fix a bug with catalog api where filter query parameter was not working correctly. endpoint fixed -
absolutelightning marked this conversation as resolved.
Show resolved Hide resolved
/v1/catalog/services
```
1 change: 1 addition & 0 deletions agent/catalog_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ func (s *HTTPHandlers) CatalogServices(resp http.ResponseWriter, req *http.Reque
if done := s.parse(resp, req, &args.Datacenter, &args.QueryOptions); done {
return nil, nil
}
args.Filter = args.QueryOptions.Filter
var out structs.IndexedServices
defer setMeta(resp, &out.QueryMeta)

Expand Down
8 changes: 6 additions & 2 deletions agent/consul/state/catalog.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) (uint64, structs.ServiceNodes, error) {
tx := s.db.Txn(false)
defer tx.Abort()

Expand All @@ -1236,7 +1236,11 @@ 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))
}
return idx, result, nil
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func (s *Store) ServiceList(ws memdb.WatchSet, entMeta *acl.EnterpriseMeta, peerName string) (uint64, structs.ServiceList, error) {
Expand Down
12 changes: 9 additions & 3 deletions agent/consul/state/catalog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, &acl.EnterpriseMeta{}, "")
if err != nil {
t.Fatalf("err: %s", err)
}
Expand All @@ -2199,10 +2199,14 @@ func TestStateStore_Services(t *testing.T) {
Port: 1111,
}
ns1.EnterpriseMeta.Normalize()
ns1.Tags = []string{}
ns1.Meta = map[string]string{}
if err := s.EnsureService(2, "node1", ns1); err != nil {
t.Fatalf("err: %s", err)
}
ns1Dogs := testRegisterService(t, s, 3, "node1", "dogs")
ns1Dogs.Tags = []string{}
ns1Dogs.Meta = map[string]string{}
ns1Dogs.EnterpriseMeta.Normalize()

testRegisterNode(t, s, 4, "node2")
Expand All @@ -2213,7 +2217,9 @@ func TestStateStore_Services(t *testing.T) {
Address: "1.1.1.1",
Port: 1111,
}
ns2.Tags = []string{}
ns2.EnterpriseMeta.Normalize()
ns2.Meta = map[string]string{}
if err := s.EnsureService(5, "node2", ns2); err != nil {
t.Fatalf("err: %s", err)
}
Expand All @@ -2223,7 +2229,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, &acl.EnterpriseMeta{}, "")
if err != nil {
t.Fatalf("err: %s", err)
}
Expand All @@ -2232,7 +2238,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"),
Expand Down
67 changes: 67 additions & 0 deletions api/catalog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,73 @@ func TestAPI_CatalogServices_NodeMetaFilter(t *testing.T) {
})
}

func TestAPI_CatalogServices_NodeMetaViaFilter(t *testing.T) {
absolutelightning marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()
meta := map[string]string{"somekey": "somevalue", "synthetic": "true"}
c, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) {
conf.NodeMeta = meta
conf.NodeName = "foobar"
absolutelightning marked this conversation as resolved.
Show resolved Hide resolved
})
defer s.Stop()

catalog := c.Catalog()
retry.Run(t, func(r *retry.R) {
absolutelightning marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down