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

SNS Topic Policy can't be created and synth'd #7934

Closed
mbonig opened this issue May 12, 2020 · 5 comments · Fixed by #10559
Closed

SNS Topic Policy can't be created and synth'd #7934

mbonig opened this issue May 12, 2020 · 5 comments · Fixed by #10559
Assignees
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1

Comments

@mbonig
Copy link
Contributor

mbonig commented May 12, 2020

❓ General Issue

When creating an SNS TopicPolicy the Props only have topics as a field. However, there also needs to be a PolicyDocument field and without it an error occurs during synth. There doesn't appear to be any way to set it, either through the Props in the constructor or after the fact.

The Question

With this minimum example:

const topic = new Topic(this, 'topic', {});
const tp = new TopicPolicy(this, 'tp', {
    topics: [topic],
});

An error occurs during synth:

Error: Resolution error: Supplied properties not correct for "CfnTopicPolicyProps"
  policyDocument: required but missing.

Environment

  • CDK CLI Version: 1.38.0 (build d5fa31f)
  • Module Version: 1.38.0
  • OS: Mac OS 10.15.4
  • Language: Typescript

Other information

@mbonig mbonig added the needs-triage This issue or PR still needs to be triaged. label May 12, 2020
@SomayaB SomayaB added the @aws-cdk/aws-sns Related to Amazon Simple Notification Service label May 12, 2020
@SomayaB SomayaB added the guidance Question that needs advice or information. label May 12, 2020
@SomayaB
Copy link
Contributor

SomayaB commented Aug 7, 2020

Hi @mbonig, sorry for the late reply. It does seem like this is a bug, I'm getting the same error. So I'll file this as a bug and we'll add it to our workload. In the meantime, you might want to try using Topic.addToResourcePolicy(statement) instead of TopicPolicy.

@SomayaB SomayaB added bug This issue is a bug. and removed guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Aug 7, 2020
@MrArnoldPalmer MrArnoldPalmer added effort/small Small work item – less than a day of effort p1 labels Aug 17, 2020
ap00rv added a commit to ap00rv/aws-cdk that referenced this issue Sep 27, 2020
TopicPolicy class had a bug. It did specify a mandatory prop for IAM policy document to use in AWS::SNS::TopicPolicy

fixes aws#7934
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Sep 29, 2020
@sunojram
Copy link

Do we have any updates for this issue? I'm not blocked but would be nice to have this.

@MrArnoldPalmer
Copy link
Contributor

MrArnoldPalmer commented Mar 16, 2021

So after spending some time researching this and talking to @rix0rrr, I think that the correct usage here is to use addStatements to add statements to the policy document post construction. You shouldn't be synthesizing an empty policy document, and CFN won't let you deploy one anyway. Here is a short example:

export class Issue10559Stack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const topic = new sns.Topic(this, 'Topic');
    const topicPolicy = new sns.TopicPolicy(this, 'Policy', {
      topics: [topic],
    });
    
    topicPolicy.document.addStatements(new iam.PolicyStatement({
        actions: ["sns:Subscribe"],
        principals: [new iam.AccountPrincipal('430030518091')],
        resources: [topic.topicArn] 
      })
    );
  }
}

General I'd recommend using addToResourcePolicy over creating a TopicPolicy but the above allows synthesizing one.

@MrArnoldPalmer
Copy link
Contributor

Additionally added an issue to track the addition of local input validation for the policy statements passed to PolicyDocument.
#13617

@mergify mergify bot closed this as completed in #10559 Mar 17, 2021
mergify bot pushed a commit that referenced this issue Mar 17, 2021
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents.

fixes #7934

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Mar 17, 2021
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents.

fixes aws#7934

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Mar 18, 2021
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents.

fixes aws#7934

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents.

fixes aws#7934

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-sns Related to Amazon Simple Notification Service bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants