[SSSD] [PATCHES] Various view related improvements

Jakub Hrozek jhrozek at redhat.com
Fri Jan 9 13:46:27 UTC 2015


On Wed, Dec 17, 2014 at 11:47:59AM +0100, Sumit Bose wrote:
> On Wed, Dec 17, 2014 at 09:33:32AM +0100, Lukas Slebodnik wrote:
> > On (17/12/14 09:31), Lukas Slebodnik wrote:
> > >On (11/12/14 11:10), Sumit Bose wrote:
> > >>Hi,
> > >>
> > >>this patch set contains some improvements for the view related code on
> > >>IPA client. Most of them are performance related, patches 4,5 and 6 make
> > >>sure that during group requests all members are properly resovled if a
> > >>non-default view is applied to be able to handle user name overrides.
> > >>
> > >>bye,
> > >>Sumit
> > >
> > >>From c6dcdb109a1d6daa8f91b197a29f4551d9de8d8a Mon Sep 17 00:00:00 2001
> > >>From: Sumit Bose <sbose at redhat.com>
> > >>Date: Fri, 5 Dec 2014 11:11:49 +0100
> > >>Subject: [PATCH 4/7] IPA: process_members() optionally return missing members
> > >> list
> > >>
> > >>---
> > >> src/providers/ipa/ipa_s2n_exop.c | 85 +++++++++++++++++++++++++++++++---------
> > >> 1 file changed, 67 insertions(+), 18 deletions(-)
> > >>
> > >There is a warning from static analyzer. Currently it is false possitive and
> > >it isn't big chance to do such mistake, but small change in code can fix it.
> > >See html report: left_operand_is_garbage.html.gz
> > >
> > And one more time with attachment :-)
> 
> Thank you, fixed version attached.
> 
> bye,
> Sumit
> 
> > 
> > LS
> 
> 
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 

The patches are mostly good, but I see some problems even without them,
so maybe it makes sense to discuss them anyway.

When the cache is empty and I run "id -G $user", the groups are saved to
the cache with DNs looking like this:
    name=unigroup at AD.EXAMPLE.COM,cn=groups,cn=AD.EXAMPLE.COM,cn=sysdb

But later when adding the user to groups, the member-add function looks
for:
    name=unigroup at ad.example.com,cn=groups,cn=AD.EXAMPLE.COM,cn=sysdb

Looks like there is a casing mismatch...I know we made originalDN
case-insensitive but most probably not the sysdb DN. The full log
message is:
    (Fri Jan  9 14:25:19 2015) [sssd[be[ipa.example.com]]] [sysdb_update_members_ex] (0x0020): Could not add member [psuser at AD.EXAMPLE.COM] to group [name=unigroup at ad.example.com,cn=groups,cn=AD.EXAMPLE.COM,cn=sysdb]. Skipping.


Some comments on the patches inline..

> From 3911cf48ca4f0788476ce8fb50c175dbd0d83e4f Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Thu, 4 Dec 2014 13:26:32 +0100
> Subject: [PATCH 1/7] IPA: do not look up overrides on client with default view
> 
> The IPA extdom plugin returns the data with the default view already
> applied hence it is on needed to look up the override data if the client
> has the default view assigned.

ACK

> From affa294d04ed7fef4c1d9cfd92098f22f91600ea Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 5 Dec 2014 11:03:48 +0100
> Subject: [PATCH 2/7] IPA: make version check more precise
> 
> The call protected by the check does not only expect the version 1 of
> the extdom plugin is used but a specific response type as well. Since
> version 1 can return older response types as well we want to be on the
> safe side.

ACK

> From 37cc08b7d2a678f31191794e653aa89202148962 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 5 Dec 2014 11:06:26 +0100
> Subject: [PATCH 3/7] IPA: add missing break
> 
> The current request already returned the SID, we do not need to request
> it separately.

ACK

> From 6f0f58448b86052c848b78151dbc39b0d3c99737 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 5 Dec 2014 11:11:49 +0100
> Subject: [PATCH 4/7] IPA: process_members() optionally return missing members
>  list
> 
> ---
>  src/providers/ipa/ipa_s2n_exop.c | 84 +++++++++++++++++++++++++++++++---------
>  1 file changed, 66 insertions(+), 18 deletions(-)
> 
> diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
> index 6bf492623f638a9900f997a937e05cd66fca09a2..3b02e67e36caa8ee05a702dc3434c965ecee4682 100644
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -1222,7 +1222,8 @@ fail:
>  
>  static errno_t process_members(struct sss_domain_info *domain,
>                                 struct sysdb_attrs *group_attrs,
> -                               char **members)
> +                               char **members,
> +                               TALLOC_CTX *mem_ctx, char ***_missing_members)
>  {
>      int ret;
>      size_t c;
> @@ -1231,6 +1232,8 @@ static errno_t process_members(struct sss_domain_info *domain,
>      const char *dn_str;
>      struct sss_domain_info *obj_domain;
>      struct sss_domain_info *parent_domain;
> +    char **missing_members = NULL;
> +    size_t miss_count = 0;
>  
>      if (members == NULL) {
>          DEBUG(SSSDBG_TRACE_INTERNAL, "No members\n");

I think you should also set *_missing_members to NULL if there are no
members.

> From c111a183e3ceda85781bf3383a869bf8d8e555d9 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Tue, 9 Dec 2014 17:04:30 +0100
> Subject: [PATCH 5/7] IPA: rename ipa_s2n_get_groups_send() to
>  ipa_s2n_get_fqlist_send()
> 
> ---
>  src/providers/ipa/ipa_s2n_exop.c | 100 +++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 50 deletions(-)
> 
> diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
> index 3b02e67e36caa8ee05a702dc3434c965ecee4682..f3a6aa99408d2df3448a9642ef99b5f2b7082248 100644
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -877,38 +877,38 @@ done:
>      return ret;
>  }
>  
> -struct ipa_s2n_get_groups_state {
> +struct ipa_s2n_get_fqlist_state {
>      struct tevent_context *ev;
>      struct ipa_id_ctx *ipa_ctx;
>      struct sss_domain_info *dom;
>      struct sdap_handle *sh;
>      struct req_input req_input;
> -    char **group_list;
> -    size_t group_idx;
> +    char **fqname_list;
> +    size_t fqname_idx;
>      int exop_timeout;
>      struct resp_attrs *attrs;
>      struct sss_domain_info *obj_domain;
>      struct sysdb_attrs *override_attrs;
>  };
>  
> -static errno_t ipa_s2n_get_groups_step(struct tevent_req *req);
> -static void ipa_s2n_get_groups_get_override_done(struct tevent_req *subreq);
> -static void ipa_s2n_get_groups_next(struct tevent_req *subreq);
> -static errno_t ipa_s2n_get_groups_save_step(struct tevent_req *req);
> +static errno_t ipa_s2n_get_fqlist_step(struct tevent_req *req);
> +static void ipa_s2n_get_fqlist_get_override_done(struct tevent_req *subreq);
> +static void ipa_s2n_get_fqlist_next(struct tevent_req *subreq);
> +static errno_t ipa_s2n_get_fqlist_save_step(struct tevent_req *req);
>  
> -static struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX *mem_ctx,
> +static struct tevent_req *ipa_s2n_get_fqlist_send(TALLOC_CTX *mem_ctx,
>                                                    struct tevent_context *ev,
>                                                    struct ipa_id_ctx *ipa_ctx,
>                                                    struct sss_domain_info *dom,
>                                                    struct sdap_handle *sh,
>                                                    int exop_timeout,
> -                                                  char **group_list)
> +                                                  char **fqname_list)
>  {
>      int ret;
> -    struct ipa_s2n_get_groups_state *state;
> +    struct ipa_s2n_get_fqlist_state *state;
>      struct tevent_req *req;
>  
> -    req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_groups_state);
> +    req = tevent_req_create(mem_ctx, &state, struct ipa_s2n_get_fqlist_state);
>      if (req == NULL) {
>          return NULL;
>      }
> @@ -917,17 +917,17 @@ static struct tevent_req *ipa_s2n_get_groups_send(TALLOC_CTX *mem_ctx,
>      state->ipa_ctx = ipa_ctx;
>      state->dom = dom;
>      state->sh = sh;
> -    state->group_list = group_list;
> -    state->group_idx = 0;
> +    state->fqname_list = fqname_list;
> +    state->fqname_idx = 0;
>      state->req_input.type = REQ_INP_NAME;
>      state->req_input.inp.name = NULL;
>      state->exop_timeout = exop_timeout;
>      state->attrs = NULL;
>      state->override_attrs = NULL;
>  
> -    ret = ipa_s2n_get_groups_step(req);
> +    ret = ipa_s2n_get_fqlist_step(req);
>      if (ret != EOK) {
> -        DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_groups_step failed.\n");
> +        DEBUG(SSSDBG_OP_FAILURE, "ipa_s2n_get_fqlist_step failed.\n");
>          goto done;
>      }
>  
> @@ -940,25 +940,25 @@ done:
>      return req;
>  }
>  
> -static errno_t ipa_s2n_get_groups_step(struct tevent_req *req)
> +static errno_t ipa_s2n_get_fqlist_step(struct tevent_req *req)
>  {
>      int ret;
> -    struct ipa_s2n_get_groups_state *state = tevent_req_data(req,
> -                                               struct ipa_s2n_get_groups_state);
> +    struct ipa_s2n_get_fqlist_state *state = tevent_req_data(req,
> +                                               struct ipa_s2n_get_fqlist_state);
>      struct berval *bv_req;
>      struct tevent_req *subreq;
>      struct sss_domain_info *parent_domain;
> -    char *group_name = NULL;
> +    char *fqname = NULL;
>      char *domain_name = NULL;
>  
>      parent_domain = get_domains_head(state->dom);
>  
>      ret = sss_parse_name(state, parent_domain->names,
> -                         state->group_list[state->group_idx],
> -                         &domain_name, &group_name);
> +                         state->fqname_list[state->fqname_idx],
> +                         &domain_name, &fqname);

Shouldn't this variable be named just 'name'? The fqname is just being
parsed into (name,domain), right?

> From 0d742401943698789deb2dd56159713717d56a1d Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 5 Dec 2014 11:12:42 +0100
> Subject: [PATCH 6/7] IPA: resolve missing members

I couldn't do much testing b/c of the problems described earlier, but is
it possible there would be some nested group members that would require
BE_RE_GROUP also?

> +            if (missing_list != NULL) {
> +                subreq = ipa_s2n_get_fqlist_send(state, state->ev,
> +                                                 state->ipa_ctx, state->dom,
> +                                                 state->sh, state->exop_timeout,
> +                                                 BE_REQ_USER,
> +                                                 REQ_FULL_WITH_MEMBERS,
> +                                                 missing_list);
>                  if (subreq == NULL) {
>                      DEBUG(SSSDBG_OP_FAILURE,
>                            "ipa_s2n_get_fqlist_send failed.\n");

> From 6b7ab672002e3c7f60b2284f679415acc0924388 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Tue, 9 Dec 2014 17:48:46 +0100
> Subject: [PATCH 7/7] IPA: set SYSDB_INITGR_EXPIRE for RESP_USER_GROUPLIST
> 
> Since RESP_USER_GROUPLIST contains all group memberships it is
> effectively an initgroups request hence SYSDB_INITGR_EXPIRE will be set.

ACK



More information about the sssd-devel mailing list