tcp filter: Allow specifying metadata matcher for upstream cluster#2708
tcp filter: Allow specifying metadata matcher for upstream cluster#2708alyssawilk merged 6 commits intoenvoyproxy:masterfrom
Conversation
|
@zuercher here's the initial PR. I wasn't sure if straight up depending on the router from the tcp filter was the right call here, so happy to do any additional refactoring if necessary |
|
@zuercher for review, @alyssawilk for merge. |
There was a problem hiding this comment.
No need to check this pointer. Just "return cluster_metadata_match_criteria_.get();" which will return nullptr if the unique_ptr is null
There was a problem hiding this comment.
config_ can be nullptr, but not in this case, because WsHandlerImpl overrides this method. Please add a comment explaining this, or optionally just add a null-check on config_
There was a problem hiding this comment.
i added a null check because it feels safer (instead of relying on some invariant never changing), but curious how WsHandlerImpl comes into play here. is it because config_ is only null for WsHandlerImpl?
There was a problem hiding this comment.
Very reasonable to just add the null check.
WsHandlerImpl sub-classes TcpProxy, and uses it with a null config_. This is the only case the config is null. I have a TODO to fix that (require non-null config to simplify code).
|
@snowp can you resolve merge conflict? Thanks. |
This implements the `metadata_match` config for the tcp filter, which allows users to specify a metadata criteria that restricts which endpoints should be considered when connecting to an upstream cluster. Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
Resolved the merge conflicts, but looks like the rebase dismissed the reviews :( |
|
The github UI assumes changed code addresses comments by default. The comments are still there and should be visible if you click "show outdated" but I often just refer to the comments in my email to make sure I didn't miss any :-) |
|
Oh okay that makes sense. I think this PR is ready to merge now, nothing changed since I got the approvals, they just dropped off when I resolved the merge conflict. |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looks great - thanks for the addition!
|
Either of us can merge once you're resolved the merge conflict. If it helps reduce conflicts I don't think the release notes need to stay the bottom line :-) |
|
@alyssawilk This should be good to merge now! |
* Re-enable RBE. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: use only RBE cache. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: Kick CI. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: JP Simard <jp@jpsim.com>
5 participants
Description:
This implements
metadata_matchfor the tcp filter, whichwill allow users to specify a metadata criteria that specifies what
endpoints should be considered selecting an upstream connection to the cluster.
Risk Level: Low
Small optional feature
Testing:
Added a unit test that verifies that TcpFilter#metadataMatchCriteria returns the criteria specified in the config.
Release Notes:
Added support for specifying a metadata matcher for upstream clusters in the tcp filter
Fixes #2696
API Changes
envoyproxy/data-plane-api#520