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

support tls termination for tcp traffic #1431

Merged
merged 9 commits into from
May 25, 2023

Conversation

tanujd11
Copy link
Member

@tanujd11 tanujd11 commented May 19, 2023

What type of PR is this?

Feature to provide TLS termination with TCP traffic

What this PR does / why we need it:

Supports the following configuration:
Listener Protocol = TLS
TLS Mode = Terminate
Route Type =TCPRoute

Which issue(s) this PR fixes:

Fixes #1391

@tanujd11 tanujd11 requested a review from a team as a code owner May 19, 2023 22:50
@tanujd11 tanujd11 marked this pull request as draft May 19, 2023 23:02
@tanujd11 tanujd11 force-pushed the feat/support-tls-termination branch 2 times, most recently from dd09e06 to 26a6a21 Compare May 20, 2023 06:58
Signed-off-by: tanujd11 <dwiveditanuj41@gmail.com>
@tanujd11 tanujd11 force-pushed the feat/support-tls-termination branch from 26a6a21 to e4b5666 Compare May 20, 2023 06:59
@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #1431 (9bb41f0) into main (3344221) will decrease coverage by 0.04%.
The diff coverage is 62.75%.

@@            Coverage Diff             @@
##             main    #1431      +/-   ##
==========================================
- Coverage   61.98%   61.94%   -0.04%     
==========================================
  Files          79       79              
  Lines       11318    11388      +70     
==========================================
+ Hits         7015     7054      +39     
- Misses       3844     3874      +30     
- Partials      459      460       +1     
Impacted Files Coverage Δ
internal/ir/zz_generated.deepcopy.go 12.42% <0.00%> (-0.48%) ⬇️
internal/ir/xds.go 71.66% <57.14%> (-0.53%) ⬇️
internal/gatewayapi/validate.go 89.81% <67.47%> (-1.51%) ⬇️
internal/xds/translator/listener.go 82.27% <71.42%> (-0.30%) ⬇️
internal/gatewayapi/listener.go 98.11% <83.33%> (-1.89%) ⬇️
internal/gatewayapi/helpers.go 80.65% <100.00%> (+0.48%) ⬆️
internal/gatewayapi/route.go 88.07% <100.00%> (+0.01%) ⬆️
internal/provider/kubernetes/helpers.go 81.20% <100.00%> (+1.47%) ⬆️
internal/xds/translator/translator.go 80.29% <100.00%> (+0.39%) ⬆️

... and 1 file with indirect coverage changes

Signed-off-by: tanujd11 <dwiveditanuj41@gmail.com>
Signed-off-by: tanujd11 <dwiveditanuj41@gmail.com>
@tanujd11 tanujd11 marked this pull request as ready for review May 20, 2023 08:16
Xunzhuo
Xunzhuo previously approved these changes May 22, 2023
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this : ) LGTM

@zirain
Copy link
Contributor

zirain commented May 22, 2023

@tanujd11 is there any conformace test for this?

@tanujd11
Copy link
Member Author

@tanujd11 is there any conformace test for this?

No @zirain, I haven't added any e2e test for this yet. Shall I do it in this PR?

Copy link
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

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

Greate Job.

Copy link
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

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

Greate Job.

qicz
qicz previously approved these changes May 22, 2023
Copy link
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

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

Greate Job.

@zirain
Copy link
Contributor

zirain commented May 22, 2023

@tanujd11 is there any conformace test for this?

No @zirain, I haven't added any e2e test for this yet. Shall I do it in this PR?

yes, I think so

@@ -612,7 +612,9 @@ type TCPListener struct {
Port uint32
// TLS information required for TLS Passthrough, If provided, incoming
// connections' server names are inspected and routed to backends accordingly.
TLS *TLSInspectorConfig
TLSInspectorConfig *TLSInspectorConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on making this

tls:
  passthrough:
  terminate:

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel in future we could use SNIs to route traffic in TCP terminate mode as well? Wdyt? Istio does it on the basis of gateway hostname. However at the moment it does not make sense as TCPRoute does not have any SNI based routing. Maybe I am missing something. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

that case is handled in the HTTPListener

type HTTPListener struct {
, the Gateway.Listener.Hostname API field internally performs a SNI match at the xds Listener level

@tanujd11 tanujd11 dismissed stale reviews from Xunzhuo and qicz via 9bb41f0 May 24, 2023 19:08
@tanujd11
Copy link
Member Author

@arkodg PTAL

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks for adding this feature !
can you also raise a GH issue here or in upstream to add a conformance test for it, TIA

@tanujd11
Copy link
Member Author

kubernetes-sigs/gateway-api#2060 for tracking conformance test related to this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS Termination support for TLS listener
5 participants