Skip to content

Commit

Permalink
fix(ec2): functions addIngressRule and addEgressRule detect unresolve…
Browse files Browse the repository at this point in the history 
…d tokens as duplicates (aws#17221)

fixes aws#17201 

The issue is when the same security group uses these functions, so I added a private counter to `SecurityGroupBase`. However, to modify this private counter, `determineRuleScope` and `renderPeer` need to be member functions. These originally weren't member functions for a reason, and that's because `SecurityGroup` also uses these functions.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@peterwoodworth @TikiTDO
peterwoodworth authored and TikiTDO committed Feb 21, 2022
1 parent 907ee7b commit d5287e4
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 72 deletions.
152 changes: 80 additions & 72 deletions packages/@aws-cdk/aws-ec2/lib/security-group.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -121,75 +123,82 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {
public toEgressRuleConfig(): any {
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
* ║ └───────────┘ ║
* ╚═══════════════════════════════════╝
*/
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('/', '_')];
/**
* 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,
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('/', '_')];
}
}
}

function renderPeer(peer: IPeer) {
return Token.isUnresolved(peer.uniqueId) ? '{IndirectPeer}' : peer.uniqueId;
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;
}
}
}

function differentStacks(group1: SecurityGroupBase, group2: SecurityGroupBase) {
Expand Down Expand Up @@ -565,13 +574,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));
Expand Down
24 changes: 24 additions & 0 deletions packages/@aws-cdk/aws-ec2/test/security-group.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,30 @@ describe('security group', () => {

});

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: () => 'dummyid1' });
const p2 = Lazy.string({ produce: () => 'dummyid2' });
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 });
Expand Down

0 comments on commit d5287e4

Please sign in to comment.