Copilot commented on code in PR #294:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/294#discussion_r3557241266
##########
cloudstack/resource_cloudstack_security_group_rule.go:
##########
@@ -393,6 +403,32 @@ func readSecurityGroupRule(sg *cloudstack.SecurityGroup,
ruleIndex map[string]in
uuids := rule["uuids"].(map[string]interface{})
sgRules := append(sg.Ingressrule, sg.Egressrule...)
+ if rule["protocol"].(string) == "all" {
+ id, ok := uuids[uuid+"all"]
+ if !ok {
+ return
+ }
+
+ // Get the rule
+ idx, ok := ruleIndex[id.(string)]
+ if !ok {
+ return
+ }
Review Comment:
In readSecurityGroupRule()'s `protocol == "all"` branch, if the stored UUID
no longer exists in the security group (missing from ruleIndex), the code
returns without deleting the stale `uuids[uuid+"all"]` entry. The icmp/tcp/udp
branches remove stale UUIDs, so this inconsistency can leave state stuck
referencing a non-existent rule.
##########
cloudstack/resource_cloudstack_security_group_rule.go:
##########
@@ -610,6 +646,8 @@ func verifySecurityGroupRuleParams(d *schema.ResourceData,
rule map[string]inter
protocol := rule["protocol"].(string)
switch protocol {
+ case "all":
+ break
case "icmp":
if _, ok := rule["icmp_type"]; !ok {
Review Comment:
Now that `protocol = "all"` is accepted in this switch, the fallback error
message later in verifySecurityGroupRuleParams() (for invalid protocol strings)
still lists only tcp/udp/icmp as valid options. That message should be updated
to include `all` (and ideally mention numeric protocol values are accepted
too), otherwise users will get a misleading error when they mistype the value.
##########
cloudstack/resource_cloudstack_security_group_rule_test.go:
##########
@@ -40,27 +40,33 @@ func TestAccCloudStackSecurityGroupRule_basic(t *testing.T)
{
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackSecurityGroupRulesExist("cloudstack_security_group.foo"),
resource.TestCheckResourceAttr(
-
"cloudstack_security_group_rule.foo", "rule.#", "2"),
+
"cloudstack_security_group_rule.foo", "rule.#", "3"),
resource.TestCheckResourceAttr(
-
"cloudstack_security_group_rule.foo", "rule.0.cidr_list.0", "172.18.100.0/24"),
+
"cloudstack_security_group_rule.foo", "rule.0.protocol", "all"),
resource.TestCheckResourceAttr(
-
"cloudstack_security_group_rule.foo", "rule.0.protocol", "tcp"),
+
"cloudstack_security_group_rule.foo", "rule.0.cidr_list.0", "172.0.0.0/8"),
resource.TestCheckResourceAttr(
-
"cloudstack_security_group_rule.foo", "rule.0.ports.#", "1"),
+
"cloudstack_security_group_rule.foo", "rule.0.traffic_type", "egress"),
Review Comment:
These acceptance tests assert specific element ordering for `rule` (e.g.,
`rule.0`, `rule.1`), but the schema defines `rule` as a `TypeSet` (unordered).
Adding the new `protocol = "all"` rule is likely to change the set hash
ordering and make the test flaky or fail depending on Terraform/SDK ordering.
Prefer `resource.TestCheckTypeSetElemNestedAttrs` (as used in other tests like
resource_cloudstack_network_acl_rule_test.go) to assert rule presence without
relying on indices.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
