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

Pavel Březina pbrezina at redhat.com
Thu Mar 5 14:41:28 UTC 2015


On 03/05/2015 10:03 AM, Jakub Hrozek wrote:
> 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
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-be_refresh-refresh-all-domains-in-backend.patch
Type: text/x-patch
Size: 8196 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sdap_handle_acct_req_send-remove-be_req.patch
Type: text/x-patch
Size: 7983 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-be_refresh-refactor-netgroups-refresh.patch
Type: text/x-patch
Size: 8083 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-be_refresh-add-sdap_refresh_init.patch
Type: text/x-patch
Size: 6310 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-be_refresh-support-users.patch
Type: text/x-patch
Size: 5972 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-be_refresh-support-groups.patch
Type: text/x-patch
Size: 6214 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-be_refresh-get-rid-of-callback-pointers.patch
Type: text/x-patch
Size: 6152 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-sysdb-use-sysdb_user-group_dn.patch
Type: text/x-patch
Size: 7712 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150305/71c558fa/attachment-0007.bin>


More information about the sssd-devel mailing list