[SSSD] [PATCHES] be_refresh: support users and groups

Jakub Hrozek jhrozek at redhat.com
Thu Mar 5 09:03:33 UTC 2015


On Wed, Mar 04, 2015 at 09:57:02AM +0100, Pavel Březina wrote:
> On 03/03/2015 01:03 PM, Jakub Hrozek wrote:
> >On Thu, Feb 19, 2015 at 12:18:13PM +0100, Pavel Březina wrote:
> >>https://fedorahosted.org/sssd/ticket/2346
> >>
> >>Hi,
> >>the first patch add support for subdomains, which was missing in current
> >>implementation. It probably wasn't a big deal with netgroups but I think it
> >>would be with users and groups.
> >>
> >>The majority of these patches refactors the current be_refresh to make
> >>support of other object types than netgroups a lot easier and
> >>straightforward.
> >
> >Code review so far, no testing was done. I'm just sending my notes so
> >you can reply while I test the patches.
> >
> >> From e744e3dbeab301c149accbff521cbc5232c6a48d Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Fri, 13 Feb 2015 13:16:04 +0100
> >>Subject: [PATCH 1/6] be_refresh: refresh all domains in backend
> >>
> >>---
> >>  src/providers/dp_refresh.c        | 82 ++++++++++++++++++++++++---------------
> >
> >Can you also write a test for this module while we're changing it?
> 
> Well, there is a ticket for it. But the main problem is how do you want to
> write a meaningful test?
> 
> The be_refresh module consist of:
> 
> be_refresh_send (public)
> - fetch expired objects (internal)
> - refresh them (internal)
> 
> The only thing you can test is that sdap was actually called which isn't
> really worth the time IMHO. Or do you have other idea?

I was thinking about:
        - adding expired entries to cache when the test starts
        - having a dummy ptask that marks the entries as current

Then the test would run a single refresh and verify that the ptask actually
ran and the entries were found by the matching functions.

Again, this would be for master and would verify that the be_refresh_get_*
functions works and the refresh works at all.

> 
> >>  src/providers/dp_refresh.h        |  1 +
> >>  src/providers/ldap/ldap_common.h  |  1 +
> >>  src/providers/ldap/sdap_refresh.c | 15 +++++--
> >>  4 files changed, 64 insertions(+), 35 deletions(-)
> >>
> >>diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c
> >>index 817b6213ca47bba3fa34ce28fdcd1621d349b651..bd02d0cd99f9a061109f0c17797c6e018d602dc5 100644
> >>--- a/src/providers/dp_refresh.c
> >>+++ b/src/providers/dp_refresh.c
> >>@@ -117,6 +117,7 @@ typedef errno_t
> >>
> >>
> >>  struct be_refresh_cb {
> >>+    const char *name;
> >>      bool enabled;
> >>      be_refresh_get_values_t get_values;
> >>      be_refresh_send_t send_fn;
> >>@@ -137,6 +138,7 @@ struct be_refresh_ctx *be_refresh_ctx_init(TALLOC_CTX *mem_ctx)
> >>          return NULL;
> >>      }
> >>
> >>+    ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].name = "netgroups";
> >>      ctx->callbacks[BE_REFRESH_TYPE_NETGROUPS].get_values \
> >>          = be_refresh_get_netgroups;
> >
> >In retrospective I wish we didn't use a pointer function here. A pointer
> >function might make sense in cases where the function might be from a
> >different provider for instance, but the sysdb expiration will be always
> >read in the same manner. A switch-case here would be much simpler and
> >allow for easier code browsing.
> 
> Yes... do you want me to write a patch?

Yes, for master, it will make the code more readable. Let's not delay this
patchset any longer.

> 
> >I also wonder if be_refresh_send needs the be_ctx parameter at all,
> >since be_ctx is also part of the be_ptask structure?
> 
> It is signature for be_ptask send functions.
> 
> >
> >I know the two remarks above are not strictly related to your patches,
> >but I had to re-read the be_refresh code to make sure I understand it :-)
> >
> >The rest of the patch looks good to me.
> 
> I will wait until you finish testing, since the issues are below are
> trivial.
> 
> >
> >> From 85b2f150134e89672f764e7b267f7f47a285938f Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Fri, 13 Feb 2015 13:49:17 +0100
> >>Subject: [PATCH 2/6] sdap_handle_acct_req_send: remove be_req
> >>
> >>be_req was used only as a talloc context for subreq. This memory context
> >>was replace by state of the parent request which is more suitable for
> >>tevent coding style.
> >
> >ACK. I traced the parent contexts all the way up and confirmed that in
> >fact nothing changes, be_req is always used as the top-level state.
> >
> >> From a92f1ffac23e90cfff848a05ef9611fa021b78a4 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Mon, 16 Feb 2015 13:42:02 +0100
> >>Subject: [PATCH 3/6] be_refresh: refactor netgroups refresh
> >
> >[...]
> >
> >>+    state->account_req = talloc_zero(state, struct be_acct_req);
> >>+    if (state->account_req == NULL) {
> >>+        ret = ENOMEM;
> >>+        goto immediately;
> >>+    }
> >>+
> >>+    state->account_req->entry_type = entry_type;
> >>+    state->account_req->attr_type = BE_ATTR_ALL;
> >
> >I know that this attribute is currently not used really, but can you use
> >BE_ATTR_CORE here instead? IIRC all the other invokers use _CORE as
> >well, so using it here as well might make any future refactoring easier.
> >
> >> From 4cff90263b247d8701de33ad1cf76db95698659a Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Thu, 19 Feb 2015 12:04:09 +0100
> >>Subject: [PATCH 4/6] be_refresh: add sdap_refresh_init
> >
> >[...]
> >
> >>@@ -179,14 +179,11 @@ static int ldap_id_init_internal(struct be_ctx *bectx,
> >>      }
> >>
> >>      /* setup periodical refresh of expired records */
> >>-    ret = be_refresh_add_cb(bectx->refresh_ctx, BE_REFRESH_TYPE_NETGROUPS,
> >>-                            sdap_refresh_netgroups_send,
> >>-                            sdap_refresh_netgroups_recv,
> >>-                            ctx);
> >>-    if (ret != EOK && ret != EEXIST) {
> >>-        DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh of netgroups "
> >>-              "will not work [%d]: %s\n", ret, strerror(ret));
> >>-    }
> >>+    ret = sdap_refresh_init(bectx->refresh_ctx, ctx);
> >>+        if (ret != EOK && ret != EEXIST) {
> >>+            DEBUG(SSSDBG_MINOR_FAILURE, "Periodical refresh "
> >>+                  "will not work [%d]: %s\n", ret, strerror(ret));
> >>+        }
> >
> >Wrong indent, otherwise patch LGTM.
> >
> >> From 42efd926f41c4691758e5871fd3a3330802e7f41 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Thu, 19 Feb 2015 12:00:52 +0100
> >>Subject: [PATCH 5/6] be_refresh: support users
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2346
> >>---
> >>  src/db/sysdb.c                    |  7 +++++++
> >>  src/db/sysdb.h                    |  2 ++
> >>  src/providers/dp_refresh.c        | 23 +++++++++++++++++++++++
> >>  src/providers/dp_refresh.h        |  1 +
> >>  src/providers/ldap/sdap_refresh.c | 29 +++++++++++++++++++++++++++++
> >>  5 files changed, 62 insertions(+)
> >>
> >>diff --git a/src/db/sysdb.c b/src/db/sysdb.c
> >>index 61a2240016b5cb77e6fbbc3286fd1a194c5a0b48..2bb4a41aa4a9e6201ac27ac8d9a1803c1fb5c43e 100644
> >>--- a/src/db/sysdb.c
> >>+++ b/src/db/sysdb.c
> >>@@ -172,6 +172,13 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
> >>      return dn;
> >>  }
> >>
> >>+struct ldb_dn *sysdb_user_base_dn(TALLOC_CTX *mem_ctx,
> >>+                                  struct sss_domain_info *dom)
> >>+{
> >>+    return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
> >>+                          SYSDB_TMPL_USER_BASE, dom->name);
> >>+}
> >>+
> >
> >I don't like adding a convenience function and then only using it on one
> >place. Either use ldb_dn_new_fmt() directly or convert the uses of
> >ldb_dn_new_fmt to this new convenience function.
> >
> >

How did you decide about the convenience function? I'm fine with keeping it,
but then it must be used on other places in the code so that there's one
pattern to construct the base DN, not two. That's probably out of scope
of this changeset and can be done in master.

> >> From 20bcc5371af8e429b46b0289ef9fd517c1e932c4 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Thu, 19 Feb 2015 12:10:23 +0100
> >>Subject: [PATCH 6/6] be_refresh: support groups
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2346

nack after testing :-)

You need to handle group in the sdap code:
@@ -78,6 +78,9 @@ static struct tevent_req *sdap_refresh_send(TALLOC_CTX *mem_ctx,
     case BE_REQ_USER:
         state->type = "user";
         break;
+    case BE_REQ_GROUP:
+        state->type = "group";
+        break;
     case BE_REQ_NETGROUP:
         state->type = "netgroup";
         break;

To sum, up we need to
        1) Fix the group refresh
        2) Fix the BE_ATTR and indent
        3) agree about the sysdb convenience function. I'm fine with keeping
           it and write a patch for master that uses the function elsewhere
           as well. The subsequent sysdb patch wouldn't block this patchset.
        4) Write a unit test, for master, after we push this set.

Does it make sense



More information about the sssd-devel mailing list