hetznercloud / terraform-provider-hcloud

Terraform Hetzner Cloud provider
https://registry.terraform.io/providers/hetznercloud/hcloud/latest
Mozilla Public License 2.0
479 stars 72 forks source link

[Bug]: Firewall rules DELETED when name or labels in hcloud_firewall resource are changed #931

Open borisceranic opened 1 month ago

borisceranic commented 1 month ago

What happened?

While working with hcloud_firewall resource, I noticed that ALL RULES are silently deleted by Terraform hcloud provider when Terraform wants to update the firewall in-place in cases when any argument (other than rule) changes.

Consider this example:

resource "hcloud_firewall" "test" {
  name = "firewall_1"
  apply_to {
    label_selector = "test/label-1"
  }
  rule {
    description = "allow all traffic from particular IPs"
    direction   = "in"
    protocol    = "tcp"
    port        = "1-65535"
    source_ips  = [
      "1.2.3.4",
      "1.3.4.5",
    ]
  }
}

Terraform creates the resource, and it appears Just Fine (tm) in the Hetzner Cloud Console:

Terraform will perform the following actions:

  # module.this.hcloud_firewall.test will be created
  + resource "hcloud_firewall" "test" {
      + id     = (known after apply)
      + labels = (known after apply)
      + name   = "firewall_1"

      + apply_to {
          + label_selector = "test/label-1"
          + server         = (known after apply)
        }

      + rule {
          + description     = "allow all traffic from particular IPs"
          + destination_ips = []
          + direction       = "in"
          + port            = "1-65535"
          + protocol        = "tcp"
          + source_ips      = [
              + "1.2.3.4/32",
              + "1.3.4.5/32",
            ]
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.
Screenshot 2024-05-22 at 09 06 41

Then, if I update only name argument, Terraform plan shows something that appears perfectly normal and expected:

Terraform will perform the following actions:

  # module.this.hcloud_firewall.test will be updated in-place
  ~ resource "hcloud_firewall" "test" {
        id     = "1404982"
      ~ name   = "firewall_1" -> "firewall_test"
        # (1 unchanged attribute hidden)

        # (2 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Similarly, here's a perfectly normal-looking plan output when changing apply_to.label_selector argument:

Terraform will perform the following actions:

  # module.this.hcloud_firewall.test will be updated in-place
  ~ resource "hcloud_firewall" "test" {
        id     = "1404986"
        name   = "firewall_1"
        # (1 unchanged attribute hidden)

      - apply_to {
          - label_selector = "test/label-1" -> null
          - server         = 0 -> null
        }
      + apply_to {
          + label_selector = "test/label-test"
          + server         = (known after apply)
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Meanwhile, looking at the Cloud Console, the end result is actually a removal of ALL RULES:

Screenshot 2024-05-22 at 09 07 12

The following re-run of terraform plan will first refresh all resources, it will pick up the missing rules, and then it will helpfully offer to re-create them on the next apply:

Terraform will perform the following actions:

  # module.this.hcloud_firewall.test will be updated in-place
  ~ resource "hcloud_firewall" "test" {
        id     = "1404986"
        name   = "firewall_1"
        # (1 unchanged attribute hidden)

      + rule {
          + description     = "allow all traffic from particular IPs test"
          + destination_ips = []
          + direction       = "in"
          + port            = "1-65535"
          + protocol        = "tcp"
          + source_ips      = [
              + "1.2.3.4/32",
              + "1.3.4.5/32",
            ]
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

What did you expect to happen?

When changing name, labels and apply_to arguments, I expected rules specified via the rule {} block to remain set in the firewall.

Please provide a minimal working example

resource "hcloud_firewall" "test" {
  name = "firewall_1"
  apply_to {
    label_selector = "test/label-1"
  }
  rule {
    description = "allow all traffic from particular IPs"
    direction   = "in"
    protocol    = "tcp"
    port        = "1-65535"
    source_ips  = [
      "1.2.3.4",
      "1.3.4.5",
    ]
  }
}
apricote commented 1 month ago

Hey @borisceranic,

looks like this bug was introduced in #874, first released in 1.46.0. This only happens if you omit the /32 at the end of your allowed IPs. If you specify them, no rules are being removed:

resource "hcloud_firewall" "test" {
  name = "firewall_1"
  apply_to {
    label_selector = "test/label-1"
  }
  rule {
    description = "allow all traffic from particular IPs"
    direction   = "in"
    protocol    = "tcp"
    port        = "1-65535"
    source_ips  = [
      "1.2.3.4/32",
      "1.3.4.5/32",
    ]
  }
}

We will take a look at this.

apricote commented 1 month ago

This is reproducable with this e2e test:

func TestFirewallResource_Regression931(t *testing.T) {
    var f hcloud.Firewall

    res := firewall.NewRData(t, "update-keep-rules", []firewall.RDataRule{
        {
            Direction:   "in",
            Protocol:    "tcp",
            SourceIPs:   []string{"192.0.2.250"},
            Port:        "80",
            Description: "allow http in",
        },
    }, nil)

    updated := &firewall.RData{
        Name:    "update-keep-rules-changed",
        Rules:   res.Rules,
        ApplyTo: res.ApplyTo,
        Labels:  res.Labels,
    }
    updated.SetRName(res.RName())

    tmplMan := testtemplate.Manager{}
    resource.Test(t, resource.TestCase{
        PreCheck:                 teste2e.PreCheck(t),
        ProtoV6ProviderFactories: teste2e.ProtoV6ProviderFactories(),
        CheckDestroy:             testsupport.CheckResourcesDestroyed(firewall.ResourceType, firewall.ByID(t, &f)),
        Steps: []resource.TestStep{
            {
                Config: tmplMan.Render(t, "testdata/r/hcloud_firewall", res),
                Check:  testsupport.CheckResourceExists(res.TFID(), firewall.ByID(t, &f)),
            },
            {
                // Update something other than the rules
                Config: tmplMan.Render(t, "testdata/r/hcloud_firewall", updated),
            },
        },
    })
}
borisceranic commented 1 month ago

Hey @apricote ,

looks like this bug was introduced in #874, first released in 1.46.0. This only happens if you omit the /32 at the end of your allowed IPs. If you specify them, no rules are being removed:

Thanks, that's a viable workaround, I'll implement it as a stop-gap solution to prevent possible issues, until the provider is fixed.

By the way: are you saying that if I stick with the provider 1.45.0 or older (for the time being) that the issue would not happen?

apricote commented 1 month ago

By the way: are you saying that if I stick with the provider 1.45.0 or older (for the time being) that the issue would not happen?

In theory yes, but before 1.46.0 you got an error if you specified the IP directly without /32, so you would still have to add that to your configuration.

apricote commented 1 month ago

Looks like we are running into the same bug as described here: https://github.com/hetznercloud/terraform-provider-hcloud/pull/468