feat(firewall): set IPs without prefix directly (#874)

This commit allows users to specify allowed IPs directly, without having to awkwardly put an `/32` at the end. If an IP is passed, we only allow this single IP (`/32` for IPv4, `/128` for IPv6). This matches the behavior in Cloud Console. Closes #807 Closes #715 fix(firewall): unnecessary diff if user specified non-minimal IPv6 If the user specified an IPv6 which was not minimal (eg. `::0` instead of `::`), the API would modify this to the minimal form. In the next run terraform reports a diff and tries to update this over and over again. We now do this normalization locally to avoid showing these diffs to users. Closes #870 Co-authored-by: Ettore Foti <[email protected]>
hetznercloud · Feb 20, 2024 · 40df28d · 40df28d
1 parent 2109a3f
commit 40df28d
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 53 deletions.
diff --git a/internal/firewall/resource.go b/internal/firewall/resource.go
@@ -7,11 +7,11 @@ import (
  "log"
  "net"
  "strconv"
- "strings"
 
  "github.com/hashicorp/go-cty/cty"
  "github.com/hashicorp/terraform-plugin-sdk/v2/diag"
  "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+
  "github.com/hetznercloud/hcloud-go/hcloud"
  "github.com/hetznercloud/terraform-provider-hcloud/internal/control"
  "github.com/hetznercloud/terraform-provider-hcloud/internal/util/hcloudutil"
@@ -116,9 +116,7 @@ func Resource() *schema.Resource {
  Elem: &schema.Schema{
  Type: schema.TypeString,
  ValidateDiagFunc: validateIPDiag,
- StateFunc: func(i interface{}) string {
- return strings.ToLower(i.(string))
- },
+ StateFunc: normalizeIP,
  },
  Optional: true,
  },
@@ -127,9 +125,7 @@ func Resource() *schema.Resource {
  Elem: &schema.Schema{
  Type: schema.TypeString,
  ValidateDiagFunc: validateIPDiag,
- StateFunc: func(i interface{}) string {
- return strings.ToLower(i.(string))
- },
+ StateFunc: normalizeIP,
  },
  Optional: true,
  },

diff --git a/internal/firewall/resource_test.go b/internal/firewall/resource_test.go
@@ -179,49 +179,38 @@ func TestFirewallResource_ApplyTo(t *testing.T) {
  })
 }
 
-func TestFirewallResource_SourceIPs_IPv6Comparison(t *testing.T) {
+func TestFirewallResource_Normalization(t *testing.T) {
  var f hcloud.Firewall
 
  res := firewall.NewRData(t, "ipv6-firewall", []firewall.RDataRule{
  {
  Direction: "in",
  Protocol: "tcp",
+ // Uppercase
  SourceIPs: []string{"Aaaa:aaaa:aaaa:aaaa::/64"},
  Port: "22",
  },
- }, nil)
- tmplMan := testtemplate.Manager{}
-
- // TODO: Move to parallel test once API endpoint supports higher parallelism
- 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: resource.ComposeTestCheckFunc(
- testsupport.CheckResourceExists(res.TFID(), firewall.ByID(t, &f)),
- ),
- },
- {
- Config: tmplMan.Render(t, "testdata/r/hcloud_firewall", res),
- PlanOnly: true,
- },
- },
- })
-}
-
-func TestFirewallResource_DestinationIPs_IPv6Comparison(t *testing.T) {
- var f hcloud.Firewall
-
- res := firewall.NewRData(t, "ipv6-firewall", []firewall.RDataRule{
  {
- Direction: "out",
- Protocol: "tcp",
+ Direction: "out",
+ Protocol: "tcp",
+ // Uppercase
  DestinationIPs: []string{"Aaaa:aaaa:aaaa:aaaa::/64"},
  Port: "22",
  },
+ {
+ Direction: "in",
+ Protocol: "tcp",
+ // Avoidable 0
+ SourceIPs: []string{"aaaa:aaaa:aaaa:0::/64"},
+ Port: "80",
+ },
+ {
+ Direction: "out",
+ Protocol: "tcp",
+ // Avoidable 0
+ DestinationIPs: []string{"aaaa:aaaa:aaaa:0::/64"},
+ Port: "80",
+ },
  }, nil)
  tmplMan := testtemplate.Manager{}
 

diff --git a/internal/firewall/validation.go b/internal/firewall/validation.go
@@ -5,10 +5,19 @@ import (
 
  "github.com/hashicorp/go-cty/cty"
  "github.com/hashicorp/terraform-plugin-sdk/v2/diag"
+
  "github.com/hetznercloud/terraform-provider-hcloud/internal/util/hcloudutil"
 )
 
+var (
+ // These masks are also used in the Cloud Console when a user enters an IP without range.
+ defaultMaskIPv4 = net.CIDRMask(32, 32)
+ defaultMaskIPv6 = net.CIDRMask(128, 128)
+)
+
 func validateIPDiag(i interface{}, _ cty.Path) diag.Diagnostics {
+ i = normalizeIP(i)
+
  ipS := i.(string)
  ip, n, err := net.ParseCIDR(ipS)
  if err != nil {
@@ -19,3 +28,41 @@ func validateIPDiag(i interface{}, _ cty.Path) diag.Diagnostics {
  }
  return nil
 }
+
+// normalizeIP implements two closely related functions:
+// 1. It normalizes an IP address or CIDR block to a CIDR block. To allow users to specify the IP directly.
+// 2. The API modifies CIDRs to lower case and IPv6 to its minimal form. This function does the same to
+// have clean diffs, even if the user input does not match the desired format by the API.
+func normalizeIP(i interface{}) string {
+ input := i.(string)
+
+ ip, ipnet, err := net.ParseCIDR(input)
+ if err == nil {
+ // net.ParseCIDR removes any set host bits. We want to show an error to the user instead,
+ // to avoid making any assumptions about their intent.
+ // By setting the parse IP in the ipnet, the returned string will be the same as the input, only normalized & lower cased.
+ ipnet.IP = ip
+ } else {
+ ip = net.ParseIP(input)
+ if ip == nil {
+ // No CIDR or IP, just return the input string
+ return input
+ }
+ if ip.To4() != nil {
+ // IPv4
+
+ ipnet = &net.IPNet{
+ IP: ip,
+ Mask: defaultMaskIPv4,
+ }
+ } else {
+ // If To4 returns nil, IP is not IPv4 => IPv6
+ ipnet = &net.IPNet{
+ IP: ip,
+ Mask: defaultMaskIPv6,
+ }
+ }
+ }
+
+ return ipnet.String()
+}
diff --git a/internal/firewall/validation_test.go b/internal/firewall/validation_test.go
@@ -29,16 +29,6 @@ func TestValidateIPDiag(t *testing.T) {
  ip: "test",
  err: diag.Diagnostics{diag.Diagnostic{Severity: 0, Summary: "invalid CIDR address: test", Detail: "", AttributePath: cty.Path(nil)}},
  },
- {
- name: "Missing CIDR notation (IPv4)",
- ip: "10.0.0.0",
- err: diag.Diagnostics{diag.Diagnostic{Severity: 0, Summary: "invalid CIDR address: 10.0.0.0", Detail: "", AttributePath: cty.Path(nil)}},
- },
- {
- name: "Missing CIDR notation (IPv6)",
- ip: "fe80::",
- err: diag.Diagnostics{diag.Diagnostic{Severity: 0, Summary: "invalid CIDR address: fe80::", Detail: "", AttributePath: cty.Path(nil)}},
- },
  {
  name: "Host bit set (IPv4)",
  ip: "10.0.0.5/8",
@@ -58,8 +48,63 @@ func TestValidateIPDiag(t *testing.T) {
  }
 
  if test.err != nil {
- assert.Equal(t, err, test.err)
+ assert.Equal(t, test.err, err)
  }
  })
  }
 }
