feat(rds): allow using existing security groups for new instance #4495

jogold · 2019-10-14T12:49:16Z

BREAKING CHANGE: securityGroup: ec2.ISecurityGroup is now securityGroups: ec2.ISecurityGroup[] in DatabaseInstanceAttributes, removed securityGroupId from IDatabaseInstance

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

Closes aws#2949

mergify · 2019-10-14T12:49:54Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-14T13:23:15Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85

Thanks for the contribution @jogold ! One comment.

skinny85 · 2019-10-14T15:06:00Z

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

+   *
+   * @default - a new security group is created
+   */
+  readonly securityGroup?: ec2.ISecurityGroup;


You can have an array of SecurityGroups for instance, right? So this should be readonly securityGroups: ec2.ISecurityGroup[], correct?

Agree but this is to be consistent with other packages/resources in the CDK, examples are ECS Service, RDS Cluster, ELBV2 Load balancers, Lambda functions, etc. All those resources accept an array of security groups but it's always implemented with a single security group.

The only exception is the AWS CodeBuild project.

I discussed this with the team, and we came to the conclusion that array is the way to go, and the other places that don't allow arrays are actually the ones that are wrong (and they will have to change, like ECS does in #3985 ).

So, please change it to readonly securityGroups: ISecurityGroup[].

Thanks!

Done. Note there's now a BREAKING CHANGE documented in the PR description, someone of the team will need to squash this PR because mergify won't take it into account.

Is the breaking change removing the securityGroupId property?

BREAKING CHANGE: securityGroup: ec2.ISecurityGroup is now securityGroups: ec2.ISecurityGroup[] in DatabaseInstanceAttributes, removed securityGroupId from IDatabaseInstance

I think I'm missing something then. Wasn't it possible to use existing security groups with Instance before this PR? When you could pass in a single security group?

Oh, I think I see it now. It's about allowing to use an existing security group for a new Instance, right? Before, it was only possible to pass it for imported Instances?

Yes, exactly

rix0rrr · 2019-10-17T11:51:27Z

Something unrelated to this PR, hope that's okay. @jogold I want to ask you something privately. To that end, I sent an email address to the email address you use to sign your GitHub commits (jonathan@***exchange.be). No rush, but in case you don't use that email address anymore, can you please shoot me a quick line from your current address at huijbers@amazon.com ?

Thanks a lot!

jogold · 2019-10-17T12:55:29Z

@rix0rrr just replied to your email 👍

Pull request has been modified.

aws-cdk-automation · 2019-10-18T19:02:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-18T19:03:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85 · 2019-10-22T19:45:40Z

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

