Conversation
When defaultipam.newPoolData is asked for a pool of 64-bits or more, it ends up with an overflowed u64 - so, it just subtracts one to get a nearly-big-enough range (for a 64-bit subnet). When defaultipam.getAddress is called with an ipr (sub-pool range), the range it calls bitmask.SetAnyInRange with is exclusive of end. So, its end param can't be MaxUint64, because that's the max value for the top end of the range and, when checking the range, SetAnyInRange fails. When fixed-cidr-v6 behaves more like fixed-cidr, it will ask for a 64-bit range if that's what fixed-cidr-v6 needs. So, it hits the bug when allocating an address for, for example: docker network create --ipv6 --subnet fddd::/64 --ip-range fddd::/64 b46 The additional check for "ipr == base" avoids the issue in this case, by ignoring the ipr/sub-pool range if ipr is the same as the pool itself (not really a sub-pool). But, it still fails when ipr!=base. For example: docker network create --ipv6 --subnet fddd::/56 --ip-range fddd::/64 b46 So, also subtract one from 'end' if it's going to hit the max value allowed by the Bitmap. Signed-off-by: Rob Murray <rob.murray@docker.com>
b86622d to
496b457
Compare
| ctx := setupTest(t) | ||
| c := testEnv.APIClient() | ||
|
|
||
| type kv struct{ k, v string } |
There was a problem hiding this comment.
Why not use a map[string]string instead?
There was a problem hiding this comment.
Just to make the order of the tests predictable.
| for _, sn := range subnets { | ||
| for _, ipr := range ipRanges { | ||
| for _, gw := range gateways { |
There was a problem hiding this comment.
Is there a need to capture these vars in the loop? (not sure if we'd run these with --parallel)
There was a problem hiding this comment.
I don't think so - the vars aren't used inside the Run().
| ctx := testutil.StartSpan(ctx, t) | ||
| const netName = "test64br" | ||
| network.CreateNoError(ctx, t, c, netName, network.WithIPv6(), ipamSetter) | ||
| defer network.RemoveNoError(ctx, t, c, netName) |
There was a problem hiding this comment.
curious why a defer here as it's the last statement in the test, or is that mostly to guard against things possibly being added in future?
There was a problem hiding this comment.
Mostly just habit, but I did think about starting/inspecting a container ... maybe we'll want to do that at some point.
| if pool.Addr().Is6() && numAddresses == 0 { | ||
| // FIXME(robmry) - see the comment in getAddress | ||
| numAddresses-- | ||
| } |
There was a problem hiding this comment.
OHMAN yeah, this is confusing numAddresses == 0 as the condition .. so let's decrement :confused_travolta:
- What I did
Make it possible to use a
--ip-rangethat ends on a 64-bit boundary.Without this change ...
This is a workaround, although it should solve the issue in practice. I think the remaining issues are unlikely to be a problem (but added
TestIPRangeAt64BitLimitto illustrate a limitation). A proper fix might be to make the Bitmap understand ranges >=64-bits.- How I did it
When
defaultipam.newPoolDatais asked for a range of 64-bits or more, it ends up with an overflowed u64 - so, it just subtracts one to get a nearly-big-enough range (for a 64-bit subnet).When
defaultipam.getAddressis called with anipr(sub-pool range), the range it callsbitmask.SetAnyInRangewith is exclusive of end. So, its end param can't beMaxUint64, because that's the max value for the top end of the range and, when checking the range,SetAnyInRangefails.(When
fixed-cidr-v6behaves more likefixed-cidr, it will ask for a 64-bit range if that's whatfixed-cidr-v6needs.)Work around that limitation of the Bitmap, by matching the off-by-one when allocating a bit from a range.
The additional check for
ipr == baseavoids the issue in some cases, by ignoring the ipr/sub-pool range if ipr is the same as the pool itself (not really a sub-pool). But, it still fails whenipr!=base. For example:So, also subtract one from
endif it's going to hit the max value allowed by the Bitmap.- How to verify it
Without this fix, to see the problem, try:
New tests, output ...
- Description for the changelog
n/a - workaround, replaced by https://github.com/moby/moby/pull/49223