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

[ECS] Why "securityGroups" fromClusterAttributes mandatory ? #11146

Closed
Cloudrage opened this issue Oct 27, 2020 · 12 comments · Fixed by #25976
Closed

[ECS] Why "securityGroups" fromClusterAttributes mandatory ? #11146

Cloudrage opened this issue Oct 27, 2020 · 12 comments · Fixed by #25976
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@Cloudrage
Copy link

When using ECS with EC2 instances (hosts), no pb; but when using Fargate, we don't have any Security Group associated with the container instances registered to the cluster, so why it's needed at the import ?

Reproduction Steps

For example, you create a Cluster in a Stack A :

    const EcsCluster = new ecs.Cluster(this, 'EcsCluster', {
      vpc: vpc,
      clusterName: EcsClusterName,
      containerInsights: true
    });

     const SecurityGroupEcsHost = new ec2.SecurityGroup(this, 'SecurityGroupEcsHost', {
       vpc: vpc,
       allowAllOutbound: true,
       description: 'Security Group for ECS Host'
     });

    AutoScalingGroupEcsHost.addSecurityGroup(SecurityGroupEcsHost);

    EcsCluster.addAutoScalingGroup(AutoScalingGroupEcsHost;

And you want to create an ECS EC2Service in another Stack B :

clusterName: EcsClusterName,
vpc,
securityGroups: [SecurityGroupEcsHost]
});

No pb at this time because on the first Stack, you have provided SGR & ASG resources for Hosts Instances.

What did you expect to happen?

But now, I want to create ECS Fargate resources on the other Stack, and the SGR is created on this one because associated with Fargate Service.

    const EcsClusterFargate = new ecs.Cluster(this, 'EcsClusterFargate ', {
      vpc: vpc,
      clusterName: EcsClusterFargateName,
      containerInsights: true
    });

But in that case, I can't import the dedicated Cluster like that :

const EcsClusterFargate = ecs.Cluster.fromClusterAttributes(this, 'EcsCluster', {
clusterName: EcsClusterFargateName,
vpc
});

So,, why it's mandatory ?
Do I have to attach a fake SGR ?

Environment

  • CLI Version : 1.68.0
  • Framework Version: 6.14.8
  • Node.js Version: v12.15.0
  • OS : Linux
  • Language (Version): TypeScript

This is 🐛 Bug Report

@Cloudrage Cloudrage added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 27, 2020
@Cloudrage
Copy link
Author

For a workaround, I've provided the Fargate SGR create on the other Stack "B" :

    const SecurityGroupEcsFargate = new ec2.SecurityGroup(this, 'SecurityGroupEcsFargate', {
      vpc: vpc,
      allowAllOutbound: true,
      description: 'Security Group for Fargate'
    });

        const EcsCluster = ecs.Cluster.fromClusterAttributes(this, 'EcsCluster', {
          clusterName: EcsClusterName,
          vpc,
          securityGroups: [SecurityGroupEcsFargate]
        });

Even if the SGR is, not the one associated with the container instances registered to the cluster :
https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ecs.ClusterAttributes.html

Maybe you can clarify the situation ? ^^

@SomayaB SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Oct 28, 2020
@MrArnoldPalmer
Copy link
Contributor

So, this is required because of the way the Cluster L2 construct is modeled. As detailed in this comment ecs clusters don't actually have security groups. This instances within them do, but if your cluster is purely fargate that isn't a thing.

This causes a couple of weird issues, including this one. I don't see an issue changing this to be an optional property, we just need to make sure we are checking for the presence of an SG on imported clusters wherever they are needed and throwing a nice error if it's not present.

@MrArnoldPalmer MrArnoldPalmer added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Dec 7, 2020
@mimozell
Copy link

I'm experiencing the same. It's a very stupid error. If you don't need security groups with aws ecs describe-clusters <clustername> why would you need it here? :(

@tscully49
Copy link

We are having the same issue in a different context. In cloudformation, creating a fargate serivice only requires The short name or full Amazon Resource Name (ARN) of the cluster on which to run your service. If you do not specify a cluster, the default cluster is assumed. (from https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-service.html) and I would expect that to have similar behavior here. Instead, for CDK, to create a FargateService construct, I have to find the cluster with this function by passing both the securityGroups and VPC, when it seems a cluster name or ARN should be fine.

@jordie23
Copy link
Contributor

Still a problem in 1.110.

So is the work around to just add a "fake" security group since it doesn't matter at all?

@nacitar
Copy link

nacitar commented Jul 8, 2021

Still a problem in 1.110.

So is the work around to just add a "fake" security group since it doesn't matter at all?

Seems to work in my tests, just to confirm.

EDIT: But an empty array, as mentioned in the next comment, is better.

@frankwese
Copy link

the securityGroups property can contain an empty array

const vpcId = StringParameter.valueFromLookup(this, `/${stage}/VpcId`);
const cluster = ecs.Cluster.fromClusterAttributes(this, 'ImportedCluster', {
      clusterName: cdk.Fn.importValue(`${stage}-ClusterName`),
      vpc: ec2.Vpc.fromLookup(this, 'VPC', { vpcId}),
      securityGroups: [],
    })

@ranma2913
Copy link

the securityGroups property can contain an empty array

const vpcId = StringParameter.valueFromLookup(this, `/${stage}/VpcId`);
const cluster = ecs.Cluster.fromClusterAttributes(this, 'ImportedCluster', {
      clusterName: cdk.Fn.importValue(`${stage}-ClusterName`),
      vpc: ec2.Vpc.fromLookup(this, 'VPC', { vpcId}),
      securityGroups: [],
    })

This fixed my lookup issue. Thanks so much!

@kaizencc kaizencc added p2 and removed p1 labels May 16, 2022
@github-actions github-actions bot added p1 and removed p2 labels Dec 18, 2022
@github-actions
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@madeline-k
Copy link
Contributor

There is now a fromClusterArn static method that may help with some of the issues mentioned in this thread. https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ecs.Cluster.html#static-fromwbrclusterwbrarnscope-id-clusterarn

We can still change securityGroups to be an optional property here. Especially since passing an empty array is accepted. That's not a very nice UX.

@corymhall corymhall added effort/small Small work item – less than a day of effort and removed effort/medium Medium work item – several days of effort labels May 2, 2023
@pksinghus
Copy link

There is now a fromClusterArn static method that may help with some of the issues mentioned in this thread. https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ecs.Cluster.html#static-fromwbrclusterwbrarnscope-id-clusterarn

Doesn't work. I get this Error: vpc is not available for a Cluster imported using fromClusterArn(), please use fromClusterAttributes() instead. . This continues to be a problem.

@mergify mergify bot closed this as completed in #25976 Jun 15, 2023
mergify bot pushed a commit that referenced this issue Jun 15, 2023
The `securityGroups` is passed down to create a new `ec2.Connections`, where this property is already optional. Making it optional in `fromClusterAttributes` as well.


Closes #11146 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.