[Freeipa-devel] [PATCH 0058, 0064] dns: do not add (forward)zone if it is already resolvable.

[David Kupka]

On 28/08/15 13:36, Martin Basti wrote:
>
>
> On 08/28/2015 10:03 AM, Petr Spacek wrote:
>> On 27.8.2015 14:22, David Kupka wrote:
>>> @@ -2101,11 +2101,25 @@ class DNSZoneBase(LDAPObject):
>>>   class DNSZoneBase_add(LDAPCreate):
>>> +    takes_options = LDAPCreate.takes_options + (
>>> +        Flag('force',
>>> +             label=_('Force'),
>>> +             doc=_('Force DNS zone creation.')
>>> +        ),
>>> +        Flag('skip_overlap_check',
>>> +             doc=_('Force DNS zone creation even if it will overlap
>>> with '
>>> +                   'existing zone.')
>>> +        ),
>>> +    )
>>> +
>>>       has_output_params = LDAPCreate.has_output_params +
>>> dnszone_output_params
>>>       def pre_callback(self, ldap, dn, entry_attrs, attrs_list,
>>> *keys, **options):
>>>           assert isinstance(dn, DN)
>>> +        if options['force']:
>>> +            options['skip_overlap_check'] = True
>>> +
>>>           try:
>>>               entry = ldap.get_entry(dn)
>>>           except errors.NotFound:
>>> @@ -2120,6 +2134,12 @@ class DNSZoneBase_add(LDAPCreate):
>>>           entry_attrs['idnszoneactive'] = 'TRUE'
>>> +        if not options['skip_overlap_check']:
>>> +            try:
>>> +                check_zone_overlap(keys[-1])
>>> +            except RuntimeError as e:
>>> +                raise errors.InvocationError(e.message)
>>> +
>>>           return dn
>>> @@ -2673,9 +2693,9 @@ class dnszone_add(DNSZoneBase_add):
>>>       __doc__ = _('Create new DNS zone (SOA record).')
>>>       takes_options = DNSZoneBase_add.takes_options + (
>>> -        Flag('force',
>>> -             label=_('Force'),
>>> -             doc=_('Force DNS zone creation even if nameserver is
>>> not resolvable.'),
>>> +        Flag('skip_nameserver_check',
>>> +             doc=_('Force DNS zone creation even if nameserver is not '
>>> +                   'resolvable.')
>>>           ),
>>>           # Deprecated
>>> @@ -2699,6 +2719,9 @@ class dnszone_add(DNSZoneBase_add):
>>>       def pre_callback(self, ldap, dn, entry_attrs, attrs_list,
>>> *keys, **options):
>>>           assert isinstance(dn, DN)
>>> +        if options['force']:
>>> +            options['skip_nameserver_check'] = True
>> Why is it in DNSZoneBase_add.pre_callback?
>>
>> Shouldn't the equation force = (skip_nameserver_check +
>> skip_nameserver_check)
>> be handled in parameter parsing & validation? (Again, I do not know
>> the IPA
>> framework :-))
>>
>>> +
>>>           dn = super(dnszone_add, self).pre_callback(
>>>               ldap, dn, entry_attrs, attrs_list, *keys, **options)
>>> @@ -2713,7 +2736,7 @@ class dnszone_add(DNSZoneBase_add):
>>>                       error=_("Nameserver for reverse zone cannot be
>>> a relative DNS name"))
>>>               # verify if user specified server is resolvable
>>> -            if not options['force']:
>>> +            if not options['skip_nameserver_check']:
>>>                   check_ns_rec_resolvable(keys[0],
>>> entry_attrs['idnssoamname'])
>>>               # show warning about --name-server option
>>>               context.show_warning_nameserver_option = True
>>> diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
>>> index
>>> d959bb369d946217acd080e78483cc9013dda4c7..18f477d4fb6620090b7073689c8df51b65a8307a
>>> 100644
>>> --- a/ipapython/ipautil.py
>>> +++ b/ipapython/ipautil.py
>>> @@ -924,6 +924,20 @@ def host_exists(host):
>>>       else:
>>>           return True
>>> +def check_zone_overlap(zone):
>>> +    if resolver.zone_for_name(zone) == zone:
>>> +        try:
>>> +            ns = [ans.to_text() for ans in resolver.query(zone, 'NS')]
>>> +        except DNSException as e:
>>> +            root_logger.debug("Failed to resolve nameserver(s) for
>>> domain"
>>> +                " {0}: {1}".format(zone, e))
>>> +            ns = []
>>> +
>>> +        msg = u"DNS zone {0} already exists".format(zone)
>> Nitpick: I would say "already exists in DNS" to make it absolutely
>> clear. Just
>> 'already exists' might be confusing because ipa dnszone-show will say
>> that the
>> zone does not exist ...
>>
>>> +        if ns:
>>> +            msg += u" and is handled by server(s): {0}".format(',
>>> '.join(ns))
>>> +        raise RuntimeError(msg)
>>> +
>>>   def get_ipa_basedn(conn):
>>>       """
>>>       Get base DN of IPA suffix in given LDAP server.
> 0064
> NACK
>
> ipa-replica-install should have the --skip-overlap-check too, because
> any replica can be the first DNS server.

Thanks for the catch, added.

>
> 0064+0058
> Can be the options --allow-zone-overlap and --skip-overlap-check merged
> into an one name, to have just one name for same thing?
>

Each option has bit different behavior:
The '--skip-overlap-check' option in API call prevent the check to be 
performed and thus no error or warning is raised. This is the way 
'--force' option was originally working.

The '--allow-zone-overlap' options in installers do not skip the check 
but change the error to warning instead and let the installation continue.

If you think that this can confuse users we need to change the names  or 
even the logic.

Updated patches attached.

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0064.2-dns-Check-if-domain-already-exists.patch
Type: text/x-patch
Size: 4940 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150908/1401d057/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0058.4-dns-do-not-add-forward-zone-if-it-is-already-resolva.patch
Type: text/x-patch
Size: 7584 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150908/1401d057/attachment-0001.bin>