Envoy: filterChainMatch: add support for source IPs and ports

[junr03]

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.

the idea behind putting it last is that then you still can do fancy selection of the destination, based on what’s available now and then do “horizon split” if you want to treat some clients differently.

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.

[junr03]

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.

[cmluciano]

@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.

[junr03]

@cmluciano fantastic! Please tag me on the PR once it is up, and I can review. Mind if I assign you the issue?

[cmluciano]

SGTM @junr03

[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?

[PiotrSikora]

That's more of a question to @htuch, who authored the proto, but it looks that the single destination_port was driven by use_original_dst (see: https://github.com/envoyproxy/data-plane-api/pull/143), while the repeated source_ports were driven by the capabilities of v1 tcp proxy (see: https://github.com/envoyproxy/data-plane-api/pull/49).

Do we have any users of v1 tcp proxy that use multiple ports?

[cmluciano]

I can potentially create a new field that is just source_port & mark the source_ports for deprecation if that works.

[htuch]

@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.

[PiotrSikora]

@htuch I guess this is more about unwanted asymmetry between similar features, i.e. perhaps adding repeated destination_ports (and eventually deprecating singleton destination_port) would be a way to go about this?

[htuch]

@PiotrSikora sure. You can actually safely upgrade a singleton to repeated without breaking wire compatibility.

[PiotrSikora]

@htuch but then we have repeated destination_port, which doesn't follow API conventions.

[htuch]

@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.

[cmluciano]

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 source_port or perhaps create that as a new field and mark source_ports for deprecation?

[htuch]

@cmluciano I guess what I want to know now is why we care about source port at all? Is anyone using this feature?

[cmluciano]

@htuch Ah! I suppose that IS the question. I'll await more answers here / followup on #4682 before continuing

[stale[bot]]

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.

[mattklein123]

I think source IP is in progress?

[cmluciano]

/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.

[htuch]

@cmluciano don't we need this to get rid of the TCP proxy v1 field?

[cmluciano]

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.

[htuch]

Yep, I think we need to do this reasonably soon, CC @alyssawilk

[mattklein123]

I will implement this in the next few days.