[Freeipa-devel] [PATCH] 0027 Prompt for nameserver IP address in dnszone-add
Ana Krivokapic
akrivoka at redhat.com
Fri May 10 13:57:29 UTC 2013
On 05/10/2013 02:42 PM, Petr Viktorin wrote:
> 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)
>
>
Fixed.
>
>
> 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 = ()
>
Fixed.
Thanks for catching the bugs. Updated patches are attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0027-04-Prompt-for-nameserver-IP-address-in-dnszone-add.patch
Type: text/x-patch
Size: 9361 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/0ecb9f98/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0028-02-Improve-error-messages-in-dnszone-add.patch
Type: text/x-patch
Size: 2719 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/0ecb9f98/attachment-0001.bin>
More information about the Freeipa-devel
mailing list