diff --git a/.changes/next-release/bugfix-cloudformationdeploy-48475.json b/.changes/next-release/bugfix-cloudformationdeploy-48475.json new file mode 100644 index 000000000000..28543f223a63 --- /dev/null +++ b/.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" +} diff --git a/.changes/next-release/bugfix-cloudformationpackage-47320.json b/.changes/next-release/bugfix-cloudformationpackage-47320.json new file mode 100644 index 000000000000..1715aae11c80 --- /dev/null +++ b/.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``" +} diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 428708b8d3ab..1864b1ce8e79 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/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 = { diff --git a/awscli/customizations/cloudformation/deploy.py b/awscli/customizations/cloudformation/deploy.py index 3ed556353cad..8f5d02ccdd44 100644 --- a/awscli/customizations/cloudformation/deploy.py +++ b/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 } diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index ade754207de0..71de565161bc 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/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) diff --git a/awscli/customizations/cloudformation/s3uploader.py b/awscli/customizations/cloudformation/s3uploader.py index 653276b754a3..c5bea4560330 100644 --- a/awscli/customizations/cloudformation/s3uploader.py +++ b/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 diff --git a/tests/unit/customizations/cloudformation/test_artifact_exporter.py b/tests/unit/customizations/cloudformation/test_artifact_exporter.py index 578fdb605739..1ca5efa49a98 100644 --- a/tests/unit/customizations/cloudformation/test_artifact_exporter.py +++ b/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) diff --git a/tests/unit/customizations/cloudformation/test_deploy.py b/tests/unit/customizations/cloudformation/test_deploy.py index c000f5bd8a89..ddde08e1b379 100644 --- a/tests/unit/customizations/cloudformation/test_deploy.py +++ b/tests/unit/customizations/cloudformation/test_deploy.py @@ -234,13 +234,18 @@ 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 = [ @@ -248,9 +253,14 @@ def test_merge_parameters_success(self): {"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( diff --git a/tests/unit/customizations/cloudformation/test_s3uploader.py b/tests/unit/customizations/cloudformation/test_s3uploader.py index 646f1617bf77..7c58aa46900a 100644 --- a/tests/unit/customizations/cloudformation/test_s3uploader.py +++ b/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))