[SSSD] [PATCHES] Performance enhancements for AD provider

Jakub Hrozek jhrozek at redhat.com
Fri Sep 21 12:49:27 UTC 2012


On Fri, Sep 21, 2012 at 08:03:17AM -0400, Stephen Gallagher wrote:
> On 09/21/2012 05:41 AM, Jakub Hrozek wrote:
> > On Thu, Sep 20, 2012 at 09:34:37AM -0400, Stephen Gallagher wrote:
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >> 
> >> * [PATCH 1/2] AD: Detect domain controller compatibility version
> >>  This patch allows us to read the domain controller version from
> >>  the RootDSE, if it exists. This will be used by the next patch
> >> to determine if the SSSD can use tokenGroups for lookups.
> >> 
> >> * [PATCH 2/2] AD: Optimize initgroups lookups with tokenGroups
> >> If we are doing ID-mapped lookups, we can take advantage of the 
> >> AD-specific option "tokenGroups", which allows us to retrieve a 
> >> pre-flattened list of all the group SIDs for which this user is a
> >> member. From this, we can convert them to GIDs and conclude the
> >> initgroups() lookup with only two LDAP queries (the search for
> >> the user and the base search for the user's tokengroups; the 
> >> latter must be performed as a base search due to restrictions 
> >> imposed on the tokenGroups attribute by AD).
> >> 
> >> When we save these groups to the sysdb, if the group is being 
> >> seen for the first time it will be temporarily given the name of 
> >> the SID representing it. Later, when we look it up by name or 
> >> GID, this group will be replaced with the real one.
> >> 
> > 
> > I only have one question:
> > 
> > +    /* Get the list of group SIDs */ +    ret = 
> > sysdb_attrs_get_el_ext(users[0], AD_TOKENGROUPS_ATTR, + false, 
> > &el); +    if (ret != EOK) { +        if (ret == ENOENT) { + 
> > DEBUG(SSSDBG_TRACE_LIBS, +                  ("No tokenGroups 
> > entries for [%s]\n", +                   state->username));
> > 
> > Should we just return success here? Does a missing togenGroup
> > imply a user that does not belong to any group?
> > 
> > +        } +        DEBUG(SSSDBG_MINOR_FAILURE, + ("Could not read
> >  tokenGroups attribute: [%s]\n", + strerror(ret))); +        goto 
> > done; +    }
> > 
> 
> You're right, I meant to do more with that IF block and forgot about
> it (though in reality, that's a branch we should never take since all
> users are members of at least the built-in "Users" group that we later
> filter out).
> 
> Just returning success here isn't sufficient, though. We need to make
> sure that the sysdb matches. The attached patch creates a new
> ldb_message_element with zero members that will short-circuit the
> group-processing loop below and advance on to where we correct the
> group memberships.
> 
> Additionally, while fixing this I realized that I had forgotten to use
> the _recv() function in sdap_async_initgroups.c. It had been working
> by lucky coincidence because the _recv() function it was calling
> instead happened to have the exact same implementation (checking only
> for errors, not requiring a specific state variable). This is now also
> corrected.
> 

Oops, I missed that, I did take a look that the new _recv() function is
the same as the other becaue initially it struck me that it doesn't
return anything. I didn't check it gets called correctly, though..

> Thanks for the review!
> 
> 

Ack to both patches.



More information about the sssd-devel mailing list