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

(aws-appsync): addRdsDataSource doesnt support taking a DatabaseCluster #29302

Closed
Labels
@aws-cdk/aws-appsync Related to AWS AppSync effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@antoniordz96
Copy link

Describe the bug

Aurora V2 Serverless now supports the Data API but from researching online the CDK recommended way to create v2 database is using the DatabaseCluster construct.

The method addRdsDataSource part of Appsync does not allow us to pass the DatabaseCluster as a datasource.

I think we should allow either DatabaseCluster or ServerlessCluster. Right now it seems only objects that implement Iserverlesscluster are allowed.

Expected Behavior

Allow me to pass the DatabaseCluster to the addRdsDataSource method

Current Behavior

We're gonna try to use the L1 construct instead CfnDataSource as that just requires database information thats available in both constructs

Reproduction Steps

this.dbCluster = new rds.DatabaseCluster(this, 'DbCluster', {
      engine: this.databaseClusterEngine,
      backup: {
        // TODO(delta-database): Do we have a recommended back retention?
        retention: Duration.days(this.props.backupRetentionDays),
        // TODO(delta-database): What time frame do we want
        preferredWindow: this.props.backupRetentionPreferredWindow,
      },
      // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbcluster.html#cfn-rds-dbcluster-enablecloudwatchlogsexports
      cloudwatchLogsExports: ['postgresql'],
      // TODO(delta-database): Do we have a recommended log retention?
      cloudwatchLogsRetention: RetentionDays.ONE_MONTH,
      clusterIdentifier: this.props.clusterIdentifier,
      copyTagsToSnapshot: true,
      deletionProtection: this.props.deletionProtection,
      iamAuthentication: true,
      instanceUpdateBehaviour: rds.InstanceUpdateBehaviour.ROLLING,
      monitoringInterval: Duration.seconds(60),
      parameterGroup: this.databaseClusterParameterGroup,
      port: 5432,
      // TODO(ccoe) preferredMaintenanceWindow determine value
      preferredMaintenanceWindow: this.props.preferredMaintenanceWindow,
      removalPolicy: RemovalPolicy.SNAPSHOT,
      securityGroups: this.props.securityGroups,
      serverlessV2MaxCapacity: this.props.serverlessV2MaxCapacity,
      serverlessV2MinCapacity: this.props.serverlessV2MinCapacity,
      storageEncrypted: true,
      storageEncryptionKey: this.props.encryptionKey,
      subnetGroup: this.databaseSubnetGroup,
      writer: rds.ClusterInstance.serverlessV2('writer'),
      vpc: this.props.vpc,
    });

    const auroraDataSource = appSyncApi.addRdsDataSource('AuroraDataSource', dbCluster);

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.130

Framework Version

No response

Node.js Version

v18.15.0

OS

MAC

Language

TypeScript

Language Version

No response

Other information

No response

@antoniordz96 antoniordz96 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Feb 28, 2024
@antoniordz96 antoniordz96 changed the title (aws-appsync): addRdsDataSource doesnt support taking a DabataCluster (aws-appsync): addRdsDataSource doesnt support taking a DatabaseCluster Feb 28, 2024
@pahud
Copy link
Contributor

pahud commented Feb 28, 2024

Yes we should support that. I'm making it a feature request and we welcome any PRs from the community.

addRdsDataSource(
id: string,
serverlessCluster: IServerlessCluster,

@pahud pahud added p2 feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@mergify mergify bot closed this as completed in #29544 Apr 10, 2024
mergify bot pushed a commit that referenced this issue Apr 10, 2024
### Issue # (if applicable)

(aws-appsync): addRdsDataSource doesnt support taking a DatabaseCluster 

Closes #29302

### Reason for this change

AppSync CDK construct currently accept only IServerlessCluster for RDS source as cluster type. However, with Aurora V2, serverless aurora clusters such as postgres aurora v14 and above are construct using the DatabaseCluster construct and as such AppSync.addRdsDataSource() method need ability to support both type of cluster interfaces.

### Description of changes

To support the change I created a second props to support IDatabaseCluster in addition to IServerlessCluster already supported and I overloaded the constructor to support both type of props.

However, to make the change possible, some modification to aws-rds were also required:

1 - Need IDatabaseCluster interface to have grantDataApiAccess() method published as part of the interface (the method was there but not published in the interface, when IServerlessCLuster interface have it published. 

2 - need DatabaseCluster.grantDataApiAccess() to follow the same IAM permission pattern than ServerlessCluster.grantDataApiAccess() to have consistency. 

ServerlessCluster.grantDataApiAccess() method is adding automatically the required permission to secret manager if Data API is enabled. 

However, DatabaseCluster.grantDataApiAccess() do not. As such without the change it will have been required for end users to add that IAM permission to secret manager as an extra line when they will have assigned a serverless V2 cluster to AppSync datasource. 

To keep the experience unified across the 2 type of RDS clusters, i updated the method to have that IAM permission embedded.

### Description of how you validated changes

- Unit test created for the new feature
- Unit test updated and validated for RDS changes
- Integration test created for the new feature

I run all unit test and the integration test successfully.


### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment