fixing some edge cases with AWS SG Rules

[nevins-b]

This fixes two possible issues:

1. If source_security_group_id and self=true are specified then UserIDGroupPairs will have a length of 2 so UserIDGroupPairs[0] can not be used.
2. The UserIDGroupPairs need to be included in the hash to avoid collision with another rule in the same SG.

[nevins-b]

hmm this actually doesn't work as expected. It seems self=True and security_group_id are mutually exclusive. If they are both set then no single permission will match so it won't be able to find the perm after creation.

[catsby]

Hey @nevins-b – is there a sample config I can use to see this? Omit secrets of course :)

I modified the title to reflect that this is in Security Group Rules. Please correct me if I'm mistaken.

[nevins-b]

@catsby Here's the smallest example I could come up with:

variable "access_key" {}
variable "secret_key" {}

variable "aws_region" {
    default = "us-west-2"
}

provider "aws" {
    access_key = "${var.access_key}"
    secret_key = "${var.secret_key}"
    region = "${var.aws_region}"
}

resource "aws_vpc" "test" {
    cidr_block = "${var.vpc_cidr}"
    enable_dns_support = true
    enable_dns_hostnames = true
}

resource "aws_security_group" "guineapig" {
  name = "guineapig"
  vpc_id = "${aws_vpc.test.id}"
  description = "Just used as peer"
}

resource "aws_security_group" "broken" {
  name = "Broken"
  vpc_id = "${aws_vpc.test.id}"
  description = "This security group has rule issues"
}

resource "aws_security_group_rule" "the_issue" {
  security_group_id = "${aws_security_group.broken.id}"
  source_security_group_id = "${aws_security_group.guineapig.id}"

  type = "ingress"
  from_port = 80
  to_port = 80
  protocol = "tcp"
  self = true
}

[CpuID]

Could this issue potentially be causing behaviour like this? Effectively the replacement of an existing SG rule with the same thing again, when using self=true?

~ aws_security_group.elasticsearch_logstash
    ingress.#:                            "2" => "3"
    ingress.1265039627.cidr_blocks.#:     "0" => "0"
    ingress.1265039627.from_port:         "9300" => "9300"
    ingress.1265039627.protocol:          "tcp" => "tcp"
    ingress.1265039627.security_groups.#: "0" => "0"
    ingress.1265039627.self:              "1" => "1"
    ingress.1265039627.to_port:           "9300" => "9300"
    ingress.1489741676.cidr_blocks.#:     "1" => "0"
    ingress.1489741676.cidr_blocks.0:     "X.Y.0.0/16" => ""
    ingress.1489741676.from_port:         "9200" => "0"
    ingress.1489741676.protocol:          "tcp" => ""
    ingress.1489741676.security_groups.#: "0" => "0"
    ingress.1489741676.self:              "1" => "0"
    ingress.1489741676.to_port:           "9200" => "0"
    ingress.2040698479.cidr_blocks.#:     "0" => "0"
    ingress.2040698479.from_port:         "" => "9200"
    ingress.2040698479.protocol:          "" => "tcp"
    ingress.2040698479.security_groups.#: "0" => "0"
    ingress.2040698479.self:              "" => "1"
    ingress.2040698479.to_port:           "" => "9200"
    ingress.899788117.cidr_blocks.#:      "0" => "1"
    ingress.899788117.cidr_blocks.0:      "" => "X.Y.0.0/16"
    ingress.899788117.from_port:          "" => "9200"
    ingress.899788117.protocol:           "" => "tcp"
    ingress.899788117.security_groups.#:  "0" => "0"
    ingress.899788117.self:               "" => "0"
    ingress.899788117.to_port:            "" => "9200"

Seems like the X.Y.0.0/16 rule is being removed and readded effectively...

[CpuID]

Just a note this patch will need some manual tweaks before it can be considered for merging.

I did manage to tweak it locally to get it working (can post an updated PR on request), but it fails 2 acceptance tests:

2015/09/04 19:49:38 [DEBUG] Replacing octal \052 for * in: \052.nonexample.com
2015/09/04 19:49:38 [INFO] Found AWS Security Group State v0; migrating to v1
2015/09/04 19:49:38 [DEBUG] Attributes before migration: map[string]string{"type":"ingress", "protocol":"-1", "from_port":"0", "source_security_group_id":"sg-11877275", "self":"false", "to_port":"0", "security_group_id":"sg-13877277", "cidr_blocks.#":"0"}
2015/09/04 19:49:38 [DEBUG] Attributes after migration: map[string]string{"self":"false", "cidr_blocks.#":"0", "from_port":"0", "source_security_group_id":"sg-11877275", "id":"sg-2699277411", "to_port":"0", "security_group_id":"sg-13877277", "type":"ingress", "protocol":"-1"}, new id: sg-2699277411
--- FAIL: TestAWSSecurityGroupRuleMigrateState (0.00s)
    resource_aws_security_group_rule_migrate_test.go:64: bad sg rule id: sg-2699277411

         expected: sg-3766347571
--- FAIL: TestIpPermissionIDHash (0.00s)
    resource_aws_security_group_rule_test.go:103: input: egress - {
          FromPort: 80,
          IpProtocol: "tcp",
          ToPort: 8000,
          UserIdGroupPairs: [{
              GroupId: "sg-12345678",
              UserId: "987654321"
            },{
              GroupId: "sg-12345678",
              UserId: "123456789"
            },{
              GroupId: "sg-987654321",
              UserId: "123456789"
            }]
        }
        output: sg-1045157801
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x132ecd]

[radeksimko]

@catsby I reckon you plan to fix this via #3019 - i.e. this issue can be closed, right?

[josephholsten]

@nevins-b does this seem like it would be solved by #3019?

[phinze]

Going to close as I believe this is superseded by #3019

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.