daemon_config: Detect proxy env var and use it if found #1348

townsend2010 · 2020-02-10T19:14:55Z

Fixes #885

This was tricky to test. I had to create an Ubuntu VM using Hyper-V and then in the VM, install squid and set it up. Then I would set the http_proxy environment variable before running multipassd.

Also, after trying this and manually testing various failure scenarios, I have found the URLDownloader class, CommonVMImageHost and its derived classes, and the daemon need some pretty major refactoring to better handle long running network operations like when a host is unreachable and to also better getting these issues back to the user.

That said, this PR is only for getting the proxy values, but does nothing for failure scenarios. That will come with a follow up PR.

Fixes #885

codecov · 2020-02-10T21:14:55Z

Codecov Report

Merging #1348 into master will increase coverage by 0.05%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #1348      +/-   ##
==========================================
+ Coverage   73.16%   73.21%   +0.05%     
==========================================
  Files         208      208              
  Lines        7531     7550      +19     
==========================================
+ Hits         5510     5528      +18     
- Misses       2021     2022       +1

Impacted Files	Coverage Δ
src/daemon/daemon_config.h	`100% <ø> (ø)`	⬆️
src/daemon/daemon_config.cpp	`59.45% <100%> (+12.09%)`	⬆️
src/network/url_downloader.cpp	`55.17% <50%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9125a51...a4883b6. Read the comment docs.

ricab

Thank you @townsend2010, proxy support will be crucial for a lot of people, so this is very useful. I have some questions and concerns though. Some of them inline, general ones here:

Shouldn't multipass look at other variables affecting this, like https_proxy, no_proxy, or all_proxy?
How do instances get to go through the proxy? Does that somehow happen transparently? Would the user have to set the proxy manually inside the instances, or should multipass be responsible for "forwarding" that configuration? I am not say multipass should do it, just raising the question.
- There is also ftp_proxy, but that could only be needed by the instances, so that is ok if we don't forward anything.
do we have any local connections that could be inadvertently affected by application-level proxy?

ricab · 2020-02-11T10:31:28Z

src/network/url_downloader.cpp

+    {
+        if (!http_proxy.startsWith("http://"))
+        {
+            http_proxy.prepend("http://");


What if it is socks5://?

SOCKS support can come later. If a system level SOCKS proxy is set, Qt will automatically pick it up anyways.

My point is that passing http://socks://blah to Qt doesn't seem right, even if we don't have specific support for SOCKS. Perhaps this could be checked for a "://" sub-string instead. Otherwise, maybe it is preferable to require the protocol to be part of the env var.

An env var defined as http_proxy=socks://blah is not a valid proxy and would break things regardless of how we are parsing this. This is for doing something like the following:

http_proxy=192.168.1.3:3128 and then adding http:// to the beginning of the IP address (or host name if it's given) so we can then pass a valid URL to QUrl.

I am not sure I follow, http_proxy=socks5://something is normally correct, does Qt not like that?

Ah, ok, I see what you mean. Sure, I should just check to make sure like you mentioned previously.

src/network/url_downloader.cpp

ricab · 2020-02-11T11:20:00Z

src/network/url_downloader.cpp

    return reply->readAll();
 }
+
+std::unique_ptr<QNetworkProxy> discover_http_proxy()


Wouldn't this unit deserve tests?

I gave this quite a bit of thought and we would basically have to get into the business of mocking a proxy somehow. And that I don't think is worth the effort for something like this.

I was thinking something much more superficial, like just checking whether or not the application had a QNetworkProxy proxy registered with the right url and data, depending on whether/how http_proxy was defined. No proxy mocking should be required for that (?)

Ok, I added a unit test. Even after implementing the test, it still seems it's mainly testing QNetworkProxy, but it was easy enough to add, so it should be good.

townsend2010 · 2020-02-11T13:16:25Z

Hey @ricab,

Shouldn't multipass look at other variables affecting this, like https_proxy, no_proxy, or all_proxy?

No, it's not necessary for Qt when setting a proxy. You're basically telling Qt the address, port, username, and password of a proxy, the protocol is not important for that.

How do instances get to go through the proxy? Does that somehow happen transparently? Would the user have to set the proxy manually inside the instances, or should multipass be responsible for "forwarding" that configuration? I am not say multipass should do it, just raising the question.

This is not intended for instances at all. It's only for allowing multipassd itself to reach the network resources it needs such as SimpleStreams manifests, images, multipass version info on github, etc. Poking it through to instances can be an enhancement we add later, but definitely not now. The user will still need to provision proxy support for their instances, but cloud-init makes that easy.

do we have any local connections that could be inadvertently affected by application-level proxy?

This is only for Qt network access. We don't use Qt for local connections.

townsend2010 · 2020-02-11T13:19:42Z

I will also add, this PR is very focused on addressing basically two things. First, this is the only way (besides popping up a window to ask for credentials) for Qt to add proxy authorization. Qt will not pick up credentials automatically via a system proxy, so the http_proxy env var is the defacto way of doing this.

The second thing it addresses is when a user sets a local http_proxy env var for a per application proxy set up.

Based on review feedback, it makes more sense to put the proxy discovery into the DaemonConfig code.

ricab · 2020-02-11T15:15:05Z

Hi @townsend2010, thank you for the explanations.

No, it's not necessary for Qt when setting a proxy.

Sure, it is not needed, but a user that sees multipass understanding one of those may expect it to react to the others too. For instance, if /etc/environment defines both http_proxy and no_proxy, multipass will honor the first but not the second. I guess we can live with that, but I'd prefer if it was documented. For extra clarity, perhaps even a warning saying they're ignored, WDYT?

This is not intended for instances at all.

OK, I was wondering if our proxy support would feel coherent without that. But yeah, I understand it is workable without that and any such addition can be separate.

This is only for Qt network access. We don't use Qt for local connections.

Ack.

Based on review feedback

tests/test_daemon.cpp

multipass-ci-bot · 2020-02-11T18:44:07Z

Snap build available: snap refresh multipass --channel edge/pr1348

ricab

bors r+

1348: daemon_config: Detect proxy env var and use it if found r=ricab a=townsend2010 Fixes #885 Co-authored-by: Chris Townsend <[email protected]>

bors · 2020-02-11T19:30:07Z

Build failed

continuous-integration/travis-ci/push

townsend2010 · 2020-02-11T19:31:06Z

Ugh

bors r=ricab

1348: daemon_config: Detect proxy env var and use it if found r=ricab a=townsend2010 Fixes #885 Co-authored-by: Chris Townsend <[email protected]>

bors · 2020-02-11T20:10:30Z

Build succeeded

continuous-integration/travis-ci/push

url_downloader: Detect proxy env var and use it if found

ad06387

Fixes #885

townsend2010 force-pushed the detect-and-use-proxy branch from 4681126 to ad06387 Compare February 10, 2020 20:48

This comment has been minimized.

Sign in to view

ricab reviewed Feb 11, 2020

View reviewed changes

daemon_config: Move proxy discover into the daemon_config

3a8471a

Based on review feedback, it makes more sense to put the proxy discovery into the DaemonConfig code.

This comment has been minimized.

Sign in to view

Chris Townsend added 2 commits February 11, 2020 10:58

proxy_support: Accounts for socks type proxies in env var

eee45fd

Based on review feedback

tests: Add network proxy setup test

f7245b6

This comment has been minimized.

Sign in to view

ricab reviewed Feb 11, 2020

View reviewed changes

tests/test_daemon.cpp Show resolved Hide resolved

test: Add scope_guard to reset proxy when test finishes

a4883b6

townsend2010 changed the title ~~url_downloader: Detect proxy env var and use it if found~~ daemon_config: Detect proxy env var and use it if found Feb 11, 2020

ricab approved these changes Feb 11, 2020

View reviewed changes

bors bot added a commit that referenced this pull request Feb 11, 2020

Merge #1348

60a41d9

1348: daemon_config: Detect proxy env var and use it if found r=ricab a=townsend2010 Fixes #885 Co-authored-by: Chris Townsend <[email protected]>

bors bot added a commit that referenced this pull request Feb 11, 2020

Merge #1348

915e1e0

1348: daemon_config: Detect proxy env var and use it if found r=ricab a=townsend2010 Fixes #885 Co-authored-by: Chris Townsend <[email protected]>

bors bot merged commit a4883b6 into master Feb 11, 2020

bors bot deleted the detect-and-use-proxy branch February 11, 2020 20:10

daemon_config: Detect proxy env var and use it if found #1348

daemon_config: Detect proxy env var and use it if found #1348

Uh oh!

Conversation

townsend2010 commented Feb 10, 2020

Uh oh!

This comment has been minimized.

codecov bot commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

townsend2010 commented Feb 11, 2020

Uh oh!

townsend2010 commented Feb 11, 2020

Uh oh!

This comment has been minimized.

ricab commented Feb 11, 2020

Uh oh!

This comment has been minimized.

Uh oh!

multipass-ci-bot commented Feb 11, 2020

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Feb 11, 2020

Build failed

Uh oh!

townsend2010 commented Feb 11, 2020

Uh oh!

bors bot commented Feb 11, 2020

Build succeeded

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2020 •

edited

Loading