[SSSD] [PATCH] refactor nested group processing

Jakub Hrozek jhrozek at redhat.com
Tue Apr 2 12:47:47 UTC 2013


On Fri, Mar 29, 2013 at 02:47:46PM +0100, Pavel Březina wrote:
> On 03/27/2013 01:44 PM, Jakub Hrozek wrote:
> >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.
> 
> Done.
> 
> >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}.
> 
> Done.
> 
> >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.
> 
> Done.
> 
> >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.
> 
> Done.
> 
> I like it more how it was before though. We called tevent_req_done()
> only from one place which was nice and state->type has to be set
> correctly anyway.
> 

Yes, but the current flow is more readable, you can follow the code of
the request from top to bottom and it's always clear what happens next.

The code now looks good to me and passed my tests (which were by no
means as complete as Lukas' testing). I'm still not quite sure about the
flow of the sdap_nested_group_single_* request but it's a big
improvement over the previous state.

We can do more refactoring later if we see fit, right now I don't want
to hold off the patch any longer.

Ack.



More information about the sssd-devel mailing list