6d0b66
From 55a47c1bfe1ce1c27e470384c4f1d50895db25f7 Mon Sep 17 00:00:00 2001
6d0b66
From: Mark Reynolds <mreynolds@redhat.com>
6d0b66
Date: Tue, 13 Jul 2021 14:18:03 -0400
6d0b66
Subject: [PATCH] Issue 4443 - Internal unindexed searches in syncrepl/retro
6d0b66
 changelog
6d0b66
6d0b66
Bug Description:
6d0b66
6d0b66
When a non-system index is added to a backend it is
6d0b66
disabled until the database is initialized or reindexed.
6d0b66
So in the case of the retro changelog the changenumber index
6d0b66
is alway disabled by default since it is never initialized.
6d0b66
This leads to unexpected unindexed searches of the retro
6d0b66
changelog.
6d0b66
6d0b66
Fix Description:
6d0b66
6d0b66
If an index has "nsSystemIndex" set to "true" then enable it
6d0b66
immediately.
6d0b66
6d0b66
relates:  https://github.com/389ds/389-ds-base/issues/4443
6d0b66
6d0b66
Reviewed by: spichugi & tbordaz(Thanks!!)
6d0b66
---
6d0b66
 .../tests/suites/retrocl/basic_test.py        | 53 ++++++++-------
6d0b66
 .../suites/retrocl/retrocl_indexing_test.py   | 68 +++++++++++++++++++
6d0b66
 ldap/servers/plugins/retrocl/retrocl_create.c |  2 +-
6d0b66
 .../slapd/back-ldbm/ldbm_index_config.c       | 25 +++++--
6d0b66
 src/lib389/lib389/_mapped_object.py           | 13 ++++
6d0b66
 5 files changed, 130 insertions(+), 31 deletions(-)
6d0b66
 create mode 100644 dirsrvtests/tests/suites/retrocl/retrocl_indexing_test.py
6d0b66
6d0b66
diff --git a/dirsrvtests/tests/suites/retrocl/basic_test.py b/dirsrvtests/tests/suites/retrocl/basic_test.py
6d0b66
index f3bc50f29..84d513829 100644
6d0b66
--- a/dirsrvtests/tests/suites/retrocl/basic_test.py
6d0b66
+++ b/dirsrvtests/tests/suites/retrocl/basic_test.py
6d0b66
@@ -8,7 +8,6 @@
6d0b66
 
6d0b66
 import logging
6d0b66
 import ldap
6d0b66
-import time
6d0b66
 import pytest
6d0b66
 from lib389.topologies import topology_st
6d0b66
 from lib389.plugins import RetroChangelogPlugin
6d0b66
@@ -18,7 +17,8 @@ from lib389.tasks import *
6d0b66
 from lib389.cli_base import FakeArgs, connect_instance, disconnect_instance
6d0b66
 from lib389.cli_base.dsrc import dsrc_arg_concat
6d0b66
 from lib389.cli_conf.plugins.retrochangelog import retrochangelog_add_attr
6d0b66
-from lib389.idm.user import UserAccount, UserAccounts, nsUserAccounts
6d0b66
+from lib389.idm.user import UserAccount, UserAccounts
6d0b66
+from lib389._mapped_object import DSLdapObjects
6d0b66
 
6d0b66
 pytestmark = pytest.mark.tier1
6d0b66
 
6d0b66
@@ -82,7 +82,7 @@ def test_retrocl_exclude_attr_add(topology_st):
6d0b66
 
6d0b66
     log.info('Adding user1')
6d0b66
     try:
6d0b66
-        user1 = users.create(properties={
6d0b66
+        users.create(properties={
6d0b66
             'sn': '1',
6d0b66
             'cn': 'user 1',
6d0b66
             'uid': 'user1',
6d0b66
@@ -97,17 +97,18 @@ def test_retrocl_exclude_attr_add(topology_st):
6d0b66
     except ldap.ALREADY_EXISTS:
6d0b66
         pass
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.error("Failed to add user1")
6d0b66
+        log.error("Failed to add user1: " + str(e))
6d0b66
 
6d0b66
     log.info('Verify homePhone and carLicense attrs are in the changelog changestring')
6d0b66
     try:
6d0b66
-        cllist = st.search_s(RETROCL_SUFFIX, ldap.SCOPE_SUBTREE, '(targetDn=%s)' % USER1_DN)
6d0b66
+        retro_changelog_suffix = DSLdapObjects(st, basedn=RETROCL_SUFFIX)
6d0b66
+        cllist = retro_changelog_suffix.filter(f'(targetDn={USER1_DN})')
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.fatal("Changelog search failed, error: " +str(e))
6d0b66
+        log.fatal("Changelog search failed, error: " + str(e))
6d0b66
         assert False
6d0b66
     assert len(cllist) > 0
6d0b66
-    if  cllist[0].hasAttr('changes'):
6d0b66
-        clstr = (cllist[0].getValue('changes')).decode()
6d0b66
+    if  cllist[0].present('changes'):
6d0b66
+        clstr = str(cllist[0].get_attr_vals_utf8('changes'))
6d0b66
         assert ATTR_HOMEPHONE in clstr
6d0b66
         assert ATTR_CARLICENSE in clstr
6d0b66
 
6d0b66
@@ -134,7 +135,7 @@ def test_retrocl_exclude_attr_add(topology_st):
6d0b66
 
6d0b66
     log.info('Adding user2')
6d0b66
     try:
6d0b66
-        user2 = users.create(properties={
6d0b66
+        users.create(properties={
6d0b66
             'sn': '2',
6d0b66
             'cn': 'user 2',
6d0b66
             'uid': 'user2',
6d0b66
@@ -149,18 +150,18 @@ def test_retrocl_exclude_attr_add(topology_st):
6d0b66
     except ldap.ALREADY_EXISTS:
6d0b66
         pass
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.error("Failed to add user2")
6d0b66
+        log.error("Failed to add user2: " + str(e))
6d0b66
 
6d0b66
     log.info('Verify homePhone attr is not in the changelog changestring')
6d0b66
     try:
6d0b66
-        cllist = st.search_s(RETROCL_SUFFIX, ldap.SCOPE_SUBTREE, '(targetDn=%s)' % USER2_DN)
6d0b66
+        cllist = retro_changelog_suffix.filter(f'(targetDn={USER2_DN})')
6d0b66
         assert len(cllist) > 0
6d0b66
-        if  cllist[0].hasAttr('changes'):
6d0b66
-            clstr = (cllist[0].getValue('changes')).decode()
6d0b66
+        if  cllist[0].present('changes'):
6d0b66
+            clstr = str(cllist[0].get_attr_vals_utf8('changes'))
6d0b66
             assert ATTR_HOMEPHONE not in clstr
6d0b66
             assert ATTR_CARLICENSE in clstr
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.fatal("Changelog search failed, error: " +str(e))
6d0b66
+        log.fatal("Changelog search failed, error: " + str(e))
6d0b66
         assert False
6d0b66
 
6d0b66
 def test_retrocl_exclude_attr_mod(topology_st):
6d0b66
@@ -228,19 +229,20 @@ def test_retrocl_exclude_attr_mod(topology_st):
6d0b66
             'homeDirectory': '/home/user1',
6d0b66
             'userpassword': USER_PW})
6d0b66
     except ldap.ALREADY_EXISTS:
6d0b66
-        pass
6d0b66
+        user1 = UserAccount(st, dn=USER1_DN)
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.error("Failed to add user1")
6d0b66
+        log.error("Failed to add user1: " + str(e))
6d0b66
 
6d0b66
     log.info('Verify homePhone and carLicense attrs are in the changelog changestring')
6d0b66
     try:
6d0b66
-        cllist = st.search_s(RETROCL_SUFFIX, ldap.SCOPE_SUBTREE, '(targetDn=%s)' % USER1_DN)
6d0b66
+        retro_changelog_suffix = DSLdapObjects(st, basedn=RETROCL_SUFFIX)
6d0b66
+        cllist = retro_changelog_suffix.filter(f'(targetDn={USER1_DN})')
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.fatal("Changelog search failed, error: " +str(e))
6d0b66
+        log.fatal("Changelog search failed, error: " + str(e))
6d0b66
         assert False
6d0b66
     assert len(cllist) > 0
6d0b66
-    if  cllist[0].hasAttr('changes'):
6d0b66
-        clstr = (cllist[0].getValue('changes')).decode()
6d0b66
+    if  cllist[0].present('changes'):
6d0b66
+        clstr = str(cllist[0].get_attr_vals_utf8('changes'))
6d0b66
         assert ATTR_HOMEPHONE in clstr
6d0b66
         assert ATTR_CARLICENSE in clstr
6d0b66
 
6d0b66
@@ -267,24 +269,25 @@ def test_retrocl_exclude_attr_mod(topology_st):
6d0b66
 
6d0b66
     log.info('Modify user1 carLicense attribute')
6d0b66
     try:
6d0b66
-        st.modify_s(USER1_DN, [(ldap.MOD_REPLACE, ATTR_CARLICENSE, b"123WX321")])
6d0b66
+        user1.replace(ATTR_CARLICENSE, "123WX321")
6d0b66
     except ldap.LDAPError as e:
6d0b66
         log.fatal('test_retrocl_exclude_attr_mod: Failed to update user1 attribute: error ' + e.message['desc'])
6d0b66
         assert False
6d0b66
 
6d0b66
     log.info('Verify carLicense attr is not in the changelog changestring')
6d0b66
     try:
6d0b66
-        cllist = st.search_s(RETROCL_SUFFIX, ldap.SCOPE_SUBTREE, '(targetDn=%s)' % USER1_DN)
6d0b66
+        cllist = retro_changelog_suffix.filter(f'(targetDn={USER1_DN})')
6d0b66
         assert len(cllist) > 0
6d0b66
         # There will be 2 entries in the changelog for this user, we are only
6d0b66
         #interested in the second one, the modify operation.
6d0b66
-        if  cllist[1].hasAttr('changes'):
6d0b66
-            clstr = (cllist[1].getValue('changes')).decode()
6d0b66
+        if  cllist[1].present('changes'):
6d0b66
+            clstr = str(cllist[1].get_attr_vals_utf8('changes'))
6d0b66
             assert ATTR_CARLICENSE not in clstr
6d0b66
     except ldap.LDAPError as e:
6d0b66
-        log.fatal("Changelog search failed, error: " +str(e))
6d0b66
+        log.fatal("Changelog search failed, error: " + str(e))
6d0b66
         assert False
6d0b66
 
6d0b66
+
6d0b66
 if __name__ == '__main__':
6d0b66
     # Run isolated
6d0b66
     # -s for DEBUG mode
6d0b66
diff --git a/dirsrvtests/tests/suites/retrocl/retrocl_indexing_test.py b/dirsrvtests/tests/suites/retrocl/retrocl_indexing_test.py
6d0b66
new file mode 100644
6d0b66
index 000000000..b1dfe962c
6d0b66
--- /dev/null
6d0b66
+++ b/dirsrvtests/tests/suites/retrocl/retrocl_indexing_test.py
6d0b66
@@ -0,0 +1,68 @@
6d0b66
+import logging
6d0b66
+import pytest
6d0b66
+import os
6d0b66
+from lib389._constants import RETROCL_SUFFIX, DEFAULT_SUFFIX
6d0b66
+from lib389.topologies import topology_st as topo
6d0b66
+from lib389.plugins import RetroChangelogPlugin
6d0b66
+from lib389.idm.user import UserAccounts
6d0b66
+from lib389._mapped_object import DSLdapObjects
6d0b66
+log = logging.getLogger(__name__)
6d0b66
+
6d0b66
+
6d0b66
+def test_indexing_is_online(topo):
6d0b66
+    """Test that the changenmumber index is online right after enabling the plugin
6d0b66
+
6d0b66
+    :id: 16f4c001-9e0c-4448-a2b3-08ac1e85d40f
6d0b66
+    :setup: Standalone Instance
6d0b66
+    :steps:
6d0b66
+        1. Enable retro cl
6d0b66
+        2. Perform some updates
6d0b66
+        3. Search for "(changenumber>=-1)", and it is not partially unindexed
6d0b66
+        4. Search for "(&(changenumber>=-1)(targetuniqueid=*))", and it is not partially unindexed
6d0b66
+    :expectedresults:
6d0b66
+        1. Success
6d0b66
+        2. Success
6d0b66
+        3. Success
6d0b66
+        4. Success
6d0b66
+    """
6d0b66
+
6d0b66
+    # Enable plugin
6d0b66
+    topo.standalone.config.set('nsslapd-accesslog-logbuffering',  'off')
6d0b66
+    plugin = RetroChangelogPlugin(topo.standalone)
6d0b66
+    plugin.enable()
6d0b66
+    topo.standalone.restart()
6d0b66
+
6d0b66
+    # Do a bunch of updates
6d0b66
+    users = UserAccounts(topo.standalone, DEFAULT_SUFFIX)
6d0b66
+    user_entry = users.create(properties={
6d0b66
+        'sn': '1',
6d0b66
+        'cn': 'user 1',
6d0b66
+        'uid': 'user1',
6d0b66
+        'uidNumber': '11',
6d0b66
+        'gidNumber': '111',
6d0b66
+        'givenname': 'user1',
6d0b66
+        'homePhone': '0861234567',
6d0b66
+        'carLicense': '131D16674',
6d0b66
+        'mail': 'user1@whereever.com',
6d0b66
+        'homeDirectory': '/home'
6d0b66
+    })
6d0b66
+    for count in range(0, 10):
6d0b66
+        user_entry.replace('mail', f'test{count}@test.com')
6d0b66
+
6d0b66
+    # Search the retro cl, and check for error messages
6d0b66
+    filter_simple = '(changenumber>=-1)'
6d0b66
+    filter_compound = '(&(changenumber>=-1)(targetuniqueid=*))'
6d0b66
+    retro_changelog_suffix = DSLdapObjects(topo.standalone, basedn=RETROCL_SUFFIX)
6d0b66
+    retro_changelog_suffix.filter(filter_simple)
6d0b66
+    assert not topo.standalone.searchAccessLog('Partially Unindexed Filter')
6d0b66
+
6d0b66
+    # Search the retro cl again with compound filter
6d0b66
+    retro_changelog_suffix.filter(filter_compound)
6d0b66
+    assert not topo.standalone.searchAccessLog('Partially Unindexed Filter')
6d0b66
+
6d0b66
+
6d0b66
+if __name__ == '__main__':
6d0b66
+    # Run isolated
6d0b66
+    # -s for DEBUG mode
6d0b66
+    CURRENT_FILE = os.path.realpath(__file__)
6d0b66
+    pytest.main(["-s", CURRENT_FILE])
6d0b66
diff --git a/ldap/servers/plugins/retrocl/retrocl_create.c b/ldap/servers/plugins/retrocl/retrocl_create.c
6d0b66
index 571e6899f..5bfde7831 100644
6d0b66
--- a/ldap/servers/plugins/retrocl/retrocl_create.c
6d0b66
+++ b/ldap/servers/plugins/retrocl/retrocl_create.c
6d0b66
@@ -133,7 +133,7 @@ retrocl_create_be(const char *bedir)
6d0b66
     val.bv_len = strlen(val.bv_val);
6d0b66
     slapi_entry_add_values(e, "cn", vals);
6d0b66
 
6d0b66
-    val.bv_val = "false";
6d0b66
+    val.bv_val = "true"; /* enables the index */
6d0b66
     val.bv_len = strlen(val.bv_val);
6d0b66
     slapi_entry_add_values(e, "nssystemindex", vals);
6d0b66
 
6d0b66
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_index_config.c b/ldap/servers/slapd/back-ldbm/ldbm_index_config.c
6d0b66
index 9722d0ce7..38e7368e1 100644
6d0b66
--- a/ldap/servers/slapd/back-ldbm/ldbm_index_config.c
6d0b66
+++ b/ldap/servers/slapd/back-ldbm/ldbm_index_config.c
6d0b66
@@ -25,7 +25,7 @@ int ldbm_instance_index_config_delete_callback(Slapi_PBlock *pb, Slapi_Entry *en
6d0b66
 #define INDEXTYPE_NONE 1
6d0b66
 
6d0b66
 static int
6d0b66
-ldbm_index_parse_entry(ldbm_instance *inst, Slapi_Entry *e, const char *trace_string, char **index_name, char *err_buf)
6d0b66
+ldbm_index_parse_entry(ldbm_instance *inst, Slapi_Entry *e, const char *trace_string, char **index_name, PRBool *is_system_index, char *err_buf)
6d0b66
 {
6d0b66
     Slapi_Attr *attr;
6d0b66
     const struct berval *attrValue;
6d0b66
@@ -78,6 +78,15 @@ ldbm_index_parse_entry(ldbm_instance *inst, Slapi_Entry *e, const char *trace_st
6d0b66
         }
6d0b66
     }
6d0b66
 
6d0b66
+    *is_system_index = PR_FALSE;
6d0b66
+    if (0 == slapi_entry_attr_find(e, "nsSystemIndex", &attr)) {
6d0b66
+        slapi_attr_first_value(attr, &sval);
6d0b66
+        attrValue = slapi_value_get_berval(sval);
6d0b66
+        if (strcasecmp(attrValue->bv_val, "true") == 0) {
6d0b66
+            *is_system_index = PR_TRUE;
6d0b66
+        }
6d0b66
+    }
6d0b66
+
6d0b66
     /* ok the entry is good to process, pass it to attr_index_config */
6d0b66
     if (attr_index_config(inst->inst_be, (char *)trace_string, 0, e, 0, 0, err_buf)) {
6d0b66
         slapi_ch_free_string(index_name);
6d0b66
@@ -101,9 +110,10 @@ ldbm_index_init_entry_callback(Slapi_PBlock *pb __attribute__((unused)),
6d0b66
                                void *arg)
6d0b66
 {
6d0b66
     ldbm_instance *inst = (ldbm_instance *)arg;
6d0b66
+    PRBool is_system_index = PR_FALSE;
6d0b66
 
6d0b66
     returntext[0] = '\0';
6d0b66
-    *returncode = ldbm_index_parse_entry(inst, e, "from ldbm instance init", NULL, NULL);
6d0b66
+    *returncode = ldbm_index_parse_entry(inst, e, "from ldbm instance init", NULL, &is_system_index /* not used */, NULL);
6d0b66
     if (*returncode == LDAP_SUCCESS) {
6d0b66
         return SLAPI_DSE_CALLBACK_OK;
6d0b66
     } else {
6d0b66
@@ -126,17 +136,21 @@ ldbm_instance_index_config_add_callback(Slapi_PBlock *pb __attribute__((unused))
6d0b66
 {
6d0b66
     ldbm_instance *inst = (ldbm_instance *)arg;
6d0b66
     char *index_name = NULL;
6d0b66
+    PRBool is_system_index = PR_FALSE;
6d0b66
 
6d0b66
     returntext[0] = '\0';
6d0b66
-    *returncode = ldbm_index_parse_entry(inst, e, "from DSE add", &index_name, returntext);
6d0b66
+    *returncode = ldbm_index_parse_entry(inst, e, "from DSE add", &index_name, &is_system_index, returntext);
6d0b66
     if (*returncode == LDAP_SUCCESS) {
6d0b66
         struct attrinfo *ai = NULL;
6d0b66
         /* if the index is a "system" index, we assume it's being added by
6d0b66
          * by the server, and it's okay for the index to go online immediately.
6d0b66
          * if not, we set the index "offline" so it won't actually be used
6d0b66
          * until someone runs db2index on it.
6d0b66
+         * If caller wants to add an index that they want to be online
6d0b66
+         * immediately they can also set "nsSystemIndex" to "true" in the
6d0b66
+         * index config entry (e.g. is_system_index).
6d0b66
          */
6d0b66
-        if (!ldbm_attribute_always_indexed(index_name)) {
6d0b66
+        if (!is_system_index && !ldbm_attribute_always_indexed(index_name)) {
6d0b66
             ainfo_get(inst->inst_be, index_name, &ai;;
6d0b66
             PR_ASSERT(ai != NULL);
6d0b66
             ai->ai_indexmask |= INDEX_OFFLINE;
6d0b66
@@ -386,13 +400,14 @@ ldbm_instance_index_config_enable_index(ldbm_instance *inst, Slapi_Entry *e)
6d0b66
     char *index_name = NULL;
6d0b66
     int rc = LDAP_SUCCESS;
6d0b66
     struct attrinfo *ai = NULL;
6d0b66
+    PRBool is_system_index = PR_FALSE;
6d0b66
 
6d0b66
     index_name = slapi_entry_attr_get_charptr(e, "cn");
6d0b66
     if (index_name) {
6d0b66
         ainfo_get(inst->inst_be, index_name, &ai;;
6d0b66
     }
6d0b66
     if (!ai) {
6d0b66
-        rc = ldbm_index_parse_entry(inst, e, "from DSE add", &index_name, NULL);
6d0b66
+        rc = ldbm_index_parse_entry(inst, e, "from DSE add", &index_name, &is_system_index /* not used */, NULL);
6d0b66
     }
6d0b66
     if (rc == LDAP_SUCCESS) {
6d0b66
         /* Assume the caller knows if it is OK to go online immediately */
6d0b66
diff --git a/src/lib389/lib389/_mapped_object.py b/src/lib389/lib389/_mapped_object.py
6d0b66
index b6d778b01..fe610d175 100644
6d0b66
--- a/src/lib389/lib389/_mapped_object.py
6d0b66
+++ b/src/lib389/lib389/_mapped_object.py
6d0b66
@@ -148,6 +148,19 @@ class DSLdapObject(DSLogging, DSLint):
6d0b66
 
6d0b66
         return True
6d0b66
 
6d0b66
+    def search(self, scope="subtree", filter='objectclass=*'):
6d0b66
+        search_scope = ldap.SCOPE_SUBTREE
6d0b66
+        if scope == 'base':
6d0b66
+            search_scope = ldap.SCOPE_BASE
6d0b66
+        elif scope == 'one':
6d0b66
+            search_scope = ldap.SCOPE_ONE
6d0b66
+        elif scope == 'subtree':
6d0b66
+            search_scope = ldap.SCOPE_SUBTREE
6d0b66
+        return self._instance.search_ext_s(self._dn, search_scope, filter,
6d0b66
+                                           serverctrls=self._server_controls,
6d0b66
+                                           clientctrls=self._client_controls,
6d0b66
+                                           escapehatch='i am sure')
6d0b66
+
6d0b66
     def display(self, attrlist=['*']):
6d0b66
         """Get an entry but represent it as a string LDIF
6d0b66
 
6d0b66
-- 
6d0b66
2.31.1
6d0b66