[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