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.