@aws-cdk-lib/aws-events: EventBus resource policy naming collision across stacks

wtrep commented 5 months ago

Describe the bug

Deploying two instances of the same stack (within the same account) containing an event bus with the same resource policy fails. The issue is that the StatementId field of the synthesized AWS::Events::EventBusPolicy resource must be unique across stacks. StatementId is internally populated from the sid of the provided iam.PolicyStatement:

https://github.com/aws/aws-cdk/blob/9a346647970ff006fa7695c2a43dd1109e13dbb5/packages/aws-cdk-lib/aws-events/lib/event-bus.ts#L336-L355

Expected Behavior

The sid is documented in the following manner:

The Sid (statement ID) is an optional identifier that you provide for the policy statement. You can assign a Sid value to each statement in a statement array. In services that let you specify an ID element, such as SQS and SNS, the Sid value is just a sub-ID of the policy document's ID. In IAM, the Sid value must be unique within a JSON policy.

I don't think it's reasonable to assume that end users should be aware that this value must be globally unique in this particular context. While the EventBridge Bus resource requires adding a sid for each statement of its resource policy, the service supports having two buses with the same policy containing the same sid.

To troubleshoot this issue, I had to read the underlying CDK source code to understand the underlying assumptions.

Current Behavior

addToResourcePolicy requires providing a sid. This value is used as the StatementId of the AWS::Events::EventBusPolicy resource which must be unique across stacks.

Reproduction Steps

Create the following stack:

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as events from 'aws-cdk-lib/aws-events';
import * as iam from 'aws-cdk-lib/aws-iam';

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

    const eventBus = new events.EventBus(this, 'EventBus');
    eventBus.addToResourcePolicy(
      new iam.PolicyStatement({
        sid: 'AllowTrustedAccountToPutEvents',
        actions: ['events:PutEvents'],
        principals: [new iam.AccountPrincipal(process.env.TRUSTED_ACCOUNT_ID)],
        resources: [eventBus.eventBusArn],
      }
    ));
  }
}

Deploy that stack twice:

import * as cdk from 'aws-cdk-lib';
import { EventBusStack } from '../lib/event-bus-stack';

const app = new cdk.App();
new EventBusStack(app, 'EventBusStack-12345', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION }
});
new EventBusStack(app, 'EventBusStack-67890', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION }
});

You'll get the following error message:

 ❌  EventBusStack-12345 failed: Error: The stack named EventBusStack-12345 failed creation, it may need to be manually deleted from the AWS console: ROLLBACK_COMPLETE: cdk-AllowTrustedAccountToPutEvents already exists in stack arn:aws:cloudformation:us-east-1:<REDACTED>:stack/EventBusStack-67890/<REDACTED>

Possible Solution

I think the CDK should follow the documented best practice:

A better approach is to specify as few names as possible. If you omit resource names, the AWS CDK will generate them for you in a way that won't cause problems. (Best practices for developing and deploying cloud infrastructure with the AWS CDK)

In the context of the addToResourcePolicy method in the EventBus class, it think the StatementId should be an autogenerated unique value for each policy statement. That autogenerated value could also be used as the underlying sid to respect the typing definition of the iam.PolicyStatement.

Additional Information/Context

Sidenote: the raw PutPermission API call doesn't require providing a StatementId when a raw JSON policy is provided via the Policy parameter while the CloudFormation resource requires a StatementId but supports providing a Statement (which has the same badly copy pasted documentation as the upstream Policy). This is incredibly confusing.

CDK CLI Version

2.133.0 (build dcc1e75)

Framework Version

No response

Node.js Version

v20.11.1

OS

macOS 14.4.1

Language

TypeScript

Language Version

5.3.3

Other information

No response

alexbaileymembr commented 5 months ago

👍 this just confused me for an hour or so.

    // Does not work
    eventBus.addToResourcePolicy(
      new PolicyStatement({
        effect: Effect.ALLOW,
        actions: ['events:PutEvents'],
        principals: [new AccountPrincipal('123456789012')],
        resources: ['*']
      })
    );

    // Works
    eventBus.addToResourcePolicy(
      new PolicyStatement({
        sid: 'AllowOtherAccountToPutEvents',
        effect: Effect.ALLOW,
        actions: ['events:PutEvents'],
        principals: [new AccountPrincipal('123456789012')],
        resources: ['*']
      })
    );

More confusing is the SID actually ends up with a prefix once created ("Sid": "cdk-AllowOtherAccountToPutEvents")!

khushail commented 3 months ago

Hi @wtrep , thanks for reporting this. I agree there is some confusion reg SID mentioned in the shared docs. Although it has been mentioned as a note that Each StatementId must be unique. there is some mismatch - https://docs.aws.amazon.com/eventbridge/latest/APIReference/API_PutPermission.html#API_PutPermission_RequestParameters

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-events-eventbuspolicy.html#aws-resource-events-eventbuspolicy-properties

Marking this as P2 which would mean team won't be able to immediately work on this but if you / community would like to contribute towards the resolution, team would be happy to review your PR!

aws / aws-cdk