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