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.

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

feat(s3): add skip destination validation property #30916

Merged
merged 13 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"BAllowBucketNotificationsTolambdaeventsources3F741608059EF9F709",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"TopicPolicyA1747468",
Expand Down Expand Up @@ -305,7 +306,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"Topic3Policy49BDDFBD",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"YourBucketAllowBucketNotificationsTocdkinteglambdabuckets3notificationsMyFunction47013F39EAF10051"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"ObjectCreatedTopicPolicyA938ECFC",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"EncryptedQueueKey6F4FD304",
Expand Down Expand Up @@ -322,7 +323,8 @@
}
]
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"Bucket2Policy945B22E3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@
"NotificationConfiguration": {
"EventBridgeConfiguration": {}
},
"Managed": true
"Managed": true,
"SkipDestinationValidation": false
},
"DependsOn": [
"MyEventBridgeBucketPolicy8F5346E3"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@ def handler(event: dict, context):
props = event["ResourceProperties"]
notification_configuration = props["NotificationConfiguration"]
managed = props.get('Managed', 'true').lower() == 'true'
skipDestinationValidation = props.get('SkipDestinationValidation', 'false').lower() == 'true'
stack_id = event['StackId']
old = event.get("OldResourceProperties", {}).get("NotificationConfiguration", {})
if managed:
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)
s3.put_bucket_notification_configuration(Bucket=props["BucketName"], NotificationConfiguration=config, SkipDestinationValidation=skipDestinationValidation)
except Exception as e:
logging.exception("Failed to put bucket notification configuration")
response_status = "FAILED"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM public.ecr.aws/lambda/python:3.7
FROM public.ecr.aws/lambda/python:3.11

gracelu0 marked this conversation as resolved.
Show resolved Hide resolved
ADD . /opt/lambda
WORKDIR /opt/lambda
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ def make_event(request_type: str, managed: bool):
"Managed": str(managed),
"BucketName": "BucketName",
"NotificationConfiguration": make_notification_configuration(),
"SkipDestinationValidation": str(False),
},
}

Expand All @@ -43,6 +44,19 @@ def make_event_with_eventbridge(request_type: str, managed: bool):
"Managed": str(managed),
"BucketName": "BucketName",
"NotificationConfiguration": make_notification_configuration_with_eventbridge(),
"SkipDestinationValidation": str(False),
},
}

def make_event_with_skip_destination_validation(request_type: str, managed: bool):
return {
"StackId": "StackId",
"RequestType": request_type,
"ResourceProperties": {
"Managed": str(managed),
"BucketName": "BucketName",
"NotificationConfiguration": make_notification_configuration(),
"SkipDestinationValidation": str(True),
},
}

Expand Down Expand Up @@ -96,6 +110,7 @@ def test_create(self, _, mock_s3: MagicMock):
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -109,6 +124,7 @@ def test_update(self, _, mock_s3: MagicMock):
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -119,7 +135,25 @@ def test_delete(self, _, mock_s3: MagicMock):

index.handler(event, {})

mock_s3.put_bucket_notification_configuration.assert_called_once_with(Bucket=event["ResourceProperties"]["BucketName"], NotificationConfiguration={})
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration={},
SkipDestinationValidation=False,
)

@patch('index.s3')
@patch("index.submit_response")
def test_skip_destination_validation(self, _, mock_s3: MagicMock):

event = make_event_with_skip_destination_validation("Create", True)

index.handler(event, {})

mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
SkipDestinationValidation=True,
)


class UnmanagedCleanBucketTest(unittest.TestCase):
Expand All @@ -136,6 +170,7 @@ def test_create(self, _, mock_s3: MagicMock):
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -151,6 +186,7 @@ def test_create_with_eventbridge(self, _, mock_s3: MagicMock):
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=event["ResourceProperties"]["NotificationConfiguration"],
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -171,6 +207,7 @@ def test_update(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"]
),
SkipDestinationValidation=False,
)


Expand All @@ -192,6 +229,7 @@ def test_delete_existing_s3_notifications(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"]
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -212,6 +250,7 @@ def test_update_with_eventbridge(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"]
),
SkipDestinationValidation=False,
)


