Missing security group to allow ALB to access ECS/Fargate Service

denis-ryzhkov commented 3 years ago

@gwdp, thank you for supporting this plugin!

My ECS/Fargate Task had "Stopped reason: Task failed ELB health checks" because AWS::ECS::Service created by this plugin did not have a security group to allow ALB to access this Service.

Please replace the workaround below with proper fix of the plugin code:

resources:
  Resources:

    AlbToServiceSecGroup:
      Type: AWS::EC2::SecurityGroup
      Properties:
        GroupDescription: "Allows ALB to access ECS Service"
        VpcId: REDACTED

    AlbToServiceSecGroupIngress:
      Type: AWS::EC2::SecurityGroupIngress
      DependsOn:
      - ${self:custom.albSecGroupResourceName}
      Properties:
        GroupId: !Ref AlbToServiceSecGroup
        SourceSecurityGroupId: !Ref ${self:custom.albSecGroupResourceName}
        IpProtocol: -1

custom:
    albSecGroupResourceName: REDACTED_ServerlessService_Stage_ALBSecGroup_Stage_EcsService
    # Security group created by serverless-ecs-plugin for ALB

ecs:
- vpc:
    securityGroupIds:
    - !Ref AlbToServiceSecGroup

Even with this workaround, all securityGroupIds (AlbToServiceSecGroup and e.g. RdsClientSecGroup) are applied both to AWS::ECS::Service (OK) and to ALB, which does not need these security groups.

Please implement additional serviceSecurityGroupIds option with a list of security groups for AWS::ECS::Service only.

gwdp commented 3 years ago

Hello @denis-ryzhkov,

I see you are defining your own VPC outside of the plugin.

The plugin takes care of the groups needed to make everything work when it's creating its own VPC (https://github.com/Hybridless/serverless-ecs-plugin/blob/6a965c00477140d366a976f4efab92b28f31ee14/src/resources/loadBalancer.ts#L83), otherwise, you are fully in charge of handling the security groups outside of the plugin, since there are multiple use cases that could not need that. I recognize this is not explicitly on the documentation and will improve it and document the required sec groups when you are using a user-defined VPC. (PR is very welcome if you have time for it :) )

If you are using the VPC, only for this service and it's okay for you to let the plugin manage it, I would strongly recommend to defined the VPC CIRD and subnets and letting the plugin do the rest.

denis-ryzhkov commented 3 years ago

Thank you for reply, @gwdp!

Could you please give an example of use case you mentioned, when this plugin creates ALB, creates ECS Service, but does not create a security group to allow this ALB to access this Service, and this behavior is desired by the user?

Even with VPC defined outside of the plugin, the plugin creates another security group anyway: https://github.com/Hybridless/serverless-ecs-plugin/blob/6a965c00477140d366a976f4efab92b28f31ee14/src/resources/loadBalancer.ts#L95

So it is not the "create no security groups at all" case, when we use external VPC. Then why not to create the missing security group too? It just connects two resources (ALB and Service) created by the plugin, it is needed in all cases I see (please prove me wrong), it requires a complex workaround above to create it outside of the plugin, as it references both ALB and the Service created inside the plugin.

And, it is already implemented: https://github.com/Hybridless/serverless-ecs-plugin/blob/6a965c00477140d366a976f4efab92b28f31ee14/src/resources/loadBalancer.ts#L117

Please just make it available for external VPC case, at least with option allowAlbToAccessService: false by default - I wonder who would keep it false and why :)

gwdp commented 3 years ago

Hey @denis-ryzhkov, sorry for the delay.

Until now, there were 2 use cases for the plugin, first, you let the plugin create the VPC and ALB for you, second, you create the VPC and the ALB on your own and specify for the plugin to use.

Correct me if wrong, but you are actually using on a third use case, you create the VPC but not the ALB, which is a case I haven't predicted and you are absolutely right on relying on the plugin to create everything is required for that ALB to work on your defined VPC.

If this assumption is correct, I see the plugin is checking for useExistingVPC on https://github.com/Hybridless/serverless-ecs-plugin/blob/6a965c00477140d366a976f4efab92b28f31ee14/src/resources/loadBalancer.ts#L111 which actually should be checking if the ALB is user-defined or not, not the VPC.

I might need to check the whole plugin for this rule and check if there isn't everything else that needs to be changed in order to support self-defined VPC but auto-created ALB.

