[SSSD] [PATCH] Adding option to disable retrieving large AD groups.
Jakub Hrozek
jhrozek at redhat.com
Wed May 15 11:54:10 UTC 2013
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 */
More information about the sssd-devel
mailing list