RSDK-9990- Expose SOCKS proxy as fallback dialer #414
Conversation
|
|
||
| // OnlySocksProxyEnvVar is the name of an environment variable used if all network | ||
| // traffic should be done through SOCKS5. | ||
| OnlySocksProxyEnvVar = "ONLY_SOCKS_PROXY" |
There was a problem hiding this comment.
[q] Will there be an equivalent ONLY_INTERNET? Or, is the idea that having SOCKS_PROXY unset means "internet only"?
There was a problem hiding this comment.
I think that should be ok, + internet will almost always be prioritized/chosen even without this knob
There was a problem hiding this comment.
Discussed offline; this is also helpful for development, as we can force all traffic through the SOCKS proxy even with a WiFi connection (ssh still possible.)
rpc/dialer.go
Outdated
| return nil, fmt.Errorf("error creating SOCKS proxy dialer to address %q from environment: %w", | ||
| proxyAddr, err) | ||
| } | ||
| conn, err := dialer.(proxy.ContextDialer).DialContext(ctx, network, addr) |
There was a problem hiding this comment.
Why do you have to cast to proxy.ContextDialer here? Is DialContext not available on the SOCKS5 dialer?
There was a problem hiding this comment.
yep, by default it only has Dial
viambot
added
safe to test
|
|
||
| // OnlySocksProxyEnvVar is the name of an environment variable used if all network | ||
| // traffic should be done through SOCKS5. | ||
| OnlySocksProxyEnvVar = "ONLY_SOCKS_PROXY" |
There was a problem hiding this comment.
Discussed offline; this is also helpful for development, as we can force all traffic through the SOCKS proxy even with a WiFi connection (ssh still possible.)
| // the fallback immediately (in 0 nanoseconds). | ||
| fallbackTimer.Reset(0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should there not be a
case <-ctx.Done():
here? Will this not spawn infinite SOCKS-proxy-dialing goroutines if neither WiFi not SOCKS proxy are available?
There was a problem hiding this comment.
once primary and fallback dial functions are done, we will exit because of L359. If ctx gets cancelled, the two dials will end and also end up at L359
There was a problem hiding this comment.
Cool; assume you mean L364 in the new line numbers.
| fallback = res | ||
| } | ||
| if primary.done && fallback.done { | ||
| return nil, primary.error |
There was a problem hiding this comment.
[q] Can you explain what's happening in this case? No connection can be returned even though they both completed?
There was a problem hiding this comment.
if both primary and fallback are done with errors, this means neither connection attempt succeeded. return the error from the primary dial attempt in that case.
Co-authored-by: Benjamin Rewis <[email protected]>
viambot
added
safe to test
viambot
added
safe to test
rpc/dialer.go
Outdated
| return nil, fmt.Errorf("error creating SOCKS proxy dialer to address %q from environment: %w", | ||
| proxyAddr, err) | ||
| } | ||
| conn, err := dialer.(proxy.ContextDialer).DialContext(ctx, network, addr) |
There was a problem hiding this comment.
yep, by default it only has Dial
| fallback = res | ||
| } | ||
| if primary.done && fallback.done { | ||
| return nil, primary.error |
There was a problem hiding this comment.
if both primary and fallback are done with errors, this means neither connection attempt succeeded. return the error from the primary dial attempt in that case.
| // the fallback immediately (in 0 nanoseconds). | ||
| fallbackTimer.Reset(0) | ||
| } | ||
| } |
There was a problem hiding this comment.
once primary and fallback dial functions are done, we will exit because of L359. If ctx gets cancelled, the two dials will end and also end up at L359
| // the fallback immediately (in 0 nanoseconds). | ||
| fallbackTimer.Reset(0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Cool; assume you mean L364 in the new line numbers.
rpc/dialer.go
Outdated
| go dialer(primaryCtx, primaryDial, true) | ||
|
|
||
| // wait a small amount before starting the fallback dial (to prioritize the primary connection method). | ||
| fallbackTimer := time.NewTimer(time.Second) |
There was a problem hiding this comment.
time.Second
If I'm reading Golang source code correctly, their fallback delay is customizable and defaults to 300ms. I understand not exposing this as a knob, but any reason to bump to 1s? Did you see SOCKS being "over" prioritized with 300ms?
There was a problem hiding this comment.
nope, let me test and drop to 300ms if no issues
viambot
added
safe to test
viambot
added
safe to test
cheukt
changed the title
cheukt
changed the title
|
@benjirewis given the new direction of the ticket around dynamic switching of connection types, going to cut a release here first to fully support agent using the socks proxy. I also unexported one of the functions to make it easier to change the impls. tested manually with the same test cases and also with the agent |
|
see viamrobotics/agent#79 for corresponding agent changes |
this change will make SOCKS proxy a fallback option (if SocksProxyEnvVar is specified), but will not actively replace a connection if it is already established. If OnlySocksProxyEnvVar is specified, only the SOCKS connection is used.
It adds a small delay to the SOCKs dialer as a way to prioritize a connection made without the SOCKS dialer.
Conceptually, pretty similar to our Dial code and it borrows heavily from the net.DialContext code.
tested a few scenarios:
cc: @dgottlieb if interested