[SSSD] [PATCH] refactor nested group processing

Jakub Hrozek jhrozek at redhat.com
Wed Mar 27 12:44:42 UTC 2013


On Wed, Mar 27, 2013 at 11:03:46AM +0100, Lukas Slebodnik wrote:
> On (25/03/13 14:54), Jakub Hrozek wrote:
> >On Mon, Mar 25, 2013 at 02:48:44PM +0100, Pavel Březina wrote:
> >> On 03/22/2013 10:59 PM, Lukas Slebodnik wrote:
> >> >On (21/03/13 16:10), Pavel Březina wrote:
> >> >>On 03/21/2013 03:43 PM, Lukas Slebodnik wrote:
> >> >>>On (22/02/13 13:46), Pavel Brezina wrote:
> >> >>>>----- Original Message -----
> >> >>>>>From: "Pavel Březina" <pbrezina at redhat.com>
> >> >>>>>To: sssd-devel at lists.fedorahosted.org
> >> >>>>>Sent: Thursday, February 21, 2013 11:43:26 AM
> >> >>>>>Subject: [SSSD] [PATCH] refactor nested group processing
> >> >>>>>
> >> >>>>>https://fedorahosted.org/sssd/ticket/1784
> >> >>>>
> >> >>>>I should have probably add that this is the final version. I will attach unit test once I get familiar with cmocka, but it does not really prevent reviewing the patch.
> >> >>>
> >> >>>I would like to test your patches, but the second patch no longer applies
> >> >>>on the current master. Could you resend rebased patches?
> >> >>
> >> >>Thanks. Rebased patches are attached. I did not test it again though,
> >> >>but it compiles fine and the diff looks all right. It may be
> >> >>interesting to determine what patch broke it.
> >> >>
> >> >
> >> >I had problems with you patches.
> >> >The simplest cycle schema:
> >> >    ________
> >> >    |  CG1 |------------>sssduser1
> >> >    |______|<---|
> >> >       |        |
> >> >       |        |
> >> >       |     ________
> >> >       |---->|  CG2 |--->sssduser2
> >> >             |______|
> >> >
> >> >sssd client configured with ipa-client-install and two additional options
> >> >entry_cache_timeout = 5
> >> >memcache_timeout = 1
> >> >
> >> >How to reproduce.
> >> >1. remove cache(also logs, but it is not important)
> >> >2. service sssd start
> >> >3. run commands
> >> >
> >> >sh-4.2$ getent group cyclegroup1
> >> >cyclegroup1:*:30007:sssduser1,sssduser2
> >> >sh-4.2$ getent group cyclegroup2
> >> >cyclegroup2:*:30008:sssduser2
> >> >  #after second (memcache_timeout)
> >> >sh-4.2$ getent group cyclegroup2
> >> >cyclegroup2:*:30008:sssduser2,sssduser1
> >> >
> >> >-----------------------------
> >> >
> >> >Also sometimes with the same groups scheme one user appeared two times,
> >> >but I could not exactly reproduce steps.
> >> 
> >> The problem is even in master. Seems to be another ghost users issue.
> >> 
> >
> >I agree, the EEXIST error is coming from memberof plugin:
> >https://fedorahosted.org/sssd/ticket/1846
> >
> >> >
> >> >If you could not reproduce problem, I could send you off the list
> >> >logs, cache, any configurations, rpms or even provide access to IPA server.
> >> >
> >> >And last notice, commit 4709ff46db0dbe073aef061b796d2fd7adeaf18f
> >> >with title "LDAP: If deref search fails, try again without deref"
> >> >cause little problem with applying patches.
> >> >
> >> >LS
> >> 
> >> I amended my patches to keep the functionality introduced in this
> >> commit. I.e. if deref fails with ENOTSUP, we will try again without
> >> dereference.
> >> 
> 
> I tested functionality of your patches and except bug from ticket
> (https://fedorahosted.org/sssd/ticket/1846) everything work like expected.
> Tested cases:
>     --nonexisting group
>     --empty group
>     --user in group
>     --nested groups
>     --user in more nested group(no duplication)
>     --cycle
>     --deref
>     --nonderef
> 
> I have to say, that I learnd a lot of new things.
> 
> (mini Ack) -> So someone else could look to code.
> I am not yet expert in tevent coding style
> 
> LS

Thanks for testing!

I reviewed the code today and I only have some suggestions given that
Lukas' testing went fine.

In general when debugging the nested group code in the past, I missed
more DEBUG information about the progress (as opposed to just errors).
This is getting better in the attached patch but maybe more could still
be added to the utility functions at the start of the module or
sdap_nested_group_recv() for example.

There could be a simple wrapper macro around
sdap_nested_group_search_cache named _sysdb_search_{users,groups}, that
would be more readable than true|false, similar to how you coded up
sdap_nested_group_hash_{user,group}.

step_down is a misleading variable name, recurse would be much better.

In the sdap_nested_group_single_ request, instead of reusing the _done
callback for both direct process and nesting process, can you create
another callback? I think that would be more readable. The same about
sdap_nested_group_deref_done. I don't think the flow should be
controlled by boolean variables that much.

You're right that sdap_nested_group_get_ipa_user() should be set as a
function pointer from IPA backend, but it's unlikely to happen in 1.10.
Please file a ticket about that.

The lookup_uknown request could be split into better steps, too. Instead
of relying on state->type, why not have a callback after check for user,
then check for groups go to another callback and if either of them
succeeds, finish the request. I know there was a similar flow in the
previous version, but there the steps were shared inside one
super-request. With smallish requests in this version, I think it would
be nicer than stricly keeping one callback.



More information about the sssd-devel mailing list