On Wed, May 15, 2013 at 05:35:34PM +0200, Lukas Slebodnik wrote: > On (15/05/13 17:30), Lukas Slebodnik wrote: > >On (15/05/13 13:54), Jakub Hrozek wrote: > >>On Tue, May 14, 2013 at 07:04:42PM +0200, Lukas Slebodnik wrote: > >>> On (14/05/13 16:04), Jakub Hrozek wrote: > >>> >On Mon, May 13, 2013 at 05:08:44PM +0200, Lukas Slebodnik wrote: > >>> >> ehlo, > >>> >> > >>> >> Jakub, Pavel and me disscus about ticket > >>> >> https://fedorahosted.org/sssd/ticket/1823 > >>> >> "getgrnam / getgrgid for large user groups is too slow due to range > >>> >> retrieval > >>> >> functionality" > >>> >> We decided to add new option to turn the range retrieval with AD. > >>> >> Another > >>> >> solution could be to add new option to limit the number of group > >>> >> members > >>> >> processed. > >>> >> > >>> >> Any comments are welcomed. > >>> >> > >>> >> This commit adds new option ldap_disable_large_groups with default > >>> >> value FALSE. > >>> >> If this option is enabled, large groups(>1500) will not be retrieved > >>> >> and > >>> >> behaviour will be similar like was before commit ae8d047122c > >>> >> "LDAP: Handle very large Active Directory groups" > >>> >> > >>> >> LS > >>> > > >>> >This is a good start, but I would prefer if the option name included > >>> >"range retrieval", for two reasons: > >>> > * the current option name would make the user think that the option > >>> > works with every provider > >>> > * in general, the range extension might not be limited to group > >>> > objects on the AD side. > >>> > > >>> >What about "ldap_disable_range_retrieval" ? > >>> > > >>> Option renamed. > >>> > >>> >Similarly the parameter of sdap_parse_entry should be renamed, > >>> >sdap_parse_entry can be used to parse any generic entry, not just group > >>> >(it should have no knowledge about existence of groups). > >>> > > >>> >I was also wondering if the check could be moved all the way to > >>> >sdap_parse_range() to keep the knowledge about range retrievals there. > >>> >Maybe introduce a new error code (or use something from errno) that would > >>> >be returned when the feature is off. If this is not possible because it > >>> >might break the flow of sdap_parse_entry(), then please #define the > >>> >";range" string and put it into sdap_range.h. > >>> > > >>> >Can you also send a follow-up patch that removes the functions > >>> >sdap_parse_user() and sdap_parse_group(). Looks like dead code. > >>> > > >>> functions removed in first patch. > >>> > >>> >> --- a/src/man/sssd-ldap.5.xml > >>> >> +++ b/src/man/sssd-ldap.5.xml > >>> >> @@ -1201,6 +1201,25 @@ > >>> >> </varlistentry> > >>> >> > >>> >> <varlistentry> > >>> >> + <term>ldap_disable_large_groups (boolean)</term> > >>> >> + <listitem> > >>> >> + <para> > >>> >> + Disable very large Active Directory > >>> >> groups. > >>> >> + </para> > >>> >> + <para> > >>> >> + This option has effect only, if LDAP > >>> >> server is > >>> >> + Active Directory > >>> > > >>> > what about "...if SSSD is configured as an Active Directory > >>> > client..." ? > >>> > > >>> Changed. > >>> > >>> Updated patches are attached. > >>> > >>> LS > >> > >>[PATCH 1/2] Removing unused functions. > >>Ack > >> > >>[PATCH 2/2] Adding option to disable retrieving large AD groups. > >> > >>> index > >>> 799213300857f182b092287741a40665a0ae62d5..266944908d12a64d439c59a28d60ab9cea5b4b1f > >>> 100644 > >>> --- a/src/man/sssd-ldap.5.xml > >>> +++ b/src/man/sssd-ldap.5.xml > >>> @@ -1201,6 +1201,27 @@ > >>> </varlistentry> > >>> > >>> <varlistentry> > >>> + <term>ldap_disable_range_retrieval (boolean)</term> > >>> + <listitem> > >>> + <para> > >>> + Disable Active Directory range retrieval. > >>> + </para> > >>> + <para> > >>> + This option has effect only, if SSSD is > >>> configured > >>> + as an Active Directory client. > >>> + Active Directory 2008R2 allows only 1500 > >>> group > >>> + members to be retrieved in single lookup. > >>> Therefore > >>> + if this option is enabled, members of larger > >>> groups > >>> + will not be retrieved. 1500 is default value > >>> of > >>> + MaxValRange policy. > >> > >>One more request to change the man page - can we say something like: > >> > >>"Active Directory limits the number of members to be retrieved in a > >>single lookup using the MaxValRange policy (which defaults to 1500 > >>members). If a group contains more members, the reply would include an > >>AD-specific range extension. This option disables parsing of the > >>range extension, therefore large groups will appear as having no > >>members". > >> > >Thank you very much for this paragraph. > > > >>> + </para> > >>> + <para> > >>> + Default: False > >>> + </para> > >>> + </listitem> > >>> + </varlistentry> > >>> + > >>> + <varlistentry> > >>> <term>ldap_sasl_minssf (integer)</term> > >>> <listitem> > >>> <para> > >>> diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h > >>> --- a/src/providers/ldap/sdap.c > >>> +++ b/src/providers/ldap/sdap.c > >>> @@ -95,7 +95,8 @@ int sdap_get_map(TALLOC_CTX *memctx, > >>> int sdap_parse_entry(TALLOC_CTX *memctx, > >>> struct sdap_handle *sh, struct sdap_msg *sm, > >>> struct sdap_attr_map *map, int attrs_num, > >>> - struct sysdb_attrs **_attrs, char **_dn) > >>> + struct sysdb_attrs **_attrs, char **_dn, > >>> + bool disable_range_retrieval) > >>> { > >>> struct sysdb_attrs *attrs; > >>> BerElement *ber = NULL; > >>> @@ -190,23 +191,25 @@ int sdap_parse_entry(TALLOC_CTX *memctx, > >>> while (str) { > >>> base64 = false; > >>> > >> > >>> - ret = sdap_parse_range(tmp_ctx, str, &base_attr, &range_offset); > >>> - if (ret == EAGAIN) { > >>> + ret = sdap_parse_range(tmp_ctx, str, &base_attr, &range_offset, > >>> + disable_range_retrieval); > >>> + switch(ret) { > >>> + case EAGAIN: > >>> /* This attribute contained range values and needs more to > >>> * be retrieved > >>> */ > >>> /* TODO: return the set of attributes that need additional > >>> retrieval > >>> * For now, we'll continue below and treat it as regular > >>> values. > >>> */ > >>> - > >>> - } else if (ret != EOK) { > >>> + case ECANCELED: > >I added comment /* FALLTHROUGH */ > >>> + case EOK: > >>> + break; > >>> + default: > >>> DEBUG(SSSDBG_MINOR_FAILURE, > >>> - ("Could not determine if attribute [%s] was ranged\n", > >>> - str)); > >>> + ("Could not determine if attribute [%s] was ranged\n", > >>> str)); > >>> goto done; > >>> } > >>> > >>> - > >>> if (map) { > >> > >>I would prefer if we could skip checking the schema here and just shortcut > >>to next iteration. Maybe use another variable to store the return value > >>of sdap_parse_range() to be defensive in case there was another > >>assignment to "ret" between sdap_parse_range() is called and ECANCELED > >>checked? > >I did not found any simple and readable solution without nested "if > >statments" > >or with complicated if conditions with additional booolean variable. > > > >> > >>But looking at the 200+ lines function, I'm OK with this approach as long > >>as we have a refactoring ticket for sdap_parse_entry. > >> > >>> for (a = 1; a < attrs_num; a++) { > >>> /* check if this attr is valid with the chosen schema */ > >>> @@ -230,6 +233,11 @@ int sdap_parse_entry(TALLOC_CTX *memctx, > >>> store = true; > >>> } > >>> > >>> + if (disable_range_retrieval && ret == ECANCELED) { > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >I removed this part, because ECANCELED is returned only if > >disable_range_retrieval is true; > > > >>> + ret = EOK; > >>> + store = false; > >>> + } > >>> + > >>> if (store) { > >>> vals = ldap_get_values_len(sh->ldap, sm->msg, str); > >>> if (!vals) { > >> > >>> @@ -84,6 +85,17 @@ errno_t sdap_parse_range(TALLOC_CTX *mem_ctx, > >>> ("[%s] contains sub-attribute other than a range, > >>> returning whole\n", > >>> attr_desc)); > >>> goto done; > >>> + } else if (disable_range_retrieval) { > >>> + /* This is range sub-attribute, but we want to ignore it. > >>> + * thing in case it's dealt with elsewhere. > >> > >>This second sentence in the comment doesn't sound like English to me :-) > >>I think we can simply remove it. > >> > >Removed. (copy&paste problem) > > > >>> + */ > >>> + *base_attr = talloc_strdup(mem_ctx, attr_desc); > >>> + if (!*base_attr) { > >>> + ret = ENOMEM; > >>> + } else { > >>> + ret = ECANCELED; > >>> + } > >>> + goto done; > >>> } > >>> > >>> /* Get the end of the range */ > > > >Updated patch attached. > > > >LS > One more time. > I forgot, that I thave to attach two patches. > > LS
I have one last request for the patch before I ack: I think it would be nicer if the sdap_get_generic_state structure contained a pointer to the opts rather than the boolean. Then we could just pass the result of dp_opt_get_bool() to the parsing function and could also use the opts in the future if we extend the parsing again with some other tunable. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
