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(elasticsearch): L2 for ElasticsearchDomain #8369

Merged
merged 46 commits into from
Sep 30, 2020

Conversation

stephanh
Copy link
Contributor

@stephanh stephanh commented Jun 4, 2020

I was using cdk to stand up an Elasticsearch domain and thought I might as well generalise the effort.

Can I please get some feedback on the current implementation and if it is something you are happy to accept?

This implementation is still missing tests, more documentation and features like importing the resource. I will add them once I know I'm on the right track.


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

@stephanh stephanh marked this pull request as ready for review June 4, 2020 10:15
@iliapolo
Copy link
Contributor

Hi @stephanh - Thanks for submitting this!

It looks good, definitely a good starting point. Before you continue though, i'd like to ask two things:

  1. Create a README file for the common patterns and usage. This will give us a good window into the API ergonomics.

  2. Please add a comment on the PR detailing two or three benefits this API offers over the naive Cfn* resources. For example the automatic creation of the LogGroupResourcePolicy, and easy access to metrics. This will serve us for posterity and also justify the effort taking place here. P.S if you can somehow incorporate this into the README itself, that would be even better.

Looking forward to it :)

@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 22, 2020
@stephanh
Copy link
Contributor Author

I have updated the readme to provide more usage information (I haven't implemented the IAM permission management part yet).

Advantages this will provide over CfnDomain:

  • Nicer ergonomics for configuration. For example:
    • nodeToNodeEncription: true vs nodeToNodeEncryption: { enabled: true }
    • Setting enabled to true for ebsOptions if ebsConfiguration has been supplied.
    • Using a struct for log configuration instead of a hashtable.
  • Automatically creating log groups with the right permissions (requires custom resource) for any of the three supported log types if requested.
  • Helper for accessing most common metrics and other ES metrics by name. (Is there a way to avoid having to supply the account id to get the metric?)
  • Helper for granting IAM permissions to the cluster and indecies (grant methods). I still need to add those.

I think having the fundamentals in place will also make it easier for others to incrementally add improvements later.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 23, 2020
@iliapolo iliapolo marked this pull request as draft June 30, 2020 16:20
@iliapolo iliapolo changed the title DRAFT -- feat(elasticsearch): Add l2 cdk construct for Elasticsearch domain feat(elasticsearch): L2 for ElasticsearchDomain Jun 30, 2020
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.

First round. I think we are on the right track but there are still some API changes i'm thinking about towards the next round.

packages/@aws-cdk/aws-elasticsearch/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated 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 Jul 8, 2020
@mergify mergify bot dismissed iliapolo’s stale review July 9, 2020 00:08

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 Jul 9, 2020
@iliapolo iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 9, 2020
@iliapolo
Copy link
Contributor

@stephanh See #8920.

Lets add a README section about this?

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 11, 2020
@stephanh stephanh force-pushed the elasticsearch branch 3 times, most recently from 575ef34 to 593c588 Compare July 19, 2020 07:36
@stephanh
Copy link
Contributor Author

stephanh commented Jul 19, 2020

I have added some grant helper methods.

I think the remaining 2 items are:

  • tests
  • fromXXX methods to import existing domains.

@stephanh stephanh marked this pull request as ready for review August 2, 2020 02:56
@stephanh
Copy link
Contributor Author

stephanh commented Aug 2, 2020

@iliapolo thanks to @adamelmore major contribution I think we now have everything and this PR is ready.

@iliapolo
Copy link
Contributor

iliapolo commented Aug 3, 2020

@stephanh Awesome, love the cooperation :)

I'll take a look this week.

@adamdottv
Copy link
Contributor

@iliapolo will you have a chance to review this week?

@iliapolo
Copy link
Contributor

Hi @adamelmore - Its definitely on my docket for this week, Appreciate the patience.

@hoegertn
Copy link
Contributor

With AdvancedSecurityOptions being in CFN now it would be great to add them here too. What do you think?

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.

@stephanh Very cool, lots of potential here.

I did have quite a few comments on the API, specifically around the decision to make things required and not providing default values. Let me know if i'm missing some consideration you made.

packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticsearch/lib/domain.ts Outdated 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 Aug 15, 2020
@mergify mergify bot dismissed iliapolo’s stale review September 30, 2020 08:11

Pull request has been modified.

@stephanh stephanh requested a review from iliapolo September 30, 2020 08:13
* Use custom resource to updated domain access policy after the domain creation
* Updated readme
* Added integration test for basic unsigned auth
iliapolo
iliapolo previously approved these changes Sep 30, 2020
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.

@stephanh Took the liberty to apply a couple of minor changes and approved it.

Thanks so much for all the work!

@mergify mergify bot dismissed iliapolo’s stale review September 30, 2020 18:35

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Sep 30, 2020

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: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: acf0089
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Sep 30, 2020

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

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.

7 participants