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

Custom DNS resolver and proxy support for WebSocket clients. #488

Open
wants to merge 6 commits into
base: master
from

Conversation

@dspearson
Copy link

dspearson commented Feb 19, 2019

No description provided.

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 19, 2019

@dspearson thanks! I have a couple of concerns regarding this functionality:

  1. Most probably we want to provide two different options: dns-options (to create a new DNS resolver) and name-resolver (to reuse the existing one). With the connection pool it’s more intuitive, as you spin and reuse the same resolver per each connection created within the pool. But here I’m forced to spin new resolver on each connection open (in most cases to resolve a single hostname). Which might be an overkill...

  2. I don’t have experience with proxies for WS and WSS, but it feels like we need to set “Proxy-Connection: Keep-Alive” to ensure that the proxy would not close connection to the peer right away. What do you think?

By pre-instantiating an instance of the resolver, and passing it to the
function as an argument, we avoid repeatedly creating new instances per-call.
@dspearson

This comment has been minimized.

Copy link
Author

dspearson commented Feb 19, 2019

I haven't really looked at the WS spec in detail, nor looked at the methods for maintaining an open connection. However, I see that in the http-proxy-headers function, there is
(.set headers "Proxy-Connection" "Keep-Alive"))
and this appears to be called as part of the chain.

Regarding the first point, I have addressed it in a follow-up commit by simply removing dns-options, and allowing the passing of a pre-instantiated instance of the name resolver via name-resolver.

Cheers!

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 19, 2019

Yeah, there's a :keep-alive? setting that's set to true by default and it effectively adds an appropriate header. What we probably want to do here, is to force this setting to be true (just assoc it when creating the handler).

| `heartbeats` | optional configuration to send Ping frames to the server periodically (if the connection is idle), configuration keys are `:send-after-idle` (in milliseconds), `:payload` (optional, empty frame by default) and `:timeout` (optional, to close the connection if Pong is not received after specified timeout).
| `name-resolver` | specify the mechanism to resolve the address of the unresolved named address. When not set or equals to `:default`, JDK's built-in domain name lookup mechanism is used (blocking). Set to`:noop` not to resolve addresses or pass an instance of `io.netty.resolver.AddressResolverGroup` you need.
| `proxy-options` | a map to specify proxy settings. HTTP, SOCKS4 and SOCKS5 proxies are supported. Note, that when using proxy `connections-per-host` configuration is still applied to the target host disregarding tunneling settings. If you need to limit number of connections to the proxy itself use `total-connections` setting.
Supported `proxy-options` are

This comment has been minimized.

Copy link
@kachayev

kachayev Feb 19, 2019

Collaborator

Please, add a single empty line before this one.

@dspearson dspearson closed this Feb 19, 2019
@dspearson dspearson reopened this Feb 19, 2019
@dspearson

This comment has been minimized.

Copy link
Author

dspearson commented Feb 19, 2019

Erroneously hit close, sorry about that.

Addressed your comments. Thanks for your input.

@@ -809,7 +811,7 @@
"websocket-deflater"
WebSocketClientCompressionHandler/INSTANCE)))
(#(when (some? proxy-options)
(let [proxy (proxy-handler (assoc proxy-options :ssl? ssl?))]
(let [proxy (proxy-handler (assoc proxy-options :ssl? ssl? :keep-alive? keep-alive?))]

This comment has been minimized.

Copy link
@kachayev

kachayev Feb 19, 2019

Collaborator

Sorry, I was not clear enough... I meant just assoc here true to prevent self-shooting

(assoc proxy-options :ssl? ssl? :keep-alive? true)

In such a case even if I passed :keep-alive?with my proxy-options (we never validate keys anyway), it still will be set to true when passing to the handler's builder. In such a case we don't need :keep-alive? being mentioned as an option.

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 19, 2019

Erroneously hit close, sorry about that.

Oh, yeah... I'm doing this randomly all the time :)

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 19, 2019

Thanks a lot!

@dspearson

This comment has been minimized.

Copy link
Author

dspearson commented Feb 19, 2019

I admit I still don't know my way around the code that well. I am a little dubious about the validity of associating :ssl? with the proxy-options there, having copied it from elsewhere, and looking at http-proxy-handler has me scratching my head. The value is given by looking at the WebSocket connection scheme, and appears unrelated to the protocol used by the proxy itself.

I'm not using the option myself, so haven't tested it, sorry.

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 20, 2019

We need :ssl? to be passed to proxy handler just to be sure if we need to open tunnel. It should work for Websockets the same way it works for any HTTP connection but overall HTTPS proxies are not that common.

@ztellman

This comment has been minimized.

Copy link
Owner

ztellman commented Mar 25, 2019

@kachayev, is this ready to be merged?

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Mar 26, 2019

@ztellman Yeah, this one is ready.

@dspearson

This comment has been minimized.

Copy link
Author

dspearson commented Apr 22, 2019

Let me know if I need to do any further changes here, since I note my branch is a little behind.

@HaskellZhangSong

This comment has been minimized.

Copy link

HaskellZhangSong commented May 14, 2019

Just drop by, sorry for my abrupt intrusion.
I am using this fantastic package and this is one of the features what I am waiting for. Only one suggestion. Normally, in some traditional command line software and some packages, proxy will use http_proxy and https_proxy environment variables as proxy by default, see python websocket package

python websocket package line 152.
and wget line 1125
and curl line 2257.

All of them use these two environment variables as their proxy by default. I suppose that is a convention of linux/unix software/package. Would it be better if aleph also supports this?

@dspearson

This comment has been minimized.

Copy link
Author

dspearson commented May 15, 2019

This pull request is specifically only for adding the DNS resolver and proxy settings for WebSockets. Additional features for proxy selection from the environment should probably be in a different branch, if anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.