Bucket Notifications #201

eladb · 2018-06-30T00:29:30Z

Adds support for S3 bucket notifications. The bucket.onEvent method will add a notification destination for a bucket.

Since CloudFormation bucket notification support require two-phase deployments (due to the fact PutBucketNotification will fail if the destination policy has not been updated, and CloudFormation cannot create the policy until the bucket is created). The reason this is a limitation in CloudFormation is that they could not model the 1:1 relationship between the bucket and the notifications using the current semantics of CloudFormation.

From CloudFormation docs:

If you create the target resource and related permissions in the same template, you might have a circular dependency. For example, you might use the AWS::Lambda::Permission resource to grant the S3 bucket to invoke a Lambda function. However, AWS CloudFormation can't create the S3 bucket until the bucket has permission to invoke the function (AWS CloudFormation checks if the S3 bucket can invoke the function). If you're using Refs to pass the bucket name, this leads to a circular dependency. To avoid this dependency, you can create all resources without specifying the notification configuration. Then, update the stack with a notification configuration.

In the CDK, we can model this relationship by encapsulating the notifications custom resource behind a bucket. This means that users don't interact with this resource directly, but rather just subscribe to notifications on a bucket, and the resource (and accompanying handler) will be created as needed.

The s3.INotificationDestination interface is used to allow SNS, SQS and Lambda to implement notification destinations. This interface inverts the control and allows the destination to prepare to receive notifications. For example, it can modify it's policy appropriately.

SNS, SQS and Lambda destinations are going to be supported in an upcoming change.

RomainMuller · 2018-07-03T09:27:43Z

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

