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(a)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(a)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"
+
+
+(a)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
+
+
+(a)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?
+
+
+(a)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@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@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@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@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@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@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@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