-
Notifications
You must be signed in to change notification settings - Fork 52
Add logic to allow updating ACL rule items and disallow port list #239
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
@bradh352 would you be able to help review this? This fix is for the issue was raised against 0.6.0-rc2 regarding updateNetworkACLItems not being used as well as disallowing using ports list to map with how ACS API uses it. |
At first glance this is a much better approach. I'll see if I can carve out some time tomorrow to test it out. That said, I do see a test failed here that probably needs to be fixed first. |
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
Applied the following config , creation and updating worked fine
resource "cloudstack_network_acl_rule" "test" {
acl_id = "b70ac793-4bf1-434d-a75d-baf7e959a1c8"
rule {
rule_number = 16
action = "allow"
cidr_list = ["10.0.0.0/24"]
protocol = "tcp"
port = "81-83"
traffic_type = "ingress"
description = "testing terraform ACL issue-new-1"
}
rule {
rule_number = 17
action = "allow"
cidr_list = ["10.0.0.0/24"]
protocol = "tcp"
port = "2222-2223"
traffic_type = "ingress"
description = "testing terraform ACL issue - 2dsg"
}
rule {
rule_number = 18
action = "allow"
cidr_list = ["10.0.0.0/24"]
protocol = "tcp"
port = "8081"
traffic_type = "ingress"
description = "testing terraform ACL issue - 2"
}
rule {
rule_number = 19
action = "allow"
cidr_list = ["10.0.0.0/24"]
protocol = "tcp"
port = "8086"
traffic_type = "Egress"
description = "testing terraform ACL issue - 3"
}
}
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 = "b70ac793-4bf1-434d-a75d-baf7e959a1c8"
+ id = (known after apply)
+ managed = false
+ parallelism = 2
+ rule {
+ action = "allow"
+ cidr_list = [
+ "10.0.0.0/24",
]
+ description = "testing terraform ACL issue-new"
+ icmp_code = (known after apply)
+ icmp_type = (known after apply)
+ port = "80-82"
+ protocol = "tcp"
+ rule_number = 16
+ traffic_type = "ingress"
+ uuids = (known after apply)
}
+ rule {
+ action = "allow"
+ cidr_list = [
+ "10.0.0.0/24",
]
+ description = "testing terraform ACL issue - 1"
+ icmp_code = (known after apply)
+ icmp_type = (known after apply)
+ port = "2222-2223"
+ protocol = "tcp"
+ rule_number = 17
+ traffic_type = "ingress"
+ uuids = (known after apply)
}
+ rule {
+ action = "allow"
+ cidr_list = [
+ "10.0.0.0/24",
]
+ description = "testing terraform ACL issue - 2"
+ icmp_code = (known after apply)
+ icmp_type = (known after apply)
+ port = "8080"
+ protocol = "tcp"
+ rule_number = 18
+ traffic_type = "ingress"
+ uuids = (known after apply)
}
+ rule {
+ action = "allow"
+ cidr_list = [
+ "10.0.0.0/24",
]
+ description = "testing terraform ACL issue - 3"
+ icmp_code = (known after apply)
+ icmp_type = (known after apply)
+ port = "8081"
+ protocol = "tcp"
+ rule_number = 19
+ 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...
cloudstack_network_acl_rule.test: Creation complete after 3s [id=b70ac793-4bf1-434d-a75d-baf7e959a1c8]
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
╭─ ~/Desktop/cloudstack-India-demo/cloudstack-terraform-copy ✔ ╱ 5s ╱ Azure subscription 1 ╱ 12:47:21 PM
╰─ terraform apply
cloudstack_network_acl_rule.test: Refreshing state... [id=b70ac793-4bf1-434d-a75d-baf7e959a1c8]
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# cloudstack_network_acl_rule.test will be updated in-place
~ resource "cloudstack_network_acl_rule" "test" {
id = "b70ac793-4bf1-434d-a75d-baf7e959a1c8"
# (3 unchanged attributes hidden)
~ rule {
~ description = "testing terraform ACL issue-new" -> "testing terraform ACL issue-new-1"
~ port = "80-82" -> "81-83"
# (9 unchanged attributes hidden)
}
~ rule {
~ description = "testing terraform ACL issue - 1" -> "testing terraform ACL issue - 2dsg"
# (10 unchanged attributes hidden)
}
~ rule {
~ port = "8080" -> "8081"
# (10 unchanged attributes hidden)
}
~ rule {
~ port = "8081" -> "8086"
# (10 unchanged attributes hidden)
}
}
Plan: 0 to add, 1 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: Modifying... [id=b70ac793-4bf1-434d-a75d-baf7e959a1c8]
cloudstack_network_acl_rule.test: Modifications complete after 2s [id=b70ac793-4bf1-434d-a75d-baf7e959a1c8]
@Pearl1594 can you update the documentation (ports to port) , and fix the test run I think the test related acl_rule should also be fixed |
@Pearl1594, the tests are failing. Could you please look into it Also, will it affect the following pr as it's the same resource type |
closing this PR - opening a new one that simplifies the logic and taken into consideration latest changes. |
This PR attempts to deprecate the usage of "ports" with the introduction of "rule_number" support in the 0.6.0 release.
Caution has been taken to allow backward compatibility for delete operation. However creation of new config and update is disallowed if
ports
is used. Users will be prompted to migrate to useport
parameter.