[SSSD] [PATCH] nested groups unit test

Pavel Březina pbrezina at redhat.com
Tue Feb 18 12:46:14 UTC 2014


On 01/31/2014 02:37 PM, Pavel Březina wrote:
> On 11/05/2013 12:22 PM, Jakub Hrozek wrote:
>> On Thu, Jul 18, 2013 at 03:45:35PM +0200, Pavel Březina wrote:
>>> On 07/18/2013 02:38 PM, Pavel Březina wrote:
>>>> Hi,
>>>> this is the first unit test for nested groups. It covers only the most
>>>> basic situation when we are trying to resolve one group with no
>>>> members.
>>>>
>>>> Even though it is only one test, the patch set is quite big. This is
>>>> because it creates the possibility to mock providers related modules.
>>>> Most of the patches are just a preparation for unit testing providers.
>>>>
>>>> Patches 1-5
>>>> Moves the code around to reduce number of dependencies. (E.g. you do
>>>> not
>>>> want to load fail over when you are testing nested groups.)
>>>>
>>>> Patch 6
>>>> Mocks basic SDAP interface.
>>>>
>>>> Patch 7
>>>> Mocks sysdb objects - currently user and rfc2307bis group. You can
>>>> decide what set of attributes the object should posses. For example,
>>>> creating a user requires only basedn and name parameter, to construct
>>>> originalDN and name attributes. The rest is provided by (attrname,
>>>> value) pairs via variadic function.
>>>>
>>>> E.g.:
>>>> mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
>>>>
>>>> get_attr_type() translates the sysdb attribute name to proper data
>>>> type.
>>>> This should be extended as needed.
>>>>
>>>> Patch 8
>>>> Adds provider tests related common object files and cflags in makefile
>>>>
>>>> Patch 9
>>>> New macro sss_will_return_always(fn, value). This can be used to mock
>>>> function data in such way that any call of mock() will return the
>>>> value.
>>>>
>>>> It was just pushed also to cmocka upstream as will_return_always().
>>>>
>>>> Patch 10-11
>>>> Unit test.
>>>>
>>>> Patch 12
>>>> Removes a noisy debug message.
>>>>
>>>> I would like to get this reviewed before I continue with more test
>>>> cases, so the framework is tuned enough.
>>>>
>>>> Off topic:
>>>> I also created new macro called fail_msg, which will make the test fail
>>>> printing a message. I didn't use this macro in the end, but it made its
>>>> way to cmocka upstream.
>>>
>>> Oops, I did some reorganization while writing this mail but sent
>>> patches generated sooner, so the numbers does not match. Reordered
>>> patches are attached.
>>>
>>>
>>
>> The patches no longer apply (not suprising after such a long time on the
>> list..)
>>
>> I will add some quick comments inline without trying out the patches.
>>
>>>  From 9b370ed83ec5c85819781145de92c39977fd19eb Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Tue, 16 Jul 2013 11:21:02 +0200
>>> Subject: [PATCH 01/12] sdap: move non async functions from
>>> sdap_async.c to
>>>   sdap_utils.c
>>>
>>> ---
>>>   Makefile.am                     |   1 +
>>>   src/providers/ldap/sdap_async.c | 107 --------------------------------
>>>   src/providers/ldap/sdap_utils.c | 131
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 132 insertions(+), 107 deletions(-)
>>>   create mode 100644 src/providers/ldap/sdap_utils.c
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index
>>> 515843420834bbbd0e5b4c86c114294c89afa752..6cfc363cea9eae833a8a4bdff12309286475b89a
>>> 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -1450,6 +1450,7 @@ libsss_ldap_common_la_SOURCES = \
>>>       src/providers/ldap/sdap_reinit.c \
>>>       src/providers/ldap/sdap_dyndns.c \
>>>       src/providers/ldap/sdap_refresh.c \
>>> +    src/providers/ldap/sdap_utils.c \
>>>       src/providers/ldap/sdap.c
>>>   libsss_ldap_common_la_LDFLAGS = \
>>>       -avoid-version
>>> diff --git a/src/providers/ldap/sdap_async.c
>>> b/src/providers/ldap/sdap_async.c
>>> index
>>> bdd3c3c8b2ed4cc7246c008ddbfe531b82d880bc..6d29c1a566f82c2ce0dead2ca8073ace15afbf56
>>> 100644
>>> --- a/src/providers/ldap/sdap_async.c
>>> +++ b/src/providers/ldap/sdap_async.c
>>> @@ -23,26 +23,8 @@
>>>   #include "util/util.h"
>>>   #include "providers/ldap/sdap_async_private.h"
>>>
>>> -#define REALM_SEPARATOR '@'
>>>   #define REPLY_REALLOC_INCREMENT 10
>>>
>>> -void make_realm_upper_case(const char *upn)
>>
>> This function seems to be called from a single location only. Can we
>> either ditch it in favor of get_uppercase_realm() from usertools.c or
>> make it static?
>>
>>> -{
>>> -    char *c;
>>> -
>>> -    c = strchr(upn, REALM_SEPARATOR);
>>> -    if (c == NULL) {
>>> -        DEBUG(9, ("No realm delimiter found in upn [%s].\n", upn));
>>> -        return;
>>> -    }
>>> -
>>> -    while(*(++c) != '\0') {
>>> -        c[0] = toupper(*c);
>>> -    }
>>> -
>>> -    return;
>>> -}
>>> -
>>
>>>  From 4f76600299c31489c98f960d162ab7c1d2a29126 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Wed, 17 Jul 2013 13:20:43 +0200
>>> Subject: [PATCH 02/12] sdap: move non async functions from
>>>   sdap_async_connection.c to sdap_utils.c
>>
>> Looks good to me.
>>
>>>  From dab6d9221bafe0e18f599b88de10b7cc57a57b8e Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Wed, 17 Jul 2013 14:28:26 +0200
>>> Subject: [PATCH 03/12] sdap: move sdap_get_id_specific_filter() to
>>>   sdap_utils.c
>>
>> I think this makes sense.
>>
>> Since we started talking about LDAP provider refactoring on IRC
>> recently, I would like to bring up the ldap_*.c vs. sdap_*.c confusion.
>>
>> In my opinion, we should separate libsss_ldap.la as discussed on #sssd
>> the other day and keep its sources in sdap*.c. Only code specific to the
>> initialization of the LDAP provider should be kept in ldap*.c
>>
>>
>>>  From a97267f35a598e7f944cf993b37cd6b43db7a572 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Wed, 17 Jul 2013 13:50:34 +0200
>>> Subject: [PATCH 04/12] ldap: move options related content from
>>> ldap_common.c
>>>   to ldap_options.c
>>>
>>> ---
>>>   src/providers/ldap/ldap_common.c                   | 886
>>> --------------------
>>>   .../ldap/{ldap_common.c => ldap_options.c}         | 921
>>> ---------------------
>>>   2 files changed, 1807 deletions(-)
>>>   copy src/providers/ldap/{ldap_common.c => ldap_options.c} (53%)
>>>
>>> diff --git a/src/providers/ldap/ldap_common.c
>>> b/src/providers/ldap/ldap_common.c
>>> index
>>> 5782c4615ddd3778d16e52986eae065c3c61b8e5..7a8bf93d2366a517e545f1faecdc3aefa2be36e7
>>> 100644
>>> --- a/src/providers/ldap/ldap_common.c
>>> +++ b/src/providers/ldap/ldap_common.c
>>> @@ -39,892 +39,6 @@
>>>   /* a fd the child process would log into */
>>>   int ldap_child_debug_fd = -1;
>>>
>>> -int
>>> -sdap_domain_destructor(void *mem)
>>> -{
>>> -    struct sdap_domain *dom =
>>> -            talloc_get_type(mem, struct sdap_domain);
>>> -    DLIST_REMOVE(*(dom->head), dom);
>>> -    return 0;
>>> -}
>>> -
>>> -struct sdap_domain *
>>> -sdap_domain_get(struct sdap_options *opts,
>>> -                struct sss_domain_info *dom)
>>> -{
>>> -    struct sdap_domain *sditer = NULL;
>>> -
>>> -    DLIST_FOR_EACH(sditer, opts->sdom) {
>>> -        if (sditer->dom == dom) {
>>> -            break;
>>> -        }
>>> -    }
>>> -
>>> -    return sditer;
>>> -}
>>> -
>>> -errno_t
>>> -sdap_domain_add(struct sdap_options *opts,
>>> -                struct sss_domain_info *dom,
>>> -                struct sdap_domain **_sdom)
>>> -{
>>> -    struct sdap_domain *sdom;
>>> -
>>> -    sdom = talloc_zero(opts, struct sdap_domain);
>>> -    if (sdom == NULL) {
>>> -        return ENOMEM;
>>> -    }
>>> -    sdom->dom = dom;
>>> -    sdom->head = &opts->sdom;
>>> -
>>> -    if (opts->sdom) {
>>> -        /* Only allow subdomains of the parent domain */
>>> -        if (dom->parent == NULL ||
>>> -            dom->parent != opts->sdom->dom) {
>>> -            DEBUG(SSSDBG_OP_FAILURE, ("Domain %s is not a subdomain
>>> of %s\n",
>>> -                  dom->name, opts->sdom->dom->name));
>>> -            return EINVAL;
>>> -        }
>>> -    }
>>> -
>>> -    talloc_set_destructor((TALLOC_CTX *)sdom, sdap_domain_destructor);
>>> -    DLIST_ADD_END(opts->sdom, sdom, struct sdap_domain *);
>>> -
>>> -    if (_sdom) *_sdom = sdom;
>>> -    return EOK;
>>> -}
>>> -
>>> -errno_t
>>> -sdap_domain_subdom_add(struct sdap_id_ctx *sdap_id_ctx,
>>> -                       struct sdap_domain *sdom_list,
>>> -                       struct sss_domain_info *parent)
>>> -{
>>> -    struct sss_domain_info *dom;
>>> -    struct sdap_domain *sdom, *sditer;
>>> -    char *basedn;
>>> -    errno_t ret;
>>> -
>>> -    for (dom = get_next_domain(parent, true);
>>> -         dom && IS_SUBDOMAIN(dom); /* if we get back to a parent,
>>> stop */
>>> -         dom = get_next_domain(dom, false)) {
>>> -
>>> -        DLIST_FOR_EACH(sditer, sdom_list) {
>>> -            if (sditer->dom == dom) {
>>> -                break;
>>> -            }
>>> -        }
>>> -
>>> -        if (sditer == NULL) {
>>> -            /* New sdap domain */
>>> -            DEBUG(SSSDBG_TRACE_FUNC, ("subdomain %s is a new one,
>>> will "
>>> -                  "create a new sdap domain object\n", dom->name));
>>> -
>>> -            ret = sdap_domain_add(sdap_id_ctx->opts, dom, &sdom);
>>> -            if (ret != EOK) {
>>> -                DEBUG(SSSDBG_OP_FAILURE,
>>> -                    ("Cannot add new sdap domain for domain %s [%d]:
>>> %s\n",
>>> -                    parent->name, ret, strerror(ret)));
>>> -                return ret;
>>> -            }
>>> -        } else {
>>> -            sdom = sditer;
>>> -        }
>>> -
>>> -        /* Convert the domain name into search base */
>>> -        ret = domain_to_basedn(sdom, sdom->dom->name, &basedn);
>>> -        if (ret != EOK) {
>>> -            DEBUG(SSSDBG_OP_FAILURE,
>>> -                ("Cannot convert domain name [%s] to base DN [%d]:
>>> %s\n",
>>> -                dom->name, ret, strerror(ret)));
>>> -            talloc_free(basedn);
>>> -            return ret;
>>> -        }
>>> -
>>> -        /* Update search bases */
>>> -        talloc_zfree(sdom->search_bases);
>>> -        sdom->search_bases = talloc_array(sdom, struct
>>> sdap_search_base *, 2);
>>> -        if (sdom->search_bases == NULL) {
>>> -            return ret;
>>> -        }
>>> -        sdom->search_bases[1] = NULL;
>>> -
>>> -        ret = sdap_create_search_base(sdom, basedn,
>>> LDAP_SCOPE_SUBTREE, NULL,
>>> -                                      &sdom->search_bases[0]);
>>> -        talloc_free(basedn);
>>> -        if (ret) {
>>> -            DEBUG(SSSDBG_OP_FAILURE, ("Cannot create new sdap search
>>> base\n"));
>>> -            return ret;
>>> -        }
>>> -
>>> -        sdom->user_search_bases = sdom->search_bases;
>>> -        sdom->group_search_bases = sdom->search_bases;
>>> -        sdom->netgroup_search_bases = sdom->search_bases;
>>> -        sdom->sudo_search_bases = sdom->search_bases;
>>> -        sdom->service_search_bases = sdom->search_bases;
>>> -        sdom->autofs_search_bases = sdom->search_bases;
>>> -    }
>>> -
>>> -    return EOK;
>>> -}
>>> -
>>> -void
>>> -sdap_domain_remove(struct sdap_options *opts,
>>> -                   struct sss_domain_info *dom)
>>> -{
>>> -    struct sdap_domain *sdom;
>>> -
>>> -    sdom = sdap_domain_get(opts, dom);
>>> -    if (sdom == NULL) return;
>>> -
>>> -    DLIST_REMOVE(*(sdom->head), sdom);
>>> -}
>>> -
>>
>> Would it make sense to separate sdap_domain_* functions to an sdap
>> module instead?
>>
>> Moving ldap_get_$provider_options makes sense to me.
>>
>>>  From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Wed, 17 Jul 2013 13:51:37 +0200
>>> Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip
>>> when unit
>>>   testing
>>
>> I agree with Lukas, I would prefer not to add ifdefs to the code unless
>> needed.
>>
>>>  From d1e5b04f0cc7ece9bf7fccd564626e488d532951 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Tue, 26 Mar 2013 15:07:17 +0100
>>> Subject: [PATCH 06/12] tests: mock SDAP
>>
>> Looks good to me.
>>
>>
>>>  From e64bce82249f1b8be7bd5eb8bd4fd01d7a5fb5dd Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Tue, 26 Mar 2013 15:18:21 +0100
>>> Subject: [PATCH 07/12] tests: mock sysdb users and groups
>>
>> I think the approach is fine for unit tests. I don't see us change the
>> types of sysdb attributes frequently.
>>
>>
>>>  From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Wed, 17 Jul 2013 13:58:54 +0200
>>> Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
>>>   tests
>>
>> Looks good to me.
>>
>>
>>>  From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Thu, 18 Jul 2013 13:24:26 +0200
>>> Subject: [PATCH 09/12] tests: new macro sss_will_return_always
>>
>> Is this feature available in all cmocka releases (IIRC you wrote it) ?
>>
>>>  From 65da81b7fe5418f44ec743fc11520c5e68a85435 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Tue, 26 Mar 2013 15:06:44 +0100
>>> Subject: [PATCH 10/12] tests: add confdb_path to sss_test_ctx
>>
>> Looks good to me.
>>
>>
>>>  From 03e318c1319f8b3545195d7deee3d55793fc941a Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Wed, 17 Jul 2013 13:59:36 +0200
>>> Subject: [PATCH 11/12] tests: nested groups unit test
>>
>> I only scrolled through this one, but in general looks good.
>>
>>
>>>  From 53ff2f0812b6ae81642513aee693284ad134ba47 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>> Date: Thu, 18 Jul 2013 14:05:05 +0200
>>> Subject: [PATCH 12/12] tests: don't print debug message when test dir
>>> does
>>>   not exist
>>
>> Looks good to me.
>
> Rebased version with all issues addressed is attached.

Rebased again atop Nikolai's DEBUG patches. I hope it is the last time :-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sdap-move-non-async-functions-from-sdap_async.c-to-s.patch
Type: text/x-patch
Size: 9709 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sdap-move-non-async-functions-from-sdap_async_connec.patch
Type: text/x-patch
Size: 2404 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sdap-move-sdap_get_id_specific_filter-to-sdap_utils..patch
Type: text/x-patch
Size: 2501 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ldap-move-options-related-content-from-ldap_common.c.patch
Type: text/x-patch
Size: 57435 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-ldap-move-domain-related-content-from-ldap_common.c-.patch
Type: text/x-patch
Size: 13357 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-make-make_realm_upper_case-static.patch
Type: text/x-patch
Size: 2721 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-tests-add-confdb_path-to-sss_test_ctx.patch
Type: text/x-patch
Size: 1963 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-tests-mock-SDAP.patch
Type: text/x-patch
Size: 7408 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-tests-mock-sysdb-users-and-groups.patch
Type: text/x-patch
Size: 7968 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-tests-prepare-makefile-for-provider-related-unit-tes.patch
Type: text/x-patch
Size: 1124 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-tests-new-macro-sss_will_return_always.patch
Type: text/x-patch
Size: 851 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-tests-nested-groups-unit-test.patch
Type: text/x-patch
Size: 8570 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-tests-don-t-print-debug-message-when-test-dir-does-n.patch
Type: text/x-patch
Size: 1073 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140218/9080e03f/attachment-0012.bin>


More information about the sssd-devel mailing list