[SSSD] [PATCH] refactor nested group processing

Pavel Březina pbrezina at redhat.com
Fri Mar 29 13:47:46 UTC 2013


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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-refactor-nested-group-processing-add-new-code.patch
Type: text/x-patch
Size: 72850 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130329/8144f4ca/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-refactor-nested-group-processing-replace-old-code.patch
Type: text/x-patch
Size: 61387 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130329/8144f4ca/attachment-0001.bin>


More information about the sssd-devel mailing list