Skip to content
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

fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency #1510

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

akazantzidis
Copy link

@akazantzidis akazantzidis commented Aug 24, 2024

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated acceptance tests in /fwprovider/tests for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

Proof of Work

This PR is handling the below 2 issues:
Issue 1:
Rule resources produce the same rule ID:
Example rules:

resource "proxmox_virtual_environment_cluster_firewall_security_group" "test" {
  name    = "test"
  comment = "Managed by Terraform"

  rule {
    type    = "in"
    action  = "REJECT"
    comment = "DENY HTTP 100.35"
    source  = "192.168.100.35"
    dport   = "80"
    proto   = "tcp"
    log     = "info"
  }

  rule {
    type    = "in"
    action  = "REJECT"
    comment = "DENY HTTPS 110.35"
    source  = "192.168.110.35"
    dport   = "443"
    proto   = "tcp"
    log     = "info"
  }

  rule {
    type    = "in"
    action  = "REJECT"
    comment = "DENY HTTPS 130.35"
    dest    = "192.168.130.35"
    dport   = "443"
    proto   = "tcp"
    log     = "info"
  }
}

resource "proxmox_virtual_environment_firewall_rules" "outbound" {
  node_name = "pve"
  container_id = "101"
  rule {
    type    = "out"
    action  = "REJECT"
    comment = "DENY TO HTTP 1.5"
    dest    = "192.168.1.5"
    sport   = "80"
    proto   = "tcp"
    log     = "info"
  }
  rule {
    type    = "out"
    action  = "REJECT"
    comment = "DENY TO HTTPS 1.5"
    dest    = "192.168.1.5"
    dport   = "443"
    proto   = "tcp"
    log     = "info"
  }
  lifecycle {
    ignore_changes = []
  }
}

resource "proxmox_virtual_environment_firewall_rules" "inbound" {
  node_name = "pve"
  container_id = "101"
  rule {
    type    = "in"
    action  = "ACCEPT"
    comment = "Allow HTTP"
    dest    = "0.0.0.0/0"
    dport   = "80"
    proto   = "tcp"
    log     = "info"
  }
  rule {
    type    = "in"
    action  = "ACCEPT"
    comment = "Allow HTTPS"
    dest    = "0.0.0.0/0"
    dport   = "443"
    proto   = "tcp"
    log     = "info"
  }
  rule {
    security_group = "test"
  }
  depends_on = [ 
      proxmox_virtual_environment_cluster_firewall_security_group.test 
      ]
  }

Provider's Output:

Plan: 3 to add, 0 to change, 0 to destroy.
proxmox_virtual_environment_firewall_rules.outbound: Creating...
proxmox_virtual_environment_cluster_firewall_security_group.test: Creating...
proxmox_virtual_environment_firewall_rules.outbound: Creation complete after 0s [id=rule-2020289146] <-
proxmox_virtual_environment_cluster_firewall_security_group.test: Creation complete after 0s [id=test]
proxmox_virtual_environment_firewall_rules.inbound: Creating...
proxmox_virtual_environment_firewall_rules.inbound: Creation complete after 0s [id=rule-2020289146] <-

