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.

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 {
+	rtm, err := rulesToMap(computeClient, d, sg.Rules)
-		if v["group"] == d.Get("name") {
+	if err != nil {
-			v["self"] = "1"
+		return err
-		} else {
-			v["self"] = "0"
-		}
 	}
 	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, newSGRSet := oldSGRaw.(*schema.Set), newSGRaw.(*schema.Set)
-		oldSGRSet := schema.NewSet(secgroupRuleV2Hash, oldSGRSlice)
-		newSGRSet := schema.NewSet(secgroupRuleV2Hash, newSGRSlice)
 		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 {
+	for i, rawRule := range rawRules {
-		rawMap := raw.(map[string]interface{})
+		createRuleOptsList[i] = resourceSecGroupRuleCreateOptsV2(d, rawRule)
-		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,
-		}
 	}
 	return createRuleOptsList
 }
 
-func resourceSecGroupRuleCreateOptsV2(d *schema.ResourceData, raw interface{}) secgroups.CreateRuleOpts {
+func resourceSecGroupRuleCreateOptsV2(d *schema.ResourceData, rawRule interface{}) secgroups.CreateRuleOpts {
-	rawMap := raw.(map[string]interface{})
+	rawRuleMap := rawRule.(map[string]interface{})
-	groupId := rawMap["from_group_id"].(string)
+	groupId := rawRuleMap["from_group_id"].(string)
-	if rawMap["self"].(bool) {
+	if rawRuleMap["self"].(bool) {
 		groupId = d.Id()
 	}
 	return secgroups.CreateRuleOpts{
 		ParentGroupID: d.Id(),
-		FromPort:      rawMap["from_port"].(int),
+		FromPort:      rawRuleMap["from_port"].(int),
-		ToPort:        rawMap["to_port"].(int),
+		ToPort:        rawRuleMap["to_port"].(int),
-		IPProtocol:    rawMap["ip_protocol"].(string),
+		IPProtocol:    rawRuleMap["ip_protocol"].(string),
-		CIDR:          rawMap["cidr"].(string),
+		CIDR:          rawRuleMap["cidr"].(string),
 		FromGroupID:   groupId,
 	}
 }
 
-func resourceSecGroupRuleV2(d *schema.ResourceData, raw interface{}) secgroups.Rule {
+func resourceSecGroupRuleV2(d *schema.ResourceData, rawRule interface{}) secgroups.Rule {
-	rawMap := raw.(map[string]interface{})
+	rawRuleMap := rawRule.(map[string]interface{})
 	return secgroups.Rule{
-		ID:            rawMap["id"].(string),
+		ID:            rawRuleMap["id"].(string),
 		ParentGroupID: d.Id(),
-		FromPort:      rawMap["from_port"].(int),
+		FromPort:      rawRuleMap["from_port"].(int),
-		ToPort:        rawMap["to_port"].(int),
+		ToPort:        rawRuleMap["to_port"].(int),
-		IPProtocol:    rawMap["ip_protocol"].(string),
+		IPProtocol:    rawRuleMap["ip_protocol"].(string),
-		IPRange:       secgroups.IPRange{CIDR: rawMap["cidr"].(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,
+			"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())
 }

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(`
+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" {
-    region = "%s"
 		name = "test_group_1"
 		description = "first test security group"
-    }`,
+		rule {
-	OS_REGION_NAME)
+			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
+		}
+	}`)