Merge pull request #4657 from thaJeztah/improve_ValidateIPAddress

opts: ValidateIPAddress: improve error, godoc, and tests

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")

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.

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))
+			}
+		})
 	}
 }