Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Respect HTTP_PROXY/HTTPS_PROXY #10209
Respect HTTP_PROXY/HTTPS_PROXY #10209
Changes from 9 commits
72459fb
05fa8b5
37cd786
e2df595
8281dfc
59088d7
700abc2
d41122a
d225e71
810d958
dd5e232
7981450
726297f
1823b90
f57114d
ad44ccf
de6c715
df6c5e2
bde8fb2
bbc3af7
79e3f3f
fcf43ee
e24d3d0
749d889
4e2a20a
cad2d6c
0edbc1d
df2853e
926f7bb
23fdd82
a94ce4b
dd2cb23
2815f4c
5238d5a
4f14ec4
b343ff8
0774cc4
b6dff62
b861f75
f5b3b71
e127340
76dfcbe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can
conf
be nil here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conf
can be nil when the caller doesn't useproxy.WithTLSConfig()
to use a custom tls config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not sure I fully understand this tlsConfig logic. Why are we setting NextProtos to
ProtocolReverseTunnel
here in what (I think) is supposed to be a generic proxy, but only in the case when tlsConfig wasn't passed in?@smallinsky Can you take a look at these changes too pls - do you remember why we're setting ProtocolReverseTunnel here? Is this proxy supposed to be used by the agents only? Just want to make sure we don't break any scenarios.
Is this proxy is now used by both reverse tunnel agents and tsh, I wonder if we should make the TLS config mandatory and have the caller pass it appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProtocolReverseTunnel
is the default from before the tls config was injected. As long as it doesn't break anything, I think making tls config mandatory is the way to go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the dialer is misleading. It is used in
TunnelAuthDialer
and Reverse tunnel Agent call so under the hood it is Reverse Tunnel Dialer where a connection is established to Reverse tunnel service thus in case of TLSRouting mode thealpncommon.ProtocolReverseTunnel
protocol is set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can
conf
be nil here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same q as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same q as above.