On Mon, Sep 14, 2015 at 07:53:06PM +0200, Pavel Reichl wrote:
On 09/14/2015 05:29 PM, Jakub Hrozek wrote:
>On Sun, Sep 13, 2015 at 01:43:03PM +0200, Pavel Reichl wrote:
struct sss_iface_addr **_iface_addrs);
>>+
>>+struct sss_iface_addr *
>>+sss_iface_addr_get_next(struct sss_iface_addr *address);
>>+
>>+struct sockaddr_storage*
>>+sss_iface_addr_get_item(struct sss_iface_addr *address);
>
>When we agree on the rest, please add tests for these two functions. I
>know they are trivial, but we should keep adding tests.
>
Hello Jakub, thanks for comments. I hope I addressed all of them except the request for
tests - I'll do it with next iteration of patch. Thanks!
From e48fc2844ab41cec3a2aa98b01468c673fb33c74 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Sat, 12 Sep 2015 09:09:35 -0400
Subject: [PATCH] DDNS: execute nsupdate for single update of PTR rec
nsupdate fails definitely if any of update request fails when GSSAPI is used.
As tmp solution nsupdate is executed for each update.
Resolves:
https://fedorahosted.org/sssd/ticket/2783
---
src/providers/dp_dyndns.c | 155 +++++++++++++++++++++++----------------
src/providers/dp_dyndns.h | 11 ++-
src/providers/ldap/sdap_dyndns.c | 77 ++++++++++++++++++-
3 files changed, 175 insertions(+), 68 deletions(-)
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index 0577743cb2daca9c0e86b5beb6bf059ee7b5783f..e78f7586b22f42572749d9407adad6b4c231bf4d
100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -52,6 +52,25 @@ struct sss_iface_addr {
struct sockaddr_storage *addr;
};
+struct sockaddr_storage*
+sss_iface_addr_get_item(struct sss_iface_addr *address)
We should rename this one, see below..
+{
+ if (address == NULL) {
+ return NULL;
+ }
+
+ return address->addr;
+}
+
+struct sss_iface_addr *sss_iface_addr_get_next(struct sss_iface_addr *address)
+{
+ if (address) {
+ return address->next;
+ }
+
+ return NULL;
+}
+
void sss_iface_addr_concatenate(struct sss_iface_addr **list,
struct sss_iface_addr *list2)
{
@@ -293,80 +312,92 @@ nsupdate_msg_add_fwd(char *update_msg, struct sss_iface_addr
*addresses,
return talloc_asprintf_append(update_msg, "send\n");
}
-static char *
-nsupdate_msg_add_ptr(char *update_msg, struct sss_iface_addr *addresses,
- const char *hostname, int ttl, uint8_t remove_af,
- struct sss_iface_addr *old_addresses)
+static uint8_t *nsupdate_convert_address(struct sockaddr_storage *add_address)
+{
+ uint8_t *addr;
+
+ switch(add_address->ss_family) {
+ case AF_INET:
+ addr = (uint8_t *) &((struct sockaddr_in *) add_address)->sin_addr;
+ break;
+ case AF_INET6:
+ addr = (uint8_t *) &((struct sockaddr_in6 *) add_address)->sin6_addr;
+ break;
+ default:
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
+ addr = NULL;
+ break;
+ }
+
+ return addr;
+}
+
Not directly related to this patch, but this going back and forth between
sockaddr_storage and uint8_t pointer made me wonder if we should change
resolv_get_string_ptr_address to accept sockaddr storage instead of
these hacks?
+static bool nsupdate_should_remove_addr(int address_family, uint8_t
remove_af)
+{
+ bool ret = false;
+
+ switch(address_family) {
+ case AF_INET:
+ if (remove_af & DYNDNS_REMOVE_A) {
+ ret = true;
+ }
+ break;
+ case AF_INET6:
+ if (remove_af & DYNDNS_REMOVE_AAAA) {
+ ret = true;
+ }
+ break;
+ default:
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
+ ret = false;
+ }
+
+ return ret;
+}
+
+static char *nsupdate_msg_add_ptr(char *update_msg,
+ struct sockaddr_storage *address,
+ const char *hostname,
+ int ttl,
+ uint8_t remove_af,
+ bool delete)
{
- struct sss_iface_addr *new_record, *old_record;
char *strptr;
uint8_t *addr;
- DLIST_FOR_EACH(old_record, old_addresses) {
- switch(old_record->addr->ss_family) {
- case AF_INET:
- if (!(remove_af & DYNDNS_REMOVE_A)) {
- continue;
- }
- addr = (uint8_t *) &((struct sockaddr_in *)
old_record->addr)->sin_addr;
- break;
- case AF_INET6:
- if (!(remove_af & DYNDNS_REMOVE_AAAA)) {
- continue;
- }
- addr = (uint8_t *) &((struct sockaddr_in6 *)
old_record->addr)->sin6_addr;
- break;
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
- return NULL;
- }
+ if (delete
+ && !nsupdate_should_remove_addr(address->ss_family, remove_af)) {
This seems a bit strange to me, because if we hit this condition, then
the whole request is aborted. Shouldn't we just skip this address
instead?
+ return NULL;
+ }
- strptr = resolv_get_string_ptr_address(update_msg,
old_record->addr->ss_family,
- addr);
- if (strptr == NULL) {
- return NULL;
- }
+ addr = nsupdate_convert_address(address);
+ if (addr == NULL) {
+ return NULL;
+ }
+ strptr = resolv_get_string_ptr_address(update_msg, address->ss_family,
+ addr);
+ if (strptr == NULL) {
+ return NULL;
+ }
+
+ if (delete) {
/* example: update delete 38.78.16.10.in-addr.arpa. in PTR */
update_msg = talloc_asprintf_append(update_msg,
"update delete %s in PTR\n"
"send\n",
strptr);
- talloc_free(strptr);
- if (update_msg == NULL) {
- return NULL;
- }
- }
-
- /* example: update add 11.78.16.10.in-addr.arpa. 85000 in PTR
testvm.example.com */
- DLIST_FOR_EACH(new_record, addresses) {
- switch(new_record->addr->ss_family) {
- case AF_INET:
- addr = (uint8_t *) &((struct sockaddr_in *)
new_record->addr)->sin_addr;
- break;
- case AF_INET6:
- addr = (uint8_t *) &((struct sockaddr_in6 *)
new_record->addr)->sin6_addr;
- break;
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
- return NULL;
- }
-
- strptr = resolv_get_string_ptr_address(update_msg,
new_record->addr->ss_family,
- addr);
- if (strptr == NULL) {
- return NULL;
- }
-
+ } else {
/* example: update delete 38.78.16.10.in-addr.arpa. in PTR */
update_msg = talloc_asprintf_append(update_msg,
"update add %s %d in PTR %s.\n"
"send\n",
strptr, ttl, hostname);
- talloc_free(strptr);
- if (update_msg == NULL) {
- return NULL;
- }
+ }
+
+ talloc_free(strptr);
+ if (update_msg == NULL) {
+ return NULL;
}
return update_msg;
@@ -472,8 +503,8 @@ errno_t
be_nsupdate_create_ptr_msg(TALLOC_CTX *mem_ctx, const char *realm,
const char *servername, const char *hostname,
const unsigned int ttl, uint8_t remove_af,
- struct sss_iface_addr *addresses,
- struct sss_iface_addr *old_addresses,
+ struct sockaddr_storage *address,
+ bool del_phase,
char **_update_msg)
{
errno_t ret;
@@ -490,8 +521,8 @@ be_nsupdate_create_ptr_msg(TALLOC_CTX *mem_ctx, const char *realm,
goto done;
}
- update_msg = nsupdate_msg_add_ptr(update_msg, addresses, hostname,
- ttl, remove_af, old_addresses);
+ update_msg = nsupdate_msg_add_ptr(update_msg, address, hostname, ttl,
+ remove_af, del_phase);
if (update_msg == NULL) {
ret = ENOMEM;
goto done;
diff --git a/src/providers/dp_dyndns.h b/src/providers/dp_dyndns.h
index 9f72331b6fd68e17e9eb91505a13fc839d3f54e1..75c595c22976b498fc08e88ee1015cbce0646822
100644
--- a/src/providers/dp_dyndns.h
+++ b/src/providers/dp_dyndns.h
@@ -98,8 +98,8 @@ errno_t
be_nsupdate_create_ptr_msg(TALLOC_CTX *mem_ctx, const char *realm,
const char *servername, const char *hostname,
const unsigned int ttl, uint8_t remove_af,
- struct sss_iface_addr *addresses,
- struct sss_iface_addr *old_addresses,
+ struct sockaddr_storage *address,
+ bool del_phase,
I don't think the option should contain 'phase' in its name, the fact
that we have some phases is a detail from the sdap layer.
char **_update_msg);
/* Returns:
@@ -133,4 +133,11 @@ errno_t
sss_get_dualstack_addresses(TALLOC_CTX *mem_ctx,
struct sockaddr *ss,
struct sss_iface_addr **_iface_addrs);
+
+struct sss_iface_addr *
+sss_iface_addr_get_next(struct sss_iface_addr *address);
+
+struct sockaddr_storage*
+sss_iface_addr_get_item(struct sss_iface_addr *address);
+
#endif /* DP_DYNDNS_H_ */
diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
index 2a179fd1b5e88bdf2442657ff6fa1dcc55417467..b1246026c7f44d5cbe7766522b5cb6e73c4bd2cb
100644
--- a/src/providers/ldap/sdap_dyndns.c
+++ b/src/providers/ldap/sdap_dyndns.c
@@ -60,6 +60,8 @@ struct sdap_dyndns_update_state {
enum be_nsupdate_auth auth_type;
bool fallback_mode;
char *update_msg;
+ struct sss_iface_addr *ptr_addr_iter;
+ bool del_phase;
};
static void sdap_dyndns_update_addrs_done(struct tevent_req *subreq);
@@ -70,6 +72,9 @@ static errno_t sdap_dyndns_update_step(struct tevent_req *req);
static errno_t sdap_dyndns_update_ptr_step(struct tevent_req *req);
static void sdap_dyndns_update_done(struct tevent_req *subreq);
static void sdap_dyndns_update_ptr_done(struct tevent_req *subreq);
+static errno_t
+sdap_dyndns_next_ptr_record(struct sdap_dyndns_update_state *state,
+ struct tevent_req *req);
struct tevent_req *
sdap_dyndns_update_send(TALLOC_CTX *mem_ctx,
@@ -106,6 +111,8 @@ sdap_dyndns_update_send(TALLOC_CTX *mem_ctx,
state->ev = ev;
state->opts = opts;
state->auth_type = auth_type;
+ state->ptr_addr_iter = NULL;
+ state->del_phase = true;
/* fallback servername is overriden by user option */
conf_servername = dp_opt_get_string(opts, DP_OPT_DYNDNS_SERVER);
@@ -381,6 +388,15 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
}
talloc_free(state->update_msg);
+
+ /* init iterator for addresses to be deleted */
+ state->ptr_addr_iter = state->dns_addrlist;
+ if (state->ptr_addr_iter == NULL) {
+ /* init iterator for addresses to be added */
+ state->del_phase = false;
+ state->ptr_addr_iter = state->addresses;
+ }
+
ret = sdap_dyndns_update_ptr_step(req);
if (ret != EOK) {
tevent_req_error(req, ret);
@@ -396,6 +412,7 @@ sdap_dyndns_update_ptr_step(struct tevent_req *req)
struct sdap_dyndns_update_state *state;
const char *servername;
struct tevent_req *subreq;
+ struct sockaddr_storage *item;
It's not an 'item', it's an address (The function is misnamed, too)
state = tevent_req_data(req, struct sdap_dyndns_update_state);
@@ -405,10 +422,21 @@ sdap_dyndns_update_ptr_step(struct tevent_req *req)
servername = state->servername;
}
- ret = be_nsupdate_create_ptr_msg(state, state->realm,
- servername, state->hostname,
- state->ttl, state->remove_af,
- state->addresses, state->dns_addrlist,
+ if (state->del_phase) {
+ /* add 'del' addresses */
+ item = sss_iface_addr_get_item(state->ptr_addr_iter);
+ } else {
+ /* add 'add' addresses */
+ item = sss_iface_addr_get_item(state->ptr_addr_iter);
+ }
+
+ if (item == NULL) {
+ return EIO;
+ }
+
+ ret = be_nsupdate_create_ptr_msg(state, state->realm, servername,
+ state->hostname, state->ttl,
+ state->remove_af, item, state->del_phase,
&state->update_msg);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "Can't get addresses for DNS update\n");
@@ -454,10 +482,20 @@ sdap_dyndns_update_ptr_done(struct tevent_req *subreq)
}
}
+ ret = sdap_dyndns_next_ptr_record(state, req);
+ if (ret == EAGAIN) {
+ return;
+ }
+
tevent_req_error(req, ret);
return;
}
+ ret = sdap_dyndns_next_ptr_record(state, req);
+ if (ret == EAGAIN) {
+ return;
+ }
+
tevent_req_done(req);
}
@@ -468,6 +506,37 @@ sdap_dyndns_update_recv(struct tevent_req *req)
return EOK;
}
+static errno_t
+sdap_dyndns_next_ptr_record(struct sdap_dyndns_update_state *state,
+ struct tevent_req *req)
+{
This function is still out of the request..no tevent request function
should be outside _send() and _recv() pair.