Bug 50630 - ADC sync_from_ucs: Group members are replaced as whole instead of applying a diff
ADC sync_from_ucs: Group members are replaced as whole instead of applying a ...
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: AD Connector
UCS 4.4
Other Linux
: P5 normal (vote)
: UCS 4.4-3-errata
Assigned To: Julia Bremer
Johannes Keiser
https://github.com/univention/univent...
:
Depends on: 50629
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-12 07:36 CET by Florian Best
Modified: 2020-04-15 18:06 CEST (History)
7 users (show)

See Also:
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?: Yes
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number: 2019111921000718, 2020010821000479
Bug group (optional): External feedback, UCS Performance
Max CVSS v3 score:


Attachments
patch? (git:fbest/50630-group-member-sync-from-ucs) (1.84 KB, patch)
2019-12-12 08:32 CET, Florian Best
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2019-12-12 07:36:00 CET
The AD-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.

+++ This bug was initially created as a clone of Bug #50629 +++
Comment 1 Florian Best univentionstaff 2019-12-12 08:32:07 CET
Created attachment 10262 [details]
patch? (git:fbest/50630-group-member-sync-from-ucs)

Here is an untested patch.
Comment 2 Christina Scheinig univentionstaff 2020-01-08 11:59:45 CET
The customer installed the patch and now gets the following traceback, when he adds a user to a group (via UCS):

08.01.2020 10:52:28,946 LDAP        (WARNING): sync failed, saved as rejected
08.01.2020 10:52:28,946 LDAP        (WARNING): Traceback (most recent call last):
 File "/usr/lib/pymodules/python2.7/univention/connector/__init__.py", line 785, in __sync_file_from_ucs
   if ((old_dn and not self.sync_from_ucs(key, object, premapped_ucs_dn, unicode(old_dn, 'utf8'))) or (not old_dn and not self.sync_from_ucs(key, object, premapped_ucs_dn, old_dn))):
 File "/usr/lib/pymodules/python2.7/univention/connector/ad/__init__.py", line 2645, in sync_from_ucs
   f(self, property_type, object)
 File "/usr/lib/pymodules/python2.7/univention/connector/ad/__init__.py", line 176, in group_members_sync_from_ucs
   return connector.group_members_sync_from_ucs(key, object)
 File "/usr/lib/pymodules/python2.7/univention/connector/ad/__init__.py", line 1806, in group_members_sync_from_ucs
   self.lo_s4.lo.modify_s(compatible_modstring(object['dn']), [(ldap.MOD_ADD, 'member', list(map(compatible_modstring, add_members)))])
AttributeError: ad instance has no attribute 'lo_s4'
Comment 3 Florian Best univentionstaff 2020-01-08 14:53:59 CET
Oh, replace "self.lo_s4" with "self.lo_ad" in the patch!
Comment 4 Daniel Tröder univentionstaff 2020-01-15 08:15:36 CET
There is a PR for this bug: https://github.com/univention/univention-corporate-server/pull/16
Comment 5 Mathieu Simon 2020-01-16 11:47:28 CET
Hi Daniel

The PR is only geared towards Florian's branch (which has that typo he has mentioned). This I've not updated this ticket since I'd like help him in getting his patch properly into shape for inclusion as I'm definitely interested in getting this upstream.

Regards
Mathieu
Comment 6 Christina Scheinig univentionstaff 2020-02-04 13:27:24 CET
After updating the UCS version to 4.4-3 the patch throws a traceback when a user is removed from a group.

 04.02.2020 10:09:18.505 LDAP        (PROCESS): sync from ucs: [        
group] [    modify]
cn=rg5_systemadministratoren,cn=groups,DC=ad,DC=schein,DC=ig
04.02.2020 10:09:18.544 LDAP        (WARNING): sync failed, saved as
rejected
04.02.2020 10:09:18.545 LDAP        (WARNING): Traceback (most recent
call last):
   File "/usr/lib/pymodules/python2.7/univention/connector/__init__.py",
line 785, in __sync_file_from_ucs
     if ((old_dn and not self.sync_from_ucs(key, object,
premapped_ucs_dn, unicode(old_dn, 'utf8'))) or (not old_dn and not
self.sync_from_ucs(key, object, premapped_ucs_dn, old_dn))):
   File
"/usr/lib/pymodules/python2.7/univention/connector/ad/__init__.py", line
2645, in sync_from_ucs
     f(self, property_type, object)
   File
"/usr/lib/pymodules/python2.7/univention/connector/ad/__init__.py", line
176, in group_members_sync_from_ucs
     return connector.group_members_sync_from_ucs(key, object)
   File
"/usr/lib/pymodules/python2.7/univention/connector/ad/__init__.py", line
1808, in group_members_sync_from_ucs
     self.lo_ad.lo.modify_s(compatible_modstring(object['dn']), [(ldap.MOD_DEL, 'member', list(map(compatible_modstring, del_members)))])
AttributeError: 'module' object has no attribute 'MOD_DEL'
Comment 7 Arvid Requate univentionstaff 2020-02-04 14:17:33 CET
Are you refering to an installation with patch from Comment 1 applied?
There's another typo in the patch, it's ldap.MOD_DELETE instead of ldap.MOD_DEL.
Comment 8 Julia Bremer univentionstaff 2020-03-05 09:52:39 CET
Successful build
Package: univention-ad-connector
Version: 13.0.0-31A~4.4.0.202003050945
Branch: ucs_4.4-0
Scope: errata4.4-3


cc53c886f8 Bug #50630: yaml
25b3ba09dd Bug #50630: Diff group members in ad-connector sync


I fixed the typos in the patch and merged it.
The AD-Connector tests pass and the list of group members is not replaced as a whole any more.
Comment 9 Johannes Keiser univentionstaff 2020-03-06 14:20:20 CET
OK: ADD/DELETE  instead of REPLACE
OK: jenkins tests
OK: yaml
-> verified
Comment 10 Arvid Requate univentionstaff 2020-03-10 14:20:13 CET
http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-3/job/ADConnectorMultiEnv/Version=s4connector-w2k8r2-german/lastCompletedBuild/testReport/

* http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-3/job/ADConnectorMultiEnv/Version=s4connector-w2k8r2-german/24/testReport/55_adconnector/101sync_initial_membership_ad_to_ucs/admember237/


On the other hand, this test setup is *very* new and special, because it has S4-Connector *and* AD-Connector installed (for Bug #50492), and three S4-Connector related tests also failed, so it may simply be flaky. Please note that the term "admember237" is wrong, it was just a typo in the new test-scenario file. It's actually master237.
Comment 11 Arvid Requate univentionstaff 2020-03-11 16:41:45 CET
I've run this test several times on my VMs and at first it consistently failed. Then all of a sudden, it works, consistently. The funny thing is: Neither the test-user nor the test-groups get synchronized bei the S4-Connector to Samba/AD, so the "interference"/"timing" theory is ruled out to. I've just installed the ADC, configured SSL for the communication, installed ucs-test-adconnector and run the 101* test case individually. Crazy Heisenbug.
Comment 12 Arvid Requate univentionstaff 2020-03-13 16:45:19 CET
The test case in question is stable since two runs:

* http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-3/job/ADConnectorMultiEnv/Version=s4connector-w2k8r2-german/lastCompletedBuild/testReport/55_adconnector/101sync_initial_membership_ad_to_ucs/history/

Additionally one of the other 4 S4-Connector related test cases could be explained by Bug #50944.

So I'd assume that this Bug is not causing any regression.
Comment 13 Johannes Keiser univentionstaff 2020-03-16 15:21:24 CET
(In reply to Arvid Requate from comment #12)
> The test case in question is stable since two runs:
> 
> *
> http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-3/job/
> ADConnectorMultiEnv/Version=s4connector-w2k8r2-german/lastCompletedBuild/
> testReport/55_adconnector/101sync_initial_membership_ad_to_ucs/history/
> 
> Additionally one of the other 4 S4-Connector related test cases could be
> explained by Bug #50944.
> 
> So I'd assume that this Bug is not causing any regression.

-> OK verified
Comment 14 Erik Damrose univentionstaff 2020-03-18 12:27:45 CET
<http://errata.software-univention.de/ucs/4.4/494.html>