-
Notifications
You must be signed in to change notification settings - Fork 53
fix: Enhance network ACL rule creation & reading #240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodeBleu
commented
Oct 7, 2025
This commit addresses multiple issues in the `cloudstack_network_acl_rule` resource to improve reliability and compatibility with configurations lacking port specifications: 1. **Fix `aclid` usage in `createNetworkACLRule`**: - Replaced `p.SetAclid(d.Id())` with `p.SetAclid(d.Get("acl_id").(string))` to use the configured `acl_id` instead of the unset resource ID during creation. This resolves CloudStack API error 431 (CSExceptionErrorCode: 9999) caused by an empty `aclid` value. 2. **Support TCP/UDP rules without ports**: - Modified `createNetworkACLRule` to create rules for TCP/UDP protocols when no ports are specified, using a default "all ports" rule with a UUID stored as `all_ports`. This ensures compatibility with configs omitting the optional `ports` attribute. - Updated `resourceCloudStackNetworkACLRuleRead` to handle TCP/UDP rules with no ports, adding them to the state even if the `ports` set is empty. 3. **Add retry logic for API consistency**: - Introduced retry logic in `resourceCloudStackNetworkACLRuleRead` using `retry.RetryContext` to handle eventual consistency in CloudStack's `ListNetworkACLs` API, retrying for up to 30 seconds if the API call fails or returns no rules. 4. **Improve validation in `verifyNetworkACLRuleParams`**: - Relaxed validation to allow empty `ports` for TCP/UDP protocols, aligning with the schema where `ports` is optional. This prevents validation errors for configs without ports. 5. **Enhance logging for debugging**: - Added detailed `[DEBUG]` and `[ERROR]` logs across `resourceCloudStackNetworkACLRuleCreate`, `createNetworkACLRules`, `createNetworkACLRule`, `resourceCloudStackNetworkACLRuleRead`, and `verifyNetworkACLRuleParams` to trace rule creation, validation, and API interactions. - Included rule indices and detailed error messages in `createNetworkACLRules` for better error reporting. 6. **Defer `d.SetId` in `Create`**: - Moved `d.SetId(d.Get("acl_id").(string))` in `resourceCloudStackNetworkACLRuleCreate` to after successful rule creation to avoid premature state updates. These changes resolve the "Provider produced inconsistent result after apply: Root object was present, but now absent" error by ensuring rules are created correctly and consistently read from CloudStack. The fixes also improve robustness for multi-rule configurations and eventual consistency scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Terraform configuration
resource "cloudstack_network_acl_rule" "test" {
acl_id = "343c82e1-be7f-4576-bdb2-d8fa3acec7fa"
rule {
action = "allow"
cidr_list = ["10.0.0.0/24"]
protocol = "tcp"
traffic_type = "ingress"
}
}
Before fix
terraform apply
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
+ create
Terraform will perform the following actions:
# cloudstack_network_acl_rule.test will be created
+ resource "cloudstack_network_acl_rule" "test" {
+ acl_id = "343c82e1-be7f-4576-bdb2-d8fa3acec7fa"
+ id = (known after apply)
+ managed = false
+ parallelism = 2
+ rule {
+ action = "allow"
+ cidr_list = [
+ "10.0.0.0/24",
]
+ icmp_code = (known after apply)
+ icmp_type = (known after apply)
+ ports = []
+ protocol = "tcp"
+ traffic_type = "ingress"
+ uuids = (known after apply)
}
}
Plan: 1 to add, 0 to change, 0 to destroy.
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: yes
cloudstack_network_acl_rule.test: Creating...
╷
│ Error: Provider produced inconsistent result after apply
│
│ When applying changes to cloudstack_network_acl_rule.test, provider "provider[\"registry.terraform.io/cloudstack/cloudstack\"]" produced an unexpected new value: Root object was present, but now absent.
│
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
After fix
terraform apply
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
+ create
Terraform will perform the following actions:
# cloudstack_network_acl_rule.test will be created
+ resource "cloudstack_network_acl_rule" "test" {
+ acl_id = "343c82e1-be7f-4576-bdb2-d8fa3acec7fa"
+ id = (known after apply)
+ managed = false
+ parallelism = 2
+ rule {
+ action = "allow"
+ cidr_list = [
+ "10.0.0.0/24",
]
+ icmp_code = (known after apply)
+ icmp_type = (known after apply)
+ ports = []
+ protocol = "tcp"
+ rule_number = (known after apply)
+ traffic_type = "ingress"
+ uuids = (known after apply)
# (1 unchanged attribute hidden)
}
}
Plan: 1 to add, 0 to change, 0 to destroy.
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: yes
cloudstack_network_acl_rule.test: Creating...
cloudstack_network_acl_rule.test: Creation complete after 1s [id=343c82e1-be7f-4576-bdb2-d8fa3acec7fa]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm except for one thing (and I realise this problem is not introduced in this PR) the methods are too big. If you can please factor out some blocks with their heading comments.
Because there is another WIP PR for this same resource #239, I'm going to go ahead and merge this now because it fixes the existing functionality. I don't want to change things and have to go through more testing again right now so it can hopefully make it into the 0.6.0 release.
Not sure exactly which comments you are referring to, but I feel like comments are good to explain what is going on. If there are some to be refactored out, then maybe with @Pearl1594 PR they can be cleaned up. |