Repository: kubernetes/minikube
Pull request #15394 (2022-11-23 00:18:30)

Fix subnet checking
**Problem:**
While checking for a free subnet, if an IPv6 network is found `n.IP` is set to `0.0.0.0`, which is then set to `startSubnet`. This will cause an error on the next loop of the check as it add 9 to `0.0.0.0` instead of the starting IP, resulting in the second IP to check for being `0.9.0.0`.

**Expected:**
```
Checking 192.168.49.0
Checking 192.177.49.0
Checking 192.186.49.0
...
```

**Actual:**
```
Checking 192.168.49.0
Checking 0.9.0.0
Checking 0.18.0.0
...
```

**Solution:**
Assign the value returned from `n.IP` to its own variable to not interrupt the checking loop.


Patch:
diff --git a/pkg/network/network.go b/pkg/network/network.go
index bf7b1d644..05fd93864 100644
--- a/pkg/network/network.go
+++ b/pkg/network/network.go
@@ -123,7 +123,7 @@ func lookupInInterfaces(ip net.IP) (*Parameters, *net.IPNet, error) {
 // inspect initialises IPv4 network parameters struct from given address addr.
 // addr can be single address (like "192.168.17.42"), network address (like "192.168.17.0") or in CIDR form (like "192.168.17.42/24 or "192.168.17.0/24").
 // If addr belongs to network of local network interface, parameters will also contain info about that network interface.
-func inspect(addr string) (*Parameters, error) {
+var inspect = func(addr string) (*Parameters, error) {
 
 	// extract ip from addr
 	ip, network, err := net.ParseCIDR(addr)
@@ -191,7 +191,7 @@ func inspect(addr string) (*Parameters, error) {
 
 // isSubnetTaken returns if local network subnet exists and any error occurred.
 // If will return false in case of an error.
-func isSubnetTaken(subnet string) (bool, error) {
+var isSubnetTaken = func(subnet string) (bool, error) {
 	ifAddrs, err := net.InterfaceAddrs()
 	if err != nil {
 		return false, fmt.Errorf("failed listing network interface addresses: %w", err)
@@ -225,19 +225,20 @@ func IsUser(network string) bool {
 
 // FreeSubnet will try to find free private network beginning with startSubnet, incrementing it in steps up to number of tries.
 func FreeSubnet(startSubnet string, step, tries int) (*Parameters, error) {
+	currSubnet := startSubnet
 	for try := 0; try < tries; try++ {
-		n, err := inspect(startSubnet)
+		n, err := inspect(currSubnet)
 		if err != nil {
 			return nil, err
 		}
-		startSubnet = n.IP
-		if isSubnetPrivate(startSubnet) {
-			taken, err := isSubnetTaken(startSubnet)
+		subnet := n.IP
+		if isSubnetPrivate(subnet) {
+			taken, err := isSubnetTaken(subnet)
 			if err != nil {
 				return nil, err
 			}
 			if !taken {
-				if ok := reserveSubnet(startSubnet, defaultReservationPeriod); ok {
+				if ok := reserveSubnet(subnet, defaultReservationPeriod); ok {
 					klog.Infof("using free private subnet %s: %+v", n.CIDR, n)
 					return n, nil
 				}
@@ -249,13 +250,13 @@ func FreeSubnet(startSubnet string, step, tries int) (*Parameters, error) {
 			klog.Infof("skipping subnet %s that is not private", n.CIDR)
 		}
 		prefix, _ := net.ParseIP(n.IP).DefaultMask().Size()
-		nextSubnet := net.ParseIP(startSubnet).To4()
+		nextSubnet := net.ParseIP(currSubnet).To4()
 		if prefix <= 16 {
 			nextSubnet[1] += byte(step)
 		} else {
 			nextSubnet[2] += byte(step)
 		}
-		startSubnet = nextSubnet.String()
+		currSubnet = nextSubnet.String()
 	}
 	return nil, fmt.Errorf("no free private network subnets found with given parameters (start: %q, step: %d, tries: %d)", startSubnet, step, tries)
 }
@@ -265,7 +266,7 @@ func FreeSubnet(startSubnet string, step, tries int) (*Parameters, error) {
 //   - true, if new reservation was created or expired one renewed
 //
 // uses sync.Map to manage reservations thread-safe
-func reserveSubnet(subnet string, period time.Duration) bool {
+var reserveSubnet = func(subnet string, period time.Duration) bool {
 	// put 'zero' reservation{} Map value for subnet Map key
 	// to block other processes from concurrently changing this subnet
 	zero := reservation{}


Test patch:
diff --git a/pkg/network/network_test.go b/pkg/network/network_test.go
new file mode 100644
index 000000000..9bff64d09
--- /dev/null
+++ b/pkg/network/network_test.go
@@ -0,0 +1,116 @@
+/*
+Copyright 2022 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package network
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestIsSubnetPrivate(t *testing.T) {
+	tests := []struct {
+		subnet   string
+		expected bool
+	}{
+		{"9.255.255.255", false},
+		{"10.0.0.0", true},
+		{"10.255.255.255", true},
+		{"11.0.0.0", false},
+		{"172.15.255.255", false},
+		{"172.16.0.0", true},
+		{"172.31.255.255", true},
+		{"172.32.0.0", false},
+		{"192.167.255.255", false},
+		{"192.168.0.0", true},
+		{"192.168.255.255", true},
+		{"192.169.0.0", false},
+	}
+	for _, test := range tests {
+		got := isSubnetPrivate(test.subnet)
+		if got != test.expected {
+			t.Errorf("isSubnetPrivate(%q) = %t; expected = %t", test.subnet, got, test.expected)
+		}
+	}
+}
+
+func TestFreeSubnet(t *testing.T) {
+	reserveSubnet = func(subnet string, period time.Duration) bool { return true }
+
+	t.Run("NoRetriesSuccess", func(t *testing.T) {
+		startingSubnet := "192.168.0.0"
+		subnet, err := FreeSubnet(startingSubnet, 0, 1)
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectedIP := startingSubnet
+		if subnet.IP != expectedIP {
+			t.Errorf("expected IP = %q; got = %q", expectedIP, subnet.IP)
+		}
+	})
+
+	t.Run("FirstSubnetTaken", func(t *testing.T) {
+		count := 0
+		isSubnetTaken = func(subnet string) (bool, error) {
+			count++
+			return count == 1, nil
+		}
+
+		startingSubnet := "192.168.0.0"
+		subnet, err := FreeSubnet(startingSubnet, 9, 2)
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectedIP := "192.168.9.0"
+		if subnet.IP != expectedIP {
+			t.Errorf("expected IP = %q; got = %q", expectedIP, subnet.IP)
+		}
+	})
+
+	t.Run("FirstSubnetIPV6NetworkFound", func(t *testing.T) {
+		count := 0
+		inspect = func(addr string) (*Parameters, error) {
+			count++
+			p := &Parameters{IP: addr}
+			if count == 1 {
+				p.IP = "0.0.0.0"
+			}
+			return p, nil
+		}
+
+		startingSubnet := "10.0.0.0"
+		subnet, err := FreeSubnet(startingSubnet, 9, 2)
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectedIP := "10.9.0.0"
+		if subnet.IP != expectedIP {
+			t.Errorf("expepcted IP = %q; got = %q", expectedIP, subnet.IP)
+		}
+	})
+
+	t.Run("NonPrivateSubnet", func(t *testing.T) {
+		startingSubnet := "192.167.0.0"
+		_, err := FreeSubnet(startingSubnet, 9, 1)
+		if err == nil {
+			t.Fatalf("expected to fail since IP non-private but no error thrown")
+		}
+		if !strings.Contains(err.Error(), startingSubnet) {
+			t.Errorf("expected starting subnet of %q to be included in error, but intead got: %v", startingSubnet, err)
+		}
+	})
+}