filter chain match: support source CIDRs and ports #7064
Conversation
This PR also fully deprecates the tcp_proxy v1 configuration. This will be deleted following the standard deprecation cycle. All new uses should use filter chain matching. Fixes #4457 Signed-off-by: Matt Klein <mklein@lyft.com>
|
@PiotrSikora @htuch PTAL @htuch This PR contains a fix for the docs build on master. Feel free to pull it out into a different PR in the morning if you want since this might take a few iterations. |
Signed-off-by: Matt Klein <mklein@lyft.com>
| @@ -64,6 +64,8 @@ message Filter { | |||
| // 4. Transport protocol. | |||
| // 5. Application protocols (e.g. ALPN for TLS protocol). | |||
| // 6. Source type (e.g. any, local or external network). | |||
| // 7. Source IP address. | |||
There was a problem hiding this comment.
Can you comment on the rationale behind the ordering here? Is it arbitrary? Is there a practical reason to select source IP/port as the LSBs?
There was a problem hiding this comment.
I talked to @PiotrSikora about this offline. Effectively, it's arbitrary, and since we are adding new ones it seems best to add them at the end? IMO this needs to be configurable but that's out of scope for this change. @PiotrSikora any additional thoughts here?
There was a problem hiding this comment.
It's not really arbitrary. The order is trying to more and more precisely describe destination (what service are you connecting to) with source selectors added at the end for the "split horizon".
This order allows filter chains to effectively act as virtual servers with fancy selectors (transport protocols, application protocols, etc.), while still being able to configure "split horizon" on a per virtual server basis for those that need it.
If source selectors had priority, then "split horizon" would be at the top of the decision tree, requiring all virtual servers to be configured for each source.
The only thing that I'm not 100% sure about is the order between source IPs and ports... Thinking about this now, perhaps it should be reversed (source port with higher priority) to allow matching connections from port X from any IP, but I honestly cannot think of a "real" use case for source port matching anyway.
There is an open issue for making the order configurable (#3411), but in the year+ that this order has been in place, I don't recall anyone complaining that the existing order doesn't work for their use case.
There was a problem hiding this comment.
Yeah I don't have a strong opinion on the order of IP vs. port. Perhaps ship and iterate? I'm fine either way.
There was a problem hiding this comment.
Thanks for the explanation @PiotrSikora. I wonder if we should capture this somewhere, it's useful design rationale.
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
| @@ -64,6 +64,8 @@ message Filter { | |||
| // 4. Transport protocol. | |||
| // 5. Application protocols (e.g. ALPN for TLS protocol). | |||
| // 6. Source type (e.g. any, local or external network). | |||
| // 7. Source IP address. | |||
There was a problem hiding this comment.
It's not really arbitrary. The order is trying to more and more precisely describe destination (what service are you connecting to) with source selectors added at the end for the "split horizon".
This order allows filter chains to effectively act as virtual servers with fancy selectors (transport protocols, application protocols, etc.), while still being able to configure "split horizon" on a per virtual server basis for those that need it.
If source selectors had priority, then "split horizon" would be at the top of the decision tree, requiring all virtual servers to be configured for each source.
The only thing that I'm not 100% sure about is the order between source IPs and ports... Thinking about this now, perhaps it should be reversed (source port with higher priority) to allow matching connections from port X from any IP, but I honestly cannot think of a "real" use case for source port matching anyway.
There is an open issue for making the order configurable (#3411), but in the year+ that this order has been in place, I don't recall anyone complaining that the existing order doesn't work for their use case.
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@PiotrSikora thanks for the review. Updated. |
This PR also fully deprecates the tcp_proxy v1 configuration.
This will be deleted following the standard deprecation cycle.
All new uses should use filter chain matching.
Fixes #4457
Risk Level: Medium
Testing: New tests
Docs Changes: Added
Release Notes: Added
Deprecation Notes: Added