[SSSD] [PATCH] DDNS: execute nsupdate for single update of PTR rec
Jakub Hrozek
jhrozek at redhat.com
Mon Sep 14 15:29:34 UTC 2015
On Sun, Sep 13, 2015 at 01:43:03PM +0200, Pavel Reichl wrote:
> Hello,
>
> this patch works around the bug in nsupdate (https://bugzilla.redhat.com/show_bug.cgi?id=1262430)
>
> thanks!
> From 4ce49023285b9435abcd3b86d88b40151b76b24c Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at 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 | 60 ++++++++++++++++++++++-----------
> src/providers/dp_dyndns.h | 11 +++++--
> src/providers/ldap/sdap_dyndns.c | 71 +++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 120 insertions(+), 22 deletions(-)
>
> diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> index 0577743cb2daca9c0e86b5beb6bf059ee7b5783f..845ab7adf6ec3341d19a0d3b7486dd686b3a903f 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)
> +{
> + 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)
> {
> @@ -294,34 +313,34 @@ nsupdate_msg_add_fwd(char *update_msg, struct sss_iface_addr *addresses,
> }
>
The intent is good, but I think the implementation is too complex. The
caller keeps both add iter and del and then passes both to
nsupdate_msg_add_ptr which has quite duplicated code.
The add and delete blocks in nsupdate_msg_add_ptr now differ only in
uding add or delete in the message, right? Could we simplify that? For
readability's sake, we could even create wrappers like
nsupdate_msg_add_ptr() and +nsupdate_msg_del_ptr() that would call an
internal function with only either add or del.
> static char *
> -nsupdate_msg_add_ptr(char *update_msg, struct sss_iface_addr *addresses,
> +nsupdate_msg_add_ptr(char *update_msg,
> + struct sockaddr_storage *add_address,
> const char *hostname, int ttl, uint8_t remove_af,
> - struct sss_iface_addr *old_addresses)
> + struct sockaddr_storage *del_address)
> {
> - 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) {
> + if (del_address != NULL) {
> + switch(del_address->ss_family) {
> case AF_INET:
> if (!(remove_af & DYNDNS_REMOVE_A)) {
> - continue;
> + return NULL;
> }
> - addr = (uint8_t *) &((struct sockaddr_in *) old_record->addr)->sin_addr;
> + addr = (uint8_t *) &((struct sockaddr_in *) del_address)->sin_addr;
> break;
> case AF_INET6:
> if (!(remove_af & DYNDNS_REMOVE_AAAA)) {
> - continue;
> + return NULL;
> }
> - addr = (uint8_t *) &((struct sockaddr_in6 *) old_record->addr)->sin6_addr;
> + addr = (uint8_t *) &((struct sockaddr_in6 *) del_address)->sin6_addr;
> break;
> default:
> DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
> return NULL;
> }
>
> - strptr = resolv_get_string_ptr_address(update_msg, old_record->addr->ss_family,
> + strptr = resolv_get_string_ptr_address(update_msg, del_address->ss_family,
> addr);
> if (strptr == NULL) {
> return NULL;
> @@ -336,23 +355,25 @@ nsupdate_msg_add_ptr(char *update_msg, struct sss_iface_addr *addresses,
> if (update_msg == NULL) {
> return NULL;
> }
> +
> + return update_msg;
> }
>
> /* 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) {
> + if (add_address != NULL) {
> + switch(add_address->ss_family) {
> case AF_INET:
> - addr = (uint8_t *) &((struct sockaddr_in *) new_record->addr)->sin_addr;
> + addr = (uint8_t *) &((struct sockaddr_in *) add_address)->sin_addr;
> break;
> case AF_INET6:
> - addr = (uint8_t *) &((struct sockaddr_in6 *) new_record->addr)->sin6_addr;
> + addr = (uint8_t *) &((struct sockaddr_in6 *) add_address)->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,
> + strptr = resolv_get_string_ptr_address(update_msg, add_address->ss_family,
> addr);
> if (strptr == NULL) {
> return NULL;
> @@ -469,11 +490,12 @@ done:
> }
>
> errno_t
> +
Extra line.
> 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 *add_address,
> + struct sockaddr_storage *del_address,
> char **_update_msg)
> {
> errno_t ret;
> @@ -490,8 +512,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, add_address, hostname,
> + ttl, remove_af, del_address);
> if (update_msg == NULL) {
> ret = ENOMEM;
> goto done;
> diff --git a/src/providers/dp_dyndns.h b/src/providers/dp_dyndns.h
> index 9f72331b6fd68e17e9eb91505a13fc839d3f54e1..1bbe806354c2d8b57fe323703d5d993ac0f6dc0f 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 *add_address,
> + struct sockaddr_storage *del_address,
> 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);
When we agree on the rest, please add tests for these two functions. I
know they are trivial, but we should keep adding tests.
> +
> #endif /* DP_DYNDNS_H_ */
> diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
> index 2a179fd1b5e88bdf2442657ff6fa1dcc55417467..670a9e82e001dcb22c536013cfaa345684b86bc1 100644
> --- a/src/providers/ldap/sdap_dyndns.c
> +++ b/src/providers/ldap/sdap_dyndns.c
> @@ -60,6 +60,9 @@ struct sdap_dyndns_update_state {
> enum be_nsupdate_auth auth_type;
> bool fallback_mode;
> char *update_msg;
> + struct sss_iface_addr *ptr_add_addr_iter;
> + struct sss_iface_addr *ptr_del_addr_iter;
> + bool del_phase;
Do we need both iterators /and/ a flag? Can't we get away with either a
single iterator and a flag or two iterators?
> };
>
> static void sdap_dyndns_update_addrs_done(struct tevent_req *subreq);
> @@ -70,6 +73,8 @@ 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 bool 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,9 @@ sdap_dyndns_update_send(TALLOC_CTX *mem_ctx,
> state->ev = ev;
> state->opts = opts;
> state->auth_type = auth_type;
> + state->ptr_add_addr_iter = NULL;
> + state->ptr_del_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 +389,15 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
> }
>
> talloc_free(state->update_msg);
> +
> + /* init iterator for addresses to be deleted */
> + state->ptr_del_addr_iter = state->dns_addrlist;
> + if (state->ptr_del_addr_iter == NULL) {
> + /* init iterator for addresses to be added */
> + state->del_phase = false;
> + state->ptr_add_addr_iter = state->addresses;
> + }
> +
> ret = sdap_dyndns_update_ptr_step(req);
> if (ret != EOK) {
> tevent_req_error(req, ret);
> @@ -396,6 +413,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 *add_item, *del_item;
Similarly here, we can only be adding /or/ deleting, so we could use
just one variable, right?
>
> state = tevent_req_data(req, struct sdap_dyndns_update_state);
>
> @@ -405,10 +423,28 @@ sdap_dyndns_update_ptr_step(struct tevent_req *req)
> servername = state->servername;
> }
>
> + del_item = NULL;
> + add_item = NULL;
> +
> + if (state->del_phase) {
> + /* add 'del' addresses */
> + del_item = sss_iface_addr_get_item(state->ptr_del_addr_iter);
> + if (del_item == NULL) {
> + return EIO;
> + }
> +
> + } else {
> + /* add 'add' addresses */
> + add_item = sss_iface_addr_get_item(state->ptr_add_addr_iter);
> + if (add_item == NULL) {
> + return EIO;
> + }
> + }
> +
> ret = be_nsupdate_create_ptr_msg(state, state->realm,
> servername, state->hostname,
> state->ttl, state->remove_af,
> - state->addresses, state->dns_addrlist,
> + add_item, del_item,
> &state->update_msg);
> if (ret != EOK) {
> DEBUG(SSSDBG_OP_FAILURE, "Can't get addresses for DNS update\n");
> @@ -454,10 +490,14 @@ sdap_dyndns_update_ptr_done(struct tevent_req *subreq)
> }
> }
>
> + if (sdap_dyndns_next_ptr_record(state, req)) return;
> +
> tevent_req_error(req, ret);
> return;
> }
>
> + if (sdap_dyndns_next_ptr_record(state, req)) return;
> +
> tevent_req_done(req);
> }
>
> @@ -801,3 +841,32 @@ sdap_dyndns_timer_conn_recv(struct tevent_req *req)
> TEVENT_REQ_RETURN_ON_ERROR(req);
> return EOK;
> }
> +
> +static bool sdap_dyndns_next_ptr_record(struct sdap_dyndns_update_state *state,
> + struct tevent_req *req)
If you add a new function to an existing record, please add it below the
first function that calls it. The tevent requests should start with
_send, continue through _recv and be readable from top to bottom without
jumping around.
I would also prefer if we returned errno here instead of bool, with
EAGAIN if we continue with next step.
> +{
> + if (state->del_phase) {
> + /* iterate to next address to delete */
> + state->ptr_del_addr_iter = sss_iface_addr_get_next(state->ptr_del_addr_iter);
> + if (state->ptr_del_addr_iter == NULL) {
> + /* init iterator for addresses to be added */
> + state->del_phase = false;
> + state->ptr_add_addr_iter = state->addresses;
> + }
> + } else {
> + /* iterate to next address to add */
> + state->ptr_add_addr_iter = sss_iface_addr_get_next(state->ptr_add_addr_iter);
> + }
> +
> + if (state->ptr_add_addr_iter != NULL || state->ptr_del_addr_iter != NULL) {
> + errno_t ret;
> +
> + state->fallback_mode = false;
> + ret = sdap_dyndns_update_ptr_step(req);
> + if (ret == EOK) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> --
> 2.4.3
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list