[Freeipa-devel] [PATCH] 3 Add ability to specify netmask with IP addresses during installation

[Martin Kosek]

On Tue, 2011-05-10 at 20:10 +0200, Jan Cholasta wrote:
> On 21.4.2011 09:36, Jan Cholasta wrote:
> > On 20.4.2011 22:08, Rob Crittenden wrote:
> >> Jan Cholasta wrote:
> >>> On 11.4.2011 12:48, Jan Cholasta wrote:
> >>>> On 8.4.2011 16:26, Rob Crittenden wrote:
> >>>>> Jan Cholasta wrote:
> >>>>>> On 29.3.2011 22:15, Rob Crittenden wrote:
> >>>>>>> Jan Cholasta wrote:
> >>>>>>>> Sorry, forgot to attach the patch.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Is this why you have some blind excepts?
> >>>>>>>
> >>>>>>> installutils._IPAddressWithPrefix('192.168.0.1/33')
> >>>>>>> Traceback (most recent call last):
> >>>>>>> File "<stdin>", line 1, in <module>
> >>>>>>> File "ipaserver/install/installutils.py", line 167, in __init__
> >>>>>>> net = netaddr.IPNetwork(addr)
> >>>>>>> File "/usr/lib/python2.7/site-packages/netaddr/ip/__init__.py", line
> >>>>>>> 919, in __init__
> >>>>>>> implicit_prefix, flags)
> >>>>>>> File "/usr/lib/python2.7/site-packages/netaddr/ip/__init__.py", line
> >>>>>>> 782, in parse_ip_network
> >>>>>>> value = ip._value
> >>>>>>> UnboundLocalError: local variable 'ip' referenced before assignment
> >>>>>>>
> >>>>>>> We should get an upstream bug filed on python-netaddr about this.
> >>>>>>
> >>>>>> https://github.com/drkjam/netaddr/issues/closed#issue/5
> >>>>>> https://github.com/drkjam/netaddr/issues/closed#issue/6
> >>>>>> https://github.com/drkjam/netaddr/issues/closed#issue/8
> >>>>>>
> >>>>>> Apparently it's already been fixed for the next release.
> >>>>>>
> >>>>>> IMHO it's not much of an issue for us, because the exception gets
> >>>>>> caught
> >>>>>> in parse_ip_address and that's currently the only place where
> >>>>>> _IPAddressWithPrefix is used.
> >>>>>>
> >>>>>>>
> >>>>>>> Shoudl parse_ip_address() raise an exception on bad data rather than
> >>>>>>> returning 0.0.0.0?
> >>>>>>
> >>>>>> I've been down that road and it would need a rewrite of the
> >>>>>> fragile IP
> >>>>>> address handling logic of ipa-server-install, which is something I'd
> >>>>>> rather avoid.
> >>>>>>
> >>>>>>>
> >>>>>>> >>> installutils.parse_ip_address('355.555.3.3')
> >>>>>>> _IPAddressWithPrefix('0.0.0.0')
> >>>>>>>
> >>>>>>> or
> >>>>>>>
> >>>>>>> >>> installutils.parse_ip_address('192.168.0.1/55')
> >>>>>>> _IPAddressWithPrefix('0.0.0.0')
> >>>>>>>
> >>>>>>> Should it disallow net addresses like 192.168.0.0?
> >>>>>>
> >>>>>> If you mean network and broadcast addresses, it probably should. It
> >>>>>> might be a good idea to disallow localhost, multicast and/or
> >>>>>> link-local
> >>>>>> addresses too.
> >>>>>
> >>>>> Are you going to resubmit the patch with these added or should we
> >>>>> open a
> >>>>> separate ticket?
> >>>>
> >>>> I'm going to resubmit it. Right now it disallows loopback, IANA
> >>>> reserved, link-local, network, multicast and broadcast IP addresses.
> >>>> Does it make sense to also allow only IP addresses attached to one of
> >>>> the local network interfaces? Perhaps it would be sufficient just to
> >>>> print a warning. Or should we not care about that at all?
> >>>
> >>> Sending the updated patch.
> >>
> >> This looks ok, just one question. Should we add a dependency on the
> >> iproute package because of the /sbin/ip package?
> >
> > Yes, we should.
> >
> >>
> >> rob
> >
> >
> 
> Split the patch to 3 smaller pieces:
> 
> Patch 18 adds the ability to parse netmasks in IP addresses passed to 
> server install.
> https://fedorahosted.org/freeipa/ticket/1212
> 
> This patch requires patch 18 and fixes DNS reverse zone setup to honor 
> the netmask.
> https://fedorahosted.org/freeipa/ticket/910
> 
> Patch 19 requires patch 18 and adds stricter checking of IP addresses.
> https://fedorahosted.org/freeipa/ticket/1213
> 
> Honza

Thanks for splitting of the patches, it is now much clearer what is done
and where. Please fix pylint errors first before the review, there were
several of them when I applied all 3 patches:

./make-lint 
ipalib/plugins/host.py:122: [E1120, remove_fwd_ptr] No value passed for parameter 'ip_prefix_len' in function call
ipalib/plugins/host.py:325: [E1120, host_add.pre_callback] No value passed for parameter 'ip_prefix_len' in function call
ipalib/plugins/host.py:384: [E1120, host_add.post_callback] No value passed for parameter 'ip_prefix_len' in function call

Martin