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

Gateway API: support TLS termination with TLSRoute/TCPRoute #5481

Merged
merged 5 commits into from
Jul 21, 2023

Conversation

skriss
Copy link
Member

@skriss skriss commented Jun 15, 2023

Adds support for TLS termination with the TLS
listener protocol. Envoy is configured to terminate
TLS and then to proxy TCP traffic to the backend.
This configuration is compatible with both TLSRoute
and TCPRoute.

Closes #5461.

Leaving as a draft for now until there's a clear consensus on the upstream issue (kubernetes-sigs/gateway-api#2111) around TLS termination with TLSRoute.

)

tcpProxyFilter := envoy_v3.TCPProxy(listener.Name, listener.TCPProxy, cfg.newInsecureAccessLog())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ends up duplicating some of the code from the existing SecureVirtualHost + TCPProxy code path. It might be possible to change the implementation for TCPRoutes to just configure a SecureVirtualHost of * along with a Secret/TCPProxy attached there, instead of using the Listener-level TCPProxy that I added to the DAG with the TCPRoute PR. Will look into it some, but at least what's here now works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, made this change in 5088897

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #5481 (fea4b08) into main (cca0691) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5481      +/-   ##
==========================================
+ Coverage   78.52%   78.55%   +0.02%     
==========================================
  Files         138      138              
  Lines       19018    19027       +9     
==========================================
+ Hits        14934    14946      +12     
+ Misses       3801     3799       -2     
+ Partials      283      282       -1     
Impacted Files Coverage Δ
internal/dag/gatewayapi_processor.go 93.96% <100.00%> (+0.22%) ⬆️
internal/xdscache/v3/listener.go 92.37% <100.00%> (+0.04%) ⬆️

@skriss skriss added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Jun 15, 2023
@skriss skriss force-pushed the pr-gwapi-tls-tcproute branch 2 times, most recently from a058a75 to acb3cff Compare June 21, 2023 19:24
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2023
@skriss skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 10, 2023
@skriss
Copy link
Member Author

skriss commented Jul 17, 2023

I would love to get this into the upcoming release, but unfortunately there hasn't been a clear consensus on the upstream issue. We could merge it anyway, assuming that the upstream spec will eventually allow this (or at least not disallow it), but that does create a risk that we'd have to make a breaking behavior change in the future if something unexpected changes. @sunjayBhatia WDYT?

Adds support for TLS termination with the TLS
listener protocol. Envoy is configured to terminate
TLS and then to proxy TCP traffic to the backend.

Signed-off-by: Steve Kriss <[email protected]>
Code might be simpler if we add a secure
vhost of "*" and use the existing TCPProxy
code flow from there, I think it should
already work.

Signed-off-by: Steve Kriss <[email protected]>
Signed-off-by: Steve Kriss <[email protected]>
Signed-off-by: Steve Kriss <[email protected]>
@sunjayBhatia
Copy link
Member

I would love to get this into the upcoming release, but unfortunately there hasn't been a clear consensus on the upstream issue. We could merge it anyway, assuming that the upstream spec will eventually allow this (or at least not disallow it), but that does create a risk that we'd have to make a breaking behavior change in the future if something unexpected changes. @sunjayBhatia WDYT?

just pinged that issue to hopefully get it triaged

it is a little funny since the Gateway fields are v1beta1 and core but the route resources are v1alpha2 so how does the api versioning guarantee work

since TLSRoute etc. is still in alpha I think it'll be ok in the slim chance behavior has to change? We can push to get conformance etc. codified if we do merge this so it's less likely to change out from under us

@skriss
Copy link
Member Author

skriss commented Jul 20, 2023

Ah that's a great point, kinda forgot that these are still alpha resources 😁

@skriss skriss marked this pull request as ready for review July 20, 2023 19:14
@skriss skriss requested a review from a team as a code owner July 20, 2023 19:14
@skriss skriss requested review from tsaarni and sunjayBhatia and removed request for a team July 20, 2023 19:14
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! e2e tests can come in the form of conformance tests once the upstream issue for this specification is triaged etc.

@skriss skriss merged commit be3ba06 into projectcontour:main Jul 21, 2023
27 checks passed
@skriss skriss deleted the pr-gwapi-tls-tcproute branch July 21, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Termination mode for TLS listener protocol (TLSRoutes, TCPRoutes)
2 participants