Fix infinite loop in e2e ip generator when cidr exhausted

Author: snarkychef

cmd/eks-a-tool/cmd/uniqueip.go

@@ -48,5 +48,5 @@ func preRunUniqueIp(cmd *cobra.Command, args []string) {
 func generateUniqueIP(ctx context.Context) (string, error) {
 	cidr := viper.GetString("cidr")
 	ipgen := networkutils.NewIPGenerator(&networkutils.DefaultNetClient{})
-	return ipgen.GenerateUniqueIP(cidr)
+	return ipgen.GenerateUniqueIP(cidr, nil)
 }

internal/test/e2e/ipmanager.go

@@ -20,29 +20,28 @@ func newE2EIPManager(logger logr.Logger, networkCidr string) *E2EIPManager {
 	}
 }
 
-func (ipman *E2EIPManager) reserveIP() string {
+func (ipman *E2EIPManager) reserveIP() (string, error) {
 	return ipman.getUniqueIP(ipman.networkCidr, ipman.networkIPs)
 }
 
-func (ipman *E2EIPManager) reserveIPPool(count int) networkutils.IPPool {
+func (ipman *E2EIPManager) reserveIPPool(count int) (networkutils.IPPool, error) {
 	pool := networkutils.NewIPPool()
 	for i := 0; i < count; i++ {
-		pool.AddIP(ipman.reserveIP())
+		ip, err := ipman.reserveIP()
+		if err != nil {
+			return networkutils.IPPool{}, err
+		}
+		pool.AddIP(ip)
 	}
-	return pool
+	return pool, nil
 }
 
-func (ipman *E2EIPManager) getUniqueIP(cidr string, usedIPs map[string]bool) string {
+func (ipman *E2EIPManager) getUniqueIP(cidr string, usedIPs map[string]bool) (string, error) {
 	ipgen := networkutils.NewIPGenerator(&networkutils.DefaultNetClient{})
-	ip, err := ipgen.GenerateUniqueIP(cidr)
-	for ; err != nil || usedIPs[ip]; ip, err = ipgen.GenerateUniqueIP(cidr) {
-		if err != nil {
-			ipman.logger.V(2).Info("Warning: getting unique IP failed", "error", err)
-		}
-		if usedIPs[ip] {
-			ipman.logger.V(2).Info("Warning: generated IP is already taken", "IP", ip)
-		}
+	ip, err := ipgen.GenerateUniqueIP(cidr, usedIPs)
+	if err != nil {
+		return "", err
 	}
 	usedIPs[ip] = true
-	return ip
+	return ip, nil
 }

internal/test/e2e/run.go

@@ -26,7 +26,7 @@ const (
 	testResultError      = "error"
 	nonAirgappedHardware = "nonAirgappedHardware"
 	airgappedHardware    = "AirgappedHardware"
-	maxIPPoolSize        = 10
+	maxIPPoolSize        = 7
 	minIPPoolSize        = 1
 	tinkerbellIPPoolSize = 2
 
@@ -388,24 +388,33 @@ func splitTests(testsList []string, conf ParallelRunConf) ([]instanceRunConf, er
 		if vsphereTestsRe.MatchString(testName) {
 			if privateNetworkTestsRe.MatchString(testName) {
 				if multiClusterTest {
-					ips = vspherePrivateIPMan.reserveIPPool(maxIPPoolSize)
+					ips, err = vspherePrivateIPMan.reserveIPPool(maxIPPoolSize)
 				} else {
-					ips = vspherePrivateIPMan.reserveIPPool(minIPPoolSize)
+					ips, err = vspherePrivateIPMan.reserveIPPool(minIPPoolSize)
 				}
 			} else {
 				if multiClusterTest {
-					ips = vsphereIPMan.reserveIPPool(maxIPPoolSize)
+					ips, err = vsphereIPMan.reserveIPPool(maxIPPoolSize)
 				} else {
-					ips = vsphereIPMan.reserveIPPool(minIPPoolSize)
+					ips, err = vsphereIPMan.reserveIPPool(minIPPoolSize)
 				}
 			}
+			if err != nil {
+				return nil, fmt.Errorf("failed to reserve IP pool for test %s: %v", testName, err)
+			}
 		} else if nutanixTestsRe.MatchString(testName) {
-			ips = nutanixIPMan.reserveIPPool(minIPPoolSize)
+			ips, err = nutanixIPMan.reserveIPPool(minIPPoolSize)
+			if err != nil {
+				return nil, fmt.Errorf("failed to reserve IP pool for test %s: %v", testName, err)
+			}
 		} else if cloudstackTestRe.MatchString(testName) {
 			if multiClusterTest {
-				ips = cloudstackIPMan.reserveIPPool(maxIPPoolSize)
+				ips, err = cloudstackIPMan.reserveIPPool(maxIPPoolSize)
 			} else {
-				ips = cloudstackIPMan.reserveIPPool(minIPPoolSize)
+				ips, err = cloudstackIPMan.reserveIPPool(minIPPoolSize)
+			}
+			if err != nil {
+				return nil, fmt.Errorf("failed to reserve IP pool for test %s: %v", testName, err)
 			}
 		}
 
@@ -442,13 +451,19 @@ func appendNonAirgappedTinkerbellRunConfs(awsSession *session.Session, testsList
 		if start > end/2 {
 			break
 		}
-		ipPool := ipManager.reserveIPPool(tinkerbellIPPoolSize)
+		ipPool, err := ipManager.reserveIPPool(tinkerbellIPPoolSize)
+		if err != nil {
+			return nil, fmt.Errorf("failed to reserve IP pool for tinkerbell test %s: %v", nonAirgappedTinkerbellTestsWithCount[start].Name, err)
+		}
 		runConfs = append(runConfs, newInstanceRunConf(awsSession, conf, len(runConfs), nonAirgappedTinkerbellTestsWithCount[start].Name, ipPool, []*api.Hardware{}, nonAirgappedTinkerbellTestsWithCount[start].Count, false, VSphereTestRunnerType, testRunnerConfig))
 
 		// Pop from both ends to run a longer count tests and shorter count tests together
 		// to efficiently use the available hardware.
 		if end-start > start {
-			ipPool := ipManager.reserveIPPool(tinkerbellIPPoolSize)
+			ipPool, err := ipManager.reserveIPPool(tinkerbellIPPoolSize)
+			if err != nil {
+				return nil, fmt.Errorf("failed to reserve IP pool for tinkerbell test %s: %v", nonAirgappedTinkerbellTestsWithCount[end-start].Name, err)
+			}
 			runConfs = append(runConfs, newInstanceRunConf(awsSession, conf, len(runConfs), nonAirgappedTinkerbellTestsWithCount[end-start].Name, ipPool, []*api.Hardware{}, nonAirgappedTinkerbellTestsWithCount[end-start].Count, false, VSphereTestRunnerType, testRunnerConfig))
 		}
 	}
@@ -469,7 +484,10 @@ func appendAirgappedTinkerbellRunConfs(awsSession *session.Session, testsList []
 		return nil, err
 	}
 	for _, test := range airgappedTinkerbellTestsWithCount {
-		ipPool := ipManager.reserveIPPool(tinkerbellIPPoolSize)
+		ipPool, err := ipManager.reserveIPPool(tinkerbellIPPoolSize)
+		if err != nil {
+			return nil, fmt.Errorf("failed to reserve IP pool for airgapped tinkerbell test %s: %v", test.Name, err)
+		}
 		runConfs = append(runConfs, newInstanceRunConf(awsSession, conf, len(runConfs), test.Name, ipPool, []*api.Hardware{}, test.Count, true, VSphereTestRunnerType, testRunnerConfig))
 	}
 

pkg/networkutils/ipgenerator.go

@@ -19,32 +19,57 @@ func NewIPGenerator(netClient NetClient) IPGenerator {
 	}
 }
 
-func (ipgen IPGenerator) GenerateUniqueIP(cidrBlock string) (string, error) {
+func (ipgen IPGenerator) GenerateUniqueIP(cidrBlock string, usedIPs map[string]bool) (string, error) {
 	_, cidr, err := net.ParseCIDR(cidrBlock)
 	if err != nil {
 		return "", err
 	}
-	uniqueIp, err := ipgen.randIp(cidr)
-	if err != nil {
-		return "", err
-	}
-	for IsIPInUse(ipgen.netClient, uniqueIp.String()) {
-		uniqueIp, err = ipgen.randIp(cidr)
-		if err != nil {
-			return "", err
+
+	// Start from the first IP in the CIDR range
+	ip := incrementIP(copyIP(cidr.IP))
+	checked := 0
+
+	// Iterate sequentially through all IPs in the range
+	for cidr.Contains(ip) {
+		ipStr := ip.String()
+		checked++
+
+		if usedIPs != nil && usedIPs[ipStr] {
+			ip = incrementIP(ip)
+			continue
+		}
+
+		if IsIPInUse(ipgen.netClient, ipStr) {
+			ip = incrementIP(ip)
+			continue
 		}
+
+		return ipStr, nil
 	}
-	return uniqueIp.String(), nil
+
+	return "", fmt.Errorf("no available IPs in CIDR %s (checked %d IPs)", cidrBlock, checked)
 }
 
-// generates a random ip within the specified cidr block.
-func (ipgen IPGenerator) randIp(cidr *net.IPNet) (net.IP, error) {
-	newIp := *new(net.IP)
-	for i := 0; i < 4; i++ {
-		newIp = append(newIp, byte(ipgen.rand.Intn(255))&^cidr.Mask[i]|cidr.IP[i])
-	}
-	if !cidr.Contains(newIp) {
-		return nil, fmt.Errorf("random IP generation failed")
+// incrementIP returns a new IP address incremented by 1.
+func incrementIP(ip net.IP) net.IP {
+	// Make a copy to avoid modifying the original
+	newIP := copyIP(ip)
+
+	// Increment from the last byte
+	for i := len(newIP) - 1; i >= 0; i-- {
+		newIP[i]++
+		if newIP[i] != 0 {
+			// No overflow, we're done
+			break
+		}
+		// Overflow occurred, continue to next byte
 	}
-	return newIp, nil
+	return newIP
+}
+
+// copyIP creates a copy of the IP address.
+func copyIP(ip net.IP) net.IP {
+	dup := make(net.IP, len(ip))
+	copy(dup, ip)
+	return dup
 }

pkg/networkutils/ipgenerator_test.go

@@ -3,6 +3,8 @@ package networkutils_test
 import (
 	"errors"
 	"net"
+	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -19,12 +21,148 @@ func (n *DummyNetClient) DialTimeout(network, address string, timeout time.Durat
 	return nil, errors.New("")
 }
 
+// MockNetClientAllInUse simulates all IPs being in use.
+type MockNetClientAllInUse struct{}
+
+func (n *MockNetClientAllInUse) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) {
+	// Simulate all IPs are in use by returning ECONNREFUSED
+	// IsIPInUse checks for: err == nil OR errors.Is(err, syscall.ECONNREFUSED) OR errors.Is(err, syscall.ECONNRESET)
+	return nil, &net.OpError{
+		Op:  "dial",
+		Net: "tcp",
+		Err: syscall.ECONNREFUSED,
+	}
+}
+
+// MockNetClientSomeInUse simulates specific IPs being in use.
+type MockNetClientSomeInUse struct {
+	inUseIPs map[string]bool
+}
+
+func (n *MockNetClientSomeInUse) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) {
+	// Extract IP from address (format is "IP:port")
+	ip := strings.Split(address, ":")[0]
+	if n.inUseIPs[ip] {
+		// Simulate IP in use - return ECONNREFUSED
+		return nil, &net.OpError{
+			Op:  "dial",
+			Net: "tcp",
+			Err: syscall.ECONNREFUSED,
+		}
+	}
+	// Simulate IP not in use - return generic error
+	return nil, errors.New("connection timeout")
+}
+
 func TestGenerateUniqueIP(t *testing.T) {
 	cidrBlock := "1.2.3.4/16"
 
 	ipgen := networkutils.NewIPGenerator(&DummyNetClient{})
-	ip, err := ipgen.GenerateUniqueIP(cidrBlock)
+	ip, err := ipgen.GenerateUniqueIP(cidrBlock, nil)
 	if err != nil {
 		t.Fatalf("GenerateUniqueIP() ip = %v error: %v", ip, err)
 	}
 }
+
+func TestGenerateUniqueIPWithUsedIPsMap(t *testing.T) {
+	cidrBlock := "192.168.1.0/29" // Small range: .0 to .7 (6 usable IPs)
+
+	// Mark first 3 IPs as used
+	usedIPs := map[string]bool{
+		"192.168.1.1": true,
+		"192.168.1.2": true,
+		"192.168.1.3": true,
+	}
+
+	ipgen := networkutils.NewIPGenerator(&DummyNetClient{})
+	ip, err := ipgen.GenerateUniqueIP(cidrBlock, usedIPs)
+	if err != nil {
+		t.Fatalf("GenerateUniqueIP() error = %v", err)
+	}
+
+	// Should skip the used IPs and return .4 or later
+	if ip == "192.168.1.1" || ip == "192.168.1.2" || ip == "192.168.1.3" {
+		t.Errorf("GenerateUniqueIP() returned used IP: %v", ip)
+	}
+}
+
+func TestGenerateUniqueIPExhaustion(t *testing.T) {
+	cidrBlock := "10.0.0.0/30" // Very small range: only .0 to .3 (2 usable IPs)
+
+	// Use a client that marks all IPs as in use
+	ipgen := networkutils.NewIPGenerator(&MockNetClientAllInUse{})
+	_, err := ipgen.GenerateUniqueIP(cidrBlock, nil)
+	if err == nil {
+		t.Fatal("GenerateUniqueIP() expected error for exhausted IP pool, got nil")
+	}
+
+	// Verify error message mentions the CIDR
+	if !strings.Contains(err.Error(), cidrBlock) {
+		t.Errorf("Error message should mention CIDR %s, got: %v", cidrBlock, err)
+	}
+}
+
+func TestGenerateUniqueIPWithNetworkInUse(t *testing.T) {
+	cidrBlock := "10.1.1.0/29" // Small range for testing
+
+	// Mark first 2 IPs as in use on network
+	mockClient := &MockNetClientSomeInUse{
+		inUseIPs: map[string]bool{
+			"10.1.1.1": true,
+			"10.1.1.2": true,
+		},
+	}
+
+	ipgen := networkutils.NewIPGenerator(mockClient)
+	ip, err := ipgen.GenerateUniqueIP(cidrBlock, nil)
+	if err != nil {
+		t.Fatalf("GenerateUniqueIP() error = %v", err)
+	}
+
+	// Should skip the in-use IPs
+	if ip == "10.1.1.1" || ip == "10.1.1.2" {
+		t.Errorf("GenerateUniqueIP() returned in-use IP: %v", ip)
+	}
+}
+
+func TestGenerateUniqueIPCombinedUsedAndNetwork(t *testing.T) {
+	cidrBlock := "172.16.0.0/29"
+
+	// Mark some IPs as used in map
+	usedIPs := map[string]bool{
+		"172.16.0.1": true,
+		"172.16.0.2": true,
+	}
+
+	// Mark some IPs as in use on network
+	mockClient := &MockNetClientSomeInUse{
+		inUseIPs: map[string]bool{
+			"172.16.0.3": true,
+			"172.16.0.4": true,
+		},
+	}
+
+	ipgen := networkutils.NewIPGenerator(mockClient)
+	ip, err := ipgen.GenerateUniqueIP(cidrBlock, usedIPs)
+	if err != nil {
+		t.Fatalf("GenerateUniqueIP() error = %v", err)
+	}
+
+	// Should skip all used and in-use IPs, return .5 or later
+	usedOrInUse := []string{"172.16.0.1", "172.16.0.2", "172.16.0.3", "172.16.0.4"}
+	for _, badIP := range usedOrInUse {
+		if ip == badIP {
+			t.Errorf("GenerateUniqueIP() returned used/in-use IP: %v", ip)
+		}
+	}
+}
+
+func TestGenerateUniqueIPInvalidCIDR(t *testing.T) {
+	cidrBlock := "invalid-cidr"
+
+	ipgen := networkutils.NewIPGenerator(&DummyNetClient{})
+	_, err := ipgen.GenerateUniqueIP(cidrBlock, nil)
+	if err == nil {
+		t.Fatal("GenerateUniqueIP() expected error for invalid CIDR, got nil")
+	}
+}

test/framework/network.go

@@ -31,7 +31,7 @@ func PopIPFromEnv(ipPoolEnvVar string) (string, error) {
 
 func GenerateUniqueIp(cidr string) (string, error) {
 	ipgen := networkutils.NewIPGenerator(&networkutils.DefaultNetClient{})
-	ip, err := ipgen.GenerateUniqueIP(cidr)
+	ip, err := ipgen.GenerateUniqueIP(cidr, nil)
 	if err != nil {
 		return "", fmt.Errorf("getting unique IP for cidr %s: %v", cidr, err)
 	}