Fixed issue with GenerateNonOverlappingIPv4Subnet not correctly randomizing enough and add better random distribution across multiple private ranges - #95
Conversation
…mizing enough and add better random distribution across multiple private ranges
|
Caution Review failedThe pull request is closed. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
network/ipv4.go (1)
72-74: Fix inconsistent prefix validation: /8 is allowed but never producible
GenerateNonOverlappingIPv4Subnetaccepts 8..30, butgenerateRandomSubnetrejectsprefixLen <= parentOnes, making /8 impossible in any private range. Either disallow 8, or allow equal-to-parent ingenerateRandomSubnet. Easiest: restrict to 9..30 and skip parent ranges that can’t host the requested prefix.Apply:
- if prefixLen < 8 || prefixLen > 30 { + if prefixLen < 9 || prefixLen > 30 { return nil, nil, fmt.Errorf("invalid prefix length %d: must be between 8 and 30", prefixLen) }Also applies to: 35-38
🧹 Nitpick comments (5)
network/ipv4.go (2)
50-66: Avoid fmt+ParseCIDR; build IPNet directly to reduce allocsMinor perf/readability tweak: construct the IP/mask without string formatting and re-parsing.
Apply:
- // Convert back to 4 bytes - randBytes := []byte{ - byte(newIP >> 24), - byte(newIP >> 16), - byte(newIP >> 8), - byte(newIP), - } - - newSubnet := fmt.Sprintf("%s/%d", net.IP(randBytes).String(), prefixLen) - _, result, _ := net.ParseCIDR(newSubnet) - return result + // Convert back to 4 bytes and build IPNet directly + ipBytes := net.IP{ + byte(newIP >> 24), + byte(newIP >> 16), + byte(newIP >> 8), + byte(newIP), + } + mask := net.CIDRMask(prefixLen, 32) + return &net.IPNet{IP: ipBytes.Mask(mask), Mask: mask}
76-82: Optional: inject RNG for testability and distribution controlIf you ever need deterministic-but-configurable distribution, accept a
*rand.Randor use a package-level var that tests can seed. Keeps prod behavior while enabling controlled tests.network/ipv4_test.go (3)
211-250: Reduce flake risk in distribution assertionsRequiring each range count > 50 out of 300 is statistically fine but still a hard threshold. Consider a softer bound (e.g., > 30) or asserting non-zero plus max-min spread < some delta to avoid rare CI flakes if RNG behavior changes.
388-426: Nit: avoid repeated CIDR parsing inside the loopParsing the same CIDRs each iteration is avoidable. Move them out of the loop.
Apply:
- // Now generate more - should use other ranges - for i := 0; i < 100; i++ { - subnet, gateway, err := GenerateNonOverlappingIPv4Subnet(existingNetworks, 24) + // Pre-parse ranges once + _, range192, _ := net.ParseCIDR("192.168.0.0/16") + _, range172, _ := net.ParseCIDR("172.16.0.0/12") + _, range10, _ := net.ParseCIDR("10.0.0.0/8") + + // Now generate more - should use other ranges + for i := 0; i < 100; i++ { + subnet, gateway, err := GenerateNonOverlappingIPv4Subnet(existingNetworks, 24) assert.NoError(t, err, "Should be able to generate subnet %d after exhausting 192.168.0.0/16", i+1) assert.NotNil(t, subnet) assert.NotNil(t, gateway) - // Should not be in 192.168.0.0/16 - _, range192, _ := net.ParseCIDR("192.168.0.0/16") + // Should not be in 192.168.0.0/16 assert.False(t, range192.Contains(subnet.IP), "Subnet %s should not be in exhausted 192.168.0.0/16 range", subnet.String()) - // Should be in 172.16.0.0/12 or 10.0.0.0/8 - _, range172, _ := net.ParseCIDR("172.16.0.0/12") - _, range10, _ := net.ParseCIDR("10.0.0.0/8") + // Should be in 172.16.0.0/12 or 10.0.0.0/8 inOtherRange := range172.Contains(subnet.IP) || range10.Contains(subnet.IP) assert.True(t, inOtherRange, "Subnet %s should be in 172.16.0.0/12 or 10.0.0.0/8", subnet.String()) existingNetworks = append(existingNetworks, subnet) }
342-386: Add a test for invalid prefixes aligning with production validationOnce you update prod validation to 9..30, add a quick negative test for 8 and 31 to lock behavior.
I can add a small table-driven test if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
network/ipv4.go(2 hunks)network/ipv4_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
network/ipv4_test.go (1)
network/ipv4.go (1)
GenerateNonOverlappingIPv4Subnet(70-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
network/ipv4.go (1)
83-103: 'for range ' is OK (go 1.25); add guard to skip parent ranges where prefixLen <= parent maskgo.mod declares "go 1.25.0" — the Go 1.22+ pattern is supported. Still avoid 1000 futile attempts by skipping parent ranges:
for _, rng := range ranges { + ones, _ := rng.network.Mask.Size() + if prefixLen <= ones { + continue + } for range 1000 { // Try 1000 times to find non-overlapping subnetLikely an incorrect or invalid review comment.
network/ipv4_test.go (1)
120-147: Tests look solid; good coverage and scaling checksSubnets are accumulated to enforce non-overlap, uniqueness is asserted, and long runs are gated. Looks good.
Also applies to: 149-177, 178-210, 297-341, 342-386
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
network/ipv4.go (2)
44-50: Consider overflow protection for subnet calculations.While unlikely with typical prefix lengths, the bit shift operations could theoretically overflow for edge cases.
Add bounds checking:
// Calculate how many subnets of the requested size can fit in the parent subnetBits := prefixLen - parentOnes + if subnetBits > 32 { + return nil + } maxSubnets := uint32(1) << subnetBits
85-105: LGTM with a minor suggestion on gateway IP construction.The logic correctly generates non-overlapping subnets and properly constructs the gateway IP. The retry mechanism with 1000 attempts per range provides good coverage.
One minor optimization - you could avoid the copy allocation by directly constructing the gateway IP:
- network := candidate.IP.To4() - ip := make(net.IP, 4) - copy(ip, network) - ip[3] = 0x1 // make the gateway the first ip and copy since we have a shared copy we need to change + ip := make(net.IP, 4) + copy(ip, candidate.IP.To4()) + ip[3] = 0x1 // make the gateway the first ipnetwork/ipv4_bench_test.go (1)
57-80: Consider the computational intensity of this benchmark.Generating 1000 subnets per benchmark iteration (b.N times) could be extremely time-consuming and may not provide meaningful performance metrics due to the high variance.
Consider reducing the inner loop count or restructuring the benchmark:
func BenchmarkHighVolumeGeneration(b *testing.B) { // Save and restore original RNG originalRNG := rng defer func() { rng = originalRNG }() // Use a seeded RNG for consistent benchmarking rng = rand.New(rand.NewSource(42)) + // Pre-generate base set outside of benchmark timing + var baseNetworks []*net.IPNet + for j := 0; j < 900; j++ { + subnet, _, _ := GenerateNonOverlappingIPv4Subnet(baseNetworks, 24) + baseNetworks = append(baseNetworks, subnet) + } + b.ResetTimer() for i := 0; i < b.N; i++ { - var existingNetworks []*net.IPNet - - // Generate 1000 subnets in each benchmark iteration - for j := 0; j < 1000; j++ { - subnet, _, err := GenerateNonOverlappingIPv4Subnet(existingNetworks, 24) + // Copy base networks for isolation + existingNetworks := make([]*net.IPNet, len(baseNetworks)) + copy(existingNetworks, baseNetworks) + + // Benchmark generating the next 100 subnets + for j := 0; j < 100; j++ { + subnet, _, err := GenerateNonOverlappingIPv4Subnet(existingNetworks, 24) if err != nil { b.Fatal(err) } existingNetworks = append(existingNetworks, subnet) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(3 hunks)go.mod(1 hunks)network/ipv4.go(2 hunks)network/ipv4_bench_test.go(1 hunks)network/ipv4_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
network/ipv4_bench_test.go (1)
network/ipv4.go (1)
GenerateNonOverlappingIPv4Subnet(72-107)
network/ipv4_test.go (1)
network/ipv4.go (1)
GenerateNonOverlappingIPv4Subnet(72-107)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (22)
Makefile (3)
1-1: LGTM!The addition of the
benchtarget to the PHONY declaration is correct and follows best practices for Makefile organization.
25-25: LGTM!The integration of the bench target into the test workflow makes sense for continuous performance monitoring during the test cycle.
37-40: LGTM!The bench target is properly implemented with clear output formatting and runs all benchmarks in the project.
network/ipv4.go (4)
39-43: LGTM!The validation logic correctly ensures that the requested prefix length is larger than the parent network's prefix to allow proper subnet creation.
54-69: LGTM!The subnet calculation and IP construction logic is correct and efficient. The direct construction of the IPNet avoids unnecessary operations.
74-76: LGTM!The tightened validation range from 8-30 to 9-30 is appropriate, as a /8 prefix would be impossible to generate within any of the private ranges (the largest being /8 itself).
78-84: Good randomization implementation!The shuffling of private ranges before attempting generation provides better distribution across the available IP ranges, addressing the core issue mentioned in the PR objectives.
network/ipv4_bench_test.go (2)
10-26: LGTM!The benchmark properly saves and restores the original RNG to ensure test isolation and uses a seeded RNG for deterministic results.
28-55: LGTM!Good benchmark for testing performance with existing subnets. The pre-population of 100 subnets provides a realistic scenario for collision detection performance.
network/ipv4_test.go (13)
121-148: LGTM!Comprehensive test for generating 100 non-overlapping subnets with proper uniqueness validation and overlap checking.
150-177: LGTM!Good progressive test with 500 subnets and helpful progress logging for debugging long-running tests.
179-210: LGTM!Excellent use of
testing.Short()to skip this computationally intensive test in CI/quick test runs.
212-273: Well-designed distribution test!This test effectively validates that the randomization changes achieve their goal of better distribution across private ranges. The lenient thresholds (>30 subnets per range) appropriately account for randomness while still ensuring reasonable distribution.
275-319: LGTM!Good test to verify that range selection is properly randomized. The threshold of at least 2 ranges being selected first in 30 runs is reasonable to avoid flaky tests while still validating randomization.
321-358: Comprehensive prefix length validation!Excellent coverage of edge cases and boundary conditions for prefix length validation.
360-385: LGTM!Good focused test on the specific 9-30 boundary validation with clear error message verification.
387-440: Excellent compatibility matrix test!This test effectively validates which prefix lengths can fit in which private ranges, ensuring the subnet generation logic correctly handles range limitations.
442-473: LGTM!Good test for verifying deterministic behavior when using a seeded RNG, which is crucial for reproducible testing and debugging.
475-503: LGTM!Thorough validation of the optimized subnet construction logic, ensuring all generated subnets have correct structure and proper gateway configuration.
505-548: LGTM!Good integration test with pre-existing networks across all three private ranges.
550-594: LGTM!Well-structured capacity tests with progressive sizes to validate the algorithm can handle various scales of subnet generation.
596-635: Excellent exhaustion test!This test effectively validates that the algorithm properly falls back to other ranges when one range is exhausted. The pre-parsing of CIDR ranges is a good optimization for the repeated checks in the loop.
1 participant
Summary by CodeRabbit
New Improvements
Bug Fixes
Tests
Chores