From b0ee27d6536f09c618cb8882e87d29390a8daf4b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 11 Nov 2023 13:04:29 +0100 Subject: [PATCH] opts: ValidateIPAddress: improve error, godoc, and tests - document accepted values - add test-coverage for the function's behavior (including whitespace handling), and use sub-tests. - improve error-message to use uppercase for "IP", and to use a common prefix. Signed-off-by: Sebastiaan van Stijn --- cli/command/service/update_test.go | 2 +- opts/opts.go | 12 +++-- opts/opts_test.go | 84 ++++++++++++++++++++++++------ 3 files changed, 77 insertions(+), 21 deletions(-) diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index a6e0e10679..fc30e7be56 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -199,7 +199,7 @@ func TestUpdateDNSConfig(t *testing.T) { // IPv6 flags.Set("dns-add", "2001:db8:abc8::1") // Invalid dns record - assert.ErrorContains(t, flags.Set("dns-add", "x.y.z.w"), "x.y.z.w is not an ip address") + assert.Check(t, is.ErrorContains(flags.Set("dns-add", "x.y.z.w"), "IP address is not correctly formatted: x.y.z.w")) // domains with duplicates flags.Set("dns-search-add", "example.com") diff --git a/opts/opts.go b/opts/opts.go index 4e7790f0fc..80de16052c 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -224,13 +224,17 @@ type ValidatorFctType func(val string) (string, error) // ValidatorFctListType defines a validator function that returns a validated list of string and/or an error type ValidatorFctListType func(val string) ([]string, error) -// ValidateIPAddress validates an Ip address. +// ValidateIPAddress validates if the given value is a correctly formatted +// IP address, and returns the value in normalized form. Leading and trailing +// whitespace is allowed, but it does not allow IPv6 addresses surrounded by +// square brackets ("[::1]"). +// +// Refer to [net.ParseIP] for accepted formats. func ValidateIPAddress(val string) (string, error) { - ip := net.ParseIP(strings.TrimSpace(val)) - if ip != nil { + if ip := net.ParseIP(strings.TrimSpace(val)); ip != nil { return ip.String(), nil } - return "", fmt.Errorf("%s is not an ip address", val) + return "", fmt.Errorf("IP address is not correctly formatted: %s", val) } // ValidateMACAddress validates a MAC address. diff --git a/opts/opts_test.go b/opts/opts_test.go index 1c91df5a4e..98b97003a8 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -6,27 +6,79 @@ import ( "testing" "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func TestValidateIPAddress(t *testing.T) { - if ret, err := ValidateIPAddress(`1.2.3.4`); err != nil || ret == "" { - t.Fatalf("ValidateIPAddress(`1.2.3.4`) got %s %s", ret, err) + tests := []struct { + doc string + input string + expectedOut string + expectedErr string + }{ + { + doc: "IPv4 loopback", + input: `127.0.0.1`, + expectedOut: `127.0.0.1`, + }, + { + doc: "IPv4 loopback with whitespace", + input: ` 127.0.0.1 `, + expectedOut: `127.0.0.1`, + }, + { + doc: "IPv6 loopback long form", + input: `0:0:0:0:0:0:0:1`, + expectedOut: `::1`, + }, + { + doc: "IPv6 loopback", + input: `::1`, + expectedOut: `::1`, + }, + { + doc: "IPv6 loopback with whitespace", + input: ` ::1 `, + expectedOut: `::1`, + }, + { + doc: "IPv6 lowercase", + input: `2001:db8::68`, + expectedOut: `2001:db8::68`, + }, + { + doc: "IPv6 uppercase", + input: `2001:DB8::68`, + expectedOut: `2001:db8::68`, + }, + { + doc: "IPv6 with brackets", + input: `[::1]`, + expectedErr: `IP address is not correctly formatted: [::1]`, + }, + { + doc: "IPv4 partial", + input: `127`, + expectedErr: `IP address is not correctly formatted: 127`, + }, + { + doc: "random invalid string", + input: `random invalid string`, + expectedErr: `IP address is not correctly formatted: random invalid string`, + }, } - if ret, err := ValidateIPAddress(`127.0.0.1`); err != nil || ret == "" { - t.Fatalf("ValidateIPAddress(`127.0.0.1`) got %s %s", ret, err) - } - - if ret, err := ValidateIPAddress(`::1`); err != nil || ret == "" { - t.Fatalf("ValidateIPAddress(`::1`) got %s %s", ret, err) - } - - if ret, err := ValidateIPAddress(`127`); err == nil || ret != "" { - t.Fatalf("ValidateIPAddress(`127`) got %s %s", ret, err) - } - - if ret, err := ValidateIPAddress(`random invalid string`); err == nil || ret != "" { - t.Fatalf("ValidateIPAddress(`random invalid string`) got %s %s", ret, err) + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { + actualOut, actualErr := ValidateIPAddress(tc.input) + assert.Check(t, is.Equal(tc.expectedOut, actualOut)) + if tc.expectedErr == "" { + assert.Check(t, actualErr) + } else { + assert.Check(t, is.Error(actualErr, tc.expectedErr)) + } + }) } }