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 hostnames to TCPRoute and UDPRoute #727

Closed
robscott opened this issue Jul 20, 2021 · 11 comments
Closed

Add hostnames to TCPRoute and UDPRoute #727

robscott opened this issue Jul 20, 2021 · 11 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@robscott
Copy link
Member

What would you like to be added:
A new hostnames field for TCPRoute and UDPRoute to match the corresponding fields in HTTPRoute and TLSRoute.

Why is this needed:
Although hostname is not particularly meaningful for TCP and UDP, it can be useful when attaching other concepts to these routes. For example, hostnames could be useful for policy attachment as described by GEP #713. This change would also be helpful for DNS implementations such as external-dns: kubernetes-sigs/external-dns#2045 (comment).

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

This also will be helpful for the scenario when gateways receives TLS traffic, terminates it on it and then forwards TCP/UDP traffic based on SNI. Currently, there is no way to route using SNI field on TCP/UDP routes besides making a new listener per hostname for TCP/UDP routes like so:

apiVersion: networking.x-k8s.io/v1alpha1
kind: TCPRoute
metadata:
  name: redis
spec:
  rules:
    - forwardTo:
        serviceName: redis
        port: 6379
---
apiVersion: networking.x-k8s.io/v1alpha1
kind: TCPRoute
metadata:
  name: mysql
spec:
  rules:
    - forwardTo:
        serviceName: mysql
        port: 3306
---
kind: Gateway
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
  name: ingress
spec:
  gatewayClassName: acme-lb
  listeners:
    - protocol: TLS
      port: 443
      hostname: redis.example.com
      tls:
        certificateRef:
          kind: Secret
          group: core
          name: example-com-cert
      routes:
        kind: TCPRoute
        selector:
          matchLabels:
            name: redis
    - protocol: TLS
      port: 443
      hostname: mysql.example.com
      tls:
        certificateRef:
          kind: Secret
          group: core
          name: example-com-cert
      routes:
        kind: TCPRoute
        selector:
          matchLabels:
            name: mysql

If we are able to set hostnames on TCP/UDP routes will can rewrite it like this:

apiVersion: networking.x-k8s.io/v1alpha1
kind: TLSRoute
metadata:
  name: redis
spec:
  rules:
    - match:
        hostnames: ["redis.example.com"] // or snis
      forwardTo:
        serviceName: redis
        port: 6379
---
apiVersion: networking.x-k8s.io/v1alpha1
kind: TLSRoute
metadata:
  name: mysql
spec:
  rules:
    - match:
        hostnames: ["mysql.example.com"] // or snis
      forwardTo:
        serviceName: mysql
        port: 3306
---
kind: Gateway
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
  name: ingress
spec:
  gatewayClassName: acme-lb
  listeners:
    - protocol: TLS
      port: 443
      hostname: *.example.com
      tls:
        certificateRef:
          kind: Secret
          group: core
          name: example-com-cert
      routes:
        kind: TCPRoute

@jpeach
Copy link
Contributor

jpeach commented Jul 21, 2021

This also will be helpful for the scenario when gateways receives TLS traffic, terminates it on it and then forwards TCP/UDP traffic based on SNI.

Routing based on SNI should be done using TLSRoute.

@jpeach
Copy link
Contributor

jpeach commented Jul 21, 2021

What would you like to be added:
A new hostnames field for TCPRoute and UDPRoute to match the corresponding fields in HTTPRoute and TLSRoute.

In the case of HTTPRoute and TLSRoute, the hostname are match criteria for the rules. In this case they would not be, so we would have to add them in some distinct field. This probably weakens the consistency of the approach.

For external DNS, the Gateway is the object that owns the address, so that's probably also the best authority for what hostnames ought to be registered. As a starting points, external DNS could register any precise listener hostnames on the Gateway as well as any hostnames specified by annotations on the Gateway, leaving it to other controllers to propagate hostnames from route types up to the annotations.

@howardjohn
Copy link
Contributor

In the case of HTTPRoute and TLSRoute, the hostname are match criteria for the rules. In this case they would not be, so we would have to add them in some distinct field. This probably weakens the consistency of the approach.

