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

feat(msk): Cluster L2 Construct #9908

Merged
merged 59 commits into from
May 10, 2021
Merged

Conversation

curtiseppel
Copy link

@curtiseppel curtiseppel commented Aug 22, 2020

L2 Construct for a MSK Cluster.

I wrote this for internal use and thought I'd share it. I tried to follow the example resource and design guidelines as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation.

Closes #9603


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

@gitpod-io
Copy link

gitpod-io bot commented Oct 25, 2020

@curtiseppel
Copy link
Author

@iliapolo I've updated this PR to support the latest Kafka version and to support MSK's new authentication method - SASL/SCRAM. I also added a bunch of tests and the build is now passing.

@iliapolo
Copy link
Contributor

iliapolo commented Oct 26, 2020

@curtiseppel Thats awesome, thanks! Apologies the review is lagging behind, its still on my radar though.

Copy link
Contributor

@iliapolo iliapolo left a comment

Choose a reason for hiding this comment

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

@curtiseppel I had several comments asking to flatten out configuration properties, in contrast to the CloudFormation structure. I just want to clarify the reasoning here.

Excessive nesting makes it hard to read and reason about, I know it allows for future proofing, but we normally don't do that. From prior experience, it's usually better to provide the best experience possible and employ deprecations in the future if necessary, than to sacrifice the ergonomics of the current API for some future unknown capabilities.

Thanks for this PR!

packages/@aws-cdk/aws-msk/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster.ts Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster-props.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk/test/cluster.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk/test/msk.test.ts Show resolved Hide resolved
packages/@aws-cdk/aws-msk/lib/cluster.ts Show resolved Hide resolved
@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 9, 2021
Unnecessary expansion of an object that is already of the expected type.

Co-authored-by: Eli Polonsky <Eli.polonsky@gmail.com>
@mergify mergify bot dismissed iliapolo’s stale review January 10, 2021 03:56

Pull request has been modified.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 10, 2021
Co-authored-by: Eli Polonsky <Eli.polonsky@gmail.com>
@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 4, 2021
@mergify mergify bot dismissed iliapolo’s stale review May 5, 2021 05:38

Pull request has been modified.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 5, 2021
iliapolo
iliapolo previously approved these changes May 5, 2021
Copy link
Contributor

@iliapolo iliapolo left a comment

Choose a reason for hiding this comment

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

@curtiseppel

Awesome, thanks for all the hard work and sticking with this PR for so long :)

@mergify
Copy link
Contributor

mergify bot commented May 5, 2021

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

@iliapolo
Copy link
Contributor

iliapolo commented May 5, 2021

@curtiseppel Looks like there are some build failures after rebasing from master. Have a look?

@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 6, 2021
@mergify mergify bot dismissed iliapolo’s stale review May 9, 2021 23:46

Pull request has been modified.

@curtiseppel
Copy link
Author

@curtiseppel

Awesome, thanks for all the hard work and sticking with this PR for so long :)

Thanks for all of the useful feedback 🎉

@iliapolo The snapshot test was failing due to lambda assets changing in the custom resource package. I've updated it now.

@curtiseppel curtiseppel requested a review from iliapolo May 9, 2021 23:47
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 10, 2021
@mergify
Copy link
Contributor

mergify bot commented May 10, 2021

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: ae9c3d1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit ce119ba into aws:master May 10, 2021
@mergify
Copy link
Contributor

mergify bot commented May 10, 2021

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

john-tipper pushed a commit to john-tipper/aws-cdk that referenced this pull request May 10, 2021
L2 Construct for a MSK Cluster. 

I wrote this for internal use and thought I'd share it. I tried to follow the [example resource](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/example-construct-library/lib/example-resource.ts) and [design guidelines](https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md) as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation.

Closes aws#9603

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
eladb pushed a commit that referenced this pull request May 12, 2021
* chore(cloudfront): remove the use of calculateFunctionHash (#14583)

`calculateFunctionHash()` was used to compute the 'refresh token' of the
custom resource for the EdgeFunction construct.

This method is private to the lambda module and is deemed to be changed.
Instead, use the lambda function version's logical id.

The logical id of the version includes computing the function hash (among
others) and is a more reliable determinant of whether the underlying
function version changed.


----

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

* feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) (#14365)

As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name

* feat(appmesh): change HealthChecks to use protocol-specific union-like classes (#14432)

BREAKING CHANGE: HealthChecks require use of static factory methods

fixes #11640

----

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

* feat(msk): Cluster L2 Construct (#9908)

L2 Construct for a MSK Cluster. 

I wrote this for internal use and thought I'd share it. I tried to follow the [example resource](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/example-construct-library/lib/example-resource.ts) and [design guidelines](https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md) as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation.

Closes #9603

----

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

* feat(kms): allow specifying key spec and key usage (#14478)

This allows specifying key spec and key usage, so you can create asymmetric keys.

closes #5639


----

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

* chore: add `@types/jest` to a package that was missing it (#14609)

----

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

* chore: issue template for bugs to have an SSCCE example (#14615)

Ask customers to provide reproducible code snippets
to reduce the amount of triage time required.

----

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

* fix(cli): synth fails if there was an error when synthesizing the stack (#14613)

All stacks created inside a pipeline stage will be flagged for validation. After synth is done, the CLI will validate all flagged stacks plus the stacks that were explicitly specified.

----

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

* chore: annotate `aws-lambda-go` with `docker` requirement (#14618)

The `nozem` build tool needs to know that `docker` is required to
build/test this particular package.


----

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

* feat(elbv2): preserveClientIp for NetworkTargetGroup (#14589)

Allows users to configure client IP preservation for network target groups.

See: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-targetgroup-targetgroupattribute.html

----

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

* chore: npm-check-updates && yarn upgrade (#14620)

Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date.

* chore(mergify): add @BenChaimberg to team roster

* chore(release): 1.103.0

* chore: mark "otaviomacedo" as core contributor (#14619)

Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com>

* chore(cli): add npm command to upgrade notice (#14621)

A colleague had to go look up the command to update the CDK CLI. It occurred to me that is probably common with developers who don't work with NPM on a daily basis, such as anyone who isn't developing in TypeScript or JavaScript.

Put the necessary command right in the upgrade notice.

----

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

* fix(lambda): custom resource fails to connect to efs filesystem (#14431)

Fixes: #14430

----

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

* feat(cfnspec): cloudformation spec v35.2.0 (#14610)

* feat: cloudformation spec v35.2.0

* add spec patches

Co-authored-by: AWS CDK Team <aws-cdk@amazon.com>
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* fix(lambda-nodejs): handler filename missing from error message (#14564)


----

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

* chore(build): clarify prerequisites in CONTRIBUTING and verify before build (#14642)

The build script at the top level of the repository performs a prerequisites check before beginning the build. This verification is not complete as it does not include the required checks for .NET and Python executables. Further, the prerequisites documentation in the contributing guide does not note the Docker requirement.

----

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

Co-authored-by: Niranjan Jayakar <nija@amazon.com>
Co-authored-by: OksanaH <34384274+OksanaH@users.noreply.github.com>
Co-authored-by: Dominic Fezzie <fezzid@amazon.com>
Co-authored-by: Curtis <curtis.eppel@cultureamp.com>
Co-authored-by: Shinya Tsuda <shinya@dacci.org>
Co-authored-by: Rico Huijbers <rix0rrr@gmail.com>
Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com>
Co-authored-by: Griffin Byatt <byatt.griffin@gmail.com>
Co-authored-by: Ben Chaimberg <chaimber@amazon.com>
Co-authored-by: AWS CDK Team <aws-cdk@amazon.com>
Co-authored-by: Mitchell Valine <valinm@amazon.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Jerry Kindall <52084730+Jerry-AWS@users.noreply.github.com>
Co-authored-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com>
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
L2 Construct for a MSK Cluster. 

I wrote this for internal use and thought I'd share it. I tried to follow the [example resource](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/example-construct-library/lib/example-resource.ts) and [design guidelines](https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md) as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation.

Closes aws#9603

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

[aws-msk] L2 construct for Cluster
4 participants