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(consumption-opensearch): opensearch api as separate construct #694

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

alexvt-amz
Copy link
Contributor

Issue #, if available:

Description of changes:

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@vgkowski vgkowski changed the title fix: opensearch api as separate construct feat: opensearch api as separate construct Aug 1, 2024
@lmouhib lmouhib changed the title feat: opensearch api as separate construct feat(consumption-opensearch): opensearch api as separate construct Sep 10, 2024
@lmouhib lmouhib added the e2e-ready This PR is ready to be E2E-tested label Sep 10, 2024
@github-actions github-actions bot added e2e-testing This PR is currently being E2E-tested e2e-completed The E2E tests have completed on this PR and removed e2e-ready This PR is ready to be E2E-tested e2e-testing This PR is currently being E2E-tested labels Sep 10, 2024
@lmouhib lmouhib added e2e-ready This PR is ready to be E2E-tested and removed e2e-completed The E2E tests have completed on this PR labels Sep 14, 2024
@github-actions github-actions bot added e2e-testing This PR is currently being E2E-tested e2e-completed The E2E tests have completed on this PR and removed e2e-ready This PR is ready to be E2E-tested e2e-testing This PR is currently being E2E-tested labels Sep 14, 2024
Copy link
Contributor

@lmouhib lmouhib left a comment

Choose a reason for hiding this comment

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

Need a fix for license headers

Copy link
Contributor

@lmouhib lmouhib left a comment

Choose a reason for hiding this comment

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

few things to change in the documentation.

@@ -8,23 +8,22 @@ class ExampleOpenSearchApiStack extends cdk.Stack {
constructor(scope: Construct, id: string , props:cdk.StackProps) {

super(scope, id, props);
this.node.setContext('@data-solutions-framework-on-aws/removeDataOnDestroy', true);
/// !show
const osCluster = new dsf.consumption.OpenSearchCluster(this, 'MyOpenSearchCluster',{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to how the construct should be initialized, and not testing it through the openseach cluster construct. We have an example on kafka-api unit tests kafka-api.test.ts


The construct leverages the [CDK Provider Framework](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources-readme.html#provider-framework) to deploy a custom resource to manage, and provide `addRoleMapping` and `callOpenSearchApi` methods. Both methods return the custom resource so that allows to enforce sequental execution of the API calls. By default all API calls will be executed simultaneously and are independent of each other.

[example OpenSearch API](examples/opensearch-api.lit.ts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide a dummy example without initializing the actual OS cluster with construct, but provide how to initialize the OS-api construct.

[OpenSearch Roles API](https://opensearch.org/docs/2.13/security/access-control/api#create-role-mapping) does not allow to update individual roles, requiring to pass array of roles that needs to be applied.
To avoid overwriting prevously added roles `addRoleMapping` method provides `persist` parameter to store previously added roles inside the construct. To avoid racing conditions you also need to execute multiple `addRoleMapping` calls sequentionally as shown below.

```typescript
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide this through an example lit file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-completed The E2E tests have completed on this PR
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants