aws-s3-notifications: addEventNotification creates new TopicPolicy instead of using the existing one

[iriskraja77]

Describe the bug

When I create an SNS Topic using TopicPolicy, calling bucket.addObjectCreatedNotification(...) will create a new TopicPolicy instead of using the existing one. Therefore, the existing TopicPolicy is overriden.

Expected Behavior

Two Policy Statements are present in the SNS Topic policy.

Current Behavior

Only one policy statement (added from S3 Notifications module) is present in the topic statement.

Reproduction Steps

Example code to reproduce:

    const bucketName = `${this.account}-example-bucket`;

    const topic = new Topic(this, 'snsTopic', {});
    const topicPolicy = new TopicPolicy(this, 'snsTopicPolicy', {
      topics: [topic]
    });
    // add to the Topic Policy
    topicPolicy.document.addStatements(new PolicyStatement({
      actions: ['sns:Publish'],
      effect: Effect.DENY,
      resources: [topic.topicArn],
      principals: [new AnyPrincipal()],
      conditions: {
        'StringNotEqualsIfExists': {
          'aws:PrincipalServiceName': 's3.amazonaws.com',
        },
      }
    }))
    const bucket = new Bucket(this, 's3bucket', {
      bucketName
    });

    // Overwrites the topic policy instead of adding to it.
    bucket.addObjectCreatedNotification(new SnsDestination(topic));

Possible Solution

No response

Additional Information/Context

The compiled CloudFormation template contains two resources for AWS::SNS::TopicPolicy, both linked to the SNS Topic. However the later one will replace the first one.

CDK CLI Version

2.23.0

Framework Version

No response

Node.js Version

14.15.0

OS

MacOS

Language

Typescript

Language Version

No response

Other information

No response

[peterwoodworth]

Hey @iriskraja77,

Our documentation for TopicPolicy states that

You almost never need to define this construct directly. All AWS resources that support resource policies have a method called addToResourcePolicy(), which will automatically create a new resource policy if one doesn't exist yet, otherwise it will add to the existing policy. Prefer to use addToResourcePolicy() instead.

I just tested it out, if you instead use addToResourcePolicy() you'll find that the behavior you want will occur 🙂

This still isn't perfect for our users though. I can entertain a fix for this - What we need is to make our Topics know if they've had a TopicPolicy associated with them when a TopicPolicy is created by the user. Topics have a private policy field which gets associated with a TopicPolicy if you use the addToResourcePolicy() method. This policy field would have to be set by the topic

[westhouseK]

Hi, @peterwoodworth

May I work on it? I would like to challenge this issue! I read aws-s3-notifications source code.

What we need is to make our Topics know if they've had a "TopicPolicy" associated with them when a "TopicPolicy" is created by the user.

Giving a Topic that has a TopicPolicy as an argument to new SnsDestination() should throw an error message, right?

Are the corrections correct in the following places? https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-s3-notifications/lib/sns.ts#L13-L21

[scanlonp]

I looked how this would be changed. Exposing Topic.policy (in sns/topic-base) and adding a line to the constructor of TopicPolicy (in sns/policy) to the effect of props.topics.map(t => t.setPolicy(this));. This does not feel clean to me since addToResourcePolicy takes care of all of this.

I think this is working as intended since it looks like your use case is covered by our recommended use patterns. Is there a use case that is not covered by addToResourcePolicy()? Unless you have any objections, I am going to close this.

[github-actions[bot]]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.