Skip to content

Commit 45a6462

Browse files
snarkychefsnarkychef
committed
Fix infinite loop in e2e ip generator when cidr exhausted
1 parent b10b5a9 commit 45a6462

File tree

6 files changed

+89
-47
lines changed

6 files changed

+89
-47
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,5 +48,5 @@ func preRunUniqueIp(cmd *cobra.Command, args []string) {
4848
func generateUniqueIP(ctx context.Context) (string, error) {
4949
cidr := viper.GetString("cidr")
5050
ipgen := networkutils.NewIPGenerator(&networkutils.DefaultNetClient{})
51-
return ipgen.GenerateUniqueIP(cidr)
51+
return ipgen.GenerateUniqueIP(cidr, nil)
5252
}

‎internal/test/e2e/ipmanager.go‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,29 +20,28 @@ func newE2EIPManager(logger logr.Logger, networkCidr string) *E2EIPManager {
2020
}
2121
}
2222

23-
func (ipman *E2EIPManager) reserveIP() string {
23+
func (ipman *E2EIPManager) reserveIP() (string, error) {
2424
return ipman.getUniqueIP(ipman.networkCidr, ipman.networkIPs)
2525
}
2626

27-
func (ipman *E2EIPManager) reserveIPPool(count int) networkutils.IPPool {
27+
func (ipman *E2EIPManager) reserveIPPool(count int) (networkutils.IPPool, error) {
2828
pool := networkutils.NewIPPool()
2929
for i := 0; i < count; i++ {
30-
pool.AddIP(ipman.reserveIP())
30+
ip, err := ipman.reserveIP()
31+
if err != nil {
32+
return networkutils.IPPool{}, err
33+
}
34+
pool.AddIP(ip)
3135
}
32-
return pool
36+
return pool, nil
3337
}
3438

35-
func (ipman *E2EIPManager) getUniqueIP(cidr string, usedIPs map[string]bool) string {
39+
func (ipman *E2EIPManager) getUniqueIP(cidr string, usedIPs map[string]bool) (string, error) {
3640
ipgen := networkutils.NewIPGenerator(&networkutils.DefaultNetClient{})
37-
ip, err := ipgen.GenerateUniqueIP(cidr)
38-
for ; err != nil || usedIPs[ip]; ip, err = ipgen.GenerateUniqueIP(cidr) {
39-
if err != nil {
40-
ipman.logger.V(2).Info("Warning: getting unique IP failed", "error", err)
41-
}
42-
if usedIPs[ip] {
43-
ipman.logger.V(2).Info("Warning: generated IP is already taken", "IP", ip)
44-
}
41+
ip, err := ipgen.GenerateUniqueIP(cidr, usedIPs)
42+
if err != nil {
43+
return "", err
4544
}
4645
usedIPs[ip] = true
47-
return ip
46+
return ip, nil
4847
}

‎internal/test/e2e/run.go‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const (
2626
testResultError = "error"
2727
nonAirgappedHardware = "nonAirgappedHardware"
2828
airgappedHardware = "AirgappedHardware"
29-
maxIPPoolSize = 10
29+
maxIPPoolSize = 7
3030
minIPPoolSize = 1
3131
tinkerbellIPPoolSize = 2
3232

@@ -388,24 +388,33 @@ func splitTests(testsList []string, conf ParallelRunConf) ([]instanceRunConf, er
388388
if vsphereTestsRe.MatchString(testName) {
389389
if privateNetworkTestsRe.MatchString(testName) {
390390
if multiClusterTest {
391-
ips = vspherePrivateIPMan.reserveIPPool(maxIPPoolSize)
391+
ips, err = vspherePrivateIPMan.reserveIPPool(maxIPPoolSize)
392392
} else {
393-
ips = vspherePrivateIPMan.reserveIPPool(minIPPoolSize)
393+
ips, err = vspherePrivateIPMan.reserveIPPool(minIPPoolSize)
394394
}
395395
} else {
396396
if multiClusterTest {
397-
ips = vsphereIPMan.reserveIPPool(maxIPPoolSize)
397+
ips, err = vsphereIPMan.reserveIPPool(maxIPPoolSize)
398398
} else {
399-
ips = vsphereIPMan.reserveIPPool(minIPPoolSize)
399+
ips, err = vsphereIPMan.reserveIPPool(minIPPoolSize)
400400
}
401401
}
402+
if err != nil {
403+
return nil, fmt.Errorf("failed to reserve IP pool for test %s: %v", testName, err)
404+
}
402405
} else if nutanixTestsRe.MatchString(testName) {
403-
ips = nutanixIPMan.reserveIPPool(minIPPoolSize)
406+
ips, err = nutanixIPMan.reserveIPPool(minIPPoolSize)
407+
if err != nil {
408+
return nil, fmt.Errorf("failed to reserve IP pool for test %s: %v", testName, err)
409+
}
404410
} else if cloudstackTestRe.MatchString(testName) {
405411
if multiClusterTest {
406-
ips = cloudstackIPMan.reserveIPPool(maxIPPoolSize)
412+
ips, err = cloudstackIPMan.reserveIPPool(maxIPPoolSize)
407413
} else {
408-
ips = cloudstackIPMan.reserveIPPool(minIPPoolSize)
414+
ips, err = cloudstackIPMan.reserveIPPool(minIPPoolSize)
415+
}
416+
if err != nil {
417+
return nil, fmt.Errorf("failed to reserve IP pool for test %s: %v", testName, err)
409418
}
410419
}
411420

@@ -442,13 +451,19 @@ func appendNonAirgappedTinkerbellRunConfs(awsSession *session.Session, testsList
442451
if start > end/2 {
443452
break
444453
}
445-
ipPool := ipManager.reserveIPPool(tinkerbellIPPoolSize)
454+
ipPool, err := ipManager.reserveIPPool(tinkerbellIPPoolSize)
455+
if err != nil {
456+
return nil, fmt.Errorf("failed to reserve IP pool for tinkerbell test %s: %v", nonAirgappedTinkerbellTestsWithCount[start].Name, err)
457+
}
446458
runConfs = append(runConfs, newInstanceRunConf(awsSession, conf, len(runConfs), nonAirgappedTinkerbellTestsWithCount[start].Name, ipPool, []*api.Hardware{}, nonAirgappedTinkerbellTestsWithCount[start].Count, false, VSphereTestRunnerType, testRunnerConfig))
447459

448460
// Pop from both ends to run a longer count tests and shorter count tests together
449461
// to efficiently use the available hardware.
450462
if end-start > start {
451-
ipPool := ipManager.reserveIPPool(tinkerbellIPPoolSize)
463+
ipPool, err := ipManager.reserveIPPool(tinkerbellIPPoolSize)
464+
if err != nil {
465+
return nil, fmt.Errorf("failed to reserve IP pool for tinkerbell test %s: %v", nonAirgappedTinkerbellTestsWithCount[end-start].Name, err)
466+
}
452467
runConfs = append(runConfs, newInstanceRunConf(awsSession, conf, len(runConfs), nonAirgappedTinkerbellTestsWithCount[end-start].Name, ipPool, []*api.Hardware{}, nonAirgappedTinkerbellTestsWithCount[end-start].Count, false, VSphereTestRunnerType, testRunnerConfig))
453468
}
454469
}
@@ -469,7 +484,10 @@ func appendAirgappedTinkerbellRunConfs(awsSession *session.Session, testsList []
469484
return nil, err
470485
}
471486
for _, test := range airgappedTinkerbellTestsWithCount {
472-
ipPool := ipManager.reserveIPPool(tinkerbellIPPoolSize)
487+
ipPool, err := ipManager.reserveIPPool(tinkerbellIPPoolSize)
488+
if err != nil {
489+
return nil, fmt.Errorf("failed to reserve IP pool for airgapped tinkerbell test %s: %v", test.Name, err)
490+
}
473491
runConfs = append(runConfs, newInstanceRunConf(awsSession, conf, len(runConfs), test.Name, ipPool, []*api.Hardware{}, test.Count, true, VSphereTestRunnerType, testRunnerConfig))
474492
}
475493

‎pkg/networkutils/ipgenerator.go‎

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,32 +19,57 @@ func NewIPGenerator(netClient NetClient) IPGenerator {
1919
}
2020
}
2121

22-
func (ipgen IPGenerator) GenerateUniqueIP(cidrBlock string) (string, error) {
22+
func (ipgen IPGenerator) GenerateUniqueIP(cidrBlock string, usedIPs map[string]bool) (string, error) {
2323
_, cidr, err := net.ParseCIDR(cidrBlock)
2424
if err != nil {
2525
return "", err
2626
}
27-
uniqueIp, err := ipgen.randIp(cidr)
28-
if err != nil {
29-
return "", err
30-
}
31-
for IsIPInUse(ipgen.netClient, uniqueIp.String()) {
32-
uniqueIp, err = ipgen.randIp(cidr)
33-
if err != nil {
34-
return "", err
27+
28+
// Start from the first IP in the CIDR range
29+
ip := incrementIP(copyIP(cidr.IP))
30+
checked := 0
31+
32+
// Iterate sequentially through all IPs in the range
33+
for cidr.Contains(ip) {
34+
ipStr := ip.String()
35+
checked++
36+
37+
if usedIPs != nil && usedIPs[ipStr] {
38+
ip = incrementIP(ip)
39+
continue
40+
}
41+
42+
if IsIPInUse(ipgen.netClient, ipStr) {
43+
ip = incrementIP(ip)
44+
continue
3545
}
46+
47+
return ipStr, nil
3648
}
37-
return uniqueIp.String(), nil
49+
50+
return "", fmt.Errorf("no available IPs in CIDR %s (checked %d IPs)", cidrBlock, checked)
3851
}
3952

40-
// generates a random ip within the specified cidr block.
41-
func (ipgen IPGenerator) randIp(cidr *net.IPNet) (net.IP, error) {
42-
newIp := *new(net.IP)
43-
for i := 0; i < 4; i++ {
44-
newIp = append(newIp, byte(ipgen.rand.Intn(255))&^cidr.Mask[i]|cidr.IP[i])
45-
}
46-
if !cidr.Contains(newIp) {
47-
return nil, fmt.Errorf("random IP generation failed")
53+
// incrementIP returns a new IP address incremented by 1.
54+
func incrementIP(ip net.IP) net.IP {
55+
// Make a copy to avoid modifying the original
56+
newIP := copyIP(ip)
57+
58+
// Increment from the last byte
59+
for i := len(newIP) - 1; i >= 0; i-- {
60+
newIP[i]++
61+
if newIP[i] != 0 {
62+
// No overflow, we're done
63+
break
64+
}
65+
// Overflow occurred, continue to next byte
4866
}
49-
return newIp, nil
67+
return newIP
68+
}
69+
70+
// copyIP creates a copy of the IP address.
71+
func copyIP(ip net.IP) net.IP {
72+
dup := make(net.IP, len(ip))
73+
copy(dup, ip)
74+
return dup
5075
}

‎pkg/networkutils/ipgenerator_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ func TestGenerateUniqueIP(t *testing.T) {
2323
cidrBlock := "1.2.3.4/16"
2424

2525
ipgen := networkutils.NewIPGenerator(&DummyNetClient{})
26-
ip, err := ipgen.GenerateUniqueIP(cidrBlock)
26+
ip, err := ipgen.GenerateUniqueIP(cidrBlock, nil)
2727
if err != nil {
2828
t.Fatalf("GenerateUniqueIP() ip = %v error: %v", ip, err)
2929
}

‎test/framework/network.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func PopIPFromEnv(ipPoolEnvVar string) (string, error) {
3131

3232
func GenerateUniqueIp(cidr string) (string, error) {
3333
ipgen := networkutils.NewIPGenerator(&networkutils.DefaultNetClient{})
34-
ip, err := ipgen.GenerateUniqueIP(cidr)
34+
ip, err := ipgen.GenerateUniqueIP(cidr, nil)
3535
if err != nil {
3636
return "", fmt.Errorf("getting unique IP for cidr %s: %v", cidr, err)
3737
}

0 commit comments

Comments
 (0)