[SSSD] [PATCHSET] intg: sss_override

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Wed Oct 7 15:40:04 UTC 2015


On 10/07/2015 03:25 PM, Pavel Reichl wrote:
> Hello,
>
> please see first version of patch set. Please consider this to be work in progress.
>
> The test 'test_local_override_group' fails, there seem to be a bug in sss_override. Pavel is working on the patch. He kindly provided me with early version of patch and it fixed the test.
>
> Currently also test for #2790 test_local_overrides_regr_2790() is failing which is probably just bug in the test itself.
>
> Thanks for any comments.

Yay, more tests :)! I had a quick look and left some comments below.

> 0001-intg-Support-users-with-arb.-gecos-homedir-shell.patch
>
>
>  From f8068960a2d393ca180ac40687620bbb4deb003c Mon Sep 17 00:00:00 2001
> From: Pavel Reichl<preichl at redhat.com>
> Date: Mon, 5 Oct 2015 10:00:41 -0400
> Subject: [PATCH 1/5] intg: Support users with arb. gecos, homedir, shell
>
> Add support for setting arbitrary values of user attribute: gecos,
> homedir and shell.

I have a similar patch in my "intg: Add more LDAP tests" patch set:
https://lists.fedorahosted.org/pipermail/sssd-devel/2015-September/025045.html

We can use either mine (after modifications) or yours.

> Resolves:
> https://fedorahosted.org/sssd/ticket/2732
> ---
>   src/tests/intg/ldap_ent.py | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/src/tests/intg/ldap_ent.py b/src/tests/intg/ldap_ent.py
> index 30eed9d6426f47401712fd88dee7f11bd747b944..4f1e259cd707356dc6c7e819bbd0fc5195f1f1a9 100644
> --- a/src/tests/intg/ldap_ent.py
> +++ b/src/tests/intg/ldap_ent.py
> @@ -18,13 +18,13 @@
>   #
>
>
> -def user(base_dn, uid, uidNumber, gidNumber):
> +def user(base_dn, uid, uidNumber, gidNumber, gecos, homedir, shell):

I'm going a kwargs approach, but I like yours better. However, could you
please keep the argument names matching actual LDAP attribute names?
I.e. use "homeDirectory" and "loginShell" instead. Could you assign
defaults to these arguments here as well? Sometimes it can be useful to use
this function directly. E.g. when creating just one user, as I do in my patch
set.

Also, will we ever need to specify "cn", or "sn" as well?

>       """
>       Generate an RFC2307(bis) user add-modlist for passing to ldap.add*
>       """
>       uidNumber = str(uidNumber)
>       gidNumber = str(gidNumber)
> -    return (
> +    attrs = (
>           "uid=" + uid + ",ou=Users," + base_dn,
>           [
>               ('objectClass',     ['top', 'inetOrgPerson', 'posixAccount']),
> @@ -33,11 +33,15 @@ def user(base_dn, uid, uidNumber, gidNumber):
>               ('uidNumber',       [uidNumber]),
>               ('gidNumber',       [gidNumber]),
>               ('userPassword',    ['Password' + uidNumber]),
> -            ('homeDirectory',   ['/home/' + uid]),
> -            ('loginShell',      ['/bin/bash']),
> +            ('homeDirectory',   [homedir if homedir else '/home/' + uid]),
> +            ('loginShell',      [shell if shell else '/bin/bash']),
>           ]
>       )
>
> +    if gecos:
> +        attrs[1].append(('gecos', [gecos]))
> +    return attrs
> +

Just checking a string for truth wouldn't be sufficient, because an empty
string is False and we might need to pass it sometimes. So, I think, we need
to test against None here and above.

>
>   def group(base_dn, cn, gidNumber, member_uids=[]):
>       """
> @@ -85,11 +89,11 @@ class List(list):
>       def __init__(self, base_dn):
>           self.base_dn = base_dn
>
> -    def add_user(self, uid, uidNumber, gidNumber,
> -                 base_dn=None):
> +    def add_user(self, uid, uidNumber, gidNumber, gecos=None,
> +                 homedir=None, shell=None, base_dn=None):
>           """Add an RFC2307(bis) user add-modlist."""
>           self.append(user(base_dn or self.base_dn,
> -                         uid, uidNumber, gidNumber))
> +                         uid, uidNumber, gidNumber, gecos, homedir, shell))
>
>       def add_group(self, cn, gidNumber, member_uids=[],
>                     base_dn=None):
> -- 2.4.3
>
>
> 0002-intg-new-test-for-user-local-overrides.patch
>
>
>  From e4028b9a81c498a20de5c12a1f0a788be7608463 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 2/5] intg: new test for user local overrides
>
> Introduce a new integration test for local view overrides.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2732
> ---
>   src/tests/intg/ldap_local_override_test.py | 376 +++++++++++++++++++++++++++++
>   1 file changed, 376 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..35e47b012fdd271dd50b4c2ce3749d28a7a746d8
> --- /dev/null
> +++ b/src/tests/intg/ldap_local_override_test.py
> @@ -0,0 +1,376 @@
> +#
> +# LDAP integration test

Could you please put a more specific description here?

> +#
> +# 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):
> + at pytest.fixture(scope="module")
> +def ldap_conn(request, ds_inst):
> +def create_ldap_fixture(request, ldap_conn, ent_list):
> +def create_conf_fixture(request, contents):
> +def stop_sssd():
> +def start_sssd():
> +def restart_sssd():
> +def create_sssd_fixture(request):

I really don't like how we keep copying this stuff around. I'll look into
making a module with these.

> +
> +def load_data_to_ldap(request, ldap_conn):
> +    ent_list = ldap_ent.List(LDAP_BASE_DN)
> +    ent_list.add_user("bwayne", 10001, 20001,
> +                      gecos='Bruce Wayne',
> +                      shell='/bin/bash',
> +                      homedir='/home/gotham')
> +
> +    ent_list.add_user("ckent", 10002, 20001,
> +                      gecos='Clark Joseph Kent',
> +                      shell='/bin/sh',
> +                      homedir='/home/smallville')
> +
> +    ent_list.add_user("pparker", 10003, 20001)
> +
> +    ent_list.add_user("mmccready", 10004, 20001,
> +                      gecos='Mindy McCready',
> +                      shell='/bin/dash',
> +                      homedir='/home/nowhere')
> +
> +    ent_list.add_group("superheroes", 2001,
> +                       ["bwayne", "ckent", "pparker", "mmccready"])
> +

These accounts are fun, sure, but they don't make it easier to figure out
their intended purpose and what the test does. Can we name them, their
homedirs and shells closer to their actual use in the tests? E.g.
"overriden_user", "original_user", "/bin/original_shell", or some such?

BTW, the latest Batman game ("Arkham Knight") is quite good.

> +def check_users_0():
> +def check_users_1():

Could you please add descriptions for all functions, especially these two?

> +    # clean up - remove override file
> +    subprocess.check_call(["rm", override_filename])

All cleanup needs to be happening in the fixture teardown not in tests,
because otherwise it won't happen if the test fails.

Correspondingly, there should be no environment modifications done in the
tests which are not cleaned up by their fixture teardowns.

Nick


More information about the sssd-devel mailing list