Issue 2:
Rule checking/state logic is inconsistent, which produces the below issues:

  1. Terraform plan yields continuously that change is detected and needs to be applied when no actual change happen.
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:

  # proxmox_virtual_environment_firewall_rules.inbound will be updated in-place
  ~ resource "proxmox_virtual_environment_firewall_rules" "inbound" {
        id           = "rule-2020289146"
        # (2 unchanged attributes hidden)

      ~ rule {
          - sport   = "80" -> null
            # (9 unchanged attributes hidden)
        }

        # (2 unchanged blocks hidden)
    }

  # proxmox_virtual_environment_firewall_rules.outbound will be updated in-place
  ~ resource "proxmox_virtual_environment_firewall_rules" "outbound" {
        id           = "rule-2020289146"
        # (2 unchanged attributes hidden)

      ~ rule {
          ~ action  = "ACCEPT" -> "REJECT"
          ~ comment = "Allow HTTP" -> "DENY HTTP 1.5"
          ~ dest    = "0.0.0.0/0" -> "192.168.1.5"
          - dport   = "80" -> null
          ~ type    = "in" -> "out"
            # (5 unchanged attributes hidden)
        }
      ~ rule {
          ~ action  = "ACCEPT" -> "REJECT"
          ~ comment = "Allow HTTPS" -> "DENY HTTPS 1.5"
          ~ dest    = "0.0.0.0/0" -> "192.168.1.5"
          ~ type    = "in" -> "out"
            # (5 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 2 to change, 0 to destroy.
proxmox_virtual_environment_firewall_rules.outbound: Modifying... [id=rule-2020289146]
proxmox_virtual_environment_firewall_rules.inbound: Modifying... [id=rule-2020289146]
proxmox_virtual_environment_firewall_rules.inbound: Modifications complete after 0s [id=rule-2020289146]
proxmox_virtual_environment_firewall_rules.outbound: Modifications complete after 1s [id=rule-2020289146]

Apply complete! Resources: 0 added, 2 changed, 0 destroyed.
...
proxmox_virtual_environment_cluster_firewall_security_group.test: Refreshing state... [id=test]
proxmox_virtual_environment_firewall_rules.outbound: Refreshing state... [id=rule-2020289146]
proxmox_virtual_environment_firewall_rules.inbound: Refreshing state... [id=rule-2020289146]

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:

  # proxmox_virtual_environment_firewall_rules.inbound will be updated in-place
  ~ resource "proxmox_virtual_environment_firewall_rules" "inbound" {
        id           = "rule-2020289146"
...
proxmox_virtual_environment_firewall_rules.outbound: Refreshing state... [id=rule-2020289146]
proxmox_virtual_environment_cluster_firewall_security_group.test: Refreshing state... [id=test]
proxmox_virtual_environment_firewall_rules.inbound: Refreshing state... [id=rule-2020289146]

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:

  # proxmox_virtual_environment_firewall_rules.inbound will be updated in-place
  ~ resource "proxmox_virtual_environment_firewall_rules" "inbound" {
        id           = "rule-2020289146"
        # (2 unchanged attributes hidden)

      ~ rule {
          - sport   = "80" -> null
            # (9 unchanged attributes hidden)
        }

        # (2 unchanged blocks hidden)
    }

  # proxmox_virtual_environment_firewall_rules.outbound will be updated in-place
  ~ resource "proxmox_virtual_environment_firewall_rules" "outbound" {
        id           = "rule-2020289146"
        # (2 unchanged attributes hidden)

      ~ rule {
          ~ action  = "ACCEPT" -> "REJECT"
          ~ comment = "Allow HTTP" -> "DENY HTTP 1.5"
          ~ dest    = "0.0.0.0/0" -> "192.168.1.5"
          - dport   = "80" -> null
          ~ type    = "in" -> "out"
            # (5 unchanged attributes hidden)
        }
      ~ rule {
          ~ action  = "ACCEPT" -> "REJECT"
          ~ comment = "Allow HTTPS" -> "DENY HTTPS 1.5"
          ~ dest    = "0.0.0.0/0" -> "192.168.1.5"
          ~ type    = "in" -> "out"
            # (5 unchanged attributes hidden)
        }
    }

Plan: 0 to add, 2 to change, 0 to destroy.
  1. Errors during rule delete/update operations : ... no rule at position X ...
proxmox_virtual_environment_firewall_rules.outbound: Refreshing state... [id=rule-2020289146]
proxmox_virtual_environment_cluster_firewall_security_group.test: Refreshing state... [id=test]
proxmox_virtual_environment_firewall_rules.inbound: Refreshing state... [id=rule-2020289146]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following
symbols:
  - destroy

Terraform will perform the following actions:

  # proxmox_virtual_environment_cluster_firewall_security_group.test will be destroyed
  - resource "proxmox_virtual_environment_cluster_firewall_security_group" "test" {
      - comment = "Managed by Terraform" -> null
      - id      = "test" -> null
      - name    = "test" -> null

      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTP 100.35" -> null
          - dport   = "80" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 0 -> null
          - proto   = "tcp" -> null
          - source  = "192.168.100.35" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTPS 110.35" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 1 -> null
          - proto   = "tcp" -> null
          - source  = "192.168.110.35" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTPS 130.35" -> null
          - dest    = "192.168.130.35" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 2 -> null
          - proto   = "tcp" -> null
          - type    = "in" -> null
        }
    }

  # proxmox_virtual_environment_firewall_rules.inbound will be destroyed
  - resource "proxmox_virtual_environment_firewall_rules" "inbound" {
      - container_id = 101 -> null
      - id           = "rule-2020289146" -> null
      - node_name    = "pve" -> null

      - rule {
          - action  = "ACCEPT" -> null
          - comment = "Allow HTTP" -> null
          - dest    = "0.0.0.0/0" -> null
          - dport   = "80" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 0 -> null
          - proto   = "tcp" -> null
          - sport   = "80" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "ACCEPT" -> null
          - comment = "Allow HTTPS" -> null
          - dest    = "0.0.0.0/0" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 1 -> null
          - proto   = "tcp" -> null
          - type    = "in" -> null
        }
      - rule {
          - enabled        = true -> null
          - pos            = 2 -> null
          - security_group = "test" -> null
        }
    }

  # proxmox_virtual_environment_firewall_rules.outbound will be destroyed
  - resource "proxmox_virtual_environment_firewall_rules" "outbound" {
      - container_id = 101 -> null
      - id           = "rule-2020289146" -> null
      - node_name    = "pve" -> null

      - rule {
          - action  = "ACCEPT" -> null
          - comment = "Allow HTTP" -> null
          - dest    = "0.0.0.0/0" -> null
          - dport   = "80" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 0 -> null
          - proto   = "tcp" -> null
          - sport   = "80" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "ACCEPT" -> null
          - comment = "Allow HTTPS" -> null
          - dest    = "0.0.0.0/0" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 1 -> null
          - proto   = "tcp" -> null
          - type    = "in" -> null
        }
    }

Plan: 0 to add, 0 to change, 3 to destroy.
proxmox_virtual_environment_firewall_rules.outbound: Destroying... [id=rule-2020289146]
proxmox_virtual_environment_firewall_rules.inbound: Destroying... [id=rule-2020289146]
proxmox_virtual_environment_firewall_rules.outbound: Destruction complete after 0s
Error: error deleting firewall rule 2: received an HTTP 500 response - Reason: no rule at position 2

Error: error deleting firewall rule 1: received an HTTP 500 response - Reason: no rule at position 1 

This PR fixes the rule unique id creation and the state/rule consistency and as result no more errors during rule delete/update operations.

  1. Unique id of rule resources:
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:

  # proxmox_virtual_environment_cluster_firewall_security_group.test will be created
  + resource "proxmox_virtual_environment_cluster_firewall_security_group" "test" {
      + comment = "Managed by Terraform"
      + id      = (known after apply)
      + name    = "test"

      + rule {
          + action  = "REJECT"
          + comment = "DENY HTTP 100.35"
          + dport   = "80"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + source  = "192.168.100.35"
          + type    = "in"
        }
      + rule {
          + action  = "REJECT"
          + comment = "DENY HTTPS 110.35"
          + dport   = "443"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + source  = "192.168.110.35"
          + type    = "in"
        }
      + rule {
          + action  = "REJECT"
          + comment = "DENY HTTPS 130.35"
          + dest    = "192.168.130.35"
          + dport   = "443"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + type    = "in"
        }
    }

  # proxmox_virtual_environment_firewall_rules.inbound will be created
  + resource "proxmox_virtual_environment_firewall_rules" "inbound" {
      + container_id = 101
      + id           = (known after apply)
      + node_name    = "pve"

      + rule {
          + action  = "ACCEPT"
          + comment = "Allow HTTP"
          + dest    = "0.0.0.0/0"
          + dport   = "80"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + type    = "in"
        }
      + rule {
          + action  = "ACCEPT"
          + comment = "Allow HTTPS"
          + dest    = "0.0.0.0/0"
          + dport   = "443"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + type    = "in"
        }
      + rule {
          + enabled        = true
          + pos            = (known after apply)
          + security_group = "test"
        }
    }

  # proxmox_virtual_environment_firewall_rules.outbound will be created
  + resource "proxmox_virtual_environment_firewall_rules" "outbound" {
      + container_id = 101
      + id           = (known after apply)
      + node_name    = "pve"

      + rule {
          + action  = "REJECT"
          + comment = "DENY HTTP 1.5"
          + dest    = "192.168.1.5"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + sport   = "80"
          + type    = "out"
        }
      + rule {
          + action  = "REJECT"
          + comment = "DENY HTTPS 1.5"
          + dest    = "192.168.1.5"
          + dport   = "443"
          + enabled = true
          + log     = "info"
          + pos     = (known after apply)
          + proto   = "tcp"
          + type    = "out"
        }
    }

Plan: 3 to add, 0 to change, 0 to destroy.
proxmox_virtual_environment_firewall_rules.outbound: Creating...
proxmox_virtual_environment_cluster_firewall_security_group.test: Creating...
proxmox_virtual_environment_firewall_rules.outbound: Creation complete after 1s [id=role-1724524866788386] <-
proxmox_virtual_environment_cluster_firewall_security_group.test: Creation complete after 1s [id=test]
proxmox_virtual_environment_firewall_rules.inbound: Creating...
proxmox_virtual_environment_firewall_rules.inbound: Creation complete after 0s [id=role-1724523131556197] <-

Apply complete! Resources: 3 added, 0 changed, 0 destroyed.
  1. Terraform plan is now only creating diff when actual changes exist:
proxmox_virtual_environment_firewall_rules.outbound: Refreshing state... [id=role-1724524866788386]
proxmox_virtual_environment_cluster_firewall_security_group.test: Refreshing state... [id=test]
proxmox_virtual_environment_firewall_rules.inbound: Refreshing state... [id=role-1724523131556197]

No changes. Your infrastructure matches the configuration.
  1. Terraform update/delete operations do not yield position errors any more:
proxmox_virtual_environment_cluster_firewall_security_group.test: Refreshing state... [id=test]
proxmox_virtual_environment_firewall_rules.outbound: Refreshing state... [id=role-1724524866788386]
proxmox_virtual_environment_firewall_rules.inbound: Refreshing state... [id=role-1724523131556197]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following
symbols:
  - destroy

Terraform will perform the following actions:

  # proxmox_virtual_environment_cluster_firewall_security_group.test will be destroyed
  - resource "proxmox_virtual_environment_cluster_firewall_security_group" "test" {
      - comment = "Managed by Terraform" -> null
      - id      = "test" -> null
      - name    = "test" -> null

      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTP 100.35" -> null
          - dport   = "80" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 0 -> null
          - proto   = "tcp" -> null
          - source  = "192.168.100.35" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTPS 110.35" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 1 -> null
          - proto   = "tcp" -> null
          - source  = "192.168.110.35" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTPS 130.35" -> null
          - dest    = "192.168.130.35" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 2 -> null
          - proto   = "tcp" -> null
          - type    = "in" -> null
        }
    }

  # proxmox_virtual_environment_firewall_rules.inbound will be destroyed
  - resource "proxmox_virtual_environment_firewall_rules" "inbound" {
      - container_id = 101 -> null
      - id           = "role-1724523131556197" -> null
      - node_name    = "pve" -> null

      - rule {
          - action  = "ACCEPT" -> null
          - comment = "Allow HTTP" -> null
          - dest    = "0.0.0.0/0" -> null
          - dport   = "80" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 0 -> null
          - proto   = "tcp" -> null
          - type    = "in" -> null
        }
      - rule {
          - action  = "ACCEPT" -> null
          - comment = "Allow HTTPS" -> null
          - dest    = "0.0.0.0/0" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 1 -> null
          - proto   = "tcp" -> null
          - type    = "in" -> null
        }
      - rule {
          - enabled        = true -> null
          - pos            = 2 -> null
          - security_group = "test" -> null
        }
    }

  # proxmox_virtual_environment_firewall_rules.outbound will be destroyed
  - resource "proxmox_virtual_environment_firewall_rules" "outbound" {
      - container_id = 101 -> null
      - id           = "role-1724524866788386" -> null
      - node_name    = "pve" -> null

      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTP 1.5" -> null
          - dest    = "192.168.1.5" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 0 -> null
          - proto   = "tcp" -> null
          - sport   = "80" -> null
          - type    = "out" -> null
        }
      - rule {
          - action  = "REJECT" -> null
          - comment = "DENY HTTPS 1.5" -> null
          - dest    = "192.168.1.5" -> null
          - dport   = "443" -> null
          - enabled = true -> null
          - log     = "info" -> null
          - pos     = 1 -> null
          - proto   = "tcp" -> null
          - type    = "out" -> null
        }
    }

