Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s3-notifications): cdk destroy deletes external/existing s3 notification events #29939

Merged
merged 5 commits into from
Apr 24, 2024
Merged

fix(s3-notifications): cdk destroy deletes external/existing s3 notification events #29939

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fcbff13
fix(s3-notifications): cdk destroy deletes external/existing s3 notif…
GavinZZ Apr 23, 2024
cc054de
Update tests
GavinZZ Apr 23, 2024
28cf3d4
Update tests
GavinZZ Apr 23, 2024
aeb003e
Update integ tests
GavinZZ Apr 24, 2024
cb96945
Merge branch 'main' into yuanhaoz/fix_s3_notifications
mergify[bot] Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions ...ages/@aws-cdk/custom-resource-handlers/lib/aws-s3/notifications-resource-handler/index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def handler(event: dict, context):
config = handle_managed(event["RequestType"], notification_configuration)
else:
config = handle_unmanaged(props["BucketName"], stack_id, event["RequestType"], notification_configuration, old)
s3.put_bucket_notification_configuration(Bucket=props["BucketName"], NotificationConfiguration=config)
put_bucket_notification_configuration(Bucket=props["BucketName"], NotificationConfiguration=config)
except Exception as e:
logging.exception("Failed to put bucket notification configuration")
response_status = "FAILED"
Expand All @@ -41,13 +41,17 @@ def with_id(n):

# find external notifications
external_notifications = {}
existing_notifications = s3.get_bucket_notification_configuration(Bucket=bucket)
existing_notifications = get_bucket_notification_configuration(Bucket=bucket)
for t in CONFIGURATION_TYPES:
if request_type == 'Update':
ids = [with_id(n) for n in old.get(t, [])]
old_incoming_ids = [n['Id'] for n in ids]
# if the notification was created by us, we know what id to expect so we can filter by it.
external_notifications[t] = [n for n in existing_notifications.get(t, []) if not n['Id'] in old_incoming_ids]
elif request_type == 'Delete':
# For 'Delete' request, old parameter is an empty dict so we cannot use this to determine which are external
# notifications. Fall back to rely on the stack naming logic.
external_notifications[t] = [n for n in existing_notifications.get(t, []) if not n['Id'].startswith(f"{stack_id}-")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Falling back to the previous correct solution 37be7b9#diff-00d1f29e73955f487795d4a9873bcff824e70d6268e7926e44b7defb31b4ecb1L74

elif request_type == 'Create':
# if this is a create event then all existing notifications are external
external_notifications[t] = [n for n in existing_notifications.get(t, [])]
Expand Down Expand Up @@ -75,6 +79,12 @@ def with_id(n):

return notifications

def get_bucket_notification_configuration(bucket):
return s3.get_bucket_notification_configuration(Bucket=bucket)

def put_bucket_notification_configuration(bucket, notification_configuration):
s3.put_bucket_notification_configuration(Bucket=bucket, NotificationConfiguration=notification_configuration)

def submit_response(event: dict, context, response_status: str, error_message: str):
response_body = json.dumps(
{
Expand Down
78 changes: 42 additions & 36 deletions ...aws-cdk/custom-resource-handlers/test/aws-s3/notifications-resource-handler/test_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ def test_create(self, _, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
event["ResourceProperties"]["NotificationConfiguration"],
Bucket=event["ResourceProperties"]["BucketName"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update this test file because this PR only updated the handler file but didn't change or run the test files. This results in all these tests failing.

NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -108,8 +108,8 @@ def test_update(self, _, put):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
event["ResourceProperties"]["NotificationConfiguration"],
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -120,7 +120,7 @@ def test_delete(self, _, put: MagicMock):

index.handler(event, {})

put.assert_called_once_with(event["ResourceProperties"]["BucketName"], {})
put.assert_called_once_with(Bucket=event["ResourceProperties"]["BucketName"], NotificationConfiguration={})


class UnmanagedCleanBucketTest(unittest.TestCase):
Expand All @@ -136,8 +136,8 @@ def test_create(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
event["ResourceProperties"]["NotificationConfiguration"],
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -152,8 +152,8 @@ def test_create_with_eventbridge(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
event["ResourceProperties"]["NotificationConfiguration"],
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -170,8 +170,11 @@ def test_update(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
event["ResourceProperties"]["NotificationConfiguration"],
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"]
),
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -188,8 +191,11 @@ def test_update_with_eventbridge(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
event["ResourceProperties"]["NotificationConfiguration"],
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"]
),
)


Expand All @@ -207,9 +213,9 @@ def test_update_with_existing_eventbridge(self, _, get: MagicMock, put: MagicMoc
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
make_eventbridge_configuration(),
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
)
Expand All @@ -228,8 +234,8 @@ def test_delete(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
make_empty_notification_configuration(),
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=make_empty_notification_configuration(),
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -246,8 +252,8 @@ def test_delete_with_eventbridge_should_not_remove_eventbridge(self, _, get: Mag
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
make_empty_notification_configuration_with_eventbridge(),
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=make_empty_notification_configuration_with_eventbridge(),
)


Expand All @@ -266,8 +272,8 @@ def test_create(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
Expand All @@ -287,8 +293,8 @@ def test_create_with_eventbridge(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
Expand All @@ -308,8 +314,8 @@ def test_create_with_existing_eventbridge(self, _, get: MagicMock, put: MagicMoc
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
Expand All @@ -329,8 +335,8 @@ def test_update(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
Expand All @@ -350,8 +356,8 @@ def test_update_with_eventbridge(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
Expand All @@ -371,8 +377,8 @@ def test_update_without_eventbridge_should_not_remove_existing_eventbridge(self,
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
merge_notification_configurations(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=merge_notification_configurations(
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
Expand All @@ -392,8 +398,8 @@ def test_delete(self, _, get: MagicMock, put: MagicMock):
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
current_notifications,
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=current_notifications,
)

@patch("index.put_bucket_notification_configuration")
Expand All @@ -410,8 +416,8 @@ def test_delete_with_eventbridge_should_not_remove_eventbridge(self, _, get: Mag
index.handler(event, {})

put.assert_called_once_with(
event["ResourceProperties"]["BucketName"],
current_notifications,
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=current_notifications,
)


Expand Down
Loading