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". > + </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: > + 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? 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) { > + 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. > + */ > + *base_attr = talloc_strdup(mem_ctx, attr_desc); > + if (!*base_attr) { > + ret = ENOMEM; > + } else { > + ret = ECANCELED; > + } > + goto done; > } > > /* Get the end of the range */ _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel