On 04/20/2015 09:51 AM, Jan Cholasta wrote:
Dne 9.4.2015 v 13:56 Petr Vobornik napsal(a):
On 04/08/2015 10:21 AM, Jan Cholasta wrote:
Hi,

Dne 31.3.2015 v 12:11 Petr Vobornik napsal(a):
the old implementation tried to get all entries which are member of
group. That means also user. User can't have any members therefore this
costly processing was unnecessary.

New implementation reduces the search only to entries which can have
entries.

Also page size was removed to avoid paging by small pages(default size:
100) which is very slow for many members.

https://fedorahosted.org/freeipa/ticket/4947

Useful to test with #809

1) To search for entries with members, you should search for entries
with the member attribute set ('(member=*)'), not for entries with some
arbitrary object class.

Replaced, new presence index added



2) I don't like how the search in get_memberindirect is limited to an
arbitrary hard-coded subtree. You should go through the object's
attribute_members to figure out which subtrees to search.


The subtree search was removed.


3) Since memberindirect and memberofindirect are not real attributes,
you must define their syntax in ipaldap before you cat set them using
.raw[], otherwise they will be decoded to wrong type.

Added.


4) The processing of memberof should be done even when memberofindirect
is not requested, otherwise its value will depend on whether
memberofindirect was requested or not.

True, but it's the same behavior as before. Could be changed in other
patch.

OK. Should we file a ticket?

AFAIK, memberof and memberofindirect are requested always together atm. Do we have a use case for this change? In any case, I've opened a ticket about more finer control of fetching members (as was discussed previously in triage and dev mtgs), it might be part of it.

https://fedorahosted.org/freeipa/ticket/4995




5) I would prefer if all membership processing
(.convert_attribute_members() and .get_indirect_members()) was done in a
single LDAPObject method.

Now, as before, get_indirect_members is called before post callbacks and
convert_attribute_members after. If it should be combined, it should be
done separately.

OK, but at least move get_indirect_members to LDAPObject.


Moved
--
Petr Vobornik
From 68164aa2a5acba14dbfd6bb6d0f2b45cadf6a503 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Tue, 31 Mar 2015 10:59:37 +0200
Subject: [PATCH] speed up indirect member processing

the old implementation tried to get all entries which are member of group.
That means also user. User can't have any members therefore this costly
processing was unnecessary.

New implementation reduces the search only to entries which have members.

Also page size was removed to avoid paging by small pages(default size: 100)
which is very slow for many members.

https://fedorahosted.org/freeipa/ticket/4947
---
 install/updates/20-indices.update |  2 +-
 ipalib/plugins/baseldap.py        | 72 +++++++++++++++++++++++++++++++
 ipalib/plugins/host.py            |  2 +-
 ipalib/plugins/role.py            |  8 ++--
 ipapython/ipaldap.py              |  2 +
 ipaserver/plugins/ldap2.py        | 90 ---------------------------------------
 6 files changed, 81 insertions(+), 95 deletions(-)

diff --git a/install/updates/20-indices.update b/install/updates/20-indices.update
index a8a432d9c28a7fb4ca74582e36d4c39fd98df2cf..a9ec9f9eb9bcc228dcbb7eba99879ce5a8251e80 100644
--- a/install/updates/20-indices.update
+++ b/install/updates/20-indices.update
@@ -27,7 +27,7 @@ default:nsSystemIndex: false
 only:nsIndexType: eq,pres,sub
 
 dn: cn=member,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
-only:nsIndexType: eq,sub
+only:nsIndexType: eq,pres,sub
 
 dn: cn=uniquemember,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
 only:nsIndexType: eq,sub
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index ca4e54fd269aba9fdcf234f6450382fb7daabe42..b06b570cbc353aa903e5b2218932b8c6e59ce31b 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -663,6 +663,67 @@ class LDAPObject(Object):
                         new_attr.append(new_value)
                         break
 
