introduce String method on PortMapping

[ndeloof]

This introduce PortMapping.String() for (some) symmetry with ParsePortSpec

[thaJeztah]

We should use net.JoinHostPort() here, to account for IPv6 addresses; https://pkg.go.dev/net#JoinHostPort

e.g.;

Suggested change

	return fmt.Sprintf("%s:%s:%s", p.Binding.HostIP, p.Binding.HostPort, p.Port)
	return net.JoinHostPort(p.Binding.HostIP, p.Binding.HostPort+":"+p.Port.Port())

But I need to have a closer look, as there's many possible formats (ports mapped here can also have the protocol (e.g. 80/tcp), which may have to be taken into account here.

And of course we also have PortMap (which is a list of mapped ports) 😞

[thaJeztah]

Oh, I guess HostPort doesn't have the scheme, so maybe that part is not an issue

[thaJeztah]

Do we know if HostIP defaults to 0.0.0.0 (or :: IPv6)? 🤔

[thaJeztah]

I rebased, and pushed some changes;

• updated the test to use a test-table to test various permutations
• swapped the fmt.Sprintf to string-concat the values (ISTR some of these may be run in a loop, so probably doesn't hurt to slightly optimize)

diff --git a/nat/nat.go b/nat/nat.go
index 5b53382..6c80825 100644
--- a/nat/nat.go
+++ b/nat/nat.go
@@ -151,7 +151,7 @@ type PortMapping struct {
 }
 
 func (p *PortMapping) String() string {
-	return net.JoinHostPort(p.Binding.HostIP, fmt.Sprintf("%s:%s", p.Binding.HostPort, p.Port))
+	return net.JoinHostPort(p.Binding.HostIP, p.Binding.HostPort+":"+string(p.Port))
 }
 
 func splitParts(rawport string) (string, string, string) {
diff --git a/nat/nat_test.go b/nat/nat_test.go
index 3497643..571bf9e 100644
--- a/nat/nat_test.go
+++ b/nat/nat_test.go
@@ -699,14 +699,50 @@ func TestParseNetworkOptsSctp(t *testing.T) {
 }
 
 func TestStringer(t *testing.T) {
-	spec := "192.168.1.100:8080:6000/udp"
-	mappings, err := ParsePortSpec(spec)
-	if err != nil {
-		t.Fatal(err)
+	tests := []struct {
+		doc      string
+		in       string
+		expected string
+	}{
+		{
+			doc:      "no host mapping",
+			in:       ":8080:6000/tcp",
+			expected: ":8080:6000/tcp",
+		},
+		{
+			doc:      "no proto",
+			in:       "192.168.1.100:8080:6000",
+			expected: "192.168.1.100:8080:6000/tcp",
+		},
+		{
+			doc:      "ipv4 mapping",
+			in:       "192.168.1.100:8080:6000/udp",
+			expected: "192.168.1.100:8080:6000/udp",
+		},
+		{
+			doc:      "ipv6 mapping",
+			in:       "[::1]:8080:6000/udp",
+			expected: "[::1]:8080:6000/udp",
+		},
+		{
+			doc:      "ipv6 legacy mapping",
+			in:       "::1:8080:6000/udp",
+			expected: "[::1]:8080:6000/udp",
+		},
 	}
-	for _, m := range mappings {
-		if m.String() != spec {
-			t.Fail()
-		}
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			mappings, err := ParsePortSpec(tc.in)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(mappings) != 1 {
+				// All tests produce a single mapping
+				t.Fatalf("Expected 1 got %d", len(mappings))
+			}
+			if actual := mappings[0].String(); actual != tc.expected {
+				t.Errorf("Expected %s got %s", tc.expected, actual)
+			}
+		})
 	}
 }
-- 
2.44.0

[robmry]

Maybe worth adding an in with a missing port, just to check it doesn't come out as a zero or something.

For maximum evil, perhaps ::::80 -> [::]::80/tcp?

[thaJeztah]

OK; added some evil permutations 👍