Skip to content

Commit bca99c6

Browse files
author
Elad Ben-Israel
authored
fix(aws-s3-deployment): avoid deletion during update using physical ids (#1006)
When a BucketDeployment resource is created, a unique physical ID is generated and returned via `ResourcePhysicalId`. The same ID will then be used in update/delete. This tells CloudFormation not to issue a DELETE operation after an UPDATE, which was the cause for #981. Also, allow destination prefix to be "/", which is the same as not specifying a prefix. Fixes #981
1 parent 3605f9c commit bca99c6

File tree

7 files changed

+183
-36
lines changed

7 files changed

+183
-36
lines changed

‎packages/@aws-cdk/aws-s3-deployment/lambda/src/index.py‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
import json
66
import traceback
77
import logging
8+
from uuid import uuid4
9+
810
from botocore.vendored import requests
911
from zipfile import ZipFile
1012

@@ -17,7 +19,7 @@
1719
def handler(event, context):
1820

1921
def cfn_error(message=None):
20-
logger.info("| cfn_error: %s" % message)
22+
logger.error("| cfn_error: %s" % message)
2123
cfn_send(event, context, CFN_FAILED, reason=message)
2224

2325
try:
@@ -29,41 +31,64 @@ def cfn_error(message=None):
2931
# extract resource properties
3032
props = event['ResourceProperties']
3133
old_props = event.get('OldResourceProperties', {})
34+
physical_id = event.get('PhysicalResourceId', None)
3235

3336
try:
3437
source_bucket_name = props['SourceBucketName']
3538
source_object_key = props['SourceObjectKey']
3639
dest_bucket_name = props['DestinationBucketName']
3740
dest_bucket_prefix = props.get('DestinationBucketKeyPrefix', '')
38-
retain_on_delete = props.get('RetainOnDelete', "false") == "true"
41+
retain_on_delete = props.get('RetainOnDelete', "true") == "true"
3942
except KeyError as e:
40-
cfn_error("missing request resource property %s" % str(e))
43+
cfn_error("missing request resource property %s. props: %s" % (str(e), props))
4144
return
4245

46+
# treat "/" as if no prefix was specified
47+
if dest_bucket_prefix == "/":
48+
dest_bucket_prefix = ""
49+
4350
s3_source_zip = "s3://%s/%s" % (source_bucket_name, source_object_key)
4451
s3_dest = "s3://%s/%s" % (dest_bucket_name, dest_bucket_prefix)
4552

4653
old_s3_dest = "s3://%s/%s" % (old_props.get("DestinationBucketName", ""), old_props.get("DestinationBucketKeyPrefix", ""))
54+
55+
# obviously this is not
56+
if old_s3_dest == "s3:///":
57+
old_s3_dest = None
58+
4759
logger.info("| s3_dest: %s" % s3_dest)
4860
logger.info("| old_s3_dest: %s" % old_s3_dest)
4961

62+
# if we are creating a new resource, allocate a physical id for it
63+
# otherwise, we expect physical id to be relayed by cloudformation
64+
if request_type == "Create":
65+
physical_id = "aws.cdk.s3deployment.%s" % str(uuid4())
66+
else:
67+
if not physical_id:
68+
cfn_error("invalid request: request type is '%s' but 'PhysicalResourceId' is not defined" % request_type)
69+
return
70+
5071
# delete or create/update (only if "retain_on_delete" is false)
5172
if request_type == "Delete" and not retain_on_delete:
5273
aws_command("s3", "rm", s3_dest, "--recursive")
5374

5475
# if we are updating without retention and the destination changed, delete first
5576
if request_type == "Update" and not retain_on_delete and old_s3_dest != s3_dest:
77+
if not old_s3_dest:
78+
logger.warn("cannot delete old resource without old resource properties")
79+
return
80+
5681
aws_command("s3", "rm", old_s3_dest, "--recursive")
5782

5883
if request_type == "Update" or request_type == "Create":
5984
s3_deploy(s3_source_zip, s3_dest)
6085

61-
cfn_send(event, context, CFN_SUCCESS)
86+
cfn_send(event, context, CFN_SUCCESS, physicalResourceId=physical_id)
6287
except KeyError as e:
6388
cfn_error("invalid request. Missing key %s" % str(e))
6489
except Exception as e:
6590
logger.exception(e)
66-
cfn_error()
91+
cfn_error(str(e))
6792

6893
#---------------------------------------------------------------------------------------------------
6994
# populate all files from s3_source_zip to a destination bucket

‎packages/@aws-cdk/aws-s3-deployment/lambda/test/test.py‎

Lines changed: 124 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,14 @@
1212
class TestHandler(unittest.TestCase):
1313
def setUp(self):
1414
logger = logging.getLogger()
15-
logger.addHandler(logging.NullHandler())
1615

1716
# clean up old aws.out file (from previous runs)
1817
try: os.remove("aws.out")
1918
except OSError: pass
2019

2120
def test_invalid_request(self):
2221
resp = invoke_handler("Create", {}, expected_status="FAILED")
23-
self.assertEqual(resp["Reason"], "missing request resource property 'SourceBucketName'")
22+
self.assertEqual(resp["Reason"], "missing request resource property 'SourceBucketName'. props: {}")
2423

2524
def test_create_update(self):
2625
invoke_handler("Create", {
@@ -34,6 +33,20 @@ def test_create_update(self):
3433
"s3 sync --delete contents.zip s3://<dest-bucket-name>/"
3534
)
3635

36+
def test_create_with_backslash_prefix_same_as_no_prefix(self):
37+
invoke_handler("Create", {
38+
"SourceBucketName": "<source-bucket>",
39+
"SourceObjectKey": "<source-object-key>",
40+
"DestinationBucketName": "<dest-bucket-name>",
41+
"DestinationBucketKeyPrefix": "/"
42+
})
43+
44+
self.assertAwsCommands(
45+
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
46+
"s3 sync --delete contents.zip s3://<dest-bucket-name>/"
47+
)
48+
49+
3750
def test_create_update_with_dest_key(self):
3851
invoke_handler("Create", {
3952
"SourceBucketName": "<source-bucket>",
@@ -47,12 +60,13 @@ def test_create_update_with_dest_key(self):
4760
"s3 sync --delete contents.zip s3://<dest-bucket-name>/<dest-key-prefix>"
4861
)
4962

50-
def test_delete(self):
63+
def test_delete_no_retain(self):
5164
invoke_handler("Delete", {
5265
"SourceBucketName": "<source-bucket>",
5366
"SourceObjectKey": "<source-object-key>",
54-
"DestinationBucketName": "<dest-bucket-name>"
55-
})
67+
"DestinationBucketName": "<dest-bucket-name>",
68+
"RetainOnDelete": "false"
69+
}, physical_id="<physicalid>")
5670

5771
self.assertAwsCommands("s3 rm s3://<dest-bucket-name>/ --recursive")
5872

@@ -61,18 +75,30 @@ def test_delete_with_dest_key(self):
6175
"SourceBucketName": "<source-bucket>",
6276
"SourceObjectKey": "<source-object-key>",
6377
"DestinationBucketName": "<dest-bucket-name>",
64-
"DestinationBucketKeyPrefix": "<dest-key-prefix>"
65-
})
78+
"DestinationBucketKeyPrefix": "<dest-key-prefix>",
79+
"RetainOnDelete": "false"
80+
}, physical_id="<physicalid>")
6681

6782
self.assertAwsCommands("s3 rm s3://<dest-bucket-name>/<dest-key-prefix> --recursive")
6883

69-
def test_delete_with_retain(self):
84+
def test_delete_with_retain_explicit(self):
7085
invoke_handler("Delete", {
7186
"SourceBucketName": "<source-bucket>",
7287
"SourceObjectKey": "<source-object-key>",
7388
"DestinationBucketName": "<dest-bucket-name>",
7489
"RetainOnDelete": "true"
75-
})
90+
}, physical_id="<physicalid>")
91+
92+
# no aws commands (retain)
93+
self.assertAwsCommands()
94+
95+
# RetainOnDelete=true is the default
96+
def test_delete_with_retain_implicit_default(self):
97+
invoke_handler("Delete", {
98+
"SourceBucketName": "<source-bucket>",
99+
"SourceObjectKey": "<source-object-key>",
100+
"DestinationBucketName": "<dest-bucket-name>"
101+
}, physical_id="<physicalid>")
76102

77103
# no aws commands (retain)
78104
self.assertAwsCommands()
@@ -83,7 +109,7 @@ def test_delete_with_retain_explicitly_false(self):
83109
"SourceObjectKey": "<source-object-key>",
84110
"DestinationBucketName": "<dest-bucket-name>",
85111
"RetainOnDelete": "false"
86-
})
112+
}, physical_id="<physicalid>")
87113

88114
self.assertAwsCommands(
89115
"s3 rm s3://<dest-bucket-name>/ --recursive"
@@ -100,7 +126,7 @@ def test_update_same_dest(self):
100126
"DestinationBucketName": "<dest-bucket-name>",
101127
}, old_resource_props={
102128
"DestinationBucketName": "<dest-bucket-name>",
103-
})
129+
}, physical_id="<physical-id>")
104130