Expand All @@ -233,6 +272,7 @@ def test_update_with_existing_eventbridge(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -250,6 +290,7 @@ def test_delete(self, _, mock_s3: MagicMock):
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=make_empty_notification_configuration(),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -267,6 +308,7 @@ def test_delete_with_eventbridge_should_not_remove_eventbridge(self, _, mock_s3:
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=make_empty_notification_configuration_with_eventbridge(),
SkipDestinationValidation=False,
)


Expand All @@ -289,6 +331,7 @@ def test_create(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -309,6 +352,7 @@ def test_create_with_eventbridge(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -329,6 +373,7 @@ def test_create_with_existing_eventbridge(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -349,6 +394,7 @@ def test_update(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -369,6 +415,7 @@ def test_update_with_eventbridge(self, _, mock_s3: MagicMock):
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -387,6 +434,7 @@ def test_update_without_eventbridge_should_not_remove_existing_eventbridge(self,
current_notifications,
event["ResourceProperties"]["NotificationConfiguration"],
),
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -404,6 +452,7 @@ def test_delete(self, _, mock_s3: MagicMock):
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=current_notifications,
SkipDestinationValidation=False,
)

@patch('index.s3')
Expand All @@ -421,6 +470,7 @@ def test_delete_with_eventbridge_should_not_remove_eventbridge(self, _, mock_s3:
mock_s3.put_bucket_notification_configuration.assert_called_once_with(
Bucket=event["ResourceProperties"]["BucketName"],
NotificationConfiguration=current_notifications,
SkipDestinationValidation=False,
)


Expand Down
10 changes: 10 additions & 0 deletions packages/aws-cdk-lib/aws-s3/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,16 @@ const bucket = s3.Bucket.fromBucketAttributes(this, 'ImportedBucket', {
bucket.addEventNotification(s3.EventType.OBJECT_CREATED, new s3n.SnsDestination(topic));
```

If you do not want for S3 to validate permissions of Amazon SQS, Amazon SNS, and Lambda destinations you can use this flag:
yerzhan7 marked this conversation as resolved.
Show resolved Hide resolved

```ts
declare const myQueue: sqs.Queue;
const bucket = new s3.Bucket(this, 'MyBucket', {
notificationsSkipDestinationValidation: true,
});
bucket.addEventNotification(s3.EventType.OBJECT_REMOVED, new s3n.SqsDestination(myQueue));
```

When you add an event notification to a bucket, a custom resource is created to
manage the notifications. By default, a new role is created for the Lambda
function that implements this feature. If you want to use your own role instead,
Expand Down
11 changes: 11 additions & 0 deletions packages/aws-cdk-lib/aws-s3/lib/bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,8 @@ export abstract class BucketBase extends Resource implements IBucket {

protected notificationsHandlerRole?: iam.IRole;

protected notificationsSkipDestinationValidation?: boolean;

protected objectOwnership?: ObjectOwnership;

constructor(scope: Construct, id: string, props: ResourceProps = {}) {
Expand Down Expand Up @@ -890,6 +892,7 @@ export abstract class BucketBase extends Resource implements IBucket {
this.notifications = new BucketNotifications(this, 'Notifications', {
bucket: this,
handlerRole: this.notificationsHandlerRole,
skipDestinationValidation: this.notificationsSkipDestinationValidation ?? false,
});
}
cb(this.notifications);
Expand Down Expand Up @@ -1652,6 +1655,13 @@ export interface BucketProps {
*/
readonly notificationsHandlerRole?: iam.IRole;

/**
* Skips notification validation of Amazon SQS, Amazon SNS, and Lambda destinations.
*
* @default false
*/
readonly notificationsSkipDestinationValidation?: boolean;

/**
* Inteligent Tiering Configurations
*
Expand Down Expand Up @@ -1911,6 +1921,7 @@ export class Bucket extends BucketBase {
});

this.notificationsHandlerRole = props.notificationsHandlerRole;
this.notificationsSkipDestinationValidation = props.notificationsSkipDestinationValidation;

const { bucketEncryption, encryptionKey } = this.parseEncryption(props);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ interface NotificationsProps {
* The role to be used by the lambda handler
*/
handlerRole?: iam.IRole;

/**
* Skips notification validation of Amazon SQS, Amazon SNS, and Lambda destinations.
*/
skipDestinationValidation: boolean;
}

/**
Expand All @@ -40,11 +45,13 @@ export class BucketNotifications extends Construct {
private resource?: cdk.CfnResource;
private readonly bucket: IBucket;
private readonly handlerRole?: iam.IRole;
private readonly skipDestinationValidation: boolean;

constructor(scope: Construct, id: string, props: NotificationsProps) {
super(scope, id);
this.bucket = props.bucket;
this.handlerRole = props.handlerRole;
this.skipDestinationValidation = props.skipDestinationValidation;
}

/**
Expand Down Expand Up @@ -133,6 +140,7 @@ export class BucketNotifications extends Construct {
BucketName: this.bucket.bucketName,
NotificationConfiguration: cdk.Lazy.any({ produce: () => this.renderNotificationConfiguration() }),
Managed: managed,
SkipDestinationValidation: this.skipDestinationValidation ?? false,
},
});

Expand Down
Loading
Loading