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(events): add multiple event bus policies on a single event bus #27340

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

postsa
Copy link
Contributor

@postsa postsa commented Sep 28, 2023

Enable the creation of multiple event bus policies on a single event bus.

Closes #24671.

The result of the Policies created by the integration test is a resource policy on the event bus that looks like

{
  "Version": "2012-10-17",
  "Statement": [{
    "Sid": "Statement2",
    "Effect": "Allow",
    "Principal": {
      "AWS": "arn:aws:iam::<account-id>:root"
    },
    "Action": "events:PutRule",
    "Resource": "arn:aws:events:us-west-2:<account-id>:event-bus/StackBusAA0A1E4B"
  }, {
    "Sid": "Statement1",
    "Effect": "Allow",
    "Principal": {
      "AWS": "arn:aws:iam::<account-id>:root"
    },
    "Action": "events:PutEvents",
    "Resource": "arn:aws:events:us-west-2:<account-id>:event-bus/StackBusAA0A1E4B"
  }]
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added bug This issue is a bug. p2 labels Sep 28, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team September 28, 2023 16:25
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Sep 28, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 28, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍
A couple of changes are needed in my opinion.

Note (for maintainers)
Looks to me like adding a policy with multiple statements to an event bus is acceptable.
But, this behavior is not reflected in the CloudFormation definition.
The fix proposed in this PR is a workaround that creates multiple policies for the same event bus.
The ideal solution would probably be to adapt the CloudFormation template declaration to accept multiple statements and then change the CDK implementation.

packages/aws-cdk-lib/aws-events/lib/event-bus.ts Outdated Show resolved Hide resolved
@postsa postsa requested a review from lpizzinidev October 5, 2023 16:15
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 6, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks 👍
But higher-level evaluation could be necessary.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 7, 2023
@postsa postsa force-pushed the bugfix-24671 branch 2 times, most recently from 5e9f8ab to a560960 Compare October 12, 2023 17:02
@bs-u27
Copy link

bs-u27 commented Nov 9, 2023

+1

(meant to do it on the issue my apologies)

@kaizencc kaizencc changed the title fix(events): allow the addition of more than one EventBusPolicy feat(events): add multiple event bus policies on a single event bus Dec 19, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the best we're going to get, so I don't mind adding this in. It's not really a hack to just specify more EventBusPolicy resources, we do this elsewhere though I can't come up with an example off the top of my head.

@kaizencc kaizencc added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 19, 2023
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023
Copy link
Contributor

mergify bot commented Dec 19, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 601fca6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 4dde502 into aws:main Dec 19, 2023
9 checks passed
Copy link
Contributor

mergify bot commented Dec 19, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify bot pushed a commit that referenced this pull request Jan 2, 2024
#27340 introduced the ability to create multiple event bus policies on a single event bus. To facilitate this, the logical Id was changed from `"Policy"` to the statementId. This triggers a replacement, which fails in CloudFormation because the statement ID does not change. The idea behind this PR is simple -- we are updating the statement ID of the policy to trigger a change for anyone who updates to the new version.

I think we are okay with this change because no one should be depending on the statementIds of their policies. And since the policy is not a stateful resource, updating the policy should not harm anyone. I have checked the feasibility of this PR on my own, hence the lack of an integ test.

closes #28520 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this pull request Jan 5, 2024
aws#27340 introduced the ability to create multiple event bus policies on a single event bus. To facilitate this, the logical Id was changed from `"Policy"` to the statementId. This triggers a replacement, which fails in CloudFormation because the statement ID does not change. The idea behind this PR is simple -- we are updating the statement ID of the policy to trigger a change for anyone who updates to the new version.

I think we are okay with this change because no one should be depending on the statementIds of their policies. And since the policy is not a stateful resource, updating the policy should not harm anyone. I have checked the feasibility of this PR on my own, hence the lack of an integ test.

closes aws#28520 

----

*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
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2 pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-events: EventBus and EventBusPolicy only accept a single statement
6 participants