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

Add SkipTLSHandshake optional function to CustomDialer #1147

Merged
merged 2 commits into from
Nov 30, 2022
Merged

Add SkipTLSHandshake optional function to CustomDialer #1147

merged 2 commits into from
Nov 30, 2022

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Nov 29, 2022

Follow up from #1146

This adds a small interface SkipTLSHandshake() bool that when implemented a CustomDialer may opt into skipping the handshake if not needed by the dialer implementation.

/cc @oderwat

@oderwat
This adds an option to skip the TLS connection wrapper
c9b2fd8 
This option to skip the TLS wrapper is meant to be used if a custom
dialer already handled the TLS handshake. I discussed my use case,
which is using NATS from a Wasm module running in the browser in
Slack with @derekcollison. There may be other use cases when the
custom dialer is using some kind of tunneling, for example.
@wallyqs wallyqs requested a review from piotrpio November 29, 2022 20:36
@wallyqs wallyqs mentioned this pull request Nov 29, 2022
Closed
piotrpio
piotrpio reviewed Nov 29, 2022
View reviewed changes
Copy link
Collaborator

@piotrpio piotrpio 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 good. Would be nice to have documentation on this though. As skipTLSDialer will not be visible in godoc, I would suggest to document this either on CustomDialer interface or CustomDialer field on Options.

@piotrpio
Copy link
Collaborator

@wallyqs Also, staticcheck complains about:

ws_test.go:900:2: this value of nc is never used (SA4006)

@wallyqs wallyqs force-pushed the custom-dialer-skip-tls branch from 5ae4fe2 to 51c8882 Compare November 29, 2022 21:32
piotrpio
piotrpio approved these changes Nov 29, 2022
View reviewed changes
Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM! One check still failing (field skipTLS unused), I've added a suggestion.

example_test.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 29, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 85.869% when pulling 65b7870 on custom-dialer-skip-tls into 398a1ec on main.

@wallyqs @piotrpio
Add SkipTLSHandshake small interface to CustomDialer
65b7870 
Signed-off-by: Waldemar Quevedo <[email protected]>

Co-authored-by: Piotr Piotrowski <[email protected]>
@wallyqs wallyqs force-pushed the custom-dialer-skip-tls branch from d7099ff to 65b7870 Compare November 29, 2022 23:23
@wallyqs wallyqs merged commit 907b219 into main Nov 30, 2022
@wallyqs wallyqs deleted the custom-dialer-skip-tls branch November 30, 2022 00:02
@oderwat
Copy link
Contributor

oderwat commented Nov 30, 2022

I want to thank all of you that worked on this and got it upstream so quickly!

@piotrpio piotrpio mentioned this pull request Dec 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piotrpio piotrpio piotrpio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wallyqs @piotrpio @coveralls @oderwat