From 3db7922b53dbe6b208fad2970186d88396ae6a49 Mon Sep 17 00:00:00 2001 From: Joe Topjian Date: Wed, 11 Nov 2015 05:43:59 +0000 Subject: [PATCH] provider/openstack: Security Group Rule fixes This commit fixes an issue with security group rules where the rules were not being correctly computed due to a typo in the rule map. Once rules were successfully computed, the rules then needed to be converted into a Set so they can be correctly ordered. --- .../resource_openstack_compute_secgroup_v2.go | 114 +++++---- ...urce_openstack_compute_secgroup_v2_test.go | 232 +++++++++++++++++- 2 files changed, 288 insertions(+), 58 deletions(-) diff --git a/builtin/providers/openstack/resource_openstack_compute_secgroup_v2.go b/builtin/providers/openstack/resource_openstack_compute_secgroup_v2.go index 3cc0cbf0c..4e91c78a8 100644 --- a/builtin/providers/openstack/resource_openstack_compute_secgroup_v2.go +++ b/builtin/providers/openstack/resource_openstack_compute_secgroup_v2.go @@ -38,8 +38,9 @@ func resourceComputeSecGroupV2() *schema.Resource { ForceNew: false, }, "rule": &schema.Schema{ - Type: schema.TypeList, + Type: schema.TypeSet, Optional: true, + Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "id": &schema.Schema{ @@ -79,6 +80,7 @@ func resourceComputeSecGroupV2() *schema.Resource { }, }, }, + Set: secgroupRuleV2Hash, }, }, } @@ -129,13 +131,10 @@ func resourceComputeSecGroupV2Read(d *schema.ResourceData, meta interface{}) err d.Set("name", sg.Name) d.Set("description", sg.Description) - rtm := rulesToMap(sg.Rules) - for _, v := range rtm { - if v["group"] == d.Get("name") { - v["self"] = "1" - } else { - v["self"] = "0" - } + + rtm, err := rulesToMap(computeClient, d, sg.Rules) + if err != nil { + return err } log.Printf("[DEBUG] rulesToMap(sg.Rules): %+v", rtm) d.Set("rule", rtm) @@ -164,14 +163,11 @@ func resourceComputeSecGroupV2Update(d *schema.ResourceData, meta interface{}) e if d.HasChange("rule") { oldSGRaw, newSGRaw := d.GetChange("rule") - oldSGRSlice, newSGRSlice := oldSGRaw.([]interface{}), newSGRaw.([]interface{}) - oldSGRSet := schema.NewSet(secgroupRuleV2Hash, oldSGRSlice) - newSGRSet := schema.NewSet(secgroupRuleV2Hash, newSGRSlice) + oldSGRSet, newSGRSet := oldSGRaw.(*schema.Set), newSGRaw.(*schema.Set) secgrouprulesToAdd := newSGRSet.Difference(oldSGRSet) secgrouprulesToRemove := oldSGRSet.Difference(newSGRSet) log.Printf("[DEBUG] Security group rules to add: %v", secgrouprulesToAdd) - log.Printf("[DEBUG] Security groups rules to remove: %v", secgrouprulesToRemove) for _, rawRule := range secgrouprulesToAdd.List() { @@ -231,67 +227,83 @@ func resourceComputeSecGroupV2Delete(d *schema.ResourceData, meta interface{}) e } func resourceSecGroupRulesV2(d *schema.ResourceData) []secgroups.CreateRuleOpts { - rawRules := d.Get("rule").([]interface{}) + rawRules := d.Get("rule").(*schema.Set).List() createRuleOptsList := make([]secgroups.CreateRuleOpts, len(rawRules)) - for i, raw := range rawRules { - rawMap := raw.(map[string]interface{}) - groupId := rawMap["from_group_id"].(string) - if rawMap["self"].(bool) { - groupId = d.Id() - } - createRuleOptsList[i] = secgroups.CreateRuleOpts{ - ParentGroupID: d.Id(), - FromPort: rawMap["from_port"].(int), - ToPort: rawMap["to_port"].(int), - IPProtocol: rawMap["ip_protocol"].(string), - CIDR: rawMap["cidr"].(string), - FromGroupID: groupId, - } + for i, rawRule := range rawRules { + createRuleOptsList[i] = resourceSecGroupRuleCreateOptsV2(d, rawRule) } return createRuleOptsList } -func resourceSecGroupRuleCreateOptsV2(d *schema.ResourceData, raw interface{}) secgroups.CreateRuleOpts { - rawMap := raw.(map[string]interface{}) - groupId := rawMap["from_group_id"].(string) - if rawMap["self"].(bool) { +func resourceSecGroupRuleCreateOptsV2(d *schema.ResourceData, rawRule interface{}) secgroups.CreateRuleOpts { + rawRuleMap := rawRule.(map[string]interface{}) + groupId := rawRuleMap["from_group_id"].(string) + if rawRuleMap["self"].(bool) { groupId = d.Id() } return secgroups.CreateRuleOpts{ ParentGroupID: d.Id(), - FromPort: rawMap["from_port"].(int), - ToPort: rawMap["to_port"].(int), - IPProtocol: rawMap["ip_protocol"].(string), - CIDR: rawMap["cidr"].(string), + FromPort: rawRuleMap["from_port"].(int), + ToPort: rawRuleMap["to_port"].(int), + IPProtocol: rawRuleMap["ip_protocol"].(string), + CIDR: rawRuleMap["cidr"].(string), FromGroupID: groupId, } } -func resourceSecGroupRuleV2(d *schema.ResourceData, raw interface{}) secgroups.Rule { - rawMap := raw.(map[string]interface{}) +func resourceSecGroupRuleV2(d *schema.ResourceData, rawRule interface{}) secgroups.Rule { + rawRuleMap := rawRule.(map[string]interface{}) return secgroups.Rule{ - ID: rawMap["id"].(string), + ID: rawRuleMap["id"].(string), ParentGroupID: d.Id(), - FromPort: rawMap["from_port"].(int), - ToPort: rawMap["to_port"].(int), - IPProtocol: rawMap["ip_protocol"].(string), - IPRange: secgroups.IPRange{CIDR: rawMap["cidr"].(string)}, + FromPort: rawRuleMap["from_port"].(int), + ToPort: rawRuleMap["to_port"].(int), + IPProtocol: rawRuleMap["ip_protocol"].(string), + IPRange: secgroups.IPRange{CIDR: rawRuleMap["cidr"].(string)}, } } -func rulesToMap(sgrs []secgroups.Rule) []map[string]interface{} { +func rulesToMap(computeClient *gophercloud.ServiceClient, d *schema.ResourceData, sgrs []secgroups.Rule) ([]map[string]interface{}, error) { sgrMap := make([]map[string]interface{}, len(sgrs)) for i, sgr := range sgrs { + groupId := "" + self := false + if sgr.Group.Name != "" { + if sgr.Group.Name == d.Get("name").(string) { + self = true + } else { + // Since Nova only returns the secgroup Name (and not the ID) for the group attribute, + // we need to look up all security groups and match the name. + // Nevermind that Nova wants the ID when setting the Group *and* that multiple groups + // with the same name can exist... + allPages, err := secgroups.List(computeClient).AllPages() + if err != nil { + return nil, err + } + securityGroups, err := secgroups.ExtractSecurityGroups(allPages) + if err != nil { + return nil, err + } + + for _, sg := range securityGroups { + if sg.Name == sgr.Group.Name { + groupId = sg.ID + } + } + } + } + sgrMap[i] = map[string]interface{}{ - "id": sgr.ID, - "from_port": sgr.FromPort, - "to_port": sgr.ToPort, - "ip_protocol": sgr.IPProtocol, - "cidr": sgr.IPRange.CIDR, - "group": sgr.Group.Name, + "id": sgr.ID, + "from_port": sgr.FromPort, + "to_port": sgr.ToPort, + "ip_protocol": sgr.IPProtocol, + "cidr": sgr.IPRange.CIDR, + "self": self, + "from_group_id": groupId, } } - return sgrMap + return sgrMap, nil } func secgroupRuleV2Hash(v interface{}) int { @@ -301,6 +313,8 @@ func secgroupRuleV2Hash(v interface{}) int { buf.WriteString(fmt.Sprintf("%d-", m["to_port"].(int))) buf.WriteString(fmt.Sprintf("%s-", m["ip_protocol"].(string))) buf.WriteString(fmt.Sprintf("%s-", m["cidr"].(string))) + buf.WriteString(fmt.Sprintf("%s-", m["from_group_id"].(string))) + buf.WriteString(fmt.Sprintf("%s-", m["self"].(bool))) return hashcode.String(buf.String()) } diff --git a/builtin/providers/openstack/resource_openstack_compute_secgroup_v2_test.go b/builtin/providers/openstack/resource_openstack_compute_secgroup_v2_test.go index e78865b8a..4cb99fa74 100644 --- a/builtin/providers/openstack/resource_openstack_compute_secgroup_v2_test.go +++ b/builtin/providers/openstack/resource_openstack_compute_secgroup_v2_test.go @@ -19,7 +19,7 @@ func TestAccComputeV2SecGroup_basic(t *testing.T) { CheckDestroy: testAccCheckComputeV2SecGroupDestroy, Steps: []resource.TestStep{ resource.TestStep{ - Config: testAccComputeV2SecGroup_basic, + Config: testAccComputeV2SecGroup_basic_orig, Check: resource.ComposeTestCheckFunc( testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.foo", &secgroup), ), @@ -28,6 +28,84 @@ func TestAccComputeV2SecGroup_basic(t *testing.T) { }) } +func TestAccComputeV2SecGroup_update(t *testing.T) { + var secgroup secgroups.SecurityGroup + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeV2SecGroupDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeV2SecGroup_basic_orig, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.foo", &secgroup), + ), + }, + resource.TestStep{ + Config: testAccComputeV2SecGroup_basic_update, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.foo", &secgroup), + testAccCheckComputeV2SecGroupRuleCount(t, &secgroup, 2), + ), + }, + }, + }) +} + +func TestAccComputeV2SecGroup_groupID(t *testing.T) { + var secgroup1, secgroup2, secgroup3 secgroups.SecurityGroup + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeV2SecGroupDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeV2SecGroup_groupID_orig, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_1", &secgroup1), + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_2", &secgroup2), + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_3", &secgroup3), + testAccCheckComputeV2SecGroupGroupIDMatch(t, &secgroup1, &secgroup3), + ), + }, + resource.TestStep{ + Config: testAccComputeV2SecGroup_groupID_update, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_1", &secgroup1), + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_2", &secgroup2), + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_3", &secgroup3), + testAccCheckComputeV2SecGroupGroupIDMatch(t, &secgroup2, &secgroup3), + ), + }, + }, + }) +} + +func TestAccComputeV2SecGroup_self(t *testing.T) { + var secgroup secgroups.SecurityGroup + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeV2SecGroupDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeV2SecGroup_self, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeV2SecGroupExists(t, "openstack_compute_secgroup_v2.test_group_1", &secgroup), + testAccCheckComputeV2SecGroupGroupIDMatch(t, &secgroup, &secgroup), + resource.TestCheckResourceAttr( + "openstack_compute_secgroup_v2.test_group_1", "rule.1118853483.self", "true"), + resource.TestCheckResourceAttr( + "openstack_compute_secgroup_v2.test_group_1", "rule.1118853483.from_group_id", ""), + ), + }, + }, + }) +} + func testAccCheckComputeV2SecGroupDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) computeClient, err := config.computeV2Client(OS_REGION_NAME) @@ -81,10 +159,148 @@ func testAccCheckComputeV2SecGroupExists(t *testing.T, n string, secgroup *secgr } } -var testAccComputeV2SecGroup_basic = fmt.Sprintf(` - resource "openstack_compute_secgroup_v2" "foo" { - region = "%s" - name = "test_group_1" - description = "first test security group" - }`, - OS_REGION_NAME) +func testAccCheckComputeV2SecGroupRuleCount(t *testing.T, secgroup *secgroups.SecurityGroup, count int) resource.TestCheckFunc { + return func(s *terraform.State) error { + if len(secgroup.Rules) != count { + return fmt.Errorf("Security group rule count does not match. Expected %d, got %d", count, len(secgroup.Rules)) + } + + return nil + } +} + +func testAccCheckComputeV2SecGroupGroupIDMatch(t *testing.T, sg1, sg2 *secgroups.SecurityGroup) resource.TestCheckFunc { + return func(s *terraform.State) error { + if len(sg2.Rules) == 1 { + if sg1.Name != sg2.Rules[0].Group.Name || sg1.TenantID != sg2.Rules[0].Group.TenantID { + return fmt.Errorf("%s was not correctly applied to %s", sg1.Name, sg2.Name) + } + } else { + return fmt.Errorf("%s rule count is incorrect", sg2.Name) + } + + return nil + } +} + +var testAccComputeV2SecGroup_basic_orig = fmt.Sprintf(` + resource "openstack_compute_secgroup_v2" "foo" { + name = "test_group_1" + description = "first test security group" + rule { + from_port = 22 + to_port = 22 + ip_protocol = "tcp" + cidr = "0.0.0.0/0" + } + rule { + from_port = 1 + to_port = 65535 + ip_protocol = "udp" + cidr = "0.0.0.0/0" + } + rule { + from_port = -1 + to_port = -1 + ip_protocol = "icmp" + cidr = "0.0.0.0/0" + } + }`) + +var testAccComputeV2SecGroup_basic_update = fmt.Sprintf(` + resource "openstack_compute_secgroup_v2" "foo" { + name = "test_group_1" + description = "first test security group" + rule { + from_port = 2200 + to_port = 2200 + ip_protocol = "tcp" + cidr = "0.0.0.0/0" + } + rule { + from_port = -1 + to_port = -1 + ip_protocol = "icmp" + cidr = "0.0.0.0/0" + } +}`) + +var testAccComputeV2SecGroup_groupID_orig = fmt.Sprintf(` + resource "openstack_compute_secgroup_v2" "test_group_1" { + name = "test_group_1" + description = "first test security group" + rule { + from_port = 22 + to_port = 22 + ip_protocol = "tcp" + cidr = "0.0.0.0/0" + } + } + + resource "openstack_compute_secgroup_v2" "test_group_2" { + name = "test_group_2" + description = "second test security group" + rule { + from_port = -1 + to_port = -1 + ip_protocol = "icmp" + cidr = "0.0.0.0/0" + } + } + + resource "openstack_compute_secgroup_v2" "test_group_3" { + name = "test_group_3" + description = "third test security group" + rule { + from_port = 80 + to_port = 80 + ip_protocol = "tcp" + from_group_id = "${openstack_compute_secgroup_v2.test_group_1.id}" + } + }`) + +var testAccComputeV2SecGroup_groupID_update = fmt.Sprintf(` + resource "openstack_compute_secgroup_v2" "test_group_1" { + name = "test_group_1" + description = "first test security group" + rule { + from_port = 22 + to_port = 22 + ip_protocol = "tcp" + cidr = "0.0.0.0/0" + } + } + + resource "openstack_compute_secgroup_v2" "test_group_2" { + name = "test_group_2" + description = "second test security group" + rule { + from_port = -1 + to_port = -1 + ip_protocol = "icmp" + cidr = "0.0.0.0/0" + } + } + + resource "openstack_compute_secgroup_v2" "test_group_3" { + name = "test_group_3" + description = "third test security group" + rule { + from_port = 80 + to_port = 80 + ip_protocol = "tcp" + from_group_id = "${openstack_compute_secgroup_v2.test_group_2.id}" + } + }`) + +var testAccComputeV2SecGroup_self = fmt.Sprintf(` + resource "openstack_compute_secgroup_v2" "test_group_1" { + name = "test_group_1" + description = "first test security group" + rule { + from_port = 22 + to_port = 22 + ip_protocol = "tcp" + self = true + } + }`)