gsutil: Pass proxy information via Boto options.

gsutil: Pass proxy information via Boto options.

Since the move to gsutil 4.7, download_from_google_storage --no_auth has
been broken for people behind proxies, as it unsets the BOTO_CONFIG
environment variable despite gsutil requiring proxy information to be
set in a Boto configuration file.

This has been reported in
https://github.com/GoogleCloudPlatform/gsutil/issues/241, and it was
suggested we worked around the issue for now by passing the proxy
information ourselves via "gsutil -o".

Implement it by parsing the http_proxy environment variable and
transforming its contents into a series of "-o" arguments that are
passed to the actual gsutil script.

BUG=chromium:443523

[Raphael Kubo da Costa (rakuco), 2015-01-07 19:26:06]

raphael.kubo.da.costa@intel.com changed reviewers:
+ agable@chromium.org, hinoka@google.com, primiano@chromium.org

[Ryan Tseng, 2015-01-07 22:21:58]

This looks like a good solution.  A couple of comments.

https://codereview.chromium.org/841823002/diff/1/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/841823002/diff/1/gsutil.py#newcode125
gsutil.py:125: def get_boto_proxy_options_from_environment():
for completeness sake (since I've seen all these permutations before) lets also
look for ['http_proxy', 'HTTP_PROXY', 'HTTPS_PROXY', 'https_proxy']

https://codereview.chromium.org/841823002/diff/1/gsutil.py#newcode126
gsutil.py:126: """Converts the contents of the 'http_proxy' environment variable
into the
"""One line

More lines
...
"""

So rewrite this as:
"""Derive proxy options for gsutil from the environment.

<more description if needed>
"""

https://codereview.chromium.org/841823002/diff/1/gsutil.py#newcode133
gsutil.py:133: 'The "http_proxy" environment is set, but has an invalid format.
Its '
2 indents

[Raphael Kubo da Costa (rakuco), 2015-01-08 12:10:00]

Patch v2 should address all the comments. As I mentioned in my email to
infra-dev, the only possible drawbacks I see here are:
* Usernames and passwords are passed on the command line if they are part of the
environment variable.
* The environment variables are parsed and passed to gsutil even if it is not
invoked by download_from_google_storage.

[agable, 2015-01-08 20:39:01]

https://codereview.chromium.org/841823002/diff/20001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/841823002/diff/20001/gsutil.py#newcode132
gsutil.py:132: for env_var in ('http_proxy', 'HTTP_PROXY', 'https_proxy',
'HTTPS_PROXY'):
Shouldn't we prioritize an https proxy over an http one?

https://codereview.chromium.org/841823002/diff/20001/gsutil.py#newcode143
gsutil.py:143: 'format must be "http://[user[:password]]host:port".' %
proxy_variable)
[user[:password]@]host:port

[Raphael Kubo da Costa (rakuco), 2015-01-08 22:29:02]

https://codereview.chromium.org/841823002/diff/20001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/841823002/diff/20001/gsutil.py#newcode132
gsutil.py:132: for env_var in ('http_proxy', 'HTTP_PROXY', 'https_proxy',
'HTTPS_PROXY'):
On 2015/01/08 20:39:01, agable wrote:
> Shouldn't we prioritize an https proxy over an http one?

I don't have a strong opinion here, as the whole thing isn't standardized. I'll
change the order.

[Raphael Kubo da Costa (rakuco), 2015-01-08 22:30:41]

Patch v3 is up for review.

[Primiano Tucci (use gerrit), 2015-01-09 17:17:31]

Many thanks for taking the time of doing this.
LGTM with some small suggestions (please look at them) but I'm NOT an OWNER.

https://codereview.chromium.org/841823002/diff/40001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/841823002/diff/40001/gsutil.py#newcode132
gsutil.py:132: proxy_variable = None
I think you can squeeze lines 132-136 in:
vars = ('https_proxy', 'HTTPS_PROXY', 'http_proxy', 'HTTP_PROXY')
proxy_variable = next((os.environ[v] for v in vars if v in os.environ), None)

https://codereview.chromium.org/841823002/diff/40001/gsutil.py#newcode137
gsutil.py:137: if proxy_variable is None:
I think here you want to check: if not proxy_variable
If the variable is set but is empty I guess you want to bail out anyways.

https://codereview.chromium.org/841823002/diff/40001/gsutil.py#newcode142
gsutil.py:142: raise EnvironmentError(
Not sure you want to be so strict on the port. What if
http_proxy=http://myhost.corp ?
I'd make it more gentle and narrow the check only for hostname:
if proxy_url.hostname is None: raise ......

and below do something like
'Boto:proxy_port=%d' % (proxy_url.port or 80)

[Raphael Kubo da Costa, 2015-01-09 17:54:12]

kubito@gmail.com changed reviewers:
+ kubito@gmail.com

[Raphael Kubo da Costa, 2015-01-09 17:54:13]

https://codereview.chromium.org/841823002/diff/40001/gsutil.py
File gsutil.py (right):

https://codereview.chromium.org/841823002/diff/40001/gsutil.py#newcode137
gsutil.py:137: if proxy_variable is None:
On 2015/01/09 17:17:31, Primiano Tucci wrote:
> I think here you want to check: if not proxy_variable
> If the variable is set but is empty I guess you want to bail out anyways.

Doing this works, but if I only store os.environ[v] I can't say which variable
was used in the EnvironmentError message below. And then if the value is empty
the if check down there will fail with a proper message about the format being
wrong anyway.

https://codereview.chromium.org/841823002/diff/40001/gsutil.py#newcode142
gsutil.py:142: raise EnvironmentError(
On 2015/01/09 17:17:31, Primiano Tucci wrote:
> Not sure you want to be so strict on the port. What if
> http_proxy=http://myhost.corp ?
> I'd make it more gentle and narrow the check only for hostname:
> if proxy_url.hostname is None: raise ......
> 
> and below do something like
> 'Boto:proxy_port=%d' % (proxy_url.port or 80)

As far as I can see, httplib2 (which gsutil uses) requires both the hostname and
the port to be set, otherwise the port will be set to 0 by default.

[Raphael Kubo da Costa (rakuco), 2015-01-16 17:08:04]

https://codereview.chromium.org/844373002 landed instead, closing this one.