On 18.2.2014 17:34, Nathaniel McCallum wrote:
On Tue, 2014-02-18 at 17:06 +0100, Petr Viktorin wrote:
On 02/18/2014 04:45 PM, Petr Spacek wrote:
Hello,

Add wait_for_dns option to default.conf.

This option makes record changes in DNS tree synchronous.
IPA calls will wait until new data are visible over DNS protocol.

It is intended only for testing - it should prevent tests from
failing if there is bigger delay between change in LDAP and DNS.

I would recommend value like 10 seconds.

Here are a few Python nitpicks you requested.

Thank you very much. This new version solves problems you found + adds proper handling for real DNS timeouts.

It seems to me like a more general TimeoutError would be useful in a
broader context. DNSTimeout seems overly narrow to me, unless I'm
missing something.

I would like to keep them separate. DNSTimeout shouldn't be handled at all because it means that your DNS server or database is dead or broken in some interesting way.

I assume that generic TimeoutError could be interpreted as 'try it again'/'failover' or something like that.

Maybe the DNSTimeout is not the best name, I'm open to suggestions.

--
Petr^2 Spacek
From 7ad81ab266754afb1e5b33b459bc92399ff2f09c Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Fri, 14 Feb 2014 15:33:24 +0100
Subject: [PATCH] Add wait_for_dns option to default.conf.

This option makes record changes in DNS tree synchronous.
IPA calls will wait until new data are visible over DNS protocol.

It is intended only for testing - it should prevent tests from
failing if there is bigger delay between change in LDAP and DNS.
---
 ipa-client/man/default.conf.5 |   3 +
 ipalib/constants.py           |   1 +
 ipalib/errors.py              |  18 ++++++
 ipalib/plugins/dns.py         | 145 ++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 163 insertions(+), 4 deletions(-)

diff --git a/ipa-client/man/default.conf.5 b/ipa-client/man/default.conf.5
index 5d5a48db62cb97e7424b42b6cb70d0c872b2bc34..7e3e02858732789776b9225ae6e9cffeac4004d1 100644
--- a/ipa-client/man/default.conf.5
+++ b/ipa-client/man/default.conf.5
@@ -178,6 +178,9 @@ Used internally in the IPA source package to verify that the API has not changed
 .B verbose <boolean>
 When True provides more information. Specifically this sets the global log level to "info".
 .TP
+.B wait_for_dns <time in seconds>
+Controls whether the IPA commands dnsrecord\-{add,mod,del} work synchronously or not. The DNS commands will wait up to the specified time until the DNS server returns an up\-to\-date answer to a query for modified records. The DNS commands will return a DNSTimeout exception if the answer doesn't match the expected value after the specified timeout. The DNS queries will be sent to the resolver configured in /etc/resolv.conf on the IPA server. Do not enable this in production! It could cause problems if the resolver on IPA server uses a caching server instead of a local authoritative server. The default is disabled (the option is not present).
+.TP
 .B xmlrpc_uri <URI>
 Specifies the URI of the XML\-RPC server for a client. This may be used by IPA, and is used by some external tools, such as ipa\-getcert. Example: https://ipa.example.com/ipa/xml
 .TP
diff --git a/ipalib/constants.py b/ipalib/constants.py
index ae0827729764983675d5ae59bbd16bad1c0805ce..d6955f8cb62822d123f6debcefa4a2f35e40aa96 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -139,6 +139,7 @@ DEFAULT_CONFIG = (
     ('debug', False),
     ('startup_traceback', False),
     ('mode', 'production'),
+    ('wait_for_dns', False),
 
     # CA plugin:
     ('ca_host', FQDN),  # Set in Env._finalize_core()
diff --git a/ipalib/errors.py b/ipalib/errors.py
index 716decb2b41baf5470a1dc23c0cfb5d1c995e5ff..e6563c0a0ea0f65457b942426e45283dd237ed6e 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1512,6 +1512,24 @@ class DatabaseTimeout(DatabaseError):
     format = _('LDAP timeout')
 
 
+class DNSTimeout(ExecutionError):
+    """
+    **4212** Raised when an DNS query didn't return expected answer
+    in expected time period
+
+    For example:
+
+    >>> raise DNSTimeout(expected="zone3.test. 86400 IN A 192.0.2.1", \
+                         got="zone3.test. 86400 IN A 192.168.1.1")
+    Traceback (most recent call last):
+      ...
+    DNSTimeout: DNS query timeout: Expected {zone3.test. 86400 IN A 192.0.2.1} got {zone3.test. 86400 IN A 192.168.1.1}
+    """
+
+    errno = 4212
+    format = _('DNS query timeout: Expected {%(expected)s} got {%(got)s}')
+
+
 class CertificateError(ExecutionError):
     """
     **4300** Base class for Certificate execution errors (*4300 - 4399*).
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index e7301a9f78466e9a790d26f03bfab757de501ed6..ee330df93e02e895971b7b1090ea969080e072e4 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -24,6 +24,7 @@ import netaddr
 import time
 import re
 import dns.name
+import dns.resolver
 
 from ipalib.request import context
 from ipalib import api, errors, output
@@ -2396,6 +2397,112 @@ class dnsrecord(LDAPObject):
                                   'NS record except when located in a zone root '
                                   'record (RFC 6672, section 2.3)'))
 
+    def wait_for_modified_entries(self, entries):
+        '''Call wait_for_modified_attrs for all entries in given dict.
+
+        :param entries:
+            Dict {(dns_domain, dns_name): entry_for_wait_for_modified_attrs}
+        '''
+        for entry_name in entries:
+            dns_domain = dns.name.from_text(entry_name[0])
+            dns_name = dns.name.from_text(entry_name[1], origin=dns_domain)
+            self.wait_for_modified_attrs(entries[entry_name], dns_name,
+                                         dns_domain)
+
+    def wait_for_modified_attrs(self, entry_attrs, dns_name, dns_domain):
+        '''Wait until DNS resolver returns up-to-date answer for given entry.
+
+        :param entry_attrs:
+            None if the entry was deleted from LDAP or
+            LDAPEntry instance containing at least all modified attributes.
+        :param dns_name: FQDN as dns.name.Name instance or string
+        '''
+        record_attr_suf = 'record'
+        max_retries = int(self.api.env['wait_for_dns'])
+        warn_retries = max_retries / 2
+        period = 1  # second
+
+        # represent data in LDAP as dictionary rdtype => rrset
+        ldap_rrsets = {}
+        if entry_attrs:
+            for attr in entry_attrs.iterkeys():
+                if not attr.endswith(record_attr_suf):
+                    continue
+
+                rdtype = dns.rdatatype.from_text(attr[0:-len(record_attr_suf)])
+                if entry_attrs[attr]:
+                    try:
+                        # TTL here can be arbitrary value because it is ignored
+                        # during comparison
+                        ldap_rrset = dns.rrset.from_text(
+                            dns_name, 86400, dns.rdataclass.IN, rdtype,
+                            *map(str, entry_attrs[attr]))
+
+                        # make sure that all names are absolute so RRset
+                        # comparison will work
+                        for ldap_rr in ldap_rrset:
+                            ldap_rr.choose_relativity(origin=dns_domain,
+                                                      relativize=False)
+                    except dns.exception.SyntaxError as e:
+                        self.log.error(
+                            'DNS syntax error: %s %s %s: %s',
+                            dns_name, dns.rdatatype.to_text(rdtype),
+                            entry_attrs[attr], e)
+                        raise
+                else:
+                    ldap_rrset = None
+
+                ldap_rrsets[rdtype] = ldap_rrset
+        else:
+            # all records were deleted => name should not exist in DNS
+            rdtype = dns.rdatatype.from_text('A')
+            ldap_rrsets[rdtype] = 'NXDOMAIN'
+
+        for rdtype in ldap_rrsets:
+            failures = 0
+            logfn = self.log.debug
+            # compare ldap_rrsets with data in DNS
+            ldap_rrset = ldap_rrsets[rdtype]
+            logfn('querying DNS server - expecting answer {%r}',
+                  str(ldap_rrset))
+
+            while failures < max_retries:
+                if failures >= warn_retries:
+                    logfn = self.log.warn
+                try:
+                    dns_answer = dns.resolver.query(dns_name, rdtype,
+                                                    dns.rdataclass.IN,
+                                                    raise_on_no_answer=False)
+                    dns_rrset = dns_answer.rrset
+                except dns.resolver.NXDOMAIN:
+                    dns_rrset = 'NXDOMAIN'
+                except dns.resolver.NoNameservers:
+                    dns_rrset = 'SERVFAIL/timeout'
+
+                if dns_rrset == ldap_rrset:
+                    logfn('DNS answer matches expectations')
+                    return
+
+                failures += 1
+                logfn('waiting for DNS answer {%r} - got {%r}; '
+                      'waiting %s seconds before next try',
+                      str(ldap_rrset), str(dns_rrset), period)
+
+                time.sleep(period)
+
+            # Maximum number of retries was reached:
+            # Do not raise exception only if we have got n successive SERVFAILs
+            # or if we are adding NS record and answer have been NXDOMAIN.
+            # May be that user intentionally created invalid zone
+            # or the NS record is configured only on parent (this) server.
+            if ((rdtype == dns.rdatatype.NS and dns_rrset == 'NXDOMAIN')
+               or dns_rrset == 'SERVFAIL/timeout'):
+                    self.log.warn('giving up after %s retries - '
+                                  'may be that {%r} is expected answer',
+                                  failures, str(dns_rrset))
+            else:
+                    raise errors.DNSTimeout(expected=ldap_rrset, got=dns_rrset)
+
 api.register(dnsrecord)
 
 
@@ -2558,6 +2665,11 @@ class dnsrecord_add(LDAPCreate):
                 entry_attrs[attr] = list(set(old_entry.get(attr, []) + vals))
 
         self.obj.check_record_type_collisions(keys, old_entry, entry_attrs)
+        # TODO: morph context to {(keys[0], keys[1]): entry_attrs.copy()}
+        context.dnsrecord_entry_mods = getattr(context, 'dnsrecord_entry_mods',
+                                               {})
+        context.dnsrecord_entry_mods[(keys[0], keys[1])] = entry_attrs.copy()
+
         return dn
 
     def exc_callback(self, keys, options, exc, call_func, *call_args, **call_kwargs):
@@ -2585,6 +2697,8 @@ class dnsrecord_add(LDAPCreate):
 
         self.obj.postprocess_record(entry_attrs, **options)
 
+        if self.api.env['wait_for_dns']:
+            self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
         return dn
 
 api.register(dnsrecord_add)
@@ -2660,6 +2774,10 @@ class dnsrecord_mod(LDAPUpdate):
                 entry_attrs[attr] = list(set(old_entry[attr] + new_dnsvalue))
 
         self.obj.check_record_type_collisions(keys, old_entry, entry_attrs)
+
+        context.dnsrecord_entry_mods = getattr(context, 'dnsrecord_entry_mods',
+                                               {})
+        context.dnsrecord_entry_mods[(keys[0], keys[1])] = entry_attrs.copy()
         return dn
 
     def execute(self, *keys, **options):
@@ -2681,7 +2799,13 @@ class dnsrecord_mod(LDAPUpdate):
                     break
 
             if del_all:
-                return self.obj.methods.delentry(*keys, version=options['version'])
+                result = self.obj.methods.delentry(*keys)
+                # indicate that entry was deleted
+                context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
+
+        if self.api.env['wait_for_dns']:
+            self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
+
         return result
 
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
@@ -2825,7 +2949,10 @@ class dnsrecord_del(LDAPUpdate):
         # set del_all flag in context
         # when the flag is enabled, the entire DNS record object is deleted
         # in a post callback
-        setattr(context, 'del_all', del_all)
+        context.del_all = del_all
+        context.dnsrecord_entry_mods = getattr(context, 'dnsrecord_entry_mods',
+                                               {})
+        context.dnsrecord_entry_mods[(keys[0], keys[1])] = entry_attrs.copy()
 
         return dn
 
@@ -2837,13 +2964,23 @@ class dnsrecord_del(LDAPUpdate):
                         error=_('Zone record \'%s\' cannot be deleted') \
                                 % _dns_zone_record
                       )
-            return self.obj.methods.delentry(*keys, version=options['version'])
+            result = self.obj.methods.delentry(*keys,
+                                               version=options['version'])
+            if self.api.env['wait_for_dns']:
+                entries = {(keys[0], keys[1]): None}
+                self.obj.wait_for_modified_entries(entries)
+            return result
 
         result = super(dnsrecord_del, self).execute(*keys, **options)
 
         if getattr(context, 'del_all', False) and not \
                 self.obj.is_pkey_zone_record(*keys):
-            return self.obj.methods.delentry(*keys, version=options['version'])
+            result = self.obj.methods.delentry(*keys,
+                                               version=options['version'])
+            context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
+
+        if self.api.env['wait_for_dns']:
+            self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
         return result
 
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
-- 
1.8.5.3

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to