From 9188c04faf61ce4fc91c84db37e8ba875e084864 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Mon, 26 Oct 2015 17:21:29 -0700 Subject: [PATCH 1/7] Fix regression from client switch over The configservice subscribe command was using an attribute that no longer existed. --- .../customizations/configservice/subscribe.py | 2 +- .../configservice/test_subscribe.py | 23 ++++++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/awscli/customizations/configservice/subscribe.py b/awscli/customizations/configservice/subscribe.py index c19b95e18011..16f2081fd18c 100644 --- a/awscli/customizations/configservice/subscribe.py +++ b/awscli/customizations/configservice/subscribe.py @@ -151,7 +151,7 @@ def _check_bucket_exists(self, bucket): return s3_bucket_exists(self._s3_client, bucket) def _create_bucket(self, bucket): - region_name = self._s3_client._endpoint.region_name + region_name = self._s3_client.meta.region_name params = { 'Bucket': bucket } diff --git a/tests/unit/customizations/configservice/test_subscribe.py b/tests/unit/customizations/configservice/test_subscribe.py index da7d4bf4e62b..91e24154c11a 100644 --- a/tests/unit/customizations/configservice/test_subscribe.py +++ b/tests/unit/customizations/configservice/test_subscribe.py @@ -11,6 +11,7 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import mock +import botocore.session from botocore.exceptions import ClientError from awscli.testutils import unittest @@ -21,7 +22,8 @@ class TestS3BucketHelper(unittest.TestCase): def setUp(self): - self.s3_client = mock.Mock() + self.session = botocore.session.get_session() + self.s3_client = mock.Mock(self.session.create_client('s3')) self.helper = S3BucketHelper(self.s3_client) self.error_response = { 'Error': { @@ -53,7 +55,7 @@ def test_bucket_exists(self): def test_bucket_no_exist(self): name = 'MyBucket/MyPrefix' self.s3_client.head_bucket.side_effect = self.bucket_no_exists_error - self.s3_client._endpoint.region_name = 'us-east-1' + self.s3_client.meta.region_name = 'us-east-1' bucket, prefix = self.helper.prepare_bucket(name) # Ensure that the create bucket was called with the proper args. self.s3_client.create_bucket.assert_called_with( @@ -66,7 +68,7 @@ def test_bucket_no_exist(self): def test_bucket_no_exist_with_location_constraint(self): name = 'MyBucket/MyPrefix' self.s3_client.head_bucket.side_effect = self.bucket_no_exists_error - self.s3_client._endpoint.region_name = 'us-west-2' + self.s3_client.meta.region_name = 'us-west-2' bucket, prefix = self.helper.prepare_bucket(name) # Ensure that the create bucket was called with the proper args. self.s3_client.create_bucket.assert_called_with( @@ -113,7 +115,9 @@ def test_output_create_bucket(self): class TestSNSTopicHelper(unittest.TestCase): def setUp(self): - self.sns_client = mock.Mock() + self.session = botocore.session.get_session() + self.sns_client = mock.Mock(self.session.create_client( + 'sns', 'us-east-1')) self.helper = SNSTopicHelper(self.sns_client) def test_sns_topic_by_name(self): @@ -151,17 +155,20 @@ def test_output_new_topic(self): class TestSubscribeCommand(unittest.TestCase): def setUp(self): - self.session = mock.Mock() + self.session = botocore.session.get_session() # Set up the client mocks. - self.s3_client = mock.Mock() - self.sns_client = mock.Mock() - self.config_client = mock.Mock() + self.s3_client = mock.Mock(self.session.create_client('s3')) + self.sns_client = mock.Mock(self.session.create_client( + 'sns', 'us-east-1')) + self.config_client = mock.Mock(self.session.create_client( + 'config', 'us-east-1')) self.config_client.describe_configuration_recorders.return_value = \ {'ConfigurationRecorders': []} self.config_client.describe_delivery_channels.return_value = \ {'DeliveryChannels': []} + self.session = mock.Mock(self.session) self.session.create_client.side_effect = [ self.s3_client, self.sns_client, From cddc86f68f7373532989f582631acb31eac6d461 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 28 Oct 2015 13:11:22 -0700 Subject: [PATCH 2/7] Add some functional test for subscribe --- .../configservice/test_subscribe.py | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 tests/functional/configservice/test_subscribe.py diff --git a/tests/functional/configservice/test_subscribe.py b/tests/functional/configservice/test_subscribe.py new file mode 100644 index 000000000000..92c13b1764af --- /dev/null +++ b/tests/functional/configservice/test_subscribe.py @@ -0,0 +1,146 @@ +# Copyright 2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"). You +# may not use this file except in compliance with the License. A copy of +# the License is located at +# +# http://aws.amazon.com/apache2.0/ +# +# or in the "license" file accompanying this file. This file is +# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF +# ANY KIND, either express or implied. See the License for the specific +# language governing permissions and limitations under the License. +from awscli.testutils import BaseAWSCommandParamsTest + + +class TestSubscribe(BaseAWSCommandParamsTest): + prefix = 'configservice subscribe' + + def setUp(self): + super(TestSubscribe, self).setUp() + self.parsed_responses = [ + {}, # S3 HeadBucket + {'TopicArn': 'my-topic-arn'}, # SNS CreateTopic + {}, # PutConfigurationRecorder + {}, # PutDeliveryChannel + {}, # StartConfigurationRecorder + {'ConfigurationRecorders': {}}, # DescribeConfigurationRecorders + {'DeliveryChannels': {}} # DescribeDeliveryChannels + ] + + def test_subscribe_when_bucket_exists_and_new_sns_topic(self): + self.prefix += ' --s3-bucket mybucket --sns-topic mytopic' + self.prefix += ' --iam-role myrole' + self.run_cmd(self.prefix) + + self.assertEqual(len(self.operations_called), 7) + # S3 operations + self.assertEqual(self.operations_called[0][0].name, 'HeadBucket') + self.assertEqual(self.operations_called[0][1], {'Bucket': 'mybucket'}) + + # SNS operations + self.assertEqual(self.operations_called[1][0].name, 'CreateTopic') + self.assertEqual(self.operations_called[1][1], {'Name': 'mytopic'}) + + # Config operations + self.assertEqual( + self.operations_called[2][0].name, + 'PutConfigurationRecorder') + self.assertEqual( + self.operations_called[2][1], + {'ConfigurationRecorder': {'name': 'default', 'roleARN': 'myrole'}} + ) + self.assertEqual( + self.operations_called[3][0].name, + 'PutDeliveryChannel') + self.assertEqual( + self.operations_called[3][1], + {'DeliveryChannel': { + 'name': 'default', + 's3BucketName': 'mybucket', + 'snsTopicARN': 'my-topic-arn'}} + ) + self.assertEqual( + self.operations_called[4][0].name, + 'StartConfigurationRecorder') + self.assertEqual( + self.operations_called[4][1], + {'ConfigurationRecorderName': 'default'} + ) + self.assertEqual( + self.operations_called[5][0].name, + 'DescribeConfigurationRecorders') + self.assertEqual(self.operations_called[5][1], {}) + self.assertEqual( + self.operations_called[6][0].name, + 'DescribeDeliveryChannels') + self.assertEqual(self.operations_called[6][1], {}) + + def test_subscribe_when_bucket_exists_and_sns_topic_arn_provided(self): + self.parsed_responses.pop(1) + self.prefix += ' --s3-bucket mybucket --sns-topic arn:mytopic' + self.prefix += ' --iam-role myrole' + self.run_cmd(self.prefix) + + self.assertEqual(len(self.operations_called), 6) + # S3 operations + self.assertEqual(self.operations_called[0][0].name, 'HeadBucket') + self.assertEqual(self.operations_called[0][1], {'Bucket': 'mybucket'}) + + # Config operations + self.assertEqual( + self.operations_called[1][0].name, + 'PutConfigurationRecorder') + self.assertEqual( + self.operations_called[1][1], + {'ConfigurationRecorder': {'name': 'default', 'roleARN': 'myrole'}} + ) + self.assertEqual( + self.operations_called[2][0].name, + 'PutDeliveryChannel') + self.assertEqual( + self.operations_called[2][1], + {'DeliveryChannel': { + 'name': 'default', + 's3BucketName': 'mybucket', + 'snsTopicARN': 'arn:mytopic'}} + ) + self.assertEqual( + self.operations_called[3][0].name, + 'StartConfigurationRecorder') + self.assertEqual( + self.operations_called[3][1], + {'ConfigurationRecorderName': 'default'} + ) + self.assertEqual( + self.operations_called[4][0].name, + 'DescribeConfigurationRecorders') + self.assertEqual(self.operations_called[4][1], {}) + self.assertEqual( + self.operations_called[5][0].name, + 'DescribeDeliveryChannels') + self.assertEqual(self.operations_called[5][1], {}) + + def test_subscribe_when_bucket_needs_to_be_created(self): + # Make the HeadObject request fail now and should try to create a new + # bucket. + self.parsed_responses = None + self.http_response.status_code = 404 + self.parsed_response = {'Error': {'Code': 404, 'Message': ''}} + + self.prefix += ' --s3-bucket mybucket --sns-topic arn:mytopic' + self.prefix += ' --iam-role myrole' + self.run_cmd(self.prefix, expected_rc=255) + # This will fail because there is no current way to specify + # a change in status code in BaseAWSCommandParamsTest + # As of now only one status code applies to all parsed responses. + # Therefore the CreateBucket will be the one that receives the 404. + # But it does not matter because we are just checking that the bucket + # is attempted to be made if we determine the bucket does not exist + self.assertEqual(len(self.operations_called), 2) + + self.assertEqual(self.operations_called[0][0].name, 'HeadBucket') + self.assertEqual(self.operations_called[0][1], {'Bucket': 'mybucket'}) + self.assertEqual(self.operations_called[1][0].name, 'CreateBucket') + self.assertEqual( + self.operations_called[1][1]['Bucket'], 'mybucket') From e70abb6114415106680dcd40754b30ee8c3a694f Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 28 Oct 2015 14:30:17 -0700 Subject: [PATCH 3/7] Update functional test --- .../configservice/test_subscribe.py | 169 +++++++++--------- 1 file changed, 81 insertions(+), 88 deletions(-) diff --git a/tests/functional/configservice/test_subscribe.py b/tests/functional/configservice/test_subscribe.py index 92c13b1764af..3c5188ef3c2c 100644 --- a/tests/functional/configservice/test_subscribe.py +++ b/tests/functional/configservice/test_subscribe.py @@ -10,7 +10,10 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import mock + from awscli.testutils import BaseAWSCommandParamsTest +from awscli.customizations.configservice.subscribe import S3BucketHelper class TestSubscribe(BaseAWSCommandParamsTest): @@ -34,47 +37,39 @@ def test_subscribe_when_bucket_exists_and_new_sns_topic(self): self.run_cmd(self.prefix) self.assertEqual(len(self.operations_called), 7) - # S3 operations - self.assertEqual(self.operations_called[0][0].name, 'HeadBucket') - self.assertEqual(self.operations_called[0][1], {'Bucket': 'mybucket'}) - - # SNS operations - self.assertEqual(self.operations_called[1][0].name, 'CreateTopic') - self.assertEqual(self.operations_called[1][1], {'Name': 'mytopic'}) + list_of_operation_names_called = [] + list_of_parameters_called = [] + for operation_called in self.operations_called: + list_of_operation_names_called.append(operation_called[0].name) + list_of_parameters_called.append(operation_called[1]) - # Config operations - self.assertEqual( - self.operations_called[2][0].name, - 'PutConfigurationRecorder') - self.assertEqual( - self.operations_called[2][1], - {'ConfigurationRecorder': {'name': 'default', 'roleARN': 'myrole'}} - ) self.assertEqual( - self.operations_called[3][0].name, - 'PutDeliveryChannel') - self.assertEqual( - self.operations_called[3][1], - {'DeliveryChannel': { - 'name': 'default', - 's3BucketName': 'mybucket', - 'snsTopicARN': 'my-topic-arn'}} + list_of_operation_names_called, [ + 'HeadBucket', + 'CreateTopic', + 'PutConfigurationRecorder', + 'PutDeliveryChannel', + 'StartConfigurationRecorder', + 'DescribeConfigurationRecorders', + 'DescribeDeliveryChannels' + ] ) self.assertEqual( - self.operations_called[4][0].name, - 'StartConfigurationRecorder') - self.assertEqual( - self.operations_called[4][1], - {'ConfigurationRecorderName': 'default'} + list_of_parameters_called, [ + {'Bucket': 'mybucket'}, # S3 HeadBucket + {'Name': 'mytopic'}, # SNS CreateTopic + {'ConfigurationRecorder': { # PutConfigurationRecorder + 'name': 'default', 'roleARN': 'myrole'}}, + {'DeliveryChannel': { # PutDeliveryChannel + 'name': 'default', + 's3BucketName': 'mybucket', + 'snsTopicARN': 'my-topic-arn'}}, + # StartConfigurationRecorder + {'ConfigurationRecorderName': 'default'}, + {}, # DescribeConfigurationRecorders + {} # DescribeDeliveryChannels + ] ) - self.assertEqual( - self.operations_called[5][0].name, - 'DescribeConfigurationRecorders') - self.assertEqual(self.operations_called[5][1], {}) - self.assertEqual( - self.operations_called[6][0].name, - 'DescribeDeliveryChannels') - self.assertEqual(self.operations_called[6][1], {}) def test_subscribe_when_bucket_exists_and_sns_topic_arn_provided(self): self.parsed_responses.pop(1) @@ -83,64 +78,62 @@ def test_subscribe_when_bucket_exists_and_sns_topic_arn_provided(self): self.run_cmd(self.prefix) self.assertEqual(len(self.operations_called), 6) - # S3 operations - self.assertEqual(self.operations_called[0][0].name, 'HeadBucket') - self.assertEqual(self.operations_called[0][1], {'Bucket': 'mybucket'}) + list_of_operation_names_called = [] + list_of_parameters_called = [] + for operation_called in self.operations_called: + list_of_operation_names_called.append(operation_called[0].name) + list_of_parameters_called.append(operation_called[1]) - # Config operations - self.assertEqual( - self.operations_called[1][0].name, - 'PutConfigurationRecorder') self.assertEqual( - self.operations_called[1][1], - {'ConfigurationRecorder': {'name': 'default', 'roleARN': 'myrole'}} + list_of_operation_names_called, [ + 'HeadBucket', + 'PutConfigurationRecorder', + 'PutDeliveryChannel', + 'StartConfigurationRecorder', + 'DescribeConfigurationRecorders', + 'DescribeDeliveryChannels' + ] ) self.assertEqual( - self.operations_called[2][0].name, - 'PutDeliveryChannel') - self.assertEqual( - self.operations_called[2][1], - {'DeliveryChannel': { - 'name': 'default', - 's3BucketName': 'mybucket', - 'snsTopicARN': 'arn:mytopic'}} + list_of_parameters_called, [ + {'Bucket': 'mybucket'}, # S3 HeadBucket + {'ConfigurationRecorder': { # PutConfigurationRecorder + 'name': 'default', 'roleARN': 'myrole'}}, + {'DeliveryChannel': { # PutDeliveryChannel + 'name': 'default', + 's3BucketName': 'mybucket', + 'snsTopicARN': 'arn:mytopic'}}, + # StartConfigurationRecorder + {'ConfigurationRecorderName': 'default'}, + {}, # DescribeConfigurationRecorders + {} # DescribeDeliveryChannels + ] ) - self.assertEqual( - self.operations_called[3][0].name, - 'StartConfigurationRecorder') - self.assertEqual( - self.operations_called[3][1], - {'ConfigurationRecorderName': 'default'} - ) - self.assertEqual( - self.operations_called[4][0].name, - 'DescribeConfigurationRecorders') - self.assertEqual(self.operations_called[4][1], {}) - self.assertEqual( - self.operations_called[5][0].name, - 'DescribeDeliveryChannels') - self.assertEqual(self.operations_called[5][1], {}) def test_subscribe_when_bucket_needs_to_be_created(self): - # Make the HeadObject request fail now and should try to create a new - # bucket. - self.parsed_responses = None - self.http_response.status_code = 404 - self.parsed_response = {'Error': {'Code': 404, 'Message': ''}} + with mock.patch('botocore.endpoint.Session.send') as \ + http_session_send_patch: + # Mock for HeadBucket request + head_bucket_response = mock.Mock() + head_bucket_response.status_code = 404 + head_bucket_response.content = b'' + head_bucket_response.headers = {} - self.prefix += ' --s3-bucket mybucket --sns-topic arn:mytopic' - self.prefix += ' --iam-role myrole' - self.run_cmd(self.prefix, expected_rc=255) - # This will fail because there is no current way to specify - # a change in status code in BaseAWSCommandParamsTest - # As of now only one status code applies to all parsed responses. - # Therefore the CreateBucket will be the one that receives the 404. - # But it does not matter because we are just checking that the bucket - # is attempted to be made if we determine the bucket does not exist - self.assertEqual(len(self.operations_called), 2) + # Mock for CreateBucket request + create_bucket_response = mock.Mock() + create_bucket_response.status_code = 200 + create_bucket_response.content = b'' + create_bucket_response.headers = {} - self.assertEqual(self.operations_called[0][0].name, 'HeadBucket') - self.assertEqual(self.operations_called[0][1], {'Bucket': 'mybucket'}) - self.assertEqual(self.operations_called[1][0].name, 'CreateBucket') - self.assertEqual( - self.operations_called[1][1]['Bucket'], 'mybucket') + http_session_send_patch.side_effect = [ + head_bucket_response, create_bucket_response + ] + + s3_client = self.driver.session.create_client('s3') + bucket_helper = S3BucketHelper(s3_client) + bucket_helper.prepare_bucket('mybucket') + send_call_list = http_session_send_patch.call_args_list + self.assertEqual(send_call_list[0][0][0].method, 'HEAD') + # Since the HeadObject fails with 404, the CreateBucket which is + # is a PUT request should be made. + self.assertEqual(send_call_list[1][0][0].method, 'PUT') From df7acf05ea7af65f1bf0a92d553b2af4736750af Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 28 Oct 2015 14:50:33 -0700 Subject: [PATCH 4/7] Update CHANGELOG --- CHANGELOG.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c3b316693968..a030098203dc 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,9 @@ Next Release (TBD) respectively. The old arguments are still supported for backwards compatibility, but are no longer documented. (`issue 1599 `__) +* bugfix:``aws configservice subscribe``: Fix an issue when creating a + new S3 bucket + (`issue 1593 `__) 1.9.1 From 88f82741ee6a2e5f87ab013ef856319260b01890 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 28 Oct 2015 17:02:18 -0700 Subject: [PATCH 5/7] Fix one potential leaky patch --- awscli/testutils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/awscli/testutils.py b/awscli/testutils.py index 13d684600ee1..3497e8c5f5f1 100644 --- a/awscli/testutils.py +++ b/awscli/testutils.py @@ -301,6 +301,7 @@ def tearDown(self): self.environ_patch.stop() if self.make_request_is_patched: self.make_request_patch.stop() + self.make_request_is_patched = False def before_call(self, params, **kwargs): self._store_params(params) @@ -310,6 +311,9 @@ def _store_params(self, params): self.last_params = params['body'] def patch_make_request(self): + if self.make_request_is_patched: + self.make_request_patch.stop() + self.make_request_is_patched = False make_request_patch = self.make_request_patch.start() if self.parsed_responses is not None: make_request_patch.side_effect = lambda *args, **kwargs: \ From 7bdc7ff8b838cb53d6e0e26f1d647ec6c7990bdd Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 28 Oct 2015 17:26:02 -0700 Subject: [PATCH 6/7] Fix another leaky patch --- tests/unit/output/test_text_output.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/unit/output/test_text_output.py b/tests/unit/output/test_text_output.py index 267e6778edba..d19a1f173098 100644 --- a/tests/unit/output/test_text_output.py +++ b/tests/unit/output/test_text_output.py @@ -55,13 +55,10 @@ def setUp(self): 'Groups': [] } - def patch_make_request(self): - make_request_patch = self.make_request_patch.start() - make_request_patch.side_effect = [ - (self.http_response, self.first_parsed_response), - (self.http_response, self.second_parsed_response), + self.parsed_responses = [ + self.first_parsed_response, + self.second_parsed_response ] - self.make_request_is_patched = True def test_text_response(self): output = self.run_cmd('iam list-users --output text', expected_rc=0)[0] From 63ce1e6d65133f2bb33817ada756cac74fed1ce2 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 28 Oct 2015 17:30:25 -0700 Subject: [PATCH 7/7] Add a comment about why patches are bad --- awscli/testutils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/awscli/testutils.py b/awscli/testutils.py index 3497e8c5f5f1..bcf4c1dcb8b9 100644 --- a/awscli/testutils.py +++ b/awscli/testutils.py @@ -311,6 +311,10 @@ def _store_params(self, params): self.last_params = params['body'] def patch_make_request(self): + # If you do not stop a previously started patch, + # it can never be stopped if you call start() again on the same + # patch again... + # So stop the current patch before calling start() on it again. if self.make_request_is_patched: self.make_request_patch.stop() self.make_request_is_patched = False