Please, confirm to me if this is the case, if so allow me some time (probably this week), to modify it and release and a beta version for you to test it out.

This is a case we certainly want to support :)

denis-ryzhkov commented 3 years ago

You are absolutely right, @gwdp!

I do use existing VPC and I configure the plugin to create both ECS Service and ALB.

And I definitely want the plugin to create the missing security group to allow this new ALB to access this new ECS Service.

Eager to test the new plugin version, please keep me posted!

gwdp commented 2 years ago

@denis-ryzhkov sorry for taking too long.. Could you give 0.0.21-alpha2 a try, please?

denis-ryzhkov commented 2 years ago

@gwdp thank you!

I can confirm that ALB[ToService]SecGroupIngress is created by the plugin now:

"REDACTED_SLS_SERVICE_STAGE_ALBSecGroup_STAGE_ECS_SERVICE_ALBSecGroupIngress": {
  "Type": "AWS::EC2::SecurityGroupIngress",
  "DeletionPolicy": "Delete",
  "Properties": {
    "Description": "Ingress from the ALB - task REDACTED_SLS_SERVICE_STAGE_ECS_SERVICE_Service_STAGE",
    "GroupId": {
      "Ref": "REDACTED_SLS_SERVICE_STAGE_ContainerSecGroup_STAGE"
    },
    "IpProtocol": -1,
    "SourceSecurityGroupId": {
      "Ref": "REDACTED_SLS_SERVICE_STAGE_ALBSecGroup_STAGE_ECS_SERVICE"
    }
  }
},

However, the ...ContainerSecGroup..., this ingress should be attached to is not present in the generated .serverless/cloudformation-template-update-stack.json, resulting in the expected error on sls deploy:

Error: The CloudFormation template is invalid:
Template format error: Unresolved resource dependencies
[REDACTED_SLS_SERVICE_STAGE_ContainerSecGroup_STAGE]
in the Resources block of the template

I guess this sec group is already created by the plugin and attached to AWS::ECS::Service.Properties.NetworkConfiguration.AwsvpcConfiguration.SecurityGroups, just in another use case.

Please enable creation of this sec group for our use case too. Once you fix it, please let me know, I'm eager to test it.

P.S. I'd propose better naming, but if it blocks delivery, please skip it:

ContainerSecGroup --> EcsServiceSecGroup
ALBSecGroupIngress --> AlbToEcsServiceSecGroupIngress because ALBSecGroupIngress may be read as "ingress of ALBSecGroup", which it is not

gwdp commented 2 years ago

@denis-ryzhkov Thanks for the testing and feedback again!!

Made the fix and updated the resources constants.. Still, haven't included exactly what you proposed (one to maintain small names (due CF limit) and second keep the 'ECS' all caps standard used on code :) )

Changed are available on 0.0.21-beta1, let me know if you encounter any issues with it. New auto-scaling feature is probably ready for use as well.

denis-ryzhkov commented 2 years ago

@gwdp thank you!

When I check the diff of .serverless/cloudformation-template-update-stack.json created by sls package with my workarounds vs 0.0.21-beta1 and no workarounds, I see:

ECSServiceSecGroup and its IngressSelf are created - great!
ALBToECSSecGroupIngress.Properties.SourceSecurityGroupId.Ref: ECSServiceSecGroup - great!
My AlbToServiceSecGroup is deleted from AWS::ECS::Service...SecurityGroups - ok, but your ECSServiceSecGroup is not added there instead :(

Please attach ECSServiceSecGroup to AWS::ECS::Service so that: AWS::ECS::Service.Properties.NetworkConfiguration.AwsvpcConfiguration.SecurityGroups[].Ref: ECSServiceSecGroup

Once you fix it, please let me know, I'm eager to test it.

gwdp commented 2 years ago

@denis-ryzhkov My bad, I was in doubt about this sec. group.. Fixed now, third time is the charm :) Available at 0.0.21-beta9

denis-ryzhkov commented 2 years ago

@gwdp it works, finally! Thanks a lot, the issue is closed.

gwdp commented 2 years ago

Awesome, releasing 0.0.21 tonight

denis-ryzhkov commented 2 years ago

hey @gwdp how are you? just a friendly reminder to release 0.0.21

gwdp commented 2 years ago

@denis-ryzhkov Done earlier today. Sorry for the wait :)

Hybridless / serverless-ecs-plugin

Missing security group to allow ALB to access ECS/Fargate Service #2