Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of host-local IP address ranges #294

Merged
merged 3 commits into from
Sep 6, 2016

Conversation

dcbw
Copy link
Member

@dcbw dcbw commented Sep 2, 2016

This fixes a couple of issues with RangeEnd/RangeStart and also has a different take on #292.

dcbw added 3 commits September 2, 2016 16:37
When RangeEnd is given, a.end = RangeEnd+1.

If when getSearchRange() is called and lastReservedIP equals
RangeEnd, a.nextIP() only compares lastReservedIP (which in this
example is RangeEnd) against a.end (which in this example is
RangeEnd+1) and they clearly don't match, so a.nextIP() returns
start=RangeEnd+1 and end=RangeEnd.

Get() happily allocates RangeEnd+1 because it only compares 'cur'
to the end returned by getSearchRange(), not to a.end, and thus
allocates past RangeEnd.

Since a.end is inclusive (eg, host-local will allocate a.end) the
fix is to simply set a.end equal to RangeEnd.
And also make sure that RangeStart and RangeEnd are sane.
…works

There aren't any IPs to allocate in /32 or /31 networks, so don't allow them.
@dcbw
Copy link
Member Author

dcbw commented Sep 2, 2016

@freehan @containernetworking/cni-maintainers

@freehan
Copy link
Contributor

freehan commented Sep 2, 2016

LGTM. Thanks!

@jellonek
Copy link
Member

jellonek commented Sep 3, 2016

LGTM

@rosenhouse
Copy link
Contributor

This looks great, thanks for filling in so much test coverage. Merging...

@rosenhouse rosenhouse merged commit 07a8a28 into containernetworking:master Sep 6, 2016
@tomdee tomdee mentioned this pull request Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants