[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