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 <pcech(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/>.
> > > +#
> > > +
> > //snip
> >
> > > +(a)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 <pcech(a)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 <pcech(a)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 <pcech(a)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 <pcech(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 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
+
+
+(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_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)
+
+
+(a)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 == []
+
+
+(a)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?
+
+(a)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")]
+
+
+(a)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")])
+
+
+(a)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?
+
+(a)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")])
+
+
+(a)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