Bugfix: Implement egress firewall cidr validation by elivlo · Pull Request #297 · apache/cloudstack-terraform-provider

elivlo · 2026-06-25T08:30:00Z

Hi :)

This PR adds validation to the egress firewall for the set cidr_list. The set currently allows the cidr 0.0.0.0/0 that get is rewritten to the cidr of the network. So the resource gets recreated everytime terraform runs.

So this PR will disallow the cidr 0.0.0.0/0.

Copilot

Pull request overview

This PR addresses a CloudStack egress firewall drift issue by rejecting the special CIDR 0.0.0.0/0 in cidr_list, which CloudStack rewrites to the network CIDR and can cause perpetual Terraform diffs/recreates.

Changes:

Add validation in egress firewall rule parameter verification to disallow 0.0.0.0/0 in cidr_list.
Add unit tests covering the new CIDR validation behavior (disallow 0.0.0.0/0, allow a normal subnet CIDR).
Ignore a local .worktrees/ directory via .gitignore.

Reviewed changes

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

File	Description
cloudstack/resource_cloudstack_egress_firewall.go	Adds rule-level validation rejecting `0.0.0.0/0` for egress firewall `cidr_list` (and cleans up unused params).
cloudstack/resource_cloudstack_egress_firewall_test.go	Adds unit tests for the new CIDR validation behavior.
.gitignore	Ignores `.worktrees/` directory.

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

artem-sidorenko · 2026-07-10T10:03:19Z

@elivlo the 0.0.0.0/0 was a very nice hack I used several times to avoid specification of specific subnet, now it's gone :-D

weizhouapache · 2026-08-17T09:45:09Z

@elivlo
IMHO, if cidr 0.0.0.0/0 is allowed in cloudstack ,disallowing it in Terraform does not make sense.

weizhouapache · 2026-08-17T09:46:43Z

@elivlo the 0.0.0.0/0 was a very nice hack I used several times to avoid specification of specific subnet, now it's gone :-D

@artem-sidorenko
just to confirm, did you mean it's gone with this PR, or in the upstream ?

artem-sidorenko · 2026-08-17T13:14:47Z

@weizhouapache yes, this PR will block that. Cloudstack accepts that, however as far I understood that it's more a side-effect than a real decision to have it this way. I didn't see that in the docs of cloudstack as well

artem-sidorenko · 2026-08-17T13:17:06Z

@weizhouapache this PR makes here sense anyway. You can't really use the 0.0.0.0/0 with terraform - terraform would get the real CIDR from cloudtack back (and not 0.0.0.0/0) and it will enforce updates all the time. It was kind of joke to @elivlo (we are colleagues)

weizhouapache · 2026-08-17T14:36:25Z

thanks @artem-sidorenko
I checked the cloudstack source code, there is indeed a line below

Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());

If the source cidr list is empty, or contain "0.0.0.0/0", it will be replaced with network cidr.

I am not a terraform user, so leave to you guys to determine whether it is good to disallow 0.0.0.0/0, and maybe empty source cidr too.

elivlo · 2026-08-17T14:40:14Z

@weizhouapache this PR makes here sense anyway. You can't really use the 0.0.0.0/0 with terraform - terraform would get the real CIDR from cloudtack back (and not 0.0.0.0/0) and it will enforce updates all the time. It was kind of joke to @elivlo (we are colleagues)

@weizhouapache That's exactly the reason why I suggest the change. 0.0.0.0/0 is silently replaced by Cloudstack so adding validation in terraform would prevent that. It could, however, break the usage of the provider. But since it still is pre v1 it should be allowed to change the behavior.

elivlo · 2026-08-17T14:44:14Z

thanks @artem-sidorenko I checked the cloudstack source code, there is indeed a line below
Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());
If the source cidr list is empty, or contain "0.0.0.0/0", it will be replaced with network cidr.

I am not a terraform user, so leave to you guys to determine whether it is good to disallow 0.0.0.0/0, and maybe empty source cidr too.

Good catch! I would not change this in cloudstack source code so the experience in the UI will stay simple and familiar.

sureshanaparti · 2026-08-18T06:53:57Z

thanks @artem-sidorenko I checked the cloudstack source code, there is indeed a line below
Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());
If the source cidr list is empty, or contain "0.0.0.0/0", it will be replaced with network cidr.

I am not a terraform user, so leave to you guys to determine whether it is good to disallow 0.0.0.0/0, and maybe empty source cidr too.

@weizhouapache is this documented anywhere in cloudstack docs? in that case, we should have this behavior consistent with the terraform as well.

weizhouapache · 2026-08-18T07:06:07Z

thanks @artem-sidorenko I checked the cloudstack source code, there is indeed a line below
Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());
If the source cidr list is empty, or contain "0.0.0.0/0", it will be replaced with network cidr.
I am not a terraform user, so leave to you guys to determine whether it is good to disallow 0.0.0.0/0, and maybe empty source cidr too.
@weizhouapache is this documented anywhere in cloudstack docs? in that case, we should have this behavior consistent with the terraform as well.

@sureshanaparti
in the doc, it mentions

