Tests: Test transformation of bash-ldap-id-ldap-auth netgroup #7648

aborah-sudo · 2024-10-16T03:47:29Z

Test transformation of bash-ldap-id-ldap-auth netgroup

ikerexxe

Why are you using a specially created function to run libc's group membership, when the framework already provides an automation to check this?

As an example test_netgroups__add_remove_netgroup_member uses client.tools.getent.netgroup("ng-2") to check the membership.

danlavu

Please reference current code for examples to better conform to the new coding standards. Let's pause any other PRs until this one meets the new guidelines.

src/tests/system/tests/test_netgroups.py

danlavu · 2024-10-18T05:58:06Z

This is much better, but you resolved an issue that still needs to be resolved. These tests are specific to LDAP should go in test_ldap.py. Is there a reason why we cannot use the generic provider? We want to start using this for more coverage and reduce the amount of test cases.

Not related to this PR, here is an example of using one test to cover the same scenario for AD/IPA/LDAP/Samba, 12 lines of code instead of copying the test case. https://github.com/SSSD/sssd/blob/master/src/tests/system/tests/test_failover.py

I'll take a closer look later, all the checks need to be green before we merge, please resolve the ci failures, like the 'static code analysis' and the ci system tests.

aborah-sudo · 2024-10-18T06:33:20Z

This is much better, but you resolved an issue that still needs to be resolved. These tests are specific to LDAP should go in test_ldap.py. Is there a reason why we cannot use the generic provider? We want to start using this for more coverage and reduce the amount of test cases.

Not related to this PR, here is an example of using one test to cover the same scenario for AD/IPA/LDAP/Samba, 12 lines of code instead of copying the test case. https://github.com/SSSD/sssd/blob/master/src/tests/system/tests/test_failover.py

I'll take a closer look later, all the checks need to be green before we merge, please resolve the ci failures, like the 'static code analysis' and the ci system tests.

These are ldap specific tests. I have tries running them against GenericProvider but its failed . So moving them to _test_ldap.py

src/tests/system/tests/test_netgroups.py

src/tests/system/tests/test_ldap.py

src/tests/system/tests/test_netgroups.py

ikerexxe

I've consulted Sumit and although the netgroup functionality is provided, IPA's exposed interface is not the same as in LDAP or AD's environment. So I have an important question before I continue reviewing the code. Ultimately what do we want to test? Do we want to test that the high-level functionality works, or that the functionality of each provider works? According to the response, we can decide what to do with this PR.

aborah-sudo · 2024-10-28T13:10:21Z

I've consulted Sumit and although the netgroup functionality is provided, IPA's exposed interface is not the same as in LDAP or AD's environment. So I have an important question before I continue reviewing the code. Ultimately what do we want to test? Do we want to test that the high-level functionality works, or that the functionality of each provider works? According to the response, we can decide what to do with this PR.

The high-level functionality works

ikerexxe

Some minor comments added. I think there is not much time left to approve it.

src/tests/system/tests/test_netgroups.py

ikerexxe · 2024-10-29T15:17:37Z

src/tests/system/tests/test_netgroups.py

+    client.sssd.restart()
+
+    member = client.tools.getent.netgroup("nested_netgroup").members
+    assert "(testhost1,ng1,ldap.test)" in member
+    assert "(-,ng3,)" in member
+    assert "(testhost5,ng2,ldap.test)" in member


Why is this needed? Are you expecting some change may happen?

No, this is assertion to make sure everything works

What do you mean by everything? What is the exact use case you are trying to test?

After sssd restart i want to make sure that Nesting netgroups works and i can remove that part if it does not make sense .

IMO it doesn't. @danlavu what do you think? Does it make sense to check again the nesting groups after restarting sssd?

src/tests/system/tests/test_netgroups.py

danlavu

Overall it looks good, mostly grammatical changes requested. I didn't go into why, the change, but if you would like to explain why moving forward I'm happy to do so or I can just provide the change.

There is one part of 'test_netgroup__lookup_nested_groups' that is out scope, thanks Anuj, good work.

danlavu · 2024-11-12T03:44:05Z

src/tests/system/tests/test_netgroups.py

+    for id in [1, 2]:
+        provider.user(f"ng{id}").add()
+
+    netgroup_qa = provider.netgroup("QAUsers").add()


We should change the group names, it's more concise to call them "group" and "nested_group".

danlavu · 2024-11-12T03:48:55Z

src/tests/system/tests/test_netgroups.py

+    """
+    :title: Netgroup contains a member that has a host and domain specified.
+    :setup:
+        1. Create users, groups.


'Create users and groups.'

danlavu · 2024-11-12T03:48:57Z

src/tests/system/tests/test_netgroups.py

+@pytest.mark.topology(KnownTopology.LDAP)
+@pytest.mark.topology(KnownTopology.AD)
+@pytest.mark.topology(KnownTopology.Samba)
+def test_netgroup__host_and_domain(client: Client, provider: GenericProvider, user: str, domain: str, expected: str):


A more descriptive name, like
'test_netgroup__lookup_nested_groups_with_host_and_domain_values_present'

danlavu · 2024-11-12T03:49:08Z

src/tests/system/tests/test_netgroups.py

+    """
+    :title: Nesting netgroups and verifying user memberships using LDAP with sssd.
+    :setup:
+        1. Create users, groups.


