fix minor parsing bug, add unit test for network sorting, minor clean…

…up and docs

pkg/cluster/internal/providers/docker/network.go

@@ -76,7 +76,8 @@ func ensureNetwork(name string) error {
 	if isIPv6UnavailableError(err) {
 		// only one attempt, IPAM is automatic in ipv4 only
 		return createNetworkNoDuplicates(name, "")
-	} else if isPoolOverlapError(err) {
+	}
+	if isPoolOverlapError(err) {
 		// pool overlap suggests perhaps another process created the network
 		// check if network exists already and remove any duplicate networks
 		exists, err := checkIfNetworkExists(name)
@@ -100,7 +101,8 @@ func ensureNetwork(name string) error {
 		if err == nil {
 			// success!
 			return nil
-		} else if isPoolOverlapError(err) {
+		}
+		if isPoolOverlapError(err) {
 			// pool overlap suggests perhaps another process created the network
 			// check if network exists already and remove any duplicate networks
 			exists, err := checkIfNetworkExists(name)
@@ -111,10 +113,10 @@ func ensureNetwork(name string) error {
 				return nil
 			}
 			// otherwise we'll try again
-		} else {
-			// unknown error ...
-			return err
+			continue
 		}
+		// unknown error ...
+		return err
 	}
 	return errors.New("exhausted attempts trying to find a non-overlapping subnet")
 }
@@ -152,52 +154,78 @@ func createNetwork(name, ipv6Subnet string) error {
 }
 
 func sortedNetworksWithName(name string) ([]string, error) {
-	// list all networks by this name
-	lsOut, err := exec.Output(exec.Command(
-		"docker", "network", "ls",
-		"--filter=name=^"+regexp.QuoteMeta(name)+"$",
-		"--format={{.ID}}", // output as unambiguous IDs
-	))
+	// query which networks exist with the name
+	ids, err := networksWithName(name)
+	if err != nil {
+		return nil, err
+	}
+	// we can skip sorting if there are less than 2
+	if len(ids) < 2 {
+		return ids, nil
+	}
+	// inspect them to get more detail for sorting
+	networks, err := inspectNetworks(ids)
 	if err != nil {
 		return nil, err
 	}
+	// deterministically sort networks
+	// NOTE: THIS PART IS IMPORTANT!
+	sortNetworkInspectEntries(networks)
+	// return network IDs
+	sortedIDs := make([]string, 0, len(networks))
+	for i := range networks {
+		sortedIDs = append(sortedIDs, networks[i].ID)
+	}
+	return sortedIDs, nil
+}
+
+func sortNetworkInspectEntries(networks []networkInspectEntry) {
+	sort.Slice(networks, func(i, j int) bool {
+		// we want networks with active containers first
+		if len(networks[i].Containers) > len(networks[j].Containers) {
+			return true
+		}
+		return networks[i].ID < networks[j].ID
+	})
+}
 
-	// now inspect each
-	ids := strings.Split(strings.TrimSuffix(string(lsOut), "\n"), "\n")
-	inspectOut, err := exec.Output(exec.Command("docker", append([]string{"network", "inspect"}, ids...)...))
-	// NOTE: a network could be deleted between the ls and inspect calls, that's fine
+func inspectNetworks(networkIDs []string) ([]networkInspectEntry, error) {
+	inspectOut, err := exec.Output(exec.Command("docker", append([]string{"network", "inspect"}, networkIDs...)...))
+	// NOTE: the caller can detect if the network isn't present in the output anyhow
+	// we don't want to fail on this here.
 	if err != nil && !isOnlyErrorNoSuchNetwork(err) {
 		return nil, err
 	}
-
 	// parse
-	type networkInspectEntry struct {
-		ID string `json:"Id"`
-		// NOTE: we don't care about the contents here but we need to parse
-		// how many entries exist in the containers map
-		Containers map[string]map[string]string `json:"Containers"`
-	}
-
 	networks := []networkInspectEntry{}
 	if err := json.Unmarshal(inspectOut, &networks); err != nil {
 		return nil, errors.Wrap(err, "failed to decode networks list")
 	}
+	return networks, nil
+}
 
-	// deterministically sort networks
-	// NOTE: THIS PART IS IMPORTANT!
-	sort.Slice(networks, func(i, j int) bool {
-		if len(networks[i].Containers) < len(networks[j].Containers) {
-			return true
-		}
-		return networks[i].ID < networks[j].ID
-	})
+type networkInspectEntry struct {
+	ID string `json:"Id"`
+	// NOTE: we don't care about the contents here but we need to parse
+	// how many entries exist in the containers map
+	Containers map[string]map[string]string `json:"Containers"`
+}
 
-	// return network IDs
-	sortedIDs := make([]string, 0, len(networks))
-	for i := range networks {
-		sortedIDs = append(sortedIDs, networks[i].ID)
+// networksWithName returns a list of network IDs for networks with this name
+func networksWithName(name string) ([]string, error) {
+	lsOut, err := exec.Output(exec.Command(
+		"docker", "network", "ls",
+		"--filter=name=^"+regexp.QuoteMeta(name)+"$",
+		"--format={{.ID}}", // output as unambiguous IDs
+	))
+	if err != nil {
+		return nil, err
 	}
-	return sortedIDs, nil
+	cleaned := strings.TrimSuffix(string(lsOut), "\n")
+	if cleaned == "" { // avoid returning []string{""}
+		return nil, nil
+	}
+	return strings.Split(cleaned, "\n"), nil
 }
 
 func checkIfNetworkExists(name string) (bool, error) {

pkg/cluster/internal/providers/docker/network_test.go

@@ -19,12 +19,12 @@ package docker
 import (
 	"fmt"
 	"regexp"
-	"strings"
 	"testing"
 
 	"sigs.k8s.io/kind/pkg/errors"
 	"sigs.k8s.io/kind/pkg/exec"
 
+	"sigs.k8s.io/kind/pkg/internal/assert"
 	"sigs.k8s.io/kind/pkg/internal/integration"
 )
 
@@ -35,7 +35,10 @@ func TestIntegrationEnsureNetworkConcurrent(t *testing.T) {
 
 	// cleanup
 	cleanup := func() {
-		_ = exec.Command("docker", "network", "rm", testNetworkName).Run()
+		ids, _ := networksWithName(testNetworkName)
+		if len(ids) > 0 {
+			_ = deleteNetworks(ids...)
+		}
 	}
 	cleanup()
 	defer cleanup()
@@ -56,7 +59,6 @@ func TestIntegrationEnsureNetworkConcurrent(t *testing.T) {
 			rerr := exec.RunErrorForError(err)
 			if rerr != nil {
 				t.Errorf("%q", rerr.Output)
-				t.Errorf("%v", strings.HasPrefix(string(rerr.Output), "Error response from daemon: network with name") && strings.HasSuffix(string(rerr.Output), "already exists\n"))
 			}
 			t.Errorf("%+v", errors.StackTrace(err))
 		}
@@ -122,3 +124,97 @@ func Test_generateULASubnetFromName(t *testing.T) {
 		})
 	}
 }
+
+func Test_sortNetworkInspectEntries(t *testing.T) {
+	cases := []struct {
+		Name     string
+		Networks []networkInspectEntry
+		Sorted   []networkInspectEntry
+	}{
+		{
+			Name: "simple ID sort",
+			Networks: []networkInspectEntry{
+				{
+					ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e",
+				},
+				{
+					ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae",
+				},
+			},
+			Sorted: []networkInspectEntry{
+				{
+					ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae",
+				},
+				{
+					ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e",
+				},
+			},
+		},
+		{
+			Name: "containers attached sort",
+			Networks: []networkInspectEntry{
+				{
+					ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae",
+				},
+				{
+					ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e",
+					Containers: map[string]map[string]string{
+						"a37779e06f3b694eba491dd450aad18bbbaa0a0fce2952e7c9195ea45ae79d41": {
+							"Name":       "buildx_buildkit_kind-builder0",
+							"EndpointID": "8f6411fb4360059b2f91028f91ef03130abc96d6381afc265ce53c9df89d5a3d",
+						},
+					},
+				},
+				{
+					ID: "f0445f08b9989921da00250d778975202267fbab364e5fbad0ceb6db24f3f91e",
+				},
+				{
+					ID: "128154205c7d88c7bb9c255d389bc9e222b58a48cf83619976e7665a48e79918",
+					Containers: map[string]map[string]string{
+						"aad18bbbaa0a0fce2952e7c9195ea45ae79d41a37779e06f3b694eba491dd450": {
+							"Name":       "fakey-fake",
+							"EndpointID": "f03130abc96d6381afc265ce53c9df89d5a3d8f6411fb4360059b2f91028f91e",
+						},
+					},
+				},
+			},
+			Sorted: []networkInspectEntry{
+				{
+					ID: "128154205c7d88c7bb9c255d389bc9e222b58a48cf83619976e7665a48e79918",
+					Containers: map[string]map[string]string{
+						"aad18bbbaa0a0fce2952e7c9195ea45ae79d41a37779e06f3b694eba491dd450": {
+							"Name":       "fakey-fake",
+							"EndpointID": "f03130abc96d6381afc265ce53c9df89d5a3d8f6411fb4360059b2f91028f91e",
+						},
+					},
+				},
+				{
+					ID: "dc7f897c237215c3b73d2c9ba1d4e116d872793a6c1c0e5bf083762998de8b4e",
+					Containers: map[string]map[string]string{
+						"a37779e06f3b694eba491dd450aad18bbbaa0a0fce2952e7c9195ea45ae79d41": {
+							"Name":       "buildx_buildkit_kind-builder0",
+							"EndpointID": "8f6411fb4360059b2f91028f91ef03130abc96d6381afc265ce53c9df89d5a3d",
+						},
+					},
+				},
+				{
+					ID: "1ed9912325a0d08594ee786de91ebd961e631643877b5ee58ec906b640813eae",
+				},
+				{
+					ID: "f0445f08b9989921da00250d778975202267fbab364e5fbad0ceb6db24f3f91e",
+				},
+			},
+		},
+	}
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.Name, func(t *testing.T) {
+			toSort := make([]networkInspectEntry, len(tc.Networks))
+			for i := range tc.Networks {
+				toSort[i] = tc.Networks[i]
+			}
+			sortNetworkInspectEntries(toSort)
+			assert.DeepEqual(t, tc.Sorted, toSort)
+		})
+	}
+}