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

In the case of an ECS Service runningon  EC2 capacity, pointing a Load
Balancer to the Service involves updating the SecurityGroups of the
capacity associated to the Cluster (to allow traffic from the Load
Balancer).

If these resources are in different stacks, this is liable to create
cyclic references: Service points to Cluster, Security Groups point
to Load Balancer, cyclic references arise from the way these resources
are typically colocated.

This changes makes it so the ingress/egress rules will be created in
the same stack as the Service (which points to both Cluster and LB),
hence making sure there are no cycles.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

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" },
+  }));
+}