Merge pull request #1223 from kyleknap/subscribe-fix

Fix issue with configservice subscribe command

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 <https://github.com/aws/aws-cli/pull/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 <https://github.com/aws/aws-cli/pull/1216/files>`__)
+
+
 1.7.14
 ======
 

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>', 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))
 

awscli/customizations/configservice/subscribe.py

@@ -14,6 +14,7 @@
 import sys
 
 from awscli.customizations.commands import BasicCommand
+from awscli.customizations.utils import s3_bucket_exists
 from awscli.customizations.s3.utils import find_bucket_key
 
 
@@ -138,17 +139,16 @@ 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
-        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
-        return bucket_exists
+        self._s3_client.meta.events.unregister(
+            'after-call',
+            unique_id='awscli-error-handler')
+        return s3_bucket_exists(self._s3_client, bucket)
 
     def _create_bucket(self, bucket):
         region_name = self._s3_client._endpoint.region_name
@@ -171,6 +171,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):

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

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

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))