-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Implement filtering of some endpoints #5579
Conversation
It looks cool, I like it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great and powerful feature, thank you!
I only read from my phone, so I probably missed it, but I did not see how you will handle cached results (since I don't think the filter is used by the key used to compute the hash of cached requests)
@pierresouchay You didn't miss it. I just haven't handled that yet. There are a couple of ways I am thinking about handling the cache. Option 1, is to just make the filter part of the cache key. This will just do what is expected in all cases. Option 2, would be to make unfiltered RPC requests to the servers and then execute the filter on the results returned from the cache. Multiple different queries that use the same RPC could then use the same raw results and the local agent could filter for each request independently. This would also have a benefit of moving some of the CPU load for the filtering from the servers to the edges. The downside is that if you have a ton of data coming back through that RPC the filtering will not reduce the bandwidth necessary to transmit the request. I think for now Option 1 is where I am going to take this. We already take the tag/meta filter into account for the cache key so it would make sense to do the same for this. |
Yes, even with crazy scenarii like I was trying to think of (several clients using cache with different filtering criteria), I think approach 1 is pragmatic for now and make sense regarding the existing codebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level this is looking awesome @mkeeler! 🎉
I agree that adding filter to to the Cache request type makes total sense for now.
The TODO list also looks perfect - I'll hold off approving until more of that is done but this is looking great to me so far!
This test also exposed a problem in go-bexpr which I fixed and this pulls in the updated lib.
Refactored the summarizeServices test to do this into two subtests.
Co-Authored-By: mkeeler <mkeeler@users.noreply.github.com>
**Command - Unfiltered** | ||
|
||
```sh | ||
curl -X GET localhost:8500/v1/catalog/service/api-internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be clearer to settle on either -G
or -X GET
but not a random mix of both in the examples provided here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-G
and -X GET
are not functionally equivalent.
From the man pages:
For -G
When used, this option will make all data specified with -d, --data, --data-binary or
--data-urlencode to be used in an HTTP GET request instead of the POST request that
otherwise would be used. The data will be appended to the URL with a '?' separator.
For -X
This option only changes the actual word used in the HTTP request, it does not alter
the way curl behaves. So for example if you want to make a proper HEAD request, using
-X HEAD will not suffice. You need to use the -I, --head option.
In general we could omit the -X GET
as it will do a GET by default. However after talking with @kaitlincarter-hc the consensus was that to be as consistent with the rest of our docs we should have it in there. For all the queries where
--data-urlencode
is used, curl would by default make it a POST request and put the data in the body of the request. Using -X GET
makes it a GET request but the filter is then still transmitted in the body. Using -G
forces curl to shove the filter in as a query param while still performing the urlencoding.
This seems like a deficiency with curl not being able to urlencode query params easier but its all we have to work with right now.
Also of note is that in all the cases where we are using -X GET
we could use -G
so we could standardize on that here. @kaitlincarter-hc what do you think?
Co-Authored-By: mkeeler <mkeeler@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs turned out really well, nice work!
Fixes: #4222
Data Filtering
This PR will implement filtering for the following endpoints:
Supported HTTP Endpoints
/agent/checks
/agent/services
/catalog/nodes
/catalog/service/:service
/catalog/connect/:service
/catalog/node/:node
/health/node/:node
/health/checks/:service
/health/service/:service
/health/connect/:service
/health/state/:state
/internal/ui/nodes
/internal/ui/services
More can be added going forward and any endpoint which is used to list some data is a good candidate.
Usage
When using the HTTP API a
filter
query parameter can be used to pass a filter expression to Consul. Filter Expressions take the general form of:Normal boolean logic and precedence is supported. All of the actual filtering and evaluation logic is coming from the go-bexpr library
Other changes
Adding the
Internal.ServiceDump
RPC endpoint. This will allow the UI to filter services better.Remaining Tasks:
/agent/checks
/agent/services
/catalog/nodes
/catalog/service/:service
/catalog/connect/:service
/catalog/node/:node
/health/node/:node
/health/checks/:service
/health/service/:service
/health/connect/:service
/health/state/:state
/internal/ui/nodes
/internal/ui/services
Catalog.ListNodes
RPCCatalog.ServiceNodes
RPCCatalog.NodeServices
RPCHealth.NodeChecks
RPCHealth.ServiceChecks
RPCHealth.ServiceNodes
RPCHealth.ChecksInState
RPCInternal.NodeDump
RPCInternal.ServiceDump
RPCconsul catalog
cli.Make HTTP Options request dump a JSON payload of the allowed fields and types (Optional)Not going to happen for the initial release.