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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,5 @@ doc/source/tutorial/services.rst
# Pyenv
.python-version

# virtualenv
.venv
5 changes: 3 additions & 2 deletions awscli/customizations/s3/filegenerator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def is_special_file(path):
"""
This function checks to see if a special file. It checks if the
file is a character special device, block special device, FIFO, or
socket.
socket.
"""
mode = os.stat(path).st_mode
# Character special device.
Expand Down Expand Up @@ -318,8 +318,9 @@ def list_objects(self, s3_path, dir_op):
yield self._list_single_object(s3_path)
else:
lister = BucketLister(self._client)
request_payer = self.request_parameters.get('RequestPayer')
for key in lister.list_objects(bucket=bucket, prefix=prefix,
page_size=self.page_size):
page_size=self.page_size, request_payer=request_payer):
source_path, response_data = key
if response_data['Size'] == 0 and source_path.endswith('/'):
if self.operation_name == 'delete':
Expand Down
2 changes: 1 addition & 1 deletion awscli/customizations/s3/s3handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ def _format_local_path(self, path):


class DeleteRequestSubmitter(BaseTransferRequestSubmitter):
REQUEST_MAPPER_METHOD = None
REQUEST_MAPPER_METHOD = RequestParamsMapper.map_delete_params
RESULT_SUBSCRIBER_CLASS = DeleteResultSubscriber

def can_submit(self, fileinfo):
Expand Down
17 changes: 13 additions & 4 deletions awscli/customizations/s3/subcommands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.



class MvCommand(S3TransferCommand):
Expand All @@ -744,7 +744,7 @@ class RmCommand(S3TransferCommand):
USAGE = "<S3Uri>"
ARG_TABLE = [{'name': 'paths', 'nargs': 1, 'positional_arg': True,
'synopsis': USAGE}, DRYRUN, QUIET, RECURSIVE, INCLUDE,
EXCLUDE, ONLY_SHOW_ERRORS, PAGE_SIZE]
EXCLUDE, ONLY_SHOW_ERRORS, PAGE_SIZE, REQUEST_PAYER]


class SyncCommand(S3TransferCommand):
Expand All @@ -757,7 +757,7 @@ class SyncCommand(S3TransferCommand):
"<LocalPath> or <S3Uri> <S3Uri>"
ARG_TABLE = [{'name': 'paths', 'nargs': 2, 'positional_arg': True,
'synopsis': USAGE}] + TRANSFER_ARGS + \
[METADATA, METADATA_DIRECTIVE]
[METADATA, METADATA_DIRECTIVE, REQUEST_PAYER]


class MbCommand(S3Command):
Expand Down Expand Up @@ -985,11 +985,18 @@ def run(self):
'result_queue': result_queue,
}

fgen_request_parameters = {}
fgen_request_parameters = {
'RequestPayer': self.parameters.get('request_payer'),
}
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.

'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:

aws-cli/awscli/customizations/s3/subcommands.py

Lines 999 to 1007 in 9cdd69c

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.


# SSE-C may be neaded for HeadObject for copies/downloads/deletes
# If the operation is s3 to s3, the FileGenerator should use the
# copy source key and algorithm. Otherwise, use the regular
Expand Down Expand Up @@ -1111,6 +1118,8 @@ def __init__(self, cmd, parameters, usage):
self.parameters['follow_symlinks'] = True
if 'source_region' not in parameters:
self.parameters['source_region'] = None
if 'request_payer' not in parameters:
self.parameters['request_payer'] = None
if self.cmd in ['sync', 'mb', 'rb']:
self.parameters['dir_op'] = True
if self.cmd == 'mv':
Expand Down
22 changes: 20 additions & 2 deletions awscli/customizations/s3/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

kwargs = {'Bucket': bucket, 'PaginationConfig': {'PageSize': page_size}}
if prefix is not None:
kwargs['Prefix'] = prefix
if request_payer is not None:
kwargs['RequestPayer'] = request_payer

paginator = self._client.get_paginator('list_objects')
pages = paginator.paginate(**kwargs)
Expand Down Expand Up @@ -424,11 +426,13 @@ def map_put_object_params(cls, request_params, cli_params):
cls._set_metadata_params(request_params, cli_params)
cls._set_sse_request_params(request_params, cli_params)
cls._set_sse_c_request_params(request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_get_object_params(cls, request_params, cli_params):
"""Map CLI params to GetObject request params"""
cls._set_sse_c_request_params(request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_copy_object_params(cls, request_params, cli_params):
Expand All @@ -440,11 +444,13 @@ def map_copy_object_params(cls, request_params, cli_params):
cls._set_sse_request_params(request_params, cli_params)
cls._set_sse_c_and_copy_source_request_params(
request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_head_object_params(cls, request_params, cli_params):
"""Map CLI params to HeadObject request params"""
cls._set_sse_c_request_params(request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_create_multipart_upload_params(cls, request_params, cli_params):
Expand All @@ -453,21 +459,33 @@ def map_create_multipart_upload_params(cls, request_params, cli_params):
cls._set_sse_request_params(request_params, cli_params)
cls._set_sse_c_request_params(request_params, cli_params)
cls._set_metadata_params(request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_upload_part_params(cls, request_params, cli_params):
"""Map CLI params to UploadPart request params"""
cls._set_sse_c_request_params(request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_upload_part_copy_params(cls, request_params, cli_params):
"""Map CLI params to UploadPartCopy request params"""
cls._set_sse_c_and_copy_source_request_params(
request_params, cli_params)
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def map_delete_params(cls, request_params, cli_params):
cls._set_request_payer_param(request_params, cli_params)

@classmethod
def _set_request_payer_param(cls, request_params, cli_params):
if cli_params.get('request_payer'):
request_params['RequestPayer'] = cli_params['request_payer']

@classmethod
def _set_general_object_params(cls, request_params, cli_params):
# Paramters set in this method should be applicable to the following
# Parameters set in this method should be applicable to the following
# operations involving objects: PutObject, CopyObject, and
# CreateMultipartUpload.
general_param_translation = {
Expand Down