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.
On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
[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.
I haven't gone through the code yet, but I encountered similar problem in sudo. There is a boolean in sudo_ctx that says whether a full refresh is already in progress or not. If it is then the online callback just returns.
See sdap_sudo_full_refresh_online_cb().
On 22/04/13 10:38, Pavel Březina wrote:
On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
[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.
Hi Does this patch cause a Linux client to ask the DC for a DNS update? If so, absolutely brilliant. Cheers, Steve
On Mon, Apr 22, 2013 at 01:10:45PM +0200, steve wrote:
On 22/04/13 10:38, Pavel Březina wrote:
On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
[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.
Hi Does this patch cause a Linux client to ask the DC for a DNS update? If so, absolutely brilliant. Cheers, Steve
Not "ask", but more like "try" the update. It's a push model, not a pull one.
On 22/04/13 13:19, Jakub Hrozek wrote:
On Mon, Apr 22, 2013 at 01:10:45PM +0200, steve wrote:
On 22/04/13 10:38, Pavel Březina wrote:
On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
[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.
Hi Does this patch cause a Linux client to ask the DC for a DNS update? If so, absolutely brilliant. Cheers, Steve
Not "ask", but more like "try" the update. It's a push model, not a pull one.
Sorry, that's what I meant. I think. Would it cater for when a Linux client had had an IP update via DHCP? IOW, it would add for Linux clients what windows clients already do?
On Mon, Apr 22, 2013 at 02:01:08PM +0200, steve wrote:
On 22/04/13 13:19, Jakub Hrozek wrote:
On Mon, Apr 22, 2013 at 01:10:45PM +0200, steve wrote:
On 22/04/13 10:38, Pavel Březina wrote:
On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
[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.
Hi Does this patch cause a Linux client to ask the DC for a DNS update? If so, absolutely brilliant. Cheers, Steve
Not "ask", but more like "try" the update. It's a push model, not a pull one.
Sorry, that's what I meant. I think. Would it cater for when a Linux client had had an IP update via DHCP? IOW, it would add for Linux clients what windows clients already do?
That's the plan, yes.
On Mon, Apr 22, 2013 at 10:38:51AM +0200, Pavel Březina wrote:
On 04/17/2013 09:03 PM, Jakub Hrozek wrote:
[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.
I haven't gone through the code yet, but I encountered similar problem in sudo. There is a boolean in sudo_ctx that says whether a full refresh is already in progress or not. If it is then the online callback just returns.
See sdap_sudo_full_refresh_online_cb().
Yes, that's what I modeled the update after, but the sudo periodic task does several things I don't really need. Is there anything in my patch that is not correct?
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 inaddition to the automatic update performed when the back endbecomes online.
goes online sounds better to me.
This option is optional and applicable only when dyndns_updateis 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 :-)
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 inaddition to the automatic update performed when the back endbecomes online.goes online sounds better to me.
OK, fixed.
This option is optional and applicable only when dyndns_updateis 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.
On (30/04/13 17:51), 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 inaddition to the automatic update performed when the back endbecomes online.goes online sounds better to me.
OK, fixed.
This option is optional and applicable only when dyndns_updateis 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.
I want to comment only 2nd patch.
You introduced compile time warnings:
src/resolv/async_resolv.c: In function ‘resolv_get_string_ptr_address’: src/resolv/async_resolv.c:1433:9: warning: zero-length gnu_printf format string [-Wformat-zero-length] src/resolv/async_resolv.c:1424:17: warning: unused variable ‘hex_digits’ [-Wunused-variable]
From fda33381912699bfdcf255bfe2b7794a66218a4f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 30 Apr 2013 16:40:00 +0200 Subject: [PATCH 2/5] resolver: Return PTR record as string
This is a requirement to update the PTR records. Includes a unit test.
src/resolv/async_resolv.c | 40 ++++++++++++++++++ src/resolv/async_resolv.h | 4 ++ src/tests/resolv-tests.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+)
diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c index 60d9e05bfa61f1490263a3defa21cab1c709c5ac..b56a68da79fd25b775da8e19702999b1c3cfb57b 100644 --- a/src/resolv/async_resolv.c +++ b/src/resolv/async_resolv.c @@ -1416,6 +1416,46 @@ resolv_get_string_address_index(TALLOC_CTX *mem_ctx, return address; }
+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) {
int i;char hexbyte[3];straddr = talloc_asprintf(mem_ctx, "");if (!straddr) {return NULL;}for (i = 15; i >= 0; i--) {snprintf(hexbyte, 3, "%02x", address[i]);
^^^^^^ You call function snprintf 16 times, only because Pavel don't like bit operators. It is useless.
Please use first version of this patch and also warnings disappear with previous version.
straddr = talloc_asprintf_append(straddr, "%c.%c.",hexbyte[1], hexbyte[0]);}straddr = talloc_asprintf_append(straddr, "ip6.arpa.");- } else if (family == AF_INET) {
straddr = talloc_asprintf(mem_ctx,"%u.%u.%u.%u.in-addr.arpa.",(address[3]),(address[2]),(address[1]),(address[0]));- } else {
DEBUG(SSSDBG_CRIT_FAILURE, ("Unknown address family\n"));return NULL;- }
- return straddr;
+}
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/30/2013 01:42 PM, Lukas Slebodnik wrote:
On (30/04/13 17:51), 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 updateperformed when the back end + becomes online.
goes online sounds better to me.
OK, fixed.
This option is optional andapplicable 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.
I want to comment only 2nd patch.
You introduced compile time warnings:
src/resolv/async_resolv.c: In function ‘resolv_get_string_ptr_address’: src/resolv/async_resolv.c:1433:9: warning: zero-length gnu_printf format string [-Wformat-zero-length] src/resolv/async_resolv.c:1424:17: warning: unused variable ‘hex_digits’ [-Wunused-variable]
From fda33381912699bfdcf255bfe2b7794a66218a4f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 30 Apr 2013 16:40:00 +0200 Subject: [PATCH 2/5] resolver: Return PTR record as string
This is a requirement to update the PTR records. Includes a unit test. --- src/resolv/async_resolv.c | 40 ++++++++++++++++++ src/resolv/async_resolv.h | 4 ++ src/tests/resolv-tests.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+)
diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c index 60d9e05bfa61f1490263a3defa21cab1c709c5ac..b56a68da79fd25b775da8e19702999b1c3cfb57b 100644 --- a/src/resolv/async_resolv.c +++ b/src/resolv/async_resolv.c @@ -1416,6 +1416,46 @@ resolv_get_string_address_index(TALLOC_CTX *mem_ctx, return address; }
+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) { + int i; + char hexbyte[3]; + + straddr = talloc_asprintf(mem_ctx, ""); + if (!straddr) { + return NULL; + } + + for (i = 15; i >= 0; i--) { + snprintf(hexbyte, 3, "%02x", address[i]);
^^^^^^ You call function snprintf 16 times, only because Pavel don't like bit operators. It is useless.
Please use first version of this patch and also warnings disappear with previous version.
I'm going to contradict this, actually. Given that this is a computationally-rare event, I'd say that the readability gains are well-worth the slight loss in performance. Let's not optimize unnecessarily. This is *not* going to be a bottleneck.
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 inaddition to the automatic update performed when the back endbecomes online.goes online sounds better to me.
OK, fixed.
This option is optional and applicable only when dyndns_updateis 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.
On Thu, May 02, 2013 at 11:38:21AM +0200, Pavel Březina wrote:
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 inaddition to the automatic update performed when the back endbecomes online.goes online sounds better to me.
OK, fixed.
This option is optional and applicable only when dyndns_updateis 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.
The attached patches rename the commit message to include new name of dyndns_force_tcp and remove the compilation warning in the resolver code.
On Thu, May 02, 2013 at 08:03:18PM +0200, Jakub Hrozek wrote:
Code-wise ack.
The attached patches rename the commit message to include new name of dyndns_force_tcp and remove the compilation warning in the resolver code.
Pavel spotted a compilation warning in resolv_get_string_ptr_address. New patches attached, but just the one that includes resolv_get_string_ptr_address changed.
On Fri, May 03, 2013 at 01:20:00PM +0200, Jakub Hrozek wrote:
On Thu, May 02, 2013 at 08:03:18PM +0200, Jakub Hrozek wrote:
Code-wise ack.
The attached patches rename the commit message to include new name of dyndns_force_tcp and remove the compilation warning in the resolver code.
Pavel spotted a compilation warning in resolv_get_string_ptr_address. New patches attached, but just the one that includes resolv_get_string_ptr_address changed.
Lukas found out that the patches didn't compile on their own when testing them. I probably squashed a change into a different patch that I should have when developing them. This would have broken git-bisect.
No functional change is present in these patches, I just shuffled some hunks to make the patches compile on their own.
On (03/05/13 16:29), Jakub Hrozek wrote:
On Fri, May 03, 2013 at 01:20:00PM +0200, Jakub Hrozek wrote:
On Thu, May 02, 2013 at 08:03:18PM +0200, Jakub Hrozek wrote:
Code-wise ack.
The attached patches rename the commit message to include new name of dyndns_force_tcp and remove the compilation warning in the resolver code.
Pavel spotted a compilation warning in resolv_get_string_ptr_address. New patches attached, but just the one that includes resolv_get_string_ptr_address changed.
Lukas found out that the patches didn't compile on their own when testing them. I probably squashed a change into a different patch that I should have when developing them. This would have broken git-bisect.
No functional change is present in these patches, I just shuffled some hunks to make the patches compile on their own.
Patches have been tested with AD dyn dns updates.
Ack
On Fri, May 03, 2013 at 08:13:29PM +0200, Lukas Slebodnik wrote:
On (03/05/13 16:29), Jakub Hrozek wrote:
On Fri, May 03, 2013 at 01:20:00PM +0200, Jakub Hrozek wrote:
On Thu, May 02, 2013 at 08:03:18PM +0200, Jakub Hrozek wrote:
Code-wise ack.
The attached patches rename the commit message to include new name of dyndns_force_tcp and remove the compilation warning in the resolver code.
Pavel spotted a compilation warning in resolv_get_string_ptr_address. New patches attached, but just the one that includes resolv_get_string_ptr_address changed.
Lukas found out that the patches didn't compile on their own when testing them. I probably squashed a change into a different patch that I should have when developing them. This would have broken git-bisect.
No functional change is present in these patches, I just shuffled some hunks to make the patches compile on their own.
Patches have been tested with AD dyn dns updates.
Ack
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Stephen Gallagher -
steve