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
filterChainMatch: add support for source IPs and ports #4457
Comments
Will leave unassigned for now. I'll try and get something from the team at Lyft to work on this, but if a member of the OSS community wants to work on this I am happy to help. |
@junr03 Not sure if you found anyone for this yet, but I'm testing a commit locally for this now. Will push up if tests pass. |
@cmluciano fantastic! Please tag me on the PR once it is up, and I can review. Mind if I assign you the issue? |
SGTM @junr03 |
@PiotrSikora I failed to update this issue with a question that I had, and @cmluciano just reminded me of it. In the original V1 tcp proxy config both source and destination ports allowed for multiple ports https://www.envoyproxy.io/docs/envoy/latest/api-v1/network_filters/tcp_proxy_filter#route. However, the filter change match proto has repeated field for destination ports but a single uint32 for the source port. Was this intended? |
That's more of a question to @htuch, who authored the proto, but it looks that the single Do we have any users of v1 tcp proxy that use multiple ports? |
I can potentially create a new field that is just source_port & mark the source_ports for deprecation if that works. |
@cmluciano is there a good reason for this API churn? @PiotrSikora is correct about how this came to be in the v2 API, but I'm not sure there is much benefit from turning this back to a singleton. |
@htuch I guess this is more about unwanted asymmetry between similar features, i.e. perhaps adding repeated |
@PiotrSikora sure. You can actually safely upgrade a singleton to repeated without breaking wire compatibility. |
@htuch but then we have repeated |
@PiotrSikora I don't mind that much. Nobody is using these features right now, so unless there is a compelling case to make it more complicated, just make them singletons. |
@htuch To confirm, are you saying it's ok for me to drop repeated from the source_ports? Also should we rename it to be |
@cmluciano I guess what I want to know now is why we care about source port at all? Is anyone using this feature? |
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions. |
I think source IP is in progress? |
/unassign @cmluciano Unfortunately I'm not sure if anyone still has a use for this issue. unassigning myself in case anyone wants to clean up the PR I had started before. |
@cmluciano don't we need this to get rid of the TCP proxy |
Ah somehow I missed the attached issue with the deprecated line. I've been trying to get UDP plumbed through the upstream pieces of the codebase, but I can table that if this is higher priority. |
Yep, I think we need to do this reasonably soon, CC @alyssawilk |
I will implement this in the next few days. |
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>
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>
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 envoyproxy/envoy#4457 Signed-off-by: Matt Klein <mklein@lyft.com> Mirrored from https://github.com/envoyproxy/envoy @ 866d0438d12ce9d39afb35ba200b0107bf3d6de3
filterChainMatch: add support for source IPs and ports
Description:
There exist fields for this type of matching in the FilterHeadMatch proto already, they just have not been implemented. The implementation should be straightforward, as it should be very similar to the destination implementation.
As far as ordering @PiotrSikora and I were chatting, and believe that this should be added as the last matching criteria.
Relevant to this issue is the fact that v1 tcp proxy filter supports source port and ips based routing, and in order for a v2 tcp proxy filter to support the same expressiveness we need the filter match criteria to support source based matching.
The text was updated successfully, but these errors were encountered: