[PATCH] replmd changes to handle single-valued link conflicts
Tim Beale
timbeale at catalyst.net.nz
Thu Sep 28 04:01:44 UTC 2017
Hi,
The attached patch-set fixes the following bugs:
- 13055: Samba replication cannot resolve single-valued link conflicts
- 13059: Link attributes on Samba have different initial RMD_VERSION
compared to Windows
The bulk of the changes are to handle conflicts correctly for
single-valued link attributes. It required a bit of refactoring in the
replmd code to support this.
Patches can also be viewed here:
http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/tim-replmd-latest
Please let me know if you have any review comments.
Thanks,
Tim
-------------- next part --------------
From c1101cd595a2ddac360dbca13a605decdf1f4610 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 18 Sep 2017 13:47:56 +1200
Subject: [PATCH 01/22] replmd: Refactor adding the backlink in
replmd_process_linked_attribute()
The code to add the backlink is the same in both the 'if' and the 'else'
case, so move it outside the if-else block.
(We're going to rework this block of code quite a bit in order to
support single-value linked attribute conflicts, aka bug #13055).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 34 ++++++++-----------------
1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 2e0f705..60a9c8c 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7301,19 +7301,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
talloc_free(tmp_ctx);
return ret;
}
-
- if (active) {
- /* add the new backlink */
- ret = replmd_add_backlink(module, replmd_private,
- schema,
- msg->dn,
- &guid, true, attr,
- parent);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
- }
} else {
unsigned offset;
/* get a seq_num for this change */
@@ -7367,17 +7354,18 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
talloc_free(tmp_ctx);
return ret;
}
+ }
- if (active) {
- ret = replmd_add_backlink(module, replmd_private,
- schema,
- msg->dn,
- &guid, true, attr,
- parent);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
+ /* if the new link is active, then add the new backlink */
+ if (active) {
+ ret = replmd_add_backlink(module, replmd_private,
+ schema,
+ msg->dn,
+ &guid, true, attr,
+ parent);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
}
}
--
2.7.4
From 6e9186b6469d7238650884c5d85a94426619f2a4 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 18 Sep 2017 16:33:30 +1200
Subject: [PATCH 02/22] replmd: Refactor logic to check if replicated link is
newer
This is precursor work for supporting single-link conflicts.
Split out the code to check if the link update is newer. It's now safe
to call this from the main codepath. This also means we can combine the 2
calls to get the seqnum into a single common call.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 85 ++++++++++++++-----------
1 file changed, 49 insertions(+), 36 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 60a9c8c..f0c5ab6 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7142,6 +7142,40 @@ static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar,
return ret;
}
+/**
+ * @returns true if the replication linked attribute info is newer than we
+ * already have in our DB
+ * @param pdn the existing linked attribute info in our DB
+ * @param la the new linked attribute info received during replication
+ */
+static bool replmd_link_update_is_newer(struct parsed_dn *pdn,
+ struct drsuapi_DsReplicaLinkedAttribute *la)
+{
+ /* see if this update is newer than what we have already */
+ struct GUID invocation_id = GUID_zero();
+ uint32_t version = 0;
+ uint32_t originating_usn = 0;
+ NTTIME change_time = 0;
+
+ if (pdn == NULL) {
+
+ /* no existing info so update is newer */
+ return true;
+ }
+
+ dsdb_get_extended_dn_guid(pdn->dsdb_dn->dn, &invocation_id, "RMD_INVOCID");
+ dsdb_get_extended_dn_uint32(pdn->dsdb_dn->dn, &version, "RMD_VERSION");
+ dsdb_get_extended_dn_uint32(pdn->dsdb_dn->dn, &originating_usn, "RMD_ORIGINATING_USN");
+ dsdb_get_extended_dn_nttime(pdn->dsdb_dn->dn, &change_time, "RMD_CHANGETIME");
+
+ return replmd_update_is_newer(&invocation_id,
+ &la->meta_data.originating_invocation_id,
+ version,
+ la->meta_data.version,
+ change_time,
+ la->meta_data.originating_change_time);
+}
+
/*
process one linked attribute structure
*/
@@ -7244,40 +7278,24 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
+ if (!replmd_link_update_is_newer(pdn, la)) {
+ DEBUG(3,("Discarding older DRS linked attribute update to %s on %s from %s\n",
+ old_el->name, ldb_dn_get_linearized(msg->dn),
+ GUID_string(tmp_ctx, &la->meta_data.originating_invocation_id)));
+ talloc_free(tmp_ctx);
+ return LDB_SUCCESS;
+ }
+
+ /* get a seq_num for this change */
+ ret = ldb_sequence_number(ldb, LDB_SEQ_NEXT, &seq_num);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
+ }
if (pdn != NULL) {
- /* see if this update is newer than what we have already */
- struct GUID invocation_id = GUID_zero();
- uint32_t version = 0;
- uint32_t originating_usn = 0;
- NTTIME change_time = 0;
uint32_t rmd_flags = dsdb_dn_rmd_flags(pdn->dsdb_dn->dn);
- dsdb_get_extended_dn_guid(pdn->dsdb_dn->dn, &invocation_id, "RMD_INVOCID");
- dsdb_get_extended_dn_uint32(pdn->dsdb_dn->dn, &version, "RMD_VERSION");
- dsdb_get_extended_dn_uint32(pdn->dsdb_dn->dn, &originating_usn, "RMD_ORIGINATING_USN");
- dsdb_get_extended_dn_nttime(pdn->dsdb_dn->dn, &change_time, "RMD_CHANGETIME");
-
- if (!replmd_update_is_newer(&invocation_id,
- &la->meta_data.originating_invocation_id,
- version,
- la->meta_data.version,
- change_time,
- la->meta_data.originating_change_time)) {
- DEBUG(3,("Discarding older DRS linked attribute update to %s on %s from %s\n",
- old_el->name, ldb_dn_get_linearized(msg->dn),
- GUID_string(tmp_ctx, &la->meta_data.originating_invocation_id)));
- talloc_free(tmp_ctx);
- return LDB_SUCCESS;
- }
-
- /* get a seq_num for this change */
- ret = ldb_sequence_number(ldb, LDB_SEQ_NEXT, &seq_num);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
-
if (!(rmd_flags & DSDB_RMD_FLAG_DELETED)) {
/* remove the existing backlink */
ret = replmd_add_backlink(module, replmd_private,
@@ -7303,12 +7321,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
}
} else {
unsigned offset;
- /* get a seq_num for this change */
- ret = ldb_sequence_number(ldb, LDB_SEQ_NEXT, &seq_num);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
+
/*
* We know where the new one needs to be, from the *next
* pointer into pdn_list.
--
2.7.4
From 3f49e48d01b55411879db604bad1a2845a80da05 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 18 Sep 2017 16:39:44 +1200
Subject: [PATCH 03/22] replmd: Remove unused originating_usn variable
The previous refactor makes it obvious that we aren't actually using
this variable for anything.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index f0c5ab6..73ec9c8 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7154,7 +7154,6 @@ static bool replmd_link_update_is_newer(struct parsed_dn *pdn,
/* see if this update is newer than what we have already */
struct GUID invocation_id = GUID_zero();
uint32_t version = 0;
- uint32_t originating_usn = 0;
NTTIME change_time = 0;
if (pdn == NULL) {
@@ -7165,7 +7164,6 @@ static bool replmd_link_update_is_newer(struct parsed_dn *pdn,
dsdb_get_extended_dn_guid(pdn->dsdb_dn->dn, &invocation_id, "RMD_INVOCID");
dsdb_get_extended_dn_uint32(pdn->dsdb_dn->dn, &version, "RMD_VERSION");
- dsdb_get_extended_dn_uint32(pdn->dsdb_dn->dn, &originating_usn, "RMD_ORIGINATING_USN");
dsdb_get_extended_dn_nttime(pdn->dsdb_dn->dn, &change_time, "RMD_CHANGETIME");
return replmd_update_is_newer(&invocation_id,
--
2.7.4
From d432a72229aa2d087f5eea1f039b7d559713d16a Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 19 Sep 2017 13:41:02 +1200
Subject: [PATCH 04/22] selftest: Make sure single-link conflict retains the
deleted link
There should only ever be one active value for a single-valued link
attribute. When a conflict occurs the 'losing' value should still be
present, but should be marked as deleted.
This change is just making the test criteria stricter to make sure that
we fix the bug correctly.
Note that the only way to query the deleted link attributes present
is to send a DRS request.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/torture/drs/python/link_conflicts.py | 49 ++++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/source4/torture/drs/python/link_conflicts.py b/source4/torture/drs/python/link_conflicts.py
index 4af3cd3..b666ebf 100644
--- a/source4/torture/drs/python/link_conflicts.py
+++ b/source4/torture/drs/python/link_conflicts.py
@@ -35,7 +35,8 @@ from ldb import SCOPE_BASE
import random
import time
-from samba.dcerpc import drsuapi
+from drs_base import AbstractLink
+from samba.dcerpc import drsuapi, misc
# specifies the order to sync DCs in
DC1_TO_DC2 = 1
@@ -55,6 +56,9 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
"dn": self.ou,
"objectclass": "organizationalUnit"})
+ (self.drs, self.drs_handle) = self._ds_bind(self.dnsname_dc1)
+ (self.drs2, self.drs2_handle) = self._ds_bind(self.dnsname_dc2)
+
# disable replication for the tests so we can control at what point
# the DCs try to replicate
self._disable_inbound_repl(self.dnsname_dc1)
@@ -68,7 +72,7 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
super(DrsReplicaLinkConflictTestCase, self).tearDown()
def get_guid(self, samdb, dn):
- """Returns an object's GUID"""
+ """Returns an object's GUID (in string format)"""
res = samdb.search(base=dn, attrs=["objectGUID"], scope=ldb.SCOPE_BASE)
return self._GUID_string(res[0]['objectGUID'][0])
@@ -137,6 +141,37 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self.assertTrue(val in res2[0][attr],
"%s '%s' not found on DC2" %(attr, val))
+ def _check_replicated_links(self, src_obj_dn, expected_links):
+ """Checks that replication sends back the expected linked attributes"""
+
+ hwm = drsuapi.DsReplicaHighWaterMark()
+ hwm.tmp_highest_usn = 0
+ hwm.reserved_usn = 0
+ hwm.highest_usn = 0
+
+ self._check_replication([src_obj_dn],
+ drsuapi.DRSUAPI_DRS_WRIT_REP,
+ dest_dsa=None,
+ drs_error=drsuapi.DRSUAPI_EXOP_ERR_SUCCESS,
+ nc_dn_str=src_obj_dn,
+ exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
+ expected_links=expected_links,
+ highwatermark=hwm)
+
+ # Check DC2 as well
+ self.set_test_ldb_dc(self.ldb_dc2)
+
+ self._check_replication([src_obj_dn],
+ drsuapi.DRSUAPI_DRS_WRIT_REP,
+ dest_dsa=None,
+ drs_error=drsuapi.DRSUAPI_EXOP_ERR_SUCCESS,
+ nc_dn_str=src_obj_dn,
+ exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
+ expected_links=expected_links,
+ highwatermark=hwm,
+ drs=self.drs2, drs_handle=self.drs2_handle)
+ self.set_test_ldb_dc(self.ldb_dc1)
+
def _test_conflict_single_valued_link(self, sync_order):
"""
Tests a simple single-value link conflict, i.e. each DC adds a link to
@@ -176,6 +211,16 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self.assertTrue(res1[0]["managedBy"][0] == target2_ou,
"Expected most recent update to win conflict")
+ # we can't query the deleted links over LDAP, but we can check DRS
+ # to make sure the DC kept a copy of the conflicting link
+ link1 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy, 0,
+ misc.GUID(src_guid), misc.GUID(target1_guid))
+ link2 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy,
+ drsuapi.DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE,
+ misc.GUID(src_guid), misc.GUID(target2_guid))
+ self._check_replicated_links(src_ou, [link1, link2])
+
+
def test_conflict_single_valued_link(self):
# repeat the test twice, to give each DC a chance to resolve the conflict
self._test_conflict_single_valued_link(sync_order=DC1_TO_DC2)
--
2.7.4
From a6f5f642519dcf558b9427304f7c3dd44b378a31 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Sep 2017 11:21:48 +1300
Subject: [PATCH 05/22] selftest: Print link meta-data when developer debugging
is used
For Windows, DRS is the only way to see the RMD_VERSION of a link, or to
tell what inactive links the DC. Add some debug to display this
information. By default, this debug is turned off.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/torture/drs/python/drs_base.py | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py
index c2760e2..10f2e63 100644
--- a/source4/torture/drs/python/drs_base.py
+++ b/source4/torture/drs/python/drs_base.py
@@ -253,10 +253,22 @@ class DrsBaseTestCase(SambaToolCmdTest):
next_object = next_object.next_object
print("Linked Attributes: %d" % ctr6.linked_attributes_count)
- ctr6_links = self._get_ctr6_links(ctr6)
- for link in ctr6_links:
+ for lidx in range(0, ctr6.linked_attributes_count):
+ l = ctr6.linked_attributes[lidx]
+ try:
+ target = ndr_unpack(drsuapi.DsReplicaObjectIdentifier3,
+ l.value.blob)
+ except:
+ target = ndr_unpack(drsuapi.DsReplicaObjectIdentifier3Binary,
+ l.value.blob)
+
print("Link Tgt %s... <-- Src %s"
- %(link.targetDN[:25], link.identifier))
+ %(target.dn[:25], l.identifier.guid))
+ state = "Del"
+ if l.flags & drsuapi.DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE:
+ state = "Act"
+ print(" v%u %s changed %u" %(l.meta_data.version, state,
+ l.meta_data.originating_change_time))
print("HWM: %d" %(ctr6.new_highwatermark.highest_usn))
print("Tmp HWM: %d" %(ctr6.new_highwatermark.tmp_highest_usn))
--
2.7.4
From 73894d6c4d2383055b2cefc741d50800dd9ceaf6 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 26 Sep 2017 13:55:11 +1300
Subject: [PATCH 06/22] selftest: Add test for deleted single-valued link
conflict
Currently we're only testing the case where the links have been modified
independently on 2 different DCs and both the links are active. We also
want to test the case where one link is active and the other is deleted.
Technically, this isn't really a conflict - the links involve different
target DNs, and the end result is still only one active link.
It's still probably worth having these tests to prove that fixing bug
13055 doesn't break anything.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/torture/drs/python/link_conflicts.py | 104 +++++++++++++++++++++++++++
1 file changed, 104 insertions(+)
diff --git a/source4/torture/drs/python/link_conflicts.py b/source4/torture/drs/python/link_conflicts.py
index b666ebf..95e2950 100644
--- a/source4/torture/drs/python/link_conflicts.py
+++ b/source4/torture/drs/python/link_conflicts.py
@@ -541,3 +541,107 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self._test_full_sync_link_conflict(sync_order=DC1_TO_DC2)
self._test_full_sync_link_conflict(sync_order=DC2_TO_DC1)
+ def _test_conflict_single_valued_link_deleted_winner(self, sync_order):
+ """
+ Tests a single-value link conflict where the more-up-to-date link value
+ is deleted.
+ """
+ src_ou = self.unique_dn("OU=src")
+ src_guid = self.add_object(self.ldb_dc1, src_ou)
+ self.sync_DCs()
+
+ # create a unique target on each DC
+ target1_ou = self.unique_dn("OU=target1")
+ target2_ou = self.unique_dn("OU=target2")
+
+ target1_guid = self.add_object(self.ldb_dc1, target1_ou)
+ target2_guid = self.add_object(self.ldb_dc2, target2_ou)
+
+ # add the links for the respective targets, and delete one of the links
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.add_link_attr(self.ldb_dc2, src_ou, "managedBy", target2_ou)
+ self.ensure_unique_timestamp()
+ self.del_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+
+ # sync the 2 DCs
+ self.sync_DCs(sync_order=sync_order)
+
+ res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
+ scope=SCOPE_BASE, attrs=["managedBy"])
+ res2 = self.ldb_dc2.search(base="<GUID=%s>" % src_guid,
+ scope=SCOPE_BASE, attrs=["managedBy"])
+
+ # Although the more up-to-date link value is deleted, this shouldn't
+ # trump DC1's active link
+ self.assert_attrs_match(res1, res2, "managedBy", 1)
+
+ self.assertTrue(res1[0]["managedBy"][0] == target2_ou,
+ "Expected active link win conflict")
+
+ # we can't query the deleted links over LDAP, but we can check that
+ # the deleted links exist using DRS
+ link1 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy, 0,
+ misc.GUID(src_guid), misc.GUID(target1_guid))
+ link2 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy,
+ drsuapi.DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE,
+ misc.GUID(src_guid), misc.GUID(target2_guid))
+ self._check_replicated_links(src_ou, [link1, link2])
+
+ def test_conflict_single_valued_link_deleted_winner(self):
+ # repeat the test twice, to give each DC a chance to resolve the conflict
+ self._test_conflict_single_valued_link_deleted_winner(sync_order=DC1_TO_DC2)
+ self._test_conflict_single_valued_link_deleted_winner(sync_order=DC2_TO_DC1)
+
+ def _test_conflict_single_valued_link_deleted_loser(self, sync_order):
+ """
+ Tests a single-valued link conflict, where the losing link value is deleted.
+ """
+ src_ou = self.unique_dn("OU=src")
+ src_guid = self.add_object(self.ldb_dc1, src_ou)
+ self.sync_DCs()
+
+ # create a unique target on each DC
+ target1_ou = self.unique_dn("OU=target1")
+ target2_ou = self.unique_dn("OU=target2")
+
+ target1_guid = self.add_object(self.ldb_dc1, target1_ou)
+ target2_guid = self.add_object(self.ldb_dc2, target2_ou)
+
+ # add the links - we want the link to end up deleted on DC2, but active on
+ # DC1. DC1 has the better version and DC2 has the better timestamp - the
+ # better version should win
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.del_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.ensure_unique_timestamp()
+ self.add_link_attr(self.ldb_dc2, src_ou, "managedBy", target2_ou)
+ self.del_link_attr(self.ldb_dc2, src_ou, "managedBy", target2_ou)
+
+ self.sync_DCs(sync_order=sync_order)
+
+ res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
+ scope=SCOPE_BASE, attrs=["managedBy"])
+ res2 = self.ldb_dc2.search(base="<GUID=%s>" % src_guid,
+ scope=SCOPE_BASE, attrs=["managedBy"])
+
+ # check the object has only have one occurence of the single-valued
+ # attribute and it matches on both DCs
+ self.assert_attrs_match(res1, res2, "managedBy", 1)
+
+ self.assertTrue(res1[0]["managedBy"][0] == target1_ou,
+ "Expected most recent update to win conflict")
+
+ # we can't query the deleted links over LDAP, but we can check DRS
+ # to make sure the DC kept a copy of the conflicting link
+ link1 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy,
+ drsuapi.DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE,
+ misc.GUID(src_guid), misc.GUID(target1_guid))
+ link2 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy, 0,
+ misc.GUID(src_guid), misc.GUID(target2_guid))
+ self._check_replicated_links(src_ou, [link1, link2])
+
+ def test_conflict_single_valued_link_deleted_loser(self):
+ # repeat the test twice, to give each DC a chance to resolve the conflict
+ self._test_conflict_single_valued_link_deleted_loser(sync_order=DC1_TO_DC2)
+ self._test_conflict_single_valued_link_deleted_loser(sync_order=DC2_TO_DC1)
+
--
2.7.4
From 63759f035c66ec09a5fd4343e99d3bdae085e428 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Sep 2017 14:43:53 +1300
Subject: [PATCH 07/22] selftest: Add conflict test where the single-valued
link already exists
As well as testing scenarios where both variants of the link are new, we
should also check the case where the received link already exists on the
DC as an inactive (i.e. previously deleted) link.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
selftest/knownfail.d/link_conflicts | 2 +
source4/torture/drs/python/link_conflicts.py | 59 ++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/selftest/knownfail.d/link_conflicts b/selftest/knownfail.d/link_conflicts
index e0c1ded..2a81b24 100644
--- a/selftest/knownfail.d/link_conflicts
+++ b/selftest/knownfail.d/link_conflicts
@@ -1,4 +1,6 @@
# Currently Samba can't resolve a conflict for a single-valued link attribute
samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_single_valued_link\(vampire_dc\)
samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_single_valued_link\(promoted_dc\)
+samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_existing_single_valued_link\(vampire_dc\)
+samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_existing_single_valued_link\(promoted_dc\)
diff --git a/source4/torture/drs/python/link_conflicts.py b/source4/torture/drs/python/link_conflicts.py
index 95e2950..c41d036 100644
--- a/source4/torture/drs/python/link_conflicts.py
+++ b/source4/torture/drs/python/link_conflicts.py
@@ -645,3 +645,62 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self._test_conflict_single_valued_link_deleted_loser(sync_order=DC1_TO_DC2)
self._test_conflict_single_valued_link_deleted_loser(sync_order=DC2_TO_DC1)
+ def _test_conflict_existing_single_valued_link(self, sync_order):
+ """
+ Tests a single-valued link conflict, where the conflicting link value
+ already exists (as inactive) on both DCs.
+ """
+ # create the link objects
+ src_ou = self.unique_dn("OU=src")
+ src_guid = self.add_object(self.ldb_dc1, src_ou)
+
+ target1_ou = self.unique_dn("OU=target1")
+ target2_ou = self.unique_dn("OU=target2")
+ target1_guid = self.add_object(self.ldb_dc1, target1_ou)
+ target2_guid = self.add_object(self.ldb_dc1, target2_ou)
+
+ # add the links, but then delete them
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.del_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target2_ou)
+ self.del_link_attr(self.ldb_dc1, src_ou, "managedBy", target2_ou)
+ self.sync_DCs()
+
+ # re-add the links independently on each DC
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+ self.ensure_unique_timestamp()
+ self.add_link_attr(self.ldb_dc2, src_ou, "managedBy", target2_ou)
+
+ # try to sync the 2 DCs (this currently fails)
+ try:
+ self.sync_DCs(sync_order=sync_order)
+ except Exception, e:
+ self.fail("Replication could not resolve link conflict: %s" % e)
+
+ res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
+ scope=SCOPE_BASE, attrs=["managedBy"])
+ res2 = self.ldb_dc2.search(base="<GUID=%s>" % src_guid,
+ scope=SCOPE_BASE, attrs=["managedBy"])
+
+ # check the object has only have one occurence of the single-valued
+ # attribute and it matches on both DCs
+ self.assert_attrs_match(res1, res2, "managedBy", 1)
+
+ # here we expect DC2 to win because it has the more recent link
+ self.assertTrue(res1[0]["managedBy"][0] == target2_ou,
+ "Expected most recent update to win conflict")
+
+ # we can't query the deleted links over LDAP, but we can check DRS
+ # to make sure the DC kept a copy of the conflicting link
+ link1 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy, 0,
+ misc.GUID(src_guid), misc.GUID(target1_guid))
+ link2 = AbstractLink(drsuapi.DRSUAPI_ATTID_managedBy,
+ drsuapi.DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE,
+ misc.GUID(src_guid), misc.GUID(target2_guid))
+ self._check_replicated_links(src_ou, [link1, link2])
+
+ def test_conflict_existing_single_valued_link(self):
+ # repeat the test twice, to give each DC a chance to resolve the conflict
+ self._test_conflict_existing_single_valued_link(sync_order=DC1_TO_DC2)
+ self._test_conflict_existing_single_valued_link(sync_order=DC2_TO_DC1)
+
--
2.7.4
From db1f2650307076d371b1984824700aabbb8d01a2 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 19 Sep 2017 11:59:58 +1200
Subject: [PATCH 08/22] replmd: Partial fix for single-valued link conflict
This is the first part of the fix for resolving a single-valued link
conflict.
When processing the replication data for a linked attribute, if we don't
find a match for the link target value, check if the link is a
single-valued attribute and it currently has an active link. If so, then
use the active link instead.
This change means we delete the existing active link (and backlink)
before adding the new link. This prevents the failure in the subsequent
dsdb_check_single_valued_link() check that was happening previously
(because the link would end up with 2 active values).
This is only a partial fix. It stops replication from failing completely
if we ever hit this situation (which means the test is no longer
hitting an assertion when replicating). However, ideally the existing
active link should be retained and just marked as deleted (with this
change, the existing link is overwritten completely).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 74 ++++++++++++++++++++++++-
source4/torture/drs/python/link_conflicts.py | 7 +--
2 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 73ec9c8..2a1069c 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7143,6 +7143,56 @@ static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar,
}
/**
+ * Finds the current active Parsed-DN value for a single-valued linked
+ * attribute, if one exists.
+ * @param ret_pdn assigned the active Parsed-DN, or NULL if none was found
+ * @returns LDB_SUCCESS (regardless of whether a match was found), unless
+ * an error occurred
+ */
+static int replmd_get_active_singleval_link(struct ldb_module *module,
+ TALLOC_CTX *mem_ctx,
+ struct parsed_dn pdn_list[],
+ unsigned int count,
+ const struct dsdb_attribute *attr,
+ struct parsed_dn **ret_pdn)
+{
+ unsigned int i;
+
+ *ret_pdn = NULL;
+
+ if (!(attr->ldb_schema_attribute->flags & LDB_ATTR_FLAG_SINGLE_VALUE)) {
+
+ /* nothing to do for multi-valued linked attributes */
+ return LDB_SUCCESS;
+ }
+
+ for (i = 0; i < count; i++) {
+ int ret = LDB_SUCCESS;
+ struct parsed_dn *pdn = &pdn_list[i];
+
+ /* skip any inactive links */
+ if (dsdb_dn_is_deleted_val(pdn->v)) {
+ continue;
+ }
+
+ /* we've found an active value for this attribute */
+ *ret_pdn = pdn;
+
+ if (pdn->dsdb_dn == NULL) {
+ struct ldb_context *ldb = ldb_module_get_ctx(module);
+
+ ret = really_parse_trusted_dn(mem_ctx, ldb, pdn,
+ attr->syntax->ldap_oid);
+ }
+
+ return ret;
+ }
+
+ /* no active link found */
+ return LDB_SUCCESS;
+}
+
+/**
* @returns true if the replication linked attribute info is newer than we
* already have in our DB
* @param pdn the existing linked attribute info in our DB
@@ -7276,6 +7326,28 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
+ if (pdn == NULL && active) {
+
+ /*
+ * check if there's a conflict for single-valued links, i.e.
+ * an active linked attribute already exists, but it has a
+ * different target value
+ */
+ ret = replmd_get_active_singleval_link(module, tmp_ctx, pdn_list,
+ old_el->num_values, attr,
+ &pdn);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
+ }
+
+ if (pdn != NULL) {
+ DBG_WARNING("Link conflict for %s linked from %s\n",
+ ldb_dn_get_linearized(pdn->dsdb_dn->dn),
+ ldb_dn_get_linearized(msg->dn));
+ }
+ }
+
if (!replmd_link_update_is_newer(pdn, la)) {
DEBUG(3,("Discarding older DRS linked attribute update to %s on %s from %s\n",
old_el->name, ldb_dn_get_linearized(msg->dn),
@@ -7299,7 +7371,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
ret = replmd_add_backlink(module, replmd_private,
schema,
msg->dn,
- &guid, false, attr,
+ &pdn->guid, false, attr,
parent);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
diff --git a/source4/torture/drs/python/link_conflicts.py b/source4/torture/drs/python/link_conflicts.py
index c41d036..036472b 100644
--- a/source4/torture/drs/python/link_conflicts.py
+++ b/source4/torture/drs/python/link_conflicts.py
@@ -193,11 +193,8 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self.ensure_unique_timestamp()
self.add_link_attr(self.ldb_dc2, src_ou, "managedBy", target2_ou)
- # try to sync the 2 DCs (this currently fails)
- try:
- self.sync_DCs(sync_order=sync_order)
- except Exception, e:
- self.fail("Replication could not resolve link conflict: %s" % e)
+ # sync the 2 DCs
+ self.sync_DCs(sync_order=sync_order)
res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
scope=SCOPE_BASE, attrs=["managedBy"])
--
2.7.4
From cfc0ee184ed58c7ae1524933cb28562d1c38702c Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Sep 2017 13:44:29 +1300
Subject: [PATCH 09/22] replmd: Handle conflicts for single-valued link
attributes better
If 2 DCs independently set a single-valued linked attribute to differing
values, Samba should be able to resolve this problem when replication
occurs.
If the received information is better, then we want to set the existing
link attribute in our DB as inactive.
If our own information is better, then we still want to add the received
link attribute, but mark it as inactive so that it doesn't clobber our
own link.
This still isn't a complete solution. When we add the received attribute
as inactive, we really should be incrementing the version, updating the
USN, etc. Also this only deals with the case where the received link is
completely new (i.e. a received link conflicting with an existing
inactive link isn't handled).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
selftest/knownfail.d/link_conflicts | 2 -
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 89 +++++++++++++++++++++++--
2 files changed, 82 insertions(+), 9 deletions(-)
diff --git a/selftest/knownfail.d/link_conflicts b/selftest/knownfail.d/link_conflicts
index 2a81b24..a927997 100644
--- a/selftest/knownfail.d/link_conflicts
+++ b/selftest/knownfail.d/link_conflicts
@@ -1,6 +1,4 @@
# Currently Samba can't resolve a conflict for a single-valued link attribute
-samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_single_valued_link\(vampire_dc\)
-samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_single_valued_link\(promoted_dc\)
samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_existing_single_valued_link\(vampire_dc\)
samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_existing_single_valued_link\(promoted_dc\)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 2a1069c..80a5cad 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7224,6 +7224,54 @@ static bool replmd_link_update_is_newer(struct parsed_dn *pdn,
la->meta_data.originating_change_time);
}
+/**
+ * Marks an existing linked attribute value as deleted in the DB
+ * @param pdn the parsed-DN of the target-value to delete
+ */
+static int replmd_delete_link_value(struct ldb_module *module,
+ struct replmd_private *replmd_private,
+ TALLOC_CTX *mem_ctx,
+ struct ldb_dn *src_obj_dn,
+ const struct dsdb_schema *schema,
+ const struct dsdb_attribute *attr,
+ uint64_t seq_num,
+ bool is_active,
+ struct GUID *target_guid,
+ struct dsdb_dn *target_dsdb_dn,
+ struct ldb_val *output_val)
+{
+ struct ldb_context *ldb = ldb_module_get_ctx(module);
+ time_t t;
+ NTTIME now;
+ const struct GUID *invocation_id = NULL;
+ int ret;
+
+ t = time(NULL);
+ unix_to_nt_time(&now, t);
+
+ invocation_id = samdb_ntds_invocation_id(ldb);
+ if (invocation_id == NULL) {
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+
+ /* if the existing link is active, remove its backlink */
+ if (is_active) {
+
+ ret = replmd_add_backlink(module, replmd_private, schema,
+ src_obj_dn, target_guid, false,
+ attr, NULL);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+ }
+
+ /* mark the existing value as deleted */
+ ret = replmd_update_la_val(mem_ctx, output_val, target_dsdb_dn,
+ target_dsdb_dn, invocation_id, seq_num,
+ seq_num, now, true);
+ return ret;
+}
+
/*
process one linked attribute structure
*/
@@ -7244,6 +7292,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
struct ldb_message_element *old_el;
time_t t = time(NULL);
struct parsed_dn *pdn_list, *pdn, *next;
+ struct parsed_dn *conflict_pdn = NULL;
struct GUID guid = GUID_zero();
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
bool ignore_link;
@@ -7335,17 +7384,11 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
*/
ret = replmd_get_active_singleval_link(module, tmp_ctx, pdn_list,
old_el->num_values, attr,
- &pdn);
+ &conflict_pdn);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
}
-
- if (pdn != NULL) {
- DBG_WARNING("Link conflict for %s linked from %s\n",
- ldb_dn_get_linearized(pdn->dsdb_dn->dn),
- ldb_dn_get_linearized(msg->dn));
- }
}
if (!replmd_link_update_is_newer(pdn, la)) {
@@ -7363,6 +7406,38 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
+ /* resolve any single-valued link conflicts */
+ if (conflict_pdn != NULL) {
+
+ DBG_WARNING("Link conflict for %s attribute on %s\n",
+ attr->lDAPDisplayName, ldb_dn_get_linearized(msg->dn));
+
+ if (replmd_link_update_is_newer(conflict_pdn, la)) {
+ DBG_WARNING("Using received value %s, over existing target %s\n",
+ ldb_dn_get_linearized(dsdb_dn->dn),
+ ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn));
+
+ ret = replmd_delete_link_value(module, replmd_private,
+ old_el, msg->dn, schema,
+ attr, seq_num, true,
+ &conflict_pdn->guid,
+ conflict_pdn->dsdb_dn,
+ conflict_pdn->v);
+
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
+ }
+ } else {
+ DBG_WARNING("Using existing target %s, over received value %s\n",
+ ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
+ ldb_dn_get_linearized(dsdb_dn->dn));
+
+ /* don't add the link as active */
+ active = false;
+ }
+ }
+
if (pdn != NULL) {
uint32_t rmd_flags = dsdb_dn_rmd_flags(pdn->dsdb_dn->dn);
--
2.7.4
From ddc01311919539fea1bba6ee2ce3feb5f879d5ae Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Sep 2017 16:23:29 +1300
Subject: [PATCH 10/22] replmd: Use replmd_set_la_val() when adding new links
replmd_set_la_val() and replmd_build_la_val() are almost identical. When
we were processing the replicated link attributes we were calling one
function if the link was new, and a different one if the link existed.
I think we should be able to get away with using replmd_set_la_val() in
both cases.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 39 ++++++++++++-------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 80a5cad..2dac151 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7297,6 +7297,8 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
bool ignore_link;
enum deletion_state deletion_state = OBJECT_NOT_DELETED;
+ struct dsdb_dn *old_dsdb_dn = NULL;
+ struct ldb_val *val_to_update = NULL;
/*
* get the attribute being modified, the search result for the source object,
@@ -7454,16 +7456,9 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
}
}
- ret = replmd_set_la_val(tmp_ctx, pdn->v, dsdb_dn, pdn->dsdb_dn,
- &la->meta_data.originating_invocation_id,
- la->meta_data.originating_usn, seq_num,
- la->meta_data.originating_change_time,
- la->meta_data.version,
- !active);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
+ val_to_update = pdn->v;
+ old_dsdb_dn = pdn->dsdb_dn;
+
} else {
unsigned offset;
@@ -7502,16 +7497,20 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
old_el->num_values++;
- ret = replmd_build_la_val(tmp_ctx, &old_el->values[offset], dsdb_dn,
- &la->meta_data.originating_invocation_id,
- la->meta_data.originating_usn, seq_num,
- la->meta_data.originating_change_time,
- la->meta_data.version,
- !active);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
+ val_to_update = &old_el->values[offset];
+ old_dsdb_dn = dsdb_dn;
+ }
+
+ /* set the link attribute's value to the info that was received */
+ ret = replmd_set_la_val(tmp_ctx, val_to_update, dsdb_dn, old_dsdb_dn,
+ &la->meta_data.originating_invocation_id,
+ la->meta_data.originating_usn, seq_num,
+ la->meta_data.originating_change_time,
+ la->meta_data.version,
+ !active);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
}
/* if the new link is active, then add the new backlink */
--
2.7.4
From 433c0df30bc8fad5f47bb3877c4403dc12df6589 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 27 Sep 2017 16:37:59 +1300
Subject: [PATCH 11/22] replmd: Mark link conflicts as inactive correctly
The previous patch to handle link conflicts was simply overriding the
received information and marking the link as deleted. We should be doing
this as a separate operation to make it clear what has happened, and so
that the new (i.e. inactive) link details get replicated out.
This patch changes it so that when a conflict occurs, we immediately
overwrite the received information to mark it as deleted, and to update
the version/USN/timestamp/originating_invocation_id to make it clear
that this is a new change.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 26 +++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 2dac151..892cf4d 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7299,6 +7299,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
enum deletion_state deletion_state = OBJECT_NOT_DELETED;
struct dsdb_dn *old_dsdb_dn = NULL;
struct ldb_val *val_to_update = NULL;
+ bool add_as_inactive = false;
/*
* get the attribute being modified, the search result for the source object,
@@ -7435,8 +7436,11 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
ldb_dn_get_linearized(dsdb_dn->dn));
- /* don't add the link as active */
- active = false;
+ /*
+ * we want to keep our existing active link and add the
+ * received link as inactive
+ */
+ add_as_inactive = true;
}
}
@@ -7513,8 +7517,22 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
- /* if the new link is active, then add the new backlink */
- if (active) {
+ if (add_as_inactive) {
+
+ /* Set the new link as inactive/deleted to avoid conflicts */
+ ret = replmd_delete_link_value(module, replmd_private, old_el,
+ msg->dn, schema, attr, seq_num,
+ false, &guid, dsdb_dn,
+ val_to_update);
+
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
+ }
+
+ } else if (active) {
+
+ /* if the new link is active, then add the new backlink */
ret = replmd_add_backlink(module, replmd_private,
schema,
msg->dn,
--
2.7.4
From 8712cd1509c756463261a6bd9cb77c1d388dc3a7 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 09:42:14 +1300
Subject: [PATCH 12/22] replmd: Handle single-valued conflicts for an existing
link
Currently the code only handles the case where the received link
attribute is a new link (i.e. pdn == NULL). As well as this, we need to
handle the case where the conflicting link already exists, i.e. it's a
deleted link that has been re-added on another DC.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
selftest/knownfail.d/link_conflicts | 4 ----
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 18 +++++++++++-------
source4/torture/drs/python/link_conflicts.py | 7 ++-----
3 files changed, 13 insertions(+), 16 deletions(-)
delete mode 100644 selftest/knownfail.d/link_conflicts
diff --git a/selftest/knownfail.d/link_conflicts b/selftest/knownfail.d/link_conflicts
deleted file mode 100644
index a927997..0000000
--- a/selftest/knownfail.d/link_conflicts
+++ /dev/null
@@ -1,4 +0,0 @@
-# Currently Samba can't resolve a conflict for a single-valued link attribute
-samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_existing_single_valued_link\(vampire_dc\)
-samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_conflict_existing_single_valued_link\(promoted_dc\)
-
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 892cf4d..b060940 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7378,20 +7378,24 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
- if (pdn == NULL && active) {
+ /*
+ * check if there's a conflict for single-valued links, i.e. an active
+ * linked attribute already exists, but it has a different target value
+ */
+ if (active) {
+ struct parsed_dn *active_pdn = NULL;
- /*
- * check if there's a conflict for single-valued links, i.e.
- * an active linked attribute already exists, but it has a
- * different target value
- */
ret = replmd_get_active_singleval_link(module, tmp_ctx, pdn_list,
old_el->num_values, attr,
- &conflict_pdn);
+ &active_pdn);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
}
+
+ if (active_pdn != pdn) {
+ conflict_pdn = active_pdn;
+ }
}
if (!replmd_link_update_is_newer(pdn, la)) {
diff --git a/source4/torture/drs/python/link_conflicts.py b/source4/torture/drs/python/link_conflicts.py
index 036472b..c8b6556 100644
--- a/source4/torture/drs/python/link_conflicts.py
+++ b/source4/torture/drs/python/link_conflicts.py
@@ -668,11 +668,8 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self.ensure_unique_timestamp()
self.add_link_attr(self.ldb_dc2, src_ou, "managedBy", target2_ou)
- # try to sync the 2 DCs (this currently fails)
- try:
- self.sync_DCs(sync_order=sync_order)
- except Exception, e:
- self.fail("Replication could not resolve link conflict: %s" % e)
+ # try to sync the 2 DCs
+ self.sync_DCs(sync_order=sync_order)
res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
scope=SCOPE_BASE, attrs=["managedBy"])
--
2.7.4
From 64fb5ffa612831eda7d6b0defd4b21be81e71f69 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 10:22:55 +1300
Subject: [PATCH 13/22] replmd: Move link conflict handling into separate
function
Link conflict handling is a corner-case. The logic in
replmd_process_linked_attribute() is already reasonably busy/complex.
Split out the handling of link conflicts into a separate function so
that it doesn't detract from the core replmd_process_linked_attribute()
logic too much.
This refactor should not alter functionality.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 151 +++++++++++++++---------
1 file changed, 98 insertions(+), 53 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index b060940..f2c3a61 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7272,6 +7272,90 @@ static int replmd_delete_link_value(struct ldb_module *module,
return ret;
}
+/**
+ * Checks for a conflict in single-valued link attributes, and tries to
+ * resolve the problem if possible. Single-valued links should only ever have
+ * one active value. If we already have an active link value, and during
+ * replication we receive an active link value for a different target DN, then
+ * we need to resolve this inconsistency and determine which value should be
+ * active. If the received info is better/newer than the existing link
+ * attribute, then we need to set our existing link as deleted. If the received
+ * info is worse/older, then we continue to add it, but set it as an inactive
+ * link (we set @param add_as_inactive to true in this case).
+ * Note that this is a corner-case that is unlikely to happen (but if it does
+ * happen, we don't want it to break replication completely).
+ */
+static int replmd_check_singleval_la_conflict(struct ldb_module *module,
+ struct replmd_private *replmd_private,
+ TALLOC_CTX *mem_ctx,
+ struct ldb_dn *src_obj_dn,
+ struct drsuapi_DsReplicaLinkedAttribute *la,
+ struct dsdb_dn *dsdb_dn,
+ struct parsed_dn *pdn,
+ struct parsed_dn *pdn_list,
+ struct ldb_message_element *old_el,
+ const struct dsdb_schema *schema,
+ const struct dsdb_attribute *attr,
+ uint64_t seq_num,
+ bool *add_as_inactive)
+{
+ struct parsed_dn *active_pdn = NULL;
+ struct parsed_dn *conflict_pdn = NULL;
+ int ret;
+
+ /*
+ * check if there's a conflict for single-valued links, i.e. an active
+ * linked attribute already exists, but it has a different target value
+ */
+ ret = replmd_get_active_singleval_link(module, mem_ctx, pdn_list,
+ old_el->num_values, attr,
+ &active_pdn);
+
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
+ if (active_pdn != pdn) {
+ conflict_pdn = active_pdn;
+ }
+
+ /* resolve any single-valued link conflicts */
+ if (conflict_pdn != NULL) {
+
+ DBG_WARNING("Link conflict for %s attribute on %s\n",
+ attr->lDAPDisplayName, ldb_dn_get_linearized(src_obj_dn));
+
+ if (replmd_link_update_is_newer(conflict_pdn, la)) {
+ DBG_WARNING("Using received value %s, over existing target %s\n",
+ ldb_dn_get_linearized(dsdb_dn->dn),
+ ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn));
+
+ ret = replmd_delete_link_value(module, replmd_private,
+ old_el, src_obj_dn, schema,
+ attr, seq_num, true,
+ &conflict_pdn->guid,
+ conflict_pdn->dsdb_dn,
+ conflict_pdn->v);
+
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+ } else {
+ DBG_WARNING("Using existing target %s, over received value %s\n",
+ ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
+ ldb_dn_get_linearized(dsdb_dn->dn));
+
+ /*
+ * we want to keep our existing active link and add the
+ * received link as inactive
+ */
+ *add_as_inactive = true;
+ }
+ }
+
+ return LDB_SUCCESS;
+}
+
/*
process one linked attribute structure
*/
@@ -7292,7 +7376,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
struct ldb_message_element *old_el;
time_t t = time(NULL);
struct parsed_dn *pdn_list, *pdn, *next;
- struct parsed_dn *conflict_pdn = NULL;
struct GUID guid = GUID_zero();
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
bool ignore_link;
@@ -7378,26 +7461,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
- /*
- * check if there's a conflict for single-valued links, i.e. an active
- * linked attribute already exists, but it has a different target value
- */
- if (active) {
- struct parsed_dn *active_pdn = NULL;
-
- ret = replmd_get_active_singleval_link(module, tmp_ctx, pdn_list,
- old_el->num_values, attr,
- &active_pdn);
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
-
- if (active_pdn != pdn) {
- conflict_pdn = active_pdn;
- }
- }
-
if (!replmd_link_update_is_newer(pdn, la)) {
DEBUG(3,("Discarding older DRS linked attribute update to %s on %s from %s\n",
old_el->name, ldb_dn_get_linearized(msg->dn),
@@ -7413,38 +7476,20 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return ret;
}
- /* resolve any single-valued link conflicts */
- if (conflict_pdn != NULL) {
-
- DBG_WARNING("Link conflict for %s attribute on %s\n",
- attr->lDAPDisplayName, ldb_dn_get_linearized(msg->dn));
-
- if (replmd_link_update_is_newer(conflict_pdn, la)) {
- DBG_WARNING("Using received value %s, over existing target %s\n",
- ldb_dn_get_linearized(dsdb_dn->dn),
- ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn));
-
- ret = replmd_delete_link_value(module, replmd_private,
- old_el, msg->dn, schema,
- attr, seq_num, true,
- &conflict_pdn->guid,
- conflict_pdn->dsdb_dn,
- conflict_pdn->v);
-
- if (ret != LDB_SUCCESS) {
- talloc_free(tmp_ctx);
- return ret;
- }
- } else {
- DBG_WARNING("Using existing target %s, over received value %s\n",
- ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
- ldb_dn_get_linearized(dsdb_dn->dn));
-
- /*
- * we want to keep our existing active link and add the
- * received link as inactive
- */
- add_as_inactive = true;
+ /*
+ * check for single-valued link conflicts, i.e. an active linked
+ * attribute already exists, but it has a different target value
+ */
+ if (active) {
+ ret = replmd_check_singleval_la_conflict(module, replmd_private,
+ tmp_ctx, msg->dn, la,
+ dsdb_dn, pdn, pdn_list,
+ old_el, schema, attr,
+ seq_num,
+ &add_as_inactive);
+ if (ret != LDB_SUCCESS) {
+ talloc_free(tmp_ctx);
+ return ret;
}
}
--
2.7.4
From 19de01926cdae160093dde25a12f99375d9b5d76 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 12:01:34 +1300
Subject: [PATCH 14/22] replmd: Change replmd_check_singleval_la_conflict()
logic flow
Return immediately if there's no conflict, which reduces nesting.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 54 ++++++++++++-------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index f2c3a61..17588ff 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7319,38 +7319,38 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
conflict_pdn = active_pdn;
}
- /* resolve any single-valued link conflicts */
- if (conflict_pdn != NULL) {
-
- DBG_WARNING("Link conflict for %s attribute on %s\n",
- attr->lDAPDisplayName, ldb_dn_get_linearized(src_obj_dn));
+ if (conflict_pdn == NULL) {
+ return LDB_SUCCESS;
+ }
- if (replmd_link_update_is_newer(conflict_pdn, la)) {
- DBG_WARNING("Using received value %s, over existing target %s\n",
- ldb_dn_get_linearized(dsdb_dn->dn),
- ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn));
+ /* resolve any single-valued link conflicts */
+ DBG_WARNING("Link conflict for %s attribute on %s\n",
+ attr->lDAPDisplayName, ldb_dn_get_linearized(src_obj_dn));
- ret = replmd_delete_link_value(module, replmd_private,
- old_el, src_obj_dn, schema,
- attr, seq_num, true,
- &conflict_pdn->guid,
- conflict_pdn->dsdb_dn,
- conflict_pdn->v);
+ if (replmd_link_update_is_newer(conflict_pdn, la)) {
+ DBG_WARNING("Using received value %s, over existing target %s\n",
+ ldb_dn_get_linearized(dsdb_dn->dn),
+ ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn));
- if (ret != LDB_SUCCESS) {
- return ret;
- }
- } else {
- DBG_WARNING("Using existing target %s, over received value %s\n",
- ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
- ldb_dn_get_linearized(dsdb_dn->dn));
+ ret = replmd_delete_link_value(module, replmd_private, old_el,
+ src_obj_dn, schema, attr,
+ seq_num, true, &conflict_pdn->guid,
+ conflict_pdn->dsdb_dn,
+ conflict_pdn->v);
- /*
- * we want to keep our existing active link and add the
- * received link as inactive
- */
- *add_as_inactive = true;
+ if (ret != LDB_SUCCESS) {
+ return ret;
}
+ } else {
+ DBG_WARNING("Using existing target %s, over received value %s\n",
+ ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
+ ldb_dn_get_linearized(dsdb_dn->dn));
+
+ /*
+ * we want to keep our existing active link and add the
+ * received link as inactive
+ */
+ *add_as_inactive = true;
}
return LDB_SUCCESS;
--
2.7.4
From eb537da62a1870167a8c31b2971ff6f709da2e8d Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 12:10:11 +1300
Subject: [PATCH 15/22] replmd: Small refactor to
replmd_check_singleval_la_conflict()
Now that the code is all in one place we can refactor it to make it
slightly more readable.
- added more code comments
- tweaked the 'no conflict' return logic to try to make what it's checking
for more obvious
- removed conflict_pdn (we can just use active_pdn instead)
- added a placeholder variable and tweaked a parameter name
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13055
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 43 ++++++++++++++++---------
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 17588ff..28bc682 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7280,10 +7280,15 @@ static int replmd_delete_link_value(struct ldb_module *module,
* we need to resolve this inconsistency and determine which value should be
* active. If the received info is better/newer than the existing link
* attribute, then we need to set our existing link as deleted. If the received
- * info is worse/older, then we continue to add it, but set it as an inactive
- * link (we set @param add_as_inactive to true in this case).
+ * info is worse/older, then we should continue to add it, but set it as an
+ * inactive link (we set @param add_as_inactive to true in this case).
* Note that this is a corner-case that is unlikely to happen (but if it does
* happen, we don't want it to break replication completely).
+ * @param pdn_being_modified the parsed DN corresponding to the received link
+ * target (note this is NULL if the link does not already exist in our DB)
+ * @param pdn_list all the source object's Parsed-DNs for this attribute, i.e.
+ * any existing active or inactive values for the attribute in our DB.
+ * @param dsdb_dn the target DN for the received link attribute
*/
static int replmd_check_singleval_la_conflict(struct ldb_module *module,
struct replmd_private *replmd_private,
@@ -7291,7 +7296,7 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
struct ldb_dn *src_obj_dn,
struct drsuapi_DsReplicaLinkedAttribute *la,
struct dsdb_dn *dsdb_dn,
- struct parsed_dn *pdn,
+ struct parsed_dn *pdn_being_modified,
struct parsed_dn *pdn_list,
struct ldb_message_element *old_el,
const struct dsdb_schema *schema,
@@ -7300,7 +7305,7 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
bool *add_as_inactive)
{
struct parsed_dn *active_pdn = NULL;
- struct parsed_dn *conflict_pdn = NULL;
+ bool update_is_newer = false;
int ret;
/*
@@ -7315,35 +7320,41 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
return ret;
}
- if (active_pdn != pdn) {
- conflict_pdn = active_pdn;
- }
-
- if (conflict_pdn == NULL) {
+ /*
+ * If no active value exists (or the received info is for the currently
+ * active value), then no conflict exists
+ */
+ if (active_pdn == NULL || active_pdn == pdn_being_modified) {
return LDB_SUCCESS;
}
- /* resolve any single-valued link conflicts */
DBG_WARNING("Link conflict for %s attribute on %s\n",
attr->lDAPDisplayName, ldb_dn_get_linearized(src_obj_dn));
- if (replmd_link_update_is_newer(conflict_pdn, la)) {
+ /* Work out how to resolve the conflict based on which info is better */
+ update_is_newer = replmd_link_update_is_newer(active_pdn, la);
+
+ if (update_is_newer) {
DBG_WARNING("Using received value %s, over existing target %s\n",
ldb_dn_get_linearized(dsdb_dn->dn),
- ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn));
+ ldb_dn_get_linearized(active_pdn->dsdb_dn->dn));
+ /*
+ * Delete our existing active link. The received info will then
+ * be added (through normal link processing) as the active value
+ */
ret = replmd_delete_link_value(module, replmd_private, old_el,
src_obj_dn, schema, attr,
- seq_num, true, &conflict_pdn->guid,
- conflict_pdn->dsdb_dn,
- conflict_pdn->v);
+ seq_num, true, &active_pdn->guid,
+ active_pdn->dsdb_dn,
+ active_pdn->v);
if (ret != LDB_SUCCESS) {
return ret;
}
} else {
DBG_WARNING("Using existing target %s, over received value %s\n",
- ldb_dn_get_linearized(conflict_pdn->dsdb_dn->dn),
+ ldb_dn_get_linearized(active_pdn->dsdb_dn->dn),
ldb_dn_get_linearized(dsdb_dn->dn));
/*
--
2.7.4
From 80f2a175ef5d40a62c8918682a3f058946566b21 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 14:42:08 +1300
Subject: [PATCH 16/22] selftest: Add test for initial link attribute
RMD_VERSION value
While testing link conflicts I noticed that links on Windows start from
a different RMD_VERSION compared to Samba. This adds a simple test to
highlight the problem.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13059
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
selftest/knownfail.d/link_conflicts | 5 ++++
source4/torture/drs/python/link_conflicts.py | 37 ++++++++++++++++++++++++----
2 files changed, 37 insertions(+), 5 deletions(-)
create mode 100644 selftest/knownfail.d/link_conflicts
diff --git a/selftest/knownfail.d/link_conflicts b/selftest/knownfail.d/link_conflicts
new file mode 100644
index 0000000..8c1c94f
--- /dev/null
+++ b/selftest/knownfail.d/link_conflicts
@@ -0,0 +1,5 @@
+# Samba starts its RMD_VERSION from zero. Windows starts from one.
+samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_link_attr_version\(vampire_dc\)
+samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_link_attr_version\(promoted_dc\)
+
+
diff --git a/source4/torture/drs/python/link_conflicts.py b/source4/torture/drs/python/link_conflicts.py
index c8b6556..6522fb6 100644
--- a/source4/torture/drs/python/link_conflicts.py
+++ b/source4/torture/drs/python/link_conflicts.py
@@ -141,14 +141,16 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self.assertTrue(val in res2[0][attr],
"%s '%s' not found on DC2" %(attr, val))
- def _check_replicated_links(self, src_obj_dn, expected_links):
- """Checks that replication sends back the expected linked attributes"""
-
+ def zero_highwatermark(self):
+ """Returns a zeroed highwatermark so that all DRS data gets returned"""
hwm = drsuapi.DsReplicaHighWaterMark()
hwm.tmp_highest_usn = 0
hwm.reserved_usn = 0
hwm.highest_usn = 0
+ return hwm
+ def _check_replicated_links(self, src_obj_dn, expected_links):
+ """Checks that replication sends back the expected linked attributes"""
self._check_replication([src_obj_dn],
drsuapi.DRSUAPI_DRS_WRIT_REP,
dest_dsa=None,
@@ -156,7 +158,7 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
nc_dn_str=src_obj_dn,
exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
expected_links=expected_links,
- highwatermark=hwm)
+ highwatermark=self.zero_highwatermark())
# Check DC2 as well
self.set_test_ldb_dc(self.ldb_dc2)
@@ -168,7 +170,7 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
nc_dn_str=src_obj_dn,
exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
expected_links=expected_links,
- highwatermark=hwm,
+ highwatermark=self.zero_highwatermark(),
drs=self.drs2, drs_handle=self.drs2_handle)
self.set_test_ldb_dc(self.ldb_dc1)
@@ -698,3 +700,28 @@ class DrsReplicaLinkConflictTestCase(drs_base.DrsBaseTestCase):
self._test_conflict_existing_single_valued_link(sync_order=DC1_TO_DC2)
self._test_conflict_existing_single_valued_link(sync_order=DC2_TO_DC1)
+ def test_link_attr_version(self):
+ """
+ Checks the link attribute version starts from the correct value
+ """
+ # create some objects and add a link
+ src_ou = self.unique_dn("OU=src")
+ src_guid = self.add_object(self.ldb_dc1, src_ou)
+ target1_ou = self.unique_dn("OU=target1")
+ target1_guid = self.add_object(self.ldb_dc1, target1_ou)
+ self.add_link_attr(self.ldb_dc1, src_ou, "managedBy", target1_ou)
+
+ # get the link info via replication
+ ctr6 = self._get_replication(drsuapi.DRSUAPI_DRS_WRIT_REP,
+ dest_dsa=None,
+ drs_error=drsuapi.DRSUAPI_EXOP_ERR_SUCCESS,
+ exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ,
+ highwatermark=self.zero_highwatermark(),
+ nc_dn_str=src_ou)
+
+ self.assertTrue(ctr6.linked_attributes_count == 1,
+ "DRS didn't return a link")
+ link = ctr6.linked_attributes[0]
+ self.assertTrue(link.meta_data.version == 1,
+ "Link version started from %u, not 1" % link.meta_data.version)
+
--
2.7.4
From 71f345bd479d3b68f3f60667eb8a099676d9f03e Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 15:01:21 +1300
Subject: [PATCH 17/22] replmd: Remove static values passed to
replmd_build_la_val()
replmd_build_la_val() is used to populate a new link attribute value
from scratch. The version parameter is always passed in as the initial
value (zero), and deleted is always passed in as false.
For cases (like replication) where we want to set version/deleted to
something other than the defaults, we can use replmd_set_la_val()
instead.
This patch changes these 2 parameters to variables instead.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13059
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 28bc682..f57e3b3 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -923,7 +923,7 @@ static void replmd_ldb_message_sort(struct ldb_message *msg,
static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb_dn *dsdb_dn,
const struct GUID *invocation_id, uint64_t seq_num,
- uint64_t local_usn, NTTIME nttime, uint32_t version, bool deleted);
+ uint64_t local_usn, NTTIME nttime);
static int parsed_dn_compare(struct parsed_dn *pdn1, struct parsed_dn *pdn2);
@@ -994,7 +994,7 @@ static int replmd_add_fix_la(struct ldb_module *module, TALLOC_CTX *mem_ctx,
}
ret = replmd_build_la_val(el->values, p->v, p->dsdb_dn,
&ac->our_invocation_id,
- ac->seq_num, ac->seq_num, now, 0, false);
+ ac->seq_num, ac->seq_num, now);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
@@ -2138,7 +2138,7 @@ static int get_parsed_dns_trusted(struct ldb_module *module,
*/
static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb_dn *dsdb_dn,
const struct GUID *invocation_id, uint64_t seq_num,
- uint64_t local_usn, NTTIME nttime, uint32_t version, bool deleted)
+ uint64_t local_usn, NTTIME nttime)
{
struct ldb_dn *dn = dsdb_dn->dn;
const char *tstring, *usn_string, *flags_string;
@@ -2150,7 +2150,8 @@ static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct ds
int ret;
const char *dnstring;
char *vstring;
- uint32_t rmd_flags = deleted?DSDB_RMD_FLAG_DELETED:0;
+ uint32_t version = 0;
+ uint32_t rmd_flags = 0;
tstring = talloc_asprintf(mem_ctx, "%llu", (unsigned long long)nttime);
if (!tstring) {
@@ -2597,8 +2598,7 @@ static int replmd_modify_la_add(struct ldb_module *module,
/* Make the new linked attribute ldb_val. */
ret = replmd_build_la_val(new_values, &new_values[num_values],
dns[i].dsdb_dn, invocation_id,
- seq_num, seq_num,
- now, 0, false);
+ seq_num, seq_num, now);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
@@ -3080,8 +3080,7 @@ static int replmd_modify_la_replace(struct ldb_module *module,
new_p->v,
new_p->dsdb_dn,
invocation_id,
- seq_num, seq_num,
- now, 0, false);
+ seq_num, seq_num, now);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
--
2.7.4
From 8b0c8172c6d2309a51e487917d7d4006170c27d8 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 15:09:34 +1300
Subject: [PATCH 18/22] replmd: Fix RMD_VERSION inital value to match Windows
The initial value for RMD_VERSION is one on Windows. The MS-DRSR spec
states the following:
dwVersion: A 32-bit integer. Set to 1 when a value for the attribute is
set for the first time. On each subsequent originating update, if the
current value of dwVersion is less than 0xFFFFFFFF, then increment it
by 1; otherwise set it to 0
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13059
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
selftest/knownfail.d/link_conflicts | 5 -----
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 7 +++++--
2 files changed, 5 insertions(+), 7 deletions(-)
delete mode 100644 selftest/knownfail.d/link_conflicts
diff --git a/selftest/knownfail.d/link_conflicts b/selftest/knownfail.d/link_conflicts
deleted file mode 100644
index 8c1c94f..0000000
--- a/selftest/knownfail.d/link_conflicts
+++ /dev/null
@@ -1,5 +0,0 @@
-# Samba starts its RMD_VERSION from zero. Windows starts from one.
-samba4.drs.link_conflicts.python\(vampire_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_link_attr_version\(vampire_dc\)
-samba4.drs.link_conflicts.python\(promoted_dc\).link_conflicts.DrsReplicaLinkConflictTestCase.test_link_attr_version\(promoted_dc\)
-
-
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index f57e3b3..34d548e 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -54,6 +54,9 @@
#undef DBGC_CLASS
#define DBGC_CLASS DBGC_DRS_REPL
+/* the RMD_VERSION for linked attributes starts from 1 */
+#define RMD_VERSION_INITIAL 1
+
/*
* It's 29/12/9999 at 23:59:59 UTC as specified in MS-ADTS 7.1.1.4.2
* Deleted Objects Container
@@ -2150,7 +2153,7 @@ static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct ds
int ret;
const char *dnstring;
char *vstring;
- uint32_t version = 0;
+ uint32_t version = RMD_VERSION_INITIAL;
uint32_t rmd_flags = 0;
tstring = talloc_asprintf(mem_ctx, "%llu", (unsigned long long)nttime);
@@ -2394,7 +2397,7 @@ static int replmd_update_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct d
bool deleted)
{
uint32_t old_version;
- uint32_t version = 0;
+ uint32_t version = RMD_VERSION_INITIAL;
NTSTATUS status;
/*
--
2.7.4
From ed2c746d2bc7123caf4359a390cfff122a4265c7 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 15:58:16 +1300
Subject: [PATCH 19/22] replmd: Make replmd_set_la_val() closer to
replmd_build_la_val()
These two functions are almost identical. The main difference between
them is the RMD_ADDTIME. replmd_set_la_val() tries to use the
RMD_ADDTIME of the old_dsdb_dn. Whereas replmd_build_la_val() always
uses the time passed in.
Change replmd_set_la_val() so it can accept a NULL old_dsdb_dn (i.e. if
it's a new linked attribute that's being set). If so, it'll end up using
the nttime parameter passed in, same as replmd_build_la_val() does.
Also update replmd_process_linked_attribute (which used to use
replmd_build_la_val()) to now pass in a NULL old_dsdb_dn. There
shouldn't be a difference in behaviour either way, but this exercises
the code change.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 34d548e..1707117 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -2309,7 +2309,7 @@ static int replmd_set_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb
struct ldb_val iid;
struct ldb_val usnv, local_usnv;
struct ldb_val vers, flagsv;
- const struct ldb_val *old_addtime;
+ const struct ldb_val *old_addtime = NULL;
NTSTATUS status;
int ret;
const char *dnstring;
@@ -2349,7 +2349,10 @@ static int replmd_set_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb
if (ret != LDB_SUCCESS) return ret;
/* get the ADDTIME from the original */
- old_addtime = ldb_dn_get_extended_component(old_dsdb_dn->dn, "RMD_ADDTIME");
+ if (old_dsdb_dn != NULL) {
+ old_addtime = ldb_dn_get_extended_component(old_dsdb_dn->dn,
+ "RMD_ADDTIME");
+ }
if (old_addtime == NULL) {
old_addtime = &tval;
}
@@ -7564,7 +7567,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
old_el->num_values++;
val_to_update = &old_el->values[offset];
- old_dsdb_dn = dsdb_dn;
+ old_dsdb_dn = NULL;
}
/* set the link attribute's value to the info that was received */
--
2.7.4
From 5c04ff697f50d17dd0ccaac33e115d0fee9423c4 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 16:19:29 +1300
Subject: [PATCH 20/22] replmd: Fix talloc inconsistency in replmd_set_la_val()
All the other talloc_asprintf()s in this function use the mem_ctx, but
for some reason the vstring was using the dsdb_dn->dn. This probably
isn't a big deal, but might have unintentional side-effects.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 1707117..40948ef 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -2377,7 +2377,7 @@ static int replmd_set_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb
ret = ldb_dn_set_extended_component(dn, "RMD_LOCAL_USN", &local_usnv);
if (ret != LDB_SUCCESS) return ret;
- vstring = talloc_asprintf(dn, "%lu", (unsigned long)version);
+ vstring = talloc_asprintf(mem_ctx, "%lu", (unsigned long)version);
vers = data_blob_string_const(vstring);
ret = ldb_dn_set_extended_component(dn, "RMD_VERSION", &vers);
if (ret != LDB_SUCCESS) return ret;
--
2.7.4
From eca257f17b051fc6504677ed9fbf6fec2220af53 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 15:48:58 +1300
Subject: [PATCH 21/22] replmd: Get rid of duplicated replmd_build_la_val()
code
replmd_build_la_val() and replmd_set_la_val() are pretty much identical.
Keep the replmd_build_la_val() API (as it makes it clearer we're
creating a new linked attribute), but replace the code with a call to
replmd_set_la_val().
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 77 +++----------------------
1 file changed, 7 insertions(+), 70 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 40948ef..d1eedcc 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -122,6 +122,10 @@ static int replmd_check_upgrade_links(struct ldb_context *ldb,
const char *ldap_oid);
static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar,
struct la_entry *la);
+static int replmd_set_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb_dn *dsdb_dn,
+ struct dsdb_dn *old_dsdb_dn, const struct GUID *invocation_id,
+ uint64_t usn, uint64_t local_usn, NTTIME nttime,
+ uint32_t version, bool deleted);
enum urgent_situation {
REPL_URGENT_ON_CREATE = 1,
@@ -2143,76 +2147,9 @@ static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct ds
const struct GUID *invocation_id, uint64_t seq_num,
uint64_t local_usn, NTTIME nttime)
{
- struct ldb_dn *dn = dsdb_dn->dn;
- const char *tstring, *usn_string, *flags_string;
- struct ldb_val tval;
- struct ldb_val iid;
- struct ldb_val usnv, local_usnv;
- struct ldb_val vers, flagsv;
- NTSTATUS status;
- int ret;
- const char *dnstring;
- char *vstring;
- uint32_t version = RMD_VERSION_INITIAL;
- uint32_t rmd_flags = 0;
-
- tstring = talloc_asprintf(mem_ctx, "%llu", (unsigned long long)nttime);
- if (!tstring) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- tval = data_blob_string_const(tstring);
-
- usn_string = talloc_asprintf(mem_ctx, "%llu", (unsigned long long)seq_num);
- if (!usn_string) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- usnv = data_blob_string_const(usn_string);
-
- usn_string = talloc_asprintf(mem_ctx, "%llu", (unsigned long long)local_usn);
- if (!usn_string) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- local_usnv = data_blob_string_const(usn_string);
-
- vstring = talloc_asprintf(mem_ctx, "%lu", (unsigned long)version);
- if (!vstring) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- vers = data_blob_string_const(vstring);
-
- status = GUID_to_ndr_blob(invocation_id, dn, &iid);
- if (!NT_STATUS_IS_OK(status)) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
-
- flags_string = talloc_asprintf(mem_ctx, "%u", rmd_flags);
- if (!flags_string) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- flagsv = data_blob_string_const(flags_string);
-
- ret = ldb_dn_set_extended_component(dn, "RMD_FLAGS", &flagsv);
- if (ret != LDB_SUCCESS) return ret;
- ret = ldb_dn_set_extended_component(dn, "RMD_ADDTIME", &tval);
- if (ret != LDB_SUCCESS) return ret;
- ret = ldb_dn_set_extended_component(dn, "RMD_INVOCID", &iid);
- if (ret != LDB_SUCCESS) return ret;
- ret = ldb_dn_set_extended_component(dn, "RMD_CHANGETIME", &tval);
- if (ret != LDB_SUCCESS) return ret;
- ret = ldb_dn_set_extended_component(dn, "RMD_LOCAL_USN", &local_usnv);
- if (ret != LDB_SUCCESS) return ret;
- ret = ldb_dn_set_extended_component(dn, "RMD_ORIGINATING_USN", &usnv);
- if (ret != LDB_SUCCESS) return ret;
- ret = ldb_dn_set_extended_component(dn, "RMD_VERSION", &vers);
- if (ret != LDB_SUCCESS) return ret;
-
- dnstring = dsdb_dn_get_extended_linearized(mem_ctx, dsdb_dn, 1);
- if (dnstring == NULL) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- *v = data_blob_string_const(dnstring);
-
- return LDB_SUCCESS;
+ return replmd_set_la_val(mem_ctx, v, dsdb_dn, NULL, invocation_id,
+ seq_num, local_usn, nttime,
+ RMD_VERSION_INITIAL, false);
}
static int replmd_update_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb_dn *dsdb_dn,
--
2.7.4
From 151b5154e67a2a9dde1a2782a5b45ceffd9274e7 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 28 Sep 2017 16:13:05 +1300
Subject: [PATCH 22/22] replmd: Remove unnecessary replmd_build_la_val() param
replmd_build_la_val() is creating a new link attribute. In this case,
the RMD_ORIGINATING_USN and RMD_LOCAL_USN are always going to be the
same thing, so we don't need to pass them in as 2 separate parameters.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index d1eedcc..32c7abe 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -929,7 +929,7 @@ static void replmd_ldb_message_sort(struct ldb_message *msg,
}
static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb_dn *dsdb_dn,
- const struct GUID *invocation_id, uint64_t seq_num,
+ const struct GUID *invocation_id,
uint64_t local_usn, NTTIME nttime);
static int parsed_dn_compare(struct parsed_dn *pdn1, struct parsed_dn *pdn2);
@@ -1001,7 +1001,7 @@ static int replmd_add_fix_la(struct ldb_module *module, TALLOC_CTX *mem_ctx,
}
ret = replmd_build_la_val(el->values, p->v, p->dsdb_dn,
&ac->our_invocation_id,
- ac->seq_num, ac->seq_num, now);
+ ac->seq_num, now);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
@@ -2143,12 +2143,13 @@ static int get_parsed_dns_trusted(struct ldb_module *module,
RMD_LOCAL_USN = local_usn
RMD_VERSION = version
*/
-static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v, struct dsdb_dn *dsdb_dn,
- const struct GUID *invocation_id, uint64_t seq_num,
+static int replmd_build_la_val(TALLOC_CTX *mem_ctx, struct ldb_val *v,
+ struct dsdb_dn *dsdb_dn,
+ const struct GUID *invocation_id,
uint64_t local_usn, NTTIME nttime)
{
return replmd_set_la_val(mem_ctx, v, dsdb_dn, NULL, invocation_id,
- seq_num, local_usn, nttime,
+ local_usn, local_usn, nttime,
RMD_VERSION_INITIAL, false);
}
@@ -2541,7 +2542,7 @@ static int replmd_modify_la_add(struct ldb_module *module,
/* Make the new linked attribute ldb_val. */
ret = replmd_build_la_val(new_values, &new_values[num_values],
dns[i].dsdb_dn, invocation_id,
- seq_num, seq_num, now);
+ seq_num, now);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
@@ -3023,7 +3024,7 @@ static int replmd_modify_la_replace(struct ldb_module *module,
new_p->v,
new_p->dsdb_dn,
invocation_id,
- seq_num, seq_num, now);
+ seq_num, now);
if (ret != LDB_SUCCESS) {
talloc_free(tmp_ctx);
return ret;
--
2.7.4
More information about the samba-technical
mailing list