[SSSD] [PATCH] Adding option to disable retrieving large AD groups.

Jakub Hrozek jhrozek at redhat.com
Thu May 23 09:42:30 UTC 2013


On Tue, May 21, 2013 at 04:30:17PM +0200, Lukas Slebodnik wrote:
> On (16/05/13 19:47), Jakub Hrozek wrote:
> >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.
> 
> Updated patched attached.
> Only file src/providers/ldap/sdap_async.c was changed.
> 
> LS

Ack



More information about the sssd-devel mailing list