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 <[email protected]> >>>>> 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
