Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add request_payer parameter to sync, cp, and rm subcommands #3016

Closed

Conversation

sp-niemand
Copy link

@sp-niemand sp-niemand commented Dec 5, 2017

Adds support for Requester Pays buckets to some aws s3 subcommands.

Addresses several related issues:
#2845
#2557

Also this (?):
#797

@iandees
Copy link

iandees commented Dec 28, 2017

I applied this patch and aws s3 sync between a requester-pays bucket in eu-central-1 and a bucket I own in us-east-1 does not work:

fatal error: An error occurred (AccessDenied) when calling the ListObjects operation: Access Denied

@sp-niemand
Copy link
Author

sp-niemand commented Dec 28, 2017

@iandees Are both buckets "requester pays"? Did you run the command from your local machine? Also, were there any "files" in the target "folder"?

@iandees
Copy link

iandees commented Dec 28, 2017

The from bucket is requester pays in eu-central-1 (eox-s2maps) and the to bucket is one I created in us-east-1.

@sp-niemand
Copy link
Author

@iandees Thank you. Will check it.

@iandees
Copy link

iandees commented Dec 29, 2017

Running a sync to my local directory works, so it is likely that I didn't set up permissions for the destination bucket properly:

$ aws s3 sync --source-region=eu-central-1 --request-payer=requester s3://eox-s2maps /home/ec2-user/eox-s2maps
download: s3://eox-s2maps/LICENSE to ../../../../../home/ec2-user/eox-s2maps/LICENSE
download: s3://eox-s2maps/README to ../../../../../home/ec2-user/eox-s2maps/README

@iandees
Copy link

iandees commented Dec 29, 2017

Yep, I fixed my permissions (I was using the wrong AWS profile when calling awscli) and this patch works. I hope it gets merged in!

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request. The interface and approach looks fine to me. I just had some comments related to cleaning up how it gets plumbed in to make more consistent with how arguments have been added in the past. The only other thing that I would really like to see is tests. Specifically it would be great to have:

  • Unit tests for the functionality added to BucketLister and RequestParamsMapper

  • Functional tests to make sure all of the commands properly map in the RequestPayer method gets mapped in correctly.

  • A couple of integration tests for sanity sake to make sure that it works end to end when hitting the s3 servers. I would imagine you would have one case for cp, sync, and maybe rm.

Given there is some work left to get the PR in a place we would be comfortable merging, we would be willing to build on top of your PR and champion it till gets merged. However if you want to continue working on it till completion, we would happily continue giving feedback till it gets merged. Let us know and thanks again!

@@ -357,10 +357,12 @@ def __init__(self, client, date_parser=_date_parser):
self._client = client
self._date_parser = date_parser

def list_objects(self, bucket, prefix=None, page_size=None):
def list_objects(self, bucket, prefix=None, page_size=None, request_payer=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that I would prefer that we made this more generalized such as an extra_args that takes a dictionary of extra arguments that get passed in. That way if we want to pass information like Delimeter or UrlEncoding it would be easier.

@@ -725,7 +725,7 @@ class CpCommand(S3TransferCommand):
"or <S3Uri> <S3Uri>"
ARG_TABLE = [{'name': 'paths', 'nargs': 2, 'positional_arg': True,
'synopsis': USAGE}] + TRANSFER_ARGS + \
[METADATA, METADATA_DIRECTIVE, EXPECTED_SIZE, RECURSIVE]
[METADATA, METADATA_DIRECTIVE, EXPECTED_SIZE, RECURSIVE, REQUEST_PAYER]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the cp and sync, it would be great if this can be added to the TRANSFER_ARGS list so it will get included in the mv command as well and it does not have to be explicitly added to each list for those commands.

rgen_request_parameters = {
'RequestPayer': self.parameters.get('request_payer'),
}
rgen_kwargs['request_parameters'] = rgen_request_parameters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for the request parameters that get passed to the FileGenerator's, I think I would prefer to do something more like this:

RequestParamsMapper.map_head_object_params(
fgen_head_object_params, self.parameters)
if paths_type == 's3s3':
RequestParamsMapper.map_head_object_params(
fgen_head_object_params, {
'sse_c': self.parameters.get('sse_c_copy_source'),
'sse_c_key': self.parameters.get('sse_c_copy_source_key')
}
)

where we:

  1. Add a key ListObjects to both the fgen_request_parameters and rgen_request_parameters parameters that is an empty dictionary.

  2. Use the RequestParamsMapper class to map the RequesterPayer parameter into those parameter dictionaries based on if the argument was provided to the commandline.

I am mainly suggesting this because:

  1. There is already precedence with how we set the HeadObject key in the request_parameters
  2. It will make it easier to add future parameters for ListObjects.

fgen_head_object_params = {}
fgen_request_parameters['HeadObject'] = fgen_head_object_params
fgen_kwargs['request_parameters'] = fgen_request_parameters

rgen_request_parameters = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question that I have is for a cross-copy sync command that has one bucket that uses requester pays and the other one that does not use requester pays, is there any unintentional side effects of passing RequestPayer to a ListObjects (or any other) call for the bucket where request payer is not enabled. I do not think so but I would need to double check this. Just wanted to see if you ran into this at all.

@sp-niemand
Copy link
Author

@kyleknap I was waiting for detailed feedback from you, guys :) Thank you.
I'll try to finish this myself.

@aquanyc
Copy link

aquanyc commented Jan 30, 2018

@sp-niemand

We urgently need this as well.

Please see email I sent your github email about this.

@kyleknap
Copy link
Contributor

Hey @sp-niemand if you do not mind, I am going to start working on pushing this to completion. There is fair amount of users now that are eagerly waiting for this to get merged in. Let me know if there is any issues in me doing so.

@sp-niemand
Copy link
Author

@kyleknap Yes, please.
Sadly, I didn't find time to finalize this.

@kyleknap
Copy link
Contributor

No worries. Thanks for doing the initial stab at this. I will build on top of your commit.

@aquanyc
Copy link

aquanyc commented Feb 14, 2018

@kyleknap
Can you provide any idea of timeline for this to be done ? Thanks

@kyleknap
Copy link
Contributor

@aquanyc I just submitted a PR that added the necessary updates for this feature: #3147. Once that goes through reviews and feedback is incorporated, it will be merged and part of the subsequent CLI release.

@sp-niemand I kept your commit but decided to open up a new PR when sending in my changes. I mainly did this because I did a rebase on the develop branch so I did not want to force push to your branch. I would appreciate it if you can take a look at it as well to make sure I did not miss anything.

I'm closing out this PR in favor of: #3147. For those watching this PR, feel free to hop over to the PR I just recently opened and try the implementation out as that PR should be the one that gets merged in the end.

@kyleknap kyleknap closed this Feb 14, 2018
@diehlaws diehlaws added feature-request A feature should be added or improved. and removed enhancement labels Jan 4, 2019
@justnance justnance added enhancement and removed feature-request A feature should be added or improved. labels Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants