Support Descriptions for Security Group Rules

[thomasbiddle]

Finally! AWS has added support for description of individual rules within security groups! This would be an amazing addition for Terraform:

https://aws.amazon.com/blogs/aws/new-descriptions-for-security-group-rules/

Terraform Version

Terraform v0.10.2

Affected Resource(s)

Please list the resources as a list, for example:

• security_group
• security_group_rule

Possible implementations of this could be having a description attribute for these resources where appropriate. The security_group already has a description attribute, so this would be implemented specifically within the ingress block.

Terraform Configuration Files

resource "aws_security_group_rule" "allow_all" {
  type            = "ingress"
  from_port       = 0
  to_port         = 65535
  protocol        = "tcp"
  cidr_blocks     = ["0.0.0.0/0"]
  prefix_list_ids = ["pl-12c4e678"]

  description = "Description of this rule"
  security_group_id = "sg-123456"
}

resource "aws_security_group" "allow_all" {
  name        = "allow_all"
  description = "Allow all inbound traffic"

  ingress {
    from_port   = 0
    to_port     = 65535
    protocol    = "tcp"
    cidr_blocks = ["0.0.0.0/0"]

    description = "Description of this rule"
  }
}

[atkinsonm]

It doesn't look like the Go SDK supports descriptions for ingress/egress yet: http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#AuthorizeSecurityGroupIngressInput, http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#AuthorizeSecurityGroupEgressInput.

[jnahelou]

Following documentation http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#IpRange

Expected Behavior

resource "aws_security_group_rule" "auto-rule" {
  type      = "ingress"
  from_port = "443"
  to_port   = "443"
  protocol  = "TCP"

  cidr_blocks = [{
    CidrIp = "0.0.0.0/0"
    Description = "Foo"
  }]

  security_group_id = "${aws_security_group.foo-editable.id}"
}

Actual Behavior

1 error(s) occurred:

* aws_security_group_rule.auto-rule: cidr_blocks.0 must be a single value, not a map

"cidr_blocks": {
  Type:     schema.TypeList,
  Optional: true,
  Elem: &schema.Schema{
    Type:         schema.TypeString,
    ValidateFunc: validateCIDRNetworkAddress,
  },
},

[pdecat]

There is a set of separate APIs for managing security group rules descriptions :

• http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.UpdateSecurityGroupRuleDescriptionsEgress
• http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.UpdateSecurityGroupRuleDescriptionsEgressRequest
• http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.UpdateSecurityGroupRuleDescriptionsEgressWithContext
• http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.UpdateSecurityGroupRuleDescriptionsIngress
• http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.UpdateSecurityGroupRuleDescriptionsIngressRequest
• http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#EC2.UpdateSecurityGroupRuleDescriptionsIngressWithContext

[joelthompson]

There is a set of separate APIs for managing security group rules descriptions

No, you should be able to do it directly with AuthorizeSecurityGroupIngress/Egress; it's just embedded deeper into their model of IpPermissions, e.g., http://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#IpRange

[joelthompson]

I started looking into potentially implementing this, but it looks messier than I thought. @thomasbiddle's suggestion doesn't align cleanlly with the AWS data model because he has the description on the aws_security_group_rule resource, while in AWS, the description is actually on the individual CIDR blocks or prefix lists.

A more "natural" way (as in, consistent with the AWS data models) of implementing this would be to have the TF look like:

resource "aws_security_group_rule" "allow_all" {
  type            = "ingress"
  from_port       = 0
  to_port         = 65535
  protocol        = "tcp"
  cidr_blocks     = [
    {
      cidr = "0.0.0.0/0"
      description = "the entire intrawebz"
    }
  ]
  prefix_list_ids = [
    {
      prefix_list = "pl-12c4e678"
      description = "what is this pl anyway?"
    }
  ]
  description = "Description of this rule"
  security_group_id = "sg-123456"
}

But that's now a breaking change in the HCL for a security group rule. This could be worked around by specifying a unique aws_security_group_rule resource (or unique ingress/egress block) for every unique description, but this feels pretty ugly to me as it'll require a lot of duplication.

Is there a clean way to have something the elements of cidr_blocks be either a schema.TypeString OR a schema.Resource depending on what's specified? That would probably be the easiest.

Alternatively, we could have cidr_blocks_with_descriptions and prefix_list_ids_with_descriptions that support this structure, but that seems really gross.

[ewbankkit]

@joelthompson IMHO the Terraform aws_security_group_rule resource (and inline rules in the aws_security_group resource) don't map 100% cleanly to the underlying AWS SG ingress/egress resources (a single ingress or egress can be represented by multiple aws_security_group_rules). We've found that the most maintainable (in the face of refactoring) and scalable solution is to just have a single (IPV6) CIDR block or PL or peer SG per aws_security_group_rule resource (maybe with an additional count attribute if we have a list of CIDRs). This seems to map to what AWS calls "Security Group Rule"s in the console. In this case a single description attribute per rule seems workable.
Comments welcome on #1587.

[sshvetsov]

@ewbankkit thanks for the patch!
I wonder however, how to go about implementing the breaking change to aws_security_group_rule and aws_security_group, which would allow us to set description per source (ipRange, ipv6Range, PrefixListId, UserIdGroupPair) like AWS API:

{
    "FromPort": integer,
    "IpProtocol": "string",
    "IpRanges": [
        {
            "CidrIp": "string",
            "Description": "string"
        }
        ...
    ],
    "Ipv6Ranges": [
        {
            "CidrIpv6": "string",
            "Description": "string"
        }
        ...
    ],
    "PrefixListIds": [
        {
            "Description": "string",
            "PrefixListId": "string"
        }
        ...
    ],
    "ToPort": integer,
    "UserIdGroupPairs": [
        {
            "Description": "string",
            "GroupId": "string",
            "GroupName": "string",
            "PeeringStatus": "string",
            "UserId": "string",
            "VpcId": "string",
            "VpcPeeringConnectionId": "string"
        }
        ...
    ]
}

[aprice]

Why not just allow duplication for those who want to use different descriptions for the same rule for different sources?

resource "aws_security_group_rule" "allow_all" {
  type            = "ingress"
  from_port       = 0
  to_port         = 65535
  protocol        = "tcp"
  cidr_blocks     = ["0.0.0.0/0"]
  prefix_list_ids = ["pl-12c4e678"]

  description = "Description of this rule"
  security_group_id = "sg-123456"
}

resource "aws_security_group" "allow_all" {
  name        = "allow_all"
  description = "Allow all inbound traffic"

  ingress {
    from_port   = 0
    to_port     = 65535
    protocol    = "tcp"
    cidr_blocks = ["1.2.3.4/32"]

    description = "Steve's PC"
  }

  ingress {
    from_port   = 0
    to_port     = 65535
    protocol    = "tcp"
    cidr_blocks = ["4.3.2.1/32"]

    description = "Jane's PC"
  }
}

Yes, you have to duplicate the whole block for each target you want to have a different description, but it at least lets you manage descriptions if you choose to without breaking compatibility in the configuration format. If you want the same description for several targets for the same rule, you can do that with a single block.

[cliefooghe]

With last terraform version( 0.10.5), i try to use description for security groupe rules, but an error occured:

1 error(s) occurred:

• aws_security_group.allow_all: ingress.0: invalid or unknown key: description

[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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!