Adcli: adding class and methods for adcli #145

shridhargadekar · 2025-01-09T09:22:39Z

Adding adcli class, and methods including info, discovery, join

jakub-vavra-cz · 2025-01-10T11:26:37Z

sssd_test_framework/utils/adcli.py

+    def _join(
+        self,
+        *,
+        domain: str| None = None,


There should be probably either domain or domain_controller.
The manual page states that it expects domain so I would go with that.
Getting both None or both populated is a recipe for a disaster.

pbrezina

I'd like to bring in Sumit as well.

I don't really know if we have use for this in SSSD, I don't really think so. I'm fine with including the patch though. However, in this case, operation that changes something must be also able to revert that change. In this case join, so this should be MultihostReentrantUtility.

pbrezina · 2025-01-15T12:18:10Z

sssd_test_framework/utils/adcli.py

+        self.fs: LinuxFileSystem = fs
+        """Filesystem utils."""
+
+    def _info(


Why do you use underscores on the methods name? Those should be all public methods, right?

pbrezina · 2025-01-15T12:23:08Z

sssd_test_framework/utils/adcli.py

+        self,
+        *,
+        domain: str | None = None,
+        domain_controller: str | None,


default to None as well?

pbrezina · 2025-01-15T12:24:22Z

sssd_test_framework/utils/adcli.py

+    def _join(
+        self,
+        *,
+        domain: str| None = None,
+        domain_controller: str| None = None,
+        domain_realm: str| None = None,
+        host_fqdn: str| None = None,
+        host_keytab: str| None = None,
+        computer_name: str| None = None,
+        login_ccache: str| None = None,
+        login_user: str| None = None,
+        login_type: str| None = None,
+        domain_ou: str| None = None,
+        service_name: str| None = None,
+        os_name: str| None = None,
+        os_version: str| None = None,
+        os_service_pack: str| None = None,
+        user_principal: str| None = None,
+        trusted_for_delegation: str| None = None,
+        dont_expire_password: str| None = None,
+        add_service_principal: str| None = None,
+        description: str| None = None,
+        setattrs: str| None = None,
+        no_password: str| None = None,
+        promp_password: str| None = None,
+        stdin_password: str| None = None,
+        one_time_password: str| None = None,
+        show_password: str| None = None,
+        show_details: str| None = None,
+        add_samba_data: str| None = None,
+        samba_data_tool: str| None = None,
+        ldap_passwd: str| None = None,


Use **kwargs or *args to handle additional argument and keep only required arguments here. Otherwise it would be difficult to maintain.

sumit-bose · 2025-01-15T12:57:30Z

I'd like to bring in Sumit as well.

I don't really know if we have use for this in SSSD, I don't really think so. I'm fine with including the patch though. However, in this case, operation that changes something must be also able to revert that change. In this case join, so this should be MultihostReentrantUtility.

Hi,

on which level the revert should work. E.g. before calling adcli join we can save the current keytab and restore it afterwards. But since the state on AD/Samba DC will be changed as well they keys from the old keytab won't work anymore. So it might be better to keep the new keytab or do a leave/join cycle with fixed options to get back into a defined state?

bye,
Sumit

pbrezina · 2025-01-17T08:20:39Z

I'd like to bring in Sumit as well.
I don't really know if we have use for this in SSSD, I don't really think so. I'm fine with including the patch though. However, in this case, operation that changes something must be also able to revert that change. In this case join, so this should be MultihostReentrantUtility.

Hi,

on which level the revert should work. E.g. before calling adcli join we can save the current keytab and restore it afterwards. But since the state on AD/Samba DC will be changed as well they keys from the old keytab won't work anymore. So it might be better to keep the new keytab or do a leave/join cycle with fixed options to get back into a defined state?

bye, Sumit

It's a question of how is it going to be used.

Right now, in sssd-test-framework, we have this: https://github.com/SSSD/sssd-test-framework/blob/master/sssd_test_framework/topology_controllers.py#L74

pytest_setup -> each host performs a backup (only once at start)
topology_setup -> run realmd join (only once when entering topology)
topology teardown -> restore from the host backup (only once when leaving topology)

In adcli, I assume that Shridar is going to call adcli.join() insinde a test. So the logic should be:

MultihostUtility (cannot be used in MultihostHost objects, only in MultihostRole objects inside tests):

utility.setup: do nothing or backup what is needed
inside test: adcli.join
utility.teardown: adcli.leave and clean up possible leftovers

MultihostReentrantUtility (cannot be used in MultihostHost objects and in MultihostRole objects):

utility.__enter__: enter new setup level, do nothing or backup what is needed
inside test or topology setup: adcli.join
utility.__exit__: adcli.leave and clean up possible leftovers

Docs: https://pytest-mh.readthedocs.io/en/latest/articles/extending/multihost-utilities.html

Other way would be to rely on the backup/restore mechanisms to make sure everything is reverted after each test. But in this case, it needs to be thoroughly documented so the expectation is clear. That might be enough.

So it really depends on how is it going to be used.

jakub-vavra-cz · 2025-01-17T09:21:42Z

@pbrezina, @shridhargadekar : We will probably need a way to either skip topology setup/teardown (no enrolling) so we can join ourselves or create topologies like KnownTopology.UnjoinedAD that will have the machines but not enrolled client. We can use these for example for tests like ldap sasl authid that needs a specific setup (--user-principal=host/name@REALM) to work correctly.

spoore1

I think there are still some questions to be addressed but, it looks like a good start for an interface to adcli. My only question currently is about additional join options.

spoore1 · 2025-01-29T21:41:54Z

sssd_test_framework/utils/adcli.py

+    def join(
+        self,
+        domain: str,
+    ) -> str:


Could we need to run join with additional options/arguments like computer name, host keytab, user principal or others? If we do, I think we might be able to use something like the arg list used in utils/tools.py in SSHKeyUtils.generate here:

https://github.com/SSSD/sssd-test-framework/blob/master/sssd_test_framework/utils/tools.py#L770

Does the change work? I'd think you'd need to keep the domain inside the CLIBuilderArgs to do something like:

if args is None: args = [] bargs: CLIBuilderArgs = { "domain": (self.cli.option.POSITIONAL, domain), } self.host.conn.exec(["adcli", "join"] + self.cli.args(bargs) + [*args])

I haven't tested the above but, I'm just trying to mirror what was done in the SSHKeyUtils.generate(). Also, I'm not sure of the adcli command order. Maybe args should be right after the join?

Adding adcli class, and methods including info, discovery, join

pbrezina · 2025-02-25T09:49:40Z

sssd_test_framework/utils/adcli.py

+        args: CLIBuilderArgs = {
+        }


Can be removed.

pbrezina · 2025-02-25T09:50:00Z

sssd_test_framework/utils/adcli.py

+        """
+        args: CLIBuilderArgs = {
+        }
+        self.host.conn.exec(["adcli", "join"] + self.cli.args(args))


pbrezina · 2025-02-25T09:51:26Z

sssd_test_framework/utils/adcli.py

+
+class ADCLI(MultihostUtility[MultihostHost]):
+    """
+    Call commands from adcli


Make a note that this utility does not revert changes.

Given the use case, I think it is fine to rely on reverting the AD host in the topology controller, but it needs to be documented.

pbrezina · 2025-02-25T09:52:58Z

sssd_test_framework/utils/adcli.py

+        """
+        args: CLIBuilderArgs = {
+        }
+        self.host.conn.exec(["adcli", "join"] + self.cli.args(args))


Does this call write any files? Like /etc/krb5.keytab maybe? If yes, you need to call fs.backup(file) to make sure it is restored. For AD state, we can rely on the topology controller, but we should handle any changes done to the host.

shridhargadekar force-pushed the adcli_0 branch from d6f3db9 to 1052062 Compare January 9, 2025 12:31

andreboscatto requested a review from pbrezina January 9, 2025 13:47

andreboscatto assigned pbrezina Jan 9, 2025

andreboscatto requested a review from spoore1 January 9, 2025 13:51

andreboscatto assigned spoore1 Jan 9, 2025

jakub-vavra-cz reviewed Jan 10, 2025

View reviewed changes

pbrezina requested changes Jan 15, 2025

View reviewed changes

pbrezina requested a review from sumit-bose January 15, 2025 12:27

shridhargadekar force-pushed the adcli_0 branch from 1052062 to dfdf2a7 Compare January 16, 2025 14:25

spoore1 reviewed Jan 29, 2025

View reviewed changes

Adcli: adding class and methods for adcli

8048de6

Adding adcli class, and methods including info, discovery, join

shridhargadekar force-pushed the adcli_0 branch from dfdf2a7 to 8048de6 Compare February 17, 2025 09:36

pbrezina requested changes Feb 25, 2025

View reviewed changes

pbrezina mentioned this pull request Feb 25, 2025

realm: add methods for realm utility #153

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adcli: adding class and methods for adcli #145

Adcli: adding class and methods for adcli #145

shridhargadekar commented Jan 9, 2025

jakub-vavra-cz Jan 10, 2025

pbrezina left a comment

pbrezina Jan 15, 2025

pbrezina Jan 15, 2025

shridhargadekar Jan 16, 2025

pbrezina Jan 15, 2025

shridhargadekar Jan 16, 2025

sumit-bose commented Jan 15, 2025

pbrezina commented Jan 17, 2025

jakub-vavra-cz commented Jan 17, 2025

spoore1 left a comment

spoore1 Jan 29, 2025

spoore1 Feb 18, 2025

pbrezina Feb 25, 2025

pbrezina Feb 25, 2025

pbrezina Feb 25, 2025

pbrezina Feb 25, 2025

Adcli: adding class and methods for adcli #145

Are you sure you want to change the base?

Adcli: adding class and methods for adcli #145

Conversation

shridhargadekar commented Jan 9, 2025

Choose a reason for hiding this comment

pbrezina 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

sumit-bose commented Jan 15, 2025

pbrezina commented Jan 17, 2025

jakub-vavra-cz commented Jan 17, 2025

spoore1 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