[SSSD] [PATCH] New dynamic DNS update options
Pavel Březina
pbrezina at redhat.com
Mon Apr 22 12:04:03 UTC 2013
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.
> + 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. :-)
>
> 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]);
> + 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.
> + } 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.
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.
Also please rename be_nsupdate_create_msg() to
be_nsupdate_create_fwd_msg() to make it similar to
be_nsupdate_create_ptr_msg().
Patch 4:
I think dyndns_force_tcp would be a better name for the option.
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 :-)
More information about the sssd-devel
mailing list