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

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

Closed
denis-ryzhkov opened this issue Oct 27, 2021 · 13 comments
Closed

Comments

@denis-ryzhkov
Copy link
Contributor

denis-ryzhkov commented Oct 27, 2021

@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
Copy link
Member

gwdp commented Oct 28, 2021

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 (

private generateSecurityGroupsByService(service: Service): any {
), 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
Copy link
Contributor Author

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:

"GroupDescription": `Access to the public facing load balancer - task ${service.getName(NamePostFix.SERVICE)}`,

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:

"Description": `Ingress from the ALB - task ${service.getName(NamePostFix.SERVICE)}`,

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
Copy link
Member

gwdp commented Nov 1, 2021

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

...(!this.cluster.getVPC().useExistingVPC() &&
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
Copy link
Contributor Author

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 added a commit that referenced this issue Nov 18, 2021
@gwdp
Copy link
Member

gwdp commented Nov 18, 2021

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

@denis-ryzhkov
Copy link
Contributor Author

@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 added a commit that referenced this issue Nov 18, 2021
…he shared cluster (which is responsible for it). [related to #2]
@gwdp
Copy link
Member

gwdp commented Nov 18, 2021

@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
Copy link
Contributor Author

@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 added a commit that referenced this issue Nov 19, 2021
@gwdp
Copy link
Member

gwdp commented Nov 19, 2021

@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
Copy link
Contributor Author

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

@gwdp
Copy link
Member

gwdp commented Nov 22, 2021

Awesome, releasing 0.0.21 tonight

@denis-ryzhkov
Copy link
Contributor Author

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

@gwdp
Copy link
Member

gwdp commented Dec 6, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants