ipam/host-local: support sets of disjoint ranges #50
Conversation
| } | ||
| return &i, nil | ||
| return &iter, nil | ||
| } | ||
|
|
||
| // Next returns the next IP in the iterator, or nil if end is reached |
There was a problem hiding this comment.
Update the comment now that the return args have changed?
| if i.cur.Equal(r.RangeEnd) { | ||
| i.rangeIdx += 1 | ||
| i.rangeIdx %= len(*i.pool) | ||
| r = (*i.pool)[i.rangeIdx] |
There was a problem hiding this comment.
What guarantees that we don't index out-of-range here? eg that when we have exhausted all IPs in all ranges in the pool, that this line doesn't crash? Shouldn't we check len(*i.pool) or something?
There was a problem hiding this comment.
Pool exhaustion and end-of-range are not the same, since we can start in the "middle" of the range set. This code wont out-of-range since we wrap it back around with a modulo.
During Canonicalize, we reject empty pools.
| } | ||
|
|
||
| // RangeFor finds the range that contains an IP, or nil if not found | ||
| func (p *Pool) RangeFor(addr net.IP) *Range { |
There was a problem hiding this comment.
Should we canonicalize the IP here too? If so, could just implement Contains() on top of RangeFor()
There was a problem hiding this comment.
Yeah, that would be a lot neater.
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(p.Contains(net.IP{192, 168, 0, 55})).To(BeNil()) | ||
| }) |
There was a problem hiding this comment.
Maybe some overlap tests and pool-family-consistency checks?
squeed
force-pushed
the
host-local-multi
branch
from
20cfa2e
to
180c24d
Compare
|
@dcbw updated, PTAL. Thanks! |
| "rangeEnd": "3ffe:ffff:0:01ff::0020" | ||
| } | ||
| [ | ||
| { |
There was a problem hiding this comment.
Could we add a comment here like "Only one address from this range is returned; the first pool with a free address is the pool from which an IP gets reserved" or something like that to explain a bit more the interaction between ranges and pools? Same for the second pool, maybe add a comment that this pool also will return one address.
I don't know, just some way of making it clearer from the example, in addition to having it spelled out in the spec.
| iter.rangeIdx = i | ||
| iter.startRange = i | ||
|
|
||
| // We advance the cursor on ever Next(), so the first call |
| } | ||
| if len(n.IPAM.Ranges[i].RangeStart) == 4 { | ||
|
|
||
| if len(n.IPAM.Ranges[i][0].RangeStart) == 4 { |
There was a problem hiding this comment.
Maybe we should change this to To4()... I know we canonicalize the start so it'll be len=4 for v4, but I don't think we use len() anywhere else for v4/v6 checks (though we do use it later for canonicalization and range checking, but only to compare lengths to each other).
In real-world address allocations, disjoint address ranges are common. Therefore, the host-local allocator should support them. This change still allows for multiple IPs in a single configuration, but also allows for a "set of subnets." Fixes: containernetworking#45
squeed
force-pushed
the
host-local-multi
branch
from
180c24d
to
27d027a
Compare
|
@dcbw Updated. The |
…istency-openshift-4.9-ose-containernetworking-plugins-alt Updating ose-containernetworking-plugins-alt images to be consistent with ART
In real-world address allocations, disjoint address ranges are common. Therefore, the host-local allocator should support them.
This change still allows for multiple IPs in a single configuration, but also allows for a "set of subnets."
I'm not entirely happy with the configuration format - currently an array of arrays. I could be convinced to write a custom JSON deserializer that accepts either an array or a single object.
Fixes: #45