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.

Sign up for GitHub

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

Jump to bottom

internal/dag: Set SNI on upstream externalName type clusters for TCPProxy #3291

Merged
merged 2 commits into from Feb 18, 2021
Merged

internal/dag: Set SNI on upstream externalName type clusters for TCPProxy #3291

merged 2 commits into from Feb 18, 2021

Conversation

stevesloka
Copy link
Member

Sets the SNI on any TCPProxy.Service which references an externalName type service
as well as having the upstream protocol of "tls".

Updates #2517

Signed-off-by: Steve Sloka slokas@vmware.com

@stevesloka stevesloka requested a review from a team as a code owner January 29, 2021 17:40
@stevesloka stevesloka requested review from jpeach, youngnick, skriss and sunjayBhatia and removed request for a team January 29, 2021 17:40
@stevesloka
Copy link
Member Author

//cc @moderation

@codecov
Copy link

codecov bot commented Jan 29, 2021 •
edited

Codecov Report

Merging #3291 (75fa93d) into main (e3a2775) will increase coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3291      +/-   ##
==========================================
+ Coverage   75.69%   75.74%   +0.05%     
==========================================
  Files          98       98              
  Lines        6405     6410       +5     
==========================================
+ Hits         4848     4855       +7     
+ Misses       1449     1447       -2     
  Partials      108      108
Impacted Files Coverage Δ
internal/dag/httpproxy_processor.go 91.56% <50.00%> (-0.53%) ⬇️
internal/dag/cache.go 91.58% <0.00%> (+0.62%) ⬆️
internal/k8s/log.go 69.56% <0.00%> (+6.52%) ⬆️

@youngnick youngnick added this to the 1.13.0 milestone Feb 1, 2021
skriss
skriss approved these changes Feb 1, 2021
View reviewed changes
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

This looks OK to me. It'd be nice to have an integration test case for this too.

sunjayBhatia
sunjayBhatia reviewed Feb 1, 2021
View reviewed changes
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.

This looks OK to me. It'd be nice to have an integration test case for this too.

thinking out loud how this might work, maybe deploy a TLS echo app/service and an external name service that points to it, and use the SNI enforcement endpoint to check it works

youngnick
youngnick approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, and agree that some integration testing around this would be great.

@skriss
Copy link
Member

skriss commented Feb 3, 2021

needs a rebase

@stevesloka
internal/dag: Set SNI on upstream externalName type clusters for TCPP…
908c014 
…roxy

Sets the SNI on any TCPProxy.Service which references an externalName type service
as well as having the upstream protocol of "tls".

Updates projectcontour#2517

Signed-off-by: Steve Sloka <slokas@vmware.com>
@skriss
Copy link
Member

skriss commented Feb 10, 2021

@stevesloka I got #3342 merged, hopefully that helps with adding a test for this. Happy to walk through it if you want.

@sunjayBhatia sunjayBhatia added this to In progress in Contour Project Board via automation Feb 11, 2021
@sunjayBhatia sunjayBhatia moved this from In progress to 1.13 Release in Contour Project Board Feb 11, 2021
skriss
skriss approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

OK with me to merge this now and file a follow-up for an integration test.

@stevesloka stevesloka merged commit 4ddb326 into projectcontour:main Feb 18, 2021
@stevesloka stevesloka deleted the tcpproxysni branch February 18, 2021 21:43
@stevesloka stevesloka added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 18, 2021
iyacontrol pushed a commit to iyacontrol/contour that referenced this pull request Feb 23, 2021
@stevesloka @iyacontrol
internal/dag: Set SNI on upstream externalName type clusters for TCPP…
8efb7ca 
…roxy (projectcontour#3291)

Sets the SNI on any TCPProxy.Service which references an externalName type service as well as having the upstream protocol of "tls".

Updates projectcontour#2517

Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: iyacontrol <gaohj2015@yeah.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunjayBhatia sunjayBhatia sunjayBhatia left review comments

@skriss skriss skriss approved these changes

@youngnick youngnick youngnick approved these changes

@jpeach jpeach Awaiting requested review from jpeach

Assignees

@stevesloka stevesloka

Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stevesloka @skriss @sunjayBhatia @youngnick