Support user project in tasks

Support user project in tasks.
Ensure user project is propagated to nested gsutil_api instances.

[markhjelm, 2017-04-24 17:27:09]

Ensures user project is propagated to nested gsutil_api instances via Task
object.

[thobrla, 2017-04-24 19:01:39]

https://codereview.appspot.com/322910043/diff/1/gslib/cloud_api_helper.py
File gslib/cloud_api_helper.py (right):

https://codereview.appspot.com/322910043/diff/1/gslib/cloud_api_helper.py#new...
gslib/cloud_api_helper.py:56: user_project: Project to be billed to, added as
query param
Super nit: end with period for consistency

https://codereview.appspot.com/322910043/diff/1/gslib/cloud_api_helper.py#new...
gslib/cloud_api_helper.py:68: url = url + '&userProject=' + user_project
url += '&userProject%s' % user_project

https://codereview.appspot.com/322910043/diff/1/gslib/command.py
File gslib/command.py (right):

https://codereview.appspot.com/322910043/diff/1/gslib/command.py#newcode2108
gslib/command.py:2108: if self.thread_gsutil_api:
Changing the member variables of the CloudApi instance from outside of it makes
it difficult to reason about.  If we want to include a default for convenience,
I don't object to that, but here we're effectively modifying the class directly
instead of passing an argument that's specific to the Task.

This raises the larger question about the expected behavior for mixed-mode
calls.  Since the API is strict, if I supply a user_project, do I need to
round-trip to all buckets included in the arguments to determine whether or not
they're requester-pays?

[markhjelm, 2017-04-24 19:45:31]

On 2017/04/24 19:01:39, thobrla wrote:
> https://codereview.appspot.com/322910043/diff/1/gslib/cloud_api_helper.py
> File gslib/cloud_api_helper.py (right):
> 
>
https://codereview.appspot.com/322910043/diff/1/gslib/cloud_api_helper.py#new...
> gslib/cloud_api_helper.py:56: user_project: Project to be billed to, added as
> query param
> Super nit: end with period for consistency

Done.


> 
>
https://codereview.appspot.com/322910043/diff/1/gslib/cloud_api_helper.py#new...
> gslib/cloud_api_helper.py:68: url = url + '&userProject=' + user_project
> url += '&userProject%s' % user_project

Done, as "url += '&userProject=%s' % user_project". [Someday I should learn
Python.]


> 
> https://codereview.appspot.com/322910043/diff/1/gslib/command.py
> File gslib/command.py (right):
> 
> https://codereview.appspot.com/322910043/diff/1/gslib/command.py#newcode2108
> gslib/command.py:2108: if self.thread_gsutil_api:
> Changing the member variables of the CloudApi instance from outside of it
makes
> it difficult to reason about.  If we want to include a default for
convenience,
> I don't object to that, but here we're effectively modifying the class
directly
> instead of passing an argument that's specific to the Task.

This is unlikely to be a good implementation, but I found it hard to track how
and when CloudApi instances get created and this was the cleanest working
solution
I could do in the time I had (short of just sticking it in a process-global
variable).
I'd be glad to revisit this, and we might well be required to when I implement
this
for XML. But for now, can you see a quick solution that would be better?

> 
> This raises the larger question about the expected behavior for mixed-mode
> calls.  Since the API is strict, if I supply a user_project, do I need to
> round-trip to all buckets included in the arguments to determine whether or
not
> they're requester-pays?

Yes, we'll have to address this for GA. For EAP the model is that a user has to
know whether or not a bucket/object is requester pays before trying to do
anything,
and there is no mixed mode. I'll add a specific note on this problem in the GA
design doc.

[houglum, 2017-04-24 20:19:14]

https://codereview.appspot.com/322910043/diff/20001/gslib/cloud_api_helper.py
File gslib/cloud_api_helper.py (right):

https://codereview.appspot.com/322910043/diff/20001/gslib/cloud_api_helper.py...
gslib/cloud_api_helper.py:68: url += '&userProject=%s' % user_project
Can we always be sure that existing query parameters already exist (and thus
that the ? character is already present) in the media link?  If not, I'd
recommend using a helper library to add the query param, as mentioned in this
S.O. question:
http://stackoverflow.com/questions/4293460/how-to-add-custom-parameters-to-an...

[thobrla, 2017-04-24 20:54:53]

https://codereview.appspot.com/322910043/diff/1/gslib/command.py
File gslib/command.py (right):

https://codereview.appspot.com/322910043/diff/1/gslib/command.py#newcode2108
gslib/command.py:2108: if self.thread_gsutil_api:
On 2017/04/24 19:01:39, thobrla wrote:
> Changing the member variables of the CloudApi instance from outside of it
makes
> it difficult to reason about.  If we want to include a default for
convenience,
> I don't object to that, but here we're effectively modifying the class
directly
> instead of passing an argument that's specific to the Task.
> 
> This raises the larger question about the expected behavior for mixed-mode
> calls.  Since the API is strict, if I supply a user_project, do I need to
> round-trip to all buckets included in the arguments to determine whether or
not
> they're requester-pays?

For some reason the inline reply did not include your comments in this thread.

If for EAP we don't support mixed-mode and the user must know ahead of time,
then we should add a TODO comment describing the eventual intended state (and
we'll need to be clear about that in the release notes as well, since users
could potentially get confused).

