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

High level construct for an AppSync Elasticsearch data source #6063

Closed
1 of 2 tasks
bartcallant opened this issue Feb 2, 2020 · 16 comments · Fixed by #14651
Closed
1 of 2 tasks

High level construct for an AppSync Elasticsearch data source #6063

bartcallant opened this issue Feb 2, 2020 · 16 comments · Fixed by #14651
Labels
@aws-cdk/aws-appsync Related to AWS AppSync @aws-cdk/aws-elasticsearch Related to Amazon Elasticsearch Service effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@bartcallant
Copy link

bartcallant commented Feb 2, 2020

While working on an application I needed to setup an AppSync API which accesses Elasticsearch.

Use Case

Currently this has to be done through the CfnXyz construct. Implementing a higher level construct would simplify the usage of Elasticsearch as a data source.

Proposed Solution

A simple class to create an Elasticsearch data source would do. Implementation could be done similar to the existing DynamoDbDataSource and LambdaDataSource.

Implementation could be as simple as this:

/**
 * Properties for an AppSync Elasticsearch data source
 */
export interface ElasticsearchDataSourceProps extends BaseDataSourceProps {
    /**
     * Region for the Amazon Elasticsearch Service domain
     */
    readonly region: string;
    /**
     * Endpoint for the Amazon Elasticsearch Service domain
     */
    readonly endpoint: string;
}

/**
 * An AppSync data source backed by Elasticsearch
 */
export class ElasticsearchDataSource extends BaseDataSource {
    constructor(scope: Construct, id: string, props: ElasticsearchDataSourceProps) {
        super(scope, id, props, {
            type: 'AMAZON_ELASTICSEARCH',
            elasticsearchConfig: {
                awsRegion: props.region,
                endpoint: props.endpoint
            }
        });
    }
}

Other

There was a PR which already added a base class for a data source> This makes it easy to add a new resolver.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@bartcallant bartcallant added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Feb 2, 2020
@hoegertn
Copy link
Contributor

hoegertn commented Feb 2, 2020

This looks great. Just go ahead and open a PR with your code. Please also add some basic tests for the new data source.

@bartcallant
Copy link
Author

bartcallant commented Feb 2, 2020

I've extended the integration test. There I noticed some mapping templates are also included in the code. So I've looked into the console and listed them below here:

Request mapping template

Get document by id
```vtl
#**
    Use the 'path' to get a single value by id.
    You must replace the <index> and <type> placeholder with actual values.
*#
{
    "version": "2017-02-28",
    "operation": "GET",
    "path": "/<index>/<type>/${context.arguments.id}",
    "params": {}
}
```
Simple term query
```vtl
#**
    The 'params' key accepts any valid Elasticsearch DSL expression.
    You must replace the <index> and <type> placeholder with actual values.
*#
{
    "version":"2017-02-28",
    "operation":"GET",
    "path":"/<index>/<type>/_search",
    "params":{
        "body": {
            "from": 0,
            "size": 50,
            "query": {
                "term" :{
                    "<field>":"${context.arguments.<field>}"
                }
            }
        }
    }
}
```
Paginate with fixed-size pages
```vtl
#**
    The 'params' key accepts any valid Elasticsearch DSL expression.
    You must replace the <index> and <type> placeholder with actual values.
*#
{
    "version": "2017-02-28",
    "operation": "GET",
    "path": "/<index>/<type>/_search",
    "params": {
        "body": {
            "from": ${context.arguments.from},
            "size": ${context.arguments.size}
        }
    }
}
```
Get all documents within a 20 mile radius
```vtl
#**
    The 'params' key accepts any valid Elasticsearch DSL expression.
    You must replace the <index> and <type> placeholder with actual values.
*#
{
    "version":"2017-02-28",
    "operation":"GET",
    "path":"/<index>/<type>/_search",
    "params":{
        "body": {
            "query": {
              "filtered" : {
                  "query" : {
                      "match_all" : {}
                  },
                  "filter" : {
                      "geo_distance" : {
                          "distance" : "20mi",
                          "location" : {
                              "lat" : 47.6205,
                              "lon" : 122.3493
                          }
                      }
                  }
              }
            }
        }
    }
}
```

Response mapping templates

Return single document's source
```vtl
#**
    $context.result contains the full response of the Elasticsearch query.
    You can use this template to return the data from an Elasticsearch
    query that returns a single result.
*#
$util.toJson($context.result.get("_source"))
```
Return a list of document sources
```
#**
    $context.result contains the full response of the Elasticsearch query.
    Select a subset of information or iterate through hits to return the
    same shape as is expected by this field.
*#
[
    #foreach($entry in $context.result.hits.hits)
        ## $velocityCount starts at 1 and increments with the #foreach loop **
        #if( $velocityCount > 1 ) , #end
        $util.toJson($entry.get("_source"))
    #end
]
```

Which of these should I include? The response templates seem good to include. For the request templates, only Get all documents within a 20 mile radius seems quite to specific to include. What do you think?

Changes can already be viewed here

