Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws-ecs-patterns: ApplicationLoadBalancedEc2Service doesn't set SecurityGroups properly, so that ALB can't reach to container #26970

Closed
acomagu opened this issue Sep 1, 2023 · 3 comments
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@acomagu
Copy link
Contributor

acomagu commented Sep 1, 2023

Describe the bug

If I use ec2.Cluster imported from another stack with ApplicationLoadBalancedEc2Service, not reachable from ALB to the container and will have unhealthy status.

There are two causes:

  • The SecurityGroup of EC2 instance doesn't allow any incoming traffic. Should allow the port range used for ALB dynamic port mapping.
    image

  • The SecurityGroup of ALB doesn't allow any outgoing TCP traffic. Should allow all TCP traffic(maybe, but at least should allow something).
    image

Expected Behavior

The SecurityGroups of EC2 instance and ALB are configured properly without additional coding, or the documentation is written to properly configure them.

Current Behavior

ALB can't see the ECS containers and recognizes them as unhealthy.

Reproduction Steps

Deploying with the following code reproduces the situation.

import * as cdk from 'aws-cdk-lib';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
import * as ecs from 'aws-cdk-lib/aws-ecs';
import * as ecsPatterns from 'aws-cdk-lib/aws-ecs-patterns';
import * as constructs from 'constructs';

export class ClusterStack extends cdk.Stack {
  readonly clusterAttributes: ecs.ClusterAttributes;

  constructor(scope: constructs.Construct, id: string) {
    super(scope, id);

    const vpc = new ec2.Vpc(this, 'VPC', {
      maxAzs: 2,
    });

    const cluster = new ecs.Cluster(this, 'Cluster', {
      capacity: {
        instanceType: ec2.InstanceType.of(ec2.InstanceClass.T2, ec2.InstanceSize.NANO),
        maxCapacity: 2,
      },
      vpc,
    });
    this.clusterAttributes = cluster;
  }
}

export class ServiceStack extends cdk.Stack {
  constructor(scope: constructs.Construct, id: string, props: { clusterAttributes: ecs.ClusterAttributes }) {
    super(scope, id);

    const cluster = ecs.Cluster.fromClusterAttributes(this, 'Cluster', props.clusterAttributes);

    new ecsPatterns.ApplicationLoadBalancedEc2Service(this, 'Service', {
      cluster,
      taskImageOptions: {
        image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
      },
      memoryLimitMiB: 128,
      circuitBreaker: { rollback: true },
    });
  }
}

const app = new cdk.App();
const clusterStack = new ClusterStack(app, 'ClusterStack');
new ServiceStack(app, 'ServiceStack', {
  clusterAttributes: clusterStack.clusterAttributes,
});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.93.0 (build 724bd01)

Framework Version

No response

Node.js Version

2.93.0

OS

Linux acm-envy3-win 5.15.90.4-microsoft-standard-WSL2 #1 SMP Tue Jul 18 21:28:32 UTC 2023 x86_64 GNU/Linux

Language

Typescript

Language Version

5.2.2

Other information

No response

@acomagu acomagu added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 1, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Sep 1, 2023
@acomagu
Copy link
Contributor Author

acomagu commented Sep 1, 2023

Solved by rewriting as follows:

diff a b
index ffd1f07..717d6d5 100644
--- a
+++ b
@@ -21,7 +21,10 @@ export class ClusterStack extends cdk.Stack {
       },
       vpc,
     });
-    this.clusterAttributes = cluster;
+    this.clusterAttributes = {
+      ...cluster,
+      securityGroups: cluster.connections.securityGroups,
+    };
   }
 }

I think it's better that there is a field like cluster.attributes where ClusterAttributes can be retrieved straightforwardly.

@acomagu acomagu closed this as completed Sep 1, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@acomagu
Copy link
Contributor Author

acomagu commented Nov 4, 2023

For anyone who got here: don't use spread notation(clusterAttribute = { ...cluster }) to copy properties. The my above code still doesn't work.

In JavaScript, class getter is not enumerable, so not copied by object spreading.

Assign all properties individually for safe:

this.clusterAttributes = {
  autoscalingGroup: cluster.autoscalingGroup,
  clusterArn: cluster.clusterArn,
  clusterName: cluster.clusterName,
  defaultCloudMapNamespace: cluster.defaultCloudMapNamespace,
  executeCommandConfiguration: cluster.executeCommandConfiguration,
  hasEc2Capacity: cluster.hasEc2Capacity,
  securityGroups: cluster.connections.securityGroups,
  vpc: cluster.vpc,
};

(I hope more convenient way to import cluster from another stack)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

1 participant