feat(redshift): add initial L2 Redshift construct #5730

bweigel · 2020-01-09T12:09:19Z

Initial commit to support Redshift as an L2 construct. This introduces the RedshiftCluster construct. It is by and large copy-pasted from @aws-cdk/aws-rds and adheres to the same functionality.

Purposeful Design Desicions

no non-VPC clusters (a cluster will only be launched in a VPC; no L2 construct for AWS::Redshift::ClusterSecurityGroup or AWS::Redshift::ClusterSecurityGroupIngress)
cluster can only be launched in private Subnets
nodes will be encrypted
cluster will not be public and won't have an elastic IP
HSM cannot be configured

Checklist

Implementation on L2 construct for Redshift
initial Unit tests
additional Unit tests to satisfy coverage requirements
Documentation in README.md

This PR closes #5711

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

- adds documentation

aws-cdk-automation · 2020-01-09T12:22:20Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-09T12:27:35Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

skinny85

Thanks so much for the contribution @bweigel ! I don't have time for a full review at this exact moment, but one thing stood out for me.

packages/@aws-cdk/aws-redshift/README.md

Pull request has been modified.

aws-cdk-automation · 2020-01-10T02:55:30Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-10T03:02:24Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-10T11:00:58Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

bweigel · 2020-01-11T06:58:05Z

@skinny85 I think I am done for now and will wait back for some feedback. One thing I need to mention is that I could not achieve a high enough coverage for the automatically generated ./lib/redshift.generated.ts:

So for now I have lowered the coverage requirements for this file.
Is there a better way to achieve coverage in this? Does this need to be tested directly?
Cheers.

aws-cdk-automation · 2020-01-11T07:29:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-13T07:17:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-13T07:39:16Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85 · 2020-01-13T16:19:06Z

One thing I need to mention is that I could not achieve a high enough coverage for the automatically generated ./lib/redshift.generated.ts:
So for now I have lowered the coverage requirements for this file.
Is there a better way to achieve coverage in this? Does this need to be tested directly?

This probably means your current proposal does not cover the entire surface area of Redshift (which is roughly what ./lib/redshift.generated.ts represents). That's totally fine though; while eventually, we want the library to have all capabilities that the L1 code has, there's absolutely no requirement to do it in one PR, so your solution of changing the coverage requirements is fine for now 🙂.

bweigel · 2020-01-21T09:06:00Z

Any update here? 😬

skinny85 · 2020-01-21T16:48:25Z

Any update here? 😬

I haven't forgotten you, just been super swamped. I will try my best to get to the review this week!

skinny85

This looks great @bweigel ! First batch of comments (second should be coming soon, but GitHub is a little flaky, and I don't want to lose the comments I've already written).

packages/@aws-cdk/aws-redshift/README.md

packages/@aws-cdk/aws-redshift/lib/cluster-ref.ts

packages/@aws-cdk/aws-redshift/lib/cluster.ts

skinny85 · 2020-01-22T01:22:21Z

packages/@aws-cdk/aws-redshift/lib/cluster.ts

+     *
+     * @default - A role is automatically created for you
+     */
+    readonly iamRoles?: IRole[];


