Skip to content

Tests: Add integration tests validating SSSD socket#8481

Merged
alexey-tikhonov merged 1 commit intoSSSD:masterfrom
aborah-sudo:socket
Mar 30, 2026
Merged

Tests: Add integration tests validating SSSD socket#8481
alexey-tikhonov merged 1 commit intoSSSD:masterfrom
aborah-sudo:socket

Conversation

@aborah-sudo
Copy link
Copy Markdown
Contributor

Add integration tests validating SSSD socket activation behavior for individual responders and mixed socket/traditional configurations.

Copy link
Copy Markdown

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

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 introduces a new suite of integration tests for SSSD's socket activation feature. The tests validate the lifecycle of individual socket-activated responders and also cover scenarios with a mix of socket-activated and traditional services. The implementation is generally good, but I've identified a high-severity issue in one of the tests where a necessary service restart is missing after configuration changes. This could make the test unreliable. My review includes a specific suggestion to fix this.

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

The code looks pretty nice however we need to separate the autofs cases to standalone ones with autofs in the test name. Autofs is not supported in image mode so we are skipping those tests based on name (see: https://gitlab.cee.redhat.com/sssd/sssd-qe/-/blob/main/test-plan/mh_jobs.yaml#L39) and we would be unable to do that here.

The tests that are not using autofs could be probably switched to topology BareLDAP that do not need nfs and kcm so they could be run on smaller footprint.

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

LGTM.

@alexey-tikhonov
Copy link
Copy Markdown
Member

/gemini review

Copy link
Copy Markdown

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

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 introduces a new set of system tests for SSSD socket activation, covering the lifecycle of socket-activated responders (NSS, PAM, SSH, and Autofs) and scenarios involving a mix of socket-activated and traditional services. The review comments point out that in the test_socket__responders__socket_activation_lifecycle_autofs and test_socket__responders__mixed_socket_and_traditional_services_autofs tests, a user user1 is created but not utilized, suggesting its removal to improve test clarity and efficiency.

@alexey-tikhonov
Copy link
Copy Markdown
Member

@danlavu, @jakub-vavra-cz, does this need @pytest.mark.importance markers?

@alexey-tikhonov
Copy link
Copy Markdown
Member

Would be great to have a 'low importance' test that would test "nss respoder is configured both in 'sssd.conf' and socket activation is enabled" (the latter should refuse to start).

@alexey-tikhonov
Copy link
Copy Markdown
Member

'system tests' fail on all platforms.

@alexey-tikhonov
Copy link
Copy Markdown
Member

@danlavu, @jakub-vavra-cz, does this need @pytest.mark.importance markers?

By default it will be "medium" importance

I would suggesting selecting one basic test as "medium" or even "high", and remaining as "low".

ack

Looks like this wasn't addressed.

@alexey-tikhonov
Copy link
Copy Markdown
Member

F44:

>       assert not client.sssd.svc.is_active(socket_service), f"{socket_responder} service should be inactive initially"
E       AssertionError: nss service should be inactive initially
E       assert not True

F45:

FAILED tests/test_socket.py::test_socket__responders__socket_activation_lifecycle[nss] (ldap) - AssertionError: nss service should be inactive initially
FAILED tests/test_socket.py::test_socket__responders__mixed_socket_and_traditional_services[nss] (ldap) - AssertionError: nss service should be inactive initially

I suspect a race condition: a random system NSS lookup triggers start of socket activation after it is enabled and before it is checked.

@alexey-tikhonov
Copy link
Copy Markdown
Member

ACK

@jakub-vavra-cz, please take a look at the updated version.

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

LGTM.

Add integration tests validating SSSD socket activation behavior
for individual responders and mixed socket/traditional configurations.

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Jakub Vávra <jvavra@redhat.com>
@sssd-bot
Copy link
Copy Markdown
Contributor

The pull request was accepted by @alexey-tikhonov with the following PR CI status:


🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🟢 ci / system (fedora-45) (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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants