[SSSD] [PATCHSET] intg: sss_override

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Tue Oct 20 13:12:31 UTC 2015


Hi Pavel,

On 10/13/2015 12:22 AM, Pavel Reichl wrote:
> Please see updated patchset.

Thank you for working on this. Please see my comments below.

First of all, we need the test in Makefile.am, so it is included into
distribution.

I'm not sure if we really need this split into three patches, to me a single
patch seems better. Anyone against?

I see that you prefer not to capitalize the start of the sentences (comments),
acronyms and such. Can I ask you to reconsider? Capitalization makes them more
readable - the eye catches the start of the sentence and doesn't try to read
acronyms as real words.

> 0001-intg-new-test-for-user-local-overrides.patch
>
>
>  From 24d4a2a7154b1adfb5b4224b0807ac822225844b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl<preichl at redhat.com>
> Date: Mon, 5 Oct 2015 10:02:05 -0400
> Subject: [PATCH 1/3] intg: new test for user local overrides

There is a practice of putting what the patch *does* into the subject, instead
of what it contains. It's more universal, i.e. it accomodates removal and
changing as well. This is practiced in the Linux kernel and many other
projects and I personally like it. I also think that starting the subject with
a capital letter makes it easier to read. If this is applied, the subject
would become:

     intg: Add test for user local overrides

I'm not sure if we're sticking to this in SSSD, though.

If we agree on this, but keep separate patches, all of them will need to be
updated.

> Introduce a new integration test for local view overrides.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2732
> ---
>   src/tests/intg/ldap_local_override_test.py | 339 +++++++++++++++++++++++++++++
>   1 file changed, 339 insertions(+)
>   create mode 100644 src/tests/intg/ldap_local_override_test.py
>
> diff --git a/src/tests/intg/ldap_local_override_test.py b/src/tests/intg/ldap_local_override_test.py
> new file mode 100644
> index 0000000000000000000000000000000000000000..66bcb3c29b9e1ac861fb4c7a28d381a8302093f2
> --- /dev/null
> +++ b/src/tests/intg/ldap_local_override_test.py
> @@ -0,0 +1,339 @@
> +#
> +# integration test for sss_override tool
> +#
> +# Copyright (c) 2015 Red Hat, Inc.
> +# Author: Pavel Reichl<preichl at redhat.com>
> +#
> +# This is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 only
> +#
> +# This program is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see<http://www.gnu.org/licenses/>.
> +#
> +import os
> +import stat
> +import ent
> +import grp
> +import pwd
> +import config
> +import signal
> +import subprocess
> +import time
> +import pytest
> +import ds_openldap
> +import ldap_ent
> +import sssd_id
> +from util import unindent
> +
> +LDAP_BASE_DN = "dc=example,dc=com"
> +
> +
> + at pytest.fixture(scope="module")
> +def ds_inst(request):
> +    """LDAP server instance fixture"""
> +    ds_inst = ds_openldap.DSOpenLDAP(
> +        config.PREFIX, 10389, LDAP_BASE_DN,
> +        "cn=admin", "Secret123")
> +    try:
> +        ds_inst.setup()
> +    except:
> +        ds_inst.teardown()
> +        raise
> +    request.addfinalizer(lambda: ds_inst.teardown())
> +    return ds_inst
> +
> +
> + at pytest.fixture(scope="module")
> +def ldap_conn(request, ds_inst):
> +    """LDAP server connection fixture"""
> +    ldap_conn = ds_inst.bind()
> +    ldap_conn.ds_inst = ds_inst
> +    request.addfinalizer(lambda: ldap_conn.unbind_s())
> +    return ldap_conn
> +
> +
> +def create_ldap_fixture(request, ldap_conn, ent_list):
> +    """Add LDAP entries and add teardown for removing them"""
> +    for entry in ent_list:
> +        ldap_conn.add_s(entry[0], entry[1])
> +
> +    def teardown():
> +        for entry in ent_list:
> +            ldap_conn.delete_s(entry[0])
> +    request.addfinalizer(teardown)
> +
> +
> +def create_conf_fixture(request, contents):
> +    """Generate sssd.conf and add teardown for removing it"""
> +    conf = open(config.CONF_PATH, "w")
> +    conf.write(contents)
> +    conf.close()
> +    os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)
> +    request.addfinalizer(lambda: os.unlink(config.CONF_PATH))
> +
> +
> +def stop_sssd():
> +    pid_file = open(config.PIDFILE_PATH, "r")
> +    pid = int(pid_file.read())
> +    os.kill(pid, signal.SIGTERM)
> +    while True:
> +        try:
> +            os.kill(pid, signal.SIGCONT)
> +        except:
> +            break
> +        time.sleep(1)
> +
> +
> +def start_sssd():
> +    """Start sssd"""
> +    if subprocess.call(["sssd", "-D", "-f"]) != 0:
> +        raise Exception("sssd start failed")
> +
> +
> +def restart_sssd():
> +    stop_sssd()
> +    start_sssd()
> +
> +
> +def create_sssd_fixture(request):
> +    """Start sssd and add teardown for stopping it and removing state"""
> +    if subprocess.call(["sssd", "-D", "-f"]) != 0:
> +        raise Exception("sssd start failed")
> +
> +    def teardown():
> +        try:
> +            stop_sssd()
> +        except:
> +            pass
> +        subprocess.call(["sss_cache", "-E"])
> +        for path in os.listdir(config.DB_PATH):
> +            os.unlink(config.DB_PATH + "/" + path)
> +        for path in os.listdir(config.MCACHE_PATH):
> +            os.unlink(config.MCACHE_PATH + "/" + path)
> +    request.addfinalizer(teardown)
> +
> +
> +def load_data_to_ldap(request, ldap_conn):
> +    ent_list = ldap_ent.List(LDAP_BASE_DN)
> +
> +    ent_list.add_user("user1", 10001, 20001,
> +                      gecos='User Number 1',
> +                      loginShell='/bin/user1_shell',
> +                      homeDirectory='/home/user1')
> +
> +    ent_list.add_user("user2", 10002, 20001,
> +                      gecos='User Number 2',
> +                      loginShell='/bin/user2_shell',
> +                      homeDirectory='/home/user2')
> +
> +    ent_list.add_group("group", 2001,
> +                       ["user2", "user1"])
> +
> +    create_ldap_fixture(request, ldap_conn, ent_list)
> +
> +
> +override_user_filename = "user_export_file"

Shouldn't a global variable name be uppercase?

> +
> +
> + at pytest.fixture
> +def sanity_rfc2307(request, ldap_conn):

Does this and other function have to have "_rfc2307" in them?
Do you plan tests for other schemas?

> +    load_data_to_ldap(request, ldap_conn)
> +
> +    conf = unindent("""\
> +        [sssd]
> +        domains             = LDAP
> +        services            = nss
> +
> +        [nss]

Might as well remove this empty section.

> +
> +        [domain/LDAP]
> +        ldap_auth_disable_tls_never_use_in_production = true
> +        ldap_schema         = rfc2307
> +        id_provider         = ldap
> +        auth_provider       = ldap
> +        sudo_provider       = ldap
> +        ldap_uri            = {ldap_conn.ds_inst.ldap_url}
> +        ldap_search_base    = {ldap_conn.ds_inst.base_dn}
> +    """).format(**locals())
> +    create_conf_fixture(request, conf)
> +    create_sssd_fixture(request)
> +
> +    def teardown():
> +        # remove user export file
> +        os.unlink(override_user_filename)
> +    request.addfinalizer(teardown)
> +
> +    return None
> +
> +
> +def check_users_from_ldap():

The name of this function is misleading, as it also requests and verifies
groups. Plus, it doesn't really ensure that the entries are retrieved from
LDAP, so that's wrong too. It only checks that some particular users and a
group retrieved from NSS match the original entries added to LDAP.

I think naming it e.g. "check_original_entries", "check_fixture_entries", or
similar will be better. What do you think?

> +    ent.assert_passwd_by_name(
> +        'user1',
> +        dict(name='user1', passwd='*', uid=10001, gid=20001,
> +             gecos='User Number 1',
> +             shell='/bin/user1_shell',
> +             dir='/home/user1'))
> +
> +    ent.assert_passwd_by_name(
> +        'user2',
> +        dict(name='user2', passwd='*', uid=10002, gid=20001,
> +             gecos='User Number 2',
> +             shell='/bin/user2_shell',
> +             dir='/home/user2'))
> +
> +    ent.assert_group_by_name(
> +        "group",
> +        dict(mem=ent.contains_only("user1", "user2")))
> +
> +
> +def test_local_override_user(ldap_conn, sanity_rfc2307):
> +    """ test commands: user-add, user-del, user-import, user export """

There is a typo in "user-export" command name.
Also, unnecessary spaces around docstring contents.
I think the test function name doesn't have to include "_local_override",
since the file name has it. So it can simply be "test_user".

> +
> +    def perform_override_1():
> +        subprocess.check_call(["sss_override", "user-add", "user1",
> +                               "-u", "10010",
> +                               "-g", "20010",
> +                               "-n", "ov_user1",
> +                               "-c", "Overriden User 1",
> +                               "-h", "/home/ov/user1",
> +                               "-s", "/bin/ov_user1_shell"])
> +
> +        # fqdn

I assume you meant "fully-qualified (user) name"? If that's so, then the
"fqdn" acronym is misused here and below. Besides, acronyms are written in
capitals.

> +        subprocess.check_call(["sss_override", "user-add", "user2 at LDAP",
> +                               "-u", "10020",
> +                               "-g", "20020",
> +                               "-n", "ov_user2",
> +                               "-c", "Overriden User 2",
> +                               "-h", "/home/ov/user2",
> +                               "-s", "/bin/ov_user2_shell"])
> +
> +    def check_users_after_override_1():
> +        ent.assert_passwd_by_name(
> +            'ov_user1',
> +            dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
> +                 gecos='Overriden User 1',
> +                 dir='/home/ov/user1',
> +                 shell='/bin/ov_user1_shell'))
> +
> +        ent.assert_passwd_by_name(
> +            'user1',
> +            dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
> +                 gecos='Overriden User 1',
> +                 dir='/home/ov/user1',
> +                 shell='/bin/ov_user1_shell'))
> +        # fqdn
> +        ent.assert_passwd_by_name(
> +            'ov_user1 at LDAP',
> +            dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
> +                 gecos='Overriden User 1',
> +                 dir='/home/ov/user1',
> +                 shell='/bin/ov_user1_shell'))
> +
> +        # fqdn
> +        ent.assert_passwd_by_name(
> +            'user1 at LDAP',
> +            dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
> +                 gecos='Overriden User 1',
> +                 dir='/home/ov/user1',
> +                 shell='/bin/ov_user1_shell'))
> +
> +        # fqdn
> +        ent.assert_passwd_by_name(
> +            'ov_user2 at LDAP',
> +            dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
> +                 gecos='Overriden User 2',
> +                 dir='/home/ov/user2',
> +                 shell='/bin/ov_user2_shell'))
> +
> +        ent.assert_passwd_by_name(
> +            'user2',
> +            dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
> +                 gecos='Overriden User 2',
> +                 dir='/home/ov/user2',
> +                 shell='/bin/ov_user2_shell'))
> +
> +    def subtest_override_override():
> +        subprocess.check_call(["sss_override", "user-add", "user1",
> +                               "-c", "Override of Overriden User 1"])
> +
> +        restart_sssd()
> +
> +        # user1 got restored to initial status (state in ldap) except for gecos
> +        ent.assert_passwd_by_name(
> +            'user1',
> +            dict(name='user1', passwd='*', uid=10001, gid=20001,
> +                 gecos='Override of Overriden User 1',
> +                 shell='/bin/user1_shell',
> +                 dir='/home/user1'))
> +
> +        ent.assert_passwd_by_name(
> +            'user1 at LDAP',
> +            dict(name='user1', passwd='*', uid=10001, gid=20001,
> +                 gecos='Override of Overriden User 1',
> +                 shell='/bin/user1_shell',
> +                 dir='/home/user1'))
> +
> +        # user1's name override to ov_user1 was removed
> +        with pytest.raises(KeyError):
> +            pwd.getpwnam('ov_user1')
> +
> +    def subtest_override_to_root():
> +        subprocess.check_call(["sss_override", "user-add", "user1",
> +                               "-u", "0"])
> +
> +        restart_sssd()
> +
> +        # nothing changed
> +        ent.assert_passwd_by_name(
> +            'user1 at LDAP',
> +            dict(name='user1', passwd='*', uid=10001, gid=20001,
> +                 gecos='Override of Overriden User 1',
> +                 shell='/bin/user1_shell',
> +                 dir='/home/user1'))
> +
> +    def drop_all_overrides():
> +        subprocess.check_call(["sss_override", "user-del", "user1"])
> +        # fqdn
> +        subprocess.check_call(["sss_override", "user-del", "user2 at LDAP"])
> +
> +    # end of local functions

There doesn't seem to be any need to have these functions defined inside the
test. We can move them out and make the test function much shorter and easier
to navigate. To distinguish them from similar functions for other tests, you
can prepend them with common prefix. E.g. "user_" in this case, or, perhaps,
to have it clearer with "_test_user_" (underscore needed to keep away
py.test).

> +
> +    perform_override_1()

I think this function can be called just "override", or if global
"user_override", or "_test_user_override".

> +    restart_sssd()

Why do we need to restart SSSD here?

> +
> +    check_users_after_override_1()

Continuing with the scheme, this can be called "user_check_after_override", or
"_test_user_check_after_override".

> +
> +    # save overrides
> +    subprocess.call(["sss_override", "user-export", override_user_filename])
> +
> +    # override already overriden user
> +    subtest_override_override()
> +
> +    # depends on outcome of ticket #2833 - commented for now
> +    # subtest_override_to_root()
> +
> +    drop_all_overrides()
> +    restart_sssd()
> +
> +    check_users_from_ldap()

Shouldn't we also check that there are no other users there?

> +
> +    # reload overrides
> +    subprocess.call(["sss_override", "user-import", override_user_filename])
> +
> +    restart_sssd()

Why do we need to restart SSSD here?

> +
> +    check_users_after_override_1()

All of the above said, I would really prefer to see this test split into a few
smaller ones, each invoking a (possibly shared) py.test fixture separately.
Since py.test doesn't really support nested tests (argh!), we'll need to have
them flat.

E.g.:

     * simple override (both with and without FQNs)
         add entries
         assert entries are not overriden
         add override
         test entries are overriden
     * root override
         add entries
         assert entries are not overriden
         add override to root
         test entries are not overriden
     * override of override
         add entries
         assert entries are not overriden
         add override
         assert entries are overriden
         add override of override
         test overrides are overriden
     * override removal
         add entries
         assert entries are not overriden
         add overrides
         assert entries are overriden
         remove overrides
         test entries are not overriden
     * override export/import
         add entries
         assert entries are not overriden
         add overrides
         assert entries are overriden
         export overrides
         remove overrides
         assert entries are not overriden
         import overrides
         test entries are overriden

Making a single test out of all these is tempting, but they are testing
different assertions and shouldn't depend on each other. Otherwise reading
such a merged test and debugging a failure will be difficult.

By "assert" above I mean verifying the setup of the test, and by "test" I mean
doing the actual test's check. In theory, everything before "test" should be a
part of the test's fixture and what starts with "test" should be the actual
test. However, it might not be so easy to do with py.test. Unfortunately,
py.test doesn't distinguish coding assertions from test assertions, hijacks
the stock "assert" statement, and I expect will get confused if you use them
in fixtures, but I haven't tried the latter.

We can probably build fixtures for these tests by calling some of the higher
ones from the lower ones.

The above also applies to the group override tests and tests in general, but
I'll not be repeating myself.

> +
> +
> +def test_local_override_user_cached(ldap_conn, sanity_rfc2307):
> +
> +    # put object to sysdb cache before perfoming tests

There's a typo in "performing".

> +    check_users_from_ldap()

Shouldn't we also check that there are no other users there?

I'll not be reviewing the other patches for now, as this is quite a lot
already and we can do another iteration.

Thank you!

Nick


More information about the sssd-devel mailing list