Plan: 0 to add, 0 to change, 3 to destroy.
proxmox_virtual_environment_firewall_rules.outbound: Destroying... [id=role-1724524866788386]
proxmox_virtual_environment_firewall_rules.inbound: Destroying... [id=role-1724523131556197]
proxmox_virtual_environment_firewall_rules.outbound: Destruction complete after 0s
proxmox_virtual_environment_firewall_rules.inbound: Destruction complete after 0s
proxmox_virtual_environment_cluster_firewall_security_group.test: Destroying... [id=test]
proxmox_virtual_environment_cluster_firewall_security_group.test: Destruction complete after 1s

Destroy complete! Resources: 3 destroyed.

FIXES #1504

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

@akazantzidis akazantzidis changed the title fix(firewall/rules) - Update firewall logic to handle unique rule ids, rules and terraform state with consistency. fix(firewall/rules): Update firewall logic to handle unique rule ids, rules and terraform state with consistency. Aug 24, 2024
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @akazantzidis 👋🏼

Thanks for the contribution! I have a question about the code, could you take a look? Thanks!

proxmox/firewall/rules.go Outdated Show resolved Hide resolved
func RulesRead(ctx context.Context, api firewall.Rule, d *schema.ResourceData) diag.Diagnostics {
var diags diag.Diagnostics

readRule := func(pos int, ruleMap map[string]interface{}) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. I didn't quite get this change. The RulesRead supposed to read rules from PVE API. Now it reads back only from the state 🤔

Copy link
Author

@akazantzidis akazantzidis Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sure let me explain:

rulesCreate function was resetting the rule state before calling RulesRead. This was redundant because the rules were already created in specific order in one take within rulesCreate and represented correctly in d. However, the correct pos value for each rule was not being set at that time.

By removing the state reset at the end of rulesCreate, rules are represented in the correct (in terms of user configuration) order.
Calling RulesRead we can iterate over the rules and for each rule id increment the pos value for each rule to set the actual rule pos.
This sets the pos attribute, and the in the correct order (according to user specified configuration),without reading them from the API.

Copy link
Author

@akazantzidis akazantzidis Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this proposal does not handle changes outside of Terraform, does stabilizes the terraform plan / apply / delete.
Next step though is the rule comparison in terms of API/state-configuration ,so outside changes to be reverted and to be ensured the terraform configuration.

@kbcz1989
Copy link

@akazantzidis thank you for working on this. I tested your changes and they do not solve the issue described in bug report (yet?).

I managed to get it working for almost all the required attributes, except for "interface", in this commit:
kbcz1989@d3306da

The issue with "interface" is that its not enough to send it with an empty value but instead "delete=iface" is required.

@akazantzidis akazantzidis marked this pull request as draft September 6, 2024 16:15
@akazantzidis
Copy link
Author

Moving it to in-progress as working in a different approach

@bpg bpg changed the title fix(firewall/rules): Update firewall logic to handle unique rule ids, rules and terraform state with consistency. fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency. Sep 8, 2024
@bpg bpg changed the title fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency. fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firewall rule attributes cannot be removed / perpetual change plan
3 participants