[SSSD] [PATCHSET] intg: sss_override

Pavel Reichl preichl at redhat.com
Tue Oct 20 14:15:09 UTC 2015



On 10/20/2015 03:12 PM, Nikolai Kondrashov wrote:
> 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?

Well, I don't like huge patches, but I probably won't cry if we decide to merge them.  :-)

>
> 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.
OK

>
>> 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.

I don't think that we are but I can do that.

>
> 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?
OK, might be I just replace it with

def get_user_export_file():
	return "user_export_file"

>
>> +
>> +
>> + 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?
No, would you propose a better name? Does "set_test_user_env()" makes sense?

>
>> +    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?

Yeah, I like check_original_entries, thanks!

>
>> +    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".
OK

>
>> +
>> +    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).

Well, I'm not sure I agree with this. I'm not thrilled to be adding prefixes. However I understand that the test function is quite long now. But IMO this trade-off is OK. I'll ask other what they prefer. (Hope that's fine)

>
>> +
>> +    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?

After override you have to restart SSSD to make the change available.

>
>> +
>> +    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?

If you mean name overrides that should be dropped ATM then yes, I can add the check.

>
>> +
>> +    # reload overrides
>> +    subprocess.call(["sss_override", "user-import", override_user_filename])
>> +
>> +    restart_sssd()
>
> Why do we need to restart SSSD here?

I'm not so sure as with override but I think so. I'll ask the author.

>
>> +
>> +    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.

OK, I'll give it a thought.
>
> 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
AFAIK this is not supported.
>          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.
Yes, that's true. On the other hand only because of complexity of the tested environment we found out about 'use after free' bug, however we would find that probably even with simple test run under valgrind...
>
> 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.

OK
>
> Thank you!
>
> Nick
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


More information about the sssd-devel mailing list