Auto-creating security group ingress can be problematic

[joeduffy]

If I specify a SecurityGroup explicitly for an ALB which already has all the ingress rules specified, the load balancer code then tries to create the same rules, and we get an error.

For example:

import * as awsx from "@pulumi/awsx";

// Create a security group to let traffic flow.
const sg = new awsx.ec2.SecurityGroup("web-sg", {
    vpc: awsx.ec2.Vpc.getDefault(),
    ingress: [{ protocol: "tcp", fromPort: 80, toPort: 80, cidrBlocks: [ "0.0.0.0/0" ] }],
    egress: [{ protocol: "-1", fromPort: 0, toPort: 0, cidrBlocks: [ "0.0.0.0/0" ] }],
});

// Create an ALB associated with the default VPC for this region and listen on port 80.
const nlb = new awsx.elasticloadbalancingv2.ApplicationLoadBalancer("web-traffic",
    { external: true, securityGroups: [ sg ] });
const listener = nlb.createListener("web-listener", { port: 80 });

This yields an error:

    error: Plan apply failed: [WARN] A duplicate Security Group rule was found on (sg-0616e60b838abac3a). This may be
    a side effect of a now-fixed Terraform issue causing two security groups with
    identical attributes but different source_security_group_ids to overwrite each
    other in the state. See https://github.com/hashicorp/terraform/pull/2376 for more
    information and instructions for recovery. Error message: the specified rule "peer: 0.0.0.0/0, TCP, from port: 80, to port: 80, ALLOW" already exists

I know how to fix this (just remove the explicit ingress rule), but it's a little unfortunate. For my example, I need to specify the egress otherwise the EC2 instance target fails health checks.

This could just be a wart we need to live with, but I figured I would file an issue so we can have the discussion and searcahability for the problem in case it bites someone else down the road.

[CyrusNajmabadi]

This was fixed. We specifically addressed this bit:

For my example, I need to specify the egress otherwise the EC2 instance target fails health checks.

which was just a bug in our impl.

[joeduffy]

@CyrusNajmabadi I just hit this again. I had a security group with ingress open to port 80 already, and then tried to add a load balancer to my program:

import * as aws from "@pulumi/aws";
import * as awsx from "@pulumi/awsx";
import * as pulumi from "@pulumi/pulumi";

const sg = new aws.ec2.SecurityGroup("web-secgrp", {
    ingress: [
        { protocol: "tcp", fromPort: 80, toPort: 80, cidrBlocks: ["0.0.0.0/0"] },
    ],
});

const alb = new awsx.elasticloadbalancingv2.ApplicationLoadBalancer("web-traffic", {
    external: true,
    securityGroups: [ sg.id ],
});
const listener = alb.createListener("web-listener", { port: 80 });

const ami = aws.getAmi({
    filters: [{ name: "name", values: ["amzn-ami-hvm-*-x86_64-ebs"] }],
    owners: [ "137112412989" ],
    mostRecent: true,
}).then(ami => ami.id);

export const ips: any[] = [];
export const hostnames: any[] = [];
for (const az of aws.getAvailabilityZones().names) {
    const server = new aws.ec2.Instance(`web-server-${az}`, {
        instanceType: "t2.micro",
        securityGroups: [ sg.name ],
        ami: ami,
        availabilityZone: az,
        userData: "#!/bin/bash\n"+
            `echo 'Hello, World -- from ${az}!' > index.html\n` +
            "nohup python -m SimpleHTTPServer 80 &",
        tags: { "Name": "web-server" },
    });
    ips.push(server.publicIp);
    hostnames.push(server.publicDns);

    alb.attachTarget(`web-target-${az}`, server);
}

The act of creating the listener attempted to create a new security group, which conflicted with my existing security group, leading to the following error:

  aws:ec2:SecurityGroupRule (web-listener-external-0-ingress):
    error: Plan apply failed: [WARN] A duplicate Security Group rule was found on (sg-0a9a0c57408cc5d9d). This may be
    a side effect of a now-fixed Terraform issue causing two security groups with
    identical attributes but different source_security_group_ids to overwrite each
    other in the state. See https://github.com/hashicorp/terraform/pull/2376 for more
    information and instructions for recovery. Error message: the specified rule "peer: 0.0.0.0/0, TCP, from port: 80, to port: 80, ALLOW" already exists

Should we provide an option for the createListener to not create a security group? (For what it's worth, I'd have assumed that would be the default...)

[CyrusNajmabadi]

So, the idea of the listeners is that they're supposed to figure this out for you, so you don't have ot manually do this stuff yourself. i.e. instead of writing the above, you should just be able to write:

const sg = new aws.ec2.SecurityGroup("web-secgrp", {
    ingress: [
        { protocol: "tcp", fromPort: 80, toPort: 80, cidrBlocks: ["0.0.0.0/0"] },
    ],
});

const alb = new awsx.elasticloadbalancingv2.ApplicationLoadBalancer("web-traffic", {
    external: true,
    securityGroups: [ sg.id ],
});

const alb = new awsx.elasticloadbalancingv2.ApplicationLoadBalancer("web-traffic", { external: true });
const listener = alb.createListener("web-listener", { port: 80, external: true });

Note: I agree there's a pain point here. Primarily when our "do it for you" then conflicts with your "but i went and did some of this manually myself". We can def try to be better here. For example, we could try to see what values you might have set for your SecurityGroup, and if you already have tehse rules setup, we wouldn't create new rules. This is a bit challenging though as checking all this information must be done synchronously, and we're currently in a bad place WRT synchronously checking resources for anything :-/

[damianh]

Just hit this issue too. (I don't know if validating a report from the wild is appreciated; let me know if not).

Also I noticed the generated security group for the load balance ingress rule is only for IPv4. I think it should include IPv6 as well.

[CyrusNajmabadi]

Just his this issue too. (I don't know if validating a report from the wild is appreciated; let me know if not).

It absolutely is! Thanks much!

[CyrusNajmabadi]

Note: the fundamental problem i think we have here (due to how we expose TF constructs) is that the SecurityGroup itself allows stating raw objects for ingress/egress rules and we also have a rich awsx object model where this is represented as SecurityGroupRule. Theses don't mix/match well. Indeed, TF itself has numerous issues where people have run into effectively the same problem.

IMO, we should consider scrapping the direct ingress/egress raw object properties on SecurityGroup. Instead, we can just say: use SecurityGroupRule instead.

Note: this doesn't mean you won't be declarative. Merely that you'd either write it like so:

const sg = new awsx.ec2.SecurityGroup("web-sg", {
    vpc: awsx.ec2.Vpc.getDefault(),
    ingress: [new SecurityGroupRule(...)],
    egress: [new SecurityGroupRule(...)],
});

or like:

const sg = new awsx.ec2.SecurityGroup("web-sg", {
    vpc: awsx.ec2.Vpc.getDefault(),
});
sg.addIngressRule(...);
sg.addEgressRule(...);

Either way you'd do this, you'd be using our rich objects, and we could make informed (and importantly, synchronous) decisions about waht should happen here.

[yuichiro12]

I really waiting for this. This issue prevent two Load Balancers sharing one SecurityGroup.

[ypresto]

AWS CDK had the same issue and resolved it with a mutable: false option on the imported security group.
aws/aws-cdk#1948

Maybe we could have a flag on ALB constructor to disable creating rules?

Also, currently, rules are created on every security group attached to LB, but perhaps it is unnecessary.

[ypresto]

IMO, awsx package is expected to hide SecurityGroup configuration (i.e. not to use passed security group) #361 (comment)

[c1moore]

I'm also running into this issue. I'm trying to create stricter security group rules than the ones pulumi generates, so I would like to completely disable this functionality.

[devgnx]

I needed to specify my own SecurityGroup configuration but, pulumi always returns that I have duplicate resources.

[rumsrami]

I am having the same problem, pulumi gives error with I try to create a listener for an Application ELB that is already assigned a security group. any update on this issue?

[MaxVynohradov]

The same issue right now, it painful enough..

[jeanlescure]

This issue is seriously causing problems for my company adopting Pulumi, and it is disheartening seeing this being a problem for over a year with not even a workaround proposed.

[jeanlescure]

There seems to be a deeper issue here, we tried just setting the SecurityGroup for the LoadBalancer (something we have done successfully before) and have two FargateService instances share the LoadBalancer and it failed with the exact same error.

[jeanlescure]

We destroyed the stack, then tried creating the LoadBalancer with NO SecurityGroups, pointed two FargateService instances with NO SecurityGroups to the LB and we got the same error.

[jeanlescure]

Here's a sample of what we are trying to achieve:

import * as awsx from '@pulumi/awsx';

const vpc = new awsx.ec2.Vpc('test-vpc', {
  vpcId: '...',
});

// LOAD BALANCER DEFINITION

const lb = new awsx.lb.ApplicationLoadBalancer('testLB', {
  vpc,
  subnets: [
    // ...
  ],
  external: true,
  tags: {
    'Name': 'testLB'
  },
});

// FIRST APP DEFINITION

const targetGroup1 = lb.createTargetGroup('testTG1', {
  port: 80,
  loadBalancer: lb,
  healthCheck: {
    path: '/api',
  },
  tags: {
    'Name': 'testTG1'
  },
});

const httpListener1 = lb.createListener('testHTTP1', {
  defaultActions: [{
    fixedResponse: {
      contentType: 'text/plain',
      statusCode: '404',
    },
    type: 'fixed-response',
  }],
  loadBalancer: lb,
  port: 80,
  protocol: 'HTTP',
  external: true,
});

httpListener1.addListenerRule('testListener1', {
  actions: [{
    targetGroupArn: targetGroup1.targetGroup.arn,
    type: 'forward',
  }],
  conditions: [
    {
      pathPattern: {
        values: [
          '/api/*',
        ],
      },
    },
  ],
  priority: 200,
});

new awsx.ecs.FargateService('api', {
  desiredCount: 1,
  subnets: [
    // ...
  ],
  taskDefinitionArgs: {
    tags: {
      'Name': 'api'
    },
    containers: {
      adminBackend: {
        image: '...',
        memory: 512,
        portMappings: [
          targetGroup1,
        ],
      },
    },
  },
});

// SECOND APP DEFINITION

const targetGroup2 = lb.createTargetGroup('testTG2', {
  port: 80,
  loadBalancer: lb,
  healthCheck: {
    path: '/ui',
  },
  tags: {
    'Name': 'testTG2'
  },
});

const httpListener2 = lb.createListener('testHTTP2', {
  defaultActions: [{
    fixedResponse: {
      contentType: 'text/plain',
      statusCode: '404',
    },
    type: 'fixed-response',
  }],
  loadBalancer: lb,
  port: 80,
  protocol: 'HTTP',
  external: true,
});

httpListener2.addListenerRule('testListener2', {
  actions: [{
    targetGroupArn: targetGroup2.targetGroup.arn,
    type: 'forward',
  }],
  conditions: [
    {
      pathPattern: {
        values: [
          '/*',
        ],
      },
    },
  ],
  priority: 200,
});

new awsx.ecs.FargateService('ui', {
  desiredCount: 1,
  subnets: [
    // ...
  ],
  taskDefinitionArgs: {
    tags: {
      'Name': 'ui'
    },
    containers: {
      adminBackend: {
        image: '...',
        memory: 512,
        portMappings: [
          targetGroup2,
        ],
      },
    },
  },
});

[jeanlescure]

A check needs to be created here to avoid generating duplicate ingress/egress rules:

https://github.com/pulumi/pulumi-awsx/blob/master/nodejs/awsx/lb/application.ts#L244-L248

[jeanlescure]

Just in case anyone comes looking for a workaround, we ended up commenting out these lines:

https://github.com/pulumi/pulumi-awsx/blob/master/nodejs/awsx/lb/application.ts#L244-L248

directly on the @pulumi/awsx installed on node_modules, less than ideal solution, but works for now while the Pulumi team gets around to fixing this.

[andyndang]

Any plan to address this? We keep encountering this and have to manually delete rules for pulumi to continue

[rocking42]

Our solution to this issue was to not use awsx for the listeners and just use the aws package. We then were able to specify the security groups without the listener creating its own ingress rules

    httpsListener = new aws.lb.Listener('name-https-listener', {
      loadBalancerArn: awsxLoadBalancer.loadBalancer.arn,
      port: 443,
      protocol,
      sslPolicy: "ELBSecurityPolicy-2016-08",
      certificateArn,
      defaultActions: [{
        type: "forward",
        targetGroupArn: awsxTg.targetGroup.arn,
      }],
    }, { parent: awsxLoadBalancer });

    const httpRedirectListener = new aws.lb.Listener('name-http-listener', {
      loadBalancerArn: awsxLoadBalancer.loadBalancer.arn,
      port: 80,
      protocol: "HTTP",
      defaultActions: [{
        type: "redirect",
        redirect: {
          port: "443",
          protocol: "HTTPS",
          statusCode: "HTTP_301",
        },
      }],
    }, { parent: awsxLoadBalancer });

[VikramVasudevan]

+1 this issue needs to be addressed

[emanuele-sibill]

Hi all, I've been having the same problem as above.

For now @jeanlescure solution (#293 (comment)) seems to work okay but it's not really stable in prod (have to use patch-package, troubles with CI/CD etc...)

@joeduffy do we have any suggestion from the Pulumi team?

The issue seems to have been fixed from the Terraform side of things (hashicorp/terraform#2376 has been merged

[baramuse]

just hit the issue today, so keeping it alive guys, still need to be addressed !

[catmeme]

I've run into this today, and it's not the first time. I'm going the path of unawsxing all our resource declarations. It's simply not worth the pain of going through abstractions that make some use cases so painful.

[JanErikFoss]

Definitely needs to be addressed

[zammitjames]

I've also just run into this. It definitely needs to be addressed!

[jkodroff]

This should not be an issue with AWSX 1.0 beta as the 1.0 version of an ALB does not attempt to create an ingress rule. The following program (as close to @joeduffy's original as possible) completes without error:

import * as awsx from "@pulumi/awsx";

// Create a security group to let traffic flow.
const sg = new awsx.classic.ec2.SecurityGroup("web-sg", {
  vpc: awsx.classic.ec2.Vpc.getDefault(),
  ingress: [{ protocol: "tcp", fromPort: 80, toPort: 80, cidrBlocks: ["0.0.0.0/0"] }],
  egress: [{ protocol: "-1", fromPort: 0, toPort: 0, cidrBlocks: ["0.0.0.0/0"] }],
});

// Create an ALB associated with the default VPC for this region and listen on port 80.
new awsx.lb.ApplicationLoadBalancer("web-traffic", {
  internal: false,
  securityGroups: [sg.id],
  listener: {
    port: 80
  }
});

Note that all pre-1.0 resources have been moved to the awsx.classic namespace so you can continue using them if you wish. The 1.0 version is available for use in all supported Pulumi languages.

To use the 1.0 beta version of AWSX from node:

npm install @pulumi/[email protected]

(Use similar commands for other languages.)

If you do use the 1.0 beta, we'd greatly appreciate your feedback!