compat: populate IPRange in IPAM config when listing networks#28396
compat: populate IPRange in IPAM config when listing networks#28396crawfordxx wants to merge 2 commits intocontainers:mainfrom
Conversation
When converting a libpod network to a Docker-compat network, the IPAMConfig.IPRange field was left empty (TODO). This field should reflect the lease range configured on each subnet so that Docker clients see the full network config on inspect. The conversion from IPRange CIDR to LeaseRange is already done in the createNetwork path (IPRange -> FirstIP/LastIP). This commit adds the reverse: reconstruct the CIDR prefix from the stored StartIP/EndIP for IPv4 networks where the range forms a valid aligned CIDR block. Fixes containers#28378 Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
7c965d3 to
a388d72
Compare
|
The MacOS arm64 build failure appears to be a transient Cirrus CI issue — the change only touches |
|
@crawfordxx i was able to rerun the test in question. it passed. Ive now asked for all failed jobs to retry. thanks for the PR |
|
ok, api test failures here now have been revealed. |
FirstIPInSubnet returns network+1 (the first usable host) rather than the network address itself, so the stored LeaseRange.StartIP is one greater than the network address. The previous implementation compared [StartIP, EndIP] = [network+1, broadcast] directly, treating them as a CIDR block. For a /25 (128 hosts) this yields count=127, which is not a power of two, causing the function to return false. Fix by recovering the network address (StartIP - 1), then computing the block size as EndIP - networkAddr + 1. This correctly handles all aligned IPv4 ranges: /25 yields size=128, /24 yields size=256, etc. The /32 case is handled separately since FirstIPInSubnet returns the address unchanged (no increment) for a single-host subnet. Signed-off-by: crawfordxx <crawfordxx@users.noreply.github.com>
|
Fixed. The bug was in The fix recovers the network address as |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| // FirstIPInSubnet/LastIPInSubnet. FirstIPInSubnet returns network+1 (first usable host), | ||
| // so StartIP is one greater than the network address. LastIPInSubnet returns the broadcast. | ||
| // Only succeeds for IPv4 ranges that form a valid aligned CIDR block. | ||
| func leaseRangeToIPRangePrefix(lr *nettypes.LeaseRange) (netip.Prefix, bool) { |
There was a problem hiding this comment.
I have a few thoughts on this!
First, this function could be a lot shorter and cleaner. I'm not a huge fan of using raw bitwise operations here; we can make the logic much more elegant and readable. Here is a quick pseudocode mockup of what I mean:
FUNCTION leaseRangeToIPRangePrefix(leaseRange):
// 1. Initial Validation
IF leaseRange IS NULL:
RETURN Null, False
startIP = ParseIPv4(leaseRange.StartIP)
endIP = ParseIPv4(leaseRange.EndIP)
// Ensure IPs are valid and mathematically start <= end
IF startIP IS INVALID OR endIP IS INVALID OR startIP > endIP:
RETURN Null, False
// 2. Calculate the total number of IPs in the range
startInt = ConvertTo32BitInteger(startIP)
endInt = ConvertTo32BitInteger(endIP)
totalIPs = (endInt - startInt) + 1
// 3. Verify the range size is a valid subnet size
// A valid subnet size must be a perfect power of 2 (1, 2, 4, 8, 16, etc.)
// In binary, a perfect power of 2 has exactly one "1" bit.
IF CountSetBits(totalIPs) != 1:
RETURN Null, False
// 4. Calculate the subnet mask / prefix length
// The number of trailing zeros in the total IPs equals the host bits
numberOfHostBits = CountTrailingZeros(totalIPs)
networkPrefixLength = 32 - numberOfHostBits
proposedPrefix = CreateSubnetPrefix(startIP, networkPrefixLength)
// 5. Verify network boundary alignment
// The start IP must perfectly match the base network address of the subnet
IF proposedPrefix DOES NOT EQUAL ApplyNetworkMask(proposedPrefix):
RETURN Null, False
// 6. Success
RETURN proposedPrefix, True
Second, we definitely need to add unit tests for this function.
Lastly (and this isn't a blocker), something in the back of my mind is screaming that this function actually belongs in libnetwork under container-libs. WDYT? @Luap99
Summary
When converting a libpod network to a Docker-compatible network in
convertLibpodNetworktoDockerNetwork, theIPAMConfig.IPRangefieldwas left empty with a
// TODO add rangecomment.This PR resolves the TODO by reconstructing the CIDR prefix from the
LeaseRangestored on each subnet. The forward conversion (DockerIPRangeCIDR →LeaseRange{StartIP, EndIP}) already exists in thecreateNetworkpath. This adds the reverse direction:StartIPandEndIPfromLeaseRange, check whether theyform a valid aligned IPv4 CIDR block (power-of-two size, aligned
start address).
netip.PrefixasIPAMConfig.IPRange.IPRangeempty.This means
docker network inspectnow shows theIPRangefornetworks created with
--ip-range(or the Docker compat API equivalent),restoring round-trip fidelity for the IPAM configuration.
Fixes #28378