fix(kinesisfirehose): can't call grantPrincipal multiple times

[WinterYukky]

Issue # (if applicable)

Closes #.

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

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

(This review is outdated)

[WinterYukky]

Exemption Request

We know at the unit test level whether this bug has been resolved

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[ozelalisen]

Left some minor comments

[ozelalisen]

While the test is good, it could be more explicit about what's being tested. The test name indicates it's checking that multiple calls return the same instance, but the first assertion is actually checking that it doesn't throw an error. Consider splitting this into two separate tests or making the test name more explicit about both behaviors being tested

[ozelalisen]

The comment backwards compatibility doesn't fully explain the purpose of this change. It's not just for backwards compatibility but also to fix a bug where multiple calls to grantPrincipal would create multiple roles with the same name. Improve the comment to better explain the purpose. Maybe something like:
Create the role only once to ensure multiple calls to grantPrincipal return the same role instance, while maintaining backwards compatibility

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.