feat(rds): make VPC optional for serverless Clusters by CorentinDoue · Pull Request #17413 · aws/aws-cdk

CorentinDoue · 2021-11-09T09:35:26Z

Fixes #17401

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

gitpod-io · 2021-11-09T09:35:28Z

skinny85

Thanks for the contribution @CorentinDoue! In general looks great, just a few minor comments, mainly on the tests.

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

skinny85 · 2021-11-09T18:35:25Z

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

      dbClusterIdentifier: clusterIdentifier,
      dbClusterParameterGroupName: clusterParameterGroupConfig?.parameterGroupName,
-      dbSubnetGroupName: this.subnetGroup.subnetGroupName,
+      dbSubnetGroupName: this.subnetGroup?.subnetGroupName,


Is it actually legal to pass dbSubnetGroupName without vpcSecurityGroupIds? Seems kind of weird...

I have no idea, the CFN doc treats them independently

cf 66e9b55#r781963251

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

skinny85 · 2021-11-09T18:44:57Z

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

+    });
+  });
+
+  test('can\'t create a Serverless cluster without vpc but with imported vpc subnets', () => {


Suggested change

test('can\'t create a Serverless cluster without vpc but with imported vpc subnets', () => {

test("can't create a Serverless cluster without VPC but with imported VPC subnets", () => {

Please use the same casing in the other test descriptions.

idem this could be handled by prettier to avoid you loosing your time ;)

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

skinny85 · 2021-11-09T18:49:24Z

BTW, a unit test is currently failing:

@aws-cdk/aws-rds: FAIL test/serverless-cluster.test.js (14.717 s)
@aws-cdk/aws-rds:   â—� serverless cluster â€º can create a Serverless cluster without vpc but with imported subnet group
@aws-cdk/aws-rds:     None of 1 resources matches resource 'AWS::RDS::DBCluster' with {
@aws-cdk/aws-rds:       "$objectLike": {
@aws-cdk/aws-rds:         "Engine": "aurora-postgresql",
@aws-cdk/aws-rds:         "DBClusterParameterGroupName": "default.aurora-postgresql10",
@aws-cdk/aws-rds:         "EngineMode": "serverless",
@aws-cdk/aws-rds:         "DBSubnetGroupName": {
@aws-cdk/aws-rds:           "Ref": "SubnetGroup12345"
@aws-cdk/aws-rds:         },
@aws-cdk/aws-rds:         "MasterUsername": {
@aws-cdk/aws-rds:           "Fn::Join": [
@aws-cdk/aws-rds:             "",
@aws-cdk/aws-rds:             [
@aws-cdk/aws-rds:               "{{resolve:secretsmanager:",
@aws-cdk/aws-rds:               {
@aws-cdk/aws-rds:                 "Ref": "DatabaseSecret3B817195"
@aws-cdk/aws-rds:               },
@aws-cdk/aws-rds:               ":SecretString:username::}}"
@aws-cdk/aws-rds:             ]
@aws-cdk/aws-rds:           ]
@aws-cdk/aws-rds:         },
@aws-cdk/aws-rds:         "MasterUserPassword": {
@aws-cdk/aws-rds:           "Fn::Join": [
@aws-cdk/aws-rds:             "",
@aws-cdk/aws-rds:             [
@aws-cdk/aws-rds:               "{{resolve:secretsmanager:",
@aws-cdk/aws-rds:               {
@aws-cdk/aws-rds:                 "Ref": "DatabaseSecret3B817195"
@aws-cdk/aws-rds:               },
@aws-cdk/aws-rds:               ":SecretString:password::}}"
@aws-cdk/aws-rds:             ]
@aws-cdk/aws-rds:           ]
@aws-cdk/aws-rds:         },
@aws-cdk/aws-rds:         "VpcSecurityGroupIds": [
@aws-cdk/aws-rds:           "SecurityGroupId12345"
@aws-cdk/aws-rds:         ]
@aws-cdk/aws-rds:       }
@aws-cdk/aws-rds:     }.
@aws-cdk/aws-rds:     - Field DBSubnetGroupName mismatch: Object type mismatch, Field VpcSecurityGroupIds missing in:
@aws-cdk/aws-rds:         {
@aws-cdk/aws-rds:           "Type": "AWS::RDS::DBCluster",
@aws-cdk/aws-rds:           "Properties": {
@aws-cdk/aws-rds:             "Engine": "aurora-postgresql",
@aws-cdk/aws-rds:             "DBClusterParameterGroupName": "default.aurora-postgresql10",
@aws-cdk/aws-rds:             "DBSubnetGroupName": "SubnetGroupId12345",
@aws-cdk/aws-rds:             "EngineMode": "serverless",
@aws-cdk/aws-rds:             "MasterUsername": {
@aws-cdk/aws-rds:               "Fn::Join": [
@aws-cdk/aws-rds:                 "",
@aws-cdk/aws-rds:                 [
@aws-cdk/aws-rds:                   "{{resolve:secretsmanager:",
@aws-cdk/aws-rds:                   {
@aws-cdk/aws-rds:                     "Ref": "DatabaseSecret3B817195"
@aws-cdk/aws-rds:                   },
@aws-cdk/aws-rds:                   ":SecretString:username::}}"
@aws-cdk/aws-rds:                 ]
@aws-cdk/aws-rds:               ]
@aws-cdk/aws-rds:             },
@aws-cdk/aws-rds:             "MasterUserPassword": {
@aws-cdk/aws-rds:               "Fn::Join": [
@aws-cdk/aws-rds:                 "",
@aws-cdk/aws-rds:                 [
@aws-cdk/aws-rds:                   "{{resolve:secretsmanager:",
@aws-cdk/aws-rds:                   {
@aws-cdk/aws-rds:                     "Ref": "DatabaseSecret3B817195"
@aws-cdk/aws-rds:                   },
@aws-cdk/aws-rds:                   ":SecretString:password::}}"
@aws-cdk/aws-rds:                 ]
@aws-cdk/aws-rds:               ]
@aws-cdk/aws-rds:             },
@aws-cdk/aws-rds:             "StorageEncrypted": true
@aws-cdk/aws-rds:           },
@aws-cdk/aws-rds:           "UpdateReplacePolicy": "Snapshot",
@aws-cdk/aws-rds:           "DeletionPolicy": "Snapshot"
@aws-cdk/aws-rds:         }
@aws-cdk/aws-rds:       983 |
@aws-cdk/aws-rds:       984 |     // THEN
@aws-cdk/aws-rds:     > 985 |     expect(stack).toHaveResource('AWS::RDS::DBCluster', {
@aws-cdk/aws-rds:           |                   ^
@aws-cdk/aws-rds:       986 |       Engine: 'aurora-postgresql',
@aws-cdk/aws-rds:       987 |       DBClusterParameterGroupName: 'default.aurora-postgresql10',
@aws-cdk/aws-rds:       988 |       EngineMode: 'serverless',
@aws-cdk/aws-rds:       at Object.<anonymous> (test/serverless-cluster.test.ts:985:19)

CorentinDoue · 2021-11-10T09:12:03Z

BTW, a unit test is currently failing

I was using yarn test locally. I didn't realize it only test the js files. It's a little misleading ;)

Pull request has been modified.

CorentinDoue · 2021-11-10T09:35:32Z

@skinny85 You can check the changes in the fixup commit. I will fixup it in one commit when the PR will be ready to merge

skinny85

Looks great @CorentinDoue! I've commented on the simple trick below that allows you to assert the absence of a value in the template.

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

skinny85 · 2021-12-15T00:30:44Z

@CorentinDoue what's the status here? Are you still planning to work on this, or should someone take this over?

CorentinDoue · 2021-12-15T09:04:09Z

@skinny85 sorry I forgot about this PR. It should be ok now.

skinny85

In general, looks really good @CorentinDoue! I have a few minor last-minute comments, if you'll humor me 😉.

skinny85 · 2021-12-15T23:02:19Z

packages/@aws-cdk/aws-rds/README.md

+**Note**: Using the Data API, you can interact with a ServerlessCluster without using its VPC. Therefore, the parameter "vpc" is optional. 
+The cluster will be created in a VPC, but you will know nothing about it.


Hmm. Is this implying that when enableDataApi is false, a VPC is required?

No, the two notions are separated

Can we polish this wording then? Because that's what it's implying right now (at least that's how I read it).

skinny85 · 2021-12-15T23:02:42Z

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

  /**
   * The VPC that this Aurora Serverless cluster has been created in.
+   *
+   * @default - No VPC related construct will be created:


Suggested change

* @default - No VPC related construct will be created:

* @default - no VPC will be used

An RDS cluster can't be outside a VPC. So if no VPC is provided the cluster will be created in a VPC, with subnets and security groups but we will not manage them at all. That's what I expect when I use the Data API, I don't want to handle network configuration.
Here an example of Aurora Cluster created without VPC (directly with CfnDBCluster)

The created cluster is

You can see there is a VPC, 3 subnets, and a security group but I never provided them and I will never interact with them.

So I propose you the folowing description:

Suggested change

* @default - No VPC related construct will be created:

* @default - The cluster will be created in the default VPC

That's what I understand of the Cloud Formation doc

https://docs.aws.amazon.com/fr_fr/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbcluster.html#aws-resource-rds-dbcluster-properties

Thanks for the detailed explanation. I agree with your suggestion 🙂.

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

skinny85 · 2021-12-15T23:07:02Z

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

-      vpcSubnets: props.vpcSubnets,
-      removalPolicy: props.removalPolicy === RemovalPolicy.RETAIN ? props.removalPolicy : undefined,
-    });
+    let securityGroups: ec2.ISecurityGroup[] | undefined = props.securityGroups;


So here, if props.vpc is undefined, but props.securityGroups is not, we will actually set these Security Groups in the Cluster. Is that even correct? Should we check this, and error out, similarly like we do if props. vpcSubnets is set without props.vpc?

I think it's possible to pass some securityGroups linked to the default VPC without passing it explicitly. But I agree it's better to force the VPC to be provided to use them. I add an error.

packages/@aws-cdk/aws-rds/test/integ.serverless-cluster-no-vpc.ts

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

Pull request has been modified.

CorentinDoue · 2022-01-11T10:17:21Z

@skinny85 I rebased and treated the last comments :)

skinny85

Looks great @CorentinDoue! A few small documentation tweaks, and we're good to go.

skinny85 · 2022-01-11T20:21:54Z

packages/@aws-cdk/aws-rds/README.md


+**Note**: Using the Data API, you can interact with a ServerlessCluster without using its VPC. Therefore, the parameter "vpc" is optional. 
+The cluster will be created in a VPC, but you will know nothing about it.
+


Actually, let's leave this example as-is, and add a new sub-section, with the appropriate header level, talking about VPC being optional.

If optional VPC and enableDataApi are indeed separate, let's separate them in our documentation as well.

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

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

skinny85 · 2022-01-11T20:37:13Z

After #18366 is merged, updating from master should fix the build (it's a problem during a release).

CorentinDoue · 2022-02-18T17:59:54Z

@skinny85 I found some time to work on this PR. It should be ok now.

skinny85 · 2022-02-18T18:58:43Z

Looks like our backwards compatibility checker doesn't like some of the changes:

err  - PROP @aws-cdk/aws-rds.ServerlessCluster.securityGroups: type Optional<Array<@aws-cdk/aws-ec2.ISecurityGroup>> (formerly Array<@aws-cdk/aws-ec2.ISecurityGroup>): output type is now optional [changed-type:@aws-cdk/aws-rds.ServerlessCluster.securityGroups]
err  - PROP @aws-cdk/aws-rds.ServerlessClusterFromSnapshot.securityGroups: type Optional<Array<@aws-cdk/aws-ec2.ISecurityGroup>> (formerly Array<@aws-cdk/aws-ec2.ISecurityGroup>): output type is now optional [changed-type:@aws-cdk/aws-rds.ServerlessClusterFromSnapshot.securityGroups]

I guess since they are protected, we can't change them to be optional.

Would you mind making these required again? Simply returning an empty list should be good enough.

CorentinDoue · 2022-02-21T12:52:08Z

Looks like our backwards compatibility checker doesn't like some of the changes:
err  - PROP @aws-cdk/aws-rds.ServerlessCluster.securityGroups: type Optional<Array<@aws-cdk/aws-ec2.ISecurityGroup>> (formerly Array<@aws-cdk/aws-ec2.ISecurityGroup>): output type is now optional [changed-type:@aws-cdk/aws-rds.ServerlessCluster.securityGroups]
err  - PROP @aws-cdk/aws-rds.ServerlessClusterFromSnapshot.securityGroups: type Optional<Array<@aws-cdk/aws-ec2.ISecurityGroup>> (formerly Array<@aws-cdk/aws-ec2.ISecurityGroup>): output type is now optional [changed-type:@aws-cdk/aws-rds.ServerlessClusterFromSnapshot.securityGroups]
I guess since they are protected, we can't change them to be optional.

Would you mind making these required again? Simply returning an empty list should be good enough.

@skinny85 fixed :)

skinny85

Looks great @CorentinDoue! A few tiny last comments, and we'll get this merged in!

skinny85 · 2022-02-21T18:53:19Z

packages/@aws-cdk/aws-rds/README.md

 const cluster = new rds.ServerlessCluster(this, 'AnotherCluster', {
  engine: rds.DatabaseClusterEngine.AURORA_MYSQL,
-  vpc,
  enableDataApi: true, // Optional - will be automatically set if you call grantDataApiAccess()


Let's revert these changes. It's enough to mention that vpc is optional below.

The example shows the minimal configuration to enable the data API. As the vpc is now optional, I think it shouldn't be in the example

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

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

…rams are used

CorentinDoue · 2022-02-22T09:22:24Z

@skinny85 it should be ok

skinny85

Thanks @CorentinDoue!

Pull request has been modified.

mergify · 2022-02-22T20:10:03Z

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 · 2022-02-22T20:51:56Z

AWS CodeBuild CI Report

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

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

mergify · 2022-02-22T20:52:22Z

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

github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label Nov 9, 2021

github-actions bot assigned skinny85 Nov 9, 2021

skinny85 previously requested changes Nov 9, 2021

View reviewed changes

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from ce03d19 to 45687af Compare November 10, 2021 09:30

CorentinDoue requested a review from skinny85 November 10, 2021 09:30

skinny85 reviewed Nov 10, 2021

View reviewed changes

packages/@aws-cdk/aws-rds/test/serverless-cluster.test.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-rds/test/serverless-cluster.test.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-rds/test/serverless-cluster.test.ts Outdated Show resolved Hide resolved

skinny85 changed the title ~~feat(aws-rds): make serverless-cluster vpc optional~~ feat(rds): make VPC optional for serverless Clusters Nov 10, 2021

skinny85 removed their assignment Dec 15, 2021

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from 45687af to 66e9b55 Compare December 15, 2021 08:23

CorentinDoue requested a review from skinny85 December 15, 2021 09:04

skinny85 previously requested changes Dec 16, 2021

View reviewed changes

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from eff911d to 7da2955 Compare January 11, 2022 10:14

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from 7da2955 to 50813de Compare January 11, 2022 10:15

CorentinDoue requested a review from skinny85 January 11, 2022 10:17

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from 50813de to 86c0802 Compare January 11, 2022 12:47

skinny85 reviewed Jan 11, 2022

View reviewed changes

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch 2 times, most recently from 77d67b3 to 6868d6d Compare February 18, 2022 17:58

CorentinDoue requested a review from skinny85 February 18, 2022 18:00

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from 6868d6d to c8cb3f7 Compare February 21, 2022 12:51

skinny85 reviewed Feb 21, 2022

View reviewed changes

CorentinDoue added 4 commits February 22, 2022 09:32

style(aws-rds): preserve vertical space

92a417b

feat(aws-rds): make serverless-cluster vpc optional

b5ccea1

docs(aws-rds): update serverless cluster doc

36dff45

feat(aws-rds): make serverless-cluster vpc required if vpc related pa…

8215643

…rams are used

CorentinDoue force-pushed the feat/make-serverless-cluster-vpc-optional branch from c8cb3f7 to 8215643 Compare February 22, 2022 08:32

CorentinDoue requested a review from skinny85 February 22, 2022 09:22

Update README.md

f2cf096

skinny85 previously approved these changes Feb 22, 2022

View reviewed changes

skinny85 approved these changes Feb 22, 2022

View reviewed changes

Merge branch 'master' into feat/make-serverless-cluster-vpc-optional

bc8ec49

mergify bot merged commit 4f7818d into aws:master Feb 22, 2022

	test('can\'t create a Serverless cluster without vpc but with imported vpc subnets', () => {
	test("can't create a Serverless cluster without VPC but with imported VPC subnets", () => {

		Note: Using the Data API, you can interact with a ServerlessCluster without using its VPC. Therefore, the parameter "vpc" is optional.
		The cluster will be created in a VPC, but you will know nothing about it.

	* @default - No VPC related construct will be created:
	* @default - no VPC will be used

	* @default - No VPC related construct will be created:
	* @default - The cluster will be created in the default VPC

Conversation

CorentinDoue commented Nov 9, 2021 • edited by skinny85 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Nov 9, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Nov 9, 2021

Uh oh!

CorentinDoue commented Nov 10, 2021

Uh oh!

CorentinDoue commented Nov 10, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Dec 15, 2021

Uh oh!

CorentinDoue commented Dec 15, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CorentinDoue commented Jan 11, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CorentinDoue commented Nov 9, 2021 •

edited by skinny85

Loading