[Freeipa-devel] [PATCH] 260 Replace DNS client based on acutil with python-dns
Petr Viktorin
pviktori at redhat.com
Tue May 15 12:02:58 UTC 2012
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
There is a forgotten acutil reference in a comment in
install/tools/ipa-dns-install:226
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.
> 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.
[...]
> + 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
[...]
--
Petr³
More information about the Freeipa-devel
mailing list