Code review comment for lp:~gmb/maas/restrict-dynamic-range-to-slash-16-bug-1382190

Revision history for this message
Julian Edwards (julian-edwards) wrote :

On Thursday 30 Oct 2014 07:46:26 you wrote:
> On 30 October 2014 02:35, Julian Edwards <email address hidden>
wrote:
> >> + if ip_range.size > 256 ** 2:
> >> + raise ValidationError(ERROR_MESSAGE_DYNAMIC_RANGE_TOO_LARGE)
> >> +
> >
> > This check assumes that the IP values will be on exact boundaries. You
> > could straddle more than a /16 and it won't notice, even though the IP
> > range is under 65536 addresses.
> How? I don't see the assumption. For example:
>
> In [12]: netaddr.IPRange('10.0.0.55', '10.1.0.55').size > 256 ** 2
> Out[12]: True
>
> Am I missing something?

Yes :)

>
> > I am not familiar with your GENERATE branch so I don't know if this is
> > going to break it or not, but I'm raising the point for your
> > consideration.
> Well, if you tried to put an oversize network through it, you'll get
> an AssertionError. Actually, I think that should change so that
> creating the GENERATEs becomes a no-op in that case (otherwise, how do
> we handle networks > /16 that people already have — say if they've
> been testing with the 1.7 betas), and I'll take care of that as a
> drive-by in this branch.

<gmb> bigjools: So, you found a nice, interesting edge case… The GENERATE
stuff expects everythign to be in the same /16, so we need to enforce that
(fixing it is horrid, and if users want a dynamic range to span two /16s then
fuck ‘em, frankly). It doesn’t blow up… it but for 10.0.0.55 - 10.1.0.54, say,
you’ll get GENERATEs for 10.0.0.0 - 10.0.255.55. Which is surprising.

« Back to merge proposal