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.