From 3517c197a3f4e9a003d8294562c032a990058f14 Mon Sep 17 00:00:00 2001 From: peterwoodworth Date: Thu, 28 Oct 2021 13:51:24 -0700 Subject: [PATCH 1/5] fix(ec2) addIngressRule and addEgressRule detect unresolved tokens as duplicates --- .../@aws-cdk/aws-ec2/lib/security-group.ts | 30 +++++++++++++++++-- .../aws-ec2/test/security-group.test.ts | 25 ++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index 54695f2d17b71..3e20063b1b32d 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/packages/@aws-cdk/aws-ec2/lib/security-group.ts @@ -68,6 +68,8 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { public readonly connections: Connections = new Connections({ securityGroups: [this] }); public readonly defaultPort?: Port; + private peerAsTokenCount: number = 0; + constructor(scope: Construct, id: string, props?: ResourceProps) { super(scope, id, props); @@ -83,7 +85,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { description = `from ${peer.uniqueId}:${connection}`; } - const [scope, id] = determineRuleScope(this, peer, connection, 'from', remoteRule); + const [scope, id] = this.determineRuleScope(peer, connection, 'from', remoteRule); // Skip duplicates if (scope.node.tryFindChild(id) === undefined) { @@ -101,7 +103,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { description = `to ${peer.uniqueId}:${connection}`; } - const [scope, id] = determineRuleScope(this, peer, connection, 'to', remoteRule); + const [scope, id] = this.determineRuleScope(peer, connection, 'to', remoteRule); // Skip duplicates if (scope.node.tryFindChild(id) === undefined) { @@ -121,6 +123,30 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { public toEgressRuleConfig(): any { return { destinationSecurityGroupId: this.securityGroupId }; } + + private determineRuleScope( + peer: IPeer, + connection: Port, + fromTo: 'from' | 'to', + remoteRule?: boolean): [SecurityGroupBase, string] { + + if (remoteRule && SecurityGroupBase.isSecurityGroup(peer) && differentStacks(this, peer)) { + // Reversed + const reversedFromTo = fromTo === 'from' ? 'to' : 'from'; + return [peer, `${this.uniqueId}:${connection} ${reversedFromTo}`]; + } else { + // Regular (do old ID escaping to in order to not disturb existing deployments) + return [this, `${fromTo} ${this.renderPeer(peer)}:${connection}`.replace('/', '_')]; + } + } + + private renderPeer(peer: IPeer) { + if (Token.isUnresolved(peer.uniqueId)) { + return this.peerAsTokenCount++ ? `'{IndirectPeer${this.peerAsTokenCount}}'` : '{IndirectPeer}'; + } else { + return peer.uniqueId; + } + } } /** diff --git a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts index 09ccc6cdc682e..dbe2dc24eac69 100644 --- a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts @@ -208,6 +208,31 @@ describe('security group', () => { }); + test('addIngressRule works multiple times with tokens', () => { + // GIVEN + const stack = new Stack(undefined, 'TestStack', { env: { account: '12345678', region: 'dummy' } }); + const vpc = new Vpc(stack, 'VPC'); + const sg = new SecurityGroup(stack, 'SG', { vpc }); + const p1 = Lazy.string({ produce: () => '80' }); + const p2 = Lazy.string({ produce: () => '5000' }); + + const peer1 = Peer.prefixList(p1); + const peer2 = Peer.prefixList(p2); + + // WHEN + sg.addIngressRule(peer1, Port.tcp(5432), 'Rule 1'); + sg.addIngressRule(peer2, Port.tcp(5432), 'Rule 2'); + + // THEN -- no crash + expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', { + "Description": "Rule 1", + }); + expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', { + "Description": "Rule 2", + }); + + }); + test('if tokens are used in ports, `canInlineRule` should be false to avoid cycles', () => { // GIVEN const p1 = Lazy.number({ produce: () => 80 }); From 868c8bd68bd9965bbfb2cbacdea8a56c390fd78d Mon Sep 17 00:00:00 2001 From: peterwoodworth Date: Thu, 28 Oct 2021 14:19:13 -0700 Subject: [PATCH 2/5] fix(ec2) addIngressRule and addEgressRule detect unresolved tokens as duplicates --- packages/@aws-cdk/aws-ec2/test/security-group.test.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts index dbe2dc24eac69..3e68e5ff1c59b 100644 --- a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts @@ -208,14 +208,14 @@ describe('security group', () => { }); - test('addIngressRule works multiple times with tokens', () => { + test('can add multiple rules using tokens on same security group', () => { // GIVEN const stack = new Stack(undefined, 'TestStack', { env: { account: '12345678', region: 'dummy' } }); const vpc = new Vpc(stack, 'VPC'); const sg = new SecurityGroup(stack, 'SG', { vpc }); - const p1 = Lazy.string({ produce: () => '80' }); - const p2 = Lazy.string({ produce: () => '5000' }); + const p1 = Lazy.string({ produce: () => 'dummyid1' }); + const p2 = Lazy.string({ produce: () => 'dummyid2' }); const peer1 = Peer.prefixList(p1); const peer2 = Peer.prefixList(p2); @@ -230,7 +230,6 @@ describe('security group', () => { expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', { "Description": "Rule 2", }); - }); test('if tokens are used in ports, `canInlineRule` should be false to avoid cycles', () => { From 3e82bae25200267ab8deb6a856a048dff0e4891e Mon Sep 17 00:00:00 2001 From: peterwoodworth Date: Thu, 28 Oct 2021 14:33:05 -0700 Subject: [PATCH 3/5] fix(ec2) addIngressRule and addEgressRule detect unresolved tokens as duplicates --- packages/@aws-cdk/aws-ec2/test/security-group.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts index 3e68e5ff1c59b..70dbe64cef335 100644 --- a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts @@ -225,10 +225,10 @@ describe('security group', () => { // THEN -- no crash expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', { - "Description": "Rule 1", + Description: 'Rule 1', }); expect(stack).toHaveResourceLike('AWS::EC2::SecurityGroupIngress', { - "Description": "Rule 2", + Description: 'Rule 2', }); }); From 998e2839d38c15a9e94b4be1a7b82cff883d0dc4 Mon Sep 17 00:00:00 2001 From: peterwoodworth Date: Thu, 28 Oct 2021 16:23:42 -0700 Subject: [PATCH 4/5] fix(ec2) addIngressRule and addEgressRule detect unresolved tokens as duplicates --- .../@aws-cdk/aws-ec2/lib/security-group.ts | 29 +++---------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index 3e20063b1b32d..a56d71c635c85 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/packages/@aws-cdk/aws-ec2/lib/security-group.ts @@ -124,7 +124,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { return { destinationSecurityGroupId: this.securityGroupId }; } - private determineRuleScope( + protected determineRuleScope( peer: IPeer, connection: Port, fromTo: 'from' | 'to', @@ -197,26 +197,6 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { * ║ └───────────┘ ║ * ╚═══════════════════════════════════╝ */ -function determineRuleScope( - group: SecurityGroupBase, - peer: IPeer, - connection: Port, - fromTo: 'from' | 'to', - remoteRule?: boolean): [SecurityGroupBase, string] { - - if (remoteRule && SecurityGroupBase.isSecurityGroup(peer) && differentStacks(group, peer)) { - // Reversed - const reversedFromTo = fromTo === 'from' ? 'to' : 'from'; - return [peer, `${group.uniqueId}:${connection} ${reversedFromTo}`]; - } else { - // Regular (do old ID escaping to in order to not disturb existing deployments) - return [group, `${fromTo} ${renderPeer(peer)}:${connection}`.replace('/', '_')]; - } -} - -function renderPeer(peer: IPeer) { - return Token.isUnresolved(peer.uniqueId) ? '{IndirectPeer}' : peer.uniqueId; -} function differentStacks(group1: SecurityGroupBase, group2: SecurityGroupBase) { return Stack.of(group1) !== Stack.of(group2); @@ -591,13 +571,12 @@ export class SecurityGroup extends SecurityGroupBase { */ private removeNoTrafficRule() { if (this.disableInlineRules) { - const [scope, id] = determineRuleScope( - this, + const [scope, id] = this.determineRuleScope( NO_TRAFFIC_PEER, NO_TRAFFIC_PORT, 'to', - false); - + false, + ); scope.node.tryRemoveChild(id); } else { const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, MATCH_NO_TRAFFIC)); From 0f94864dfa72b39025a9d2c9162f12be262c44d4 Mon Sep 17 00:00:00 2001 From: peterwoodworth Date: Fri, 29 Oct 2021 11:41:29 -0700 Subject: [PATCH 5/5] fix(ec2) addIngressRule and addEgressRule detect unresolved tokens as duplicates --- .../@aws-cdk/aws-ec2/lib/security-group.ts | 101 +++++++++--------- 1 file changed, 52 insertions(+), 49 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index a56d71c635c85..692df3629ebbf 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/packages/@aws-cdk/aws-ec2/lib/security-group.ts @@ -124,6 +124,55 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { return { destinationSecurityGroupId: this.securityGroupId }; } + /** + * Determine where to parent a new ingress/egress rule + * + * A SecurityGroup rule is parented under the group it's related to, UNLESS + * we're in a cross-stack scenario with another Security Group. In that case, + * we respect the 'remoteRule' flag and will parent under the other security + * group. + * + * This is necessary to avoid cyclic dependencies between stacks, since both + * ingress and egress rules will reference both security groups, and a naive + * parenting will lead to the following situation: + * + * ╔════════════════════╗ ╔════════════════════╗ + * ║ ┌───────────┐ ║ ║ ┌───────────┐ ║ + * ║ │ GroupA │◀────╬─┐ ┌───╬───▶│ GroupB │ ║ + * ║ └───────────┘ ║ │ │ ║ └───────────┘ ║ + * ║ ▲ ║ │ │ ║ ▲ ║ + * ║ │ ║ │ │ ║ │ ║ + * ║ │ ║ │ │ ║ │ ║ + * ║ ┌───────────┐ ║ └───┼───╬────┌───────────┐ ║ + * ║ │ EgressA │─────╬─────┘ ║ │ IngressB │ ║ + * ║ └───────────┘ ║ ║ └───────────┘ ║ + * ║ ║ ║ ║ + * ╚════════════════════╝ ╚════════════════════╝ + * + * By having the ability to switch the parent, we avoid the cyclic reference by + * keeping all rules in a single stack. + * + * If this happens, we also have to change the construct ID, because + * otherwise we might have two objects with the same ID if we have + * multiple reversed security group relationships. + * + * ╔═══════════════════════════════════╗ + * ║┌───────────┐ ║ + * ║│ GroupB │ ║ + * ║└───────────┘ ║ + * ║ ▲ ║ + * ║ │ ┌───────────┐ ║ + * ║ ├────"from A"──│ IngressB │ ║ + * ║ │ └───────────┘ ║ + * ║ │ ┌───────────┐ ║ + * ║ ├─────"to B"───│ EgressA │ ║ + * ║ │ └───────────┘ ║ + * ║ │ ┌───────────┐ ║ + * ║ └─────"to B"───│ EgressC │ ║ <-- oops + * ║ └───────────┘ ║ + * ╚═══════════════════════════════════╝ + */ + protected determineRuleScope( peer: IPeer, connection: Port, @@ -142,6 +191,9 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { private renderPeer(peer: IPeer) { if (Token.isUnresolved(peer.uniqueId)) { + // Need to return a unique value each time a peer + // is an unresolved token, else the duplicate skipper + // in `sg.addXxxRule` can detect unique rules as duplicates return this.peerAsTokenCount++ ? `'{IndirectPeer${this.peerAsTokenCount}}'` : '{IndirectPeer}'; } else { return peer.uniqueId; @@ -149,55 +201,6 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup { } } -/** - * Determine where to parent a new ingress/egress rule - * - * A SecurityGroup rule is parented under the group it's related to, UNLESS - * we're in a cross-stack scenario with another Security Group. In that case, - * we respect the 'remoteRule' flag and will parent under the other security - * group. - * - * This is necessary to avoid cyclic dependencies between stacks, since both - * ingress and egress rules will reference both security groups, and a naive - * parenting will lead to the following situation: - * - * ╔════════════════════╗ ╔════════════════════╗ - * ║ ┌───────────┐ ║ ║ ┌───────────┐ ║ - * ║ │ GroupA │◀────╬─┐ ┌───╬───▶│ GroupB │ ║ - * ║ └───────────┘ ║ │ │ ║ └───────────┘ ║ - * ║ ▲ ║ │ │ ║ ▲ ║ - * ║ │ ║ │ │ ║ │ ║ - * ║ │ ║ │ │ ║ │ ║ - * ║ ┌───────────┐ ║ └───┼───╬────┌───────────┐ ║ - * ║ │ EgressA │─────╬─────┘ ║ │ IngressB │ ║ - * ║ └───────────┘ ║ ║ └───────────┘ ║ - * ║ ║ ║ ║ - * ╚════════════════════╝ ╚════════════════════╝ - * - * By having the ability to switch the parent, we avoid the cyclic reference by - * keeping all rules in a single stack. - * - * If this happens, we also have to change the construct ID, because - * otherwise we might have two objects with the same ID if we have - * multiple reversed security group relationships. - * - * ╔═══════════════════════════════════╗ - * ║┌───────────┐ ║ - * ║│ GroupB │ ║ - * ║└───────────┘ ║ - * ║ ▲ ║ - * ║ │ ┌───────────┐ ║ - * ║ ├────"from A"──│ IngressB │ ║ - * ║ │ └───────────┘ ║ - * ║ │ ┌───────────┐ ║ - * ║ ├─────"to B"───│ EgressA │ ║ - * ║ │ └───────────┘ ║ - * ║ │ ┌───────────┐ ║ - * ║ └─────"to B"───│ EgressC │ ║ <-- oops - * ║ └───────────┘ ║ - * ╚═══════════════════════════════════╝ - */ - function differentStacks(group1: SecurityGroupBase, group2: SecurityGroupBase) { return Stack.of(group1) !== Stack.of(group2); }