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 */