Streaming filter tags + case insensitive lookups for Service Names #9703
Conversation
|
🤔 Double check that this PR does not require a changelog entry in the .changelog directory. Reference |
pierresouchay
force-pushed
the
streaming_tags_and_case_insensitive
branch
from
047f0a6 to
7a024ed
Compare
dnephin
added
theme/streaming
There was a problem hiding this comment.
Thank you for the PR! This looks great.
I think the fix for case insensitivity is exactly right. I'm going to have a closer look at the tests, and also see if we can use https://github.com/hashicorp/go-bexpr to implement the filtering. I need to look at how this relates to the filtering we do in the cache right now. Filtering in a separate place might be ok if the tag field are not part of the cache key.
I'll work on getting this merged early next week.
|
We already deployed this on our clusters: https://github.com/criteo-forks/consul/commits/1.9.3-criteo seems to work well for now |
There was a problem hiding this comment.
Thanks! Looking good, couple minor things I noticed.
I attempted to use the bexpr filters here: e2475cc, but I ran into a problem with the ServiceTags filtering, because bexpr is not case insensitive. I'll see if there is a way to make that work tomorrow. Otherwise I think copying the existing code from the server as you have done here makes sense.
Send empty array [] instead of [""] in DNS requests when TagFilter is not set Do not change case sensitivity of services anymore in `getServiceNodes()` since cache keys are now case insensitive
|
@dnephin All done |
So that all the client side filtering is in the same place. Previously only the bexpr filter was in the cache-entry. Also makes a small change to the filtering so that instead of rebuilding slices of items, the filtering can return a bool to determine if the event payload is saved or not.
There was a problem hiding this comment.
Looks great! Thank you!
I pushed one commit which moves the filtering to where we call the current req.Filter in the cache-type. With this change we keep all the filtering in the same place, and we can do fewer allocations since the values get filtered before we store them. I added some tests that I had written while experimenting with the expr option.
I'm going to ask someone to look over the changes I've made tomorrow, but I think this is ready to merge.
|
Deployment failed with the following error: |
| } | ||
|
|
||
| func serviceHasTag(sn *structs.NodeService, tag string) bool { | ||
| for _, t := range sn.Tags { |
There was a problem hiding this comment.
I'm pretty sure the answer is no, but do we ever need to handle the backcompat case where sn.Tag is set? I think not since this would have come from Consul's state store and every CSN from a version recent enough to support streaming would only use the plural field right?
There was a problem hiding this comment.
That case is handled in this PR, here: https://github.com/hashicorp/consul/pull/9703/files/d5cc206e3963ddd910ee34b8612658d85f370d38#diff-97d0de9c08776c4ff05f75254d7319f757e5b56f78b428d0b2574ee43b77bb9dR210
This is the same logic that the server users today (ignore any Tags if Tag is set).
https://github.com/hashicorp/consul/blob/master/agent/consul/health_endpoint.go#L296-L305
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/332802. |
Fixes #9695
Fixes #9702
This is also testing the 2 cases at once since I modified the DNS test: