[SSSD] [PATCH] New dynamic DNS update options

Stephen Gallagher sgallagh at redhat.com
Tue Apr 30 17:56:43 UTC 2013


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/30/2013 01:42 PM, Lukas Slebodnik wrote:
> 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.
> 

I'm going to contradict this, actually. Given that this is a
computationally-rare event, I'd say that the readability gains are
well-worth the slight loss in performance. Let's not optimize
unnecessarily. This is *not* going to be a bottleneck.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlGABdsACgkQeiVVYja6o6MRVwCffHrLFz6tnwnWSd1BaP2yIE9Q
+1wAniATxuEUNwGRpnJk1W9JJA3x23YD
=uGWS
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list