feat(rds): add grant method for Data API

[asterikx]

This PR adds a grantDataApi method to IServerlessCluster to grant access to the Data API.

The "minimum required permissions" to access the Data API are listed here.

This PR further restricts the IAM policy statement to the specific cluster (in favor of wildcarding).

Read access to the cluster secret must be granted separately via the secrets grantRead method.

TBH, the secretmanager actions included in the two IAM policy statements in the official documentation. are rather confusing to me:

• I don't know why the resource name of the resource listed in "SecretsManagerDbCredentialsAccess" statement has a rds-db-credentials prefix. That prefix is not present in
• I don't know what the secretmanager actions in the "RDSDataServiceAccess" statement are for

closes #10744

BREAKING CHANGE: Serverless cluster enableHttpEndpoint renamed to enableDataApi

---

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

[njlynch]

Thanks for the PR! Mostly looks good, a few points to improve.

[asterikx]

Should we check for enableHttpEndpoint inside the grant method? This will only be possible for newly created clusters, not for imported ones

aws-cdk/packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts

Lines 90 to 98 in f9afbc5

	/**
	* Whether to enable the HTTP endpoint for an Aurora Serverless database cluster.
	* The HTTP endpoint must be explicitly enabled to enable the Data API.
	*
	* @see https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/data-api.html
	*
	* @default false
	*/
	readonly enableHttpEndpoint?: boolean;

Renaming that property to sth. more expressive like enableDataApi might be worth considering too

[njlynch]

Should we check for enableHttpEndpoint inside the grant method? That will only be possible for newly created clusters, not for imported ones

Yes, great idea! We do the same thing for enableIamAuthentication and grantConnect on instances:

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Lines 158 to 163 in 60c782f

	public grantConnect(grantee: iam.IGrantable): iam.Grant {
	if (this.enableIamAuthentication === false) {
	throw new Error('Cannot grant connect when IAM authentication is disabled');
	}
	this.enableIamAuthentication = true;

[asterikx]

Thanks a lot.

I think the code you linked is actually flawed.

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Lines 359 to 365 in 60c782f

	/**
	* Whether to enable mapping of AWS Identity and Access Management (IAM) accounts
	* to database accounts.
	*
	* @default false
	*/
	readonly iamAuthentication?: boolean;

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Line 661 in 60c782f

this.enableIamAuthentication = props.iamAuthentication;

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Line 141 in 60c782f

protected abstract enableIamAuthentication?: boolean;

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Lines 158 to 163 in 60c782f

	public grantConnect(grantee: iam.IGrantable): iam.Grant {
	if (this.enableIamAuthentication === false) {
	throw new Error('Cannot grant connect when IAM authentication is disabled');
	}
	this.enableIamAuthentication = true;

If the user does not specify iamAuthentication in DatabaseInstanceNewProps (or specifies undefined), the this.enableIamAuthentication === false will not catch that IAM authentication is not enabled.

I run into this the same issue when testing this logic for my implementation in the serverless construct.

[asterikx]

@njlynch could you take the final decision for the method's name? :)

Also:

Renaming that property to sth. more expressive like enableDataApi might be worth considering too

[njlynch]

I think the code you linked is actually flawed.
If the user does not specify iamAuthentication in DatabaseInstanceNewProps (or specifies undefined), the this.enableIamAuthentication === false will not catch that IAM authentication is not enabled.

Right, but then (on 163), it will enable it. The logic behind this is that if you haven't explicitly specified anything about IAM authentication, and then grant a principal connect access, you likely wanted IAM auth enabled. The error only throws when a user has explicitly disabled IAM auth, and then tries to grant connect access.

could you take the final decision for the method's name? :)

All else being equal, I think I like grantDataApiAccess the best.

Renaming that property to sth. more expressive like enableDataApi might be worth considering too.

I agree; it seems the only place this is labeled the httpEndpoint is in the CloudFormation docs; the RDS user guide always seems to reference it as the Data API. enableDataApi is fine. We'll need to update the PR description to include this as a breaking change. Add something like this to the PR description:

BREAKING CHANGE: Serverless cluster enableHttpEndpoint renamed to enableDataApi.

[gitpod-io]

[shivlaks]

I agree; it seems the only place this is labeled the httpEndpoint is in the CloudFormation docs; the RDS user guide always seems to reference it as the Data API. enableDataApi is fine. We'll need to update the PR description to include this as a breaking change. Add something like this to the PR description:

BREAKING CHANGE: Serverless cluster enableHttpEndpoint renamed to enableDataApi.

It's also enable-http-endpoint in the CLI for modifying a database cluster which is documented as:

When enabled, the HTTP endpoint provides a connectionless web service API for running SQL queries on the Aurora Serverless DB cluster. You can also query your database from inside the RDS console with the query editor.

having said that, I agree that it's more intent based to expose the property as enableDataApi.

aside: can the http endpoint be enabled in regions where the data api is not available? or are they truly synonymous?

[shivlaks]

@asterikx looks pretty good. commented on a few of the open threads initiated by @njlynch

it might also be handy to update/ add an integration test with some validation steps to verify the behaviour.

[asterikx]

aside: can the http endpoint be enabled in regions where the data api is not available? or are they truly synonymous?

Not sure. Also, I don't know if the Query Editor uses the Data API or uses the HTTP endpoint in some other way :/

Pull request has been modified.

[njlynch]

Looks great! Just a few test failures to address (see Build Log for failures).

[njlynch]

Thanks for fixing those tests up. Everything looks good!

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

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 5ab5ef7
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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