Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 37 additions & 4 deletions sssd_test_framework/topology_controllers.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,21 @@ def join_domain(self, client: ClientHost, provider: IPAHost | ADHost | SambaHost
self.logger.info(f"Running realm join failed with:\n{result.stdout}\n{result.stderr}")
self.logger.info("Trying uninstall and join again.")
if isinstance(provider, (IPAHost)):
client.conn.exec(["ipa-client-install", "--uninstall", "-U"])
result = client.conn.exec(["ipa-client-install", "--uninstall", "-U"], raise_on_error=False)
if result.rc != 0:
self.logger.info(
f"Running ipa-client-install --uninstall failed with:\n{result.stdout}\n{result.stderr}"
)
self.logger.info("Trying to remove sssd.conf now")

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.

I'd state Removing sssd.conf now, unless a check for the removal of the file exists somewhere

client.fs.rm("/etc/sssd/sssd.conf")
else:
client.conn.exec(["realm", "leave", "--unattended", provider.domain], input=provider.adminpw)
result = client.conn.exec(
["realm", "leave", "--unattended", provider.domain], input=provider.adminpw, raise_on_error=False
)
if result.rc != 0:
self.logger.info(f"Running realm leave failed with:\n{result.stdout}\n{result.stderr}")
self.logger.info("Trying to remove sssd.conf now")

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.

Same here

client.fs.rm("/etc/sssd/sssd.conf")
Comment on lines 97 to +112

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error handling logic for ipa-client-install --uninstall and realm leave is duplicated. To improve maintainability and reduce redundancy, this can be refactored into a single block of code.

            if isinstance(provider, (IPAHost)):
                uninstall_cmd = ["ipa-client-install", "--uninstall", "-U"]
                uninstall_input = None
                cmd_desc = "ipa-client-install --uninstall"
            else:
                uninstall_cmd = ["realm", "leave", "--unattended", provider.domain]
                uninstall_input = provider.adminpw
                cmd_desc = "realm leave"

            result = client.conn.exec(uninstall_cmd, input=uninstall_input, raise_on_error=False)
            if result.rc != 0:
                self.logger.info(f"Running {cmd_desc} failed with:\n{result.stdout}\n{result.stderr}")
                self.logger.info("Trying to remove sssd.conf now")
                client.fs.rm("/etc/sssd/sssd.conf")

client.conn.exec(["realm", "join", provider.domain], input=provider.adminpw)


Expand Down Expand Up @@ -361,7 +373,11 @@ class GDMTopologyController(ProvisionedBackupTopologyController):
@BackupTopologyController.restore_vanilla_on_error
def topology_setup(self, client: ClientHost, ipa: IPAHost, keycloak: KeycloakHost) -> None:
short_hostname = client.conn.run("hostname").stdout.split(".")[0].strip()
hostname = f"{short_hostname}.{keycloak.domain}"
hostname = f"{short_hostname}.{ipa.domain}"
client.fs.backup("/etc/hostname")
client.fs.backup("/etc/hosts")
client.conn.run(f"echo {hostname} > /etc/hostname")
client.fs.write("/etc/hosts", client.fs.read("/etc/hosts").replace("client.test", hostname))
Comment on lines 375 to +380

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This block of code for setting the client hostname is also present in IPATopologyController. To improve maintainability and avoid code duplication, consider extracting this logic into a private helper method, e.g., _set_client_hostname(self, client: ClientHost, domain: str).


# Change client hostname to match the domain
self.logger.info(f"Changing hostname to {hostname}")
Expand All @@ -382,6 +398,16 @@ def topology_setup(self, client: ClientHost, ipa: IPAHost, keycloak: KeycloakHos

# Create an IdP client
keycloak.kclogin()

# First delete client if it exists
result = keycloak.conn.run(
"/opt/keycloak/bin/kcadm.sh get clients -q clientId=ipa_oidc_client " "--fields=id|jq -r '.[0].id'",
raise_on_error=False,
)
if result.rc == 0 and "null" not in result.stdout:
client_id = result.stdout.strip()
keycloak.conn.run(f"/opt/keycloak/bin/kcadm.sh delete clients/{client_id}")

keycloak.conn.run(
"/opt/keycloak/bin/kcadm.sh create clients -r master "
'-b \'{"clientId": "ipa_oidc_client", "clientAuthenticatorType": "client-secret", '
Expand All @@ -392,7 +418,14 @@ def topology_setup(self, client: ClientHost, ipa: IPAHost, keycloak: KeycloakHos
)

ipa.kinit()
ipa.conn.run(

# Check if IPA entry for Keycloak already exists and delete
result = ipa.conn.run("ipa idp-show keycloak", raise_on_error=False)
if "ipa_oidc_client" in result.stdout:
self.logger.info(f"IPA already enrolled in keycloak. Removing.")

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.

This doesn't need to be an f-string

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.

nitpick, two spaces before 'Removing"

ipa.conn.run("ipa idp-del keycloak")

result = ipa.conn.run(

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.

Either you check for something on result or you don't assign any value here

f"ipa idp-add keycloak --provider keycloak --base-url {keycloak.hostname}:8443/auth "
"--org master --client-id ipa_oidc_client --secret",
input="IPA_Secret123",
Expand Down
16 changes: 12 additions & 4 deletions sssd_test_framework/utils/gdm.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,19 @@ def wait_for_login(self, client: Client) -> None:
:param client: Client role object to read log
:type client: Client role
"""
result = client.journald.journalctl(
grep="Opening and taking control of.*card", unit=None, since="5 seconds ago"
)
if result.rc != 0:
result = client.journald.journalctl(grep=None, unit=None, since="5 seconds ago")

rc = 0
checks = ["Opening and taking control of.*card", "Adding device.*card"]
for check in checks:
if not re.search(check, result.stdout):
rc += 1

if rc >= len(checks):
raise AssertionError("Unable to see gnome-shell take control of video card")
Comment on lines +122 to 129

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Instead of iterating through the checks and calling re.search in a loop, it's more efficient and readable to combine the patterns into a single regular expression using |.

        checks = ["Opening and taking control of.*card", "Adding device.*card"]
        pattern = "|".join(checks)
        if not re.search(pattern, result.stdout):
            raise AssertionError("Unable to see gnome-shell take control of video card")

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.

This one makes sense

import time

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The time module is already imported at the top of this file. This import statement is redundant and violates PEP 8 style guidelines, which recommend placing all imports at the top of the file. Please remove this line.


time.sleep(2)

def check_home_screen(self) -> bool:
"""
Expand Down
Loading