Remove dependency on apparentlymart/go-cidr#661
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redbeam The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
redbeam
force-pushed
the
dependency-remove
branch
from
14f8b7f to
cd10577
Compare
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
Nice to see this deps removes with about the same amount of code:
pkg/services/dhcp/dhcp.go | 2 +-
pkg/tap/ip_pool.go | 37 +++++++++++++++----------------------
pkg/tap/ip_pool_test.go | 4 ++--
pkg/virtualnetwork/virtualnetwork.go | 15 ++++++++++++---
4 files changed, 30 insertions(+), 28 deletions(-)
All of this with native go APIs, no need to use getNextUsableIPFromSubnet, good!
| } | ||
| return nil, errors.New("cannot find available IP") | ||
|
|
||
| return netip.Addr{}, errors.New("cannot find available IP") |
There was a problem hiding this comment.
Just to be sure, if we’ve allocated all IPs from the pool, p.base.Contains(candidate) will be false, and we will return netip.Addr{}, errors.New("cannot find available IP")?
|
|
||
| func TestIPPool(t *testing.T) { | ||
| _, network, _ := net.ParseCIDR("10.0.0.0/8") | ||
| network := netip.MustParsePrefix("10.0.0.0/8") |
There was a problem hiding this comment.
Might be a good opportunity to ask claude/your favorite agent to add more extensive test coverage for this API?
There was a problem hiding this comment.
Sure, I asked Claude to do it and this is what was added:
- TestIPPoolExhaustion - Verifies that when all IPs in a small subnet (/30) are allocated, the pool correctly returns an error for the next assignment
- TestIPPoolReserve - Tests the Reserve() functionality:
- Reserving specific IPs
- Verifying reserved IPs appear in leases
- Confirming GetOrAssign() skips reserved IPs
- Testing that requesting the same MAC as a reservation returns the reserved IP - TestIPPoolMask - Validates the Mask() method returns correct CIDR bits for various subnet sizes
- TestIPPoolReleaseNonExistent - Ensures releasing a non-existent MAC doesn't panic or corrupt the pool state
- TestIPPoolSmallSubnet - Tests edge case with a /29 subnet (8 total IPs), verifying all usable IPs are assigned correctly and exhaustion is handled
- TestIPPoolReleaseAndReassign - Validates that released IPs are properly reused on subsequent assignments (tests the gap-filling behavior)
| if err != nil { | ||
| return nil, fmt.Errorf("cannot parse static lease IP %s: %w", ip, err) | ||
| } | ||
| ipPool.Reserve(addr, mac) |
There was a problem hiding this comment.
Kind of wondering if the calls to ParsePrefix and ParseIP should be done in pkg/tap/ip_pool.go, with an added error to the return values? This would make this code simpler.
Just starting a conversation, it’s fine with me if you prefer to keep the code this way.
| base *net.IPNet | ||
| count uint64 | ||
| base netip.Prefix | ||
| leases map[string]string |
There was a problem hiding this comment.
I agree, changed that.
redbeam
force-pushed
the
dependency-remove
branch
from
cd10577 to
cb2ad84
Compare
Fixes containers#651 Replace the go-cidr library with Go's standard netip package and refactor IPPool for better ergonomics and type safety. Changes: - Convert IPPool to use netip.Prefix and netip.Addr instead of net.IPNet and net.IP - Change leases map from map[string]string to map[netip.Addr]string, eliminating string conversions in hot paths - Move parsing into API: NewIPPool and Reserve now accept strings and handle parsing internally, simplifying caller code - Replace cidr.Host() iteration with netip.Addr.Next() traversal - Remove cidr.AddressCount() by iterating directly through prefix - Update callers in pkg/virtualnetwork and pkg/services/dhcp Add extensive test coverage: - Pool exhaustion with small subnets (/30, /29) - IP reservation and gap-filling behavior - Release and reassignment logic - Invalid input validation (malformed subnets and IPs) - Edge cases like non-existent MAC release Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Matus Skvarla <mskvarla@redhat.com>
redbeam
force-pushed
the
dependency-remove
branch
from
cb2ad84 to
32dbf3f
Compare
2 participants
Fixes #651
Replace the go-cidr library with Go's standard netip package. The dependency was only used in pkg/tap/ip_pool.go for counting IPs and getting the nth host address in a subnet.
Changes:
Assisted-by: Claude Sonnet 4.5 noreply@anthropic.com