On (19/08/16 13:35), Petr Cech wrote: >On 08/19/2016 01:00 PM, Petr Cech wrote: >> On 08/18/2016 12:39 PM, Lukas Slebodnik wrote: >> > On (17/08/16 14:32), Petr Cech wrote: >> > > Hello list, >> > > >> > > there is attached patch set for intg. testing of ldap nested netgroups. >> > > >> > > I used last version of Lukas patch 'sssd_netgroup.py: Resolve nested >> > > netgroups'. I don't know if it is on list. >> > > >> > > It is still WIP. It is in state that it is possible to run it. >> > > But there are comments in code what is needed to fix. >> > > >> > > >> > > If I remove some netgroups (in test), it is updated on LDAP and in >> > > cache, but >> > > sssd_netgroup.get_sssd_netgroups() returns nothing. >> > > >> > Yes, I was too strict regarding to failures in resolving nested >> > netgroups. >> > glibc(getent netgroup) does not fail if there is non-existing nested >> > netgroup. >> > >> > There is bunch of pep8 warnings. Please fix them. >> > sh$ pep8 src/tests/intg/test_netgroup.py | wc -l >> > 29 >> > >> > > Date: Wed, 17 Aug 2016 13:58:30 +0200 >> > > Subject: [PATCH 2/2] WIP: INTG: Tests for ldap nested netgroups >> > > >> > > This patch adds tests on reproducer of t2841. >> > > >> > > Resolves: >> > > https://fedorahosted.org/sssd/ticket/2841 >> > > --- >> > > src/tests/intg/Makefile.am | 1 + >> > > src/tests/intg/test_netgroup.py | 487 >> > > ++++++++++++++++++++++++++++++++++++++++ >> > > 2 files changed, 488 insertions(+) >> > > create mode 100644 src/tests/intg/test_netgroup.py >> > > >> > > diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am >> > > index >> > > b8cc5c006845f911d8518df815925455482e9f6d..b3c553539d9e74dae986fa6551041544dd687c11 >> > > 100644 >> > > --- a/src/tests/intg/Makefile.am >> > > +++ b/src/tests/intg/Makefile.am >> > > @@ -14,6 +14,7 @@ dist_noinst_DATA = \ >> > > util.py \ >> > > test_memory_cache.py \ >> > > test_ts_cache.py \ >> > > + test_netgroup.py \ >> > > $(NULL) >> > > >> > > config.py: config.py.m4 >> > > diff --git a/src/tests/intg/test_netgroup.py >> > > b/src/tests/intg/test_netgroup.py >> > > new file mode 100644 >> > > index >> > > 0000000000000000000000000000000000000000..4be2b1c7048f3d8dc4797557d6decf1367eea36d >> > > >> > > --- /dev/null >> > > +++ b/src/tests/intg/test_netgroup.py >> > > @@ -0,0 +1,487 @@ >> > > +# >> > > +# Netgroup integration test >> > > +# >> > > +# Copyright (c) 2016 Red Hat, Inc. >> > > +# Author: Petr Cech <pc...@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/>. >> > > +# >> > > + >> > //snip >> > >> > > +@pytest.fixture >> > > +def reproducer_t2841(request, ldap_conn): >> > > + ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >> > > + >> > > + ent_list.add_netgroup("t2841_netgroup1", ["(host1,user1,domain1)"]) >> > > + ent_list.add_netgroup("t2841_netgroup2", ["(host2,user2,domain2)"]) >> > > + ent_list.add_netgroup("t2841_netgroup3", [], >> > > + ["t2841_netgroup1", "t2841_netgroup2"]) >> > > + >> > > + create_ldap_fixture(request, ldap_conn, ent_list) >> > > + conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=True) >> > > + create_conf_fixture(request, conf) >> > > + create_sssd_fixture(request) >> > > + return (ldap_conn, ent_list) >> > > + >> > > + >> > > +def test_reproducer_t2841(reproducer_t2841): >> > > + """ >> > > + Adding two nested netgroup. >> > > + """ >> > > + >> > > + ldap_conn = reproducer_t2841[0] >> > > + ent_list = reproducer_t2841[1] >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup1") >> > > + assert res == sssd_netgroup.NssReturnCode.SUCCESS >> > > + assert netgroups == [('host1', 'user1', 'domain1')] >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup2") >> > > + assert res == sssd_netgroup.NssReturnCode.SUCCESS >> > > + assert netgroups == [('host2', 'user2', 'domain2')] >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup3") >> > > + assert res == sssd_netgroup.NssReturnCode.SUCCESS >> > > + assert netgroups == [('host2', 'user2', 'domain2'), >> > > + ('host1', 'user1', 'domain1')] >> > > + >> > > + # removing of t2841_netgroup1 >> > > + ldap_conn.delete_s(ent_list[0][0]) >> > > + ent_list.remove(ent_list[0]) >> > > + if subprocess.call(["sss_cache", "-N"]) != 0: >> > > + raise Exception("sssd_cache failed") >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup1") >> > > + assert res == sssd_netgroup.NssReturnCode.NOTFOUND >> > > + assert netgroups == [] >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup2") >> > > + assert res == sssd_netgroup.NssReturnCode.SUCCESS >> > > + assert netgroups == [('host2', 'user2', 'domain2')] >> > > + >> > > + # FIX: This should be SUCCES >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup3") >> > > + assert res == sssd_netgroup.NssReturnCode.NOTFOUND >> > > + assert netgroups == [] >> > Please never write test which always passes. >> > Test must cover the test case and not be green in 100% cases. >> > Otherwise we cousl have "assert True" everywhere :-) >> > >> > > + >> > > + # point A >> > > + # run_shell() >> > > + >> > > + # removing of t2841_netgroup2 >> > > + ldap_conn.delete_s(ent_list[0][0]) >> > > + ent_list.remove(ent_list[0]) >> > > + if subprocess.call(["sss_cache", "-N"]) != 0: >> > > + raise Exception("sssd_cache failed") >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup1") >> > > + assert res == sssd_netgroup.NssReturnCode.NOTFOUND >> > > + assert netgroups == [] >> > > + >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup2") >> > > + assert res == sssd_netgroup.NssReturnCode.NOTFOUND >> > > + assert netgroups == [] >> > > + >> > > + # FIX: This should be SUCCES >> > > + res, errno, netgroups = >> > > sssd_netgroup.get_sssd_netgroups("t2841_netgroup3") >> > > + assert res == sssd_netgroup.NssReturnCode.NOTFOUND >> > > + assert netgroups == [] >> > > + >> > > + # point B >> > > + # run_shell() >> > >> > I fixed the expected results(@see patch) and the test passes with latest >> > version of sssd_netgroup.py and git master. There are two possible >> > explanations >> > a) we need to close ticket 2841 as works for me >> > b) the test test_reproducer_t2841 is testing something different. >> > >> > BTW I tested the same test-case outside of cwrap with simple shell >> > script. >> > >> > sh# ldapadd -x -w Secret123 -D 'cn=admin,dc=example,dc=com' -h >> > localhost:10389 \ >> > -f netgroups.ldif >> > adding new entry "ou=Netgroups,dc=example,dc=com" >> > >> > adding new entry "cn=t2841_netgroup1,ou=Netgroups,dc=example,dc=com" >> > >> > adding new entry "cn=t2841_netgroup2,ou=Netgroups,dc=example,dc=com" >> > >> > adding new entry "cn=t2841_netgroup3,ou=Netgroups,dc=example,dc=com" >> > >> > sh# systemctl restart sssd >> > >> > sh# systemctl restart sssd >> > sh# getent netgroup t2841_netgroup1 || echo FAILED >> > t2841_netgroup1 (host1,user1,domain1) >> > sh# getent netgroup t2841_netgroup2 || echo FAILED >> > t2841_netgroup2 (host2,user2,domain2) >> > sh# getent netgroup t2841_netgroup3 || echo FAILED >> > t2841_netgroup3 (host2,user2,domain2) (host1,user1,domain1) >> > >> > >> > sh# ldapdelete -x -w Secret123 -D 'cn=admin,dc=example,dc=com' -h >> > localhost:10389 cn=t2841_netgroup1,ou=Netgroups,dc=example,dc=com >> > sh# sss_cache -N >> > >> > sh# getent netgroup t2841_netgroup1 || echo FAILED >> > FAILED >> > sh# getent netgroup t2841_netgroup2 || echo FAILED >> > t2841_netgroup2 (host2,user2,domain2) >> > sh# getent netgroup t2841_netgroup3 || echo FAILED >> > t2841_netgroup3 (host2,user2,domain2) >> > >> > sh# ldapdelete -x -w Secret123 -D 'cn=admin,dc=example,dc=com' -h >> > localhost:10389 cn=t2841_netgroup2,ou=Netgroups,dc=example,dc=com >> > sh# sss_cache -N >> > >> > sh# getent netgroup t2841_netgroup1 || echo FAILED >> > FAILED >> > sh# getent netgroup t2841_netgroup2 || echo FAILED >> > FAILED >> > sh# getent netgroup t2841_netgroup3 || echo FAILED >> > t2841_netgroup3 > >This is only the attempt to change Subject. > >Important part from previous mail: > >> > Could you explain it? >> >> The situation is: >> netgroups: A, B, C >> B and C in A >> >> I remove B and C, but the right way is >> remove B, C from A, >> not B, C themself. >> >> > LS >> >> Hello, >> >> there is whole patch set for >> https://fedorahosted.org/sssd/ticket/2841 >> attached. >> >> The first patch fixes the issue, >> the second patch adds support for netgroups to INTG tests, >> the third adds tests. >> >> This patch set is applicable after Lukas's patch >> sssd_netgroup.py: Resolve nested netgroups >> > >-- >Petr^4 Čech
>From 94d790db56243419fc0629291bc2b19a773aa851 Mon Sep 17 00:00:00 2001 >From: Petr Cech <pc...@redhat.com> >Date: Fri, 22 Jul 2016 14:28:54 +0200 >Subject: [PATCH 1/3] LDAP: Fixing of removing netgroup from cache > >There were problem with local key which wasn't properly removed. >This patch fixes it. > >Resolves: >https://fedorahosted.org/sssd/ticket/2841 >--- > src/providers/ldap/sdap_async_netgroups.c | 40 +++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > >diff --git a/src/providers/ldap/sdap_async_netgroups.c >b/src/providers/ldap/sdap_async_netgroups.c >index >df233d956df70cfcb5f68bd2afc9e2a23c50c3bb..cf7d7b12361f8cc578b891961c0c5566442f1b4e > 100644 >--- a/src/providers/ldap/sdap_async_netgroups.c >+++ b/src/providers/ldap/sdap_async_netgroups.c >@@ -38,6 +38,35 @@ bool is_dn(const char *str) > return (ret == LDAP_SUCCESS ? true : false); > } > >+static errno_t add_to_missing_attrs(TALLOC_CTX * mem_ctx, >+ struct sysdb_attrs *attrs, >+ const char *ext_key, >+ char ***_missing) >+{ >+ bool is_present = false; >+ size_t size = 0; >+ size_t ret; >+ >+ for (int i = 0; i < attrs->num; i++) { >+ if (strcmp(ext_key, attrs->a[i].name) == 0) { >+ is_present = true; >+ } >+ size++; >+ } >+ >+ if (is_present == false) { >+ ret = add_string_to_list(attrs, ext_key, _missing); >+ if (ret != EOK) { >+ goto done; >+ } >+ } >+ >+ ret = EOK; >+ >+done: >+ return ret; >+} >+ > static errno_t sdap_save_netgroup(TALLOC_CTX *memctx, > struct sss_domain_info *dom, > struct sdap_options *opts, >@@ -138,6 +167,17 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx, > goto fail; > } > >+ /* Prepare SYSDB_NETGROUP_MEMBER removing >+ * if not present in netgroup_attrs >+ */ >+ ret = add_to_missing_attrs(attrs, netgroup_attrs, SYSDB_NETGROUP_MEMBER, >+ &missing); >+ if (ret != EOK) { >+ DEBUG(SSSDBG_CRIT_FAILURE, "Failed to add [%s] to missing >attributes\n", >+ SYSDB_NETGROUP_MEMBER); >+ goto fail; >+ } >+ Thank you for test but I would appraciate a little bit simpler solution. "memberNisNetgroup"(SYSDB_NETGROUP_MEMBER) Your patch will append "memberNisNetgroup"(SYSDB_NETGROUP_MEMBER) to missing attributes if it is not in netgroup_attrs. However "memberNisNetgroup" and "originalMemberNisNetgroup" are tightly coupled attributes in sysdb. * the sysdb attributes "originalMemberNisNetgroup" contain the original value from LDAP which can be name of netgroup of dn. * the sysdb attributes "memberNisNetgroup" always contain shortname used by sssd. They are even the same if memberNisNetgroup in LDAP does not contain dn. One is generated from other. And function list_missing_attrs already find that "originalMemberNisNetgroup" is a missing attribute. Therefore if "originalMemberNisNetgroup" is in missing attibutes list then we have to add there also "memberNisNetgroup". e.g. diff --git a/src/providers/ldap/sdap_async_netgroups.c b/src/providers/ldap/sdap_async_netgroups.c index df233d9..1fe40f5 100644 --- a/src/providers/ldap/sdap_async_netgroups.c +++ b/src/providers/ldap/sdap_async_netgroups.c @@ -138,6 +138,14 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx, goto fail; } + if (string_in_list(SYSDB_ORIG_NETGROUP_MEMBER, missing, false)) { + ret = add_string_to_list(attrs, SYSDB_NETGROUP_MEMBER, &missing); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to add string into list\n"); + goto fail; + } + } + ret = sysdb_add_netgroup(dom, name, NULL, netgroup_attrs, missing, dom->netgroup_timeout, now); if (ret) goto fail; + some optional nice explanation/comment why :-) > ret = sysdb_add_netgroup(dom, name, NULL, netgroup_attrs, missing, > dom->netgroup_timeout, now); > if (ret) goto fail; >-- >2.7.4 > BTW the same bug is also ipa_save_netgroup in "src/providers/ipa/ipa_netgroups.c" But sysdb_add_netgroup is called wit NULL for missing attrinbutes :-) Anyway sdap_save_netgroup and ipa_save_netgroup do almost the same They just use different maps. You touched the netgroup related code. So it would be good to do small refactoring and reuse ldap sdap_save_netgroup in ipa_save_netgroup. So it would be good If you could fix ticket #3117 in recent future. Because you still remember the netgroup related code in LDAP. (It's not a blocker for #2841) It is just a recomendation :-) >From b284c65bb6651cb32d09b3b35599127ce995cc4f Mon Sep 17 00:00:00 2001 >From: Petr Cech <pc...@redhat.com> >Date: Wed, 17 Aug 2016 14:01:09 +0200 >Subject: [PATCH 2/3] INTG: Adding support for netgroups to ldap_ent > >Resolves: >https://fedorahosted.org/sssd/ticket/2841 >--- > src/tests/intg/ldap_ent.py | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > >diff --git a/src/tests/intg/ldap_ent.py b/src/tests/intg/ldap_ent.py >index >f8f2f7fe6977aec6fd704ad1c78a476a163a16f1..f7835a5a9c0a0f50a91c0085f02964cfafcfeebf > 100644 >--- a/src/tests/intg/ldap_ent.py >+++ b/src/tests/intg/ldap_ent.py >@@ -87,6 +87,21 @@ def group_bis(base_dn, cn, gidNumber, member_uids=[], >member_gids=[]): > return ("cn=" + cn + ",ou=Groups," + base_dn, attr_list) > > >+def netgroup(base_dn, cn, triples=[], members=[]): IIRC [] is a aangerous default value as argument and we should use (). You might ask freeIPA/python developers why >+ """ >+ Generate an RFC2307bis netgroup add-modlist for passing to ldap.add*. >+ """ >+ attr_list = [ >+ ('objectClass', ['top', 'nisNetgroup']) >+ ] >+ member_list = [] this variable is unused >+ if len(triples) > 0: >+ attr_list.append(('nisNetgroupTriple', triples)) >+ if len(members) > 0: >+ attr_list.append(('memberNisNetgroup', members)) >+ return ("cn=" + cn + ",ou=Netgroups," + base_dn, attr_list) >+ >+ > class List(list): > """LDAP add-modlist list""" > >@@ -124,3 +139,8 @@ class List(list): > self.append(group_bis(base_dn or self.base_dn, > cn, gidNumber, > member_uids, member_gids)) >+ >+ def add_netgroup(self, cn, triples=[], members=[], base_dn=None): ^^ the same case as in previous function >+ """Add an RFC2307bis netgroup add-modlist.""" >+ self.append(netgroup(base_dn or self.base_dn, >+ cn, triples, members)) >-- >2.7.4 > >From 483b2bf279f47772fddaf4aa7087f6336ddf78b9 Mon Sep 17 00:00:00 2001 >From: Petr Cech <pc...@redhat.com> >Date: Wed, 17 Aug 2016 13:58:30 +0200 >Subject: [PATCH 3/3] INTG: Tests for ldap nested netgroups > >This patch adds tests on reproducer of t2841. > >Resolves: >https://fedorahosted.org/sssd/ticket/2841 >--- > src/tests/intg/Makefile.am | 1 + > src/tests/intg/test_netgroup.py | 490 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 491 insertions(+) > create mode 100644 src/tests/intg/test_netgroup.py > >diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am >index >d73e4216310ccd1c90e6b7eb0a0e60068fc45bd5..75422a4417046116bec11a8a680fe2248e3afb69 > 100644 >--- a/src/tests/intg/Makefile.am >+++ b/src/tests/intg/Makefile.am >@@ -15,6 +15,7 @@ dist_noinst_DATA = \ > test_ldap.py \ > test_memory_cache.py \ > test_ts_cache.py \ >+ test_netgroup.py \ > $(NULL) > > config.py: config.py.m4 >diff --git a/src/tests/intg/test_netgroup.py b/src/tests/intg/test_netgroup.py >new file mode 100644 >index >0000000000000000000000000000000000000000..d004faf2c2f3ee01c1fb41e36820709427c5cbd9 >--- /dev/null >+++ b/src/tests/intg/test_netgroup.py >@@ -0,0 +1,490 @@ >+# >+# Netgroup integration test >+# >+# Copyright (c) 2016 Red Hat, Inc. >+# Author: Petr Cech <pc...@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 sys >+import pwd >+import grp >+import ent Previous 4 imports are unused >+import config >+import signal >+import subprocess >+import time >+import ldap >+import ldap.modlist >+import pytest >+import ds_openldap >+import ldap_ent >+import sssd_netgroup as sssd_ntg >+from util import * It would be good to aoin wildcard import and import just required names (in our case just unindent) It might be a leftover after debugginf with run_shell It would be also good to separate system modules with internam modules empty line might be enough e.g. import os import stat import signal import subprocess import time import ldap import ldap.modlist import pytest import config import ds_openldap import ldap_ent ... >+ >+ >+LDAP_BASE_DN = "dc=example,dc=com" >+INTERACTIVE_TIMEOUT = 4 Unused >+ >+ >+@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 >+ >+ >+@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_entries(ldap_conn, ent_list=None): >+ """Add LDAP entries from ent_list""" >+ if ent_list is not None: >+ for entry in ent_list: >+ ldap_conn.add_s(entry[0], entry[1]) >+ >+ >+def cleanup_ldap_entries(ldap_conn, ent_list=None): >+ """Remove LDAP entries added by create_ldap_entries""" >+ if ent_list is None: >+ for ou in ("Users", "Groups", "Netgroups", "Services", "Policies"): >+ for entry in ldap_conn.search_s("ou=" + ou + "," + >+ ldap_conn.ds_inst.base_dn, >+ ldap.SCOPE_ONELEVEL, >+ attrlist=[]): >+ ldap_conn.delete_s(entry[0]) >+ else: >+ for entry in ent_list: >+ ldap_conn.delete_s(entry[0]) >+ >+ >+def create_ldap_cleanup(request, ldap_conn, ent_list=None): >+ """Add teardown for removing all user/group LDAP entries""" >+ request.addfinalizer(lambda: cleanup_ldap_entries(ldap_conn, ent_list)) >+ >+ >+def create_ldap_fixture(request, ldap_conn, ent_list=None): >+ """Add LDAP entries and add teardown for removing them""" >+ create_ldap_entries(ldap_conn, ent_list) >+ create_ldap_cleanup(request, ldap_conn, ent_list) >+ >+ >+SCHEMA_RFC2307_BIS = "rfc2307bis" >+ >+ >+def format_basic_conf(ldap_conn, schema, enum): >+ """Format a basic SSSD configuration""" >+ schema_conf = "ldap_schema = " + schema + "\n" >+ schema_conf += "ldap_group_object_class = groupOfNames\n" >+ return unindent("""\ >+ [sssd] >+ debug_level = 0xffff >+ domains = LDAP >+ services = nss, pam pam responder is not used; it needn't run >+ >+ [nss] >+ debug_level = 0xffff >+ memcache_timeout = 0 netgroup is not cached in memory cache so you can remove the line >+ >+ [pam] >+ debug_level = 0xffff >+ >+ [domain/LDAP] >+ ldap_auth_disable_tls_never_use_in_production = true PLease remove this line because you are not testing authentication >+ debug_level = 0xffff >+ enumerate = {enum} It would be good to do not test with enumeration and remove argument enum from the function format_basic_conf If you want to test netgroups with enumeration then please add separate tests into test_enumeration >+ {schema_conf} >+ id_provider = ldap >+ auth_provider = ldap >+ ldap_uri = {ldap_conn.ds_inst.ldap_url} >+ ldap_search_base = {ldap_conn.ds_inst.base_dn} >+ ldap_netgroup_search_base = ou=Netgroups,{ldap_conn.ds_inst.base_dn} Do we need enbale debug_level? IMHO it would be needed just for froubleshooting >+ """).format(**locals()) >+ >+ >+def create_conf_file(contents): >+ """Create sssd.conf with specified contents""" >+ conf = open(config.CONF_PATH, "w") >+ conf.write(contents) >+ conf.close() >+ os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR) >+ >+ >+def cleanup_conf_file(): >+ """Remove sssd.conf, if it exists""" >+ if os.path.lexists(config.CONF_PATH): >+ os.unlink(config.CONF_PATH) >+ >+ >+def create_conf_cleanup(request): >+ """Add teardown for removing sssd.conf""" >+ request.addfinalizer(cleanup_conf_file) >+ >+ >+def create_conf_fixture(request, contents): >+ """ >+ Create sssd.conf with specified contents and add teardown for removing it >+ """ >+ create_conf_file(contents) >+ create_conf_cleanup(request) >+ >+ >+def create_sssd_process(): >+ """Start the SSSD process""" >+ if subprocess.call(["sssd", "-D", "-f"]) != 0: >+ raise Exception("sssd start failed") >+ >+ >+def cleanup_sssd_process(): >+ """Stop the SSSD process and remove its state""" >+ try: >+ 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) >+ except: >+ pass >+ 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) >+ >+ >+def create_sssd_cleanup(request): >+ """Add teardown for stopping SSSD and removing its state""" >+ request.addfinalizer(cleanup_sssd_process) >+ >+ >+def create_sssd_fixture(request): >+ """Start SSSD and add teardown for stopping it and removing its state""" >+ create_sssd_process() >+ create_sssd_cleanup(request) >+ >+ >+@pytest.fixture >+def add_nested_netgroup(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("nested_netgroup") Is it an appropriate name? I would say it is a empty netgroup The same applies to the name of fixture and name of following test. >+ >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_add_nested_netgroup(add_nested_netgroup): >+ """ >+ Adding nested netgroup. >+ """ >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("nested_netgroup") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [] >+ >+ >+@pytest.fixture >+def add_two_nested_netgroup(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("nested_netgroup1") >+ ent_list.add_netgroup("nested_netgroup2") The same here name of netgroups are confusing Or did you forgot to use nested netgroups here? >+ >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_add_two_nested_netgroup(add_two_nested_netgroup): >+ """ >+ Adding two nested netgroup. >+ """ >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("nested_netgroup1") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("nested_netgroup2") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [] >+ >+ >+# TODO: Test for removing one by one Is there a reason for todo? or is this line just a leftover? >+ >+@pytest.fixture >+def add_tripled_netgroup(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("tripled_netgroup", ["(host,user,domain)"]) >+ >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_add_tripled_netgroup(add_tripled_netgroup): >+ """ >+ Adding netgroup with trinity. ^^^^^^^^ I think that netgroups terminology would use triplet. >+ """ >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host", "user", "domain")] >+ >+ >+@pytest.fixture >+def add_advanced_tripled_netgroup(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("adv_tripled_netgroup", ["(host1,user1,domain1)", >+ "(host2,user2,domain2)"]) >+ I would recomment to merge this fixture ( + related test) with previous one. It woul a little bit reduce cuplicated code. >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_add_advanced_tripled_netgroup(add_advanced_tripled_netgroup): >+ """ >+ Adding netgroup with two trinities. >+ """ >+ >+ res, errno, netgroups = >sssd_ntg.get_sssd_netgroups("adv_tripled_netgroup") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert sorted(netgroups) == sorted([("host1", "user1", "domain1"), >+ ("host2", "user2", "domain2")]) >+ >+ >+@pytest.fixture >+def add_two_tripled_netgroup(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("tripled_netgroup1", ["(host1,user1,domain1)"]) >+ ent_list.add_netgroup("tripled_netgroup2", ["(host2,user2,domain2)"]) >+ the same as in previous case. The name of netgroups are not conflicting therefore it could be tested in common test (test_simple_netgroups) becasue you are testing just simple netgroups just with triplets and not nested netgroups. >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_add_two_tripled_netgroup(add_two_tripled_netgroup): >+ """ >+ Adding two netgroups, each with trinity. >+ """ >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup1") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host1", "user1", "domain1")] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup2") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host2", "user2", "domain2")] >+ >+ >+# TODO: Test for removing one by one Is there a reason for todo? or is it an leftover? >+ >+@pytest.fixture >+def add_mixed_netgroup(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("mixed_netgroup1", [], []) >+ ent_list.add_netgroup("mixed_netgroup2", [], ["mixed_netgroup1"]) >+ >+ ent_list.add_netgroup("mixed_netgroup3", ["(host1,user1,domain1)"], []) >+ ent_list.add_netgroup("mixed_netgroup4", >+ ["(host2,user2,domain2)", "(host3,user3,domain3)"], >+ []) >+ >+ ent_list.add_netgroup("mixed_netgroup5", >+ ["(host4,user4,domain4)"], >+ ["mixed_netgroup1"]) >+ ent_list.add_netgroup("mixed_netgroup6", >+ ["(host5,user5,domain5)"], >+ ["mixed_netgroup2"]) >+ >+ ent_list.add_netgroup("mixed_netgroup7", [], ["mixed_netgroup3"]) >+ ent_list.add_netgroup("mixed_netgroup8", [], >+ ["mixed_netgroup3", "mixed_netgroup4"]) >+ >+ ent_list.add_netgroup("mixed_netgroup9", >+ ["(host6,user6,domain6)"], >+ ["mixed_netgroup3", "mixed_netgroup4"]) >+ >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_add_mixed_netgroup(add_mixed_netgroup): >+ """ >+ Adding many netgroups of different type. >+ """ >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup1") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup2") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup3") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host1", "user1", "domain1")] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup4") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert sorted(netgroups) == sorted([("host2", "user2", "domain2"), >+ ("host3", "user3", "domain3")]) >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup5") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host4", "user4", "domain4")] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup6") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host5", "user5", "domain5")] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup7") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [("host1", "user1", "domain1")] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup8") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert sorted(netgroups) == sorted([("host1", "user1", "domain1"), >+ ("host2", "user2", "domain2"), >+ ("host3", "user3", "domain3")]) >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup9") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert sorted(netgroups) == sorted([("host1", "user1", "domain1"), >+ ("host2", "user2", "domain2"), >+ ("host3", "user3", "domain3"), >+ ("host6", "user6", "domain6")]) >+ >+ >+@pytest.fixture >+def reproducer_t2841(request, ldap_conn): >+ ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) >+ >+ ent_list.add_netgroup("t2841_netgroup1", ["(host1,user1,domain1)"]) >+ ent_list.add_netgroup("t2841_netgroup2", ["(host2,user2,domain2)"]) >+ ent_list.add_netgroup("t2841_netgroup3", [], >+ ["t2841_netgroup1", "t2841_netgroup2"]) >+ >+ create_ldap_fixture(request, ldap_conn, ent_list) >+ conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False) >+ create_conf_fixture(request, conf) >+ create_sssd_fixture(request) >+ return None >+ >+ >+def test_reproducer_t2841(reproducer_t2841, ldap_conn): It would be good to use more desciptive name. So it is immediately visible in pytest output what you want to test. The pytest output is very reduced for green test. and reproducer_t2841 is not very descriptive. What about test_removing_nested_netgroups? >+ """ >+ Regression test for ticket 2841. >+ https://fedorahosted.org/sssd/ticket/2841 >+ """ >+ >+ t2841_netgroup3_dn = 'cn=t2841_netgroup3,ou=Netgroups,dc=example,dc=com' >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host1', 'user1', 'domain1')] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host2', 'user2', 'domain2')] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert sorted(netgroups) == sorted([('host1', 'user1', 'domain1'), >+ ('host2', 'user2', 'domain2')]) >+ >+ # removing of t2841_netgroup1 from t2841_netgroup3 >+ old = {'memberNisNetgroup': ["t2841_netgroup1", "t2841_netgroup2"]} >+ new = {'memberNisNetgroup': ["t2841_netgroup2"]} >+ >+ ldif = ldap.modlist.modifyModlist(old, new) >+ ldap_conn.modify_s(t2841_netgroup3_dn, ldif) >+ >+ if subprocess.call(["sss_cache", "-N"]) != 0: >+ raise Exception("sssd_cache failed") >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host1', 'user1', 'domain1')] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host2', 'user2', 'domain2')] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host2', 'user2', 'domain2')] >+ >+ # removing of t2841_netgroup2 from t2841_netgroup3 >+ old = {'memberNisNetgroup': ["t2841_netgroup2"]} >+ new = {'memberNisNetgroup': []} >+ >+ ldif = ldap.modlist.modifyModlist(old, new) >+ ldap_conn.modify_s(t2841_netgroup3_dn, ldif) >+ >+ if subprocess.call(["sss_cache", "-N"]) != 0: >+ raise Exception("sssd_cache failed") >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host1', 'user1', 'domain1')] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [('host2', 'user2', 'domain2')] >+ >+ res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3") >+ assert res == sssd_ntg.NssReturnCode.SUCCESS >+ assert netgroups == [] BTW in all cases you are tesing positive results So the errno variabel is never used. It make sense to check errno just for failures. So you can replace errno with _ which means taht you are intentionaly ingoring the value. Than you very much for test. I really appreciate it. Now wi will be sure that there woudl not be a regression for the bug and we can refactor code without any worries :-) I had a many comments but they were mostly nitpicking. One more time thank you very much and good job. LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