If for EAP we're going with a fixed solution where user_project is always passed
(or never passed), then it should be quick & sufficient to pass the user project
in when you create the per-thread CloudApiDelegator here:

https://github.com/GoogleCloudPlatform/gsutil/blob/master/gslib/command.py#L2062

However, I would note in your GA design doc that you've chosen to implement it
in this fixed way and that you'll likely need to revert/unwind what you've done
for EAP.

[markhjelm, 2017-04-24 21:57:13]

> If for EAP we don't support mixed-mode and the user must know ahead of time,
> then we should add a TODO comment describing the eventual intended state (and
> we'll need to be clear about that in the release notes as well, since users
> could potentially get confused).

I will certainly identify this issue in the release notes. The "right" answer
for GA is open for discussion (and will be).


> 
> If for EAP we're going with a fixed solution where user_project is always
passed
> (or never passed), then it should be quick & sufficient to pass the user
project
> in when you create the per-thread CloudApiDelegator here:
> 
>
https://github.com/GoogleCloudPlatform/gsutil/blob/master/gslib/command.py#L2062
> 

I tried heading down this path. You also need to plumb it down through
WorkerPool and WorkerThread. And I've stopped because it's not getting
propagated to sub processes (?) for some reason. If we can run with the solution
that seems to work, I'll commit to improving it in the near future (esp. if we
want/need to support a mixed-mode mechanism).

[markhjelm, 2017-04-24 22:03:50]

On 2017/04/24 20:19:14, houglum wrote:
> https://codereview.appspot.com/322910043/diff/20001/gslib/cloud_api_helper.py
> File gslib/cloud_api_helper.py (right):
> 
>
https://codereview.appspot.com/322910043/diff/20001/gslib/cloud_api_helper.py...
> gslib/cloud_api_helper.py:68: url += '&userProject=%s' % user_project
> Can we always be sure that existing query parameters already exist (and thus
> that the ? character is already present) in the media link?  If not, I'd
> recommend using a helper library to add the query param, as mentioned in this
> S.O. question:
>
http://stackoverflow.com/questions/4293460/how-to-add-custom-parameters-to-an...

Can't be sure to 100%, so it's good to address. I chose implement the solution
very simply. Sufficient?

[thobrla, 2017-04-24 22:08:15]

Do you need to pass the argument to the ConsumerPool creation at
https://github.com/GoogleCloudPlatform/gsutil/blob/master/gslib/command.py#L1233
?

If we can identify an architectural restriction that requires us to modify a
CloudApi instance during thread execution, then I'm open to consider
alternatives.  But I'd like to understand why it's infeasible (or a large
effort) before we violate encapsulation of CloudApi.  AFAICT this is a simple
plumb-through from ConsumerPool->WorkerPool->WorkerThread

[markhjelm, 2017-04-26 17:43:19]

I found the minimal, non-invasive version of the fix to command.py. Please take
a look.

[thobrla, 2017-04-26 17:53:10]

LGTM

[markhjelm, 2017-04-26 18:07:03]

Matt, any concerns?

FYI, I looked at using urlparse, but it seemed unwieldy at best. The current
string hacking on the url certainly isn't safe in general, but I believe it's
fine for this use case for now.

[markhjelm, 2017-04-26 20:28:16]

Comments copied from https://codereview.appspot.com/316480043:

>
https://codereview.appspot.com/316480043/diff/60001/gslib/addlhelp/command_op...
> File gslib/addlhelp/command_opts.py (right):
> 
>
https://codereview.appspot.com/316480043/diff/60001/gslib/addlhelp/command_op...
> gslib/addlhelp/command_opts.py:123: -u          Allows you to specify a user
> project to be billed for the request.
> Do we bill this project even if the bucket is not requester-pays?  If so, this
> is a bit misleading.

For EAP the user project is prohibited for non-requester-pays buckets. We will
be looking at what behavior we want for GA and adjusting as necessary. The doc
for -u isn't unambiguous, I admit, but this is EAP and there will be release
notes.


> Also this only works for Google Cloud Storage, right (not s3)?

True. S3 infers what to bill to from the user's authenticated identity. 


> https://codereview.appspot.com/316480043/diff/60001/gslib/command.py
> File gslib/command.py (right):
> 
>
https://codereview.appspot.com/316480043/diff/60001/gslib/command.py#newcode2068
> gslib/command.py:2068: self.thread_gsutil_api = CloudApiDelegator(
> Don't we need to plumb user project through here?

Addressed.


> https://codereview.appspot.com/316480043/diff/60001/gslib/gcs_json_api.py
> File gslib/gcs_json_api.py (right):
> 
>
https://codereview.appspot.com/316480043/diff/60001/gslib/gcs_json_api.py#new...
> gslib/gcs_json_api.py:1180: with self._ApitoolsRequestHeaders(addl_headers):
> Don't you need to provide this header for resumable uploads as well?

No headers for JSON in the current version. The user project gets carried along
on the url used for resuming.


> It seems like there is a serious testing gap if our tests didn't catch this or
> the -m case.

Agreed. I will be spending numerous days during the GA development effort
fleshing out all the tests.

[houglum, 2017-04-26 21:42:54]

LGTM

[markhjelm, 2017-04-26 21:50:23]

Pushed to origin/requester_pays.
Closing...