Re: [PR] Allow 'all' on security group rule protocol [cloudstack-terraform-provider]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #294:
URL: https://github.com/apache/cloudstack-terraform-provider/pull/294#discussion_r3557241266


##########
cloudstack/resource_cloudstack_security_group_rule.go:
##########
@@ -393,6 +403,32 @@ func readSecurityGroupRule(sg *cloudstack.SecurityGroup, ruleIndex map[string]in
 	uuids := rule["uuids"].(map[string]interface{})
 	sgRules := append(sg.Ingressrule, sg.Egressrule...)
 
+	if rule["protocol"].(string) == "all" {
+		id, ok := uuids[uuid+"all"]
+		if !ok {
+			return
+		}
+
+		// Get the rule
+		idx, ok := ruleIndex[id.(string)]
+		if !ok {
+			return
+		}

Review Comment:
   In readSecurityGroupRule()'s `protocol == "all"` branch, if the stored UUID no longer exists in the security group (missing from ruleIndex), the code returns without deleting the stale `uuids[uuid+"all"]` entry. The icmp/tcp/udp branches remove stale UUIDs, so this inconsistency can leave state stuck referencing a non-existent rule.



##########
cloudstack/resource_cloudstack_security_group_rule.go:
##########
@@ -610,6 +646,8 @@ func verifySecurityGroupRuleParams(d *schema.ResourceData, rule map[string]inter
 
 	protocol := rule["protocol"].(string)
 	switch protocol {
+	case "all":
+		break
 	case "icmp":
 		if _, ok := rule["icmp_type"]; !ok {

Review Comment:
   Now that `protocol = "all"` is accepted in this switch, the fallback error message later in verifySecurityGroupRuleParams() (for invalid protocol strings) still lists only tcp/udp/icmp as valid options. That message should be updated to include `all` (and ideally mention numeric protocol values are accepted too), otherwise users will get a misleading error when they mistype the value.



##########
cloudstack/resource_cloudstack_security_group_rule_test.go:
##########
@@ -40,27 +40,33 @@ func TestAccCloudStackSecurityGroupRule_basic(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckCloudStackSecurityGroupRulesExist("cloudstack_security_group.foo"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_security_group_rule.foo", "rule.#", "2"),
+						"cloudstack_security_group_rule.foo", "rule.#", "3"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_security_group_rule.foo", "rule.0.cidr_list.0", "172.18.100.0/24"),
+						"cloudstack_security_group_rule.foo", "rule.0.protocol", "all"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_security_group_rule.foo", "rule.0.protocol", "tcp"),
+						"cloudstack_security_group_rule.foo", "rule.0.cidr_list.0", "172.0.0.0/8"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_security_group_rule.foo", "rule.0.ports.#", "1"),
+						"cloudstack_security_group_rule.foo", "rule.0.traffic_type", "egress"),

Review Comment:
   These acceptance tests assert specific element ordering for `rule` (e.g., `rule.0`, `rule.1`), but the schema defines `rule` as a `TypeSet` (unordered). Adding the new `protocol = "all"` rule is likely to change the set hash ordering and make the test flaky or fail depending on Terraform/SDK ordering. Prefer `resource.TestCheckTypeSetElemNestedAttrs` (as used in other tests like resource_cloudstack_network_acl_rule_test.go) to assert rule presence without relying on indices.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org