@@ -935,7 +933,7 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements
    });

    this.connections = new ec2.Connections({
-      securityGroups: [this.securityGroup],
+      securityGroups: this.securityGroups,
      defaultPort: ec2.Port.tcp(this.instanceEndpoint.port)


I see this code:

this.connections = new ec2.Connections({ securityGroups: this.securityGroups, defaultPort: ec2.Port.tcp(this.instanceEndpoint.port) });

is repeated in DatabaseInstance, DatabaseInstanceFromSnapshot and DatabaseInstanceReadReplica. Is there any way we can reduce this duplication?

aws-cdk-automation · 2019-10-23T08:32:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85

One more teeny comment, if you'll bear with me :)

skinny85 · 2019-10-24T01:35:17Z

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


  protected readonly vpcPlacement?: ec2.SubnetSelection;
  protected readonly newCfnProps: CfnDBInstanceProps;
-  protected readonly securityGroup: ec2.SecurityGroup;
+  protected readonly securityGroups: ec2.ISecurityGroup[];


So, now that you moved creating connections here - is there any need to have this field? I believe the field can now be removed, and securityGroups can be just a local variable of the constructor of DatabaseInstanceNew, correct?

aws-cdk-automation · 2019-10-24T07:41:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85

Thanks for your patience!

mergify · 2019-10-24T22:08:02Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-24T22:56:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

jogold · 2019-10-25T05:07:08Z

@skinny85 it has been merged by mergify... it doesn't have the BREAKING CHANGE in the commit message.

skinny85 · 2019-10-25T20:14:53Z

@skinny85 it has been merged by mergify... it doesn't have the BREAKING CHANGE in the commit message.

Thanks for letting me know. I'll be doing the CDK release next week (probably Monday or Tuesday), remind me to add this to the Changelog's BREAKING CHANGES section 🙂.

feat(rds): allow using existing security group for instance

cc82ba4

Closes aws#2949

jogold requested a review from skinny85 as a code owner October 14, 2019 12:49

SomayaB assigned skinny85 Oct 14, 2019

skinny85 previously requested changes Oct 14, 2019

View reviewed changes

jogold added 2 commits October 18, 2019 20:04

Merge branch 'master' into rds-existing-sg

5b4e7e0

securityGroups

8dd79de

s

14fdf33

jogold changed the title ~~feat(rds): allow using existing security group for instance~~ feat(rds): allow using existing security group for new instance Oct 19, 2019

jogold changed the title ~~feat(rds): allow using existing security group for new instance~~ feat(rds): allow using existing security groups for new instance Oct 19, 2019

skinny85 reviewed Oct 22, 2019

View reviewed changes

jogold added 2 commits October 23, 2019 09:32

Merge branch 'master' into rds-existing-sg

43f35c1

avoid duplication

edd099c

skinny85 reviewed Oct 24, 2019

View reviewed changes

jogold added 2 commits October 24, 2019 09:01

remove useless protected securityGroups

fa792a5

Merge branch 'master' into rds-existing-sg

b426a41

skinny85 approved these changes Oct 24, 2019

View reviewed changes

Merge branch 'master' into rds-existing-sg

c23ea17

mergify bot merged commit ef1ce5e into aws:master Oct 24, 2019

jogold deleted the rds-existing-sg branch October 28, 2019 08:52

jogold mentioned this pull request Oct 28, 2019

v1.15.0 #4707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): allow using existing security groups for new instance #4495

feat(rds): allow using existing security groups for new instance #4495

jogold commented Oct 14, 2019 •

edited

Loading

mergify bot commented Oct 14, 2019

aws-cdk-automation commented Oct 14, 2019

skinny85 left a comment

skinny85 Oct 14, 2019

jogold Oct 14, 2019 •

edited

Loading

jogold Oct 14, 2019

skinny85 Oct 18, 2019

jogold Oct 18, 2019

skinny85 Oct 18, 2019

jogold Oct 18, 2019

skinny85 Oct 18, 2019

skinny85 Oct 18, 2019

jogold Oct 19, 2019

rix0rrr commented Oct 17, 2019

jogold commented Oct 17, 2019

aws-cdk-automation commented Oct 18, 2019

aws-cdk-automation commented Oct 18, 2019

skinny85 Oct 22, 2019 •

edited

Loading

aws-cdk-automation commented Oct 23, 2019

skinny85 left a comment

skinny85 Oct 24, 2019

jogold Oct 24, 2019

aws-cdk-automation commented Oct 24, 2019

skinny85 left a comment

mergify bot commented Oct 24, 2019

aws-cdk-automation commented Oct 24, 2019

jogold commented Oct 25, 2019

skinny85 commented Oct 25, 2019

feat(rds): allow using existing security groups for new instance #4495

feat(rds): allow using existing security groups for new instance #4495

Conversation

jogold commented Oct 14, 2019 • edited Loading

mergify bot commented Oct 14, 2019

aws-cdk-automation commented Oct 14, 2019

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Oct 17, 2019

jogold commented Oct 17, 2019

aws-cdk-automation commented Oct 18, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 18, 2019

AWS CodeBuild CI Report

skinny85 Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 23, 2019

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 24, 2019

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 24, 2019

aws-cdk-automation commented Oct 24, 2019

AWS CodeBuild CI Report

jogold commented Oct 25, 2019

skinny85 commented Oct 25, 2019

jogold commented Oct 14, 2019 •

edited

Loading

jogold Oct 14, 2019 •

edited

Loading

skinny85 Oct 22, 2019 •

edited

Loading