diff --git a/CHANGELOG.md b/CHANGELOG.md index f492b2ef21b1..b6f1415ea50e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,8 @@ BUG FIXES: * providers/aws: Disassociate EIP before destroying. * providers/aws: ELB treats subnets as a set. * providers/aws: Fix case where in a destroy/create tags weren't reapplied. [GH-464] + * providers/aws: Fix incorrect/erroneous apply cases around security group + rules. [GH-457] * providers/consul: Fix regression where `key` param changed to `keys. [GH-475] ## 0.3.0 (October 14, 2014) diff --git a/builtin/providers/aws/resource_aws_security_group.go b/builtin/providers/aws/resource_aws_security_group.go index 459aaeb30783..4be7ca484c75 100644 --- a/builtin/providers/aws/resource_aws_security_group.go +++ b/builtin/providers/aws/resource_aws_security_group.go @@ -66,14 +66,18 @@ func resourceAwsSecurityGroup() *schema.Resource { }, "security_groups": &schema.Schema{ - Type: schema.TypeList, + Type: schema.TypeSet, Optional: true, Elem: &schema.Schema{Type: schema.TypeString}, + Set: func(v interface{}) int { + return hashcode.String(v.(string)) + }, }, "self": &schema.Schema{ Type: schema.TypeBool, Optional: true, + Default: false, }, }, }, @@ -112,7 +116,7 @@ func resourceAwsSecurityGroupIngressHash(v interface{}) int { } } if v, ok := m["security_groups"]; ok { - vs := v.([]interface{}) + vs := v.(*schema.Set).List() s := make([]string, len(vs)) for i, raw := range vs { s[i] = raw.(string) @@ -283,15 +287,28 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro sg := sgRaw.(*ec2.SecurityGroupInfo) // Gather our ingress rules - ingressRules := make([]map[string]interface{}, len(sg.IPPerms)) - for i, perm := range sg.IPPerms { - n := make(map[string]interface{}) - n["from_port"] = perm.FromPort - n["protocol"] = perm.Protocol - n["to_port"] = perm.ToPort + ingressMap := make(map[string]map[string]interface{}) + for _, perm := range sg.IPPerms { + k := fmt.Sprintf("%s-%d-%d", perm.Protocol, perm.FromPort, perm.ToPort) + m, ok := ingressMap[k] + if !ok { + m = make(map[string]interface{}) + ingressMap[k] = m + } + + m["from_port"] = perm.FromPort + m["to_port"] = perm.ToPort + m["protocol"] = perm.Protocol if len(perm.SourceIPs) > 0 { - n["cidr_blocks"] = perm.SourceIPs + raw, ok := m["cidr_blocks"] + if !ok { + raw = make([]string, 0, len(perm.SourceIPs)) + } + list := raw.([]string) + + list = append(list, perm.SourceIPs...) + m["cidr_blocks"] = list } var groups []string @@ -301,14 +318,24 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro for i, id := range groups { if id == d.Id() { groups[i], groups = groups[len(groups)-1], groups[:len(groups)-1] - n["self"] = true + m["self"] = true } } + if len(groups) > 0 { - n["security_groups"] = groups - } + raw, ok := m["security_groups"] + if !ok { + raw = make([]string, 0, len(groups)) + } + list := raw.([]string) - ingressRules[i] = n + list = append(list, groups...) + m["security_groups"] = list + } + } + ingressRules := make([]map[string]interface{}, 0, len(ingressMap)) + for _, m := range ingressMap { + ingressRules = append(ingressRules, m) } d.Set("description", sg.Description) diff --git a/builtin/providers/aws/structure.go b/builtin/providers/aws/structure.go index c349991cb00c..3af176c2abae 100644 --- a/builtin/providers/aws/structure.go +++ b/builtin/providers/aws/structure.go @@ -3,6 +3,7 @@ package aws import ( "strings" + "github.com/hashicorp/terraform/helper/schema" "github.com/mitchellh/goamz/ec2" "github.com/mitchellh/goamz/elb" ) @@ -48,7 +49,7 @@ func expandIPPerms(id string, configured []interface{}) []ec2.IPPerm { var groups []string if raw, ok := m["security_groups"]; ok { - list := raw.([]interface{}) + list := raw.(*schema.Set).List() for _, v := range list { groups = append(groups, v.(string)) } diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index 23be6f290abc..fbdd3da2a6bc 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -2,6 +2,7 @@ package schema import ( "fmt" + "log" "reflect" "strconv" "strings" @@ -647,6 +648,17 @@ func (d *ResourceData) getPrimitive( } } + if strings.HasSuffix(k, "protocol") && result == "" { + log.Printf("------------------------------------------------") + log.Printf("KEY: %s", k) + log.Printf("LEVEL: %#v", source) + log.Printf("DIFF: %#v", diff) + log.Printf("EXACT: %#v", exact) + log.Printf("Config: %#v\n\n", d.config) + log.Printf("DIFF: %#v\n\n", d.diff) + log.Printf("State: %#v\n\n", d.state) + } + if !exact || source == getSourceSet { if d.setMap != nil && source >= getSourceSet { if v, ok := d.setMap[k]; ok { diff --git a/helper/schema/schema.go b/helper/schema/schema.go index f305e547395b..7cd0fe71123a 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -205,7 +205,7 @@ func (m schemaMap) Diff( } for k, schema := range m { - err := m.diff(k, schema, result, d) + err := m.diff(k, schema, result, d, false) if err != nil { return nil, err } @@ -225,7 +225,7 @@ func (m schemaMap) Diff( // Perform the diff again for k, schema := range m { - err := m.diff(k, schema, result2, d) + err := m.diff(k, schema, result2, d, false) if err != nil { return nil, err } @@ -427,7 +427,8 @@ func (m schemaMap) diff( k string, schema *Schema, diff *terraform.InstanceDiff, - d *ResourceData) error { + d *ResourceData, + all bool) error { var err error switch schema.Type { case TypeBool: @@ -435,13 +436,13 @@ func (m schemaMap) diff( case TypeInt: fallthrough case TypeString: - err = m.diffString(k, schema, diff, d) + err = m.diffString(k, schema, diff, d, all) case TypeList: - err = m.diffList(k, schema, diff, d) + err = m.diffList(k, schema, diff, d, all) case TypeMap: - err = m.diffMap(k, schema, diff, d) + err = m.diffMap(k, schema, diff, d, all) case TypeSet: - err = m.diffSet(k, schema, diff, d) + err = m.diffSet(k, schema, diff, d, all) default: err = fmt.Errorf("%s: unknown type %#v", k, schema.Type) } @@ -453,7 +454,8 @@ func (m schemaMap) diffList( k string, schema *Schema, diff *terraform.InstanceDiff, - d *ResourceData) error { + d *ResourceData, + all bool) error { o, n, _, computedList := d.diffChange(k) nSet := n != nil @@ -481,7 +483,7 @@ func (m schemaMap) diffList( // If the new value was set, and the two are equal, then we're done. // We have to do this check here because sets might be NOT // reflect.DeepEqual so we need to wait until we get the []interface{} - if nSet && reflect.DeepEqual(os, vs) { + if !all && nSet && reflect.DeepEqual(os, vs) { return nil } @@ -502,7 +504,7 @@ func (m schemaMap) diffList( // If the counts are not the same, then record that diff changed := oldLen != newLen computed := oldLen == 0 && newLen == 0 && schema.Computed - if changed || computed { + if changed || computed || all { countSchema := &Schema{ Type: TypeInt, Computed: schema.Computed, @@ -539,7 +541,7 @@ func (m schemaMap) diffList( // just diff each. for i := 0; i < maxLen; i++ { subK := fmt.Sprintf("%s.%d", k, i) - err := m.diff(subK, &t2, diff, d) + err := m.diff(subK, &t2, diff, d, all) if err != nil { return err } @@ -549,7 +551,7 @@ func (m schemaMap) diffList( for i := 0; i < maxLen; i++ { for k2, schema := range t.Schema { subK := fmt.Sprintf("%s.%d.%s", k, i, k2) - err := m.diff(subK, schema, diff, d) + err := m.diff(subK, schema, diff, d, all) if err != nil { return err } @@ -566,8 +568,8 @@ func (m schemaMap) diffMap( k string, schema *Schema, diff *terraform.InstanceDiff, - d *ResourceData) error { - //elemSchema := &Schema{Type: TypeString} + d *ResourceData, + all bool) error { prefix := k + "." // First get all the values from the state @@ -590,7 +592,7 @@ func (m schemaMap) diffMap( old := stateMap[k] delete(stateMap, k) - if old == v { + if old == v && !all { continue } @@ -613,15 +615,35 @@ func (m schemaMap) diffSet( k string, schema *Schema, diff *terraform.InstanceDiff, - d *ResourceData) error { - return m.diffList(k, schema, diff, d) + d *ResourceData, + all bool) error { + if !all { + // This is a bit strange, but we expect the entire set to be in the diff, + // so we first diff the set normally but with a new diff. Then, if + // there IS any change, we just set the change to the entire list. + tempD := new(terraform.InstanceDiff) + tempD.Attributes = make(map[string]*terraform.ResourceAttrDiff) + if err := m.diffList(k, schema, tempD, d, false); err != nil { + return err + } + + // If we had no changes, then we're done + if tempD.Empty() { + return nil + } + } + + // We have changes, so re-run the diff, but set a flag to force + // getting all diffs, even if there is no change. + return m.diffList(k, schema, diff, d, true) } func (m schemaMap) diffString( k string, schema *Schema, diff *terraform.InstanceDiff, - d *ResourceData) error { + d *ResourceData, + all bool) error { var originalN interface{} var os, ns string o, n, _, _ := d.diffChange(k) @@ -646,7 +668,7 @@ func (m schemaMap) diffString( return fmt.Errorf("%s: %s", k, err) } - if os == ns { + if os == ns && !all { // They're the same value. If there old value is not blank or we // have an ID, then return right away since we're already setup. if os != "" || d.Id() != "" { diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index a7be6f99b0a2..5351f61d7059 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -865,6 +865,14 @@ func TestSchemaMap_Diff(t *testing.T) { Old: "2", New: "3", }, + "ports.0": &terraform.ResourceAttrDiff{ + Old: "1", + New: "1", + }, + "ports.1": &terraform.ResourceAttrDiff{ + Old: "2", + New: "2", + }, "ports.2": &terraform.ResourceAttrDiff{ Old: "", New: "5",