Skip to content

Tests: ADuser external group cache update#8046

Merged
sumit-bose merged 1 commit into
SSSD:masterfrom
shridhargadekar:ext_member
Nov 27, 2025
Merged

Tests: ADuser external group cache update#8046
sumit-bose merged 1 commit into
SSSD:masterfrom
shridhargadekar:ext_member

Conversation

@shridhargadekar

@shridhargadekar shridhargadekar commented Jul 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

In IPA-AD trust, a AD user is member of large number of IPA groups. When AD user is removed from one external-group, and cache of AD user is expired, subsequent look up of that AD user should update the user cache correctly.

verifies #7938

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 22, 2025
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a new system test to verify the correct update of an AD user's group membership in the cache after expiration. The overall approach is correct, but the implementation has several issues, including a critical typo that breaks the test, inconsistencies in the documentation, and the use of a fixed sleep which can lead to test flakiness. The provided feedback aims to correct these issues to ensure the test is robust and maintainable.

Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 22, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_ipa_trusts.py Fixed
Comment thread src/tests/system/tests/test_ipa_trusts.py Fixed
Comment thread src/tests/system/tests/test_ipa_trusts.py Fixed
Comment thread src/tests/system/tests/test_ipa_trusts.py Fixed
@shridhargadekar
shridhargadekar force-pushed the ext_member branch 3 times, most recently from 3e5ff9d to 2a6f2b2 Compare July 22, 2025 20:41
@shridhargadekar
shridhargadekar force-pushed the ext_member branch 4 times, most recently from a1cf3fd to 29ee6c3 Compare July 25, 2025 20:16
@sumit-bose

sumit-bose commented Jul 30, 2025

Copy link
Copy Markdown
Contributor

Hi,

the test is causing a failure:

 FAILED tests/test_ipa_trusts.py::test_ipa_trusts__aduser_membership_update_cache (ipa-trust-samba) - pytest_mh.conn.ssh.SSHProcessError: 
Command #23094 exited with return code 1:
  Command:
    ipa --no-prompt group-remove-member posix_group_4 --external external_group_4
  CWD:
  Env:
  Output:
      Group name: posix_group_4
      GID: 490200087
      Member groups: external_group_4
      Failed members: 
        member user: 
        member group: external_group_4: invalid 'trusted domain object': Ambiguous search, user domain was not specified
        member service: 
        member User ID override: 
    ---------------------------
    Number of members removed 0
    ---------------------------
  Error output:
FAILED tests/test_authentication.py::test_authentication__user_login_with_overriding_home_directory[first_char] (ldap) - pylibsshext.errors.LibsshChannelException: Failed to open_session: [-2]

I think the --external in the ipa group-remove-member is wrong because external_group_4 is an IPA object and is not referring to an external object. If I understand the framework code correctly you cannot use the name string directly but you have to create an IPG group object like

ipa.group("posix_group_4").remove_member(ipa.group("external_group_4"))

bye,
Sumit

@shridhargadekar
shridhargadekar force-pushed the ext_member branch 3 times, most recently from 2b197e5 to f0894cc Compare July 30, 2025 15:12
@shridhargadekar

shridhargadekar commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

updated the code, failed testcase are due to pylibssh. Should not be related to this PR

@pbrezina
pbrezina force-pushed the master branch 2 times, most recently from f5d64b3 to b854636 Compare November 4, 2025 14:27
jakub-vavra-cz
jakub-vavra-cz approved these changes Nov 21, 2025
View reviewed changes

@jakub-vavra-cz jakub-vavra-cz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sumit-bose
sumit-bose reviewed Nov 21, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_ipa_trusts.py
@shridhargadekar
shridhargadekar force-pushed the ext_member branch 2 times, most recently from 858066e to d3f7b24 Compare November 21, 2025 12:00
sumit-bose
sumit-bose reviewed Nov 24, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_ipa_trusts.py Outdated
@sumit-bose

sumit-bose commented Nov 24, 2025

Copy link
Copy Markdown
Contributor

Hi,

the Fedora 43 and 44 system test are failing in the new test (Fedora 41 is currently still running). I would suggest to reduce the number of groups from 4000 to e.g. 10. This does not change the functionality of the test but hopefully make it more stable.

For performance testing with many groups it might be better to have dedicated performance test, maybe with containers where a large amoutn of users and groups are already pre-installed?

bye,
Sumit

@shridhargadekar
shridhargadekar force-pushed the ext_member branch 2 times, most recently from 7a1c0d5 to 179e0d1 Compare November 26, 2025 07:16
@shridhargadekar

shridhargadekar commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

Hi,
As per suggestion, I've reduced the group creation from 4000 to 10.
Please check.

sumit-bose
sumit-bose approved these changes Nov 26, 2025
View reviewed changes

@sumit-bose sumit-bose left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the changes, CI is all green now, ACK.

bye,
Sumit

@shridhargadekar @sssd-bot
Tests: ADuser external group cache update
128d1b4 
AD user in external group is not cleared when expiring the cache
In IPA-AD trust, a ADuser is member of large number of IPA groups.
When ADuser is removed from one external-group, and cache of ADuser
is expired, subsequent look up should update the cache correctly.

verifies SSSD#7938

Reviewed-by: Jakub Vávra <[email protected]>
Reviewed-by: Sumit Bose <[email protected]>
@sssd-bot

sssd-bot commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

The pull request was accepted by @sumit-bose with the following PR CI status:

🟢 CodeFactor (success)
🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-41) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sumit-bose
sumit-bose merged commit ec81ea2 into SSSD:master Nov 27, 2025
16 checks passed
@sssd-bot sssd-bot mentioned this pull request Nov 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sumit-bose sumit-bose sumit-bose approved these changes
@jakub-vavra-cz jakub-vavra-cz jakub-vavra-cz approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@sumit-bose sumit-bose

@jakub-vavra-cz jakub-vavra-cz

Labels

Accepted backport-to-sssd-2-9 Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@shridhargadekar @sumit-bose @sssd-bot @jakub-vavra-cz @github-advanced-security @ikerexxe @alexey-tikhonov