Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

aws-events-targets: KinesisFirehoseStream not accepting IDeliveryStream for imported DeliveryStream #25451

Closed
pags opened this issue May 5, 2023 · 5 comments · Fixed by #30189
Closed

aws-events-targets: KinesisFirehoseStream not accepting IDeliveryStream for imported DeliveryStream #25451

pags opened this issue May 5, 2023 · 5 comments · Fixed by #30189
Labels
@aws-cdk/aws-events-targets bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@pags
Copy link

pags commented May 5, 2023 •
edited

Describe the bug

When trying to add a KinesisFirehoseStream target to an event Rule, an error is thrown out of the aws-iam module.

Expected Behavior

CDK will successfully create an event rule with a target of an existing Kinesis Firehose stream.

Current Behavior

An error is thrown:

util_1.sum(values.map(v => (cdk.Token.isUnresolved(v) ? tokenSize : v.length) + 3 /* quotes, separator */));

TypeError: Cannot read properties of undefined (reading 'length')
    at @aws-cdk/aws-iam/lib/policy-statement.js:551:91

Reproduction Steps

new events.Rule(this, 'rule', {
			eventPattern: {
				source: ['aws.s3'],
				detail: {
					eventName: ['PutObject'],
					requestParameters: {
						bucketName: [bucket.bucketName]
					}
				}
			}
		}).addTarget(new targets.KinesisFirehoseStream(firehose.DeliveryStream.fromDeliveryStreamArn(this, 'firehose', 'my arn here')));

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.77.0 (build 06a0b19)

Framework Version

No response

Node.js Version

v16.14.2

OS

Ubuntu

Language

Typescript

Language Version

No response

Other information

No response
@pags pags added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 5, 2023
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label May 5, 2023
@pahud pahud self-assigned this May 5, 2023
@pahud
Copy link
Contributor

pahud commented May 5, 2023

I believe this is because targets.KinesisFirehoseStream() is accepting CfnDeliveryStream as its parameter but fromDeliveryStreamArn actually returns IDeliveryStream instead.

image

I guess we probably need to allow KinesisFirehoseStream to accept the interface instead.

aws-cdk/packages/aws-cdk-lib/aws-events-targets/lib/kinesis-firehose-stream.ts

Line 26 in c3744ae

constructor(private readonly stream: firehose.CfnDeliveryStream, private readonly props: KinesisFirehoseStreamProps = {}) {

@pahud pahud added p1 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 May 5, 2023
@pahud pahud removed their assignment May 5, 2023
@pahud pahud added @aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose @aws-cdk/aws-events-targets and removed @aws-cdk/aws-iam Related to AWS Identity and Access Management labels May 5, 2023
@pahud pahud changed the title events: IAM Policy Error aws-events-targets: KinesisFirehoseStream not accepting IDeliveryStream for imported DeliveryStream May 5, 2023
@pags
Copy link
Author

pags commented May 5, 2023

Thank you for the insight, I am using JS (not TS) so did not have any type hints. Is there a prescribed way to get a CfnDeliveryStream instance based on what I have now?

@peterwoodworth
Copy link
Contributor

Assuming you've created a deliverystream like so:

const stream = new firehose.DeliveryStream(this, 'Delivery Stream', {
  ...
});

You can access the CfnDeliveryStream like so with escape hatches

const cfnStream = stream.node.defaultChild

@peterwoodworth
Copy link
Contributor

Sorry, you're importing it. If your DeliveryStream isn't managed by CloudFormation or CDK at all, you could cdk import the resource to be managed by your stack, and then the above comment could work. Else, if it's managed by CDK or another CloudFormation stack already and you want to keep it within that stack, I'm not sure there's a workaround here

@peterwoodworth peterwoodworth removed the @aws-cdk/aws-kinesisfirehose Related to Amazon Kinesis Data Firehose label May 5, 2023
@GavinZZ GavinZZ mentioned this issue May 13, 2024
Merged
1 task
@mergify mergify bot closed this as completed in #30189 May 16, 2024
mergify bot pushed a commit that referenced this issue May 16, 2024
@GavinZZ
fix(events_targets): kinesisfirehosestream not accepting ideliverystr…
d7e6a10 
…eam for imported deliverystream (#30189)

### Issue # (if applicable)

Closes #25451

### Reason for this change

Current events targets implementation only support L1 Delivery Stream as the input. We should support L2 IDeliveryStream as well for imported kinesis firehose stream.

### Description of changes

Add a V2 class to support kinesis firehose stream.

### Description of how you validated changes

New tests and existing tests pass.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
@aws-cdk/aws-events-targets bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Milestone
No milestone
3 participants
@pahud @pags @peterwoodworth