+
+func Test_normalizeIP(t *testing.T) {
+ tests := []struct {
+ name string
+ input string
+ want string
+ }{
+ {
+ name: "Valid CIDR (IPv4)",
+ input: "192.0.2.0/24",
+ want: "192.0.2.0/24",
+ },
+ {
+ name: "IP Address (IPv4)",
+ input: "192.0.2.31",
+ want: "192.0.2.31/32",
+ },
+ {
+ name: "Valid CIDR (IPv6)",
+ input: "2001:db8:123:4567::/64",
+ want: "2001:db8:123:4567::/64",
+ },
+ {
+ name: "Unreduced CIDR (IPv6)",
+ input: "2001:0db8:0123:4567::0/64",
+ want: "2001:db8:123:4567::/64",
+ },
+ {
+ name: "Uppercase CIDR (IPv6)",
+ input: "2001:DB8:123:4567::/64",
+ want: "2001:db8:123:4567::/64",
+ },
+ {
+ name: "IP Address (IPv6)",
+ input: "2001:db8:123:4567::",
+ want: "2001:db8:123:4567::/128",
+ },
+ {
+ name: "Unreduced Matching-All CIDR (IPv6)",
+ input: "::0/0",
+ want: "::/0",
+ },
+ {
+ name: "Badly formatted IP returns input",
+ input: "foobar",
+ want: "foobar",
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got := normalizeIP(tt.input)
+ assert.Equalf(t, tt.want, got, "normalizeIP(%v)", tt.input)
+ })
+ }
+}
diff --git a/website/docs/r/firewall.html.md b/website/docs/r/firewall.html.md
@@ -55,9 +55,11 @@ resource "hcloud_server" "node1" {
 - `direction` - (Required, string) Direction of the Firewall Rule. `in`
 - `protocol` - (Required, string) Protocol of the Firewall Rule. `tcp`, `icmp`, `udp`, `gre`, `esp`
 - `port` - (Required, string) Port of the Firewall Rule. Required when `protocol` is `tcp` or `udp`. You can use `any`
- to allow all ports for the specific protocol. Port ranges are also possible: `80-85` allows all ports between 80 and
- 85.
-- `source_ips` - (Required, List) List of CIDRs that are allowed within this Firewall Rule
+ to allow all ports for the specific protocol. Port ranges are also possible: `80-85` allows all ports between 80 and 85.
+- `source_ips` - (Required, List) List of IPs or CIDRs that are allowed within this Firewall Rule (when `direction`
+ is `in`)
+- `destination_ips` - (Required, List) List of IPs or CIDRs that are allowed within this Firewall Rule (when `direction`
+ is `out`)
 - `description` - (Optional, string) Description of the firewall rule
 
 `apply_to` support the following fields:
@@ -79,8 +81,9 @@ resource "hcloud_server" "node1" {
 - `direction` - (Required, string) Direction of the Firewall Rule. `in`, `out`
 - `protocol` - (Required, string) Protocol of the Firewall Rule. `tcp`, `icmp`, `udp`, `gre`, `esp`
 - `port` - (Required, string) Port of the Firewall Rule. Required when `protocol` is `tcp` or `udp`
-- `source_ips` - (Required, List) List of CIDRs that are allowed within this Firewall Rule (when `direction` is `in`)
-- `destination_ips` - (Required, List) List of CIDRs that are allowed within this Firewall Rule (when `direction`
+- `source_ips` - (Required, List) List of IPs or CIDRs that are allowed within this Firewall Rule (when `direction`
+ is `in`)
+- `destination_ips` - (Required, List) List of IPs or CIDRs that are allowed within this Firewall Rule (when `direction`
  is `out`)
 - `description` - (Optional, string) Description of the firewall rule