fix(aws-appsync): use serverlessCluster on rdsDataSource

[monholm]

Resolves: #12567

BREAKING CHANGE: RdsDataSource now takes a ServerlessCluster instead of a DatabaseCluster

---

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

[gitpod-io]

[monholm]

The failed build doesn't seem to have anything to do with the changed code. It exits with: "toomanyrequests: Rate exceeded" - not really sure what to make of that :)

[MrArnoldPalmer]

Is it not possible to use AppSync with a normal IDatabaseCluster? I'm wondering if we need to support both IServerlessCluster and IDataBaseCluster. Reading docs I haven't found anything clear on this front.

[monholm]

Hi @MrArnoldPalmer

AppSync interacts with the Data API which is only available on the serverless clusters, not the provisioned ones.

https://docs.aws.amazon.com/appsync/latest/devguide/tutorial-rds-resolvers.html
https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/data-api.html

[MrArnoldPalmer]

Side note, if you check the allow changes from upstream option on this PR I can merge in master and re-run the checks. The issues with throttling in the build have been mostly fixed. It also will allow mergify to do this automatically before merge.

[monholm]

@MrArnoldPalmer I might be blind, but where do I find this checkbox?

[MrArnoldPalmer]

Oh I guess its "Allow edits from maintainers". Check out https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

[monholm]

Thank you, I will have a look. In the mean time, I merged from master manually.

[monholm]

Ahh, our fork is located in our organization, not under my personal user account. I think this is why I'm not seeing the checkbox. Will you be able to do with this for now?

[MrArnoldPalmer]

yeah its fine, it just may require some babysitting when approved as mergify wont be able to auto-update it before merging.

So I'm running some tests locally based off the examples in the docs. The RDS one looks like this and deploys alright.

export class AppsyncRdsclusterStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const vpc = new ec2.Vpc(this, 'Vpc', {
      natGateways: 1,
    });

    const username = 'clusteradmin';
    const databaseSecret = new rds.DatabaseSecret(this, 'AuroraSecret', {
      username,
    });

    const cluster = new rds.DatabaseCluster(this, 'AuroraCluster', {
      engine: rds.DatabaseClusterEngine.auroraMysql({ version: rds.AuroraMysqlEngineVersion.VER_2_09_1 }),
      credentials: { username },
      clusterIdentifier: 'appsync-rds',
      defaultDatabaseName: 'appsync',
      instanceProps: { vpc },
    });

      const api = new appsync.GraphqlApi(this, 'GraphqlAPI', {
        name: 'graphqlapi',
        schema: appsync.Schema.fromAsset(path.join(__dirname, '..', 'schema.graphql')),
      });

      api.addRdsDataSource('rds', cluster, databaseSecret);
  }
}

I'm still verifying that the api is reading from and writing to the DB correctly. If this is supported then we will need to create a base type shared by both IServerlessCluster and IDatabaseCluster that we can accept as arguments. Or add a different add XxxDataSource method for accepting a serverless cluster.

[monholm]

Let me know if you make that work, that would be really surprising to me. If it does, I will have to have another look at it.

[MrArnoldPalmer]

So I'm unable to get this example to work, queries/mutations don't return any helpful errors, addDemo doesn't error and returns a null value from demos. I've connected to the database manually via mysqlsh in order to create the initial table and monitor if any rows were being created, but there was nothing. So either we are messing something up internally in the constructs or you're definitely right about this. Pasting the full example for anyone who is interested. I'm gonna continue on this and confirm that using ServerlessCluster works. @BryanPan342 check this out when you get a chance and let us know if you have working examples with a DatabaseCluster backed data source.

Full example

Side note, setting the databaseName property using escape hatches is fixed in master

export class AppsyncRdsclusterStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const vpc = new ec2.Vpc(this, 'Vpc', {
      natGateways: 1,
    });

    const username = 'clusteradmin';
    const databaseName = 'appsync';

    const cluster = new rds.DatabaseCluster(this, 'AuroraCluster', {
      engine: rds.DatabaseClusterEngine.auroraMysql({ version: rds.AuroraMysqlEngineVersion.VER_2_09_1 }),
      credentials: { username },
      clusterIdentifier: 'appsync-rds',
      defaultDatabaseName: databaseName,
      instanceProps: { vpc },
    });

    const api = new appsync.GraphqlApi(this, 'GraphqlAPI', {
      name: 'graphqlapi',
      schema: appsync.Schema.fromAsset(path.join(__dirname, '..', 'schema.graphql')),
    });

    const ds = api.addRdsDataSource('rds', cluster, cluster.secret!);

    // Set up a resolver for an RDS query.
    ds.createResolver({
      typeName: 'Query',
      fieldName: 'getDemos',
      requestMappingTemplate: appsync.MappingTemplate.fromString(`
      {
        "version": "2018-05-29",
        "statements": [
          "SELECT * FROM demos"
        ]
      }
      `),
      responseMappingTemplate: appsync.MappingTemplate.fromString(`
        $util.rds.toJsonObject($ctx.result)
      `),
    });

    // Set up a resolver for an RDS mutation.
    ds.createResolver({
      typeName: 'Mutation',
      fieldName: 'addDemo',
      requestMappingTemplate: appsync.MappingTemplate.fromString(`
      {
        "version": "2018-05-29",
        "statements": [
          "INSERT INTO demos VALUES (:ID, :VERSION)",
          "SELECT * FROM demos WHERE id = :ID"
        ],
        "variableMap": {
          ":ID": $util.toJson($util.autoId()),
          ":VERSION": $util.toJson($ctx.args.input.version)
        }
      }
      `),
      responseMappingTemplate: appsync.MappingTemplate.fromString(`
        $utils.toJson($utils.rds.toJsonObject($ctx.result)[1][0])
      `),
    });

    const dataSourceCfn = ds.node.defaultChild as appsync.CfnDataSource;      

    // Escape hatching to set the databaseName
    // @ts-ignore
    dataSourceCfn.relationalDatabaseConfig.rdsHttpEndpointConfig.databaseName = databaseName;
  }
}

schema

type demo {
  id: String!
  version: String!
}
type Query {
  getDemos: [ demo! ]
}
input DemoInput {
  version: String!
}
type Mutation {
  addDemo(input: DemoInput!): demo
}

EDIT: There is an issue unrelated to ServerlessCluster vs DatabaseCluster in here. Still debugging.

[monholm]

If it's any help, this is the workaround we implemented when we encountered this:

export class ServerlessDataSource extends BackedDataSource {
  constructor(
    scope: Stack,
    id: string,
    props: {
      api: GraphqlApi;
      databaseCluster: ServerlessCluster;
      secretStore: ISecret;
      name?: string;
      databaseName?: string;
    },
  ) {
    super(scope, id, props, {
      type: 'RELATIONAL_DATABASE',
      relationalDatabaseConfig: {
        rdsHttpEndpointConfig: {
          awsRegion: props.databaseCluster.stack.region,
          dbClusterIdentifier: Lazy.string({
            produce: () => {
              return Stack.of(this).formatArn({
                service: 'rds',
                resource: `cluster:${props.databaseCluster.clusterIdentifier}`,
              });
            },
          }),
          awsSecretStoreArn: props.secretStore.secretArn,
          databaseName: props.databaseName,
        },
        relationalDatabaseSourceType: 'RDS_HTTP_ENDPOINT',
      },
    });
    props.secretStore.grantRead(this);
    const clusterArn = Stack.of(this).formatArn({
      service: 'rds',
      resource: `cluster:${props.databaseCluster.clusterIdentifier}`,
    });
    // Change to grant with RDS grant becomes implemented
    Grant.addToPrincipal({
      grantee: this,
      actions: [
        'rds-data:DeleteItems',
        'rds-data:ExecuteSql',
        'rds-data:ExecuteStatement',
        'rds-data:GetItems',
        'rds-data:InsertItems',
        'rds-data:UpdateItems',
      ],
      resourceArns: [clusterArn, `${clusterArn}:*`],
      scope: this,
    });
  }
}

And implementing the above:

const rdsDS = new ServerlessDataSource(props.scope, 'RDSDS', {
    api: props.api,
    databaseCluster: props.database,
    secretStore: props.database.secret!,
    name: 'RDSDS',
    databaseName: envs.projectName!.toLowerCase(),
  });

I can confirm that this is working, and that we have been using this approach for the last month.

[MrArnoldPalmer]

Yeah so you're definitely right. AppSync requires usage of the dataApi which only serverless aurora supports. I had to enable cloudwatch logging, which we should make easier to do in the construct as well, perhaps even the default. Additionally we should add some features like when calling addRdsDataSource we add the policy from ServerlessCluster.grantDataApiAccess to the data source role as you're doing in your workaround, and we should make sure the user is aware that enableDataApi needs to be true on the ServerlessCluster.

Added #13188 and #13189 for tracking some related future work that should reduce the need for more code in your workaround.

[mergify]

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

[monholm]

Hi @MrArnoldPalmer

I'm sorry if I just added to the confusion by posting the workaround above. I just wanted to post the exact code that we can confirm is working.

To be clear, the "ServerlessDataSource" is an exact copy of "RdsDataSource" with the exception of using ServerlessCluster instead of DatabaseCluster, and adding the databaseName. See

aws-cdk/packages/@aws-cdk/aws-appsync/lib/data-source.ts

Line 324 in a349bf1

export class RdsDataSource extends BackedDataSource {

So everything regarding policies etc. is unchanged.

Pull request has been modified.

[MrArnoldPalmer]

@Simon-TechForm not at all! I thought the policies you were granting in your workaround were the data api usage ones, but I was mistaken.

[monholm]

I had to resolve some merge conflicts, which caused your review to be dismissed. I will bring it up to date again, and then it should be able to be merged. (Next time I will make sure to create the fork under my personal account).

[mergify]

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

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[monholm]

@MrArnoldPalmer please checkout #13206.

When I created the fork, I had no idea it would cause these kind of problems creating it under an organization.

#13206 contains the exact same changes, and the "Allow edits from maintainers" flag is set, so you should be able to merge it without wasting your time...

[MrArnoldPalmer]

@Simon-TechForm, thanks for that. Just curious were you merging to master locally and then pushing or were you using the update from base branch in the github PR ui? I thought that using the button wouldn't dismiss reviews but I may be wrong.

Closing in favor of #13206

[monholm]

I used the GitHub UI for everything. I don't think the updates caused your review to be dismissed, but resolving merge conflicts definitely did.

The last update did however cause one of the checks to fail (Something along the lines of "rules not applying anymore" I think? I didn't really read much into it). So I decided to go ahead and create a new pr instead.