105131
self.assertAwsCommands(
106132
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
@@ -115,62 +141,136 @@ def test_update_new_dest_retain(self):
115141
}, old_resource_props={
116142
"DestinationBucketName": "<dest-bucket-name>",
117143
"RetainOnDelete": "true"
118-
})
144+
}, physical_id="<physical-id>")
119145

120146
self.assertAwsCommands(
121147
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
122148
"s3 sync --delete contents.zip s3://<dest-bucket-name>/"
123149
)
124150

125-
def test_update_new_dest_no_retain_explicit(self):
151+
def test_update_new_dest_no_retain(self):
126152
invoke_handler("Update", {
127153
"SourceBucketName": "<source-bucket>",
128154
"SourceObjectKey": "<source-object-key>",
129155
"DestinationBucketName": "<new-dest-bucket-name>",
156+
"RetainOnDelete": "false"
130157
}, old_resource_props={
131158
"DestinationBucketName": "<old-dest-bucket-name>",
132159
"DestinationBucketKeyPrefix": "<old-dest-prefix>",
133160
"RetainOnDelete": "false"
134-
})
161+
}, physical_id="<physical-id>")
135162

136163
self.assertAwsCommands(
137164
"s3 rm s3://<old-dest-bucket-name>/<old-dest-prefix> --recursive",
138165
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
139166
"s3 sync --delete contents.zip s3://<new-dest-bucket-name>/"
140167
)
141168

142-
def test_update_new_dest_no_retain_implicit(self):
169+
def test_update_new_dest_retain_implicit(self):
143170
invoke_handler("Update", {
144171
"SourceBucketName": "<source-bucket>",
145172
"SourceObjectKey": "<source-object-key>",
146173
"DestinationBucketName": "<new-dest-bucket-name>",
147174
}, old_resource_props={
148175
"DestinationBucketName": "<old-dest-bucket-name>",
149176
"DestinationBucketKeyPrefix": "<old-dest-prefix>"
150-
})
177+
}, physical_id="<physical-id>")
151178

152179
self.assertAwsCommands(
153-
"s3 rm s3://<old-dest-bucket-name>/<old-dest-prefix> --recursive",
154180
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
155181
"s3 sync --delete contents.zip s3://<new-dest-bucket-name>/"
156182
)
157183

158-
def test_update_new_dest_prefix_no_retain_implicit(self):
184+
def test_update_new_dest_prefix_no_retain(self):
159185
invoke_handler("Update", {
160186
"SourceBucketName": "<source-bucket>",
161187
"SourceObjectKey": "<source-object-key>",
162188
"DestinationBucketName": "<dest-bucket-name>",
163-
"DestinationBucketKeyPrefix": "<new-dest-prefix>"
189+
"DestinationBucketKeyPrefix": "<new-dest-prefix>",
190+
"RetainOnDelete": "false"
164191
}, old_resource_props={
165192
"DestinationBucketName": "<dest-bucket-name>",
166-
})
193+
"RetainOnDelete": "false"
194+
}, physical_id="<physical id>")
167195

168196
self.assertAwsCommands(
169197
"s3 rm s3://<dest-bucket-name>/ --recursive",
170198
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
171199
"s3 sync --delete contents.zip s3://<dest-bucket-name>/<new-dest-prefix>"
172200
)
173201

202+
def test_update_new_dest_prefix_retain_implicit(self):
203+
invoke_handler("Update", {
204+
"SourceBucketName": "<source-bucket>",
205+
"SourceObjectKey": "<source-object-key>",
206+
"DestinationBucketName": "<dest-bucket-name>",
207+
"DestinationBucketKeyPrefix": "<new-dest-prefix>"
208+
}, old_resource_props={
209+
"DestinationBucketName": "<dest-bucket-name>",
210+
}, physical_id="<physical id>")
211+
212+
self.assertAwsCommands(
213+
"s3 cp s3://<source-bucket>/<source-object-key> archive.zip",
214+
"s3 sync --delete contents.zip s3://<dest-bucket-name>/<new-dest-prefix>"
215+
)
216+
217+
#
218+
# physical id
219+
#
220+
221+
def test_physical_id_allocated_on_create_and_reused_afterwards(self):
222+
create_resp = invoke_handler("Create", {
223+
"SourceBucketName": "<source-bucket>",
224+
"SourceObjectKey": "<source-object-key>",
225+
"DestinationBucketName": "<dest-bucket-name>",
226+
})
227+
228+
phid = create_resp['PhysicalResourceId']
229+
self.assertTrue(phid.startswith('aws.cdk.s3deployment'))
230+
231+
# now issue an update and pass in the physical id. expect the same
232+
# one to be returned back
233+
update_resp = invoke_handler("Update", {
234+
"SourceBucketName": "<source-bucket>",
235+
"SourceObjectKey": "<source-object-key>",
236+
"DestinationBucketName": "<new-dest-bucket-name>",
237+
}, old_resource_props={
238+
"DestinationBucketName": "<dest-bucket-name>",
239+
}, physical_id=phid)
240+
self.assertEqual(update_resp['PhysicalResourceId'], phid)
241+
242+
# now issue a delete, and make sure this also applies
243+
delete_resp = invoke_handler("Delete", {
244+
"SourceBucketName": "<source-bucket>",
245+
"SourceObjectKey": "<source-object-key>",
246+
"DestinationBucketName": "<dest-bucket-name>",
247+
"RetainOnDelete": "false"
248+
}, physical_id=phid)
249+
self.assertEqual(delete_resp['PhysicalResourceId'], phid)
250+
251+
def test_fails_when_physical_id_not_present_in_update(self):
252+
update_resp = invoke_handler("Update", {
253+
"SourceBucketName": "<source-bucket>",
254+
"SourceObjectKey": "<source-object-key>",
255+
"DestinationBucketName": "<new-dest-bucket-name>",
256+
}, old_resource_props={
257+
"DestinationBucketName": "<dest-bucket-name>",
258+
}, expected_status="FAILED")
259+
260+
self.assertEqual(update_resp['Reason'], "invalid request: request type is 'Update' but 'PhysicalResourceId' is not defined")
261+
262+
def test_fails_when_physical_id_not_present_in_delete(self):
263+
update_resp = invoke_handler("Delete", {
264+
"SourceBucketName": "<source-bucket>",
265+
"SourceObjectKey": "<source-object-key>",
266+
"DestinationBucketName": "<new-dest-bucket-name>",
267+
}, old_resource_props={
268+
"DestinationBucketName": "<dest-bucket-name>",
269+
}, expected_status="FAILED")
270+
271+
self.assertEqual(update_resp['Reason'], "invalid request: request type is 'Delete' but 'PhysicalResourceId' is not defined")
272+
273+
174274
# asserts that a given list of "aws xxx" commands have been invoked (in order)
175275
def assertAwsCommands(self, *expected):
176276
actual = read_aws_out()
@@ -193,7 +293,7 @@ def read_aws_out():
193293
# requestType: CloudFormation request type ("Create", "Update", "Delete")
194294
# resourceProps: map to pass to "ResourceProperties"
195295
# expected_status: "SUCCESS" or "FAILED"
196-
def invoke_handler(requestType, resourceProps, old_resource_props=None, expected_status='SUCCESS'):
296+
def invoke_handler(requestType, resourceProps, old_resource_props=None, physical_id=None, expected_status='SUCCESS'):
197297
response_url = '<response-url>'
198298

199299
event={
@@ -208,6 +308,9 @@ def invoke_handler(requestType, resourceProps, old_resource_props=None, expected
208308
if old_resource_props:
209309
event['OldResourceProperties'] = old_resource_props
210310

311+
if physical_id:
312+
event['PhysicalResourceId'] = physical_id
313+
211314
class ContextMock: log_stream_name = 'log_stream'
212315
class ResponseMock: reason = 'OK'
213316

‎packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,22 @@ export interface BucketDeploymentProps {
1919
destinationBucket: s3.BucketRef;
2020

2121
/**
22-
* Key prefix in desination.
23-
* @default No prefix (source == dest)
22+
* Key prefix in the destination bucket.
23+
*
24+
* @default "/" (unzip to root of the destination bucket)
2425
*/
2526
destinationKeyPrefix?: string;
2627

2728
/**
28-
* If this is enabled, files in destination bucket/prefix will not be deleted
29-
* when the resource is deleted or removed from the stack.
29+
* If this is set to "false", the destination files will be deleted when the
30+
* resource is deleted or the destination is updated.
31+
*
32+
* NOTICE: if this is set to "false" and destination bucket/prefix is updated,
33+
* all files in the previous destination will first be deleted and then
34+
* uploaded to the new destination location. This could have availablity
35+
* implications on your users.
3036
*
31-
* @default false (when resource is deleted, files are deleted)
37+
* @default true - when resource is deleted/updated, files are retained
3238
*/
3339
retainOnDelete?: boolean;
3440
}

0 commit comments

Comments
 (0)