I think there are legitimate use cases for matching a hostname for TCP/UDP, especially for transparent proxies. In a typical Ingress proxy, you don't really care about the destination IP address of the connection. However, for some cases you do, because that is the intended destination of the request and it was passed through (through various means) our gateway controller without modifying the destination. In this way, we can do a hostname match on the IP address. That is, the controller/proxy has some mapping of hostname (from TCPRoute) -> IP (to match the actual request), and can match that way. We are doing this today

@jpeach
Copy link
Contributor

jpeach commented Jul 21, 2021

In the case of HTTPRoute and TLSRoute, the hostname are match criteria for the rules. In this case they would not be, so we would have to add them in some distinct field. This probably weakens the consistency of the approach.

I think there are legitimate use cases for matching a hostname for TCP/UDP, especially for transparent proxies. In a typical Ingress proxy, you don't really care about the destination IP address of the connection. However, for some cases you do, because that is the intended destination of the request and it was passed through (through various means) our gateway controller without modifying the destination. In this way, we can do a hostname match on the IP address. That is, the controller/proxy has some mapping of hostname (from TCPRoute) -> IP (to match the actual request), and can match that way. We are doing this today

So in this case the gateway listens on many IP addresses (implicitly or explicitly) and you want to select the route from an IP address match? But you don't want to set up multiple listeners for each IP address, maybe because there's a lot or there's not a static set of addresses?

@howardjohn
Copy link
Contributor

So in this case the gateway listens on many IP addresses (implicitly or explicitly) and you want to select the route from an IP address match? But you don't want to set up multiple listeners for each IP address, maybe because there's a lot or there's not a static set of addresses?

Yep, pretty much. I think the common case here is mesh (although I can think of some other theoretical use cases). Where you want to say things like "TCPRoute for all traffic to mysql.ns.svc.cluster.local". We don't ever want to actually put an IP address that we will match against in the API, since having a (DNS, in this case) name is nice, even though the actual proxy will be doing an IP match under the hood.

@jpeach
Copy link
Contributor

jpeach commented Jul 22, 2021

So in this case the gateway listens on many IP addresses (implicitly or explicitly) and you want to select the route from an IP address match? But you don't want to set up multiple listeners for each IP address, maybe because there's a lot or there's not a static set of addresses?

Yep, pretty much. I think the common case here is mesh (although I can think of some other theoretical use cases). Where you want to say things like "TCPRoute for all traffic to mysql.ns.svc.cluster.local". We don't ever want to actually put an IP address that we will match against in the API, since having a (DNS, in this case) name is nice, even though the actual proxy will be doing an IP match under the hood.

I guess my worry about this is that it exposes a top-level configuration field that can't work for a lot of implementations. It would have to be a Implementation-specific support level field, and maybe also deployment-specific.

@hbagdi
Copy link
Contributor

hbagdi commented Jul 26, 2021

I guess my worry about this is that it exposes a top-level configuration field that can't work for a lot of implementations. It would have to be a Implementation-specific support level field, and maybe also deployment-specific.

I share this concern plus the following:

  • up until this point, we have focused on matching semantics based on metadata present in the protocol itself. This feature muddies that. Is there a possibility of a separate route for this purpose?
  • I suspect that most users will get confused when they read hostname in a TCPRoute.

That is, the controller/proxy has some mapping of hostname (from TCPRoute) -> IP (to match the actual request), and can match that way. We are doing this today

So this would work hand in hand with #735 to define the map itself?

@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 24, 2021
@robscott
Copy link
Member Author

Although this may still have value in the future, I don't think it's on our near term roadmap. On the other hand, #735 has gotten more traction and overlaps slightly with this one. Going to close this for now, but feel free to reopen if you have a use case.

/close

@k8s-ci-robot
Copy link
Contributor

@robscott: Closing this issue.

In response to this:

Although this may still have value in the future, I don't think it's on our near term roadmap. On the other hand, #735 has gotten more traction and overlaps slightly with this one. Going to close this for now, but feel free to reopen if you have a use case.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

7 participants