#51204 Issue 49300 - entryUSN is duplicated after memberOf operation
Closed 2 years ago by spichugi. Opened 2 years ago by spichugi.
spichugi/389-ds-base i49300  into  master

@@ -0,0 +1,240 @@ 

+ # --- BEGIN COPYRIGHT BLOCK ---

+ # Copyright (C) 2020 Red Hat, Inc.

+ # All rights reserved.

+ #

+ # License: GPL (version 3 or any later version).

+ # See LICENSE for details.

+ # --- END COPYRIGHT BLOCK ---

+ #

+ import ldap

+ import logging

+ import pytest

+ from lib389._constants import DEFAULT_SUFFIX

+ from lib389.config import Config

+ from lib389.plugins import USNPlugin, MemberOfPlugin

+ from lib389.idm.group import Groups

+ from lib389.idm.user import UserAccounts

+ from lib389.idm.organizationalunit import OrganizationalUnit

+ from lib389.tombstone import Tombstones

+ from lib389.rootdse import RootDSE

+ from lib389.topologies import topology_st, topology_m2

+ 

+ log = logging.getLogger(__name__)

+ 

+ USER_NUM = 10

+ GROUP_NUM = 3

+ 

+ 

+ def check_entryusn_no_duplicates(entryusn_list):

+     """Check that all values in the list are unique"""

+ 

+     if len(entryusn_list) > len(set(entryusn_list)):

+         raise AssertionError(f"EntryUSN values have duplicates, please, check logs")

+ 

+ 

+ def check_lastusn_after_restart(inst):

+     """Check that last usn is the same after restart"""

+ 

+     root_dse = RootDSE(inst)

+     last_usn_before = root_dse.get_attr_val_int("lastusn;userroot")

+     inst.restart()

+     last_usn_after = root_dse.get_attr_val_int("lastusn;userroot")

+     assert last_usn_after == last_usn_before

+ 

+ 

+ @pytest.fixture(scope="module")

+ def setup(topology_st, request):

+     """

+     Enable USN plug-in

+     Enable MEMBEROF plugin

+     Add test entries

+     """

+ 

+     inst = topology_st.standalone

+ 

+     log.info("Enable the USN plugin...")

+     plugin = USNPlugin(inst)

+     plugin.enable()

+ 

+     log.info("Enable the MEMBEROF plugin...")

+     plugin = MemberOfPlugin(inst)

+     plugin.enable()

+ 

+     inst.restart()

+ 

+     users_list = []

+     log.info("Adding test entries...")

+     users = UserAccounts(inst, DEFAULT_SUFFIX)

+     for id in range(USER_NUM):

+         user = users.create_test_user(uid=id)

+         users_list.append(user)

+ 

+     groups_list = []

+     log.info("Adding test groups...")

+     groups = Groups(inst, DEFAULT_SUFFIX)

+     for id in range(GROUP_NUM):

+         group = groups.create(properties={'cn': f'test_group{id}'})

+         groups_list.append(group)

+ 

+     def fin():

+         for user in users_list:

+             try:

+                 user.delete()

+             except ldap.NO_SUCH_OBJECT:

+                 pass

+         for group in groups_list:

+             try:

+                 group.delete()

+             except ldap.NO_SUCH_OBJECT:

+                 pass

+     request.addfinalizer(fin)

+ 

+     return {"users": users_list,

+             "groups": groups_list}

+ 

+ 

+ def test_entryusn_no_duplicates(topology_st, setup):

+     """Verify that entryUSN is not duplicated after memberOf operation

+ 

+     :id: 1a7d382d-1214-4d56-b9c2-9c4ed57d1683

+     :setup: Standalone instance, Groups and Users, USN and memberOf are enabled

+     :steps:

+         1. Add a member to group 1

+         2. Add a member to group 1 and 2

+         3. Check that entryUSNs are different

+         4. Check that lastusn before and after a restart are the same

+     :expectedresults:

+         1. Success

+         2. Success

+         3. Success

+         4. Success

+     """

+ 

+     inst = topology_st.standalone

+     config = Config(inst)

+     config.replace('nsslapd-accesslog-level', '260')  # Internal op

+     config.replace('nsslapd-errorlog-level', '65536')

+     config.replace('nsslapd-plugin-logging', 'on')

+     entryusn_list = []

+ 

+     users = setup["users"]

+     groups = setup["groups"]

+ 

+     groups[0].replace('member', users[0].dn)

+     entryusn_list.append(users[0].get_attr_val_int('entryusn'))

+     log.info(f"{users[0].dn}_1: {entryusn_list[-1:]}")

+     entryusn_list.append(groups[0].get_attr_val_int('entryusn'))

+     log.info(f"{groups[0].dn}_1: {entryusn_list[-1:]}")

+     check_entryusn_no_duplicates(entryusn_list)

+ 

+     groups[1].replace('member', [users[0].dn, users[1].dn])

+     entryusn_list.append(users[0].get_attr_val_int('entryusn'))

+     log.info(f"{users[0].dn}_2: {entryusn_list[-1:]}")

+     entryusn_list.append(users[1].get_attr_val_int('entryusn'))

+     log.info(f"{users[1].dn}_2: {entryusn_list[-1:]}")

+     entryusn_list.append(groups[1].get_attr_val_int('entryusn'))

+     log.info(f"{groups[1].dn}_2: {entryusn_list[-1:]}")

+     check_entryusn_no_duplicates(entryusn_list)

+ 

+     check_lastusn_after_restart(inst)

+ 

+ 

+ def test_entryusn_is_same_after_failure(topology_st, setup):

+     """Verify that entryUSN is the same after failed operation

+ 

+     :id: 1f227533-370a-48c1-b920-9b3b0bcfc32e

+     :setup: Standalone instance, Groups and Users, USN and memberOf are enabled

+     :steps:

+         1. Get current group's entryUSN value

+         2. Try to modify the group with an invalid syntax

+         3. Get new group's entryUSN value and compare with old

+         4. Check that lastusn before and after a restart are the same

+     :expectedresults:

+         1. Success

+         2. Invalid Syntax error

+         3. Should be the same

+         4. Success

+     """

+ 

+     inst = topology_st.standalone

+     users = setup["users"]

+ 

+     # We need this update so we get the latest USN pointed to our entry

+     users[0].replace('description', 'update')

+ 

+     entryusn_before = users[0].get_attr_val_int('entryusn')

+     users[0].replace('description', 'update')

+     try:

+         users[0].replace('uid', 'invalid update')

+     except ldap.NOT_ALLOWED_ON_RDN:

+         pass

+     users[0].replace('description', 'second update')

+     entryusn_after = users[0].get_attr_val_int('entryusn')

+ 

+     # entryUSN should be OLD + 2 (only two user updates)

+     assert entryusn_after == (entryusn_before + 2)

+ 

+     check_lastusn_after_restart(inst)

+ 

+ 

+ def test_entryusn_after_repl_delete(topology_m2):

+     """Verify that entryUSN is incremented on 1 after delete operation which creates a tombstone

+ 

+     :id: 1704cf65-41bc-4347-bdaf-20fc2431b218

+     :setup: An instance with replication, Users, USN enabled

+     :steps:

+         1. Try to delete a user

+         2. Check the tombstone has the incremented USN

+         3. Try to delete ou=People with users

+         4. Check the entry has a not incremented entryUSN

+     :expectedresults:

+         1. Success

+         2. Success

+         3. Should fail with Not Allowed On Non-leaf error

+         4. Success

+     """

+ 

+     inst = topology_m2.ms["master1"]

+     plugin = USNPlugin(inst)

+     plugin.enable()

+     inst.restart()

+     users = UserAccounts(inst, DEFAULT_SUFFIX)

+ 

+     try:

+         user_1 = users.create_test_user()

+         user_rdn = user_1.rdn

+         tombstones = Tombstones(inst, DEFAULT_SUFFIX)

+ 

+         user_1.replace('description', 'update_ts')

+         user_usn = user_1.get_attr_val_int('entryusn')

+ 

+         user_1.delete()

+ 

+         ts = tombstones.get(user_rdn)

+         ts_usn = ts.get_attr_val_int('entryusn')

+ 

+         assert (user_usn + 1) == ts_usn

+ 

+         user_1 = users.create_test_user()

+         org = OrganizationalUnit(inst, f"ou=People,{DEFAULT_SUFFIX}")

+         org.replace('description', 'update_ts')

+         ou_usn_before = org.get_attr_val_int('entryusn')

+         try:

+             org.delete()

+         except ldap.NOT_ALLOWED_ON_NONLEAF:

+             pass

+         ou_usn_after = org.get_attr_val_int('entryusn')

+         assert ou_usn_before == ou_usn_after

+ 

+     finally:

+         try:

+             user_1.delete()

+         except ldap.NO_SUCH_OBJECT:

+             pass

+ 

+ 

+ if __name__ == '__main__':

+     # Run isolated

+     # -s for DEBUG mode

+     CURRENT_FILE = os.path.realpath(__file__)

+     pytest.main("-s %s" % CURRENT_FILE)

file modified
+65 -44
@@ -333,6 +333,12 @@ 

      }

      slapi_ch_free_string(&usn_berval.bv_val);

  

+     /*

+      * increment the counter now and decrement in the bepostop

+      * if the operation will fail

+      */

+     slapi_counter_increment(be->be_usn_counter);

+ 

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "<-- _usn_add_next_usn\n");

  
@@ -370,6 +376,12 @@ 

  

      *mods = slapi_mods_get_ldapmods_passout(&smods);

  

+     /*

+      * increment the counter now and decrement in the bepostop

+      * if the operation will fail

+      */

+     slapi_counter_increment(be->be_usn_counter);

+ 

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "<-- _usn_mod_next_usn\n");

      return LDAP_SUCCESS;
@@ -420,6 +432,7 @@ 

  {

      Slapi_Entry *e = NULL;

      Slapi_Backend *be = NULL;

+     int32_t tombstone_incremented = 0;

      int rc = SLAPI_PLUGIN_SUCCESS;

  

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,
@@ -441,7 +454,9 @@ 

          goto bail;

      }

      _usn_add_next_usn(e, be);

+     tombstone_incremented = 1;

  bail:

+     slapi_pblock_set(pb, SLAPI_USN_INCREMENT_FOR_TOMBSTONE, &tombstone_incremented);

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "<-- usn_betxnpreop_delete\n");

  
@@ -483,7 +498,7 @@ 

      return rc;

  }

  

- /* count up the counter */

+ /* count down the counter */

  static int

  usn_bepostop(Slapi_PBlock *pb)

  {
@@ -493,25 +508,24 @@ 

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "--> usn_bepostop\n");

  

-     /* if op is not successful, don't increment the counter */

+     /* if op is not successful, decrement the counter, else - do nothing */

      slapi_pblock_get(pb, SLAPI_RESULT_CODE, &rc);

      if (LDAP_SUCCESS != rc) {

-         /* no plugin failure */

-         rc = SLAPI_PLUGIN_SUCCESS;

-         goto bail;

-     }

+         slapi_pblock_get(pb, SLAPI_BACKEND, &be);

+         if (NULL == be) {

+             rc = LDAP_PARAM_ERROR;

+             slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);

+             rc = SLAPI_PLUGIN_FAILURE;

+             goto bail;

+         }

  

-     slapi_pblock_get(pb, SLAPI_BACKEND, &be);

-     if (NULL == be) {

-         rc = LDAP_PARAM_ERROR;

-         slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);

-         rc = SLAPI_PLUGIN_FAILURE;

-         goto bail;

+         if (be->be_usn_counter) {

+             slapi_counter_decrement(be->be_usn_counter);

+         }

      }

  

-     if (be->be_usn_counter) {

-         slapi_counter_increment(be->be_usn_counter);

-     }

+     /* no plugin failure */

+     rc = SLAPI_PLUGIN_SUCCESS;

  bail:

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "<-- usn_bepostop\n");
@@ -519,13 +533,14 @@ 

      return rc;

  }

  

- /* count up the counter */

+ /* count down the counter on a failure and mod ignore */

  static int

  usn_bepostop_modify(Slapi_PBlock *pb)

  {

      int rc = SLAPI_PLUGIN_FAILURE;

      Slapi_Backend *be = NULL;

      LDAPMod **mods = NULL;

+     int32_t do_decrement = 0;

      int i;

  

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,
@@ -534,9 +549,7 @@ 

      /* if op is not successful, don't increment the counter */

      slapi_pblock_get(pb, SLAPI_RESULT_CODE, &rc);

      if (LDAP_SUCCESS != rc) {

-         /* no plugin failure */

-         rc = SLAPI_PLUGIN_SUCCESS;

-         goto bail;

+         do_decrement = 1;

      }

  

      slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods);
@@ -545,25 +558,29 @@ 

              if (mods[i]->mod_op & LDAP_MOD_IGNORE) {

                  slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                                "usn_bepostop_modify - MOD_IGNORE detected\n");

-                 goto bail; /* conflict occurred.

-                               skip incrementing the counter. */

+                 do_decrement = 1; /* conflict occurred.

+                                      decrement he counter. */

              } else {

                  break;

              }

          }

      }

  

-     slapi_pblock_get(pb, SLAPI_BACKEND, &be);

-     if (NULL == be) {

-         rc = LDAP_PARAM_ERROR;

-         slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);

-         rc = SLAPI_PLUGIN_FAILURE;

-         goto bail;

+     if (do_decrement) {

+         slapi_pblock_get(pb, SLAPI_BACKEND, &be);

+         if (NULL == be) {

+             rc = LDAP_PARAM_ERROR;

+             slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);

+             rc = SLAPI_PLUGIN_FAILURE;

+             goto bail;

+         }

+         if (be->be_usn_counter) {

+             slapi_counter_decrement(be->be_usn_counter);

+         }

      }

  

-     if (be->be_usn_counter) {

-         slapi_counter_increment(be->be_usn_counter);

-     }

+     /* no plugin failure */

+     rc = SLAPI_PLUGIN_SUCCESS;

  bail:

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "<-- usn_bepostop_modify\n");
@@ -573,34 +590,38 @@ 

  

  /* count up the counter */

  /* if the op is delete and the op was not successful, remove preventryusn */

+ /* the function is executed on TXN level */

  static int

  usn_bepostop_delete(Slapi_PBlock *pb)

  {

      int rc = SLAPI_PLUGIN_FAILURE;

      Slapi_Backend *be = NULL;

+     int32_t tombstone_incremented = 0;

  

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "--> usn_bepostop_delete\n");

  

-     /* if op is not successful, don't increment the counter */

+     /* if op is not successful and it is a tombstone entry, decrement the counter */

      slapi_pblock_get(pb, SLAPI_RESULT_CODE, &rc);

      if (LDAP_SUCCESS != rc) {

-         /* no plugin failure */

-         rc = SLAPI_PLUGIN_SUCCESS;

-         goto bail;

-     }

+         slapi_pblock_get(pb, SLAPI_USN_INCREMENT_FOR_TOMBSTONE, &tombstone_incremented);

+         if (tombstone_incremented) {

+             slapi_pblock_get(pb, SLAPI_BACKEND, &be);

+             if (NULL == be) {

+                 rc = LDAP_PARAM_ERROR;

+                 slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);

+                 rc = SLAPI_PLUGIN_FAILURE;

+                 goto bail;

+             }

  

-     slapi_pblock_get(pb, SLAPI_BACKEND, &be);

-     if (NULL == be) {

-         rc = LDAP_PARAM_ERROR;

-         slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);

-         rc = SLAPI_PLUGIN_FAILURE;

-         goto bail;

+             if (be->be_usn_counter) {

+                 slapi_counter_decrement(be->be_usn_counter);

+             }

+         }

      }

  

-     if (be->be_usn_counter) {

-         slapi_counter_increment(be->be_usn_counter);

-     }

+     /* no plugin failure */

+     rc = SLAPI_PLUGIN_SUCCESS;

  bail:

      slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM,

                    "<-- usn_bepostop_delete\n");

file modified
+13 -1
@@ -2436,7 +2436,7 @@ 

              (*(char **)value) = NULL;

          }

  	break;

- 		

+ 

      case SLAPI_SEARCH_CTRLS:

          if (pblock->pb_intop != NULL) {

              (*(LDAPControl ***)value) = pblock->pb_intop->pb_search_ctrls;
@@ -2479,6 +2479,14 @@ 

          }

          break;

  

+     case SLAPI_USN_INCREMENT_FOR_TOMBSTONE:

+         if (pblock->pb_intop != NULL) {

+             (*(int32_t *)value) = pblock->pb_intop->pb_usn_tombstone_incremented;

+         } else {

+             (*(int32_t *)value) = 0;

+         }

+         break;

+ 

      /* ACI Target Check */

      case SLAPI_ACI_TARGET_CHECK:

          if (pblock->pb_misc != NULL) {
@@ -4159,6 +4167,10 @@ 

          pblock->pb_intop->pb_paged_results_cookie = *(int *)value;

          break;

  

+     case SLAPI_USN_INCREMENT_FOR_TOMBSTONE:

+         pblock->pb_intop->pb_usn_tombstone_incremented = *((int32_t *)value);

+         break;

+ 

      /* ACI Target Check */

      case SLAPI_ACI_TARGET_CHECK:

          _pblock_assert_pb_misc(pblock);

@@ -161,6 +161,7 @@ 

  

      int pb_paged_results_index;  /* stash SLAPI_PAGED_RESULTS_INDEX */

      int pb_paged_results_cookie; /* stash SLAPI_PAGED_RESULTS_COOKIE */

+     int32_t pb_usn_tombstone_incremented; /* stash SLAPI_PAGED_RESULTS_COOKIE */

  } slapi_pblock_intop;

  

  /* Stuff that is rarely used, but still present */

@@ -7482,6 +7482,9 @@ 

  #define SLAPI_PAGED_RESULTS_INDEX  1945

  #define SLAPI_PAGED_RESULTS_COOKIE 1949

  

+ /* USN Plugin flag for tombstone entries */

+ #define SLAPI_USN_INCREMENT_FOR_TOMBSTONE 1950

+ 

  /* ACI Target Check */

  #define SLAPI_ACI_TARGET_CHECK 1946

  

Bug Description: When we assign a member to a group we have two
oprations - group modification and user modification.
As a result, they both have the same entryUSN because USN Plugin
assigns entryUSN value in bepreop but increments the counter
in the postop and a lot of things can happen in between.

Fix Description: Increment the counter in bepreop together with
entryUSN assignment. Also, decrement the counter in bepostop if
the failuer has happened.
Add test suite to cover the change.

https://pagure.io/389-ds-base/issue/49300

Reviewed by: ?

This check is done after two direct updates (and more nested). Why not doing the checking after each individual direct update.
After the first update mem1 != group1
after second update mem1 != mem2 != group1 != group2

The test is good. IMHO it could be extended if you do a successful update (a), then a failing one (b) then a successful update (c).
entryusn after (c) should be entryusn after(a) + 1. (no hole)

I think it should be also incremented in _usn_add_next_usn

In case of failure, counter is decrease in betxnpostop add/mod/modrdn/del

It is increased in bepreop mod/modrdn (_usn_mod_next_usn). I think it should also be increased in _usn_add_next_usn (betxnpreop add/del).

Note that only DEL creating a tombstone (SLAPI_PLUGIN_BE_TXN_PRE_DELETE_TOMBSTONE_FN) increases the counter (betxnpeop DEL). in case of failure we should decrease the counter only if DEL attempted to create a tombstone. It is corner case. At the moment I do not know an easy way to check that unless looking for SLAPI_ATTR_VALUE_TOMBSTONE in entry objectclass.

Except those remarks the overall patch looks very nice :)

rebased onto 1384e41bc32cfdd68af28d705f30cbb1e493da79

2 years ago

Very accurate feedback, thank you!
Please, check.

P.S. I am not sure if I fully understood the part with DEL.

rebased onto 735e162ea87fab50720f25a43ef5aeb411760213

2 years ago

As per IRC discussion, I've updated the delete part to decrement the counter only for TXN operation.

Thank you, Thierry!
Please, review.

The overall patch looks very good. I have a minor comment regarding the naming of the pblock field. In the patch it looks like a callback level while we just need a hack to transfer an information from preop to postop without adding a callback level.
I would suggest something like SLAPI_USN_INCREMENT_FOR_TOMBSTONE. It may be defined close to misc definitions like SLAPI_PAGED_RESULTS_COOKIE.

I would prefer to have this field in slapi_pblock_intop (similar to pb_paged_results_cookie)

rebased onto dc5fe10e4c4ae30aaed25023c9c89e6e653b482d

2 years ago

Sure, sounds good!
Fixed. Please, review.

I would prefer more systematic check. It is easier to understand the expected behavior (all USN should differ)
mem1!=mem2
mem1!=group1
mem1!=group2
mem2!=group1
mem2!=group2
group1!=group2

mem_1_usn and grp_1_usn are reassigned here (vs. the previous update). I would prefer you also check that the USN generated/stored (mem1/grp1) from the first update also differs from the USN generated/stored (mem1/grp1) during the second update.

Thanks @spichugi for the patch. Except those minor comments for testcase asserts the patch looks good. Feel free to push it without additional review. ACK

rebased onto ffda491

2 years ago

Pull-Request has been merged by spichugi

2 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/4257

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

2 years ago