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

[Petr Viktorin]

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.

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.

-- 
Petr³