Bug 50629 – S4C sync_from_ucs: Group members are replaced as whole instead of applying a diff

Bug 50629 - S4C sync_from_ucs: Group members are replaced as whole instead of applying a diff


Summary:	S4C sync_from_ucs: Group members are replaced as whole instead of applying a ...

Status:	RESOLVED MOVED

Product:	UCS
Classification:	Unclassified
Component:	S4 Connector
Version:	UCS 4.4
Hardware:	Other Linux

Importance:	P5 normal (vote)
Target Milestone:	---
Assigned To:	Florian Best
QA Contact:	Samba maintainers

URL:	https://git.knut.univention.de/univen...
Keywords:

Depends on:
Blocks:	50630
	Show dependency tree / graph

Reported:	2019-12-12 07:35 CET by Florian Best
Modified:	2024-03-08 12:41 CET (History)
CC List:	2 users (show)

See Also:	33621 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:

Flags:	best: Patch_Available+

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

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.