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

(aws-event-targets): Clarify that adding an imported SNS Topic as an event target does not set required permissions #25583

Closed
SamStephens opened this issue May 15, 2023 · 3 comments · Fixed by #30279
Labels
@aws-cdk/aws-events-targets documentation This is a problem with documentation. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@SamStephens
Copy link
Contributor

SamStephens commented May 15, 2023

Describe the bug

aws_events_targets.SnsTopic implicitly grants publish permissions on the Topic to the EventBus service principal.

However, this silently is a no-op when the Topic is imported.

This is a particularly painful experience because there's very little visibility as to what is going on. All you see is FailedInvocations for your EventBus Rule, with no indication as to what is going on.

Expected Behavior

I expected using aws_events_targets.SnsTopic to wire up a working integration.

Current Behavior

My integration didn't work because required permissions were not granted.

Reproduction Steps

Declare an SNS Topic in one stack. In another stack, import and use that Topic in a rule.

        topic = aws_sns.Topic.from_topic_arn(
            scope=self,
            id="Alarm",
            topic_arn='your-topic-arn',
        )

        rule = aws_events.Rule(
            scope=self,
            id="FindingNotification",
            enabled=True,
            schedule=aws_events.Schedule.rate(Duration.minutes(1)),
            targets=[
                aws_events_targets.SnsTopic(
                    topic=topic ,
                )
            ],
        )

You'll see your rule has a failed invocation every minute.

Possible Solution

If I understand how permissions are granted via Resource Policy, it's either not possible or very complex to grant access to a Topic that's not declared in our current stack. However the silent failure is confusing, and in an ideal world you'd opt into having to set up permissions yourself. Something like:

        rule = aws_events.Rule(
            scope=self,
            id="FindingNotification",
            enabled=True,
            schedule=aws_events.Schedule.rate(Duration.minutes(1)),
            targets=[
                aws_events_targets.SnsTopic(
                    topic=topic ,
                    configure_permissions=False,
                )
            ],
        )

And without setting configure_permissions to False, synthesis would fail for an imported Topic.

Additional Information/Context

No response

CDK CLI Version

2.79.1 (build 2e7f8b7)

Framework Version

2.79.1

Node.js Version

v16.18.1

OS

Ubuntu (Windows Subsystem for Linux)

Language

Python

Language Version

3.9.7

Other information

No response

@SamStephens SamStephens added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 15, 2023
@peterwoodworth
Copy link
Contributor

I think the better solution here is to clearly call this out in the targets.SnsTopic docs, rather than introducing a prop

@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p2 effort/small Small work item – less than a day of effort documentation This is a problem with documentation. and removed needs-triage This issue or PR still needs to be triaged. labels May 16, 2023
@Zishanwang1992
Copy link
Contributor

I think we can focus on auditing/logging improvement, for example the reason why the rule fails. Not sure if it can be found from Cloudtrail or CloudWatch log.

If the two resources are defined in two separate stacks, it makes sense to me to have "no-op" to comply with separate of concern.

@peterwoodworth peterwoodworth added feature-request A feature should be added or improved. and removed bug This issue is a bug. labels May 22, 2023
@peterwoodworth peterwoodworth changed the title (aws-event-targets): Adding an imported SNS Topic as an event target silently does not set required permissions (aws-event-targets): Clarify that adding an imported SNS Topic as an event target does not set required permissions May 22, 2023
@mergify mergify bot closed this as completed in #30279 May 20, 2024
mergify bot pushed a commit that referenced this issue May 20, 2024
…event target does not set required permissions (#30279)

### Issue #25583 

Closes #25583

### Reason for this change

Add documentation to clarify that imported topics have to have the required permissions set manually.

### Description of changes

Added docstring.

### Description of how you validated changes



### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

mazyu36 pushed a commit to mazyu36/aws-cdk that referenced this issue May 22, 2024
…event target does not set required permissions (aws#30279)

### Issue aws#25583 

Closes aws#25583

### Reason for this change

Add documentation to clarify that imported topics have to have the required permissions set manually.

### Description of changes

Added docstring.

### Description of how you validated changes



### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
atanaspam pushed a commit to atanaspam/aws-cdk that referenced this issue Jun 3, 2024
…event target does not set required permissions (aws#30279)

### Issue aws#25583 

Closes aws#25583

### Reason for this change

Add documentation to clarify that imported topics have to have the required permissions set manually.

### Description of changes

Added docstring.

### Description of how you validated changes



### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*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-events-targets documentation This is a problem with documentation. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
3 participants