Skip to content
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

Add Address Matching to TCPRoute and UDPRoute #735

Closed
robscott opened this issue Jul 23, 2021 · 23 comments · Fixed by #932
Closed

Add Address Matching to TCPRoute and UDPRoute #735

robscott opened this issue Jul 23, 2021 · 23 comments · Fixed by #932
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Milestone

Comments

@robscott
Copy link
Member

robscott commented Jul 23, 2021

What would you like to be added:
Add address matching to TCPRouteMatch and UDPRouteMatch.

Why this is needed:
TCP and UDP Routes currently only support custom matching extensions. It would be helpful to include some basic matching capabilities within the Routes. Address matching seems like a natural starting point. Although for most ingress use cases this could be covered by Gateway listeners, this concept could be quite helpful for mesh routing.

Also, see transparent proxy discussion in #727

@robscott robscott added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 23, 2021
@jpeach
Copy link
Contributor

jpeach commented Jul 25, 2021

In the case of addresses, whether matching is useful depends on having a listener that is implicitly attached to multiple addresses. The system has out-of-band knowledge of address semantics, so you can know that you want a route attached to address A and not to address B.

Can we make a similar argument for ports?

@robscott
Copy link
Member Author

I may be misunderstanding what you're saying here, but either or both of the following scenarios are entirely possible:

  • Gateways listen on multiple addresses, a Route only wants to be attached to one of them
  • Gateways listen on multiple ports, a Route only wants to be attached to one of them

Although both of the above are possible and this addition would help, I think this addition becomes significantly more compelling when you take into account mesh implementations that may want to match requests to a given address or port and apply some kind of routing to them.

@hbagdi
Copy link
Contributor

hbagdi commented Jul 26, 2021

Add Address and Port to TCPRouteMatch and UDPRouteMatch.

@robscott Source or destination or both?

The following 5 things identify a byte stream on a TCP/IP networking stack:

  • Source IP
  • Source Port
  • Destination IP
  • Destination Port
  • Protocol

We currently capture destination port via listener and the protocol. One could argue that the destination IP is also captured indirectly via the relationship Gateway -> Listener -> Route.
Source is not captured anywhere.

+1 to this feature. One place to define matching, rather than ensuring that a route is mapped onto the correct listener would help and make the API more user-friendly.

In the case of addresses, whether matching is useful depends on having a listener that is implicitly attached to multiple addresses.

What if the matching is based on source IP/port instead of destination IP/port?

The system has out-of-band knowledge of address semantics, so you can know that you want a route attached to address A and not to address B.

I'm not sure I understand what is the point you are trying to make here. Could you rephrase please?

@jpeach, are you trying to say that a route should not be aware of these addresses/ports because it is a leaky abstraction in that case? While it could be viewed as that, I think the same argument could be applied to the Hostname field in HTTPRoute or TLSRoute.

@youngnick
Copy link
Contributor

So, this essentially creates a similar two-directional match to the hostname field on HTTPRoute, but for address and port? That makes sense to me, with the caveat that we definitely need to be clear about what happens if things don't match.

Alternatively, matching on source address and port seems much more likely to be useful.

@robscott
Copy link
Member Author

Good point @hbagdi! I should have clarified that I was intending to match destination IP and Port with these fields. Given the potential for source IP and port matching in the future, it does probably make sense to make the config explicit here. Maybe something like this:

matches:
  - destination:
       ip: 1.2.3.4
       port: 80

this essentially creates a similar two-directional match to the hostname field on HTTPRoute, but for address and port?

That's a great way to summarize this, thanks @youngnick!

@hbagdi
Copy link
Contributor

hbagdi commented Jul 26, 2021

Given the potential for source IP and port matching in the future, it does probably make sense to make the config explicit here.

What is your hesitation to include source IP/port in the API right away?

@robscott
Copy link
Member Author

What is your hesitation to include source IP/port in the API right away?

I actually haven't thought about this as much. This particular idea/feature request was targeted at dest IP and Port despite me not actually saying that :). At first glance, it seems like source IP/Port matching may have slightly less portability, but I'd need to look into it a bit more.

@hbagdi
Copy link
Contributor

hbagdi commented Jul 26, 2021

I actually haven't thought about this as much. This particular idea/feature request was targeted at dest IP and Port despite me not actually saying that :)

Ack.

At first glance, it seems like source IP/Port matching may have slightly less portability, but I'd need to look into it a bit more.

If portability is a concern, maybe we start with a different conformance level for it?
My motivation to add in source is to add a complete feature that takes both the ends into account rather than only one.

Also, regarding destination port/ip based match, we will need to clarify how Proxy Protocol comes into play.

@jpeach
Copy link
Contributor

jpeach commented Jul 26, 2021

@jpeach, are you trying to say that a route should not be aware of these addresses/ports because it is a leaky abstraction in that case? While it could be viewed as that, I think the same argument could be applied to the Hostname field in HTTPRoute or TLSRoute.

A route can say that it wants to only match on a specific IP address because there is some out-of-band semantics attached to that address. But what does it mean for a route to only want to be exposed on a specific port, and why would you do that? For example, I have a route and I specify a match for port "21". How is that useful? Are we talking about another programmatic mapping done by a transparent proxy?

@jpeach
Copy link
Contributor

jpeach commented Jul 26, 2021

Also, regarding destination port/ip based match, we will need to clarify how Proxy Protocol comes into play.

Did you mean source address matching?

@howardjohn
Copy link
Contributor

I am not sure why port match is any less useful than SNI, hostname, ip, etc. Its just another attribute of the request. For example, maybe I want to match anything on port 22 and deny it or apply some policy.

@jpeach
Copy link
Contributor

jpeach commented Jul 26, 2021

I am not sure why port match is any less useful than SNI, hostname, ip, etc. Its just another attribute of the request. For example, maybe I want to match anything on port 22 and deny it or apply some policy.

My question is why I would care about the port. If I match on a port, then that reduces deployment agility for the gateway owner. It's extremely common to run services on "non-standard" ports.

@howardjohn
Copy link
Contributor

My question is why I would care about the port. If I match on a port, then that reduces deployment agility for the gateway owner. It's extremely common to run services on "non-standard" ports.

So does hostname. There is always a tension between generic routes attached to concrete gateways vs concrete routes attached to generate gateways. I think both are valid, they just depend on the use case.

@hbagdi
Copy link
Contributor

hbagdi commented Jul 26, 2021

Did you mean source address matching?

Both? Proxy protocol carries source and destination IPs involved, no?

I think the tension comes in from the ownership perspective. Who owns and decides ports, IPs, DNS names, hostnames.
We have use-cases where this control resides with Cluster Admin for the most part. But, there are some use-cases where the application developer could also be influencing these decisions, with some allow/deny from the Cluster Admin (who is also the Gateway owner in our model).

@jpeach
Copy link
Contributor

jpeach commented Jul 26, 2021

Did you mean source address matching?

Both? Proxy protocol carries source and destination IPs involved, no?

Yep you are right :)

@beriberikix
Copy link
Contributor

+1 to considering destination ports like any other attribute. Being able to assign a non-standard dest port to a service (or user) is sometimes needed when dealing with L4 protocols. That may be because there isn't a hostname, or the form of security can't be used in that way. Agree with the agility concern but the cases I'm thinking look like @hbagdi's scenario (cluster admin ~= app dev)!

@jpeach
Copy link
Contributor

jpeach commented Jul 27, 2021

+1 to considering destination ports like any other attribute. Being able to assign a non-standard dest port to a service (or user) is sometimes needed when dealing with L4 protocols.

You can do this today with a gateway listener on the port you want that binds a UDP or TCP route.

@youngnick
Copy link
Contributor

So, it seems like there are two things we're talking about here:

  • adding fields to hold destination IP and Port on TCPRoute and UDPRoute. This would create a two-level matching scheme, similar to hostname at the Gateway level being required to match a hostname supplied at the HTTPRoute or TLSRoute level for the route to successfully be attached. If that's the case, I can see that it might be useful.
  • adding fields to hold source IP and Port on TCPRoute and UDPRoute. In this case the details are more like a route discriminator, in that you might have multiple TCPRoutes on a Listener's Port that routed to different backends based on the Source IP or Port. To me, this fits squarely in the purpose of TCPRoute and UDPRoute, although I'm not sure if support could be core? Is being able to route to different destinations based on source address/port common on L4 load balancers? It has been a long time since I configured one. :)

tl;dr putting Destination address/port feels like a way to match Routes to individual Listeners, while Source address/port is another form of routing discriminator that lets you have multiple Routes share an individual Listener.

@howardjohn
Copy link
Contributor

tl;dr putting Destination address/port feels like a way to match Routes to individual Listeners, while Source address/port is another form of routing discriminator that lets you have multiple Routes share an individual Listener.

This sounds right to me.

Is being able to route to different destinations based on source address/port common on L4 load balancers?

I have limited expertise here, but it seems more common with applying policy (allowlist/denylist IP) than routing.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2021
@robscott
Copy link
Member Author

I believe @shaneutt is working on a proposal for this.

/assign @shaneutt
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 27, 2021
@robscott robscott added this to the v1beta1 milestone Nov 10, 2021
@robscott
Copy link
Member Author

robscott commented Dec 6, 2021

@shaneutt is adding support for source and dest address matching with GEP #735, I'm going to pull port out to another tracking issue so this one is more clearly aligned.

@robscott robscott changed the title Add Address and Port Matching to TCPRoute and UDPRoute Add Address Matching to TCPRoute and UDPRoute Dec 6, 2021
@rainest
Copy link
Contributor

rainest commented Mar 24, 2022

For anyone else that wound up here looking for the continued port matching discussion, it resulted in #1021 and GEP-957.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants