fix(firehose): remove unused role during DeliveryStream creation

[msambol]

When a DeliveryStream is created without sourceStream or encryptionKey,
an extra role is being created that is unused. This PR removes creation of that role.

I also learned that the role created for encryptionKey is used "indirectly" for a grant
put on the KMS key...interesting.

Closes #26927.

---

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

[mrgrain]

Hmm, this is kind of changing the contract for DeliveryStream.grantPrincipal. Yes technically it is still a principle, but now it might be unusable. (Although, grantPrincipal 🤨 why isn't this just called role - but that's a different story).

Can you change this.grantPrincipal to use a getter that will return the role if we already have one and only create the unused role when the property is accessed? That should give us the best of both worlds. What do you think?

[msambol]

Hmm, this is kind of changing the contract for DeliveryStream.grantPrincipal. Yes technically it is still a principle, but now it might be unusable. (Although, grantPrincipal 🤨 why isn't this just called role - but that's a different story).

Can you change this.grantPrincipal to use a getter that will return the role if we already have one and only create the unused role when the property is accessed? That should give us the best of both worlds. What do you think?

@mrgrain Thanks for the review! I'm totally up for making that change but wanted to point out one line of code: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts#L1274. There seems to be some precedent for this? I'm good either way, let me know what you'd prefer.

[mrgrain]

@mrgrain Thanks for the review! I'm totally up for making that change but wanted to point out one line of code: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts#L1274. There seems to be some precedent for this? I'm good either way, let me know what you'd prefer.

Yes there's precedent and if it were a new construct I'd probably lean that way. But because it is a change, I'd like us to comply with the previously established contract.

Pull request has been modified.

[msambol]

@mrgrain Updated per your feedback—thanks!

[msambol]

This is expected as I'm removing the unused role:

Stack: aws-cdk-firehose-delivery-stream-s3-all-properties - Resource: DeliveryStreamServiceRole964EEBCC - Impact: WILL_DESTROY

[msambol]

@mrgrain Good to ? Test failing because it's deleting a resource..

[mrgrain]

Good to ship, but we'll need the build passing. Can you run that integration test and update the snapshot?

Pull request has been modified.

[msambol]

@mrgrain I missed those other ones—apologies!

[mergify]

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]

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]

AWS CodeBuild CI Report

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

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

[mergify]

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).