Merge pull request #1711 from jeroendekorte/b-cloudstack-aclrule

Provider/Cloudstack: Fixed the acl rules to support protocol all and icmp

builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go

@@ -165,8 +165,18 @@ func resourceCloudStackNetworkACLRuleCreateRule(
 		rule["uuids"] = uuids
 	}
 
-	// If protocol is not ICMP, loop through all ports
+	// If the protocol is ALL set the needed parameters
-	if rule["protocol"].(string) != "icmp" {
+	if rule["protocol"].(string) == "all" {
+		r, err := cs.NetworkACL.CreateNetworkACL(p)
+		if err != nil {
+			return err
+		}
+		uuids["all"] = r.Id
+		rule["uuids"] = uuids
+	}
+
+	// If protocol is TCP or UDP, loop through all ports
+	if rule["protocol"].(string) == "tcp" || rule["protocol"].(string) == "udp" {
 		if ps := rule["ports"].(*schema.Set); ps.Len() > 0 {
 
 			// Create an empty schema.Set to hold all processed ports
@@ -246,17 +256,43 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface
 				}
 
 				// Update the values
-				rule["action"] = r.Action
+				rule["action"] = strings.ToLower(r.Action)
 				rule["source_cidr"] = r.Cidrlist
 				rule["protocol"] = r.Protocol
 				rule["icmp_type"] = r.Icmptype
 				rule["icmp_code"] = r.Icmpcode
-				rule["traffic_type"] = r.Traffictype
+				rule["traffic_type"] = strings.ToLower(r.Traffictype)
 				rules.Add(rule)
 			}
 
-			// If protocol is not ICMP, loop through all ports
+			if rule["protocol"].(string) == "all" {
-			if rule["protocol"].(string) != "icmp" {
+				id, ok := uuids["all"]
+				if !ok {
+					continue
+				}
+
+				// Get the rule
+				r, count, err := cs.NetworkACL.GetNetworkACLByID(id.(string))
+				// If the count == 0, there is no object found for this UUID
+				if err != nil {
+					if count == 0 {
+						delete(uuids, "all")
+						continue
+					}
+
+					return err
+				}
+
+				// Update the values
+				rule["action"] = strings.ToLower(r.Action)
+				rule["source_cidr"] = r.Cidrlist
+				rule["protocol"] = r.Protocol
+				rule["traffic_type"] = strings.ToLower(r.Traffictype)
+				rules.Add(rule)
+			}
+
+			// If protocol is tcp or udp, loop through all ports
+			if rule["protocol"].(string) == "tcp" || rule["protocol"].(string) == "udp" {
 				if ps := rule["ports"].(*schema.Set); ps.Len() > 0 {
 
 					// Create an empty schema.Set to hold all ports
@@ -523,7 +559,8 @@ func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interfac
 	}
 
 	protocol := rule["protocol"].(string)
-	if protocol == "icmp" {
+	switch protocol {
+	case "icmp":
 		if _, ok := rule["icmp_type"]; !ok {
 			return fmt.Errorf(
 				"Parameter icmp_type is a required parameter when using protocol 'icmp'")
@@ -532,8 +569,14 @@ func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interfac
 			return fmt.Errorf(
 				"Parameter icmp_code is a required parameter when using protocol 'icmp'")
 		}
-	} else {
+	case "all":
-		if protocol != "tcp" && protocol != "udp" && protocol != "all" {
+		// No additional test are needed, so just leave this empty...
+	case "tcp", "udp":
+		if _, ok := rule["ports"]; !ok {
+			return fmt.Errorf(
+				"Parameter ports is a required parameter when *not* using protocol 'icmp'")
+		}
+	default:
 		_, err := strconv.ParseInt(protocol, 0, 0)
 		if err != nil {
 			return fmt.Errorf(
@@ -541,11 +584,6 @@ func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interfac
 					"'icmp', 'all' or a valid protocol number", protocol)
 		}
 	}
-		if _, ok := rule["ports"]; !ok {
-			return fmt.Errorf(
-				"Parameter ports is a required parameter when *not* using protocol 'icmp'")
-		}
-	}
 
 	traffic := rule["traffic_type"].(string)
 	if traffic != "ingress" && traffic != "egress" {

builtin/providers/cloudstack/resource_cloudstack_network_acl_rule_test.go

@@ -21,7 +21,7 @@ func TestAccCloudStackNetworkACLRule_basic(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.foo"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_network_acl_rule.foo", "rule.#", "1"),
+						"cloudstack_network_acl_rule.foo", "rule.#", "3"),
 					resource.TestCheckResourceAttr(
 						"cloudstack_network_acl_rule.foo", "rule.3247834462.action", "allow"),
 					resource.TestCheckResourceAttr(
@@ -53,7 +53,7 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.foo"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_network_acl_rule.foo", "rule.#", "1"),
+						"cloudstack_network_acl_rule.foo", "rule.#", "3"),
 					resource.TestCheckResourceAttr(
 						"cloudstack_network_acl_rule.foo", "rule.3247834462.action", "allow"),
 					resource.TestCheckResourceAttr(
@@ -76,7 +76,7 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.foo"),
 					resource.TestCheckResourceAttr(
-						"cloudstack_network_acl_rule.foo", "rule.#", "2"),
+						"cloudstack_network_acl_rule.foo", "rule.#", "4"),
 					resource.TestCheckResourceAttr(
 						"cloudstack_network_acl_rule.foo", "rule.3247834462.action", "allow"),
 					resource.TestCheckResourceAttr(
@@ -189,6 +189,22 @@ resource "cloudstack_network_acl" "foo" {
 resource "cloudstack_network_acl_rule" "foo" {
   aclid = "${cloudstack_network_acl.foo.id}"
 
+  rule {
+  	action = "allow"
+    source_cidr = "172.18.100.0/24"
+    protocol = "all"
+    traffic_type = "ingress"
+  }
+
+  rule {
+  	action = "allow"
+    source_cidr = "172.18.100.0/24"
+    protocol = "icmp"
+    icmp_type = "-1"
+    icmp_code = "-1"
+    traffic_type = "ingress"
+  }
+
   rule {
     source_cidr = "172.16.100.0/24"
     protocol = "tcp"
@@ -217,6 +233,22 @@ resource "cloudstack_network_acl" "foo" {
 resource "cloudstack_network_acl_rule" "foo" {
   aclid = "${cloudstack_network_acl.foo.id}"
 
+  rule {
+  	action = "deny"
+    source_cidr = "172.18.100.0/24"
+    protocol = "all"
+    traffic_type = "ingress"
+  }
+
+  rule {
+  	action = "deny"
+    source_cidr = "172.18.100.0/24"
+    protocol = "icmp"
+    icmp_type = "-1"
+    icmp_code = "-1"
+    traffic_type = "ingress"
+  }
+
   rule {
 	  action = "allow"
     source_cidr = "172.16.100.0/24"