[SSSD] [PATCH] New dynamic DNS update options

Pavel Březina pbrezina at redhat.com
Thu May 2 09:38:21 UTC 2013


On 04/30/2013 05:51 PM, 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.

Commit message still contains the old name.

>
>> 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.

Code-wise ack.




More information about the sssd-devel mailing list