Skip to content

Commit

Permalink
Merge pull request #2376 from hashicorp/fix-security-group-rule-hashi…
Browse files Browse the repository at this point in the history
…ng-with-source-sg

Consider security groups with source security groups when hashing
  • Loading branch information
catsby committed Jun 19, 2015
2 parents 2410824 + 44eb55f commit f045e9e
Show file tree
Hide file tree
Showing 4 changed files with 279 additions and 27 deletions.
86 changes: 64 additions & 22 deletions builtin/providers/aws/resource_aws_security_group_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ func resourceAwsSecurityGroupRule() *schema.Resource {
Read: resourceAwsSecurityGroupRuleRead,
Delete: resourceAwsSecurityGroupRuleDelete,

SchemaVersion: 1,
MigrateState: resourceAwsSecurityGroupRuleMigrateState,

Schema: map[string]*schema.Schema{
"type": &schema.Schema{
Type: schema.TypeString,
Expand Down Expand Up @@ -61,10 +64,11 @@ func resourceAwsSecurityGroupRule() *schema.Resource {
},

"source_security_group_id": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Computed: true,
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Computed: true,
ConflictsWith: []string{"cidr_blocks"},
},

"self": &schema.Schema{
Expand All @@ -90,6 +94,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{}

ruleType := d.Get("type").(string)

var autherr error
switch ruleType {
case "ingress":
log.Printf("[DEBUG] Authorizing security group %s %s rule: %s",
Expand All @@ -105,13 +110,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{}
req.GroupName = sg.GroupName
}

_, err := conn.AuthorizeSecurityGroupIngress(req)

if err != nil {
return fmt.Errorf(
"Error authorizing security group %s rules: %s",
"rules", err)
}
_, autherr = conn.AuthorizeSecurityGroupIngress(req)

case "egress":
log.Printf("[DEBUG] Authorizing security group %s %s rule: %#v",
Expand All @@ -122,18 +121,28 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{}
IPPermissions: []*ec2.IPPermission{perm},
}

_, err = conn.AuthorizeSecurityGroupEgress(req)

if err != nil {
return fmt.Errorf(
"Error authorizing security group %s rules: %s",
"rules", err)
}
_, autherr = conn.AuthorizeSecurityGroupEgress(req)

default:
return fmt.Errorf("Security Group Rule must be type 'ingress' or type 'egress'")
}

if autherr != nil {
if awsErr, ok := autherr.(awserr.Error); ok {
if awsErr.Code() == "InvalidPermission.Duplicate" {
return fmt.Errorf(`[WARN] A duplicate Security Group rule was found. 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: %s`, awsErr.Message())
}
}

return fmt.Errorf(
"Error authorizing security group rule type %s: %s",
ruleType, autherr)
}

d.SetId(ipPermissionIDHash(ruleType, perm))

return resourceAwsSecurityGroupRuleRead(d, meta)
Expand Down Expand Up @@ -263,15 +272,32 @@ func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, er
return resp.SecurityGroups[0], nil
}

// ByGroupPair implements sort.Interface for []*ec2.UserIDGroupPairs based on
// GroupID or GroupName field (only one should be set).
type ByGroupPair []*ec2.UserIDGroupPair

func (b ByGroupPair) Len() int { return len(b) }
func (b ByGroupPair) Swap(i, j int) { b[i], b[j] = b[j], b[i] }
func (b ByGroupPair) Less(i, j int) bool {
if b[i].GroupID != nil && b[j].GroupID != nil {
return *b[i].GroupID < *b[j].GroupID
}
if b[i].GroupName != nil && b[j].GroupName != nil {
return *b[i].GroupName < *b[j].GroupName
}

panic("mismatched security group rules, may be a terraform bug")
}

func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string {
var buf bytes.Buffer
// for egress rules, an TCP rule of -1 is automatically added, in which case
// the to and from ports will be nil. We don't record this rule locally.
if ip.IPProtocol != nil && *ip.IPProtocol != "-1" {
if ip.FromPort != nil && *ip.FromPort > 0 {
buf.WriteString(fmt.Sprintf("%d-", *ip.FromPort))
}
if ip.ToPort != nil && *ip.ToPort > 0 {
buf.WriteString(fmt.Sprintf("%d-", *ip.ToPort))
buf.WriteString(fmt.Sprintf("%s-", *ip.IPProtocol))
}
buf.WriteString(fmt.Sprintf("%s-", *ip.IPProtocol))
buf.WriteString(fmt.Sprintf("%s-", ruleType))

// We need to make sure to sort the strings below so that we always
Expand All @@ -288,6 +314,22 @@ func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string {
}
}

if len(ip.UserIDGroupPairs) > 0 {
sort.Sort(ByGroupPair(ip.UserIDGroupPairs))
for _, pair := range ip.UserIDGroupPairs {
if pair.GroupID != nil {
buf.WriteString(fmt.Sprintf("%s-", *pair.GroupID))
} else {
buf.WriteString("-")
}
if pair.GroupName != nil {
buf.WriteString(fmt.Sprintf("%s-", *pair.GroupName))
} else {
buf.WriteString("-")
}
}
}

return fmt.Sprintf("sg-%d", hashcode.String(buf.String()))
}

Expand Down
101 changes: 101 additions & 0 deletions builtin/providers/aws/resource_aws_security_group_rule_migrate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package aws

import (
"fmt"
"log"
"strconv"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform/terraform"
)

func resourceAwsSecurityGroupRuleMigrateState(
v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
switch v {
case 0:
log.Println("[INFO] Found AWS Security Group State v0; migrating to v1")
return migrateSGRuleStateV0toV1(is)
default:
return is, fmt.Errorf("Unexpected schema version: %d", v)
}

return is, nil
}

func migrateSGRuleStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
if is.Empty() {
log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
return is, nil
}

perm, err := migrateExpandIPPerm(is.Attributes)

if err != nil {
return nil, fmt.Errorf("[WARN] Error making new IP Permission in Security Group migration")
}

log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
newID := ipPermissionIDHash(is.Attributes["type"], perm)
is.Attributes["id"] = newID
is.ID = newID
log.Printf("[DEBUG] Attributes after migration: %#v, new id: %s", is.Attributes, newID)
return is, nil
}

func migrateExpandIPPerm(attrs map[string]string) (*ec2.IPPermission, error) {
var perm ec2.IPPermission
tp, err := strconv.Atoi(attrs["to_port"])
if err != nil {
return nil, fmt.Errorf("Error converting to_port in Security Group migration")
}

fp, err := strconv.Atoi(attrs["from_port"])
if err != nil {
return nil, fmt.Errorf("Error converting from_port in Security Group migration")
}

perm.ToPort = aws.Long(int64(tp))
perm.FromPort = aws.Long(int64(fp))
perm.IPProtocol = aws.String(attrs["protocol"])

groups := make(map[string]bool)
if attrs["self"] == "true" {
groups[attrs["security_group_id"]] = true
}

if attrs["source_security_group_id"] != "" {
groups[attrs["source_security_group_id"]] = true
}

if len(groups) > 0 {
perm.UserIDGroupPairs = make([]*ec2.UserIDGroupPair, len(groups))
// build string list of group name/ids
var gl []string
for k, _ := range groups {
gl = append(gl, k)
}

for i, name := range gl {
perm.UserIDGroupPairs[i] = &ec2.UserIDGroupPair{
GroupID: aws.String(name),
}
}
}

var cb []string
for k, v := range attrs {
if k != "cidr_blocks.#" && strings.HasPrefix(k, "cidr_blocks") {
cb = append(cb, v)
}
}
if len(cb) > 0 {
perm.IPRanges = make([]*ec2.IPRange, len(cb))
for i, v := range cb {
perm.IPRanges[i] = &ec2.IPRange{CIDRIP: aws.String(v)}
}
}

return &perm, nil
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package aws

import (
"testing"

"github.com/hashicorp/terraform/terraform"
)

func TestAWSSecurityGroupRuleMigrateState(t *testing.T) {
cases := map[string]struct {
StateVersion int
ID string
Attributes map[string]string
Expected string
Meta interface{}
}{
"v0_1": {
StateVersion: 0,
ID: "sg-4235098228",
Attributes: map[string]string{
"self": "false",
"to_port": "0",
"security_group_id": "sg-13877277",
"cidr_blocks.#": "0",
"type": "ingress",
"protocol": "-1",
"from_port": "0",
"source_security_group_id": "sg-11877275",
},
Expected: "sg-3766347571",
},
"v0_2": {
StateVersion: 0,
ID: "sg-1021609891",
Attributes: map[string]string{
"security_group_id": "sg-0981746d",
"from_port": "0",
"to_port": "0",
"type": "ingress",
"self": "false",
"protocol": "-1",
"cidr_blocks.0": "172.16.1.0/24",
"cidr_blocks.1": "172.16.2.0/24",
"cidr_blocks.2": "172.16.3.0/24",
"cidr_blocks.3": "172.16.4.0/24",
"cidr_blocks.#": "4"},
Expected: "sg-4100229787",
},
}

for tn, tc := range cases {
is := &terraform.InstanceState{
ID: tc.ID,
Attributes: tc.Attributes,
}
is, err := resourceAwsSecurityGroupRuleMigrateState(
tc.StateVersion, is, tc.Meta)

if err != nil {
t.Fatalf("bad: %s, err: %#v", tn, err)
}

if is.ID != tc.Expected {
t.Fatalf("bad sg rule id: %s\n\n expected: %s", is.ID, tc.Expected)
}
}
}
Loading

0 comments on commit f045e9e

Please sign in to comment.