[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