fix(s3-deployment): passing any system metadata causes lambda to fail…

… on "Unknown options:" when invoking aws cli. (#6086) * fix(aws-s3-deployment): fix passing system metadata to deployment * fix(s3-deployment): fix custom metadata, tests for checking individual elements in aws commands * fix(s3-metadata): test custom types, update expected cloudformation templates * test(s3-deployment): clarify object metadata and user metadata tests * test(s3-deployment): missing semicolons * test(s3-deployment): consistent test for Expires.after * Apply suggestions from code review fix(s3-deployment): remove unnecessary fstring Co-Authored-By: Eli Polonsky <[email protected]> * fix(s3-deployment): rerun integration tests Co-authored-by: Elad Ben-Israel <[email protected]> Co-authored-by: Eli Polonsky <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
aws · Feb 6, 2020 · b30add8 · b30add8
1 parent 0d0794e
commit b30add8
Show file tree

Hide file tree

Showing 8 changed files with 210 additions and 55 deletions.
diff --git a/packages/@aws-cdk/aws-s3-deployment/README.md b/packages/@aws-cdk/aws-s3-deployment/README.md
@@ -96,7 +96,7 @@ new s3deploy.BucketDeployment(this, 'DeployWebsite', {
   sources: [s3deploy.Source.asset('./website-dist')],
   destinationBucket: websiteBucket,
   destinationKeyPrefix: 'web/static', // optional prefix in destination bucket
-  userMetadata: { "A": "1", "b": "2" }, // user-defined metadata
+  metadata: { A: "1", b: "2" }, // user-defined metadata
 
   // system-defined metadata
   contentType: "text/html",

diff --git a/packages/@aws-cdk/aws-s3-deployment/lambda/src/index.py b/packages/@aws-cdk/aws-s3-deployment/lambda/src/index.py
@@ -163,8 +163,9 @@ def create_metadata_args(raw_user_metadata, raw_system_metadata):
     system_metadata = { format_system_metadata_key(k): v for k, v in raw_system_metadata.items() }
     user_metadata = { format_user_metadata_key(k): v for k, v in raw_user_metadata.items() }
 
-    system_args = [f"--{k} '{v}'" for k, v in system_metadata.items()]
-    user_args = ["--metadata", f"'{json.dumps(user_metadata)}'"] if len(user_metadata) > 0 else []
+    flatten = lambda l: [item for sublist in l for item in sublist]
+    system_args = flatten([[f"--{k}", v] for k, v in system_metadata.items()])
+    user_args = ["--metadata", json.dumps(user_metadata, separators=(',', ':'))] if len(user_metadata) > 0 else []
 
     return system_args + user_args + ["--metadata-directive", "REPLACE"]
 

diff --git a/packages/@aws-cdk/aws-s3-deployment/lambda/test/aws b/packages/@aws-cdk/aws-s3-deployment/lambda/test/aws
@@ -24,4 +24,4 @@ if sys.argv[2] == "sync":
     sys.argv[4] = "contents.zip"
 
 with open("./aws.out", "a") as myfile:
-    myfile.write(" ".join(sys.argv[1:]) + "\n")
+    myfile.write(json.dumps(sys.argv[1:]) + "\n")
diff --git a/packages/@aws-cdk/aws-s3-deployment/lambda/test/test.py b/packages/@aws-cdk/aws-s3-deployment/lambda/test/test.py
@@ -33,8 +33,8 @@ def test_create_update(self):
         })
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/"]
         )
 
     def test_create_update_multiple_sources(self):
@@ -47,9 +47,9 @@ def test_create_update_multiple_sources(self):
         # Note: these are different files in real-life. For testing purposes, we hijack
         #       the command to output a static filename, archive.zip
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket1>/<source-object-key1> archive.zip",
-            "s3 cp s3://<source-bucket2>/<source-object-key2> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/"
+            ["s3", "cp", "s3://<source-bucket1>/<source-object-key1>", "archive.zip"],
+            ["s3", "cp", "s3://<source-bucket2>/<source-object-key2>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/"]
         )
 
     def test_create_with_backslash_prefix_same_as_no_prefix(self):
@@ -61,8 +61,8 @@ def test_create_with_backslash_prefix_same_as_no_prefix(self):
         })
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/"]
         )
 
 
@@ -75,8 +75,8 @@ def test_create_update_with_dest_key(self):
         })
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/<dest-key-prefix>"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/<dest-key-prefix>"]
         )
 
     def test_create_update_with_metadata(self):
@@ -90,8 +90,8 @@ def test_create_update_with_metadata(self):
         })
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/<dest-key-prefix> --content-type 'text/html' --content-language 'en' --metadata '{\"x-amzn-meta-best\": \"game\"}' --metadata-directive REPLACE"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/<dest-key-prefix>", "--content-type", "text/html", "--content-language", "en", "--metadata", "{\"x-amzn-meta-best\":\"game\"}", "--metadata-directive", "REPLACE"]
         )
 
     def test_delete_no_retain(self):
@@ -102,7 +102,7 @@ def test_delete_no_retain(self):
             "RetainOnDelete": "false"
         }, physical_id="<physicalid>")
 
-        self.assertAwsCommands("s3 rm s3://<dest-bucket-name>/ --recursive")
+        self.assertAwsCommands(["s3", "rm", "s3://<dest-bucket-name>/", "--recursive"])
 
     def test_delete_with_dest_key(self):
         invoke_handler("Delete", {
@@ -113,7 +113,7 @@ def test_delete_with_dest_key(self):
             "RetainOnDelete": "false"
         }, physical_id="<physicalid>")
 
