(rds): Instance support for addSecurityGroup

[markussiebert]

Description

RDS Instances (maybe Clusters, didn't checked that) should allow adding Securitygroups

Use Case

We have predefined SGs we want to add to all Resources. We do this by creating aspects, that add our special SecurityGroups. For RDS it's a hack accessing raw cfn resources and manipulate them. But it would be great to implement the same behaviour as several other resources.

Proposed Solution

RDS Instances should stick to the implementation of EC2 Instances

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

Line 430 in dd5e12d

public addSecurityGroup(securityGroup: ISecurityGroup): void {

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

Line 303 in dd5e12d

private readonly securityGroups: ISecurityGroup[] = [];

instead of holding the securitygroups only inside the constructor and than lazy map this

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

Line 746 in dd5e12d

vpcSecurityGroups: securityGroups.map(s => s.securityGroupId),

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

Line 337 in dd5e12d

const securityGroupsToken = Lazy.list({ produce: () => this.securityGroups.map(sg => sg.securityGroupId) });

Other information

no

Acknowledge

• I may be able to implement this feature request
• This feature might incur a breaking change

[skinny85]

Thanks for opening the issue @markussiebert. Actually, I think the problem is here . Instead of using this.securityGroups here, we should use this.connections.securityGroups instead. That would allow you to go instance.connections.addSecurityGroup().

[markussiebert]

Good point @skinny85 - first I thought the same! But, now I think the way it's implement for EC2 Instances is actually really good.

While thinking about this I found:
aws/aws-cdk-rfcs#51
really interesting

If I read this:

/**
 * The goal of this module is to make possible to write statements like this:
 *
 *  ```ts
 *  database.connections.allowFrom(fleet);
 *  fleet.connections.allowTo(database);
 *  rdgw.connections.allowFromCidrIp('0.3.1.5/86');
 *  rgdw.connections.allowTrafficTo(fleet, new AllPorts());
 *  ```
 *
 * The insight here is that some connecting peers have information on what ports should
 * be involved in the connection, and some don't.
 */

I personally don't want to have all Securitygroups in the connections. If I write a statement database.connections.allowFrom(myec2Instance) I don't want my Securitygroup granting access for administrators extended the same way.

  /**
   * What securityGroup(s) this object is managing connections for
   *
   * @default No security groups
   */
  readonly securityGroups?: ISecurityGroup[];

I don't want those securitygroups to be "managed" by the rds. Those securitygroups are somehow special and should be immutable!

That's how the ec2 way works.

But I would like to see all L2/L3 Constructs work the same way :-) (some Constructs say addSecurityGroup, others say addSecurityGroups ... some won't allow this...)

[skinny85]

I don't understand what's the final proposal from your post, but feel free to open a PR 😉.

[markussiebert]

Sorry - at the moment I have no proposal. I would like that all resources behave in the same way. So I think I have to look into how to contribute to the rfcs ...

Because every cdk resource behaves in a different way I was not able to add my SecurityGroup to a lot resources apart from some ec2 resources and now using another approach with prefix lists and an aspect, that creates an ingress for each security group found in the cdk app.

[github-actions]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.