pulumi / pulumi-awsx

AWS infrastructure best practices in component form!
https://www.pulumi.com/docs/guides/crosswalk/aws/
Apache License 2.0
219 stars 104 forks source link

Wrong outbound port set on ALB security group #663

Open magnattic opened 3 years ago

magnattic commented 3 years ago

I am setting up an ALB with a HTTPS listener that forwards to a HTTP target in ECS. But awsx generates the wrong security group rules, which causes the health checks to fail.

This is my code:

const caddyTargetGroup = alb.createTargetGroup('caddy', {
  vpc,
  port: 80,
  protocol: 'HTTP',
});
const caddyHttpsListener = caddyTargetGroup.createListener(
  'caddyHttps',
  { protocol: 'HTTPS', port: 443, certificateArn: certificate.arn },
  { dependsOn: [domainVerificationRecord] }
); 

The created security group for the ALB allow inbound and outbound traffic on port 443, but I need outbound traffic on port 80 obviously, or the listener cannot reach the container on port 80. When I change this by hand to port 80 in the SG it works.

Expected behavior

The created security group should allow outbound traffic on the port of the target group, not the listener port.

Current behavior

The wrong outbound port is set.

leezen commented 3 years ago

@magnattic I believe you should specify what the TargetGroup actually targets (e.g. ECS Service) and you would create a SecurityGroup for that target such that port 80 in/out is allowed.

magnattic commented 3 years ago

I am passing the target group to an ECS service like this:

const frontend = new awsx.ecs.FargateService(
  'frontend',
  {
    cluster,
    taskDefinitionArgs: {
      containers: {
        caddy: {
          image: awsx.ecs.Image.fromPath(caddyRepo, './caddy/'),
          portMappings: [caddyTargetGroup],
        },
   [...]

So are you saying that I have to create an additional ALB security group for each target manually? I was under the impression that pulumi crosswalk would do this automatically. Is this mentioned in the docs somewhere?

Why is pulumi creating an outbound rule on port 443? From what I gather this is unnecessary.

leezen commented 3 years ago

I would expect to see a security group that allows traffic on 443 to/from the load balancer (this comes from https://github.com/pulumi/pulumi-awsx/blob/99678b954766dbca4aec53419568afc7e4b940ad/nodejs/awsx/lb/application.ts#L231). Separately, I would expect you to need to allow ingress to the service itself, which might look something like this:

// Setup a security group to allow ingress.
const serviceSecurityGroup = new aws.ec2.SecurityGroup("service-sg", {
    vpcId: vpc.id,
    ingress: [{
        fromPort: 80,
        toPort: 80,
        protocol: "tcp",
        cidrBlocks: ["0.0.0.0/0"],
    }],
    egress: [{
        fromPort: 0,
        toPort: 0,
        protocol: "-1",
        cidrBlocks: ["0.0.0.0/0"],
    }],
});

const frontend = new awsx.ecs.FargateService(
  'frontend',
  securityGroups: [serviceSecurityGroup.id],
  {
    cluster,
    taskDefinitionArgs: {
      containers: {
        caddy: {
          image: awsx.ecs.Image.fromPath(caddyRepo, './caddy/'),
          portMappings: [caddyTargetGroup],
        },
   [...]
magnattic commented 3 years ago

I think the docs you linked assume in the text at the top that the port will be the same coming in and going out of the load balancer. Under Recommended rules it specifies more clearly:

Inbound

Source | Port Range | Comment 0.0.0.0/0 | listener | Allow all inbound traffic on the load balancer listener port

Outbound

Destination | Port Range | Comment instance security group | instance listener | Allow outbound traffic to instances on the instance listener port instance security group | health check | Allow outbound traffic to instances on the health check port

So inbound it should allow the ALB listener port, but outbound it should allow the instance listener port. Otherwise it won't work. In my case the listener port is 443 but the instance listener port is 80.

Separately, I would expect you to need to allow ingress to the service itself

This is also necessary, but in my case pulumi already has set a security group on the service that allows inbound traffic to the service. So this is not the issue. Even if the service allows inbound traffic on port 80, if the ALB is not allowed to send traffic on port 80 it will fail. At least that's what I think is happening in my case.

I still believe this is a bug because pulumi sets the wrong outbound rules. If I change the outbound port on the ALB security group to the port of the target group, it works as expected. I don't think the ALB needs the outbound port 443 if it is only listening to HTTPS but not forwarding to HTTPS.

tlinhart commented 3 years ago

I've just encountered the same issue as described. My ECS tasks in the target group are listening on port 1337 while I want my ALB listener to listen on port 80. The ALB security group being created by crosswalk has however outbound rule allowing traffic to 80 (the listener port), not 1337 (the target group). That's why health checks are failing.

I believe this is the relevant part of the code that is handling the security group updates but is working with the incorrect port here: https://github.com/pulumi/pulumi-awsx/blob/25aa0c836bad05aab343bc9508f69e57f3ed5288/nodejs/awsx/lb/application.ts#L237

lintong commented 1 year ago

I've just encountered the same issue as described. My ECS tasks in the target group are listening on port 1337 while I want my ALB listener to listen on port 80. The ALB security group being created by crosswalk has however outbound rule allowing traffic to 80 (the listener port), not 1337 (the target group). That's why health checks are failing.

I believe this is the relevant part of the code that is handling the security group updates but is working with the incorrect port here:

https://github.com/pulumi/pulumi-awsx/blob/25aa0c836bad05aab343bc9508f69e57f3ed5288/nodejs/awsx/lb/application.ts#L237

A side-effect to this code is that a pulumi refresh detects a delta in the any self-managed SGs attached to the ALB and removes access on 443 on the following pulumi up.

danielrbradley commented 1 year ago

Just circling back round on this. I think this is resolved in the new v1 application load balancer component as it currently allows all ports on the security group, but also allows you to define your own security group args or provider an existing security group too.

This issue therefore sits within the "classic" namespace which will be moved into a maintanance-only mode at the point of the 1.0.0 release.

One workaround for the classic TypeScript components is to customise this behaviour using stack tranformations which allow you to intercept and modify property values.