Skip to content

[autobackport: sssd-2-11] Tests: Add integration tests validating SSSD socket#8563

Open
sssd-bot wants to merge 1 commit intoSSSD:sssd-2-11from
sssd-bot:SSSD-sssd-backport-pr8481-to-sssd-2-11
Open

[autobackport: sssd-2-11] Tests: Add integration tests validating SSSD socket#8563
sssd-bot wants to merge 1 commit intoSSSD:sssd-2-11from
sssd-bot:SSSD-sssd-backport-pr8481-to-sssd-2-11

Conversation

@sssd-bot
Copy link
Copy Markdown
Contributor

This is an automatic backport of PR#8481 Tests: Add integration tests validating SSSD socket to branch sssd-2-11, created by @aborah-sudo.

Please make sure this backport is correct.

Note

The commits were cherry-picked without conflicts.

You can push changes to this pull request

git remote add sssd-bot git@github.com:sssd-bot/sssd.git
git fetch sssd-bot refs/heads/SSSD-sssd-backport-pr8481-to-sssd-2-11
git checkout SSSD-sssd-backport-pr8481-to-sssd-2-11
git push sssd-bot SSSD-sssd-backport-pr8481-to-sssd-2-11 --force

Original commits
abee6e7 - Tests: Add integration tests validating SSSD socket

Backported commits

  • d1e5f0f - Tests: Add integration tests validating SSSD socket

Original Pull Request Body

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

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>
(cherry picked from commit abee6e7)
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 test suite for SSSD socket activation, covering responder lifecycles, mixed-mode configurations, and configuration conflicts. Feedback focuses on improving test robustness by adding missing assertions for initial service inactivity across several tests, ensuring the return codes of commands are verified, and refactoring the lifecycle test to avoid duplication and better isolate socket-activation scenarios.

Comment on lines +40 to +51
if responder in ["pam", "ssh"]:
client.sssd.sssd["services"] = "nss"
else:
client.sssd.sssd["services"] = ""

client.sssd.restart(clean=True)
client.sssd.common.socket_responders([responder])

socket_unit = f"sssd-{responder}.socket"
service_unit = f"sssd-{responder}.service"

assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This section of the test has two issues:

  1. Test Duplication: The test cases for 'pam' and 'ssh' are testing a mixed-mode scenario where 'nss' runs as a traditional service. This duplicates the scenario tested in test_socket__responders__mixed_socket_and_traditional_services for the 'ssh' responder. To improve test coverage and avoid duplication, this test should be updated to a 'pure' socket-activation scenario where 'nss' is also socket-activated when testing 'pam' or 'ssh'.

  2. Missing Assertion: The test description states that the service unit should be checked for inactivity initially, but this check is missing.

The suggested code refactors the test to address both points. It sets up a pure socket-activation environment and adds the missing check for initial service inactivity. You will also need to update the assertions at the end of the test to verify that all relevant services become active after the request.

    client.sssd.sssd["services"] = ""
    client.sssd.restart(clean=True)

    responders_to_activate = [responder]
    if responder in ["pam", "ssh"]:
        responders_to_activate.append("nss")

    client.sssd.common.socket_responders(responders_to_activate)

    service_unit = f"sssd-{responder}.service"

    for resp in responders_to_activate:
        socket_unit = f"sssd-{resp}.socket"
        resp_service_unit = f"sssd-{resp}.service"
        assert client.sssd.svc.is_active(socket_unit), f"{resp} socket should be active"
        assert not client.sssd.svc.is_active(resp_service_unit), f"{resp} service should be inactive initially"

socket_unit = f"sssd-{responder}.socket"
service_unit = f"sssd-{responder}.service"

assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The test's description and steps state that it should verify that the service unit is inactive initially. This assertion is currently missing. Please add it to ensure the test fully validates the socket activation lifecycle.

Suggested change
assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
assert client.sssd.svc.is_active(socket_unit), f"{responder} socket should be active"
assert not client.sssd.svc.is_active(service_unit), f"{responder} service should be inactive initially"

socket_unit = f"sssd-{socket_responder}.socket"
socket_service = f"sssd-{socket_responder}.service"

assert client.sssd.svc.is_active(socket_unit), f"{socket_responder} socket should be active"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The test's description and steps state that it should verify that the service unit is inactive initially for the socket-activated responder. This assertion is currently missing. Please add it to ensure the test fully validates the socket activation lifecycle.

Suggested change
assert client.sssd.svc.is_active(socket_unit), f"{socket_responder} socket should be active"
assert client.sssd.svc.is_active(socket_unit), f"{socket_responder} socket should be active"
assert not client.sssd.svc.is_active(socket_service), f"{socket_responder} service should be inactive initially"

if socket_responder == "nss":
client.tools.getent.passwd(u.name)
elif socket_responder == "ssh":
client.host.conn.run(f"sss_ssh_authorizedkeys {u.name}", raise_on_error=False)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The result of this command is not checked. The test may pass even if the sss_ssh_authorizedkeys command fails. Please capture the result and assert that its return code is 0, similar to how it's done in test_socket__responders__socket_activation_lifecycle.

Suggested change
client.host.conn.run(f"sss_ssh_authorizedkeys {u.name}", raise_on_error=False)
ssh_result = client.host.conn.run(f"sss_ssh_authorizedkeys {u.name}", raise_on_error=False)
assert ssh_result.rc == 0, f"SSH authorizedkeys lookup failed for {u.name}"

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants