From 2673fe483725f24244712f23a94fed4d5495353e Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Mon, 25 Aug 2025 17:05:08 +0530 Subject: [PATCH 1/8] Support desc and ruleId in create_network_acl_rule --- .../resource_cloudstack_network_acl_rule.go | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/cloudstack/resource_cloudstack_network_acl_rule.go b/cloudstack/resource_cloudstack_network_acl_rule.go index 8740df00..b16efaca 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule.go +++ b/cloudstack/resource_cloudstack_network_acl_rule.go @@ -57,6 +57,12 @@ func resourceCloudStackNetworkACLRule() *schema.Resource { Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ + "rule_id": { + Type: schema.TypeInt, + Optional: true, + Computed: true, + }, + "action": { Type: schema.TypeString, Optional: true, @@ -100,6 +106,11 @@ func resourceCloudStackNetworkACLRule() *schema.Resource { Default: "ingress", }, + "description": { + Type: schema.TypeString, + Optional: true, + }, + "uuids": { Type: schema.TypeMap, Computed: true, @@ -198,6 +209,11 @@ func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule map[str // Create a new parameter struct p := cs.NetworkACL.NewCreateNetworkACLParams(rule["protocol"].(string)) + // If a rule ID is specified, set it + if ruleId, ok := rule["rule_id"].(int); ok && ruleId > 0 { + p.SetNumber(ruleId) + } + // Set the acl ID p.SetAclid(d.Id()) @@ -214,6 +230,11 @@ func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule map[str // Set the traffic type p.SetTraffictype(rule["traffic_type"].(string)) + // Set the description + if desc, ok := rule["description"].(string); ok && desc != "" { + p.SetReason(desc) + } + // If the protocol is ICMP set the needed ICMP parameters if rule["protocol"].(string) == "icmp" { p.SetIcmptype(rule["icmp_type"].(int)) @@ -623,6 +644,15 @@ func verifyNetworkACLParams(d *schema.ResourceData) error { } func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interface{}) error { + if ruleId, ok := rule["rule_id"]; ok && ruleId != nil { + if rId, ok := ruleId.(int); ok && rId > 0 { + if rId < 1 || rId > 65535 { + return fmt.Errorf( + "%q must be between %d and %d inclusive, got: %d", "rule_id", 1, 65535, rId) + } + } + } + action := rule["action"].(string) if action != "allow" && action != "deny" { return fmt.Errorf("Parameter action only accepts 'allow' or 'deny' as values") From fed2259a00ddc1c46e2d77a305a75fec49f2598c Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Mon, 25 Aug 2025 21:07:56 +0530 Subject: [PATCH 2/8] fix review comment --- cloudstack/resource_cloudstack_network_acl_rule.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule.go b/cloudstack/resource_cloudstack_network_acl_rule.go index b16efaca..e253c65a 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule.go +++ b/cloudstack/resource_cloudstack_network_acl_rule.go @@ -645,7 +645,8 @@ func verifyNetworkACLParams(d *schema.ResourceData) error { func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interface{}) error { if ruleId, ok := rule["rule_id"]; ok && ruleId != nil { - if rId, ok := ruleId.(int); ok && rId > 0 { + if rId, ok := ruleId.(int); ok && rId != 0 { + // Validate only if rule_id is explicitly set (non-zero) if rId < 1 || rId > 65535 { return fmt.Errorf( "%q must be between %d and %d inclusive, got: %d", "rule_id", 1, 65535, rId) From 1b1a9b815ed04ec1d9856d818607ed8ee09ae7c1 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 26 Aug 2025 16:06:52 +0530 Subject: [PATCH 3/8] change rule_id -> rule_number and add doc --- cloudstack/resource_cloudstack_network_acl_rule.go | 12 ++++++------ website/docs/r/network_acl_rule.html.markdown | 4 ++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule.go b/cloudstack/resource_cloudstack_network_acl_rule.go index e253c65a..90b59305 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule.go +++ b/cloudstack/resource_cloudstack_network_acl_rule.go @@ -57,7 +57,7 @@ func resourceCloudStackNetworkACLRule() *schema.Resource { Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ - "rule_id": { + "rule_number": { Type: schema.TypeInt, Optional: true, Computed: true, @@ -210,7 +210,7 @@ func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule map[str p := cs.NetworkACL.NewCreateNetworkACLParams(rule["protocol"].(string)) // If a rule ID is specified, set it - if ruleId, ok := rule["rule_id"].(int); ok && ruleId > 0 { + if ruleId, ok := rule["rule_number"].(int); ok && ruleId > 0 { p.SetNumber(ruleId) } @@ -644,12 +644,12 @@ func verifyNetworkACLParams(d *schema.ResourceData) error { } func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interface{}) error { - if ruleId, ok := rule["rule_id"]; ok && ruleId != nil { - if rId, ok := ruleId.(int); ok && rId != 0 { - // Validate only if rule_id is explicitly set (non-zero) + if ruleNum, ok := rule["rule_number"]; ok && ruleNum != nil { + if rId, ok := ruleNum.(int); ok && rId != 0 { + // Validate only if rule_number is explicitly set (non-zero) if rId < 1 || rId > 65535 { return fmt.Errorf( - "%q must be between %d and %d inclusive, got: %d", "rule_id", 1, 65535, rId) + "%q must be between %d and %d inclusive, got: %d", "rule_number", 1, 65535, rId) } } } diff --git a/website/docs/r/network_acl_rule.html.markdown b/website/docs/r/network_acl_rule.html.markdown index 07a2b392..c3fb7a60 100644 --- a/website/docs/r/network_acl_rule.html.markdown +++ b/website/docs/r/network_acl_rule.html.markdown @@ -48,6 +48,8 @@ The following arguments are supported: The `rule` block supports: +* `rule_number` - (Optional) The number of the ACL item, its ordering. + * `action` - (Optional) The action for the rule. Valid options are: `allow` and `deny` (defaults allow). @@ -68,6 +70,8 @@ The `rule` block supports: * `traffic_type` - (Optional) The traffic type for the rule. Valid options are: `ingress` or `egress` (defaults ingress). +* `description` - (Optional) A description indicating why the ACL rule is required. + ## Attributes Reference The following attributes are exported: From 967a37daaf4bb3ed6ce2f2fde366acd4d0c55ad3 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 26 Aug 2025 16:24:14 +0530 Subject: [PATCH 4/8] add params in unit tests --- .../resource_cloudstack_network_acl_rule_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule_test.go b/cloudstack/resource_cloudstack_network_acl_rule_test.go index 0fcc6916..a65f4670 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule_test.go +++ b/cloudstack/resource_cloudstack_network_acl_rule_test.go @@ -245,19 +245,23 @@ resource "cloudstack_network_acl_rule" "foo" { acl_id = cloudstack_network_acl.foo.id rule { + rule_number = 10 action = "allow" cidr_list = ["172.18.100.0/24"] protocol = "all" traffic_type = "ingress" + description = "Allow all traffic" } rule { + rule_number = 20 action = "allow" cidr_list = ["172.18.100.0/24"] protocol = "icmp" icmp_type = "-1" icmp_code = "-1" traffic_type = "ingress" + description = "Allow ICMP traffic" } rule { @@ -265,6 +269,7 @@ resource "cloudstack_network_acl_rule" "foo" { protocol = "tcp" ports = ["80", "443"] traffic_type = "ingress" + description = "Allow HTTP and HTTPS" } }` @@ -293,16 +298,19 @@ resource "cloudstack_network_acl_rule" "foo" { } rule { + rule_number = 10 action = "deny" - cidr_list = ["172.18.100.0/24", "172.18.101.0/24"] + cidr_list = ["172.18.100.0/24", "172.18.101.0/24"] protocol = "icmp" icmp_type = "-1" icmp_code = "-1" traffic_type = "ingress" + description = "Deny ICMP traffic" } rule { - action = "allow" + rule_number = 20 + action = "allow" cidr_list = ["172.18.100.0/24"] protocol = "tcp" ports = ["80", "443"] @@ -310,10 +318,11 @@ resource "cloudstack_network_acl_rule" "foo" { } rule { - action = "deny" + action = "deny" cidr_list = ["10.0.0.0/24"] protocol = "tcp" ports = ["80", "1000-2000"] traffic_type = "egress" + description = "Deny specific TCP ports" } }` From 930d61e36d0dcefdeae6ee848cbc72e103465e92 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 26 Aug 2025 16:53:44 +0530 Subject: [PATCH 5/8] verify description and rule_number in unit test --- .../resource_cloudstack_network_acl_rule_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule_test.go b/cloudstack/resource_cloudstack_network_acl_rule_test.go index a65f4670..d24450ea 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule_test.go +++ b/cloudstack/resource_cloudstack_network_acl_rule_test.go @@ -85,6 +85,8 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.foo"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "3"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.0.number", "10"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.action", "allow"), resource.TestCheckResourceAttr( @@ -99,6 +101,10 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { "cloudstack_network_acl_rule.foo", "rule.0.ports.0", "443"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.0.reason", "Allow all traffic"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.1.number", "20"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.action", "allow"), resource.TestCheckResourceAttr( @@ -111,6 +117,10 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { "cloudstack_network_acl_rule.foo", "rule.1.icmp_type", "-1"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.1.description", "Allow ICMP traffic"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2.description", "Allow HTTP and HTTPS"), ), }, @@ -298,7 +308,6 @@ resource "cloudstack_network_acl_rule" "foo" { } rule { - rule_number = 10 action = "deny" cidr_list = ["172.18.100.0/24", "172.18.101.0/24"] protocol = "icmp" @@ -309,7 +318,6 @@ resource "cloudstack_network_acl_rule" "foo" { } rule { - rule_number = 20 action = "allow" cidr_list = ["172.18.100.0/24"] protocol = "tcp" From 1f868fefea60727ece9f0e7a29cb128c9e0bd5e5 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 26 Aug 2025 18:33:54 +0530 Subject: [PATCH 6/8] use fields defined in schema --- cloudstack/resource_cloudstack_network_acl_rule_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule_test.go b/cloudstack/resource_cloudstack_network_acl_rule_test.go index d24450ea..cbbd4843 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule_test.go +++ b/cloudstack/resource_cloudstack_network_acl_rule_test.go @@ -86,7 +86,7 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "3"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.0.number", "10"), + "cloudstack_network_acl_rule.foo", "rule.0.rule_number", "10"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.action", "allow"), resource.TestCheckResourceAttr( @@ -102,9 +102,9 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.traffic_type", "ingress"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.0.reason", "Allow all traffic"), + "cloudstack_network_acl_rule.foo", "rule.0.description", "Allow all traffic"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.1.number", "20"), + "cloudstack_network_acl_rule.foo", "rule.1.rule_number", "20"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.action", "allow"), resource.TestCheckResourceAttr( From 38b6ed768177b9487b3b3cb7d844670e8aae71d6 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Wed, 27 Aug 2025 09:37:55 +0530 Subject: [PATCH 7/8] fix test verification sequence --- ...source_cloudstack_network_acl_rule_test.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule_test.go b/cloudstack/resource_cloudstack_network_acl_rule_test.go index cbbd4843..a7dad420 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule_test.go +++ b/cloudstack/resource_cloudstack_network_acl_rule_test.go @@ -38,6 +38,7 @@ func TestAccCloudStackNetworkACLRule_basic(t *testing.T) { { Config: testAccCloudStackNetworkACLRule_basic, Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.foo"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "3"), @@ -55,6 +56,10 @@ func TestAccCloudStackNetworkACLRule_basic(t *testing.T) { "cloudstack_network_acl_rule.foo", "rule.0.ports.0", "443"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.0.description", "Allow HTTP and HTTPS"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.1.rule_number", "20"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.action", "allow"), resource.TestCheckResourceAttr( @@ -67,6 +72,12 @@ func TestAccCloudStackNetworkACLRule_basic(t *testing.T) { "cloudstack_network_acl_rule.foo", "rule.1.icmp_type", "-1"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.1.description", "Allow ICMP traffic"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2.rule_number", "10"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2.description", "Allow all traffic"), ), }, }, @@ -85,8 +96,6 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.foo"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "3"), - resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.0.rule_number", "10"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.action", "allow"), resource.TestCheckResourceAttr( @@ -101,8 +110,6 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { "cloudstack_network_acl_rule.foo", "rule.0.ports.0", "443"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.0.traffic_type", "ingress"), - resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.0.description", "Allow all traffic"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.rule_number", "20"), resource.TestCheckResourceAttr( @@ -117,10 +124,6 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { "cloudstack_network_acl_rule.foo", "rule.1.icmp_type", "-1"), resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.1.traffic_type", "ingress"), - resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.1.description", "Allow ICMP traffic"), - resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.2.description", "Allow HTTP and HTTPS"), ), }, From da842459b40c28892023443499ebd0e00699ebdd Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Wed, 27 Aug 2025 12:36:59 +0530 Subject: [PATCH 8/8] handle review comments --- cloudstack/resource_cloudstack_network_acl_rule.go | 10 +++++----- website/docs/r/network_acl_rule.html.markdown | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cloudstack/resource_cloudstack_network_acl_rule.go b/cloudstack/resource_cloudstack_network_acl_rule.go index 90b59305..1ac6e126 100644 --- a/cloudstack/resource_cloudstack_network_acl_rule.go +++ b/cloudstack/resource_cloudstack_network_acl_rule.go @@ -210,8 +210,8 @@ func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule map[str p := cs.NetworkACL.NewCreateNetworkACLParams(rule["protocol"].(string)) // If a rule ID is specified, set it - if ruleId, ok := rule["rule_number"].(int); ok && ruleId > 0 { - p.SetNumber(ruleId) + if ruleNum, ok := rule["rule_number"].(int); ok && ruleNum > 0 { + p.SetNumber(ruleNum) } // Set the acl ID @@ -645,11 +645,11 @@ func verifyNetworkACLParams(d *schema.ResourceData) error { func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interface{}) error { if ruleNum, ok := rule["rule_number"]; ok && ruleNum != nil { - if rId, ok := ruleNum.(int); ok && rId != 0 { + if number, ok := ruleNum.(int); ok && number != 0 { // Validate only if rule_number is explicitly set (non-zero) - if rId < 1 || rId > 65535 { + if number < 1 || number > 65535 { return fmt.Errorf( - "%q must be between %d and %d inclusive, got: %d", "rule_number", 1, 65535, rId) + "%q must be between %d and %d inclusive, got: %d", "rule_number", 1, 65535, number) } } } diff --git a/website/docs/r/network_acl_rule.html.markdown b/website/docs/r/network_acl_rule.html.markdown index c3fb7a60..854b0fe6 100644 --- a/website/docs/r/network_acl_rule.html.markdown +++ b/website/docs/r/network_acl_rule.html.markdown @@ -48,7 +48,7 @@ The following arguments are supported: The `rule` block supports: -* `rule_number` - (Optional) The number of the ACL item, its ordering. +* `rule_number` - (Optional) The number of the ACL item used to order the ACL rules. The ACL rule with the lowest number has the highest priority. If not specified, the ACL item will be created with a number one greater than the highest numbered rule. * `action` - (Optional) The action for the rule. Valid options are: `allow` and `deny` (defaults allow).