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

Conflicting SNI's between HTTPRoute & TLSRoute #623

Open
Tracked by #3165
stevesloka opened this issue Apr 22, 2021 · 22 comments
Open
Tracked by #3165

Conflicting SNI's between HTTPRoute & TLSRoute #623

stevesloka opened this issue Apr 22, 2021 · 22 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@stevesloka
Copy link
Contributor

What happened:
If I have the same fqdn defined in both an HTTPRoute as well as a TLSRoute, what could the conflict resolution be in that case?

The current docs on a TLSRouteMatch sort of lead me to think that TLSRoutes should match first before HTTPRoutes, but might be nice to raise a Condition for this in the event that one is ignored over another.

Just curious if others have thought about this yet or if I'm maybe misunderstanding the spec. =)

@stevesloka stevesloka added the kind/bug Categorizes issue or PR as related to a bug. label Apr 22, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Apr 23, 2021

At any time, a listener on the Gateway can only select routes of one kind, so conflicts shouldn't happen between route of different types.
I'm curious how you are running into this.

@stevesloka
Copy link
Contributor Author

I thought you could duplicate listener ports in a Gateway and then the controller should collapse them:

If the implementation does collapse compatible Listeners, the hostname provided in the incoming client request MUST be matched to a Listener to find the correct set of Routes. The incoming hostname MUST be matched using the Hostname field for each Listener in order of most to least specific. That is, exact matches must be processed before wildcard matches.

I guess we can apply the same conflict logic within the same type (e.g. HTTPRoutes), but do across types which would make the behavior consistent in a way, but would need to raise a condition on the object to alert the user.

// ref: https://gateway-api.sigs.k8s.io/references/spec/#networking.x-k8s.io/v1alpha1.Gateway

@hbagdi
Copy link
Contributor

hbagdi commented Apr 26, 2021

I see the problem.

 87     // 1. Either each Listener within the group specifies the "HTTP"            
 88     //    Protocol or each Listener within the group specifies either                                                                                                                                          
 89     //    the "HTTPS" or "TLS" Protocol.

I think the or in "HTTPS" or "TLS" is the reason for the confusion.
I could be wrong here but I think multiplexing HTTPS and TLS routing configuration on the same port isn't something we have discussed before. Collapsing should really happen only for listeners which have the same protocol type.
I don't think many implementations would be able to support such a configuration. Is that something you already support (or even plan to support)?

/cc @robscott @jpeach @Miciah

@stevesloka
Copy link
Contributor Author

Is that something you already support (or even plan to support)?

Today Contour supports a single insecure/secure listener, so all traffic routes through those two listeners essentially. Someone could potentially create a TLS route with the same fqdn as an HTTPRoute and you'd have a conflict. We handle this today by only supporting one or the other if users create them on the same fqdn.

@jpeach
Copy link
Contributor

jpeach commented Apr 26, 2021

I see the problem.

 87     // 1. Either each Listener within the group specifies the "HTTP"            
 88     //    Protocol or each Listener within the group specifies either                                                                                                                                          
 89     //    the "HTTPS" or "TLS" Protocol.

I think the or in "HTTPS" or "TLS" is the reason for the confusion.
I could be wrong here but I think multiplexing HTTPS and TLS routing configuration on the same port isn't something we have discussed before. Collapsing should really happen only for listeners which have the same protocol type.
I don't think many implementations would be able to support such a configuration. Is that something you already support (or even plan to support)?

/cc @robscott @jpeach @Miciah

IIRC an implementation can either conflict the route on this, or could choose HTTPS (on the principle that most specific wins).

@hbagdi
Copy link
Contributor

hbagdi commented Apr 27, 2021

Today Contour supports a single insecure/secure listener, so all traffic routes through those two listeners essentially. Someone could potentially create a TLS route with the same fqdn as an HTTPRoute and you'd have a conflict. We handle this today by only supporting one or the other if users create them on the same fqdn.

TLSRoute implies that the traffic flowing through the byte stream could be non-HTTP. Can you multiple HTTP/non-HTTP traffic on the same TCP port via Contour? Only then can this conflict occur.

IIRC an implementation can either conflict the route on this, or could choose HTTPS (on the principle that most specific wins).

@jpeach, is such a conflict even possible though?

@stevesloka
Copy link
Contributor Author

Can you multiple HTTP/non-HTTP traffic on the same TCP port via Contour?

No you can't, but if someone tries to configure this, there should be some set of conditions to warn them that one was rejected and the other is used. I was just trying to think through what should happen in that case, which resource "wins" and what conditions to set so the user is aware of what's happening.

@hbagdi
Copy link
Contributor

hbagdi commented May 3, 2021

No you can't, but if someone tries to configure this, there should be some set of conditions to warn them that one was rejected and the other is used.

In that case, shouldn't there be a conflict of listeners error on the Gateway status?

Let's say we have the following:

  listeners:  # Use GatewayClass defaults for listener definition.
  - protocol: HTTP
    port: 80
    routes:
      kind: HTTPRoute
  - protocol: TLS
    port: 80
    routes:
      kind: TLSRoute

In this case, shouldn't the controller signal an error on the Gateway resource stating that these two listeners are incompatible?

@stevesloka
Copy link
Contributor Author

In this case, shouldn't the controller signal an error on the Gateway resource stating that these two listeners are incompatible?

Sorry @hbagdi I feel like I haven't been super clear. I agree with you on that this should be an error but trying to understand who would win in this case? Do all the corresponding routes that are selected by these two listeners get rejected?

If the HTTP listener was valid and the TLS was added, what should we do? I'm not sure if there are any good rules to apply short of rejecting both, but worry that you'd drop all traffic in this case.

@k8s-triage-robot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

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 Aug 2, 2021
@jpeach
Copy link
Contributor

jpeach commented Aug 3, 2021

To summarize my interpretation:

  • You have 2 listeners on port 443
  • The first "HTTPS" listener matches HTTPRoutes
  • The second "TLS" matches TLSRoutes

You end up with both an HTTPRoute and a TLSRoute binding to the same hostname (i.e. SNI name).

Either each Listener within the group specifies the “HTTP” Protocol or each Listener within the group specifies either the “HTTPS” or “TLS” Protocol.

So the spec claims that this ought to be OK (the rationale is that you can still route properly based on the TLS SNI). However, there's no language that prevents an implementation rejecting this configuration though (it's stated as "an implementation might", not MUST).

So I can think of 3 options:

  1. Reject the TLS listener as Conflicted.

  2. Accept the HTTPRoute and refuse the TLSRoute. This follows the "most specific wins" rule. In this case, HTTP is more specific than TLS, so the HTTPRoute binds to the name and the TLSRoute is refused. The listener that matches TLSRoutes would get the false condition ResolvedRefs with the reason DegradedRoutes and the TLSRoute would get the false condition Admitted.

  3. Accept the configuration. Another way to resolve the conflict is to configure the listener using ALPN protocol names. You could send HTTP protocols to the HTTPRoute and everything else to the TLSRoute. That probably has limited value in practice, but it would be a direct mapping of the requested configuration. If you do this, there won't be any errors in this config.

@youngnick
Copy link
Contributor

Catching up on this one, I think @jpeach has nailed this, and I think we should do his option 2 above.

@robscott
Copy link
Member

I think option 1 may be preferable/simpler to handle. I can't think of a use case where you'd want an HTTPS and TLS listener for the same hostname on a Gateway. If users attached different certs to each listener it could get especially difficult to implement. It seems significantly simpler to me to consider that invalid.

@howardjohn
Copy link
Contributor

I may be missing something but seems like 1 and 2 are the same? Or is the difference just rejecting the listener vs rejecting all routes on the listener? If the latter, I slightly prefer 1, seems a bit simpler.

I agree (3) could be done but seems a bit complex and not likely portable, so I would prefer to defer it (likely indefinitely)

@robscott
Copy link
Member

Yeah, my interpretation was that 1 would reject the entire listener instead of just specific routes. That would be my preferred option.

@youngnick
Copy link
Contributor

I think 1 is fine too, and agree that it is simpler.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active 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 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 rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 1, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Oct 1, 2021

/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/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Oct 1, 2021
@shaneutt shaneutt added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Aug 5, 2022
@shaneutt shaneutt added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Mar 8, 2023
@shaneutt
Copy link
Member

Since the problem here comes up in conflicts between HTTPRoutes and TLSRoutes we don't expect this to be needed for v1.0.0 as we expect only GatewayClass, Gateway and HTTPRoute to be going GA for that release. We should however choose one of the options for resolving this problem and get this solved prior to beta for TLSRoute.

@shaneutt shaneutt added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Mar 14, 2023
@mlavacca
Copy link
Member

Since We want to promote TLSRoute for 1.2, I'm adding this to the TLSRoute epic, as we need to figure out how to deal with such a situation.

Said that, I lean toward solution 1, for its simplicity.

@shaneutt
Copy link
Member

Does anyone have capacity for this one? Should we sync about the future plans for TLSRoute soon to make sure we're all on the same page?

@youngnick
Copy link
Contributor

I think we should talk more about TLSRoute as part of scoping for 1.3, once 1.2 is released, yes.

@shaneutt shaneutt moved this to Triage in Post-GA Refinement Sep 19, 2024
@shaneutt shaneutt added triage/needs-information Indicates an issue needs more information in order to work on it. and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Status: Triage
Development

No branches or pull requests

10 participants