'Create users and groups.'

danlavu · 2024-11-12T03:50:22Z

src/tests/system/tests/test_netgroups.py

+@pytest.mark.topology(KnownTopology.Samba)
+def test_netgroup__lookup_nested_groups(client: Client, provider: GenericProvider):
+    """
+    :title: Nesting netgroups and verifying user memberships using LDAP with sssd.


'Looking up nested netgroups' is sufficient as the title.

danlavu · 2024-11-12T04:02:06Z

src/tests/system/tests/test_netgroups.py

+        5. Add Circular Netgroup Nesting to nested_netgroup
+        6. Start sssd
+    :steps:
+        1. Retrieves all members of the "nested_netgroup" group using the getent netgroup tool.


'Lookup nested_netgroup'

We can drop steps 2 and 3.

danlavu · 2024-11-12T04:02:39Z

src/tests/system/tests/test_netgroups.py

+        3. Checks if a user who is not in any netgroup is part of "nested_netgroup".
+        4. After the SSSD restart, it retrieves the members of "nested_netgroup" again to ensure they still intact.
+    :expectedresults:
+        1. All members of the "nested_netgroup" group be there


"Netgroup is found, and both netgroups and users are members"

We can drop all the other steps.

danlavu · 2024-11-12T04:05:36Z

src/tests/system/tests/test_netgroups.py

+    """
+    :title: User's 'memberNisNetgroup' attribute values are the DN of the group.
+    :setup:
+        1. Create users, groups.


I made a lot of comments to the following test case, all of those comments apply here.

danlavu · 2024-11-12T04:05:41Z

src/tests/system/tests/test_netgroups.py

+    """
+    :title: User's 'memberNisNetgroup' attribute values are the DN of the group.
+    :setup:
+        1. Create users, groups.


I made a lot of comments to the following test case, all of those comments apply here.

danlavu · 2024-11-12T04:06:42Z

src/tests/system/tests/test_netgroups.py

+    client: Client, provider: GenericProvider, operation: str
+):
+    """
+    :title: User's 'memberNisNetgroup' attribute values are the DN of the group.


Remove . from all titles and steps. Let's try and make it consistent.

danlavu · 2024-11-12T15:15:53Z

src/tests/system/tests/test_netgroups.py

+    :setup:
+        1. Create users and groups
+        2. Create netgroups and add member
+        3. Add Members to nested_netgroup


lower case M in members

danlavu · 2024-11-12T15:19:43Z

src/tests/system/tests/test_netgroups.py

+    :setup:
+        1. Create users and groups
+        2. Create netgroups and add member
+        3. Start sssd


danlavu · 2024-11-12T15:22:49Z

src/tests/system/tests/test_netgroups.py

+    :steps:
+        1.  Check whether the expected member is present in the nested_group netgroup
+    :expectedresults:
+        1. Member is present in the nested_group netgroup


This is confusing, either

'Member is present in nested netgroup'
"Member is present in netgroup 'nested_netgroup'"

danlavu · 2024-11-12T15:28:46Z

src/tests/system/tests/test_netgroups.py

+        2. Create netgroups and add member
+        3. Start sssd
+    :steps:
+        1.  Check whether the expected member is present in the nested_group netgroup


'Lookup netgroup', the extra wording is not necessary.

danlavu · 2024-11-12T15:29:37Z

src/tests/system/tests/test_netgroups.py

+        4. Make netgroup and nested_netgroup members of one another, looping the groups
+        5. Start sssd
+    :steps:
+        1. Lookup nested_netgroup


Let's keep names, in quotes,

Lookup 'nested_netgroup'

danlavu · 2024-11-12T15:32:18Z

src/tests/system/tests/test_netgroups.py

+    :setup:
+        1. Create users and groups
+        2. Create netgroups and add member
+        3. Add Members to nested_netgroup


Quotes, if you are using the variable name.

Add members to netgroup 'nested_netgroup'
or

Add members to nested netgroup

One specifies the actual group name and the other option is more general. I don't care which you choose, but I'd like to see it consistent through the file.

danlavu · 2024-11-12T15:33:02Z

src/tests/system/tests/test_netgroups.py

+        2. Create a new netgroup called group and add a member (ng1) to group
+        3. Create another netgroup named nested_group and add a member (ng2) to nested_group
+        4. Modify the nested_group to replace its members with the members of group
+        5. Start sssd


uppercase SSSD

ikerexxe

Just a minor improvement. You should also check the python-system-tests result since mypy is reporting several errors.

ikerexxe · 2024-11-13T08:17:13Z

src/tests/system/tests/test_netgroups.py

+        2. Create netgroups and add member
+        3. Start SSSD
+    :steps:
+        1.  Lookup netgroup


There is an extra space between the period and the text

danlavu

Just the three minor typos, but approved. Thank you Anuij.

danlavu · 2024-11-13T18:45:35Z