https://docs.cloudstack.apache.org/en/latest/adminguide/networking_and_traffic.html#egress-fw-rules

sudo87 · 2026-08-18T07:07:26Z

@elivlo I think this PR only fixes half of the issue. The same problem also occurs when cidr_list is empty.

As @weizhouapache confirmed from the CloudStack source, an empty CIDR list gets rewritten to the network CIDR, just like 0.0.0.0/0:

Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());

Since cidr_list is optional and omitted when empty, CloudStack adds the network CIDR back, and Read then puts it into state. This results in a perpetual diff for configs where cidr_list is omitted.

Could we also reject an empty cidr_list and add a test for this case?

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cloudstack/resource_cloudstack_egress_firewall.go:595

The error message implies cidr_list is validated to be within the network subnet, but the code only checks for the specific value 0.0.0.0/0. This can mislead users who provide other out-of-subnet CIDRs that will still pass validation. Consider tightening the wording to describe the actual constraint (disallowing 0.0.0.0/0 because it gets rewritten).

			return fmt.Errorf("CIDR 0.0.0.0/0 is not allowed in egress firewall rules. cidr_list must be within the network subnet")

elivlo · 2026-08-19T09:02:28Z

@elivlo I think this PR only fixes half of the issue. The same problem also occurs when cidr_list is empty.

As @weizhouapache confirmed from the CloudStack source, an empty CIDR list gets rewritten to the network CIDR, just like 0.0.0.0/0:
Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr());
Since cidr_list is optional and omitted when empty, CloudStack adds the network CIDR back, and Read then puts it into state. This results in a perpetual diff for configs where cidr_list is omitted.

Could we also reject an empty cidr_list and add a test for this case?

I have made some changes based on the comment.
@sudo87 Is this, what you meant?

weizhouapache · 2026-08-19T09:06:44Z

hope other users will not be unhappy with it

BREAKING CHANGE: cidr_list is now required and must be a non-empty
list of valid subnet CIDRs. 0.0.0.0/0 is no longer accepted.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

cloudstack/resource_cloudstack_egress_firewall.go:70

Changing cidr_list from Optional to Required is a breaking schema change for existing configurations that previously omitted it. If the primary goal is only to prevent 0.0.0.0/0 drift, consider keeping cidr_list optional and enforcing constraints via schema validation (e.g., Elem validation to reject 0.0.0.0/0, plus MinItems: 1 only if the field is present / if empty is invalid), or document this explicitly as a breaking change (changelog/upgrade note) if the requirement is intentional.

						"cidr_list": {
							Type:     schema.TypeSet,
							Required: true,
							Elem:     &schema.Schema{Type: schema.TypeString},
							Set:      schema.HashString,
						},

cloudstack/resource_cloudstack_egress_firewall.go:601

The validation only runs if rule[\"cidr_list\"] is a *schema.Set. If the value is missing or has an unexpected type, the function silently skips validation (including the new 0.0.0.0/0 rejection). Since cidr_list is now required, it would be safer to treat a missing/wrong-typed cidr_list as an error (rather than skipping), so invalid inputs can’t bypass the new guardrails.

func verifyEgressFirewallRuleParams(_ *schema.ResourceData, rule map[string]interface{}) error {
	if cidrList, ok := rule["cidr_list"].(*schema.Set); ok {
		if cidrList.Len() == 0 {
			return fmt.Errorf("cidr_list must not be empty in egress firewall rules")
		}
		if cidrList.Contains("0.0.0.0/0") {
			return fmt.Errorf("CIDR 0.0.0.0/0 is not allowed in egress firewall rules. cidr_list must be within the network subnet")
		}
	}

cloudstack/resource_cloudstack_egress_firewall.go:598

This error message claims cidr_list "must be within the network subnet", but the code does not actually validate subnet membership—only that 0.0.0.0/0 is disallowed. Consider rewording to reflect the real constraint (e.g., explain that 0.0.0.0/0 is rewritten by CloudStack and causes perpetual diffs) to avoid misleading users.

			return fmt.Errorf("CIDR 0.0.0.0/0 is not allowed in egress firewall rules. cidr_list must be within the network subnet")

elivlo · 2026-08-19T11:39:35Z

hope other users will not be unhappy with it

BREAKING CHANGE: cidr_list is now required and must be a non-empty
list of valid subnet CIDRs. 0.0.0.0/0 is no longer accepted.

Yeah that could happen. Luckily we are pre v1 and we rather should fix the bug now than later.

CloudStack silently rewrites an egress firewall rule's cidr_list to the network's CIDR whenever it is empty or contains 0.0.0.0/0: Collections.replaceAll(sourceCidrs, "0.0.0.0/0", network.getCidr()); Since Terraform reads this rewritten value back from CloudStack, a config that omits cidr_list or sets 0.0.0.0/0 causes a perpetual diff on every plan/apply, as Terraform keeps trying to reconcile state with the rewritten CIDR CloudStack actually stored. To prevent this, cidr_list is now required and must contain at least one entry, and 0.0.0.0/0 is explicitly rejected so that CIDR values must be within the network's own subnet. BREAKING CHANGE: cidr_list is now required and must be a non-empty list of valid subnet CIDRs. 0.0.0.0/0 is no longer accepted.

artem-sidorenko · 2026-08-19T11:58:45Z

hope other users will not be unhappy with it

BREAKING CHANGE: cidr_list is now required and must be a non-empty
list of valid subnet CIDRs. 0.0.0.0/0 is no longer accepted.

I don't think this would be the case for many people - this would mean they are accepting unneeded changing terraform states all the time and this doesn't disturb them (exactly this was the reason for this MR)

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

cloudstack/resource_cloudstack_egress_firewall.go:71

The new cidr_list constraints are enforced only in verifyEgressFirewallRuleParams, which typically runs during create/update rather than at plan-time. Consider moving these checks into schema-level validation (e.g., MinItems: 1 for non-empty and an element-level ValidateFunc/ValidateDiagFunc to reject 0.0.0.0/0) so users get earlier, more consistent diagnostics and to reduce reliance on type assertions in runtime verification.

						"cidr_list": {
							Type:     schema.TypeSet,
							Required: true,
							Elem:     &schema.Schema{Type: schema.TypeString},
							Set:      schema.HashString,
						},

cloudstack/resource_cloudstack_egress_firewall.go:601

This error message is quite long for CLI output. Consider shortening it while keeping the actionable bit first (e.g., “cidr_list may not include 0.0.0.0/0”) and optionally referencing the rewrite/diff rationale after that, so it remains readable in Terraform plan/apply logs.

		return fmt.Errorf("CIDR 0.0.0.0/0 is not allowed in egress firewall rules, as CloudStack silently rewrites it to the network CIDR, causing perpetual Terraform diffs")
	}

 	"testing"

 	"github.com/apache/cloudstack-go/v2/cloudstack"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
 )


