BREAKING(aws-ec2): SecurityGroup can be used in Connections

Refactoring of the object model for connection/security groups so that a SecurityGroup object can be used as the target of an .allowTo() statement: cluster.connections.allowTo(securityGroup) Also add SecurityGroupRef.import() to allow importing a non-constructed SecurityGroup into the construct tree. As part of the refactoring: - Get rid of IDefaultConnectable, the functionality has been folded into IConnectable/Connections. - Get rid of ISecurityGroup. - Rename IConnectionPeer => ISecurityGroupRule. - Drastically simplify implementation, get rid of recursion and classes to enable the recursion to terminate. All complex logic is now nicely contained within Connections. This change is BREAKING to connections-enabled construct writers, but transparent to application builders. Fixes #579.
aws · Aug 16, 2018 · 69bd200 · 69bd200
1 parent 0220bbc
commit 69bd200
Show file tree

Hide file tree

Showing 9 changed files with 193 additions and 161 deletions.
diff --git a/packages/@aws-cdk/aws-ec2/lib/auto-scaling-group.ts b/packages/@aws-cdk/aws-ec2/lib/auto-scaling-group.ts
@@ -2,12 +2,12 @@ import autoscaling = require('@aws-cdk/aws-autoscaling');
 import iam = require('@aws-cdk/aws-iam');
 import sns = require('@aws-cdk/aws-sns');
 import cdk = require('@aws-cdk/cdk');
-import { AllConnections, AnyIPv4, IConnectionPeer } from './connection';
-import { Connections } from './connections';
+import { Connections, IConnectable } from './connections';
 import { InstanceType } from './instance-types';
 import { ClassicLoadBalancer, IClassicLoadBalancerTarget } from './load-balancer';
 import { IMachineImageSource, OperatingSystemType } from './machine-image';
 import { SecurityGroup } from './security-group';
+import { AllConnections, AnyIPv4 } from './security-group-rule';
 import { VpcNetworkRef, VpcPlacementStrategy } from './vpc-ref';
 
 /**
@@ -83,9 +83,7 @@ export interface AutoScalingGroupProps {
  *
  * The ASG spans all availability zones.
  */
