IPAM: handle subnets bigger than "/64" #49223
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
93754fa to
cc0af4d
Compare
thaJeztah
reviewed
Jan 9, 2025
70ee756 to
8924145
Compare
corhere
reviewed
Jan 14, 2025
| // Add adds address addr to the set. If addr is already in the set, it returns a | ||
| // wrapped [ErrAllocated]. If addr is not in the set's address range, it returns | ||
| // an error. | ||
| func (as AddrSet) Add(addr netip.Addr) error { |
There was a problem hiding this comment.
Although technically we can get away with value receivers while mutating the map, Go consensus is to use a pointer receiver on methods which mutate the receiver for clarity of API.
There was a problem hiding this comment.
Changed to pointers.
| // no addresses are available, it returns a wrapped [ErrNotAvailable]. | ||
| // | ||
| // When serial=true, the set is scanned starting from the address following | ||
| // the address most recently set by [AddrSet.SetAny] or [AddrSet.SetAnyInRange]. |
There was a problem hiding this comment.
Suggested change
| // the address most recently set by [AddrSet.SetAny] or [AddrSet.SetAnyInRange]. | |
| // the address most recently set by [AddrSet.AddAny] or [AddrSet.AddAnyInRange]. |
Comment on lines
76
to
79
| // If the pool has maxBitsPerBitmap or fewer, the key for the only bitmap is the pool's address. | ||
| // If there's more than one bitmap, they all have maxBitsPerBitmap. So, assume there's a free | ||
| // address in the bitmap keyed on the pool's address (no need to search other bitmaps, or work | ||
| // out if more bitmaps could be created). |
There was a problem hiding this comment.
Clarify that this assumption is unsound, with rationale
There was a problem hiding this comment.
The "unsound" assumption would be that anything in libnetwork is capable of tracking, or needs to track, 2^63 different IP addresses (9,223,372,036,854,775,808 addresses). Let alone more than that.
But, for the overly ambitious or concerned, I've reworded and included it in the godoc comment.
| } | ||
|
|
||
| // AddAny adds an arbitrary address to the set, and returns that address. Or, if | ||
| // no addresses are available, it returns a wrapped [ErrNotAvailable]. |
There was a problem hiding this comment.
Document the limitations of this function. An ErrNotAvailable returned from this function does not necessarily mean that all host IDs in the prefix have been allocated.
| // If ipr is not fully contained within the set's range, it returns an error. | ||
| // | ||
| // When serial=true, the set is scanned starting from the address following | ||
| // the address most recently set by [AddrSet.SetAny] or [AddrSet.SetAnyInRange]. |
There was a problem hiding this comment.
Suggested change
| // the address most recently set by [AddrSet.SetAny] or [AddrSet.SetAnyInRange]. | |
| // the address most recently set by [AddrSet.AddAny] or [AddrSet.AddAnyInRange]. |
daa5440 to
9a2d5df
Compare
|
I've added an extra commit, just to clarify the code that reserves network/broadcast addresses. |
akerouanton
approved these changes
Jan 20, 2025
| // Add an address that's already present. Expect an error. | ||
| err = as.Add(netip.MustParseAddr("10.20.255.255")) | ||
| assert.Check(t, is.ErrorIs(err, ErrAllocated)) | ||
| assert.Check(t, is.ErrorIs(err, ErrAllocated)) |
There was a problem hiding this comment.
Suggested change
| assert.Check(t, is.ErrorIs(err, ErrAllocated)) |
There was a problem hiding this comment.
Oops, thank you - fixed.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
The default IPAM allocator is unable to represent subnets larger than 64-bits (subnets with a smaller prefix), because it uses a Bitmap that's limited to 64-bits. When it's used to represent a 64-bit subnet, the top address can't be allocated (because bitmap.Bitmap is initialised with the number of bits it needs to represent in a uint64, so it's one short). The rest of the daemon doesn't know about these limitations, so strange things happen when a large IPv6 subnet is used. No errors are reported, addresses/subnets are just set up incorrectly. The IPAM code calculates offsets into the bitmap itself, details it shouldn't need to understand and, because it's working on offsets into a range it doesn't always notice when it's asked to set a bit outside the range. It's unusual to need a big subnet but, for example, it may be useful for modelling an ISP network, or an ISP's gateway may be in a "/56" subnet that's outside a 64-bit range used by hosts. So, use addrset.AddrSet instead of bitmap.Bitmap. Signed-off-by: Rob Murray <rob.murray@docker.com>
The first address in an IPv6 range was reserved, but that wasn't clear from comments or the code. Signed-off-by: Rob Murray <rob.murray@docker.com>
9a2d5df to
366f2b5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
docker run --ip6does not work properly when IPv6 prefix is shorter (larger) than /64 #21199The default IPAM allocator is unable to represent subnets larger than 64-bits (subnets with a smaller prefix), because it uses a Bitmap that's limited to 64-bits.
When it's used to represent a 64-bit subnet, the top address can't be allocated (because bitmap.Bitmap is initialised with the number of bits it needs to represent in a uint64, so it's one short).
The rest of the daemon doesn't know about these limitations, so strange things happen when a large IPv6 subnet is used. No errors are reported, addresses/subnets are just set up incorrectly. The IPAM code calculates offsets into the bitmap itself, details it shouldn't need to understand and, because it's working on offsets into a range it doesn't always notice when it's asked to set a bit outside the range (see the notes about "offset" in #48319).
It's unusual to need a big subnet but, for example, it may be useful for modelling an ISP network, or an ISP's gateway may be in a "/56" subnet that's outside a 64-bit range used by hosts.
- How I did it
bitmap.Bitmapfor each range of up to 63-bits that has a bit set, within the subnet it's asked to represent.- How to verify it
New unit tests for
AddrSet, plus existing tests (including the two "XFAIL" tests that now pass).- Description for the changelog
- IPAM now handles subnets bigger than "/64".