+        }
+
+        return {
+            lambdaConfigurations: lambda.length > 0 ? lambda : undefined,


What's the value add for the special-casing of empty arrays? Is it worth it?

RomainMuller · 2018-07-03T09:32:57Z

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

+                } else if (rule.endsWith('*')) {
+                    renderedRules.push({ name: 'prefix', value: rule.substr(0, rule.length - 1) });
+                } else {
+                    throw new Error('Rule must either have a "*" prefix or suffix to indicate the rule type: ' + rule);


Unfortunately, * is a character that is explicitly listed as "safe for use in S3 object keys" (see docs). I'm afraid that using a * prefix of suffix may eventually get in the way, as there is no provision for escaping those here.

Also, I'm not fond of passing strings around where the strings must conform to some semantic format - the compiler won't be able to check that, so any error becomes a run-time error.

DanielLaberge · 2018-08-01T12:54:58Z

Is there another way to define bucket notifications until this PR is merged? It's the only thing left blocking us from using CDK.

eladb · 2018-08-01T13:38:04Z

@DanielLaberge I hope that we will be able to attend to this shortly. We are heads down this week with the public release.

In the meantime, here's the ugliest workaround you could imagine.

Synthesize your stack using cdk synth and find the assigned logical ID of the bucket.

Then, you could manipulate the synthesized template in your code this way:

class BucketNotificationsExampleStack extends cdk.Stack {

    private readonly topic: sns.Topic;

    constructor(parent: cdk.App, name: string, props?: cdk.StackProps) {
        super(parent, name, props);

        this.topic = new sns.Topic(this, 'MyTopic');
        new s3.Bucket(this, 'MyBucket');
    }

    public toCloudFormation() {
        const template = super.toCloudFormation();

        template.Resources.MyBucketF68F3FF0.Properties = template.Resources.MyBucketF68F3FF0.Properties || { };
        template.Resources.MyBucketF68F3FF0.Properties.NotificationConfiguration = {
            TopicConfigurations: [
                {
                    Event: "s3:ObjectCreated:*",
                    Topic: this.topic.topicArn.resolve()
                }
            ]
        }

        return template;
    }
}

DanielLaberge · 2018-08-02T13:44:25Z

Thank you @eladb, that's helpful.

Would you mind sharing an example for delivering events to a lambda function?

Much appreciated.

Adds support for S3 bucket notifications. The `bucket.onEvent` method will add a notification destination for a bucket. The s3.INotificationDestination interface is used to allow SNS, SQS and Lambda to implement notification destinations. This interface inverts the control and allows the destination to prepare to receive notifications. For example, it can modify it's policy appropriately. Since CloudFormation bucket notification support require two-phase deployments (due to the fact PutBucketNotification will fail if the destination policy has not been updated, and CloudFormation cannot create the policy until the bucket is created). The reason this is a limitation in CloudFormation is that they could not model the 1:1 relationship between the bucket and the notifications using the current semantics of CloudFormation. In the CDK, we can model this relationship by encapsulating the notifications custom resource behind a bucket. This means that users don't interact with this resource directly, but rather just subcribe to notifications on a bucket, and the resource (and accompanying handler) will be created as needed.

rix0rrr · 2018-08-13T12:47:45Z

packages/@aws-cdk/aws-s3/lib/notification-dest.ts

+     * Registers this resource to receive notifications for the specified bucket.
+     * @param bucket The bucket. Use the `path` of the bucket as a unique ID.
+     */
+    bucketNotificationDestination(bucket: Bucket): NotificationDestinationProps;


I thought we said the standard name for this function was going to be notificationDestination (based off the name of the interface and the props type).

We did, but because of namespacing the interface name lost some of the context. However as a method of an L2 construct “notificationDestination” is not sufficient. We must have some delinataion as for where is this coming from. Otherwise it will look very odd. Maybe ‘s3NotificationDestination’?

Otherwise, I can rename the interface to ‘BucketNotificationDestination’. What do you think?

I am also wondering - ideally users shouldn’t care about these “inverted control” methods whatsoever (we now have a few of them). I wonder if we should find some convention that makes it intuitively clear that you don’t need to call this yourself.

We could prefix with something like an underscore... or something like “asBucketNotificationDestination”.

I kinda like the “as” prefix.

Thoughts?

Okay, changed to asBucketNotificationDestination. It's a mouthful, but not really intended to be used by users, and I feel the as prefix works.

rix0rrr · 2018-08-13T12:49:27Z

packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

+ * For 'Delete' operations, we send an empty NotificationConfiguration as
+ * required. We propagate errors and results as-is.
+ *
+ * Sadly, we can't use @aws-cdk/aws-lambda as it will introduce a dependency


That's terrible, and a problem with this pattern in general. Feels like we should extract to a separate package then.

I wouldn't say it's a general problem. I don't expect many custom resources will be needed in these layers (at least I hope so).

rix0rrr · 2018-08-13T12:50:53Z

packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts

+ * The function will issue a putBucketNotificationConfiguration request for the
+ * specified bucket.
+ */
+const handler = (event: any, context: any) => {


Did I not see you talk about deprecating the InlineJavascriptLambda in another PR?

Yes, but that doesn't mean people are not allowed to use this technique, as long as they understand what they are doing. I didn't want to make it "easy" for people to fall into the pitfalls of this by having a formal way to do it, but I don't think there's anything wrong with this specific instance. Do you?

eladb · 2018-08-13T12:19:00Z

packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts

+     * This is called lazily as we add notifications, so that if notifications are not added,
+     * there is no notifications resource.
+     */
+    private createResourceOnce() {


Reverse logic

eladb · 2018-08-13T12:19:33Z

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

+     * @see
+     * https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html
+     */
+    public onEvent(event: EventType, dest: INotificationDestination, ...s3KeyFilters: string[]) {


Change filter to be strongly typed. Prefix can only be specified once apparently for example

eladb · 2018-08-13T12:20:02Z

packages/@aws-cdk/aws-s3/test/notification-targets.ts

+
+/**
+ * Since we can't take a dependency on @aws-cdk/sns, this is a simple wrapper
+ * for AWS::SNS::Topic which implements IBucketNotificationTarget.


eladb · 2018-08-13T13:06:21Z

packages/@aws-cdk/aws-s3/lib/notification-dest.ts

+     * Registers this resource to receive notifications for the specified bucket.
+     * @param bucket The bucket. Use the `path` of the bucket as a unique ID.
+     */
+    bucketNotificationDestination(bucket: Bucket): NotificationDestinationProps;


We did, but because of namespacing the interface name lost some of the context. However as a method of an L2 construct “notificationDestination” is not sufficient. We must have some delinataion as for where is this coming from. Otherwise it will look very odd. Maybe ‘s3NotificationDestination’?

Otherwise, I can rename the interface to ‘BucketNotificationDestination’. What do you think?

I am also wondering - ideally users shouldn’t care about these “inverted control” methods whatsoever (we now have a few of them). I wonder if we should find some convention that makes it intuitively clear that you don’t need to call this yourself.

We could prefix with something like an underscore... or something like “asBucketNotificationDestination”.

I kinda like the “as” prefix.

Thoughts?

@rix0rrr

### Features * __@aws-cdk/cdk__: Tokens can now be transparently embedded into strings and encoded into JSON without losing their semantics. This makes it possible to treat late-bound (deploy-time) values as if they were regular strings ([@rix0rrr] in [#518](#518)). * __@aws-cdk/aws-s3__: add support for bucket notifications to Lambda, SNS, and SQS targets ([@eladb] in [#201](#201), [#560](#560), [#561](#561), [#564](#564)) * __@aws-cdk/cdk__: non-alphanumeric characters can now be used as construct identifiers ([@eladb] in [#556](#556)) * __@aws-cdk/aws-iam__: add support for `maxSessionDuration` for Roles ([@eladb] in [#545](#545)). ### Changes * __@aws-cdk/aws-lambda__ (_**BREAKING**_): most classes renamed to be shorter and more in line with official service naming (`Lambda` renamed to `Function` or ommitted) ([@eladb] in [#550](#550)) * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): move all CodePipeline actions from `@aws-cdk/aws-xxx-codepipeline` packages into the regular `@aws-cdk/aws-xxx` service packages ([@skinny85] in [#459](#459)). * __@aws-cdk/aws-custom-resources__ (_**BREAKING**_): package was removed, and the Custom Resource construct added to the __@aws-cdk/aws-cloudformation__ package ([@rix0rrr] in [#513](#513)) ### Fixes * __@aws-cdk/aws-lambda__: Lambdas that are triggered by CloudWatch Events now show up in the console, and can only be triggered the indicated Event Rule. _**BREAKING**_ for middleware writers (as this introduces an API change), but transparent to regular consumers ([@eladb] in [#558](#558)) * __@aws-cdk/aws-codecommit__: fix a bug where `pollForSourceChanges` could not be set to `false` ([@maciejwalkowiak] in [#534](#534)) * __aws-cdk__: don't fail if the `~/.aws/credentials` file is missing ([@RomainMuller] in [#541](#541)) * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support TemplateConfiguration ([@mindstorms6] in [#571](#571)). * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support ParameterOverrides ([@mindstorms6] in [#574](#574)). ### Known Issues * `cdk init` will try to init a `git` repository and fail if no global `user.name` and `user.email` have been configured.

eladb · 2018-08-16T19:38:47Z

@DanielLaberge this is now shipped with 0.8.2 - let us know if you have any feedback!
Here are the docs

RomainMuller suggested changes Jul 3, 2018

View reviewed changes

eladb mentioned this pull request Jul 4, 2018

Cloud Assemblies and the App Model #233

Closed

9 tasks

eladb force-pushed the benisrae/bucket-notifications branch from 625aef4 to 9b268cc Compare August 13, 2018 11:54

eladb changed the title ~~Bucket Notifications~~ WIP: Bucket Notifications Aug 13, 2018

rix0rrr reviewed Aug 13, 2018

View reviewed changes

rix0rrr approved these changes Aug 13, 2018

View reviewed changes

eladb commented Aug 13, 2018

View reviewed changes

Elad Ben-Israel added 2 commits August 13, 2018 16:36

CR fixes

520786a

Make filters strongly typed

b3bf78b

eladb changed the title ~~WIP: Bucket Notifications~~ Bucket Notifications Aug 13, 2018

RomainMuller approved these changes Aug 13, 2018

View reviewed changes

eladb merged commit 8cd07e6 into master Aug 13, 2018

eladb deleted the benisrae/bucket-notifications branch August 13, 2018 16:09

eladb mentioned this pull request Aug 13, 2018

SNS as a Bucket Notification Destination #554

Closed

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bucket Notifications #201

Bucket Notifications #201

eladb commented Jun 30, 2018 •

edited

Loading

RomainMuller Jul 3, 2018

RomainMuller Jul 3, 2018

eladb Aug 13, 2018

DanielLaberge commented Aug 1, 2018

eladb commented Aug 1, 2018

DanielLaberge commented Aug 2, 2018

rix0rrr Aug 13, 2018

eladb Aug 13, 2018

eladb Aug 13, 2018

rix0rrr Aug 13, 2018

eladb Aug 13, 2018

rix0rrr Aug 13, 2018

eladb Aug 13, 2018

eladb Aug 13, 2018

eladb Aug 13, 2018

eladb Aug 13, 2018

eladb Aug 13, 2018

eladb commented Aug 16, 2018

Bucket Notifications #201

Bucket Notifications #201

Conversation

eladb commented Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielLaberge commented Aug 1, 2018

eladb commented Aug 1, 2018

DanielLaberge commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Aug 16, 2018

eladb commented Jun 30, 2018 •

edited

Loading