fix(ecs): cannot separate Cluster and Ec2Service behind ALB

packages/@aws-cdk/aws-ec2/lib/security-group.ts

@@ -17,6 +17,11 @@ export interface ISecurityGroup extends IResource, IPeer {
    */
   readonly securityGroupId: string;
 
+  /**
+   * Whether the SecurityGroup has been configured to allow all outbound traffic
+   */
+  readonly allowAllOutbound: boolean;
+
   /**
    * Add an ingress rule for the current security group
    *
@@ -52,6 +57,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {
   }
 
   public abstract readonly securityGroupId: string;
+  public abstract readonly allowAllOutbound: boolean;
 
   public readonly canInlineRule = false;
   public readonly connections: Connections = new Connections({ securityGroups: [this] });
@@ -294,6 +300,7 @@ export class SecurityGroup extends SecurityGroupBase {
   public static fromSecurityGroupId(scope: Construct, id: string, securityGroupId: string, options: SecurityGroupImportOptions = {}): ISecurityGroup {
     class MutableImport extends SecurityGroupBase {
       public securityGroupId = securityGroupId;
+      public allowAllOutbound = options.allowAllOutbound ?? true;
 
       public addEgressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {
         // Only if allowAllOutbound has been disabled
@@ -305,6 +312,7 @@ export class SecurityGroup extends SecurityGroupBase {
 
     class ImmutableImport extends SecurityGroupBase {
       public securityGroupId = securityGroupId;
+      public allowAllOutbound = options.allowAllOutbound ?? true;
 
       public addEgressRule(_peer: IPeer, _connection: Port, _description?: string, _remoteRule?: boolean) {
         // do nothing
@@ -341,12 +349,15 @@ export class SecurityGroup extends SecurityGroupBase {
    */
   public readonly securityGroupVpcId: string;
 
+  /**
+   * Whether the SecurityGroup has been configured to allow all outbound traffic
+   */
+  public readonly allowAllOutbound: boolean;
+
   private readonly securityGroup: CfnSecurityGroup;
   private readonly directIngressRules: CfnSecurityGroup.IngressProperty[] = [];
   private readonly directEgressRules: CfnSecurityGroup.EgressProperty[] = [];
 
-  private readonly allowAllOutbound: boolean;
-
   constructor(scope: Construct, id: string, props: SecurityGroupProps) {
     super(scope, id, {
       physicalName: props.securityGroupName

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

@@ -1,5 +1,5 @@
 import * as ec2 from '@aws-cdk/aws-ec2';
-import { Construct, Lazy, Resource } from '@aws-cdk/core';
+import { Construct, Lazy, Resource, Stack } from '@aws-cdk/core';
 import { BaseService, BaseServiceOptions, IService, LaunchType, PropagatedTagSource } from '../base/base-service';
 import { NetworkMode, TaskDefinition } from '../base/task-definition';
 import { CfnService } from '../ecs.generated';
@@ -160,9 +160,16 @@ export class Ec2Service extends BaseService implements IEc2Service {
     if (props.taskDefinition.networkMode === NetworkMode.AWS_VPC) {
       this.configureAwsVpcNetworking(props.cluster.vpc, props.assignPublicIp, props.vpcSubnets, props.securityGroup);
     } else {
-      // Either None, Bridge or Host networking. Copy SecurityGroup from ASG.
+      // Either None, Bridge or Host networking. Copy SecurityGroups from ASG.
+      // We have to be smart here -- by default future Security Group rules would be created
+      // in the Cluster stack. However, if the Cluster is in a different stack than us,
+      // that will lead to a cyclic reference (we point to that stack for the cluster name,
+      // but that stack will point to the ALB probably created right next to us).
+      //
+      // In that case, reference the same security groups but make sure new rules are
+      // created in the current scope (i.e., this stack)
       validateNoNetworkingProps(props);
-      this.connections.addSecurityGroup(...props.cluster.connections.securityGroups);
+      this.connections.addSecurityGroup(...securityGroupsInThisStack(this, props.cluster.connections.securityGroups));
     }
 
     this.addPlacementConstraints(...props.placementConstraints || []);
@@ -218,6 +225,28 @@ function validateNoNetworkingProps(props: Ec2ServiceProps) {
   }
 }
 
+/**
+ * Force security group rules to be created in this stack.
+ *
+ * For every security group, if the scope and the group are in different stacks, return
+ * a fake "imported" security group instead. This will behave as the original security group,
+ * but new Ingress and Egress rule resources will be added in the current stack instead of the
+ * other one.
+ */
+function securityGroupsInThisStack(scope: Construct, groups: ec2.ISecurityGroup[]): ec2.ISecurityGroup[] {
+  const thisStack = Stack.of(scope);
+
+  let i = 1;
+  return groups.map(group => {
+    if (thisStack === Stack.of(group)) { return group; } // Simple case, just return the original one
+
+    return ec2.SecurityGroup.fromSecurityGroupId(scope, `SecurityGroup${i++}`, group.securityGroupId, {
+      allowAllOutbound: group.allowAllOutbound,
+      mutable: true,
+    });
+  });
+}
+
 /**
  * The built-in container instance attributes
  */

packages/@aws-cdk/aws-ecs/test/ec2/test.cross-stack.ts

@@ -0,0 +1,100 @@
+import { expect, haveResource } from '@aws-cdk/assert';
+import * as ec2 from '@aws-cdk/aws-ec2';
+import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
+import { App, Stack } from '@aws-cdk/core';
+import { Test } from 'nodeunit';
+import * as ecs from '../../lib';
+
+// Test various cross-stack Cluster/Service/ALB scenario's
+
+let app: App;
+let stack1: Stack;
+let stack2: Stack;
+let cluster: ecs.Cluster;
+let service: ecs.Ec2Service;
+
+export = {
+  "setUp"(cb: () => void) {
+    app = new App();
+
+    stack1 = new Stack(app, 'Stack1');
+    const vpc = new ec2.Vpc(stack1, 'Vpc');
+    cluster = new ecs.Cluster(stack1, 'Cluster', {
+      vpc,
+      capacity: { instanceType: new ec2.InstanceType('t2.micro'), }
+    });
+
+    stack2 = new Stack(app, 'Stack2');
+    const taskDefinition = new ecs.Ec2TaskDefinition(stack2, 'TD');
+    const container = taskDefinition.addContainer('Main', {
+      image: ecs.ContainerImage.fromRegistry('asdf'),
+      memoryLimitMiB: 512
+    });
+    container.addPortMappings({ containerPort: 8000 });
+
+    service = new ecs.Ec2Service(stack2, 'Service', {
+      cluster,
+      taskDefinition,
+    });
+
+    cb();
+  },
+
+  "ALB next to Service"(test: Test) {
+    // WHEN
+    const lb = new elbv2.ApplicationLoadBalancer(stack2, "ALB", { vpc: cluster.vpc });
+    const listener = lb.addListener("listener", { port: 80 });
+    listener.addTargets("target", {
+      port: 80,
+      targets: [service]
+    });
+
+    // THEN: it shouldn't throw due to cyclic dependencies
+    expect(stack2).to(haveResource('AWS::ECS::Service'));
+
+    expectIngress(stack2);
+
+    test.done();
+  },
+
+  "ALB next to Cluster"(test: Test) {
+    // WHEN
+    const lb = new elbv2.ApplicationLoadBalancer(stack1, "ALB", { vpc: cluster.vpc });
+    const listener = lb.addListener("listener", { port: 80 });
+    listener.addTargets("target", {
+      port: 80,
+      targets: [service]
+    });
+
+    // THEN: it shouldn't throw due to cyclic dependencies
+    expect(stack2).to(haveResource('AWS::ECS::Service'));
+    expectIngress(stack2);
+
+    test.done();
+  },
+
+  "ALB in its own stack"(test: Test) {
+    // WHEN
+    const stack3 = new Stack(app, 'Stack3');
+    const lb = new elbv2.ApplicationLoadBalancer(stack3, "ALB", { vpc: cluster.vpc });
+    const listener = lb.addListener("listener", { port: 80 });
+    listener.addTargets("target", {
+      port: 80,
+      targets: [service]
+    });
+
+    // THEN: it shouldn't throw due to cyclic dependencies
+    expect(stack2).to(haveResource('AWS::ECS::Service'));
+    expectIngress(stack2);
+
+    test.done();
+  },
+};
+
+function expectIngress(stack: Stack) {
+  expect(stack).to(haveResource('AWS::EC2::SecurityGroupIngress', {
+    FromPort: 32768,
+    ToPort: 65535,
+    GroupId: { "Fn::ImportValue": "Stack1:ExportsOutputFnGetAttClusterDefaultAutoScalingGroupInstanceSecurityGroup1D15236AGroupIdEAB9C5E1" },
+  }));
+}