aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.59k stars 3.89k forks source link

(events): ValidationError when creating an EventBus that has CrossAccount access. #22120

Closed sennyeya closed 1 year ago

sennyeya commented 2 years ago

Describe the bug

Basically, the id that's created for the support stack statementId is 5 characters too long (69 characters). It then fails validation and the stack cannot be deployed.

It looks like this has been reported before: https://github.com/aws/aws-cdk/issues/19941.

Expected Behavior

The stack deploys as expected.

Current Behavior

Error Message: Stack Deployments Failed: Error: The stack named ci-cd-EventBusPolicy-support-us-west-2-{account2} failed creation, it may need to be manually deleted from the AWS console: ROLLBACK_COMPLETE: 1 validation error detected: Value 'Allow-account-{account2}-c884c8876055cffba97afb1bc5a28125a7cac73762' at 'statementId' failed to satisfy constraint: Member must have length less than or equal to 64 (Service: AWSEvents; Status Code: 400; Error Code: ValidationException; Request ID: c521bf0c-abec-4042-90ed-54823e58a58e; Proxy: null)

Reproduction Steps

import { Stack, StackProps } from 'aws-cdk-lib';
import { IRepository } from 'aws-cdk-lib/aws-codecommit';
import {
  CodePipeline,
  CodePipelineSource,
  ShellStep,
} from 'aws-cdk-lib/pipelines';
import { Construct } from 'constructs';

export interface ReproStackProps extends StackProps {
  codeRepository: IRepository;
}

export class CodeStack extends Stack {
  public codeRepository: aws_codecommit.IRepository;
  constructor(scope: Construct, props: StackProps) {
    super(scope, 'CodeStack', props);
    this.codeRepository = aws_codecommit.Repository.fromRepositoryName(
      this,
      'code-repo',
      'repo',
    );
  }
}

export class ReproStack extends Stack {
  constructor(scope: Construct, props: ReproStackProps) {
    super(scope, 'repro', props);

    new CodePipeline(this, 'code-pipeline', {
      crossAccountKeys: true,
      synth: new ShellStep('synth', {
        input: CodePipelineSource.codeCommit(props.codeRepository, 'mainline'),
        commands: ['yarn install', 'yarn build'],
      }),
    });
  }
}
const app = new App();

const codeStack = new CodeStack(app, {
  env: {
    region: 'us-west-2',
    account: 'account1',
  },
});

new ReproStack(app, {
  env: {
    region: 'us-west-2',
    account: 'account2',
  },
  codeRepository: codeStack.codeRepository,
});

Possible Solution

When trying to upgrade past version 1.150 we ran into ValidationException while trying to create a EventBusPolicy cross account. This failure is probably happening because of the following line

statementId: Allow-account-${sourceAccount}-${this.node.addr},

It can be found here: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-events/lib/rule.ts#L400.

Additional Information/Context

No response

CDK CLI Version

2.41.0

Framework Version

No response

Node.js Version

v16.17.0

OS

MacOS Monterey 12.4

Language

Typescript

Language Version

Version 4.8.3

Other information

No response

peterwoodworth commented 2 years ago

Thanks for reporting this @ara6893,

That line you've pointed out seems to guarantee failure due to length https://github.com/aws/aws-cdk/blob/53fd0e670652a2c36e6c8c00e21b62825a3b3e72/packages/%40aws-cdk/aws-events/lib/rule.ts#L373

Allow-account- = 14 characters accountId = 12 characters - = 1 char this.node.addr = 42 characters

Adding these together results in 69 characters, which breaks the limit set by the service. We don't have any integration tests for this... We need to fix this bug and add integration tests

peterwoodworth commented 2 years ago

I am marking this issue as p2, which means that we are unable to work on this immediately.

We use +1s to help prioritize our work, and are happy to revaluate this issue based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for reprioritization.

Check out our contributing guide if you're interested in contributing yourself - there's a low chance the team will be able to address this soon but we'd be happy to review a PR 🙂

sennyeya commented 2 years ago

I'd be happy to take a stab at this. @peterwoodworth , do you have any pointers on direction for this? My first thought is to do a substring to hard cap the id to 64 characters (with .substring(0, 64)), but that might hit some issues with non-uniqueness.

peterwoodworth commented 2 years ago

We have precedent set elsewhere in our codebase to cap a hash or otherwise randomly generated ID for the sake of character limit. That solution should be fine!

github-actions[bot] commented 1 year ago

⚠️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.