-export class AutoScalingGroup extends cdk.Construct implements IClassicLoadBalancerTarget {
-    public readonly connectionPeer: IConnectionPeer;
-
+export class AutoScalingGroup extends cdk.Construct implements IClassicLoadBalancerTarget, IConnectable {
     /**
      * The type of OS instances of this fleet are running.
      */
@@ -110,8 +108,7 @@ export class AutoScalingGroup extends cdk.Construct implements IClassicLoadBalan
         super(parent, name);
 
         this.securityGroup = new SecurityGroup(this, 'InstanceSecurityGroup', { vpc: props.vpc });
-        this.connections = new Connections(this.securityGroup);
-        this.connectionPeer = this.securityGroup;
+        this.connections = new Connections({ securityGroup: this.securityGroup });
 
         if (props.allowAllOutbound !== false) {
             this.connections.allowTo(new AnyIPv4(), new AllConnections(), 'Outbound traffic allowed by default');

diff --git a/packages/@aws-cdk/aws-ec2/lib/connections.ts b/packages/@aws-cdk/aws-ec2/lib/connections.ts
@@ -1,6 +1,5 @@
-import { AnyIPv4, IConnectionPeer, IPortRange } from "./connection";
-import { SecurityGroupId } from "./ec2.generated";
-import { ISecurityGroup } from "./security-group";
+import { SecurityGroupRef } from "./security-group";
+import { AnyIPv4, IPortRange, ISecurityGroupRule } from "./security-group-rule";
 
 /**
  * The goal of this module is to make possible to write statements like this:
@@ -24,81 +23,103 @@ export interface IConnectable {
 }
 
 /**
- * An object that has a Connections object as well as a default port range.
+ * Properties to intialize a new Connections object
  */
-export interface IDefaultConnectable extends IConnectable {
-    readonly defaultPortRange: IPortRange;
+export interface ConnectionsProps {
+    /**
+     * Class that represents the rule by which others can connect to this connectable
+     *
+     * This object is required, but will be derived from securityGroup if that is passed.
+     *
+     * @default Derived from securityGroup if set.
+     */
+    securityGroupRule?: ISecurityGroupRule;
+
+    /**
+     * What securityGroup this object is managing connections for
+     *
+     * @default No security
+     */
+    securityGroup?: SecurityGroupRef;
+
+    /**
+     * Default port range for initiating connections to and from this object
+     *
+     * @default No default port range
+     */
+    defaultPortRange?: IPortRange;
 }
 
 /**
- * Manage the security group (firewall) for a connectable resource.
+ * Manage the allowed network connections for constructs with Security Groups.
  *
- * This object contains method to allow connections between objects
- * that can allow connections.
+ * Security Groups can be thought of as a firewall for network-connected
+ * devices. This class makes it easy to allow network connections to and
+ * from security groups, and between security groups individually. When
+ * establishing connectivity between security groups, it will automatically
+ * add rules in both security groups
  *
- * The .allowDefaultPortXxx() methods are only available if the resource
- * this object was created for has the concept of a default port range.
  */
 export class Connections {
-    public readonly connectionPeer: IConnectionPeer;
+    private readonly securityGroupRule: ISecurityGroupRule;
+    private readonly securityGroup?: SecurityGroupRef;
+    private readonly defaultPortRange?: IPortRange;
 
-    constructor(private readonly securityGroup: ISecurityGroup, private readonly defaultPortRange?: IPortRange) {
-        this.connectionPeer = securityGroup;
-    }
+    constructor(props: ConnectionsProps) {
+        if (!props.securityGroupRule && !props.securityGroup) {
+            throw new Error('Connections: require one of securityGroupRule or securityGroup');
+        }
 
-    /**
-     * Allow connections to the peer on their default port
-     */
-    public allowToDefaultPort(other: IDefaultConnectable, description: string) {
-        this.allowTo(other, other.defaultPortRange, description);
+        this.securityGroupRule = props.securityGroupRule || props.securityGroup!;
+        this.securityGroup = props.securityGroup;
+        this.defaultPortRange = props.defaultPortRange;
     }
 
     /**
      * Allow connections to the peer on the given port
      */
     public allowTo(other: IConnectable, portRange: IPortRange, description: string) {
         if (this.securityGroup) {
-            this.securityGroup.addEgressRule(other.connections.connectionPeer, portRange, description);
+            this.securityGroup.addEgressRule(other.connections.securityGroupRule, portRange, description);
+        }
+        if (other.connections.securityGroup) {
+            other.connections.securityGroup.addIngressRule(this.securityGroupRule, portRange, description);
+
         }
-        other.connections.allowFrom(
-            new NullConnectable(this.connectionPeer),
-            portRange,
-            description);
     }
 
     /**
      * Allow connections from the peer on the given port
      */
     public allowFrom(other: IConnectable, portRange: IPortRange, description: string) {
         if (this.securityGroup) {
-            this.securityGroup.addIngressRule(other.connections.connectionPeer, portRange, description);
+            this.securityGroup.addIngressRule(other.connections.securityGroupRule, portRange, description);
+        }
+        if (other.connections.securityGroup) {
+            other.connections.securityGroup.addEgressRule(this.securityGroupRule, portRange, description);
         }
-        other.connections.allowTo(
-            new NullConnectable(this.connectionPeer),
-            portRange,
-            description);
     }
 
     /**
      * Allow hosts inside the security group to connect to each other on the given port
      */
     public allowInternally(portRange: IPortRange, description: string) {
         if (this.securityGroup) {
-            this.securityGroup.addIngressRule(this.securityGroup, portRange, description);
+            this.securityGroup.addIngressRule(this.securityGroupRule, portRange, description);
         }
     }
 
     /**
      * Allow to all IPv4 ranges
      */
-    public allowToAnyIpv4(portRange: IPortRange, description: string) {
+    public allowToAnyIPv4(portRange: IPortRange, description: string) {
         this.allowTo(new AnyIPv4(), portRange, description);
     }
 
     /**
      * Allow from any IPv4 ranges
      */
-    public allowFromAnyIpv4(portRange: IPortRange, description: string) {
+    public allowFromAnyIPv4(portRange: IPortRange, description: string) {
         this.allowFrom(new AnyIPv4(), portRange, description);
     }
 
@@ -109,7 +130,7 @@ export class Connections {
      */
     public allowDefaultPortFrom(other: IConnectable, description: string) {
         if (!this.defaultPortRange) {
-            throw new Error('Cannot call allowDefaultPortFrom(): resource has no default port');
+            throw new Error('Cannot call allowDefaultPortFrom(): this resource has no default port');
         }
         this.allowFrom(other, this.defaultPortRange, description);
     }
@@ -119,7 +140,7 @@ export class Connections {
      */
     public allowDefaultPortInternally(description: string) {
         if (!this.defaultPortRange) {
-            throw new Error('Cannot call allowDefaultPortInternally(): resource has no default port');
+            throw new Error('Cannot call allowDefaultPortInternally(): this resource has no default port');
         }
         this.allowInternally(this.defaultPortRange, description);
     }
@@ -129,62 +150,19 @@ export class Connections {
      */
     public allowDefaultPortFromAnyIpv4(description: string) {
         if (!this.defaultPortRange) {
-            throw new Error('Cannot call allowDefaultPortFromAnyIpv4(): resource has no default port');
+            throw new Error('Cannot call allowDefaultPortFromAnyIpv4(): this resource has no default port');
         }
-        this.allowFromAnyIpv4(this.defaultPortRange, description);
-    }
-}
-
-/**
- * Connectable that represents a peer but doesn't modify any security groups
- */
-class NullConnectable implements IConnectable {
-    public readonly connections: Connections;
-
-    constructor(connectionPeer: IConnectionPeer) {
-        this.connections = new SecurityGrouplessConnections(connectionPeer);
-    }
-}
-
-/**
- * This object is used by peers who don't allow reverse connections.
- */
-export class SecurityGrouplessConnections extends Connections {
-    constructor(public readonly connectionPeer: IConnectionPeer) {
-        // Because Connections is no longer an interface but a concrete class,
-        // we must inherit from it and create it with an instance of ISecurityGroup.
-        super(new NullSecurityGroup());
-    }
-
-    public allowTo(_other: IConnectable, _connection: IPortRange, _description: string): void {
-        // Nothing to do
+        this.allowFromAnyIPv4(this.defaultPortRange, description);
     }
 
-    public allowFrom(_other: IConnectable, _connection: IPortRange, _description: string): void {
-        // Nothing to do
-    }
-}
-
-/**
- * Instance of ISecurityGroup that's only there for show.
- */
-class NullSecurityGroup implements ISecurityGroup {
-    public securityGroupId: SecurityGroupId = new SecurityGroupId();
-    public canInlineRule: boolean = false;
-
-    public addIngressRule(_peer: IConnectionPeer, _connection: IPortRange, _description: string): void {
-        // Nothing
-    }
-
-    public addEgressRule(_peer: IConnectionPeer, _connection: IPortRange, _description: string): void {
-        // Nothing
-    }
-
-    public toIngressRuleJSON() {
-        return {};
-    }
+    /**
+     * Allow connections to the security group on their default port
+     */
+    public allowToDefaultPort(other: IConnectable, description: string) {
+        if (other.connections.defaultPortRange === undefined) {
+            throw new Error('Cannot call alloToDefaultPort(): other resource has no default port');
+        }
 
-    public toEgressRuleJSON() {
-        return {};
+        this.allowTo(other, other.connections.defaultPortRange, description);
     }
 }
diff --git a/packages/@aws-cdk/aws-ec2/lib/index.ts b/packages/@aws-cdk/aws-ec2/lib/index.ts
@@ -1,10 +1,10 @@
 export * from './auto-scaling-group';
-export * from './connection';
 export * from './connections';
 export * from './instance-types';
 export * from './load-balancer';
 export * from './machine-image';
 export * from './security-group';
+export * from './security-group-rule';
 export * from './vpc';
 export * from './vpc-ref';
 

diff --git a/packages/@aws-cdk/aws-ec2/lib/load-balancer.ts b/packages/@aws-cdk/aws-ec2/lib/load-balancer.ts
@@ -1,8 +1,8 @@
 import elasticloadbalancing = require('@aws-cdk/aws-elasticloadbalancing');
 import cdk = require('@aws-cdk/cdk');
-import { AnyIPv4, IConnectionPeer, IPortRange, TcpPort } from './connection';
-import { Connections, IConnectable, IDefaultConnectable } from './connections';
-import { ISecurityGroup, SecurityGroup } from './security-group';
+import { Connections, IConnectable } from './connections';
+import { SecurityGroup, SecurityGroupRef } from './security-group';
+import { AnyIPv4, IPortRange, TcpPort } from './security-group-rule';
 import { VpcNetworkRef, VpcSubnetRef } from './vpc-ref';
 
 /**
@@ -193,8 +193,6 @@ export class ClassicLoadBalancer extends cdk.Construct implements IConnectable {
      */
     public readonly connections: Connections;
 
-    public readonly connectionPeer: IConnectionPeer;
-
     /**
      * An object controlling specifically the connections for each listener added to this load balancer
      */
@@ -211,8 +209,7 @@ export class ClassicLoadBalancer extends cdk.Construct implements IConnectable {
         super(parent, name);
 
         this.securityGroup = new SecurityGroup(this, 'SecurityGroup', { vpc: props.vpc });
-        this.connections = new Connections(this.securityGroup);
-        this.connectionPeer = this.securityGroup;
+        this.connections = new Connections({ securityGroup: this.securityGroup });
 
         // Depending on whether the ELB has public or internal IPs, pick the right backend subnets
         const subnets: VpcSubnetRef[] = props.internetFacing ? props.vpc.publicSubnets : props.vpc.privateSubnets;
@@ -329,11 +326,11 @@ export class ClassicLoadBalancer extends cdk.Construct implements IConnectable {
  * balancer's security group just for the port ranges that are involved in the
  * listener.
  */
-export class ClassicListenerPort implements IDefaultConnectable {
+export class ClassicListenerPort implements IConnectable {
     public readonly connections: Connections;
 
-    constructor(securityGroup: ISecurityGroup, public readonly defaultPortRange: IPortRange) {
-        this.connections = new Connections(securityGroup, defaultPortRange);
+    constructor(securityGroup: SecurityGroupRef, defaultPortRange: IPortRange) {
+        this.connections = new Connections({ securityGroup, defaultPortRange });
     }
 }