On 05/09/2011 07:23 PM, Stephen Gallagher wrote:
> Continuing my review. Comments inline.
>
New patches are attached. They are formatted with -M -C --patience
--full-index, per Simo's suggestion. I think it helped, especially with
patch #1.
I'd like to merge the replies into one thread. Below are replies for the
first half of patches.
Patch #1 has no changes.
Patch #2 is a new one that just defines a couple of data structures and
one function that are used in later patches in order to process MPGs.
Patches #3 and #4 (previously #2 and #3) that implement the
platform-specific dereference methods are now able to dereference MPGs,
too. The OpenLDAP patch also removes one unused variable.
If an LDAP object that matches multiple maps is dereferenced, it is
returned as multiple sysdb_attrs * results plus the corresponding map to
speed up processing.
I tested the functionality by dereferencing an object that was both a
posixUser and a posixGroup. The dereference itself worked fine and both
objects were saved to sysdb but it seems that the rest of the LDAP
provider assumes that originalDN is unique -- so later the back end
failed when it was trying to build memberships. This is something to fix
if we want to support MPGs, but I don't think it is related to this
patchset.
Patch #5 (previously #4) now has single sdap_deref_search_done()
function instead of one per method.
The patch that changed append_attrs_to_array was retired. I will send a
separate patch to remove that function altogether as it is not used
anywhere.
Patch #6 has no changes.
>
> On Wed, 2011-05-04 at 13:19 +0200, Jakub Hrozek wrote:
>>
>> [PATCH 7/9] Use fake users during RFC2307bis nested group processing
>> Instead of downloading complete user data which is potentionally very
>> slow, only download the necessary minimum information and store the
>> users as dummy entries.
>>
>
> Nack.
> sdap_nested_group_populate_users() doesn't properly sanitize the origDN
> before using it in a search filter.
>
Fixed.
>> [PATCH 8/9] Refactor RFC2307bis nested group processing
>> This patch splits checking cache and hash tables into standalone
>> functions. This will make it easy to reuse the code in a new branch that
>> uses dereferencing.
>>
>
> Nack.
>
> I realize this was a problem in my original code, but would you mind
> fixing two issues with sdap_nested_group_check_cache() while you're
> there?
>
> 1) Please rearrange the code so that all of the user processing takes
> place before the group processing. It's not terribly readable the way
> the complete group processing happens in the middle. It would be better
> to have the return value from sysdb_search_users() set mtype either to
> SYSDB_MEMBER_USER or SYSDB_MEMBER_GROUP (assumed) and then have another
> IF statement after that so we can do all of the user processing in
> series.
>
> 2) It's kind of wasteful to allocate filter twice in the case of groups.
> Better would be to hang onto it until we're sure we're not processing a
> group. Then we don't need to allocate and free the exact same filter
> twice.
>
> Please change the comment in the ENOENT block of
> sdap_nested_group_process_step() since it no longer makes sense with the
> user and group split in a sub-function.
>
>
Fixed
>
>> [PATCH 9/9] Use dereference in processing RFC2307bis nested groups
>> Instead of issuing N LDAP requests when processing a group with N users,
>> utilize the dereference functionality to pull down all the members in a
>> single LDAP request.
>>
>>
https://fedorahosted.org/sssd/ticket/799
>
> Nack.
>
> In sdap_nested_group_process_deref_step(), if
> sdap_nested_group_check_cache() returns something other than EOK, ENOENT
> or EAGAIN, it should be returned as an error. Right now, you're treating
> all errors as ENOENT.
>
Fixed
> Regarding the tuning for when to perform the deref, I don't have a
> perfect answer either. My only suggestion would be to make the value
> configurable in sssd.conf. I think we should get rid of the percentage
> and make it purely based on number of members. The way to tune it should
> be that we should deref whenever a deref will take less time to process
> than N members. I think we should default to ten members.
>
Making it configurable is a good answer :-) and I agree that the option
should only specify absolute values.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Ack
Patch 0005: Ack
Patch 0006: Ack
Patch 0007: Ack
Patch 0008: Ack
Patch 0009: Nack. Please add ldap_deref_threshold to sssd-ipa.conf (not
just sssd-ldap.conf.