+    def get_indirect_members(self, entry_attrs, attrs_list):
+        if 'memberindirect' in attrs_list:
+            self.get_memberindirect(entry_attrs)
+        if 'memberofindirect' in attrs_list:
+            self.get_memberofindirect(entry_attrs)
+
+    def get_memberindirect(self, group_entry):
+        """
+        Get indirect members
+        """
+
+        mo_filter = self.backend.make_filter({'memberof': group_entry.dn})
+        filter = self.backend.combine_filters(
+            ('(member=*)', mo_filter), self.backend.MATCH_ALL)
+        try:
+            result, truncated = self.backend.find_entries(
+                base_dn=self.api.env.basedn,
+                filter=filter,
+                attrs_list=['member'],
+                size_limit=-1, # paged search will get everything anyway
+                paged_search=True)
+            if truncated:
+                raise errors.LimitsExceeded()
+        except errors.NotFound:
+            result = []
+
+        indirect = set()
+        for entry in result:
+            indirect.update(entry.raw.get('member', []))
+        indirect.difference_update(group_entry.raw.get('member', []))
+
+        if indirect:
+            group_entry.raw['memberindirect'] = list(indirect)
+
+    def get_memberofindirect(self, entry):
+
+        dn = entry.dn
+        filter = self.backend.make_filter(
+            {'member': dn, 'memberuser': dn, 'memberhost': dn})
+        try:
+            result, truncated = self.backend.find_entries(
+                base_dn=self.api.env.basedn,
+                filter=filter,
+                attrs_list=[''])
+            if truncated:
+                raise errors.LimitsExceeded()
+        except errors.NotFound:
+            result = []
+
+        direct = set()
+        indirect = set(entry.raw.get('memberof', []))
+        for group_entry in result:
+            dn = str(group_entry.dn)
+            if dn in indirect:
+                indirect.remove(dn)
+                direct.add(dn)
+
+        entry.raw['memberof'] = list(direct)
+        if indirect:
+            entry.raw['memberofindirect'] = list(indirect)
+
     def get_password_attributes(self, ldap, dn, entry_attrs):
         """
         Search on the entry to determine if it has a password or
@@ -1205,6 +1266,8 @@ class LDAPCreate(BaseLDAPCommand, crud.Create):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.obj.get_indirect_members(entry_attrs, attrs_list)
+
         for callback in self.get_callbacks('post'):
             entry_attrs.dn = callback(
                 self, ldap, entry_attrs.dn, entry_attrs, *keys, **options)
@@ -1328,6 +1391,8 @@ class LDAPRetrieve(LDAPQuery):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.obj.get_indirect_members(entry_attrs, attrs_list)
+
         if options.get('rights', False) and options.get('all', False):
             entry_attrs['attributelevelrights'] = get_effective_rights(
                 ldap, entry_attrs.dn)
@@ -1478,6 +1543,8 @@ class LDAPUpdate(LDAPQuery, crud.Update):
                 format=_('the entry was deleted while being modified')
             )
 
+        self.obj.get_indirect_members(entry_attrs, attrs_list)
+
         if options.get('rights', False) and options.get('all', False):
             entry_attrs['attributelevelrights'] = get_effective_rights(
                 ldap, entry_attrs.dn)
@@ -1712,6 +1779,8 @@ class LDAPAddMember(LDAPModMember):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.obj.get_indirect_members(entry_attrs, attrs_list)
+
         for callback in self.get_callbacks('post'):
             (completed, entry_attrs.dn) = callback(
                 self, ldap, completed, failed, entry_attrs.dn, entry_attrs,
@@ -1814,6 +1883,8 @@ class LDAPRemoveMember(LDAPModMember):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.obj.get_indirect_members(entry_attrs, attrs_list)
+
         for callback in self.get_callbacks('post'):
             (completed, entry_attrs.dn) = callback(
                 self, ldap, completed, failed, entry_attrs.dn, entry_attrs,
@@ -2034,6 +2105,7 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
 
         if not options.get('raw', False):
             for e in entries:
+                self.obj.get_indirect_members(e, attrs_list)
                 self.obj.convert_attribute_members(e, *args, **options)
 
         for (i, e) in enumerate(entries):
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 41710f3b87b5f60ac6e9b0765a25562b7074e2e8..c47439743da45b8629d1b2afbd210d87591784ce 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -296,7 +296,7 @@ class host(LDAPObject):
     default_attributes = [
         'fqdn', 'description', 'l', 'nshostlocation', 'krbprincipalname',
         'nshardwareplatform', 'nsosversion', 'usercertificate', 'memberof',
-        'managedby', 'memberindirect', 'memberofindirect', 'macaddress',
+        'managedby', 'memberofindirect', 'macaddress',
         'userclass', 'ipaallowedtoperform', 'ipaassignedidview',
     ]
     uuid_attribute = 'ipauniqueid'
diff --git a/ipalib/plugins/role.py b/ipalib/plugins/role.py
index 55afece22165a550d570fff3aa9f25d103d20b7a..6d8d544aab12023e73bdf3c60db51cef52151aac 100644
--- a/ipalib/plugins/role.py
+++ b/ipalib/plugins/role.py
@@ -71,9 +71,11 @@ class role(LDAPObject):
     object_name_plural = _('roles')
     object_class = ['groupofnames', 'nestedgroup']
     permission_filter_objectclasses = ['groupofnames']
-    default_attributes = ['cn', 'description', 'member', 'memberof',
-        'memberindirect', 'memberofindirect',
-    ]
+    default_attributes = ['cn', 'description', 'member', 'memberof']
+    # Role could have a lot of indirect members, but they are not in
+    # attribute_members therefore they don't have to be in default_attributes
+    # 'memberindirect', 'memberofindirect',
+
     attribute_members = {
         'member': ['user', 'group', 'host', 'hostgroup', 'service'],
         'memberof': ['privilege'],
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7cda7d67d1174cddbff646fd7133a9b995cb58a2..75ff2177bd604b7861af79365496a41e93961c8c 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -665,6 +665,8 @@ class LDAPClient(object):
     _SYNTAX_OVERRIDE = CIDict({
         'managedtemplate': DN,
         'managedbase':     DN,
+        'memberindirect':  DN,
+        'memberofindirect':DN,
         'originscope':     DN,
         'idnsname':        DNSName,
         'idnssoamname':    DNSName,
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index 15e07f27bf10fbac6fdbc532bd8a3b3cd65fe374..d1d966c593883fa5449252dc1ffbc17678014bfa 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -220,102 +220,12 @@ class ldap2(LDAPClient, CrudBackend):
         if size_limit is None:
             size_limit = _get_limits()['size']
 
-        has_memberindirect = False
-        has_memberofindirect = False
-        if attrs_list:
-            new_attrs_list = []
-            for attr_name in attrs_list:
-                if attr_name == 'memberindirect':
-                    has_memberindirect = True
-                elif attr_name == 'memberofindirect':
-                    has_memberofindirect = True
-                else:
-                    new_attrs_list.append(attr_name)
-            attrs_list = new_attrs_list
-
         res, truncated = super(ldap2, self).find_entries(
             filter=filter, attrs_list=attrs_list, base_dn=base_dn, scope=scope,
             time_limit=time_limit, size_limit=size_limit,
             search_refs=search_refs, paged_search=paged_search)
-
-        if has_memberindirect or has_memberofindirect:
-
-            # For the memberof searches, we want to apply the global limit
-            # if it's larger than the requested one, so decreasing limits on
-            # the individual query only affects the query itself.
-            # See https://fedorahosted.org/freeipa/ticket/4398
-            def _max_with_none(a, b):
-                """Maximum of a and b, treating None as infinity"""
-                if a is None or b is None:
-                    return None
-                else:
-                    return max(a, b)
-            time_limit = _max_with_none(time_limit, _get_limits()['time'])
-            size_limit = _max_with_none(size_limit, _get_limits()['size'])
-
-            for entry in res:
-                if has_memberindirect:
-                    self._process_memberindirect(
-                        entry, time_limit=time_limit, size_limit=size_limit)
-                if has_memberofindirect:
-                    self._process_memberofindirect(
-                        entry, time_limit=time_limit, size_limit=size_limit)
-
         return (res, truncated)
 
-    def _process_memberindirect(self, group_entry, time_limit=None,
-                                size_limit=None):
-        filter = self.make_filter({'memberof': group_entry.dn})
-        try:
-            result, truncated = self.find_entries(
-                base_dn=self.api.env.basedn,
-                filter=filter,
-                attrs_list=['member'],
-                time_limit=time_limit,
-                size_limit=size_limit,
-                paged_search=True)
-            if truncated:
-                raise errors.LimitsExceeded()
-        except errors.NotFound:
-            result = []
-
-        indirect = set()
-        for entry in result:
-            indirect.update(entry.get('member', []))
-        indirect.difference_update(group_entry.get('member', []))
-
-        if indirect:
-            group_entry['memberindirect'] = list(indirect)
-
-    def _process_memberofindirect(self, entry, time_limit=None,
-                                  size_limit=None):
-        dn = entry.dn
-        filter = self.make_filter(
-            {'member': dn, 'memberuser': dn, 'memberhost': dn})
-        try:
-            result, truncated = self.find_entries(
-                base_dn=self.api.env.basedn,
-                filter=filter,
-                attrs_list=[''],
-                time_limit=time_limit,
-                size_limit=size_limit)
-            if truncated:
-                raise errors.LimitsExceeded()
-        except errors.NotFound:
-            result = []
-
-        direct = set()
-        indirect = set(entry.get('memberof', []))
-        for group_entry in result:
-            dn = group_entry.dn
-            if dn in indirect:
-                indirect.remove(dn)
-                direct.add(dn)
-
-        entry['memberof'] = list(direct)
-        if indirect:
-            entry['memberofindirect'] = list(indirect)
-
     config_defaults = {'ipasearchtimelimit': [2], 'ipasearchrecordslimit': [0]}
     def get_ipa_config(self, attrs_list=None):
         """Returns the IPA configuration entry (dn, entry_attrs)."""
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to