From bd1936fec5f37bc265531879dc950cdccffd3343 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Tue, 17 Mar 2015 11:19:48 -0700 Subject: [PATCH 1/3] Fix issue with configservice subscribe command When checking to see if a bucket exists, we assumed that an exception meant that the bucket did not exist when using HeadBucket. In reality, the error could be a 403. Also expanded the output to inform users if a new bucket/topic is being used or an existing one. --- .../customizations/configservice/subscribe.py | 20 +++++- .../configservice/test_subscribe.py | 66 ++++++++++++++++++- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/awscli/customizations/configservice/subscribe.py b/awscli/customizations/configservice/subscribe.py index 9075ade6e49b..2ee74e809269 100644 --- a/awscli/customizations/configservice/subscribe.py +++ b/awscli/customizations/configservice/subscribe.py @@ -13,6 +13,8 @@ import json import sys +from botocore.exceptions import ClientError + from awscli.customizations.commands import BasicCommand from awscli.customizations.s3.utils import find_bucket_key @@ -138,16 +140,25 @@ def prepare_bucket(self, s3_path): bucket_exists = self._check_bucket_exists(bucket) if not bucket_exists: self._create_bucket(bucket) + sys.stdout.write('Using new S3 bucket: %s\n' % bucket) + else: + sys.stdout.write('Using existing S3 bucket: %s\n' % bucket) return bucket, key def _check_bucket_exists(self, bucket): bucket_exists = True + self._s3_client.meta.events.unregister( + 'after-call', + unique_id='awscli-error-handler') try: # See if the bucket exists by running a head bucket self._s3_client.head_bucket(Bucket=bucket) - except Exception: - # If a client error is thrown than the bucket does not exist. - bucket_exists = False + except ClientError as e: + # If a client error is thrown. Check that it was a 404 error. + # If it was a 404 error, than the bucket does not exist. + error_code = int(e.response['Error']['Code']) + if error_code == 404: + bucket_exists = False return bucket_exists def _create_bucket(self, bucket): @@ -171,6 +182,9 @@ def prepare_topic(self, sns_topic): if not self._check_is_arn(sns_topic): response = self._sns_client.create_topic(Name=sns_topic) sns_topic_arn = response['TopicArn'] + sys.stdout.write('Using new SNS topic: %s\n' % sns_topic_arn) + else: + sys.stdout.write('Using existing SNS topic: %s\n' % sns_topic_arn) return sns_topic_arn def _check_is_arn(self, sns_topic): diff --git a/tests/unit/customizations/configservice/test_subscribe.py b/tests/unit/customizations/configservice/test_subscribe.py index 98fab14fc893..da7d4bf4e62b 100644 --- a/tests/unit/customizations/configservice/test_subscribe.py +++ b/tests/unit/customizations/configservice/test_subscribe.py @@ -11,8 +11,10 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import mock +from botocore.exceptions import ClientError from awscli.testutils import unittest +from awscli.compat import StringIO from awscli.customizations.configservice.subscribe import SubscribeCommand, \ S3BucketHelper, SNSTopicHelper @@ -21,6 +23,16 @@ class TestS3BucketHelper(unittest.TestCase): def setUp(self): self.s3_client = mock.Mock() self.helper = S3BucketHelper(self.s3_client) + self.error_response = { + 'Error': { + 'Code': '404', + 'Message': 'Not Found' + } + } + self.bucket_no_exists_error = ClientError( + self.error_response, + 'HeadBucket' + ) def test_correct_prefix_returned(self): name = 'MyBucket/MyPrefix' @@ -40,7 +52,7 @@ def test_bucket_exists(self): def test_bucket_no_exist(self): name = 'MyBucket/MyPrefix' - self.s3_client.head_bucket.side_effect = Exception() + self.s3_client.head_bucket.side_effect = self.bucket_no_exists_error self.s3_client._endpoint.region_name = 'us-east-1' bucket, prefix = self.helper.prepare_bucket(name) # Ensure that the create bucket was called with the proper args. @@ -53,7 +65,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 = Exception() + self.s3_client.head_bucket.side_effect = self.bucket_no_exists_error self.s3_client._endpoint.region_name = 'us-west-2' bucket, prefix = self.helper.prepare_bucket(name) # Ensure that the create bucket was called with the proper args. @@ -65,6 +77,39 @@ def test_bucket_no_exist_with_location_constraint(self): self.assertEqual(bucket, 'MyBucket') self.assertEqual(prefix, 'MyPrefix') + def test_bucket_client_exception_non_404(self): + name = 'MyBucket/MyPrefix' + self.error_response['Error']['Code'] = '403' + self.error_response['Error']['Message'] = 'Forbidden' + forbidden_error = ClientError(self.error_response, 'HeadBucket') + self.s3_client.head_bucket.side_effect = forbidden_error + self.s3_client._endpoint.region_name = 'us-east-1' + bucket, prefix = self.helper.prepare_bucket(name) + # A new bucket should not have been created because a 404 error + # was not thrown + self.assertFalse(self.s3_client.create_bucket.called) + # Ensure the returned bucket and key are as expected + self.assertEqual(bucket, 'MyBucket') + self.assertEqual(prefix, 'MyPrefix') + + def test_output_use_existing_bucket(self): + name = 'MyBucket/MyPrefix' + with mock.patch('sys.stdout', StringIO()) as mock_stdout: + self.helper.prepare_bucket(name) + self.assertIn( + 'Using existing S3 bucket: MyBucket', + mock_stdout.getvalue()) + + def test_output_create_bucket(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' + with mock.patch('sys.stdout', StringIO()) as mock_stdout: + self.helper.prepare_bucket(name) + self.assertIn( + 'Using new S3 bucket: MyBucket', + mock_stdout.getvalue()) + class TestSNSTopicHelper(unittest.TestCase): def setUp(self): @@ -86,6 +131,23 @@ def test_sns_topic_by_arn(self): self.assertFalse(self.sns_client.create_topic.called) self.assertEqual(sns_arn, name) + def test_output_existing_topic(self): + name = 'mysnstopic' + self.sns_client.create_topic.return_value = {'TopicArn': 'myARN'} + with mock.patch('sys.stdout', StringIO()) as mock_stdout: + self.helper.prepare_topic(name) + self.assertIn( + 'Using new SNS topic: myARN', + mock_stdout.getvalue()) + + def test_output_new_topic(self): + name = 'arn:aws:sns:us-east-1:934212987125:config' + with mock.patch('sys.stdout', StringIO()) as mock_stdout: + self.helper.prepare_topic(name) + self.assertIn( + 'Using existing SNS topic: %s' % name, + mock_stdout.getvalue()) + class TestSubscribeCommand(unittest.TestCase): def setUp(self): From 853513d9fcf9126994a86bee83fa85c5dbe16d50 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 18 Mar 2015 12:10:57 -0700 Subject: [PATCH 2/3] Consolidate bucket_exists logic into utils file Added a helper function that can be used to determine if a s3 bucket exists or not. --- awscli/customizations/cloudtrail.py | 10 ++---- .../customizations/configservice/subscribe.py | 15 ++------ awscli/customizations/utils.py | 18 ++++++++++ tests/unit/customizations/test_utils.py | 34 +++++++++++++++++++ 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/awscli/customizations/cloudtrail.py b/awscli/customizations/cloudtrail.py index 17d511a6ba31..a9a0e2707d0b 100644 --- a/awscli/customizations/cloudtrail.py +++ b/awscli/customizations/cloudtrail.py @@ -15,6 +15,7 @@ import sys from awscli.customizations.commands import BasicCommand +from awscli.customizations.utils import s3_bucket_exists from botocore.exceptions import ClientError @@ -232,13 +233,8 @@ def setup_new_bucket(self, bucket, prefix, custom_policy=None): policy = policy.replace('', prefix or '') LOG.debug('Bucket policy:\n{0}'.format(policy)) - - try: - self.s3.head_bucket(Bucket=bucket) - except ClientError: - # The bucket does not exists. This is what we want. - pass - else: + bucket_exists = s3_bucket_exists(self.s3, bucket) + if bucket_exists: raise Exception('Bucket {bucket} already exists.'.format( bucket=bucket)) diff --git a/awscli/customizations/configservice/subscribe.py b/awscli/customizations/configservice/subscribe.py index 2ee74e809269..c19b95e18011 100644 --- a/awscli/customizations/configservice/subscribe.py +++ b/awscli/customizations/configservice/subscribe.py @@ -13,9 +13,8 @@ import json import sys -from botocore.exceptions import ClientError - from awscli.customizations.commands import BasicCommand +from awscli.customizations.utils import s3_bucket_exists from awscli.customizations.s3.utils import find_bucket_key @@ -146,20 +145,10 @@ def prepare_bucket(self, s3_path): return bucket, key def _check_bucket_exists(self, bucket): - bucket_exists = True self._s3_client.meta.events.unregister( 'after-call', unique_id='awscli-error-handler') - try: - # See if the bucket exists by running a head bucket - self._s3_client.head_bucket(Bucket=bucket) - except ClientError as e: - # If a client error is thrown. Check that it was a 404 error. - # If it was a 404 error, than the bucket does not exist. - error_code = int(e.response['Error']['Code']) - if error_code == 404: - bucket_exists = False - return bucket_exists + return s3_bucket_exists(self._s3_client, bucket) def _create_bucket(self, bucket): region_name = self._s3_client._endpoint.region_name diff --git a/awscli/customizations/utils.py b/awscli/customizations/utils.py index c789cb0be6ea..746a72b0780e 100644 --- a/awscli/customizations/utils.py +++ b/awscli/customizations/utils.py @@ -14,6 +14,10 @@ Utility functions to make it easier to work with customizations. """ + +from botocore.exceptions import ClientError + + def rename_argument(argument_table, existing_name, new_name): current = argument_table[existing_name] argument_table[new_name] = current @@ -60,3 +64,17 @@ def _get_group_for_key(key, groups): for group in groups: if key in group: return group + + +def s3_bucket_exists(s3_client, bucket_name): + bucket_exists = True + try: + # See if the bucket exists by running a head bucket + s3_client.head_bucket(Bucket=bucket_name) + except ClientError as e: + # If a client error is thrown. Check that it was a 404 error. + # If it was a 404 error, than the bucket does not exist. + error_code = int(e.response['Error']['Code']) + if error_code == 404: + bucket_exists = False + return bucket_exists diff --git a/tests/unit/customizations/test_utils.py b/tests/unit/customizations/test_utils.py index 1fccc9e9e150..33be0f1467ab 100644 --- a/tests/unit/customizations/test_utils.py +++ b/tests/unit/customizations/test_utils.py @@ -14,6 +14,7 @@ from awscli.testutils import BaseAWSHelpOutputTest import mock +from botocore.exceptions import ClientError from awscli.customizations import utils @@ -63,3 +64,36 @@ def test_multiple_groups(self): parsed = FakeParsedArgs(foo='one', bar=None, qux='three') with self.assertRaises(ValueError): utils.validate_mutually_exclusive(parsed, *groups) + + +class TestS3BucketExists(unittest.TestCase): + def setUp(self): + self.s3_client = mock.Mock() + self.bucket_name = 'mybucket' + self.error_response = { + 'Error': { + 'Code': '404', + 'Message': 'Not Found' + } + } + self.bucket_no_exists_error = ClientError( + self.error_response, + 'HeadBucket' + ) + + def test_bucket_exists(self): + self.assertTrue( + utils.s3_bucket_exists(self.s3_client, self.bucket_name)) + + def test_bucket_not_exists(self): + self.s3_client.head_bucket.side_effect = self.bucket_no_exists_error + self.assertFalse( + utils.s3_bucket_exists(self.s3_client, self.bucket_name)) + + def test_bucket_exists_with_non_404(self): + self.error_response['Error']['Code'] = '403' + self.error_response['Error']['Message'] = 'Forbidden' + forbidden_error = ClientError(self.error_response, 'HeadBucket') + self.s3_client.head_bucket.side_effect = forbidden_error + self.assertTrue( + utils.s3_bucket_exists(self.s3_client, self.bucket_name)) From b7ac21defd97c95c1fb31e08d6793c4c080fb975 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 18 Mar 2015 12:25:39 -0700 Subject: [PATCH 3/3] Update Changelog for a couple of updates --- CHANGELOG.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d2283c8f979b..49a98c5b454a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,18 @@ CHANGELOG ========= +Next Release (TBD) +================== + +* bugfix:``aws configservice subscribe``: Fix issue where users could not + subscribe to a s3 bucket that they had no HeadBucket permissions to. + (`issue 1223 `__) +* bugfix:``aws cloudtrail create-subscription``: Fix issue where command would + try to fetch the contents at a url using the contents of the custom policy + as the url. + (`issue 1216 `__) + + 1.7.14 ======