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.

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

aws-cdk-kinesis: Implement Kinesis Stream L2 construct. #86

Merged
merged 4 commits into from
Jun 18, 2018

Conversation

sam-goodwin
Copy link
Contributor

Implement a L2 construct for a Kinesis stream:

  • Supports unencrypted and KMS encrypted streams
  • User can provide a KMS key or have one generated
  • User can grant read, write and read/write permissions which defines a policy with permissions to the Stream and Key (if required)
  • Streams can be exported and imported by an Arn token.

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

…or generated/user-supplied KMS encryption keys, and import/export across stacks
@sam-goodwin sam-goodwin requested a review from eladb June 12, 2018 09:21
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks 🥇 - minor fixes and merge!

*.d.ts
node_modules
dist
tsconfig.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add tslint.json (new)

assert(stream.encryptionKey === myKmsKey);
```

### Importing and Exporting Streams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be redundant to describe this idiom in every README.

@Doug-AWS can you create a doc topic on "importing and exporting constructs across stacks" and have that content there?

);
}

private grant(identity: IIdentityResource, streamActions: string[], keyActions: string[]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To improve the readability at the call site, I would bundle these into an interface:

grant(identity: IIdentityResource, actions: { streamActions: string[], keyActions: string[] }) {
  // ...
}

Then, the call site will look like this:

grant(role, { 
  streamActions: [ ... ], 
  keyActions: [ ... ]
});

/**
* An arbitrary set of tags (key–value pairs) to associate with the Kinesis stream.
*/
tags?: Array<Token | Tag> | Token;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove for now, we will introduce tags across the board (#91)

this.streamName = this.stream.ref;
this.encryptionKey = encryptionKey;

if (props.streamName) { this.addMetadata('aws:cdk:hasPhysicalName', props.streamName); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#92

@eladb
Copy link
Contributor

eladb commented Jun 13, 2018

Streams should also implement IEventRuleTarget so they can be used as a target for CloudWatch event rules.

Take a look at the events framework.

@sam-goodwin
Copy link
Contributor Author

From what I can understand, IEventRuleTarget is required so events can be routed to the kinesis stream. Having a StreamRef implement the interface means the IAM Role and Partition Key JSON path would have to be the same for all events using this stream as a target. Perhaps we need a method to create the IEventRuleTarget from the StreamRef instead of having the stream implement it by default? stream.consumeEvent(..) ..

I also need to add support for AWS::Lambda::EventSourceMapping so it's easy to attach a function to it. Would this belong in the stream package or lambda? stream.attachFunction(..) vs lambda.streamEventSource(streamRef, ..))

@eladb
Copy link
Contributor

eladb commented Jun 17, 2018

From what I can understand, IEventRuleTarget is required so events can be routed to the kinesis stream. Having a StreamRef implement the interface means the IAM Role and Partition Key JSON path would have to be the same for all events using this stream as a target. Perhaps we need a method to create the IEventRuleTarget from the StreamRef instead of having the stream implement it by default? stream.consumeEvent(..) ..

That's interesting and sounds like a the design of our CWE programming model. The ability to include a specifier when adding a target sounds like something we should add to the framework and not special-case for Kinesis. Let's punt that to a subsequent PR and discuss over as issue. Can you open one?

I also need to add support for AWS::Lambda::EventSourceMapping so it's easy to attach a function to it. Would this belong in the stream package or lambda? stream.attachFunction(..) vs lambda.streamEventSource(streamRef, ..))

It should probably be layered. Lambda should have a polymorphic method (i.e. anything that implements ILambdaEventSource):

lambda.addEventSource(source)

And then, StreamRef should have:

stream.attachToLambda(lambda) // or streamToLambda or invokeLambda, use the same terminology as the one in Kinesis docs?

Which will Do The Right Thing:

  1. Call addEventSource
  2. Any permissions are needed as well, or the event source is sufficient?

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, looks awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants