[SSSD] [PATCH] New dynamic DNS update options

Lukas Slebodnik lslebodn at redhat.com
Tue Apr 30 17:42:18 UTC 2013


On (30/04/13 17:51), Jakub Hrozek wrote:
>On Mon, Apr 22, 2013 at 02:04:03PM +0200, Pavel Březina wrote:
>> On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
>> >Hi,
>> >
>> >the attached patches implement a couple of new dynamic DNS options. The
>> >AD dyndns code will be just a wrapper around these options.
>> >
>> >[PATCH 1/5] dyndns: new option dyndns_refresh_interval
>> >This new options adds the possibility of updating the DNS entries
>> >periodically regardless if they have changed or not. This feature
>> >will be useful mainly in AD environments where the Windows clients
>> >periodically update their DNS records.
>> >
>> >There is one place (in IPA dyndns code in this patch but also in AD code
>> >later on) that I wanted to discuss specifically. It may happen that the
>> >periodic update would trigger going online in which case the online
>> >callback would fire and another dyndns update would be invoked as an
>> >online callback. To prevent a race between these two updates, there is
>> >an interval, currently hardcoded to 60 seconds that would just make the
>> >next update quit without doing anything. Ideas on how to fix the problem
>> >without a hardcoded timeout are welcome.
>> >
>> >[PATCH 2/5] resolver: Return PTR record as string
>> >Having the possibility to format a PTR record based on an A/AAAA record
>> >is a requirement to update the PTR records.
>> >Includes a unit test.
>> >
>> >[PATCH 3/5] dyndns: New option dyndns_update_ptr
>> >https://fedorahosted.org/sssd/ticket/1832
>> >
>> >While some servers, such as FreeIPA allow the PTR record to be
>> >synchronized when the forward record is updated, other servers,
>> >including Active Directory, require that the PTR record is synchronized
>> >manually.
>> >
>> >This patch adds a new option, dyndns_update_ptr that automatically
>> >generates appropriate DNS update message for updating the reverse zone.
>> >
>> >The PTR update is performed separately from the forward record update
>> >mostly because the current IPA dyndns code allows the zone to be
>> >specified in the message, so another zone must be updated using another
>> >message.
>> >
>> >This option is off by default in the IPA provider.
>> >
>> >[PATCH 4/5] dyndns: new option dyndns_use_tcp
>> >https://fedorahosted.org/sssd/ticket/1831
>> >
>> >Adds a new option that can be used to force nsupdate to only use TCP to
>> >communicate with the DNS server.
>> >
>> >[PATCH 5/5] dyndns: new option dyndns_auth
>> >This options is mostly provided for future expansion. Currently it is
>> >undocumented and both IPA and AD dynamic DNS updates default to
>> >GSS-TSIG. Allowed values are GSS-TSIG and none.
>> 
>> Hi,
>> good job. I have just few comments inline.
>> 
>> >0001-dyndns-new-option-dyndns_refresh_interval.patch
>> >
>> 
>> >                  <varlistentry>
>> >+                    <term>dyndns_refresh_interval (integer)</term>
>> >+                    <listitem>
>> >+                        <para>
>> >+                            How often should the back end perform periodic DNS update in
>> >+                            addition to the automatic update performed when the back end
>> >+                            becomes online.
>> 
>> goes online sounds better to me.
>
>OK, fixed.
>
>> 
>> >+                            This option is optional and applicable only when dyndns_update
>> >+                            is true.
>> >+                        </para>
>> >+                        <para>
>> >+                            Default: 0 (disabled)
>> >+                        </para>
>> >+                    </listitem>
>> >+                </varlistentry>
>> 
>> >+void ipa_dyndns_timer(void *pvt)
>> >+{
>> >+    struct ipa_options *ctx = talloc_get_type(pvt, struct ipa_options);
>> >+    struct sdap_id_ctx *sdap_ctx = ctx->id_ctx->sdap_id_ctx;
>> >+    struct tevent_req *req;
>> >+    struct ipa_dyndns_timer_ctx *timer_ctx;
>> >+    errno_t ret;
>> >+
>> >+    timer_ctx = talloc_zero(ctx, struct ipa_dyndns_timer_ctx);
>> >+    if (timer_ctx == NULL) {
>> >+        /* Not much we can do */
>> 
>> I agree there is not much we can do, but we can report it at least. :-)
>
>Added a DEBUG message.
>
>> 
>> >
>> >0002-resolver-Return-PTR-record-as-string.patch
>> >+char *
>> >+resolv_get_string_ptr_address(TALLOC_CTX *mem_ctx,
>> >+                              int family, uint8_t *address)
>> >+{
>> >+    char *straddr;
>> >+    static char hex_digits[] = {
>> >+                '0', '1', '2', '3', '4', '5', '6', '7',
>> >+                '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'
>> >+    };
>> >+
>> >+    if (family == AF_INET6) {
>> >+        char *cp;
>> >+        int i;
>> >+
>> >+        straddr = talloc_zero_array(mem_ctx, char, 128);
>> >+        if (!straddr) {
>> >+            return NULL;
>> >+        }
>> >+
>> >+        cp = straddr;
>> >+        for (i = 15; i >= 0; i--) {
>> >+                *cp++ = hex_digits[address[i] & 0x0f];
>> >+                *cp++ = '.';
>> >+                *cp++ = hex_digits[(address[i] >> 4) & 0x0f];
>> >+                *cp++ = '.';
>> >+        }
>> 
>> I'm not opposed this, but wouldn't it be better to use
>> sprintf("%02x") instead of this bit wise magic? Something like:
>> 
>> char hexbyte[3];
>> snprintf(hexbyte, 2, "%02x", address[i]);
>> talloc_asprintf(straddr, "%c.%c.", hexbyte[1], hexbyte[0]);
>
>OK, that's more readable.
>
>> 
>> >+        strcpy(cp, "ip6.arpa.");
>> >+    } else if (family == AF_INET) {
>> >+        straddr = talloc_asprintf(mem_ctx,
>> >+                                  "%u.%u.%u.%u.in-addr.arpa.",
>> >+                                  (address[3] & 0xff),
>> >+                                  (address[2] & 0xff),
>> >+                                  (address[1] & 0xff),
>> >+                                  (address[0] & 0xff));
>> 
>> address is already uint8_t so I believe applying 0xff is not necessary.
>
>Removed the mask.
>
>> 
>> >+    } else {
>> >+        DEBUG(SSSDBG_CRIT_FAILURE, ("Unknown address family\n"));
>> >+        return NULL;
>> >+    }
>> >+
>> >+    return straddr;
>> >+}
>> >+
>> >
>> >0003-dyndns-New-option-dyndns_update_ptr.patch
>> 
>> nsupdate_msg_add_common() should also take update_msg as parameter
>> as do other add functions.
>> 
>
>No, the purpose is to create the update message, not add contents to
>existing one. I renamed the function to nsupdate_msg_create_common.
>
>> You are leaking update_msg in be_nsupdate_create_msg() and
>> be_nsupdate_create_ptr_msg(). If one of the _add function or
>> talloc_asprintf_append() fails, you have to free the original
>> pointer.
>> 
>> I know it is all allocated on state so it will be freed later
>> anyway, but still...
>> 
>> Using tmp_ctx and "done" pattern would be much better in these functions.
>> 
>
>OK, I went with tmp_ctx.
>
>> Also please rename be_nsupdate_create_msg() to
>> be_nsupdate_create_fwd_msg() to make it similar to
>> be_nsupdate_create_ptr_msg().
>> 
>
>Renamed.
>
>> Patch 4:
>> I think dyndns_force_tcp would be a better name for the option.
>> 
>
>At first I didn't agree, but you're right that "force" more closely
>describes that without this option, nsupdate might still opt for TCP
>internally.
>
>> Patch 5:
>> Originally, I wanted to write that you shouldn't spend time on this
>> patch, but I kinda like the new way of creating nsupdate args. So
>> kudos :-)
>
>Patch 5 is the same.

I want to comment only 2nd patch.

You introduced compile time warnings:

src/resolv/async_resolv.c: In function ‘resolv_get_string_ptr_address’:
src/resolv/async_resolv.c:1433:9: warning: zero-length gnu_printf format string [-Wformat-zero-length]
src/resolv/async_resolv.c:1424:17: warning: unused variable ‘hex_digits’ [-Wunused-variable]




>From fda33381912699bfdcf255bfe2b7794a66218a4f Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Tue, 30 Apr 2013 16:40:00 +0200
>Subject: [PATCH 2/5] resolver: Return PTR record as string
>
>This is a requirement to update the PTR records.
>Includes a unit test.
>---
> src/resolv/async_resolv.c |  40 ++++++++++++++++++
> src/resolv/async_resolv.h |   4 ++
> src/tests/resolv-tests.c  | 104 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 148 insertions(+)
>
>diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c
>index 60d9e05bfa61f1490263a3defa21cab1c709c5ac..b56a68da79fd25b775da8e19702999b1c3cfb57b 100644
>--- a/src/resolv/async_resolv.c
>+++ b/src/resolv/async_resolv.c
>@@ -1416,6 +1416,46 @@ resolv_get_string_address_index(TALLOC_CTX *mem_ctx,
>     return address;
> }
> 
>+char *
>+resolv_get_string_ptr_address(TALLOC_CTX *mem_ctx,
>+                              int family, uint8_t *address)
>+{
>+    char *straddr;
>+    static char hex_digits[] = {
>+                '0', '1', '2', '3', '4', '5', '6', '7',
>+                '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'
>+    };
>+
>+    if (family == AF_INET6) {
>+        int i;
>+        char hexbyte[3];
>+
>+        straddr = talloc_asprintf(mem_ctx, "");
>+        if (!straddr) {
>+            return NULL;
>+        }
>+
>+        for (i = 15; i >= 0; i--) {
>+            snprintf(hexbyte, 3, "%02x", address[i]);
             ^^^^^^
You call function snprintf 16 times, only because Pavel don't like bit
operators. It is useless.

Please use first version of this patch and also warnings disappear with
previous version.

>+            straddr = talloc_asprintf_append(straddr, "%c.%c.",
>+                                             hexbyte[1], hexbyte[0]);
>+        }
>+        straddr = talloc_asprintf_append(straddr, "ip6.arpa.");
>+    } else if (family == AF_INET) {
>+        straddr = talloc_asprintf(mem_ctx,
>+                                  "%u.%u.%u.%u.in-addr.arpa.",
>+                                  (address[3]),
>+                                  (address[2]),
>+                                  (address[1]),
>+                                  (address[0]));
>+    } else {
>+        DEBUG(SSSDBG_CRIT_FAILURE, ("Unknown address family\n"));
>+        return NULL;
>+    }
>+
>+    return straddr;
>+}



More information about the sssd-devel mailing list