[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