Skip to content

Commit ea226b5

Browse files
authored
fix(kinesisfirehose): can't call grantPrincipal multiple times (#34682)
### Issue # (if applicable) Closes #<issue number here>. ### Reason for this change DeliveryStream creates a new iam.Role every reference grantPrincipal, so we get the error `There is already a Construct with name 'Service Role' in DeliveryStream [Delivery Stream Multiple]`. This is easy to reproduce if you use deliveryStream with multiple `grantXXX` methods. This is test result before fix codes. ``` yarn test aws-kinesisfirehose/test/delivery-stream.test.ts -t "multiple calls to grantPrincipal should return the same instance of IAM role" yarn run v1.22.22 $ jest aws-kinesisfirehose/test/delivery-stream.test.ts -t 'multiple calls to grantPrincipal should return the same instance of IAM role' ts-jest[config] (WARN) The "ts-jest" config option "isolatedModules" is deprecated and will be removed in v30.0.0. Please use "isolatedModules: true" in /workspaces/aws-cdk/packages/aws-cdk-lib/tsconfig.json instead, see https://www.typescriptlang.org/tsconfig/#isolatedModules FAIL aws-kinesisfirehose/test/delivery-stream.test.ts delivery stream ✕ multiple calls to grantPrincipal should return the same instance of IAM role (45 ms) ○ other tests... ● delivery stream › multiple calls to grantPrincipal should return the same instance of IAM role expect(received).not.toThrow() Error name: "Error" Error message: "There is already a Construct with name 'Service Role' in DeliveryStream [Delivery Stream Multiple]" 155 | 156 | constructor(scope: Construct, id: string, props: ResourceProps = {}) { > 157 | super(scope, id); | ^ 158 | 159 | if ((props.account !== undefined || props.region !== undefined) && props.environmentFromArn !== undefined) { 160 | throw new ValidationError(`Supply at most one of 'account'/'region' (${props.account}/${props.region}) and 'environmentFromArn' (${props.environmentFromArn})`, this); at Node.addChild (../../node_modules/constructs/src/construct.ts:430:13) at new Node (../../node_modules/constructs/src/construct.ts:71:17) at new Construct (../../node_modules/constructs/src/construct.ts:482:17) at new Resource (core/lib/resource.ts:157:5) at new Role (aws-iam/lib/role.ts:472:5) at new Role (core/lib/prop-injectable.ts:36:7) at WrappedClass.get grantPrincipal [as grantPrincipal] (aws-kinesisfirehose/lib/delivery-stream.ts:320:26) at aws-kinesisfirehose/test/delivery-stream.test.ts:410:33 at Object.<anonymous> (../../node_modules/expect/build/toThrowMatchers.js:74:11) at Object.throwingMatcher [as toThrow] (../../node_modules/expect/build/index.js:320:21) at Object.<anonymous> (aws-kinesisfirehose/test/delivery-stream.test.ts:410:53) 408 | }); 409 | const principal = deliveryStream.grantPrincipal; > 410 | expect(() => deliveryStream.grantPrincipal).not.toThrow(); | ^ 411 | expect(deliveryStream.grantPrincipal).toBe(principal); 412 | }); 413 | at Object.<anonymous> (aws-kinesisfirehose/test/delivery-stream.test.ts:410:53) ``` ### Description of changes I changed grantPrincipal implementation from create a new iam.Role every reference to create a new iam.Role only at first reference. ### Describe any new or updated permissions being added None. ### Description of how you validated changes Create the new unit test. ### 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*
1 parent 3774f78 commit ea226b5

File tree

2 files changed

+12
-5
lines changed

2 files changed

+12
-5
lines changed

packages/aws-cdk-lib/aws-kinesisfirehose/lib/delivery-stream.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -311,13 +311,11 @@ export class DeliveryStream extends DeliveryStreamBase {
311311
private _role?: iam.IRole;
312312

313313
public get grantPrincipal(): iam.IPrincipal {
314-
if (this._role) {
315-
return this._role;
316-
}
317-
// backwards compatibility
318-
return new iam.Role(this, 'Service Role', {
314+
// backwards compatibility - create role only once
315+
this._role = this._role ?? new iam.Role(this, 'Service Role', {
319316
assumedBy: new iam.ServicePrincipal('firehose.amazonaws.com'),
320317
});
318+
return this._role;
321319
}
322320

323321
constructor(scope: Construct, id: string, props: DeliveryStreamProps) {

packages/aws-cdk-lib/aws-kinesisfirehose/test/delivery-stream.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,15 @@ describe('delivery stream', () => {
402402
expect(deliveryStream.grantPrincipal).toBeInstanceOf(iam.Role);
403403
});
404404

405+
test('multiple calls to grantPrincipal should return the same instance of IAM role', () => {
406+
const deliveryStream = new firehose.DeliveryStream(stack, 'Delivery Stream Multiple', {
407+
destination: mockS3Destination,
408+
});
409+
const principal = deliveryStream.grantPrincipal;
410+
expect(() => deliveryStream.grantPrincipal).not.toThrow();
411+
expect(deliveryStream.grantPrincipal).toBe(principal);
412+
});
413+
405414
describe('metric methods provide a Metric with configured and attached properties', () => {
406415
let deliveryStream: firehose.DeliveryStream;
407416

0 commit comments

Comments
 (0)