-        self.assertAwsCommands("s3 rm s3://<dest-bucket-name>/<dest-key-prefix> --recursive")
+        self.assertAwsCommands(["s3", "rm", "s3://<dest-bucket-name>/<dest-key-prefix>", "--recursive"])
 
     def test_delete_with_retain_explicit(self):
         invoke_handler("Delete", {
@@ -146,7 +146,7 @@ def test_delete_with_retain_explicitly_false(self):
         }, physical_id="<physicalid>")
 
         self.assertAwsCommands(
-            "s3 rm s3://<dest-bucket-name>/ --recursive"
+            ["s3", "rm", "s3://<dest-bucket-name>/", "--recursive"]
         )
 
     #
@@ -163,8 +163,8 @@ def test_update_same_dest(self):
         }, physical_id="<physical-id>")
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/"]
         )
 
     def test_update_same_dest_cf_invalidate(self):
@@ -244,8 +244,8 @@ def test_update_new_dest_retain(self):
         }, physical_id="<physical-id>")
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/"]
         )
 
     def test_update_new_dest_no_retain(self):
@@ -261,9 +261,9 @@ def test_update_new_dest_no_retain(self):
         }, physical_id="<physical-id>")
 
         self.assertAwsCommands(
-            "s3 rm s3://<old-dest-bucket-name>/<old-dest-prefix> --recursive",
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<new-dest-bucket-name>/"
+            ["s3", "rm", "s3://<old-dest-bucket-name>/<old-dest-prefix>", "--recursive"],
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<new-dest-bucket-name>/"]
         )
 
     def test_update_new_dest_retain_implicit(self):
@@ -277,8 +277,8 @@ def test_update_new_dest_retain_implicit(self):
         }, physical_id="<physical-id>")
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<new-dest-bucket-name>/"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<new-dest-bucket-name>/"]
         )
 
     def test_update_new_dest_prefix_no_retain(self):
@@ -294,9 +294,9 @@ def test_update_new_dest_prefix_no_retain(self):
         }, physical_id="<physical id>")
 
         self.assertAwsCommands(
-            "s3 rm s3://<dest-bucket-name>/ --recursive",
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/<new-dest-prefix>"
+            ["s3", "rm", "s3://<dest-bucket-name>/", "--recursive"],
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/<new-dest-prefix>"]
         )
 
     def test_update_new_dest_prefix_retain_implicit(self):
@@ -310,8 +310,8 @@ def test_update_new_dest_prefix_retain_implicit(self):
         }, physical_id="<physical id>")
 
         self.assertAwsCommands(
-            "s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
-            "s3 sync --delete contents.zip s3://<dest-bucket-name>/<new-dest-prefix>"
+            ["s3", "cp", "s3://<source-bucket>/<source-object-key>", "archive.zip"],
+            ["s3", "sync", "--delete", "contents.zip", "s3://<dest-bucket-name>/<new-dest-prefix>"]
         )
 
     #
@@ -386,7 +386,7 @@ def read_aws_out():
         return []
 
     with open("aws.out") as f:
-        return f.read().splitlines()
+        return [json.loads(l) for l in f.read().splitlines()]
 
 #
 # invokes the handler under test

diff --git a/packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment-cloudfront.expected.json b/packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment-cloudfront.expected.json
@@ -248,7 +248,7 @@
       "Properties": {
         "Code": {
           "S3Bucket": {
-            "Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3Bucket1FAE0333"
+            "Ref": "AssetParametersa9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ffS3Bucket848A1F31"
           },
           "S3Key": {
             "Fn::Join": [
@@ -261,7 +261,7 @@
                       "Fn::Split": [
                         "||",
                         {
-                          "Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3"
+                          "Ref": "AssetParametersa9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ffS3VersionKey983DBE96"
                         }
                       ]
                     }
@@ -274,7 +274,7 @@
                       "Fn::Split": [
                         "||",
                         {
-                          "Ref": "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3"
+                          "Ref": "AssetParametersa9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ffS3VersionKey983DBE96"
                         }
                       ]
                     }
@@ -301,17 +301,17 @@
     }
   },
   "Parameters": {
-    "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3Bucket1FAE0333": {
+    "AssetParametersa9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ffS3Bucket848A1F31": {
       "Type": "String",
-      "Description": "S3 bucket for asset \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
+      "Description": "S3 bucket for asset \"a9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ff\""
     },
-    "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412S3VersionKeyE18B22C3": {
+    "AssetParametersa9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ffS3VersionKey983DBE96": {
       "Type": "String",
-      "Description": "S3 key for asset version \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
+      "Description": "S3 key for asset version \"a9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ff\""
     },
-    "AssetParameters53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412ArtifactHashA605283F": {
+    "AssetParametersa9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ffArtifactHash08605F5E": {
       "Type": "String",
-      "Description": "Artifact hash for asset \"53a43f436014c307dccbd4ca1172459431a2a8dee8a2c318ee65991c2a8d4412\""
+      "Description": "Artifact hash for asset \"a9125fa9a40550c71cde90bd478cc23091e868067a12380c1df0827d013ad2ff\""
     },
     "AssetParametersfc4481abf279255619ff7418faa5d24456fef3432ea0da59c95542578ff0222eS3Bucket9CD8B20A": {
       "Type": "String",