src/tests/system/tests/test_netgroups.py

+        2. Create netgroups and add member
+        3. Start SSSD
+    :steps:
+        1. Lookup netgroup


Lookup netgroup "nested_group"

danlavu · 2024-11-13T18:46:49Z

src/tests/system/tests/test_netgroups.py

+    :title: Netgroup contains a member that has a host and domain specified
+    :setup:
+        1. Create users and groups
+        2. Create netgroups and add member


'members' plural

danlavu · 2024-11-13T18:47:05Z

src/tests/system/tests/test_netgroups.py

+    :title: Looking up nested netgroups
+    :setup:
+        1. Create users and groups
+        2. Create netgroups and add member


'members' plural

Test transformation of bash-ldap-id-ldap-auth netgroup

aborah-sudo added Waiting for review Tests labels Oct 16, 2024

aborah-sudo requested review from ikerexxe and shridhargadekar October 16, 2024 03:47

aborah-sudo assigned ikerexxe and shridhargadekar Oct 16, 2024

ikerexxe reviewed Oct 16, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch from 0bd6e30 to 1c3d47d Compare October 16, 2024 11:53

aborah-sudo requested a review from ikerexxe October 16, 2024 11:54

danlavu requested changes Oct 18, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch from 1c3d47d to 82e0485 Compare October 18, 2024 03:32

github-advanced-security bot found potential problems Oct 18, 2024

View reviewed changes

src/tests/system/tests/test_netgroups.py Fixed Show fixed Hide fixed

aborah-sudo force-pushed the netgroup branch 5 times, most recently from f0c4c4e to 50ad1c3 Compare October 18, 2024 04:54

aborah-sudo requested a review from danlavu October 18, 2024 04:54

aborah-sudo force-pushed the netgroup branch 2 times, most recently from 40b026b to a19c0cb Compare October 18, 2024 05:01

aborah-sudo force-pushed the netgroup branch 8 times, most recently from 78ca7f8 to fe91a15 Compare October 18, 2024 06:56

aborah-sudo force-pushed the netgroup branch from 88a986b to 7834daa Compare October 23, 2024 12:22

aborah-sudo requested a review from danlavu October 23, 2024 12:23

danlavu requested changes Oct 23, 2024

View reviewed changes

src/tests/system/tests/test_netgroups.py Outdated Show resolved Hide resolved

src/tests/system/tests/test_netgroups.py Outdated Show resolved Hide resolved

src/tests/system/tests/test_netgroups.py Outdated Show resolved Hide resolved

aborah-sudo force-pushed the netgroup branch from 7834daa to 4871da8 Compare October 23, 2024 14:00

aborah-sudo requested a review from danlavu October 23, 2024 14:01

danlavu requested changes Oct 23, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch from 4871da8 to 2f12780 Compare October 23, 2024 14:23

aborah-sudo requested a review from danlavu October 23, 2024 14:23

aborah-sudo force-pushed the netgroup branch from 2f12780 to eb2666b Compare October 23, 2024 14:28

ikerexxe reviewed Oct 28, 2024

View reviewed changes

aborah-sudo requested a review from ikerexxe October 28, 2024 13:11

ikerexxe reviewed Oct 29, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch 2 times, most recently from 9b6e65b to bd1e66d Compare October 30, 2024 06:19

aborah-sudo requested a review from ikerexxe October 30, 2024 06:20

danlavu requested changes Nov 12, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch from bd1e66d to abc7d8e Compare November 12, 2024 05:53

aborah-sudo requested a review from danlavu November 12, 2024 05:53

danlavu requested changes Nov 12, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch from abc7d8e to 2d7960d Compare November 13, 2024 03:52

aborah-sudo requested a review from danlavu November 13, 2024 03:53

ikerexxe reviewed Nov 13, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch 2 times, most recently from 84a504d to 755b6e8 Compare November 13, 2024 08:28

aborah-sudo requested a review from ikerexxe November 13, 2024 08:28

danlavu approved these changes Nov 13, 2024

View reviewed changes

aborah-sudo force-pushed the netgroup branch from 755b6e8 to b217f20 Compare November 14, 2024 04:05

Tests: Test transformation of bash-ldap-id-ldap-auth netgroup

7eb8b21

Test transformation of bash-ldap-id-ldap-auth netgroup

aborah-sudo force-pushed the netgroup branch from b217f20 to 7eb8b21 Compare November 14, 2024 04:06

Tests: Test transformation of bash-ldap-id-ldap-auth netgroup #7648

Are you sure you want to change the base?

Tests: Test transformation of bash-ldap-id-ldap-auth netgroup #7648

Conversation

aborah-sudo commented Oct 16, 2024

ikerexxe left a comment

Choose a reason for hiding this comment

danlavu left a comment

Choose a reason for hiding this comment

danlavu commented Oct 18, 2024

aborah-sudo commented Oct 18, 2024

ikerexxe left a comment

Choose a reason for hiding this comment

aborah-sudo commented Oct 28, 2024

ikerexxe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danlavu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikerexxe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danlavu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment