#50981 Ticket 50980 - RFE extend usability for slapi_compute_add_search_rewriter and slapi_compute_add_evaluator
Closed 3 years ago by spichugi. Opened 4 years ago by tbordaz.
tbordaz/389-ds-base ticket_50980  into  master

file modified
+1
@@ -1436,6 +1436,7 @@ 

  	ldap/servers/slapd/regex.c \

  	ldap/servers/slapd/resourcelimit.c \

  	ldap/servers/slapd/result.c \

+ 	ldap/servers/slapd/rewriters.c \

  	ldap/servers/slapd/sasl_map.c \

  	ldap/servers/slapd/schema.c \

  	ldap/servers/slapd/schemaparse.c \

@@ -0,0 +1,54 @@ 

+ import pytest

+ import glob

+ from lib389.tasks import *

+ from lib389.utils import *

+ from lib389.topologies import topology_st

+ 

+ from lib389._constants import DEFAULT_SUFFIX, HOST_STANDALONE, PORT_STANDALONE

+ 

+ log = logging.getLogger(__name__)

+ # Skip on versions 1.4.2 and before. Rewriters are expected in 1.4.3

+ pytestmark = [pytest.mark.tier2,

+               pytest.mark.skipif(ds_is_older('1.4.3'), reason="Not implemented")]

+ 

+ 

+ rewriters_container = "cn=rewriters,cn=config"

+ 

+ def test_rewriters_container(topology_st):

+     """

+     Test checks that rewriters container exists

+     """

+ 

+     # Check container of rewriters

+     ents = topology_st.standalone.search_s(rewriters_container, ldap.SCOPE_BASE, '(objectclass=*)')

+     assert len(ents) == 1

+ 

+     log.info('Test PASSED')

+ 

+ def test_foo_filter_rewriter(topology_st):

+     """

+     Test that example filter rewriter 'foo' is register and search use it

+     """

+ 

+     libslapd = os.path.join( topology_st.standalone.ds_paths.lib_dir, 'dirsrv/libslapd.so')

+     # register foo filter rewriters

+     topology_st.standalone.add_s(Entry((

+         "cn=foo_filter,cn=rewriters,cn=config", {

+             "objectClass": "top",

+             "objectClass": "extensibleObject",

+             "cn": "foo_filter",

+             "nsslapd-libpath": libslapd,

+             "nsslapd-filterrewriter": "example_foo2cn_filter_rewriter",

+         }

+     )))

+ 

+ 

+     topology_st.standalone.restart(60)

+ 

+     # Check that the filter 'foo=foo' is rewritten into 'cn=foo'

+     ents = topology_st.standalone.search_s(rewriters_container, ldap.SCOPE_SUBTREE, '(foo=foo_filter)')

+     assert len(ents) > 0

+     assert ents[0].dn == "cn=foo_filter,cn=rewriters,cn=config"

+ 

+     log.info('Test PASSED')

+ 

@@ -1022,6 +1022,7 @@ 

          plugin_print_lists();

          plugin_startall(argc, argv, NULL /* specific plugin list */);

          compute_plugins_started();

+         (void) rewriters_init();

          if (housekeeping_start((time_t)0, NULL) == NULL) {

              return_value = 1;

              goto cleanup;

@@ -0,0 +1,261 @@ 

+ /** 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 **/

+ 

+ #include "slapi-private.h"

+ #include "slap.h"

+ 

+ typedef struct {

+     int32_t filter_rewriters;

+     int32_t computedAttr_rewriters;

+ } nb_rewriters_t;

+ 

+ #define REWRITERS_CONTAINER_DN "cn=rewriters,cn=config"

+ #define SUBSYTEM_LOG "rewriters"

+ 

+ 

+ /* This is an example entry that calls a rewriter that generate a 'foo' attribute

+  * return code (computed.c:compute_call_evaluators)

+  *   -1 : keep looking

+  *    0 : rewrote OK

+  *  else: failure

+  *

+  * dn: cn=foo,cn=rewriters,cn=config

+  * objectClass: top

+  * objectClass: extensibleObject

+  * cn: foo

+  * nsslapd-libpath: /lib/dirsrv/libslapd.so

+  * nsslapd-returnedAttrRewriter: example_foo_computedAttr_rewriter

+  */

+ int32_t

+ example_foo_computedAttr_rewriter(computed_attr_context *c, char *type, Slapi_Entry *e, slapi_compute_output_t outputfn)

+ {

+     int32_t rc = COMPUTE_CALLBACK_CONTINUE; /* Let's others computed elevator play */

+ 

+     if (strcasecmp(type, "foo") == 0) {

+         Slapi_Attr our_attr;

+         slapi_attr_init(&our_attr, "foo");

+         our_attr.a_flags = SLAPI_ATTR_FLAG_OPATTR;

+         valueset_add_string(&our_attr, &our_attr.a_present_values, "foo computed value", CSN_TYPE_UNKNOWN, NULL);

+         rc = (*outputfn)(c, &our_attr, e);

+         attr_done(&our_attr);

+     }

+     return rc;

+ }

+ 

+ 

+ /* This is an example callback to substitute

+  * attribute type 'from' with 'to' in all

+  * the filter components

+  * example_substitute_type is a callback (FILTER_APPLY_FN) used by slapi_filter_apply

+  * typedef int (FILTER_APPLY_FN)(Slapi_Filter f, void *arg)

+  * To stick to the definition, the callback is defined using 'int' rather 'int32_t'

+  */

+ typedef struct {

+     char *attrtype_from;

+     char *attrtype_to;

+ } example_substitute_type_arg_t;

+ 

+ static int

+ example_substitute_type(Slapi_Filter *f, void *arg)

+ {

+     example_substitute_type_arg_t *substitute_arg = (example_substitute_type_arg_t *) arg;

+     char *filter_type;

+ 

+     if ((substitute_arg == NULL) ||

+         (substitute_arg->attrtype_from == NULL) ||

+         (substitute_arg->attrtype_to == NULL)) {

+         return SLAPI_FILTER_SCAN_STOP;

+     }

+ 

+     /* Substitute 'from' by 'to' attribute type */

+     if (slapi_filter_get_attribute_type(f, &filter_type) == 0) {

+             if (strcasecmp(filter_type, substitute_arg->attrtype_from) == 0) {

+                 slapi_filter_changetype(f, substitute_arg->attrtype_to);

+             }

+     }

+ 

+     /* Return continue because we should

+      * substitute 'from' in all filter components

+      */

+     return SLAPI_FILTER_SCAN_CONTINUE;

+ }

+ 

+ /* This is an example entry that calls a rewriter that

+  * exchange in the filter 'foo' into 'cn

+  * return code (from computed.c:compute_rewrite_search_filter):

+  *   -1 : keep looking

+  *    0 : rewrote OK

+  *    1 : refuse to do this search

+  *    2 : operations error

+  *

+  * dn: cn=foo,cn=rewriters,cn=config

+  * objectClass: top

+  * objectClass: extensibleObject

+  * cn: foo

+  * nsslapd-libpath: /lib/dirsrv/libslapd.so

+  * nsslapd-filterrewriter: example_foo2cn_filter_rewriter

+  */

+ int32_t

+ example_foo2cn_filter_rewriter(Slapi_PBlock *pb)

+ {

+     Slapi_Filter *clientFilter = NULL;

+     int error_code = 0;

+     int rc;

+     example_substitute_type_arg_t arg;

+     arg.attrtype_from = "foo";

+     arg.attrtype_to = "cn";

+ 

+     slapi_pblock_get(pb, SLAPI_SEARCH_FILTER, &clientFilter);

+     rc = slapi_filter_apply(clientFilter, example_substitute_type, &arg, &error_code);

+     if (rc == SLAPI_FILTER_SCAN_NOMORE) {

+         return SEARCH_REWRITE_CALLBACK_CONTINUE; /* Let's others rewriter play */

+     } else {

+         slapi_log_err(SLAPI_LOG_ERR,

+                           "example_foo2cn_filter_rewriter", "Could not update the search filter - error %d (%d)\n",

+                           rc, error_code);

+         return SEARCH_REWRITE_CALLBACK_ERROR; /* operation error */

+     }

+ }

+ 

+ /*

+  * This function registers filter rewriters and computed attribute rewriters

+  * listed in each rewriter config entry

+  *

+  * cn=ADrewrite,cn=rewriters,cn=config

+  * objectClass: top

+  * objectClass: extensibleObject

+  * cn: ADrewrite

+  * nsslapd-libPath: libadrewrite

+  * nsslapd-filterRewriter: objectcategory_filter_rewrite

+  * nsslapd-filterRewriter: objectSID_rewrite

+  * nsslapd-returnedAttrRewriter: givenname_rewrite

+  * nsslapd-returnedAttrRewriter: objectcategory_returnedAttr_rewrite

+  *

+  * This is search_entry callback used by slapi_search_internal_callback

+  * typedef int (*plugin_search_entry_callback)(Slapi_Entry *e, void *callback_data)

+  * To stick to the definition, the callback is defined returning 'int' rather 'int32_t'

+  */

+ static int

+ register_rewriters(Slapi_Entry *e, void *callback_data)

+ {

+     nb_rewriters_t *nb_rewriters = callback_data;

+     char *libpath;

+     char **values = NULL;

+     slapi_search_rewrite_callback_t filter_rewriter_cb;

+     slapi_compute_callback_t computeAttr_rewriter_cb;

+ 

+     /* Load the rewriter callback  */

+     libpath = (char *) slapi_entry_attr_get_charptr(e, "nsslapd-libPath");

+ 

+     /* register the filter rewriters */

+     values = slapi_entry_attr_get_charray(e, "nsslapd-filterRewriter");

+     if (values) {

+         for (size_t i = 0; values && values[i]; ++i) {

+             filter_rewriter_cb = (slapi_search_rewrite_callback_t) sym_load(libpath, values[i], "custom filter rewriter", 1);

+             if ((filter_rewriter_cb == NULL) || slapi_compute_add_search_rewriter(filter_rewriter_cb)) {

+                 slapi_log_err(SLAPI_LOG_ERR, SUBSYTEM_LOG, "register_rewriters: "

+                               "Fail to register filerRewriter %s\n", values[i]);

+             }

+             nb_rewriters->filter_rewriters++;

+         }

+         slapi_ch_array_free(values);

+         values = NULL;

+     }

+ 

+     /* register the computed attribute rewriters */

+     values = slapi_entry_attr_get_charray(e, "nsslapd-returnedAttrRewriter");

+     if (values) {

+         for (size_t i = 0; values && values[i]; ++i) {

+             computeAttr_rewriter_cb = (slapi_compute_callback_t) sym_load(libpath, values[i], "custom computed attribute rewriter", 1);

+             if ((computeAttr_rewriter_cb == NULL) || slapi_compute_add_evaluator(computeAttr_rewriter_cb)) {

+                 slapi_log_err(SLAPI_LOG_ERR, SUBSYTEM_LOG, "register_rewriters: "

+                               "Fail to register nsslapd-returnedAttrRewriter %s\n", values[i]);

+             }

+             nb_rewriters->computedAttr_rewriters++;

+         }

+         slapi_ch_array_free(values);

+         values = NULL;

+     }

+ 

+     slapi_ch_free_string(&libpath);

+     return 0;

+ }

+ 

+ /* Adds, if it does not already exist, the container

+  * of the rewriters config entries: cn=rewriters,cn=config

+  */

+ static int32_t

+ add_rewriters_container(void)

+ {

+     char entry_string[1024] = {0};

+     Slapi_PBlock *pb = NULL;

+     Slapi_Entry *e = NULL;

+     int return_value;

+     int32_t rc = 0;

+ 

+     /* Create cn=rewriters,cn=config Slapi_Entry*/

+     PR_snprintf(entry_string, sizeof (entry_string) - 1, "dn: %s\nobjectclass: top\nobjectclass: extensibleobject\ncn: rewriters\n", REWRITERS_CONTAINER_DN);

+     e = slapi_str2entry(entry_string, 0);

+ 

+     /* Add it, if it already exist that is okay */

+     pb = slapi_add_entry_internal(e, 0, 0 /* log_change */);

+     if (pb == NULL) {

+         /* the only time NULL pb is returned is when memory allocation fails */

+         slapi_log_err(SLAPI_LOG_ERR, SUBSYTEM_LOG, "create_rewriters_container: "

+                       "NULL pblock returned from search\n");

+         slapi_entry_free(e);

+         rc = -1;

+         goto done;

+     } else {

+         slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &return_value);

+     }

+     if (return_value != LDAP_SUCCESS && return_value != LDAP_ALREADY_EXISTS) {

+         slapi_log_err(SLAPI_LOG_ERR, SUBSYTEM_LOG, "create_rewriters_container - "

+                       "Unable to create configuration entry %s: %d\n",

+                       REWRITERS_CONTAINER_DN, return_value);

+         slapi_entry_free(e);

+         rc = -1;

+     }

+ done:

+     slapi_pblock_destroy(pb);

+     return rc;

+ }

+ 

+ /* initialization of the filter rewriters and

+  * computed attributes rewriters

+  */

+ int32_t

+ rewriters_init()

+ {

+     nb_rewriters_t nb_rewriters = {0};

+ 

+     /* Create the rewricter container in case it does not exist */

+     if (add_rewriters_container()) {

+         slapi_log_err(SLAPI_LOG_ERR, SUBSYTEM_LOG, "rewriters_init: "

+                       "Fails to initialize rewriters\n");

+         return -1;

+     }

+ 

+     /* For each child of the rewriter container, register filter/computed attribute */

+     slapi_search_internal_callback(REWRITERS_CONTAINER_DN, LDAP_SCOPE_ONELEVEL, "(cn=*)",

+             NULL, 0 /* attrsonly */,

+             &nb_rewriters /* callback_data */,

+             NULL /* controls */,

+             NULL /* result_callback */,

+             register_rewriters,

+             NULL /* referral_callback */);

+ 

+     if (nb_rewriters.filter_rewriters || nb_rewriters.computedAttr_rewriters) {

+         slapi_log_err(SLAPI_LOG_INFO, SUBSYTEM_LOG,

+                       "registered rewriters for filters: %d - for computed attributes: %d\n",

+                       nb_rewriters.filter_rewriters,

+                       nb_rewriters.computedAttr_rewriters);

+     }

+ 

+     return 0;

+ } 

\ No newline at end of file

@@ -6343,7 +6343,18 @@ 

  typedef struct _computed_attr_context computed_attr_context;

  typedef int (*slapi_compute_output_t)(computed_attr_context *c, Slapi_Attr *a, Slapi_Entry *e);

  typedef int (*slapi_compute_callback_t)(computed_attr_context *c, char *type, Slapi_Entry *e, slapi_compute_output_t outputfn);

+ typedef enum slapi_compute_callback_result {

+     COMPUTE_CALLBACK_CONTINUE = -1,

+     COMPUTE_CALLBACK_DONE,

+ } slapi_compute_callback_result_t;

+ 

  typedef int (*slapi_search_rewrite_callback_t)(Slapi_PBlock *pb);

+ typedef enum slapi_search_rewrite_callback_result {

+     SEARCH_REWRITE_CALLBACK_CONTINUE = -1,

+     SEARCH_REWRITE_CALLBACK_DONE,

+     SEARCH_REWRITE_CALLBACK_REFUSE,

+     SEARCH_REWRITE_CALLBACK_ERROR,

+ } slapi_search_rewrite_callback_result_t;

  int slapi_compute_add_evaluator(slapi_compute_callback_t function);

  int slapi_compute_add_evaluator_ext(slapi_compute_callback_t function, int rootonly);

  int slapi_compute_add_search_rewriter(slapi_search_rewrite_callback_t function);

@@ -1378,6 +1378,9 @@ 

  int slapi_client_uses_non_nss(LDAP *ld);

  int slapi_client_uses_openssl(LDAP *ld);

  

+ /* rewriters.c */

+ int32_t rewriters_init();

+ 

  /* ssl.c */

  /*

   * If non NULL buf and positive bufsize is given,

Bug Description:
plugin api allows to register filter rewriter callback (slapi_compute_add_search_rewriter)
and computed attribute callback (slapi_compute_add_evaluator)
This requires to write a new plugin to register callbacks.
This RFE is to simplify the use of those plugin api interfaces
so that rewriters (filter or computed attribute) in shared library can be taken into account
as soon as listed in config entries

Fix Description:
It follows the design http://www.port389.org/docs/389ds/design/search_rewriters.html
registers callback listed in children of 'cn=rewriters,cn=config'
The rewriters.c files contains examples of filter rewriter and computed attribute

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

Reviewed by: Mark Reynolds, Alexander Bokovoy, William Brown (thanks !!)

Platforms tested: F30

Flag Day: no

Doc impact: no

should be size_t, and declared inside of the for loops

You don't need the Sun Micro copyright, and you can use 2020 for Red Hat's :-)

Indentation is funny here as well.

Most of the logging calls have incorrect indentation on the wrapped line - I'll stop commenting on each one.

rebased onto 07f68cfd25f645ff804e40b57edff6257a924424

4 years ago

@mreynolds thanks for your review. I made the changes you reported.

I think all this debugging boilerplate isn't needed now, topologies does it for you behind the scenes.

Shouldn't this be older than 1.4.4? I think it's a less than check, not less than equal to.

UserAccounts has a "create test user" helper. See src/lib389/lib389/idm/user.py

I don't think anyone runs these directly now, so we probably don't nee the name anymore?

Maybe I'm not awake enough yet, where are the rewriters called from in the search path?

rebased onto bfc7eb05e1f3e1ab63da39ea8bcdd321798d56e6

4 years ago

@mreynolds @firstyear thanks for your reviews. Patch updated

And this comment thierry:

Maybe I'm not awake enough yet, where are the rewriters called from in the search path?

Thanks by the way, it looks interesting :)

rebased onto 39ed68fd9c01ac65888e608c0c8ec3bd56503ca6

4 years ago

rebased onto f89639626ae81328bb23d442a01690b51faa3901

4 years ago

The rewriters are quite hidden in search path. They are called with compute_rewrite_search_filter (opshared.c) and compute_attribute (result.c).

So to be sure, these are only loaded from plugins at startup? These won't be "dynamic" like other plugins (This is a good thing that they are loaded at startup only IMO, dynamic adds a lot of complexity.).

Theorically slapi_compute_add_search_rewriter and slapi_compute_add_evaluator) can be called at anytime during server lifetime. So a new rewriter could be register anytime.

Currently most rewriters are registered at startup (compute_init, entry_computed_attr_init, ldbm_back_start).
An exception is view plugin (views_start) that may register several times the same filter rewriter. The view rewriter looks safe to be registered several times. The only drawback is that it will be called several times.

I agree it is better to register the new rewriters only are startup (less complexity). Another good reason to make the framework in core server than in a plugin ;)

I agree it is better to register the new rewriters only are startup (less complexity). Another good reason to make the framework in core server than in a plugin ;)

Agreed. It's much simpler to accept the server restart, than to try to make these dynamic :) and they'll perform better too as a result!

As a general aside, this PR and your objectCategory PR could be a single PR so we can see the pieces and interactions of both at once maybe? Is there a benefit to splitting them?

Yes I prefer to separate them as it was source of confusion in the previous PR (https://pagure.io/389-ds-base/pull-request/50939) that delivered the new mechanism to register rewriters and a specific rewriter (objectCategory).

we can anticipate several specific rewriters (objectCategory, objectSID, uniquemember,..) that could be part of 389-ds or potentially others projects (freeipa). Rewriters are independent of the new registering mechanism.

I think there are some improvements you could make between https://pagure.io/389-ds-base/pull-request/50988 and this if they were together, as it gives you the full picture?

Anyway, thanks for your patience on this, I appreciate you taking the time on all my comments :)

rebased onto a534d46552ae505ba4e0dac2297442c695e4da21

3 years ago

@firstyear, please have a look at the rebase. Thanks

Use topology_st.standalone.ds_paths.lib_dir instead here. I think you can have:

libslapd = os.path.join( topology_st.standalone.ds_paths.lib_dir, 'dirsrv/libslapd.so')

Which would be much more reliable as a path lookup.

You get the [0] because you are globbing, but if you just use the example I gave above, you don't need the array indexing.

int!!!! int32_t please, like every int should be sized! :)

Better idea than 0 or -1 because that's so oooooo hard to track down in the future, is to use an enum instead. have a look at: https://pagure.io/389-ds-base/pull-request/49579#_30__26

you can even give them int values etc. This way it's much easier to match what int means what, you have to use the enum variants, and the compiler can check when you are returning the wrong type.

rebased onto 86604e4ce4d05d7998f452f818dae325ae8dc67a

3 years ago

@firstyear the patch is updated according to your comments. Thanks

@firstyear, I would ideally like to merge it this week, do you have any other concern regarding this PR ?

Besides these little comments, I think it's good. I was more thoroguh in the type check today :)

This one was on purpose it is a callback for slapi_filter_apply that is 'typedef int (FILTER_APPLY_FN)(Slapi_Filter f, void *arg);'
I wanted to stick on the definition even if I guess int32_t would be accepted as well

Idem for error_code and rc that stick to
int slapi_filter_apply(struct slapi_filter f, FILTER_APPLY_FN fn, void arg, int *error_code)

similar it is a callback for each returned entry of a search (slapi_search_internal_callback)

typedef int (*plugin_search_entry_callback)(Slapi_Entry *e,  void *callback_data);

It is used to retrieve pblock[SLAPI_PLUGIN_INTOP_RESULT] and sets a 'int', this is why I declared it as an int. Should it preferably be int32_t ?

rebased onto 2710bbbb3f93f9f1c015f355989ba8f836861289

3 years ago

Thanks @firstyear for your comment. If you agree with the patch but want some changes with int32_t I can do that on the fly and push that patch before the end of the week.

This one was on purpose it is a callback for slapi_filter_apply that is 'typedef int (FILTER_APPLY_FN)(Slapi_Filter f, void *arg);'
I wanted to stick on the definition even if I guess int32_t would be accepted as well

It would be good to comment these :)

It is used to retrieve pblock[SLAPI_PLUGIN_INTOP_RESULT] and sets a 'int', this is why I declared it as an int. Should it preferably be int32_t ?

If it's an array deref then size_t I think is the correct type here ....

Anyway, with those two last comments, ack from me :)

rebased onto a11bae3

3 years ago

Pull-Request has been merged by tbordaz

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/4034

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