Merge branch 'cfn-package-fixes' into develop

* cfn-package-fixes:
  Remove blank lines
  Add changelog entries
  Fixing two bugs with CloudFormation package & deploy commands

.changes/next-release/bugfix-cloudformationdeploy-48475.json

@@ -0,0 +1,5 @@
+{
+  "description": "``deploy`` command must not override parameters with default values",
+  "category": "``cloudformation deploy``",
+  "type": "bugfix"
+}

.changes/next-release/bugfix-cloudformationpackage-47320.json

@@ -0,0 +1,5 @@
+{
+  "type": "bugfix",
+  "description": "``package`` command must use Path-style S3 URL when packaging AWS",
+  "category": "``cloudformation package``"
+}

awscli/customizations/cloudformation/artifact_exporter.py

@@ -344,7 +344,10 @@ def do_export(self, resource_id, resource_dict, parent_dir):
             url = self.uploader.upload_with_dedup(
                     temporary_file.name, "template")
 
-            resource_dict[self.PROPERTY_NAME] = url
+            # TemplateUrl property requires S3 URL to be in path-style format
+            parts = parse_s3_url(url, version_property="Version")
+            resource_dict[self.PROPERTY_NAME] = self.uploader.to_path_style_s3_url(
+                    parts["Key"], parts.get("Version", None))
 
 
 EXPORT_DICT = {

awscli/customizations/cloudformation/deploy.py

@@ -197,6 +197,11 @@ def merge_parameters(self, template_dict, parameter_overrides):
 
         for key, value in template_dict["Parameters"].items():
 
+            if key not in parameter_overrides and "Default" in value:
+                # Parameters that have default value and not overridden, should not be
+                # passed to CloudFormation
+                continue
+
             obj = {
                 "ParameterKey": key
             }

awscli/customizations/cloudformation/package.py

@@ -117,6 +117,7 @@ def _run_main(self, parsed_args, parsed_globals):
 
         self.s3_uploader = S3Uploader(s3_client,
                                       bucket,
+                                      parsed_globals.region,
                                       parsed_args.s3_prefix,
                                       parsed_args.kms_key_id,
                                       parsed_args.force_upload)

awscli/customizations/cloudformation/s3uploader.py

@@ -35,6 +35,7 @@ class S3Uploader(object):
 
     def __init__(self, s3_client,
                  bucket_name,
+                 region,
                  prefix=None,
                  kms_key_id=None,
                  force_upload=False,
@@ -44,6 +45,7 @@ def __init__(self, s3_client,
         self.kms_key_id = kms_key_id or None
         self.force_upload = force_upload
         self.s3 = s3_client
+        self.region = region
 
         self.transfer_manager = transfer_manager
         if not transfer_manager:
@@ -160,6 +162,21 @@ def file_checksum(self, file_name):
 
             return md5.hexdigest()
 
+    def to_path_style_s3_url(self, key, version=None):
+        """
+            This link describes the format of Path Style URLs
+            http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html#access-bucket-intro
+        """
+        base = "https://s3.amazonaws.com"
+        if self.region and self.region != "us-east-1":
+            base = "https://s3-{0}.amazonaws.com".format(self.region)
+
+        result = "{0}/{1}/{2}".format(base, self.bucket_name, key)
+        if version:
+            result = "{0}?versionId={1}".format(result, version)
+
+        return result
+
 
 class ProgressPercentage(BaseSubscriber):
     # This class was copied directly from S3Transfer docs

tests/unit/customizations/cloudformation/test_artifact_exporter.py

@@ -493,12 +493,14 @@ def test_export_cloudformation_stack(self, TemplateMock):
         property_name = stack_resource.PROPERTY_NAME
         exported_template_dict = {"foo": "bar"}
         result_s3_url = "s3://hello/world"
+        result_path_style_s3_url = "http://s3.amazonws.com/hello/world"
 
         template_instance_mock = Mock()
         TemplateMock.return_value = template_instance_mock
         template_instance_mock.export.return_value = exported_template_dict
 
         self.s3_uploader_mock.upload_with_dedup.return_value = result_s3_url
+        self.s3_uploader_mock.to_path_style_s3_url.return_value = result_path_style_s3_url
 
         with tempfile.NamedTemporaryFile() as handle:
             template_path = handle.name
@@ -507,11 +509,12 @@ def test_export_cloudformation_stack(self, TemplateMock):
 
             stack_resource.export(resource_id, resource_dict, parent_dir)
 
-            self.assertEquals(resource_dict[property_name], result_s3_url)
+            self.assertEquals(resource_dict[property_name], result_path_style_s3_url)
 
             TemplateMock.assert_called_once_with(template_path, parent_dir, self.s3_uploader_mock)
             template_instance_mock.export.assert_called_once_with()
             self.s3_uploader_mock.upload_with_dedup.assert_called_once_with(mock.ANY, "template")
+            self.s3_uploader_mock.to_path_style_s3_url.assert_called_once_with("world", None)
 
     def test_export_cloudformation_stack_no_upload_path_is_s3url(self):
         stack_resource = CloudFormationStackResource(self.s3_uploader_mock)

tests/unit/customizations/cloudformation/test_deploy.py

@@ -234,23 +234,33 @@ def test_merge_parameters_success(self):
         """
         template = {
             "Parameters": {
-                "Key1": {"Type": "String"}, "Key2": {"Type": "String"},
-                "Key3": "Something", "Key4": {"Type": "Number"},
+                "Key1": {"Type": "String"},
+                "Key2": {"Type": "String"},
+                "Key3": "Something",
+                "Key4": {"Type": "Number"},
+                "KeyWithDefaultValue": {"Type": "String", "Default": "something"},
+                "KeyWithDefaultValueButOverridden": {"Type": "String", "Default": "something"}
             }
         }
         overrides = {
             "Key1": "Value1",
-            "Key3": "Value3"
+            "Key3": "Value3",
+            "KeyWithDefaultValueButOverridden": "Value4"
         }
 
         expected_result = [
             # Overriden values
             {"ParameterKey": "Key1", "ParameterValue": "Value1"},
             {"ParameterKey": "Key3", "ParameterValue": "Value3"},
 
+            # Parameter contains default value, but overridden with new value
+            {"ParameterKey": "KeyWithDefaultValueButOverridden", "ParameterValue": "Value4"},
+
             # non-overriden values
             {"ParameterKey": "Key2", "UsePreviousValue": True},
             {"ParameterKey": "Key4", "UsePreviousValue": True},
+
+            # Parameter with default value is NOT sent to CloudFormation
         ]
 
         self.assertItemsEqual(

tests/unit/customizations/cloudformation/test_s3uploader.py

@@ -40,9 +40,10 @@ def setUp(self):
         self.transfer_manager_mock.upload = Mock()
         self.bucket_name = "bucketname"
         self.prefix = None
+        self.region = "us-east-1"
 
         self.s3uploader = S3Uploader(
-            self.s3client, self.bucket_name, self.prefix, None, False,
+            self.s3client, self.bucket_name, self.region, self.prefix, None, False,
             self.transfer_manager_mock)
 
     @patch("awscli.customizations.cloudformation.s3uploader.ProgressPercentage")
@@ -52,7 +53,7 @@ def test_upload_successful(self, progress_percentage_mock):
         prefix = "SomePrefix"
         remote_path_with_prefix = "{0}/{1}".format(prefix, remote_path)
         s3uploader = S3Uploader(
-            self.s3client, self.bucket_name, prefix, None, False,
+            self.s3client, self.bucket_name, self.region, prefix, None, False,
             self.transfer_manager_mock)
         expected_upload_url = "s3://{0}/{1}/{2}".format(
             self.bucket_name, prefix, remote_path)
@@ -95,7 +96,7 @@ def test_upload_force_upload(self, progress_percentage_mock):
 
         # Set ForceUpload = True
         self.s3uploader = S3Uploader(
-            self.s3client, self.bucket_name, self.prefix,
+            self.s3client, self.bucket_name, self.region, self.prefix,
             None, True, self.transfer_manager_mock)
 
         # Pretend file already exists
@@ -125,7 +126,7 @@ def test_upload_successful_custom_kms_key(self, progress_percentage_mock):
                                                     remote_path)
         # Set KMS Key Id
         self.s3uploader = S3Uploader(
-            self.s3client, self.bucket_name, self.prefix,
+            self.s3client, self.bucket_name, self.region, self.prefix,
             kms_key_id, False, self.transfer_manager_mock)
 
         # Setup mock to fake that file does not exist
@@ -231,7 +232,7 @@ def test_file_exists(self):
 
         # Let's pretend some other unknown exception happened
         s3mock = Mock()
-        uploader = S3Uploader(s3mock, self.bucket_name)
+        uploader = S3Uploader(s3mock, self.bucket_name, self.region)
         s3mock.head_object = Mock()
         s3mock.head_object.side_effect = RuntimeError()
 
@@ -261,3 +262,43 @@ def test_make_url(self):
         path = "Hello/how/are/you"
         expected = "s3://{0}/{1}".format(self.bucket_name, path)
         self.assertEquals(expected, self.s3uploader.make_url(path))
+
+    def test_to_path_style_s3_url_us_east_1(self):
+        key = "path/to/file"
+        version = "someversion"
+        region = "us-east-1"
+
+        s3uploader = S3Uploader(self.s3client, self.bucket_name, region)
+        result = s3uploader.to_path_style_s3_url(key, version)
+        self.assertEqual(
+                result,
+                "https://s3.amazonaws.com/{0}/{1}?versionId={2}".format(
+                        self.bucket_name, key, version))
+
+        # Without versionId, that query parameter should be omitted
+        s3uploader = S3Uploader(self.s3client, self.bucket_name, region)
+        result = s3uploader.to_path_style_s3_url(key)
+        self.assertEqual(
+                result,
+                "https://s3.amazonaws.com/{0}/{1}".format(
+                        self.bucket_name, key, version))
+
+    def test_to_path_style_s3_url_other_regions(self):
+        key = "path/to/file"
+        version = "someversion"
+        region = "us-west-2"
+
+        s3uploader = S3Uploader(self.s3client, self.bucket_name, region)
+        result = s3uploader.to_path_style_s3_url(key, version)
+        self.assertEqual(
+                result,
+                "https://s3-{0}.amazonaws.com/{1}/{2}?versionId={3}".format(
+                        region, self.bucket_name, key, version))
+
+        # Without versionId, that query parameter should be omitted
+        s3uploader = S3Uploader(self.s3client, self.bucket_name, region)
+        result = s3uploader.to_path_style_s3_url(key)
+        self.assertEqual(
+                result,
+                "https://s3-{0}.amazonaws.com/{1}/{2}".format(
+                        region, self.bucket_name, key))