[Freeipa-devel] [PATCH] 260 Replace DNS client based on acutil with python-dns
Martin Kosek
mkosek at redhat.com
Wed May 16 07:44:33 UTC 2012
On Tue, 2012-05-15 at 14:02 +0200, Petr Viktorin wrote:
> On 05/11/2012 06:52 PM, Martin Kosek wrote:
> > python-dns is very feature-rich and it can help us a lot with our DNS
> > related code. This patch does the first step, i.e. replaces acutil use
> > with python-dns, which is more convenient to use as you will see in the
> > patch. More integration will follow in the future.
> >
> > I send this patch rather early, so that I can get responses to this
> > patch early and also so that we are able to catch issues in a safe
> > distance from the next release.
>
> > ---
> > IPA client and server tool set used authconfig acutil module to
> > for client DNS operations. This is not optimal DNS interface for
> > several reasons:
> > - does not provide native Python object oriented interface
> > but but rather C-like interface based on functions and
> > structures which is not easy to use and extend
> > - acutil is not meant to be used by third parties besides
> > authconfig and thus can break without notice
> >
> > Replace the acutil with python-dns package which has a feature rich
> > interface for dealing with all different aspects of DNS including
> > DNSSEC. The main target of this patch is to replace all uses of
> > acutil DNS library with a use python-dns. In most cases, even
> > though the larger parts of the code are changed, the actual
> > functionality is changed only in the following cases:
> > - redundant DNS checks were removed from verify_fqdn function
> > in installutils to make the whole DNS check simpler and
> > less error-prone. Logging was improves for the remaining
> > checks
> > - improved logging for ipa-client-install DNS discovery
> >
> > https://fedorahosted.org/freeipa/ticket/2730
>
> Also relevant: https://fedorahosted.org/freeipa/ticket/1837
Yup, added to commit message.
>
> There is a forgotten acutil reference in a comment in
> install/tools/ipa-dns-install:226
Fixed.
>
> I did find some style issues/suggestions. Stop me if the bikeshedding is
> too useless :)
>
> [...]
> > diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py
> > index 86bef28b2d7fdfc8111b493bddec7ac6888f021a..800b4ef9a6396884a1fe2e93bdf94e948f35c57e 100644
> > --- a/ipa-client/ipaclient/ipadiscovery.py
> > +++ b/ipa-client/ipaclient/ipadiscovery.py
> > @@ -20,10 +20,12 @@
> > import socket
> > import os
> > from ipapython.ipa_log_manager import *
> > -import ipapython.dnsclient
> > import tempfile
> > import ldap
> > from ldap import LDAPError
> > +from dns import resolver, rdatatype
> > +from dns.exception import DNSException
> > +
> > from ipapython.ipautil import run, CalledProcessError, valid_ip, get_ipa_basedn, \
> > realm_to_suffix, format_netloc, parse_items
> >
> > @@ -311,81 +313,89 @@ class IPADiscovery:
> >
> >
> > def ipadnssearchldap(self, tdomain):
> > - servers = ""
> > - rserver = ""
> > + server = ""
> >
> > - qname = "_ldap._tcp."+tdomain
> > - # terminate the name
> > - if not qname.endswith("."):
> > - qname += "."
> > - results = ipapython.dnsclient.query(qname, ipapython.dnsclient.DNS_C_IN, ipapython.dnsclient.DNS_T_SRV)
> > + qname = "_ldap._tcp." + tdomain
> >
> > - for result in results:
> > - if result.dns_type == ipapython.dnsclient.DNS_T_SRV:
> > - rserver = result.rdata.server.rstrip(".")
> > - if result.rdata.port and result.rdata.port != 389:
> > - rserver += ":" + str(result.rdata.port)
> > - if servers:
> > - servers += "," + rserver
> > - else:
> > - servers = rserver
> > - break
> > + root_logger.debug("Search DNS for SRV record of %s", qname)
> >
> > - return servers
> > + try:
> > + answers = resolver.query(qname, rdatatype.SRV)
> > + except DNSException, e:
> > + root_logger.debug("DNS record not found: %s", e.__class__.__name__)
> > + answers = []
> > +
> > + for answer in answers:
> > + root_logger.debug("DNS record found: %s", answer)
> > + server = str(answer.target).rstrip(".")
> > + if answer.port != 389:
> > + server += ":" + str(answer.port)
> > + break
> > + return server
> >
> > def ipadnssearchntp(self, tdomain):
> > - servers = ""
> > - rserver = ""
> > + server = ""
> >
> > - qname = "_ntp._udp."+tdomain
> > - # terminate the name
> > - if not qname.endswith("."):
> > - qname += "."
> > - results = ipapython.dnsclient.query(qname, ipapython.dnsclient.DNS_C_IN, ipapython.dnsclient.DNS_T_SRV)
> > + qname = "_ntp._udp." + tdomain
> >
> > - for result in results:
> > - if result.dns_type == ipapython.dnsclient.DNS_T_SRV:
> > - rserver = result.rdata.server.rstrip(".")
> > - if servers:
> > - servers += "," + rserver
> > - else:
> > - servers = rserver
> > - break
> > + root_logger.debug("Search DNS for SRV record of %s", qname)
> >
> > - return servers
> > + try:
> > + answers = resolver.query(qname, rdatatype.SRV)
> > + except DNSException, e:
> > + root_logger.debug("DNS record not found: %s", e.__class__.__name__)
> > + answers = []
> > +
> > + for answer in answers:
> > + root_logger.debug("DNS record found: %s", answer)
> > + server = str(answer.target).rstrip(".")
> > + break
> > +
> > + return server
>
> These function ars mostly the same, except ipadnssearchntp uses "_ntp"
> instead of "_ldap", and it doesn't take the port into account (which it
> probably should?).
> The last part of ipadnssearchkrb is also very similar.
> Please merge the common parts.
Yup, these parts could be merged - done.
>
> > def ipadnssearchkrb(self, tdomain):
>
> [...]
>
> > diff --git a/ipapython/config.py b/ipapython/config.py
> > index d4c724dc9ac754cb221fe60d7c13bd0c716dd296..e06f51a318a2b20c53a8c6933c43d58c34075407 100644
> > --- a/ipapython/config.py
> > +++ b/ipapython/config.py
> [...]
> > @@ -153,7 +155,7 @@ def __parse_config(discover_server = True):
> > try:
> > s = p.get("global", "xmlrpc_uri")
> > server = urlparse.urlsplit(s)
> > - config.default_server.extend(server.netloc)
> > + config.default_server.append(server.netloc)
>
> This is unrelated to the DNS library replacement, right? It should go in
> a separate patch, in case e.g. the big patch gets reverted.
Ok.
>
> [...]
> > + try:
> > + servers = resolver.query(name, rdatatype.SRV)
> > + domain_ok = True
> > + except DNSException:
> > + pass
> > +
> > + if not domain_ok:
> > + # try cycling on domain components of FQDN
>
> This can be just:
>
> try:
> servers = resolver.query(name, rdatatype.SRV)
> domain_ok = True
> except DNSException:
> # try cycling on domain components of FQDN
>
> The extra if and flag variable just makes it less clear.
>
> > + try:
> > + domain = dns.name.from_text(socket.getfqdn())
> > + except DNSException:
> > return False
> > - dom_name = dom_name[tok+1:]
> > - name = "_ldap._tcp." + dom_name + "."
> > - rs = ipapython.dnsclient.query(name, ipapython.dnsclient.DNS_C_IN, ipapython.dnsclient.DNS_T_SRV)
> > - rl = len(rs)
> >
> > - config.default_domain = dom_name
> > + while not domain_ok:
> > + domain = domain.parent()
> > +
> > + if str(domain) == '.':
> > + return False
> > + name = "_ldap._tcp.%s" % domain
> > + try:
> > + servers = resolver.query(name, rdatatype.SRV)
> > + domain_ok = True
> > + except DNSException:
> > + pass
> > +
> > + config.default_domain = str(domain).rstrip(".")
>
> Drop the `domain_ok` variable and just use a while True/break.
> The code flow would be more clear that way, IMO.
>
> [...]
>
> > diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
> > index 4a9db11e23c1b9ab76c9fce9150bc1546426452f..71be39132ea40172595e026a9815acc58d29f4ff 100644
> > --- a/ipapython/ipautil.py
> > +++ b/ipapython/ipautil.py
> [...]
> > +def is_host_resolvable(fqdn):
> > + found = False
> > + for rdtype in (rdatatype.A, rdatatype.AAAA):
> > + try:
> > + resolver.query(fqdn, rdtype)
> > + found = True
> > + except DNSException:
> > + continue
> > +
> > + return found
> > +
>
> This seems too complicated; why not return directly?
>
> for rdtype in rdatatype.A, rdatatype.AAAA:
> try:
> resolver.query(fqdn, rdtype)
> except DNSException:
> pass
> else:
> return True
>
>
> return False
>
> > def get_ipa_basedn(conn):
> > """
> > Get base DN of IPA suffix in given LDAP server.
> > diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
> > index 3e7ae41b5fdbc11353e43a63424f19fbc331435a..76b54c94c9d1dad169545dd30c0b9a926f19a3c6 100644
> > --- a/ipaserver/install/installutils.py
> > +++ b/ipaserver/install/installutils.py
> [...]
> > +
> > + # list of verified addresses to prevent multiple searches for the same address
> > + verified = []
>
> Use a set for this.
>
> > for a in hostaddr:
> > - if a[4][0] == '127.0.0.1' or a[4][0] == '::1':
> > - raise HostForwardLookupError("The IPA Server hostname must not resolve to localhost (%s). A routable IP address must be used. Check /etc/hosts to see if %s is an alias for %s" % (a[4][0], host_name, a[4][0]))
> > + address = a[4][0]
> > + if address in verified:
> > + continue
> [...]
>
>
Fixed.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-260-2-replace-dns-client-based-on-acutil-with-python-dns.patch
Type: text/x-patch
Size: 44483 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120516/50492ee8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-261-fix-default_server-configuration-in-ipapython.config.patch
Type: text/x-patch
Size: 1007 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120516/50492ee8/attachment-0001.bin>
More information about the Freeipa-devel
mailing list