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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
