Bug 50629

Summary:	S4C sync_from_ucs: Group members are replaced as whole instead of applying a diff
Product:	UCS	Reporter:	Florian Best <best>
Component:	S4 Connector	Assignee:	Florian Best <best>
Status:	RESOLVED MOVED	QA Contact:	Samba maintainers <samba-maintainers>
Severity:	normal
Priority:	P5	CC:	bremer, requate
Version:	UCS 4.4	Flags:	best: Patch_Available+
Target Milestone:	---
Hardware:	Other
OS:	Linux
URL:	https://git.knut.univention.de/univention/ucs/-/merge_requests/171
See Also:	https://forge.univention.org/bugzilla/show_bug.cgi?id=33621 https://forge.univention.org/bugzilla/show_bug.cgi?id=48545
What kind of report is it?:	Bug Report	What type of bug is this?:	7: Crash: Bug causes crash or data loss
Who will be affected by this bug?:	2: Will only affect a few installed domains	How will those affected feel about the bug?:	2: A Pain – users won’t like this once they notice it
User Pain:	0.160	Enterprise Customer affected?:
School Customer affected?:		ISV affected?:
Waiting Support:		Flags outvoted (downgraded) after PO Review:
Ticket number:		Bug group (optional):	Large environments, UCS Performance
Max CVSS v3 score:
Bug Depends on:
Bug Blocks:	50630

Description Florian Best

2019-12-12 07:35:15 CET

The S4-Connector replaces all group members by replacing the whole members with the new+current members when syncing from UCS to AD:

services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py
  1528 »   def group_members_sync_from_ucs(self, key, object):
…
  1696 »   »   »   »   self.lo_s4.lo.modify_s(compatible_modstring(object['dn']), [(ldap.MOD_REPLACE, 'member', modlist_members)])

Instead we should remove all members with ldap.MOD_DELETE and add all new members with ldap.MOD_ADD.

The code is prone to race conditions if there are changes inbetween on AD side: causing a loss of group members.
Especially in large environments with a lot of group members this might have an performance impact.

Comment 1 Florian Best

2019-12-12 08:31:15 CET

Untested patch in git:fbest/50629-group-member-sync-from-ucs

Comment 2 Arvid Requate

2019-12-12 19:39:19 CET

IIRC it's not so clear to decide which strategy is better. If you do a diff based approach, you may also have a race between third-party (Administrator) changes to the Samba/AD data and the S4-Connector. But I think you are right, that the diff based approach a) is more precise and b) performs better. If a change operation fails due to concurrent changes, then we would simply get a reject and we would retry later.