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
feat(rds): make VPC optional for serverless Clusters #17413
feat(rds): make VPC optional for serverless Clusters #17413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @CorentinDoue! In general looks great, just a few minor comments, mainly on the tests.
@@ -442,7 +454,7 @@ export class ServerlessCluster extends ServerlessClusterBase { | |||
databaseName: props.defaultDatabaseName, | |||
dbClusterIdentifier: clusterIdentifier, | |||
dbClusterParameterGroupName: clusterParameterGroupConfig?.parameterGroupName, | |||
dbSubnetGroupName: this.subnetGroup.subnetGroupName, | |||
dbSubnetGroupName: this.subnetGroup?.subnetGroupName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually legal to pass dbSubnetGroupName
without vpcSecurityGroupIds
? Seems kind of weird...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea, the CFN doc treats them independently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}); | ||
}); | ||
|
||
test('can\'t create a Serverless cluster without vpc but with imported vpc subnets', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem this could be handled by prettier
to avoid you loosing your time ;)
BTW, a unit test is currently failing:
|
I was using |
ce03d19
to
45687af
Compare
Pull request has been modified.
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @CorentinDoue! I've commented on the simple trick below that allows you to assert the absence of a value in the template.
@CorentinDoue what's the status here? Are you still planning to work on this, or should someone take this over? |
45687af
to
66e9b55
Compare
@skinny85 sorry I forgot about this PR. It should be ok now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, looks really good @CorentinDoue! I have a few minor last-minute comments, if you'll humor me 😉.
packages/@aws-cdk/aws-rds/README.md
Outdated
**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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Is this implying that when enableDataApi
is false
, a VPC is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the two notions are separated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we polish this wording then? Because that's what it's implying right now (at least that's how I read it).
@@ -106,11 +106,13 @@ export interface ServerlessClusterProps { | |||
|
|||
/** | |||
* The VPC that this Aurora Serverless cluster has been created in. | |||
* | |||
* @default - No VPC related construct will be created: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @default - No VPC related construct will be created: | |
* @default - no VPC will be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
* @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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation. I agree with your suggestion 🙂.
vpcSubnets: props.vpcSubnets, | ||
removalPolicy: props.removalPolicy === RemovalPolicy.RETAIN ? props.removalPolicy : undefined, | ||
}); | ||
let securityGroups: ec2.ISecurityGroup[] | undefined = props.securityGroups; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
eff911d
to
7da2955
Compare
Pull request has been modified.
7da2955
to
50813de
Compare
@skinny85 I rebased and treated the last comments :) |
50813de
to
86c0802
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @CorentinDoue! A few small documentation tweaks, and we're good to go.
packages/@aws-cdk/aws-rds/README.md
Outdated
@@ -613,4 +610,7 @@ cluster.grantDataApiAccess(fn); | |||
|
|||
**Note**: To invoke the Data API, the resource will need to read the secret associated with the cluster. | |||
|
|||
**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. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
After #18366 is merged, updating from |
77d67b3
to
6868d6d
Compare
@skinny85 I found some time to work on this PR. It should be ok now. |
Looks like our backwards compatibility checker doesn't like some of the changes:
I guess since they are Would you mind making these required again? Simply returning an empty list should be good enough. |
6868d6d
to
c8cb3f7
Compare
@skinny85 fixed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @CorentinDoue! A few tiny last comments, and we'll get this merged in!
const cluster = new rds.ServerlessCluster(this, 'AnotherCluster', { | ||
engine: rds.DatabaseClusterEngine.AURORA_MYSQL, | ||
vpc, | ||
enableDataApi: true, // Optional - will be automatically set if you call grantDataApiAccess() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert these changes. It's enough to mention that vpc
is optional below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
c8cb3f7
to
8215643
Compare
@skinny85 it should be ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CorentinDoue!
Pull request has been modified.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Fixes #17401
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license