389-ds-base

Clone
Source Code
GIT

#51133 Issue 51132 - Winsync setting winSyncWindowsFilter not working as expected
Closed 3 years ago by spichugi. Opened 3 years ago by mreynolds.
mreynolds/389-ds-base winsync-test  into  master

file modified
+9 -6 
@@ -48,7 +48,7 @@ 
 

  static int windows_get_remote_tombstone(Private_Repl_Protocol *prp, const Slapi_DN *remote_dn, Slapi_Entry **remote_entry);
 

  static int windows_reanimate_tombstone(Private_Repl_Protocol *prp, const Slapi_DN *tombstone_dn, const char *new_dn);
 

  static const char *op2string(int op);
 

- static int is_subject_of_agreement_remote(Slapi_Entry *e, const Repl_Agmt *ra);
 

+ static int is_subject_of_agreement_remote(Slapi_Entry *e, const Repl_Agmt *ra, int test_filter);
 

  static int map_entry_dn_inbound(Slapi_Entry *e, Slapi_DN **dn, const Repl_Agmt *ra);
 

  static int map_entry_dn_inbound_ext(Slapi_Entry *e, Slapi_DN **dn, const Repl_Agmt *ra, int use_guid, int user_username);
 

  static int windows_update_remote_entry(Private_Repl_Protocol *prp, Slapi_Entry *remote_entry, Slapi_Entry *local_entry, int is_user);
 
@@ -57,6 +57,9 @@ 
 

  static int windows_check_mods_for_rdn_change(Private_Repl_Protocol *prp, LDAPMod **original_mods, Slapi_Entry *local_entry, Slapi_DN *remote_dn, char **newrdn);
 

  static int windows_get_superior_change(Private_Repl_Protocol *prp, Slapi_DN *local_dn, Slapi_DN *remote_dn, char **newsuperior, int to_windows);
 

  

+ #define SKIP_FILTER 0
 

+ #define TEST_FILTER 1
 

