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

[Jan Cholasta]

On 11.5.2011 15:58, Martin Kosek wrote:
> 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
>

Rewrote host.py so that it doesn't use get_reverse_zone from 
ipaserver.bindinstance (which fixes the pylint errors).

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-3.6-reverse-zone.patch
Type: text/x-patch
Size: 11721 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110512/c9b36e79/attachment.bin>