Bug 55269 - UDM users/user should use memberOf overlay instead of searching for group membership
UDM users/user should use memberOf overlay instead of searching for group mem...
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - Users
UCS 5.0
Other Linux
: P5 normal (vote)
: UCS 5.0-2-errata
Assigned To: Florian Best
Peter Stoll
https://git.knut.univention.de/univen...
:
Depends on:
Blocks: 56253
  Show dependency treegraph
 
Reported: 2022-10-14 16:29 CEST by Florian Best
Modified: 2024-05-07 20:20 CEST (History)
3 users (show)

See Also:
What kind of report is it?: Development Internal
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional): API change, Cleanup, Debt Technical, Large environments, UCS Performance
Max CVSS v3 score:
best: Patch_Available+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2022-10-14 16:29:49 CEST
The UDM users/user module currently makes a LDAP search in open() to detect the group memberships of the user.

The groups are also provided by the "memberOf" operational attribute which can be used instead.
This increases the performance.
But the "memberOf" overlay is also more accurate: it adds all groups the user is part of, not only those which are allowed to read by the current LDAP connection. This is an API change which possibly has side effects, as mentioned in Bug #54853 comment 3:
> There is different behavior with the patch:
> The first search uses the ACL's of the current user while the memberOf
> attribute does not evaluate ACL's of the user and inserts all groups.
> 
> >>> set(x.decode('UTF-8') for x in a.get(dn, ['memberOf'])['memberOf']) - set(lo.searchDn(filter=filter_format(u'(&(cn=*)(|(objectClass=univentionGroup)(objectClass=sambaGroupMapping))(uniqueMember=%s))', [dn])))                          
> {'cn=foo,cn=schueler,cn=groups,ou=abc,dc=base',}
> 
> This might cause follow up errors when one iterates over the groups without
> noObject error handling.
Comment 1 Florian Best univentionstaff 2022-10-14 16:50:02 CEST
Patch available in: https://git.knut.univention.de/univention/ucs/-/merge_requests/536
Comment 2 Florian Best univentionstaff 2022-10-14 16:55:50 CEST
On a System with 2.000 users this saves 0.88 seconds for a UDM search:

Prior:
# time curl --unix-socket /var/run/univention-directory-manager-rest-en-us.socket -s "http://Administrator:univention@localhost/udm/users/user/?properties=*" -H  "accept: application/json"  > /dev/null                      

real    0m4,820s
user    0m0,010s
sys     0m0,006s


Afterwards:
# time curl --unix-socket /var/run/univention-directory-manager-rest-en-us.socket -s "http://Administrator:univention@localhost/udm/users/user/?properties=*" -H  "accept: application/json"  > /dev/null 

real    0m3,935s
user    0m0,011s
sys     0m0,004s
Comment 3 Florian Best univentionstaff 2022-10-26 16:11:58 CEST
The comment 2 above was with a System with 2 GB RAM.

On a System with 50.000 Users and 40.000 Groups (each user in ~2 groups) with 32 GB RAM the performance difference is:

Prior:
# time curl --unix-socket /var/run/univention-directory-manager-rest-en-us.socket -s "http://Administrator:univention@localhost/udm/users/user/?foo=bar" -H  "accept: application/json" > /dev/null 

real    6m5,792s
user    0m0,064s
sys     0m0,038s


Afterwards:
# time curl --unix-socket /var/run/univention-directory-manager-rest-en-us.socket -s "http://Administrator:univention@localhost/udm/users/user/?foo=bar" -H  "accept: application/json" > /dev/null 

real    1m44,952s
user    0m0,031s
sys     0m0,055s
Comment 5 Michael Ströder 2022-11-08 12:08:09 CET
This needs way more careful considerations:

1. slapo-memberof is deprecated and to be replaced by slapo-dynlist:

See also:

ITS#9795 - Remove memberof overlay 
https://bugs.openldap.org/show_bug.cgi?id=9795

ITS#8613 - slapo-memberOf documentation update (Unsafe to use with replication) 
https://bugs.openldap.org/show_bug.cgi?id=8613

2. 'memberOf' is generated on-the-fly by slapo-dynlist. So it cannot be indexed and some use-cases are way slower with that.

3. The issues are even bigger with nested group membership.
Comment 6 Florian Best univentionstaff 2022-11-29 10:32:55 CET
(In reply to Michael Ströder from comment #5)
> This needs way more careful considerations:
Thanks for the points.
 
> 1. slapo-memberof is deprecated and to be replaced by slapo-dynlist:
> 
> See also:
> 
> ITS#9795 - Remove memberof overlay 
> https://bugs.openldap.org/show_bug.cgi?id=9795
> 
> ITS#8613 - slapo-memberOf documentation update (Unsafe to use with
> replication) 
> https://bugs.openldap.org/show_bug.cgi?id=8613
Yes, but we need to postpone this. The change to slapo-dynlist needs to be done in a different major or minor UCS release.

> 2. 'memberOf' is generated on-the-fly by slapo-dynlist. So it cannot be
> indexed and some use-cases are way slower with that.
could you elaborate? in my tests for simple receiving memberships I had only performance enhancements.
 
> 3. The issues are even bigger with nested group membership.
We don't resolve nested group memberships currently. memberOf doesn't do it as well?
Comment 7 Peter Stoll univentionstaff 2022-11-30 19:40:56 CET
Verified:

* Code review
* Package build
* Performace test before vs. after this change
* Compared test output before vs. after -> no differences
* Changelog and YAML advisory
Comment 8 Florian Best univentionstaff 2022-11-30 20:35:41 CET
The changed have been merged:

univention-directory-manager-modules.yaml
c30da519e115 | perf(udm users/user)!: add UCR variable for backwards compatibility
    Using the group memberships via `memberOf` adds all groups to the user
    which he is assigned to, even if the reading user cannot read the
    specific groups of if the memberships are no groups/group objects.
    
    As there might be code in the wild which don't do error handling when
    iterating over group memberships a UCR variable
    `directory/manager/user/group-memberships-via-memberof` can be used to
    restore the old behavior.
    
    The variable is going to be remove in UCS 5.1.

univention-directory-manager-modules (15.0.13-26)
c30da519e115 | perf(udm users/user)!: add UCR variable for backwards compatibility
210adfac9101 | perf(udm users/user)!: use "memberOf" instead of searching for group memberships
Comment 9 Florian Best univentionstaff 2022-12-07 12:10:07 CET
One test failed because it tested that "groups" are not available without open(). Simply changed to "primaryGroup" so the test behvaior is equal:
 
https://univention-dist-jenkins.k8s.knut.univention.de/job/UCS-5.0/job/UCS-5.0-2/job/AutotestUpgrade/lastCompletedBuild/SambaVersion=s4,Systemrolle=master/testReport/59_udm.50_test_udm_api/TestUdmAutoOpen/test_auto_open_false/

ucs-test (10.0.7-28)
ae69615e5fab | test(udm): adjust for "groups" availability in "users/user"