+ 

  /* Controls the direction of flow for mapped attributes */
 

  typedef enum mapping_types {
 

      bidirectional,
 
@@ -442,7 +445,7 @@ 
 

              /* Try to get the remote entry */
 

              retval = windows_get_remote_entry(prp, original_dn, &remote_entry);
 

              if (remote_entry && 0 == retval) {
 

-                 is_ours = is_subject_of_agreement_remote(remote_entry, prp->agmt);
 

+                 is_ours = is_subject_of_agreement_remote(remote_entry, prp->agmt, TEST_FILTER);
 

                  if (is_ours) {
 

                      retval = map_entry_dn_inbound(remote_entry, &local_dn, prp->agmt);
 

                      if (0 == retval && local_dn) {
 
@@ -3701,7 +3704,7 @@ 
 

                        slapi_sdn_get_dn(new_dn),
 

                        remote_entry ? slapi_entry_get_dn_const(remote_entry) : "(null)");
 

          if (0 == rc && remote_entry) {
 

-             if (!is_subject_of_agreement_remote(remote_entry, prp->agmt)) {
 

+             if (!is_subject_of_agreement_remote(remote_entry, prp->agmt, TEST_FILTER)) {
 

                  /* The remote entry is out of scope of the agreement.
 

                   * Thus, we don't map the entry_dn.
 

                   * This occurs when the remote entry is moved out. */
 
@@ -4188,7 +4191,7 @@ 
 

   *               0 -- out of scope
 

   */
 

  static int
 

- is_subject_of_agreement_remote(Slapi_Entry *e, const Repl_Agmt *ra)
 

+ is_subject_of_agreement_remote(Slapi_Entry *e, const Repl_Agmt *ra, int test_filter)
 

  {
 

      int retval = 0;
 

      int is_in_subtree = 0;
 
@@ -4222,7 +4225,7 @@ 
 

          Slapi_DN psdn = {0};
 

          Slapi_Entry *pentry = NULL;
 

  

-         if (windows_private_get_windows_filter(ra) &&
 

+         if (test_filter && windows_private_get_windows_filter(ra) &&
 

              slapi_filter_test_simple(e, windows_private_get_windows_filter(ra))) {
 

              /* type_winSyncWindowsFilter is set and the remote entry does not match the filter */
 

              goto error;
 
@@ -5617,7 +5620,7 @@ 
 

          }
 

      } else {
 

          /* Is this entry one we should be interested in ? */
 

-         if (is_subject_of_agreement_remote(e, prp->agmt)) {
 

+         if (is_subject_of_agreement_remote(e, prp->agmt, SKIP_FILTER)) {
 

              ConnResult cres = 0;
 

              const char *searchbase = slapi_entry_get_dn_const(e);
 

              char *filter = "(objectclass=*)";

Bug Description:

When processing updates from AD we search AD using a filter, and this filter can be customized via the attribute setting: winSyncWindowsFilter. However, after setting a custom filter replication appears to stop working as expected. New entries that match the filter are replicated to DS, but not updates to these entries. The problem is that when dirsync sends updates, it is just a partial entry - only containing the attributes that changed. Then the server checks the filter again on the returned entry, but if it's just a mod update then the entry is missing most of its attributes, and the filter check fails and the entry is not updated on DS.

Fix Description:

Do not check the filter on the returned entries when processing incremental updates as the filter test was already done when gathering the candidates.

relates: https://pagure.io/389-ds-base/issue/51132

isn't there a bool type we can use?

Otherwise looks good to me :)

isn't there a bool type we can use?

Bool, what is that? :-) I can fit that change in I suppose...

Are you sure we should not evaluate the filter at this point. the entry comes from dirsync, that should return a full entry not a MOD

Are you sure we should not evaluate the filter at this point. the entry comes from dirsync, that should return a full entry not a MOD

It's not a full entry though. On a mod update we only send a partial entry. But feel free to setup winsync and see the behavior for itself. I can send you the details on how to do it...

Are you sure we should not evaluate the filter at this point. the entry comes from dirsync, that should return a full entry not a MOD

It's not a full entry though. On a mod update we only send a partial entry. But feel free to setup winsync and see the behavior for itself. I can send you the details on how to do it...

Let me try and explain this again. I thought the description was pretty good, but it's obviously not clear (my bad). When we get the entries from dirsync we use the fitter to get those entries from AD. The bug is that we re-evaluated the same filter on the entries we just grabbed using that filter(it's redundant for one), but the entries returned only contain the attributes that are modified.

So we call

windows_dirsync_inc_run() -> send_dirsync_search() -> windows_process_dirsync_entry() ->
is_subject_of_agreement_remote()

send_dirsync_search() gets entries from AD using the winFilter, so why do we need to check it again is one issue, the other issue is that entry is partial because it is just a mod, so the filter will fail on the partial modified entry. We can not check the partial entry using the filter, but we don't need to, we already used the filter to find the entry in the first place.

I hope this helps clear this up.

@mreynolds you initial description was good and I agree that checking again the filter is useless. What was surprising is that the returned entry is partial. Googling I did not find good description but I trust you :). It could also be a question of ACI that we can search but not read the attribute.

Anyway the fix looks valid to me. ACK

@mreynolds you initial description was good and I agree that checking again the filter is useless. What was surprising is that the returned entry is partial.

I was VERY surprised by this as well. But think about it, we only get "entries" from AD, we don't get "mods". So how do you represent a MOD? Just send a partial entry with what changed. Otherwise you need to delete and re-add the entry.

As for ACI's, we don't check ACI's for replicated operations, so why would we do it for winsync replication? And we do bind as cn=administrator,cn=users,dc=AD,dc=TEST to AD so we have full access anyway. So I don't think we need to worry about ACI's in this case.

The partial entry contains only the changed attributes. That is an elegant way to reduce the work of a client to synchronize the entry. It should only do MOD_REPL on received attributes.
I think it would make sense to support that control on 389-DS for example with sync_repl.

rebased onto 75c8de1
3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4186

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 1
+9 -6
file changed
ldap/servers/plugins/replication/windows_protocol_util.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.