What's the meaning of passing multiple roles here? I'm genuinely asking (I've only ever seen a single role passed to resources that need it).

We usually skip the service name prefix. So this should just be roles.

What's the meaning of passing multiple roles here? I'm genuinely asking (I've only ever seen a single role passed to resources that need it).

I thought it would model the way how Redshift does roles (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-redshift-cluster.html#cfn-redshift-cluster-iamroles). You can add up to 10 roles and use them when doing queries.

The alternative would be some kind of method that allows you to attach additional roles.
What is your take on that?

Thinking about your question it could also be reasonable to just @default to no role at all. However I think it be a nicer dev experience if one was provided from the start. 🤔

Maybe I don't understand how Redshift works. Can you explain:

What does it mean to have more than one role associated with a cluster?

What doe it mean to have 0 roles associated with the cluster?

Roles are a means for Redshift to access other AWS services (like S3, Glue or DynamoDB) when using the UNLOAD or COPY commands (https://docs.aws.amazon.com/redshift/latest/mgmt/authorizing-redshift-service.html). During UNLOAD or COPY you can pass an IAM role associated with the cluster for authorization like this:

copy catdemo from 's3://awssampledbuswest2/tickit/category_pipe.txt' iam_role 'arn:aws:iam::<aws-account-id>:role/<role-name>' region 'us-west-2';

from https://docs.aws.amazon.com/redshift/latest/dg/r_COPY.html

You could have two roles attached to the cluster. One allowing access to S3, one allowing access to DynamoDB for example.

Redshift cannot access other AWS services

packages/@aws-cdk/aws-redshift/lib/cluster.ts

skinny85

Second batch of comments :)

packages/@aws-cdk/aws-redshift/lib/cluster.ts

packages/@aws-cdk/aws-redshift/lib/parameter-group.ts

packages/@aws-cdk/aws-redshift/lib/props.ts

skinny85

Thanks for your patience @bweigel !

I still don't see a reason we should have family on ClusterParameterGroupProps if there is a single value allowed there... but I won't hold the PR any longer, as it's really great otherwise, and you've shown a lot of perseverance already 🙂. I'll just remove it as a follow-up ;).

mergify · 2020-05-20T21:27:51Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-20T21:37:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 492da13
Result: FAILED
Build Logs (available for 30 days)

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

skinny85 · 2020-05-20T21:42:49Z

@bweigel can you rebase / merge with latest master? The way to handle Jest changed in the meantime (we no longer use "jest" in package.json, the configuration is held in a local jest.config.js that allows you to create exclusions like this:

aws-cdk/packages/@aws-cdk/assert/jest.config.js

Lines 6 to 7 in 0567a23

    
           statements: 75, 
        
           branches: 65,

)

Right now, the build is failing with:

@aws-cdk/aws-redshift: Jest: "global" coverage threshold for branches (80%) not met: 79.69%

Maybe add one more test to get that coverage through that (tiny) hump...?

Also, while you're at it, please please remove the family property from ClusterParameterGroupProps 😃

Pull request has been modified.

aws-cdk-automation · 2020-05-26T23:48:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 7c00c58
Result: FAILED
Build Logs (available for 30 days)

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

Pull request has been modified.

aws-cdk-automation · 2020-05-27T00:19:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4dcb710
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-05-27T00:44:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4dcb710
Result: FAILED
Build Logs (available for 30 days)

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

skinny85 · 2020-05-27T01:33:43Z

I did all of the changes I mentioned in #5730 (comment) , but now our PR build is broken 😭.

Pull request has been modified.

mergify · 2020-05-27T20:20:48Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-27T20:20:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 6dee681
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-05-27T21:13:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1e298b0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-05-27T21:58:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8e2364c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-05-27T21:58:32Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

bweigel · 2020-07-02T13:29:14Z

I lost track of this for a while.
So glad you put in the work on the final steps @skinny85 . Thanks 👍

skinny85 · 2020-07-02T17:11:59Z

No worries, thank you for the contribution 🙂

bweigel added 2 commits January 9, 2020 12:55

initial commit that builds

f9be652

adds initial tests

539a003

bweigel requested a review from skinny85 as a code owner January 9, 2020 12:09

updates README.md

fe799a2

- adds documentation

SomayaB assigned skinny85 Jan 9, 2020

skinny85 previously requested changes Jan 9, 2020

View reviewed changes

packages/@aws-cdk/aws-redshift/README.md Outdated Show resolved Hide resolved

make naming more consistent

1e29ede

Merge branch 'master' into redshift_l2

a30cb81

fix linter ignores

f3e4dee

bweigel added 3 commits January 11, 2020 07:51

add some more tests

d7bf1cd

lower coverage requirements for ./lib/redshift.generated.ts

6d859e0

updastes README.md

036e602

bweigel and others added 3 commits January 13, 2020 07:39

updates README.md

d0fe63f

add option to disable encryption

3a8acc3

Update README.md

ebeeb11

skinny85 suggested changes Jan 22, 2020

View reviewed changes

skinny85 previously requested changes Jan 22, 2020

View reviewed changes

skinny85 previously approved these changes May 20, 2020

View reviewed changes

Merge branch 'master' into redshift_l2

492da13

Merge branch 'master' into redshift_l2

7c00c58

Remove familyGroup from ClusterParameterGroupProps.

4dcb710

skinny85 previously approved these changes May 26, 2020

View reviewed changes

Merge branch 'master' into redshift_l2

6dee681

skinny85 approved these changes May 27, 2020

View reviewed changes

skinny85 changed the title ~~feat(redshift): add L2 Redshift construct~~ feat(redshift): add initial L2 Redshift construct May 27, 2020

mergify bot added 2 commits May 27, 2020 20:21

Merge branch 'master' into redshift_l2

1e298b0

Merge branch 'master' into redshift_l2

8e2364c

mergify bot merged commit 703f0fa into aws:master May 27, 2020

skinny85 mentioned this pull request May 29, 2020

[Python] aws_redshift.CfnCluster association with subnetworks failed #8225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(redshift): add initial L2 Redshift construct #5730

feat(redshift): add initial L2 Redshift construct #5730

bweigel commented Jan 9, 2020 •

edited

Loading

aws-cdk-automation commented Jan 9, 2020

aws-cdk-automation commented Jan 9, 2020

skinny85 left a comment

aws-cdk-automation commented Jan 10, 2020

aws-cdk-automation commented Jan 10, 2020

aws-cdk-automation commented Jan 10, 2020

bweigel commented Jan 11, 2020

aws-cdk-automation commented Jan 11, 2020

aws-cdk-automation commented Jan 13, 2020

aws-cdk-automation commented Jan 13, 2020

skinny85 commented Jan 13, 2020

bweigel commented Jan 21, 2020

skinny85 commented Jan 21, 2020

skinny85 left a comment

skinny85 Jan 22, 2020

bweigel Jan 22, 2020

skinny85 Jan 22, 2020

bweigel Apr 12, 2020

skinny85 left a comment

skinny85 left a comment

mergify bot commented May 20, 2020

aws-cdk-automation commented May 20, 2020

skinny85 commented May 20, 2020 •

edited

Loading

aws-cdk-automation commented May 26, 2020

aws-cdk-automation commented May 27, 2020

aws-cdk-automation commented May 27, 2020

skinny85 commented May 27, 2020

mergify bot commented May 27, 2020

aws-cdk-automation commented May 27, 2020

aws-cdk-automation commented May 27, 2020

aws-cdk-automation commented May 27, 2020

mergify bot commented May 27, 2020

bweigel commented Jul 2, 2020

skinny85 commented Jul 2, 2020

feat(redshift): add initial L2 Redshift construct #5730

feat(redshift): add initial L2 Redshift construct #5730

Conversation

bweigel commented Jan 9, 2020 • edited Loading

aws-cdk-automation commented Jan 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 9, 2020

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 10, 2020

AWS CodeBuild CI Report

bweigel commented Jan 11, 2020

aws-cdk-automation commented Jan 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 13, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 13, 2020

AWS CodeBuild CI Report

skinny85 commented Jan 13, 2020

bweigel commented Jan 21, 2020

skinny85 commented Jan 21, 2020

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Jan 22, 2020

Choose a reason for hiding this comment

bweigel Jan 22, 2020

Choose a reason for hiding this comment

skinny85 Jan 22, 2020

Choose a reason for hiding this comment

bweigel Apr 12, 2020

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented May 20, 2020

aws-cdk-automation commented May 20, 2020

AWS CodeBuild CI Report

skinny85 commented May 20, 2020 • edited Loading

aws-cdk-automation commented May 26, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 27, 2020

AWS CodeBuild CI Report

skinny85 commented May 27, 2020

mergify bot commented May 27, 2020

aws-cdk-automation commented May 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 27, 2020

AWS CodeBuild CI Report

mergify bot commented May 27, 2020

bweigel commented Jul 2, 2020

skinny85 commented Jul 2, 2020

bweigel commented Jan 9, 2020 •

edited

Loading

skinny85 commented May 20, 2020 •

edited

Loading