[Freeipa-devel] [PATCH] 0027 Prompt for nameserver IP address in dnszone-add

Fri May 10 12:42:14 UTC 2013

On 05/10/2013 01:30 PM, Ana Krivokapic wrote:
> On 05/10/2013 10:58 AM, Petr Viktorin wrote:
>> On 05/10/2013 09:30 AM, Petr Spacek wrote:
>>> On 9.5.2013 19:03, Ana Krivokapic wrote:
>>>> On 05/09/2013 02:10 PM, Martin Kosek wrote:
>>>>> On 05/09/2013 12:45 PM, Petr Viktorin wrote:
>>>>>> On 05/07/2013 07:50 PM, Ana Krivokapic wrote:
>>>>>>> Prompt for nameserver IP address in dnszone-add
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/3603
>>>>>> See Petr Špaček's mail for normal zones.
>>>>>> Also when adding a reverse zone we should not ask:
>>>>>>
>>>>>> $ ipa dnszone-add --name-from-ip=80.142.15.0/24
>>>>>> --name-server=`hostname`.
>>>>>> Zone name [15.142.80.in-addr.arpa.]:
>>>>>> Administrator e-mail address [hostmaster.15.142.80.in-addr.arpa.]:
>>>>>> [Nameserver IP address]: 1.2.3.4
>>>>>> ipa: ERROR: invalid 'ip_address': Nameserver DNS record is created
>>>>>> for for
>>>>>> forward zones only
>>>>>>
>>>>>> The Web UI also asks for the NS IP address for reverse zones and
>>>>>> fails when
>>>>>> it's given.
>>>>>>
>>>>>>
>>>>>> (Also, looking at the output above, asking for the zone name isn't
>>>>>> useful for
>>>>>> reverse zones, but I guess that's a different usability issue.)
>>>>>>
>>>>>>
>>>>>> I think the easiest way to selectively ask in CLI is a custom
>>>>>> interactive_prompt_callback (or we could have a separate
>>>>>> dnszone-add-reverse
>>>>>> command). As for the UI I don't know.
>>>>>> The question is, do we want to go that far for this bug?
>>>>> I do not like the alwaysask approach. I think it rather complicates
>>>>> things.
>>>>>
>>>>> I think we will indeed need to do the interactive_prompt_callback and
>>>>> in case
>>>>> when we detect the following cases (as per Petr Spacek's mail):
>>>>>
>>>>> new zone = example.com.
>>>>> a) NS = ns.example.com. (i.e. absolute name)
>>>>> => IP address is required because NS is defined inside the new zone
>>>>>
>>>>> b) NS = ns (i.e. relative name)
>>>>> => IP address is required because NS is defined inside the new zone
>>>>>
>>>>> we need to ask for IP address. As in this case, this is really not an
>>>>> option,
>>>>> but a required parameter. If possible, the same logic would be great
>>>>> to have
>>>>> for the UI.
>>>>>
>>>>> When deciding the question above, you can take advantage of
>>>>> get_name_in_zone
>>>>> function which will also work with cases like
>>>>> ipa dnszone-add example.com --name-server=@ --ip-address 10.16.78.47
>>>>>
>>>>> Martin
>>>>
>>>> Thanks all for the reviews. I addressed all comments in the attached
>>>> updated patch.
>>
>> Thanks, it works nicely now.
>> Could you add some tests? See tests/test_cmdline/test_cli.py for some
>> examples of tests of interactive prompting.
>
> Added - updated patch attached.
>>
>> The Web UI works, but it'd be nice if a JS expert looked over the code.
>>
>>> I went quickly through the patch and the logic seems okay.
>>>
>>> As a part of the patch I would recommend to change the error messages
>>> below to something more meaningful/accurate:
>>>
>>>   >         if zone_is_reverse(normalized_zone):
>>>   >             if not nameserver.endswith('.'):
>>>   >                 raise errors.ValidationError(name='name-server',
>>>   >                         error=_("Nameserver for reverse zone
>>> cannot be "
>>>   >                                 "a relative DNS name"))
>>>   >             elif nameserver_ip_address:
>>>   >                 raise errors.ValidationError(name='ip_address',
>>>   >                         error=_("Nameserver DNS record is created
>>> for "
>>>   >                                 "for forward zones only"))
>>>
>>> "Nameserver for reverse zone cannot be relative DNS name"
>>> => "Relative names in nameserver records for reverse zone are not
>>> supported"
>>>
>>> "Nameserver DNS record is created for for forward zones only"
>>> => "A/AAAA records in reverse zone are not supported"
>>>
>>> The problem is that both cases are completely legal from DNS point of
>>> view, but IPA interface do not allow to create such zones.
>>>
>>>
>>> The other option is to drop this 'if' branch completely, but it
>>> definitely should be in a separate patch or even a ticket.
>>
>> Is this going into this release? If yes, it's pretty late for string
>> changes, both these and the "for for" typo fix in this patch, so we
>> should leave them out.
>>
>
> I moved the string changes into a separate patch.
>

You left the string change in the first patch. Otherwise, ACK for the 
Python part.

In the UI, when you ender a reverse zone manually, the Nameserver IP 
address field should not be enabled at all (see screenshot: 
http://i.imgur.com/oj2LR2w.png)

In the second patch, please also change the message in a test:

FAIL: test_dns[10]: dnszone_add: Try to create reverse zone 
u'15.142.80.in-addr.arpa.' with NS record in it
----------------------------------------------------------------------
Traceback (most recent call last):
   File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in 
runTest
     self.test(*self.arg)
   File "/home/pviktori/freeipa/tests/test_xmlrpc/xmlrpc_test.py", line 
271, in <lambda>
     func = lambda: self.check(nice, **test)
   File "/home/pviktori/freeipa/tests/test_xmlrpc/xmlrpc_test.py", line 
285, in check
     self.check_exception(nice, cmd, args, options, expected)
   File "/home/pviktori/freeipa/tests/test_xmlrpc/xmlrpc_test.py", line 
311, in check_exception
     assert_deepequal(expected.strerror, e.strerror)
   File "/home/pviktori/freeipa/tests/util.py", line 348, in 
assert_deepequal
     VALUE % (doc, expected, got, stack)
AssertionError: assert_deepequal: expected != got.

   expected = u"invalid 'name-server': Nameserver for reverse zone 
cannot be a relative DNS name"
   got = u"invalid 'name-server': Relative names in nameserver records 
for reverse zone are not supported"
   path = ()

-- 
Petr³