[download_from_google_storage] Don't list ALL objects to check for ACLs

[download_from_google_storage] Don't list ALL objects to check for ACLs

Currently check_bucket_permissions() in download_from_google_storage.py
performs a gsutil ls gs://bucket to determine whether the user has access
to the bucket or not.
This can be an EXTREMELY expensive operation (~minutes) if the bucket in
question has a lot of objects in the root (real case: chrome-telemetry).
It is worth noting that check_bucket_permissions() is not called just for
uploads but also for downloads, hence this is slowing down all invocations
of gclient sync on users and bots machine.
Removing the check_bucket_permissions() and let gsutil fail with an
Unauthorized error message if ACLs are not met.

BUG=458059
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=294083

[Primiano Tucci (use gerrit), 2015-02-12 11:46:26]

primiano@chromium.org changed reviewers:
+ hinoka@chromium.org, mmoss@chromium.org

[Primiano Tucci (use gerrit), 2015-02-12 11:46:26]

PTAL to this minutes-saving 3 characters CL :)

[Sami, 2015-02-12 11:50:53]

On 2015/02/12 at 11:46:26, primiano wrote:
> PTAL to this minutes-saving 3 characters CL :)

Neat trick! Non-owner LGTM. Please link this to BUG=458059.

[picksi, 2015-02-12 13:52:09]

for what its worth

gsutil.py ls gs://chrome-telemetry

works fine, but:

gsutil.py ls gs://chrome-telemetry/..

Doesn't, it gives me:

GSResponseError: status=401, code=AuthenticationRequired, reason=Unauthorized.

This might be because I ctrl-C out of the first ls? Also following this error
all subsequent ls's fail too. I assume this is a security thing.

[Primiano Tucci (use gerrit), 2015-02-12 13:59:50]

Actually self NOT LGTM
This doesn't seem to work with all buckets. We need to figure out something
smarter.

[Primiano Tucci (use gerrit), 2015-02-12 14:09:45]

On 2015/02/12 13:59:50, Primiano Tucci wrote:
> Actually self NOT LGTM
> This doesn't seem to work with all buckets. We need to figure out something
> smarter.

I see the problem. This trick actually works with the gsutil binary in my PATH
(version: 4.6) but not with the gsutil checked into depot toosl (3.25).
Is there any chance that gsutil in depot_tools can be updated?

[Michael Moss, 2015-02-12 16:39:22]

https://codereview.chromium.org/923473002/diff/1/download_from_google_storage.py
File download_from_google_storage.py (right):

https://codereview.chromium.org/923473002/diff/1/download_from_google_storage...
download_from_google_storage.py:109: code, _, ls_err = gsutil.check_call('ls',
base_url + '/..')
Based on your description of "only ls the bucket itself, and not its contents",
it sounds like 'ls -b' might be the proper solution.

[Primiano Tucci (use gerrit), 2015-02-12 17:25:39]

On 2015/02/12 16:39:22, Michael Moss wrote:
>
https://codereview.chromium.org/923473002/diff/1/download_from_google_storage.py
> File download_from_google_storage.py (right):
> 
>
https://codereview.chromium.org/923473002/diff/1/download_from_google_storage...
> download_from_google_storage.py:109: code, _, ls_err = gsutil.check_call('ls',
> base_url + '/..')
> Based on your description of "only ls the bucket itself, and not its
contents",
> it sounds like 'ls -b' might be the proper solution.

Oh, I missed that (and I spent some good 15 minutes on the help page).
Unfortunately, I just checked and this has a similar problem of "..": works as
expected in the new gsutil (4.6), but sucks (doesn't do any remote check) in the
current one checked in depot-tools (3.25):

$ gsutil ls -b gs://non-existent-blah/  # this is 4.6
NotFoundException: 404 gs://non-existent-blah bucket does not exist.

$ /s/infra/depot_tools/third_party/gsutil/gsutil ls -b gs://non-existent-blah/
gs://non-existent-blah/

[hinoka, 2015-02-12 18:03:54]

fwiw this script uses gsutil 4.7.  The incantation is "gsutil.py --force-version
4.7 ls ..."

https://codereview.chromium.org/923473002/diff/1/download_from_google_storage.py
File download_from_google_storage.py (right):

https://codereview.chromium.org/923473002/diff/1/download_from_google_storage...
download_from_google_storage.py:108: def check_bucket_permissions(base_url,
gsutil):
I'd actually be okay with removing this entirely.  The intention for this was to
early exit if it detects the lack of permissions.

In reality, bucket and object acls are separate, so having bucket permission
doesn't necessarily mean we have object permissions, and vice verca.  Its good
enough to see if "gsutil cp x y" fails later on, and produce a good message on
403.

[Primiano Tucci (use gerrit), 2015-02-13 10:46:18]

New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org

[Primiano Tucci (use gerrit), 2015-02-13 10:47:31]

Thanks.
Agree with you, removing the check comletely. PTAL to the new patchset.

P.S. While testing this I also found out a bug in gsutil.
See https://github.com/GoogleCloudPlatform/gsutil/pull/251
If the PR is accepted you should probably cherry-pick that and roll gsutil in
depot_tools ;-)

[hinoka, 2015-02-13 21:17:47]

lgtm

https://codereview.chromium.org/923473002/diff/20001/gcl.sha1
File gcl.sha1 (right):

https://codereview.chromium.org/923473002/diff/20001/gcl.sha1#newcode1
gcl.sha1:1: a77e5cab00cef6aac88e16f4e7889c796a0eeb3a
This supposed to be here?

[Primiano Tucci (use gerrit), 2015-02-16 15:49:37]

New patchsets have been uploaded after l-g-t-m from hinoka@chromium.org

[Primiano Tucci (use gerrit), 2015-02-16 15:50:09]

https://codereview.chromium.org/923473002/diff/20001/gcl.sha1
File gcl.sha1 (right):

https://codereview.chromium.org/923473002/diff/20001/gcl.sha1#newcode1
gcl.sha1:1: a77e5cab00cef6aac88e16f4e7889c796a0eeb3a
On 2015/02/13 21:17:47, hinoka wrote:
> This supposed to be here?

Oh good catch, no that was a spurious result of an experiment :)

[Primiano Tucci (use gerrit), 2015-02-16 15:58:59]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-02-16 15:59:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923473002/40001

[commit-bot: I haz the power, 2015-02-16 16:02:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-16 16:02:35]

Presubmit check for 923473002-40001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    depot_tools/download_from_google_storage.py
    depot_tools/upload_to_google_storage.py

Presubmit checks took 166.8s to calculate.

[Primiano Tucci (use gerrit), 2015-02-17 09:00:54]

primiano@chromium.org changed reviewers:
+ szager@chromium.org
- mmoss@chromium.org

[Primiano Tucci (use gerrit), 2015-02-17 09:00:55]

+szager for OWNERS stamp.

[szager1, 2015-02-17 11:53:39]

lgtm

[Primiano Tucci (use gerrit), 2015-02-17 12:29:49]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-02-17 12:30:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923473002/40001

[commit-bot: I haz the power, 2015-02-17 12:33:37]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=294083

[angiegvalg, 2015-04-30 00:28:53]

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