Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

(kinesisfirehose): DeliveryStream creates useless role #26927

Closed
peterwoodworth opened this issue Aug 29, 2023 · 5 comments · Fixed by #26930
Closed

(kinesisfirehose): DeliveryStream creates useless role #26927

peterwoodworth opened this issue Aug 29, 2023 · 5 comments · Fixed by #26930
Assignees
Labels
@aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@peterwoodworth
Copy link
Contributor

peterwoodworth commented Aug 29, 2023

Describe the bug

The DeliveryStream construct always creates a role

const role = props.role ?? new iam.Role(this, 'Service Role', {

However, this role ends up with no permissions, and no reason to exist if no source stream or encryption key are used:

  "DeliveryStreamServiceRole0CF4E414": {
   "Type": "AWS::IAM::Role",
   "Properties": {
    "AssumeRolePolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Principal": {
        "Service": "firehose.amazonaws.com"
       }
      }
     ],
     "Version": "2012-10-17"
    }
   },
   "Metadata": {
    "aws:cdk:path": "EipFailTagsStack/DeliveryStream/Service Role/Resource"
   }
  },

Expected Behavior

I expect no role to be created

Current Behavior

An unnecessary role is created

Reproduction Steps

Create a DeliveryStream without an encryptionKey, encryption, or sourceStream prop.

    new firehose.DeliveryStream(this, 'DeliveryStream', { 
      destinations: [new destinations.S3Bucket(new s3.Bucket(this, 'Bucket'))]
    });

Possible Solution

In this case, will need to check for appropriate props before creating the role

Additional Information/Context

No response

CDK CLI Version

current

Framework Version

No response

Node.js Version

16

OS

mac

Language

Typescript

Language Version

No response

Other information

Can workaround for now with the following escape hatch:

stream.node.tryRemoveChild('Service Role')
@peterwoodworth peterwoodworth added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. good first issue Related to contributions. See CONTRIBUTING.md p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Aug 29, 2023
@github-actions github-actions bot added the @aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose label Aug 29, 2023
@msambol
Copy link
Contributor

msambol commented Aug 30, 2023

@peterwoodworth I'll take this.

@peterwoodworth
Copy link
Contributor Author

Thanks!

@SimardeepSingh-zsh
Copy link

const deliveryStream = new firehose.DeliveryStream(this, 'DeliveryStream', {
destinations: [new destinations.S3Bucket(new s3.Bucket(this, 'Bucket'))],
});

// Check if encryptionKey, encryption, or sourceStream props are specified
if (!deliveryStream.encryptionKey && !deliveryStream.encryption && !deliveryStream.sourceStream) {
// If none of these props are specified, do not create the IAM role
deliveryStream.node.tryRemoveChild('ServiceRole');
}

@mergify mergify bot closed this as completed in #26930 Sep 5, 2023
mergify bot pushed a commit that referenced this issue Sep 5, 2023
)

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*
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

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

mikewrighton pushed a commit that referenced this issue Sep 14, 2023
)

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*
@rehanvdm
Copy link

rehanvdm commented Jan 3, 2024

This introduces a new error for existing components: Error: There is already a Construct with name 'Service Role' in DeliveryStream

It is because the role is being created again here while it was already created in the constructor here. The solution for now is to just specify the role explicitly, like:

    const deliveryStream = new kdfAlpha.DeliveryStream(this, idPrefix + 'Firehose', {
      deliveryStreamName,
      destinations: [destinationBucket],
      role: new iam.Role(this, 'Service Role', {
        assumedBy: new iam.ServicePrincipal('firehose.amazonaws.com'),
      })
    })

This workaround works for the current logic in the component. Just commenting for when others also experience it. Don't think it is high priority since no one has picked it up. Just happens that we do the logic paths that create the role twice.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
@aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants