On 16.10.2015 12:13, Martin Babinsky wrote:
> On 10/14/2015 04:05 PM, Petr Spacek wrote:
>> On 14.10.2015 14:13, Martin Babinsky wrote:
>>> On 10/13/2015 04:00 PM, Petr Spacek wrote:
>>>> On 13.10.2015 13:37, Martin Babinsky wrote:
>>>>> On 10/13/2015 09:36 AM, Petr Spacek wrote:
>>>>>> On 12.10.2015 16:35, Martin Babinsky wrote:
>>>>>>> https://fedorahosted.org/freeipa/ticket/5200
>>>>>>> ---
>>>>>>>     ipalib/plugins/dns.py | 3 ++-
>>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
>>>>>>> index
>>>>>>> 84086f4c77d02922f237937d58031cc42d55410e..c36345faecfb9db7abced1c6bd72ddcf93473a74
>>>>>>>
>>>>>>>
>>>>>>> 100644
>>>>>>> --- a/ipalib/plugins/dns.py
>>>>>>> +++ b/ipalib/plugins/dns.py
>>>>>>> @@ -537,7 +537,8 @@ def get_reverse_zone(ipaddr, prefixlen=None):
>>>>>>>         if prefixlen is None:
>>>>>>>             revzone = None
>>>>>>>
>>>>>>> -        result = api.Command['dnszone_find']()['result']
>>>>>>> +        result = api.Command['dnszone_find'](sizelimit=0)['result']
>>>>>>> +
>>>>>>
>>>>>> NACK, this just increases the limit because LDAP server will enforce a
>>>>>> per-user limit.
>>>>>>
>>>>>>>             for zone in result:
>>>>>>>                 zonename = zone['idnsname'][0]
>>>>>>>                 if (revdns.is_subdomain(zonename.make_absolute()) and
>>>>>>
>>>>>> Generic solution should use dns.resolver.zone_for_name() to find DNS zone
>>>>>> matching the reverse name. This should also implicitly cover CNAME/DNAME
>>>>>> redirections per RFC2317.
>>>>>>
>>>>>> Using DNS implicitly means that a zone will always be found (at least the
>>>>>> root
>>>>>> zone :-). For this reason I would change final error message
>>>>>>> reason=_('DNS reverse zone for IP address %(addr)s not found')
>>>>>> to something like:
>>>>>>      reason=_('DNS reverse zone %(zone)s for IP address %(addr)s is not
>>>>>> managed
>>>>>> by this server')
>>>>>>
>>>>>>
>>>>>> These changes should fix it without adding other artificial limitation.
>>>>>>
>>>>>
>>>>> Thank you for clarification Petr.
>>>>>
>>>>> Attaching the reworked patch.
>>>>>
>>>>> -- 
>>>>> Martin^3 Babinsky
>>>>>
>>>>> freeipa-mbabinsk-0083.1-perform-more-robust-search-for-reverse-zones-when-ad.patch
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>   From 463903f4ea4f74efeb02dbb705ca0a54fab81366 Mon Sep 17 00:00:00 2001
>>>>> From: Martin Babinsky <mbabi...@redhat.com>
>>>>> Date: Mon, 12 Oct 2015 16:24:50 +0200
>>>>> Subject: [PATCH 1/3] perform more robust search for reverse zones when
>>>>> adding
>>>>>    DNS record
>>>>>
>>>>> Instead of searching for all zones to identify the correct reverse zone, 
>>>>> we
>>>>> will first ask the resolver to return the name of zone that should
>>>>> contain the
>>>>> desired record and then see if IPA manages this zone.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/5200
>>>>> ---
>>>>>    ipalib/plugins/dns.py | 21 +++++++++++++++++----
>>>>>    1 file changed, 17 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
>>>>> index
>>>>> 84086f4c77d02922f237937d58031cc42d55410e..e3c6ce46168f8b6af607a61af1e3e431cfee32c8
>>>>>
>>>>> 100644
>>>>> --- a/ipalib/plugins/dns.py
>>>>> +++ b/ipalib/plugins/dns.py
>>>>> @@ -531,25 +531,35 @@ def add_forward_record(zone, name, str_address):
>>>>>            pass # the entry already exists and matches
>>>>>
>>>>>    def get_reverse_zone(ipaddr, prefixlen=None):
>>>>> +    """
>>>>> +    resolve the reverse zone for IP address and see if it is managed by 
>>>>> IPA
>>>>> +    server
>>>>> +    :param ipaddr: host IP address
>>>>> +    :param prefixlen: network prefix length
>>>>> +    :return: tuple containing name of the reverse zone and the name of 
>>>>> the
>>>>> +    record
>>>>> +    """
>>>>>        ip = netaddr.IPAddress(str(ipaddr))
>>>>>        revdns = DNSName(unicode(ip.reverse_dns))
>>>>> +    resolved_zone = unicode(dns.resolver.zone_for_name(revdns))
>>>>>
>>>>>        if prefixlen is None:
>>>>>            revzone = None
>>>>> +        result = api.Command['dnszone_find'](resolved_zone)['result']
>>>>>
>>>>> -        result = api.Command['dnszone_find']()['result']
>>>>>            for zone in result:
>>>>>                zonename = zone['idnsname'][0]
>>>>>                if (revdns.is_subdomain(zonename.make_absolute()) and
>>>>>                   (revzone is None or zonename.is_subdomain(revzone))):
>>>>>                    revzone = zonename
>>>>
>>>> Oh, wait, this might blow up if there is a disabled DNS zone which is 
>>>> deeper
>>>> than the one returned from DNS query.
>>>>
>>>> Damn, we have opened a Pandora box!
>>>>
>>>> ipaserver/install/bindinstance.py has own implementation of
>>>> get_reverse_zone()
>>>> + additional menthods find_reverse_zone().
>>>> These are using get_reverse_zone_default() from ipalib/util.py which is
>>>> duplicating code in 'if prefixlen is not None' branch.
>>>>
>>>> And of course, are not correct in light of this change.
>>>>
>>>> My expectation would be that disabled zones should be ignored... So we 
>>>> should
>>>> get rid of this mess in the loop and use dnszone_show() which is called at
>>>> the
>>>> end. And fix the other places, preferably by deleting duplicate
>>>> implementations.
>>>>
>>>> I would expect that you can store (converted) output of
>>>> dns.resolver.zone_for_name(revdns) into revzone and delete whole 'if
>>>> prefixlen
>>>> is None' branch.
>>>>
>>>>>        else:
>>>>> +        # if prefixlen is specified, determine the zone name for the
>>>>> network
>>>>> +        # prefix
>>>>>            if ip.version == 4:
>>>>>                pos = 4 - prefixlen / 8
>>>>>            elif ip.version == 6:
>>>>>                pos = 32 - prefixlen / 4
>>>>> -        items = ip.reverse_dns.split('.')
>>>>> -        revzone = DNSName(items[pos:])
>>>>> +        revzone = revdns[pos:]
>>>>
>>>> Hmm, and this logic will breaks CNAME/DNAME (RFC2317) handling ... Damn, we
>>>> need different handling when we intend to create the zone and when we want 
>>>> to
>>>> manipulate a record in a reverse zone. Grrr.
>>>>
>>>> When we want to manipulate a record in existing zone, the whole branch does
>>>> not make sense and the result of DNS query is what we want and need.
>>>>
>>>> On the other hand, we need this when creating *a new zone* from IP address
>>>> ...
>>>>
>>>> Mastin^2, what about zone names with missing period at the end? Is it 
>>>> handled
>>>> by dnszone_show() internally?
>>>>
>>>> We have to discuss all this...
>>>>
>>>>>            try:
>>>>>                api.Command['dnszone_show'](revzone)
>>>>> @@ -558,7 +568,10 @@ def get_reverse_zone(ipaddr, prefixlen=None):
>>>>>
>>>>>        if revzone is None:
>>>>>            raise errors.NotFound(
>>>>> -            reason=_('DNS reverse zone for IP address %(addr)s not 
>>>>> found')
>>>>> % dict(addr=ipaddr)
>>>>> +            reason=_(
>>>>> +                'DNS reverse zone %(resolved_zone)s for IP address '
>>>>> +                '%(addr)s is not manager by this server') % dict(
>>>> typo s/manager/managed/
>>>>
>>>>> +                addr=ipaddr, resolved_zone=resolved_zone)
>>>>>            )
>>>>>
>>>>>        revname = revdns.relativize(revzone)
>>>>
>>>
>>> After lengthy discussion with Petr^2 I we decided to rework get_reverse_zone
>>> function a bit. Attaching updated patch.
>>
>> Well, we should get rid of prefixlen parameter. Wipe it from Earth's
>> surface! :-)
>>
>> The assumption here is that this function is always used only for 
>> manipulating
>> *records in existing zones*, so the only way to find appropriate reverse zone
>> name is to ask DNS.
>>
>> Derivation based on prefix length makes sense only for deriving reverse zone
>> names from IP addresses *with intent to create the zone*.
>>
>> In all other cases it does not make sense to use prefix. I'm sorry that I was
>> not clear.
>>
> Upon Thy request I purged the Function that Gets the Name of the Zone Reverse
> of the crawling evil thee caleth the Prefixlen.
> 
> The shallow impostor of The Function whose services no Module required nor
> used was also banished to oblivion and His noisome vapors shall bother the
> Kingdom of Identity no more.
> 
> Thou shalt rejoice when Thy review the rewritten Patch attached herein or else
> I shall be smitten by the mighty NACK thee wieldeth.

Mighty ACK! :-D

Thank you for your patience with me.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to