internal: Adds support for TLSRoute #3627
Conversation
stevesloka
added
the
release-note
stevesloka
requested review from
sunjayBhatia and
youngnick
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## main #3627 +/- ##
==========================================
- Coverage 76.18% 76.13% -0.06%
==========================================
Files 103 103
Lines 7117 7185 +68
==========================================
+ Hits 5422 5470 +48
- Misses 1578 1596 +18
- Partials 117 119 +2
|
stevesloka
force-pushed
the
tlsRoute
branch
4 times, most recently
from
6afe832
to
4fdcd84
Compare
| @@ -0,0 +1,187 @@ | |||
| # Copyright Project Contour Authors | |||
There was a problem hiding this comment.
test looks good, should we leave as the original _integration version and conver to new e2e format later?
There was a problem hiding this comment.
I can convert this one separately if necessary. PR #3665 adds code for the ingress-conformance-echo-tls fixture which is maybe what was missing for implementing this one in Go? After that merges, though, I'd love to see all new tests written in Go.
| name: tlsroute1 | ||
| spec: | ||
| rules: | ||
| - forwardTo: |
There was a problem hiding this comment.
how does it work if we have both no SNI an SNI together? I think Envoy will choose the most specific so they should be able to coexist
For criteria that allow ranges or wildcards, the most specific value in any of the configured filter chains that matches the incoming connection is going to be used (e.g. for SNI www.example.com the most specific match would be www.example.com, then *.example.com, then *.com, then any filter chain without server_names requirements).
There was a problem hiding this comment.
This is a great point, there probably should be some validation higher up across all the listeners to check that only one wildcard (e.g. '*') can exist for the same port.
Additionally, we need to validate that the same domain isn't used for both TLSRoute as well as an HTTPRoute.
| rh, c, done := setup(t) | ||
| defer done() | ||
|
|
||
| //s1 := &v1.Secret{ |
There was a problem hiding this comment.
the docs mention that a secret is required on TLSRoutes, seems like the integration test and this one have omitted it and not made the TLS secret required?
There was a problem hiding this comment.
is this because this just enables passthrough TLS and not terminating and re-encrypting?
There was a problem hiding this comment.
what are the rules that we/upstream decided on for supporting TLS passthrough/termination and how they relate to TLSRoute/TCPRoute?
There was a problem hiding this comment.
There were some discussions upstream (kubernetes-sigs/gateway-api#633 (comment)) and essentially from what I understand, TLSRoute is a passthrough TCPProxy against SNI.
If you want to terminate then route, you'd have to use TCPRoute.
@youngnick said we can't support TCPRoute so this will be a point that differs from HTTPProxy.
- HTTPProxy.TCPProxy (passthrough: true) == TLSRoute
- HTTPProxy.TCPProxy (passthrough: false) == TCPRoute
stevesloka
force-pushed
the
tlsRoute
branch
2 times, most recently
from
54e89c0
to
e3cf805
Compare
| p.Errorf("error validating routes against Listener.Routes.Selector: %s", err) | ||
| } | ||
|
|
||
| if selMatches && nsMatches { |
There was a problem hiding this comment.
Is this block of logic that we have for HTTPRoutes not needed for TLSRoutes?
There was a problem hiding this comment.
Yup it is to satisfy the API. I think I just didn't add it yet for this PR. Is it easier to merge this with out (assuming the rest is ok), then follow up with a new PR to add that with corresponding tests?
Add support for TLSRoute to enable Passthrough TCP Proxying to pods via SNI. Updates projectcontour#3440 Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Add support for TLSRoute to enable Passthrough TCP Proxying to pods via SNI.
Note: This is currently rebased from #3620 which makes the Status bits more generic. Merging that first will make this PR smaller.
Updates #3440
Signed-off-by: Steve Sloka slokas@vmware.com