Proxy support in tokio migration

[Johannesd3]

Adding a git dependency of a hyper_proxy version with tokio 1.0 support. It is not yet merged, and as far as I see there was no reaction of the maintainer: tafia/hyper-proxy#18.
Using proxytunnel for apresolve to restore the proxy support.

[ashthespy]

@Johannesd3 While you are at it, do you think you could place this behind a feature flag?
(from back then #191 (comment))

EDIT: not even sure if it is still relevant, but it does bring some extra dependencies right?

[Johannesd3]

Yes, sure. But while we're at it, would you please explain me why hyper_proxy and proxytunnel.rs are necessary?

[Johannesd3]

Most crates are in our dependency tree anyway. I'm more worried that hyper_proxy is no longer actively maintained.

[ashthespy]

Good question, but from a quick glance, the AP resolution stuff could also be handled by adapting proxytunnel.rs -- @janderholm would be the right person to answer..

[janderholm]

If I recall, it was quite a long time ago, proxy servers are usually configured to only support CONNECT if the destination port is 443. Any normal HTTP request such as apresolve need to implement the full proxy standard.

If it's possible to resolve through port 443 then the proxy tunnel could probably be used. The server is going to need to support CONNECT anyway so requiring it for resolve as well is reasonable.

[Johannesd3]

HTTPS://apresolve.spotify.com/ seems to work, so is there any reason not to use proxy tunnel?

[janderholm]

In my opinion, if you manage to hook it up, go for it :-)

[Johannesd3]

Wait, is hyper used only for apresolve? We could replace it with reqwest which supports proxies.

[ashthespy]

@Johannesd3 no, we use hyper for discovery as well (but as a server)

[Johannesd3]

Reqwest uses hyper internally, so probably it won't add many dependencies even if we keep hyper. So what do you think?

[ashthespy]

Reqwest uses hyper internally, so probably it won't add many dependencies even if we keep hyper. So what do you think?

Personally, I'd prefer not to pull in reqwest just to handle this AP query..
If we can't use the existing proxytunnel.rs a simple(r) option would be to just always use the fallback AP when using a proxy?

[Johannesd3]

It's already odd to use hyper for this simple query. There are at least some projects that rely on core but not on connect (like ncspot) and I really wonder how much it would decrease compile time if hyper was no dependency.

Using reqwest it could be something like this:

async fn request() -> reqwest::Result<APResolveData> {
    Client::new()
        .get(APRESOLVE_ENDPOINT)
        .send().await?
        .json().await
}

reqwest uses the system proxy which is given by the http_proxy and https_proxy env vars by default. I think this is really tempting, but this decision is not up to me.

If we still want to use hyper, this trait must be implemented for proxytunnel. It doesn't seem too hard, I will take a look at it.

[plietar]

https://apresolve.spotify.com/ requires a TLS library, which is a whole other can of worms.

I’d say using reqwest saves us a lot of trouble. Actually I’ve even been thinking of opening an issue on reqwest to split out the proxy code (or at least make it public). Then we can replace proxytunnel.rs, which, unlike reqwest, does not support https or socks proxies.

If people actually care about build times / binary size that much, we can make apresolve (always use the fallback) and proxy support optional. Then all the hyper/reqwest depencies go away (obviously discovery still needs hyper). I’d rather not until someone actually complains about it though.

——

I don’t know how I feel about reqwest using environment variables when we also have a flag for it. Maybe if proxytunnel is replaced by reqwest’s proxy implementation then we can start using the environment variables and deprecate the flag. Until then we should tell reqwest to use a proxy if and only if the flag is set.

[Johannesd3]

Apresolve should work now with proxytunnel, but I'm not sure about the problems @janderholm mentions.

(btw is it possible that extern crate isn't needed anymore in lib.rs?)

[sashahilton00]

I thought extern crate was more or less phased out in rust 2018

[Johannesd3]

@ashthespy We could merge this, and think about using hyper_proxy or reqwest later, or what do you think? At least it works now (I think), which was previously not the case.

[ashthespy]

@Johannesd3 Don't have the time to test this now, but if you are happy with it, will merge in..

[Johannesd3]

Yes, I would prefer this. Testing with a local proxy server instance worked, and after all it's just a WIP branch...