Network ACL Rules modifications cause churn due to being a TypeList

[bhouse-nexthop]

Overview

Inserting a rule in the middle of other rules will cause every rule after to be shown as modified. There is some logic to try to prevent the number of updates shown as being actually applied, but it causes alarm, and its not clear from the output what its actually doing or if its doing it properly.

Its not possible to simply convert the existing TypeList into a TypeSet because without a rule_number specified, the order cannot be preserved (as TypeSet uses a hash of the field). Terraform also doesn't provide any mechanism to do any inspection of the original order to auto-assign rule numbers in the right order for rules without a manually specified rule_number.

The choice was made to create a parallel ruleset schema member with all the same child elements as the rule schema member. The differences are a rule_number becomes a required field and it doesn't need to implement the legacy ports -> port conversion logic.

For the legacy rule TypeList another change was made in order to perform explicit rule_number enumeration instead of relying on Cloudstack to auto-enumerate. When applying new entries in parallel, the order wouldn't be guaranteed to be preserved without this!

This also includes test and documentation updates.

Fixes #279

Usecase

In my particular use-case, I auto-assign rule_numbers based on zones using a module as a helper. The module handles creation of the rule_number for me, and takes a starting index for each block of rules so I can logically group rules together in a "reserved range" for a particular "purpose".

Module Code

modules/cloudstack_network_acl/main.tf:

terraform {
  required_providers {
    cloudstack = {
      source  = "cloudstack/cloudstack"
    }
  }
}

variable "acl_id" {
  description = "ACL ID"
  type        = string
}

variable "managed" {
  description = "Managed"
  type        = bool
  default     = true
}

variable "rulelist" {
  description = "Rule List"
  type        = any
}

resource "cloudstack_network_acl_rule" "this" {
  acl_id             = var.acl_id
  managed            = var.managed
  dynamic "ruleset" {
    for_each = flatten([
        for list in var.rulelist : [
          for rule in list.rules : {
            rule_number  = "${list.start_idx + index(list.rules, rule) + 1}"
            description  = try(rule.description, "")
            action       = rule.action
            cidr_list    = rule.cidr_list
            protocol     = rule.protocol
            icmp_type    = try(rule.icmp_type, null)
            icmp_code    = try(rule.icmp_code, null)
            port         = try(rule.port, null)
            traffic_type = rule.traffic_type
          }
        ]
      ])
    content {
      rule_number  = ruleset.value.rule_number
      description  = "${ruleset.value.description}: ${ruleset.value.action} ${ruleset.value.traffic_type}"
      action       = ruleset.value.action
      cidr_list    = ruleset.value.cidr_list
      protocol     = ruleset.value.protocol
      icmp_type    = ruleset.value.icmp_type
      icmp_code    = ruleset.value.icmp_code
      port         = ruleset.value.port
      traffic_type = ruleset.value.traffic_type
    }
  }
}

Module Caller

myzone.tf:

locals {
  subnet_vpc = "10.252.0.0/16"

  aclrules_deny_all = {
    start_idx = 65500
    rules = [
      {
        description  = "deny egress by default"
        rule_number  = 65535
        action       = "deny"
        cidr_list    = [ "0.0.0.0/0" ]
        protocol     = "all"
        icmp_type    = null
        icmp_code    = null
        traffic_type = "egress"
      }
    ]
  }

  subnet_ntp = "10.0.1.0/24"
  aclrules_access_ntp = {
    start_idx = 1100
    rules     = [
      {
        description  = "ntp"
        action       = "allow"
        cidr_list    = [ local.subnet_ntp ]
        protocol     = "udp"
        port         = "123"
        traffic_type = "egress"
      }
    ]
  }

  subnet_ipa          = "10.0.2.0/24"
  aclrules_access_ipa = {
    start_idx = 1200
    rules     = [
      {
        description  = "IPA http"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "tcp"
        port         = "80"
        traffic_type = "egress"
      },
      {
        description  = "IPA https"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "tcp"
        port         = "443"
        traffic_type = "egress"
      },
      {
        description  = "IPA ldap"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "tcp"
        port         = "389"
        traffic_type = "egress"
      },
      {
        description  = "IPA ldaps"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "tcp"
        port         = "636"
        traffic_type = "egress"
      },
      {
        description  = "kerberos udp"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "udp"
        port         = "88"
        traffic_type = "egress"
      },
      {
        description  = "kpasswd udp"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "udp"
        port         = "464"
        traffic_type = "egress"
      },
      {
        description  = "kerberos tcp"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "tcp"
        port         = "88"
        traffic_type = "egress"
      },
      {
        description  = "kpasswd tcp"
        action       = "allow"
        cidr_list    = [ local.subnet_ipa ]
        protocol     = "tcp"
        port         = "464"
        traffic_type = "egress"
      }
    ]
  }
}

resource "cloudstack_vpc" "infra_vpc" {
  name           = "infra_vpc"
  cidr           = local.subnet_vpc
  vpc_offering   = "VPC HA"
  network_domain = var.cloudstack_network_domain
  zone           = var.cloudstack_zone
  project        = var.cloudstack_project
}

resource "cloudstack_network_acl" "myzone" {
  name   = "myzone"
  vpc_id = cloudstack_vpc.infra_vpc.id
}

module "network_acl_su" {
  source    = "./modules/cloudstack_network_acl"
  acl_id    = cloudstack_network_acl.myzone.id
  managed   = true
  # Order doesn't matter here!
  rulelist  = [ local.aclrules_deny_all, local.aclrules_access_ntp, aclrules_access_ipa ]
}

Explanation of Module usage

This is just describing the example module for how I use this PR. There are other ways to accomplish this especially for short manual lists. The below description is how to use the module in the body of this message, it doesn't directly relate to the PR itself. It can (and was previously) used with the prior implementation by replacing dynamic "ruleset" with dynamic "rule" in the module.

The implementation uses a list of lists, with each inner list having a start_idx so each rule does not need to be enumerated separately but instead is ordered naturally. As long as the start_idx isn't the same as any other start_idx, and the gap between each index is sufficiently large, no numbering should conflict. I recommend having each "block" have at least 100 entries reserved. The maximum rule_number is 65535.

In the above example, creating a new inner list to apply will only insert those rules from the inner list as the rule_number's are guaranteed to be unique, regardless of what index it has in the outer list. However, any ordering change of each inner list would impact all rules after it within the inner list (but have no impact on other lists contained in the outer list). So it is recommended to only append new rules to inner lists unless ordering is critical to prevent mass updates (but again, the blast radius would be limited to any inner list items that come after the entry, not to any other outer lists).

[Copilot]

Pull request overview

This PR addresses plan churn for cloudstack_network_acl_rule by introducing a new ruleset schema field (TypeSet) keyed by rule_number, while preserving the legacy rule (TypeList) behavior and updating docs/tests accordingly.

Changes:

• Add ruleset (TypeSet) with required rule_number to reduce spurious diffs when inserting/removing rules.
• Implement explicit rule_number assignment for legacy rule to avoid CloudStack auto-enumeration order issues.
• Update acceptance tests and documentation to cover and recommend ruleset.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
cloudstack/resource_cloudstack_network_acl_rule.go	Adds ruleset schema + hashing, UUID handling helpers, and rule-number enumeration; updates CRUD/read/update logic to support ruleset.
cloudstack/resource_cloudstack_network_acl_rule_test.go	Adds acceptance tests for ruleset behavior, including an insertion scenario and a plan-check step.
website/docs/r/network_acl_rule.html.markdown	Documents ruleset, its constraints (required rule_number, no deprecated ports), and usage guidance.

Comments suppressed due to low confidence (1)

cloudstack/resource_cloudstack_network_acl_rule.go:1393

• rulesMatch ignores rule_number, cidr_list, and description when pairing old/new rules. With ruleset (and even with rule), it’s common to have multiple rules sharing protocol/action/traffic_type/port; this matching can pair the wrong rules and then “update” an existing CloudStack rule to match a different one, effectively mutating the wrong ACL item. For ruleset, matching should primarily key off rule_number (since it’s required/unique); for legacy rule, consider including rule_number when present and/or include cidr_list in the match criteria to avoid incorrect pairings.

func rulesMatch(oldRule, newRule map[string]interface{}) bool {
	oldProtocol := oldRule["protocol"].(string)
	newProtocol := newRule["protocol"].(string)
	oldTrafficType := oldRule["traffic_type"].(string)
	newTrafficType := newRule["traffic_type"].(string)
	oldAction := oldRule["action"].(string)
	newAction := newRule["action"].(string)

	if oldProtocol != newProtocol ||
		oldTrafficType != newTrafficType ||
		oldAction != newAction {
		return false
	}

	protocol := newProtocol

	if protocol == "tcp" || protocol == "udp" {
		oldPort, oldHasPort := oldRule["port"].(string)
		newPort, newHasPort := newRule["port"].(string)

		if oldHasPort && newHasPort {
			return oldPort == newPort
		}

		if oldHasPort != newHasPort {
			return false
		}

		return true
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[vishesh92]

@bhouse-nexthop Thanks for the PR. Can you check the comments on the PR?

[bhouse-nexthop]

@bhouse-nexthop Thanks for the PR. Can you check the comments on the PR?

Addressed all comments. Please re-run Copilot review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.