@SomayaB SomayaB added the @aws-cdk/aws-elasticsearch Related to Amazon Elasticsearch Service label Feb 4, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Feb 4, 2020
@SomayaB SomayaB added the @aws-cdk/aws-appsync Related to AWS AppSync label Feb 4, 2020
@SomayaB SomayaB assigned MrArnoldPalmer and unassigned iliapolo Feb 4, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Feb 4, 2020
@MrArnoldPalmer MrArnoldPalmer added the effort/medium Medium work item – several days of effort label Feb 12, 2020
@BryanPan342 BryanPan342 removed the in-progress This issue is being actively worked on. label Jul 21, 2020
@adamdottv
Copy link
Contributor

@bartcallant @BryanPan342 where does this effort stand? Can I be helpful in getting it across the finish line?

@BryanPan342
Copy link
Contributor

@adamelmore This PR has a couple of comments to how to expand on this PR to bring it home. Mainly just adding an interface in @aws-cdk/aws-elasticsearch for abstraction for the addElasticsearchDataSource function. Similiar to the addLambdaDataSource and addDynamoDbDataSource with their IFunction and ITable params.

@adamdottv
Copy link
Contributor

@BryanPan342 if I wanted to pick up the baton, would it be kosher for me to open a new PR carrying the author's work forward and addressing the review feedback in #6108? I have a need for this and would love to help out if I won't be stepping on any toes.

@BryanPan342
Copy link
Contributor

@adamelmore yeah go for it :)

@adamdottv
Copy link
Contributor

FYI, going to wait on the L2 construct for Elasticsearch which is currently in progress. Will quickly follow-up with a corresponding AppSync data source PR once that's merged.

@cyberwombat
Copy link

Hi @adamelmore - looks like the L2 may be ready? Would love this ES data source!

@adamdottv
Copy link
Contributor

Hi @adamelmore - looks like the L2 may be ready? Would love this ES data source!

Thanks for the reminder! Will get back to this shortly.

@MrArnoldPalmer MrArnoldPalmer added p2 and removed p1 labels Nov 24, 2020
@tsykora-verimatrix
Copy link

Any ETA on the L2 ElasticSearch data source availability, please? We've lost days trying to go the CfnDataSource / CfnResolver route, but that seems to have its own issues (type_name lookup issues). Thank you.

@ignaciolarranaga
Copy link

ignaciolarranaga commented Jan 4, 2021

Guys, make it work with the following (based on the original example):

import { Construct, Fn } from '@aws-cdk/core';
import { GraphqlApi as GraphqlApiOriginal, BaseDataSourceProps, BackedDataSource } from '@aws-cdk/aws-appsync';
import { Domain } from '@aws-cdk/aws-elasticsearch';

/**
* An AppSync data source backed by Elasticsearch
*/
class ElasticSearchDataSource extends BackedDataSource {
  constructor(scope: Construct, id: string, domain: Domain, props: BaseDataSourceProps) {
    super(scope, id, props, {
      type: 'AMAZON_ELASTICSEARCH',
      elasticsearchConfig: {
        awsRegion: Fn.select(3, Fn.split(':', domain.domainArn)),
        endpoint: `https://${domain.domainEndpoint}`,
      }
    });

    // @ts-ignore
    domain.grantReadWrite(this);
  }
}

export class GraphqlApi extends GraphqlApiOriginal {
  addElasticSearchDataSource(id: string, domain: Domain) {
    return new ElasticSearchDataSource(this, id, domain, { api: this });
  }
}

Hope it helps.

@tombuckley91
Copy link

@adamelmore - Hi Adam, did you get anywhere with this? There's currently no one assigned to this so it seems like it's gone stale. Thanks for your work on this anyway 👍

@adamdottv
Copy link
Contributor

@adamelmore - Hi Adam, did you get anywhere with this? There's currently no one assigned to this so it seems like it's gone stale. Thanks for your work on this anyway 👍

Sorry all, I fell off the wagon here after wrapping up the Elasticsearch L2 work! With my current priorities, I don't see me getting to this soon, unfortunately.

@tombuckley91
Copy link

@adamelmore - That's too bad, but thanks for the reply, and good luck with your current work!

@BryanPan342
Copy link
Contributor

I created a PR for this issue :) #14651

I'm not an expert Elasticsearch user, so I would love to get a pass from people who use it.

@mergify mergify bot closed this as completed in #14651 May 13, 2021
mergify bot pushed a commit that referenced this issue May 13, 2021
Implement support for elasticsearch data source.

Endpoint is fixed to `https` URL as through testing, CloudFormations requires the domain to be in the form of `https://[...]`.

Fixes #6063

----

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

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
Implement support for elasticsearch data source.

Endpoint is fixed to `https` URL as through testing, CloudFormations requires the domain to be in the form of `https://[...]`.

Fixes aws#6063

----

*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
@aws-cdk/aws-appsync Related to AWS AppSync @aws-cdk/aws-elasticsearch Related to Amazon Elasticsearch Service effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.