WIP - Dynamically configurable Tap filter#7194
WIP - Dynamically configurable Tap filter#7194yuval-k wants to merge 13 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
signals to the backend that streaming is done for a trace
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for working on this. This is going to be awesome. Can we please start with a full cleanup of the APIs with docs, etc. and then we can go from there? Thank you!
/wait
| // Common configuration for all tap extensions. | ||
| message CommonExtensionConfig { | ||
|
|
||
| message TDSConfig { |
There was a problem hiding this comment.
Note that @htuch has either taken "TDS" for the runTime Discovery Service. I think we are at the limits of our 3-letter acronyms. Can you maybe just call it "TapDS" or something?
| // if trace is streaming, this may only be sent once. | ||
| Destination destination = 5; | ||
|
|
||
| envoy.api.v2.core.Address downstream_remote_address = 6; |
There was a problem hiding this comment.
This may not always apply if the tap is for an upstream connection. Can we future proof with this with maybe a message covering downstream connection info, upstream connection info, etc.?
| message TraceWrapper { | ||
| message Destination { | ||
| string cluster_name = 1; | ||
| google.protobuf.Struct cluster_metadata = 2; |
There was a problem hiding this comment.
i added more callbacks to HttpPerRequestTapper to support this. cluster_metadata is all the meatadata attached to the cluster under the filter's name
| // a tap will occur and the data will be written to the configured output. | ||
| OutputConfig output_config = 2 [(validate.rules).message.required = true]; | ||
|
|
||
| envoy.api.v2.core.RuntimeFractionalPercent filter_enabled = 3; |
There was a problem hiding this comment.
Docs? What is this doing specifically? Name?
There was a problem hiding this comment.
similar to other filters, it allows to gradually enable the filter
i.e the % of requests for which the filter a match is even attempted.
| // HTTP response trailers match configuration. | ||
| HttpHeadersMatch http_response_trailers_match = 8; | ||
|
|
||
| ClusterMatch destination_cluster_match = 9; |
There was a problem hiding this comment.
How is this populated/matched?
There was a problem hiding this comment.
right now it checks that the destination cluster name is one of those defined in ClusterMatch.
I can change ClusterMatch to have a StringMatcher for more flexibility in matching.
My thinking here was to allow limiting taps to specific services.
|
@mattklein123 should I start a new PR with just the APIs? or change this one? |
|
@yuval-k yeah let's just do an API PR and nail that down. That would be great. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
A draft PR for making the tap filter more dynamic. This is PR is not intended to be merged - it is intended for a high-level review, as the features will be broken down via individual small PRs.
The features are covered here are:
Risk Level: Low-Medium (mostly new opt-in functionality; no core changes)
Testing: Will be added in the per feature PRs.
Part of #6110