sureshanaparti added the networking label Jul 10, 2026

sureshanaparti requested a lite review from Copilot July 10, 2026 08:04

Copilot started reviewing on behalf of sureshanaparti July 10, 2026 08:04 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

sureshanaparti requested a review from vishesh92 July 10, 2026 08:21

sureshanaparti added this to the v0.7.0 milestone Jul 10, 2026

sureshanaparti added needs-testing needs-review labels Jul 10, 2026

sureshanaparti requested a review from weizhouapache August 14, 2026 08:26

sudo87 closed this Aug 17, 2026

sudo87 reopened this Aug 17, 2026

sureshanaparti added the status:waiting-for-author label Aug 17, 2026

sureshanaparti removed this from the v0.7.0 milestone Aug 17, 2026

Copilot AI review requested due to automatic review settings August 18, 2026 06:51

Copilot AI reviewed Aug 18, 2026

View reviewed changes

Comment thread cloudstack/resource_cloudstack_egress_firewall_test.go

Copilot started reviewing on behalf of sudo87 August 18, 2026 07:17 View session

Copilot AI review requested due to automatic review settings August 19, 2026 06:36

elivlo force-pushed the bugfix/egress-firewall-cidr-validation branch from 8666bbd to 4a1f54c Compare August 19, 2026 06:41

Copilot started reviewing on behalf of elivlo August 19, 2026 06:47 View session

Copilot AI reviewed Aug 19, 2026

View reviewed changes

elivlo force-pushed the bugfix/egress-firewall-cidr-validation branch from 4a1f54c to 0b8ef01 Compare August 19, 2026 09:00

Copilot AI review requested due to automatic review settings August 19, 2026 09:00

Copilot AI reviewed Aug 19, 2026

View reviewed changes

Copilot started reviewing on behalf of elivlo August 19, 2026 10:51 View session

elivlo force-pushed the bugfix/egress-firewall-cidr-validation branch from 0b8ef01 to 9547eb1 Compare August 19, 2026 11:37

Copilot AI review requested due to automatic review settings August 19, 2026 11:42

elivlo force-pushed the bugfix/egress-firewall-cidr-validation branch from 9547eb1 to feca20f Compare August 19, 2026 11:42

Copilot AI reviewed Aug 19, 2026

View reviewed changes

Uh oh!

Conversation

elivlo commented Jun 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

artem-sidorenko commented Jul 10, 2026

Uh oh!

weizhouapache commented Aug 17, 2026

Uh oh!

weizhouapache commented Aug 17, 2026

Uh oh!

artem-sidorenko commented Aug 17, 2026

Uh oh!

artem-sidorenko commented Aug 17, 2026

Uh oh!

weizhouapache commented Aug 17, 2026

Uh oh!

elivlo commented Aug 17, 2026

Uh oh!

elivlo commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sureshanaparti commented Aug 18, 2026

Uh oh!

weizhouapache commented Aug 18, 2026

Uh oh!

sudo87 commented Aug 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

elivlo commented Aug 19, 2026

Uh oh!

weizhouapache commented Aug 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

elivlo commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artem-sidorenko commented Aug 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

elivlo commented Aug 17, 2026 •

edited

Loading

elivlo commented Aug 19, 2026 •

edited

Loading