Add IPA IPA Trust Topology Controller #119

justin-stephenson · 2024-08-01T13:33:28Z

Add "IPATrustIPA" KnownTopology

For topology groups some changes are:

Remove "IPATrust"
Add "IPATrustAD" -- includes IPATrustAD and IPATrustSamba
Add "AnyIPATrust" -- includes IPATrustAD, IPATrustSamba, IPATrustIPA

Linked PRs:
SSSD/sssd-ci-containers#106
SSSD/sssd#7517

danlavu · 2024-08-06T03:34:31Z

@justin-stephenson, It looks great, but I do think we should rename the topologies to just IPATrust, ADTrust and then SambaTrust.

justin-stephenson · 2024-08-07T15:32:29Z

@justin-stephenson, It looks great, but I do think we should rename the topologies to just IPATrust, ADTrust and then SambaTrust.

That makes sense for the individual KnownTopology objects.

What about Topology groups? My thinking was to have the following:

IPATrustAD -- includes IPATrustAD and IPATrustSamba
AnyIPATrust -- includes IPATrustAD, IPATrustSamba, IPATrustIPA

This handles the case of any trust tests which may only run against IPA - AD trusts (AD and SambaAD specific), but in general we should try to write tests which are green for all trust types (using this AnyIPATrust KnownTopology group). On the contrary maybe we don't need IPATrustAD topology group, and those tests can just set ADTrust known topology and don't need to run against Samba AD.

What do you think?

danlavu · 2024-08-07T16:57:52Z

How about about,

TrustAnyAD
TrustAnyProvider

TrustAD could be generic, but it'd be confusing with ADTrust and just being the inverse.

justin-stephenson · 2024-08-08T13:33:57Z

How about about,

TrustAnyAD TrustAnyProvider

TrustAD could be generic, but it'd be confusing with ADTrust and just being the inverse.

Okay do we agree on the following? If yes i'll make this change to the PR.

KnownTopology:

IPATrust
ADTrust
SambaTrust

KnownTopologyGroup:

TrustAnyAD - ADTrust, SambaTrust
TrustAnyProvider - IPATrust, ADTrust, SambaTrust

pbrezina

Hi, it looks good, in general. See comments inside.

About COPR packages, we can leave it as is for now to simplify local testing. But it certainly should not be merged. If we would like to merge it, then proper downgrade of the packages must be done, otherwise we'd run tests against wip ipa.

sssd_test_framework/roles/ad.py

sssd_test_framework/roles/ipa.py

sssd_test_framework/roles/samba.py

pbrezina · 2024-08-14T10:52:11Z

sssd_test_framework/topology.py

@@ -118,6 +119,17 @@ def test_ldap(client: Client, ldap: LDAP):
    .. topology-mark:: KnownTopology.IPATrustSamba
    """

+    IPATrustIPA = SSSDTopologyMark(
+        name="ipa-trust-ipa",
+        topology=Topology(TopologyDomain("sssd", client=1, ipa=2)),


Second IPA should go into different domain.

If you put two ore more identical roles inside single domain, it should mean that the hosts are identical (i.e. ipa replica).

Updated, along with sssd tests mhc.yaml.

pbrezina · 2024-08-14T10:54:36Z

sssd_test_framework/topology_controllers.py

+        client.conn.exec(["dnf", "copr", "enable", "abbra/wip-ipa-trust", "-y"])
+
+        ipa.conn.exec(["dnf", "update", "freeipa-server", "sssd-client", "-y"])
+        client.conn.exec(["dnf", "update", "sssd-client", "-y"])


Both PR CI and IDM CI installs SSSD under test, this is not necessary and will probably break in non-local run.

This will be removed later when feature is available and does not need to be installed from COPR, let's leave this conversation open.

pbrezina · 2024-08-14T10:55:43Z

sssd_test_framework/topology_controllers.py

+        # Add ipa-ipa trust COPR and update packages
+        self.logger.info("Adding COPR and updating packages")
+        ipa.conn.exec(["dnf", "copr", "enable", "abbra/wip-ipa-trust", "-y"])
+        client.conn.exec(["dnf", "copr", "enable", "abbra/wip-ipa-trust", "-y"])
+
+        ipa.conn.exec(["dnf", "update", "freeipa-server", "sssd-client", "-y"])


This shouldn't be here, if needed as a temporary solution it should be moved before pytest is run. I.e. the CI workflow, or install it manually on local run.

My intent was to simplify the steps for QE.

This will be removed later when feature is available and does not need to be installed from COPR, let's leave this conversation open.

pbrezina · 2024-08-14T10:56:53Z

sssd_test_framework/topology_controllers.py

+        # F40 sssd-kcm fails to start with 'Invalid option --genconf-section=kcm:'
+        ipa.conn.exec(["systemctl", "restart", "sssd-kcm"])


Is this fixed/investigated already?

Only relevant with install from COPR.

This will be removed later when feature is available and does not need to be installed from COPR, let's leave this conversation open.

pbrezina · 2024-08-14T11:02:23Z

Please, also add feature detection.

justin-stephenson · 2024-08-14T20:02:19Z

Hi, it looks good, in general. See comments inside.

About COPR packages, we can leave it as is for now to simplify local testing. But it certainly should not be merged. If we would like to merge it, then proper downgrade of the packages must be done, otherwise we'd run tests against wip ipa.

Thank you for the review.

I should have been more clear about the current state. We are still in mid-development phase of IPA IPA trust feature. I created these PRs to involve QE early on in the development. We have some working functionality in sssd/ipa packages from COPR therefore they can start writing some basic tests. Dan + Jakub wanted to see these PRs to start building out the parts needed for IPA IPA Trust downstream testing.

I set the blocked label on this PR (I can move it to Draft status if you prefer). I will leave your comments about COPR and updating SSSD packages from COPR open, planning to remove later when we are ready.

jakub-vavra-cz

For the trust do both ipa servers need to be changed or only the one trusting?
If so:

Would it be possible to switch the code around so ipa2 is trusting ipa1?
That way we can keep ipa1 on the production version for other tests.

As for copr/packages, the code should be in ansible playbook in sssd-ci-containers
and enabled by a switch that is by default off. We do not want anything messing around with tested artifacts.

justin-stephenson · 2024-08-15T19:52:44Z

For the trust do both ipa servers need to be changed or only the one trusting? If so:
* Would it be possible to switch the code around so ipa2 is trusting ipa1?

* That way we can keep ipa1 on the production version for other tests.

Currently it is possible to upgrade just the 1 IPA server but with upcoming changes on FreeIPA side in the near future it will be required to upgrade both IPA masters (ipa.test and ipa2.test) to COPR for testing.

As for copr/packages, the code should be in ansible playbook in sssd-ci-containers and enabled by a switch that is by default off. We do not want anything messing around with tested artifacts.

Okay I can move this code to SSSD/sssd-ci-containers#106 but I'm not sure how to handle downgrading SSSD packages from COPR after IPA IPA trust tests are run (initially I thought this would be handled by IPA topology controller teardown).

pbrezina · 2024-08-16T10:30:00Z

For the trust do both ipa servers need to be changed or only the one trusting? If so:
* Would it be possible to switch the code around so ipa2 is trusting ipa1?

* That way we can keep ipa1 on the production version for other tests.
Currently it is possible to upgrade just the 1 IPA server but with upcoming changes on FreeIPA side in the near future it will be required to upgrade both IPA masters (ipa.test and ipa2.test) to COPR for testing.

As for copr/packages, the code should be in ansible playbook in sssd-ci-containers and enabled by a switch that is by default off. We do not want anything messing around with tested artifacts.

Okay I can move this code to SSSD/sssd-ci-containers#106 but I'm not sure how to handle downgrading SSSD packages from COPR after IPA IPA trust tests are run (initially I thought this would be handled by IPA topology controller teardown).

Thank you for explaining your plan. There's no need to handle this if we don't intent to merge it in this state. I think we can do two things here:

instruct QE to run with --mh-topology=ipa-trust-ipa, this will only run desired tests
or, if you want to run all tests, move the upgrade to IPAHost.pytest_setup() and ClientHost.pytest_setup(), this way it will be updated for all tests and we can also start testing for potential regressions

justin-stephenson · 2024-08-16T17:30:50Z

Thank you for explaining your plan. There's no need to handle this if we don't intent to merge it in this state. I think we can do two things here:
* instruct QE to run with `--mh-topology=ipa-trust-ipa`, this will only run desired tests

* or, if you want to run all tests, move the upgrade to `IPAHost.pytest_setup()` and `ClientHost.pytest_setup()`, this way it will be updated for all tests and we can also start testing for potential regressions

@jakub-vavra-cz are you okay with either of these options?

jakub-vavra-cz · 2024-08-19T07:20:51Z

@justin-stephenson:
I am okay with the either options.
I want to see a "DELETE ME BEFORE MERGING" comment for the copr repo so we do not forget do delete it.

We could use some detection for the ipa2ipa trust capability both on client and on the ipa server.
(https://github.com/SSSD/sssd-test-framework/blob/master/sssd_test_framework/hosts/client.py#L40,
https://github.com/SSSD/sssd-test-framework/blob/master/sssd_test_framework/hosts/ipa.py#L71)

That way we can prepare and merge tests before it is available and run them only where it makes a sense.

Add "IPATrustIPA" KnownTopology For topology groups some changes are: - Remove "IPATrust" - Add "IPATrustAD" -- includes IPATrustAD and IPATrustSamba - Add "AnyIPATrust" -- includes IPATrustAD, IPATrustSamba, IPATrustIPA

justin-stephenson · 2024-09-20T15:45:27Z

Rebased, needed some minor changes due to pytest-mh changes.

This was referenced Aug 1, 2024

Tests: Add support for IPA IPA Trust SSSD/sssd#7517

Open

Support IPA IPA Trust with additional IPA server SSSD/sssd-ci-containers#106

Open

justin-stephenson marked this pull request as ready for review August 1, 2024 13:43

andreboscatto assigned danlavu Aug 6, 2024

andreboscatto requested review from danlavu, pbrezina and jakub-vavra-cz August 6, 2024 12:36

andreboscatto assigned pbrezina and jakub-vavra-cz Aug 6, 2024

justin-stephenson force-pushed the ipa_ipa_trust_topology_controller branch from 50d1412 to 24b3db4 Compare August 7, 2024 19:11

justin-stephenson force-pushed the ipa_ipa_trust_topology_controller branch from 24b3db4 to f57543c Compare August 8, 2024 13:52

pbrezina requested changes Aug 14, 2024

View reviewed changes

justin-stephenson added the blocked label Aug 14, 2024

justin-stephenson force-pushed the ipa_ipa_trust_topology_controller branch from f57543c to be33278 Compare August 14, 2024 20:03

jakub-vavra-cz reviewed Aug 15, 2024

View reviewed changes

justin-stephenson force-pushed the ipa_ipa_trust_topology_controller branch from be33278 to 132cd8b Compare August 15, 2024 19:52

justin-stephenson mentioned this pull request Sep 16, 2024

multihost: add MultihostBackupHost and BackupTopologyController next-actions/pytest-mh#79

Merged

Add IPA IPA Trust Topology Controller

f12cece

Add "IPATrustIPA" KnownTopology For topology groups some changes are: - Remove "IPATrust" - Add "IPATrustAD" -- includes IPATrustAD and IPATrustSamba - Add "AnyIPATrust" -- includes IPATrustAD, IPATrustSamba, IPATrustIPA

justin-stephenson force-pushed the ipa_ipa_trust_topology_controller branch from 132cd8b to f12cece Compare September 20, 2024 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPA IPA Trust Topology Controller #119

Add IPA IPA Trust Topology Controller #119

justin-stephenson commented Aug 1, 2024 •

edited

Loading

danlavu commented Aug 6, 2024

justin-stephenson commented Aug 7, 2024

danlavu commented Aug 7, 2024

justin-stephenson commented Aug 8, 2024

pbrezina left a comment •

edited

Loading

pbrezina Aug 14, 2024

justin-stephenson Aug 14, 2024

pbrezina Aug 14, 2024

justin-stephenson Aug 14, 2024

pbrezina Aug 14, 2024

justin-stephenson Aug 14, 2024

pbrezina Aug 14, 2024

justin-stephenson Aug 14, 2024

pbrezina commented Aug 14, 2024

justin-stephenson commented Aug 14, 2024

jakub-vavra-cz left a comment

justin-stephenson commented Aug 15, 2024

pbrezina commented Aug 16, 2024 •

edited

Loading

justin-stephenson commented Aug 16, 2024

jakub-vavra-cz commented Aug 19, 2024

justin-stephenson commented Sep 20, 2024

		# F40 sssd-kcm fails to start with 'Invalid option --genconf-section=kcm:'
		ipa.conn.exec(["systemctl", "restart", "sssd-kcm"])

Add IPA IPA Trust Topology Controller #119

Are you sure you want to change the base?

Add IPA IPA Trust Topology Controller #119

Conversation

justin-stephenson commented Aug 1, 2024 • edited Loading

danlavu commented Aug 6, 2024

justin-stephenson commented Aug 7, 2024

danlavu commented Aug 7, 2024

justin-stephenson commented Aug 8, 2024

pbrezina left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbrezina commented Aug 14, 2024

justin-stephenson commented Aug 14, 2024

jakub-vavra-cz left a comment

Choose a reason for hiding this comment

justin-stephenson commented Aug 15, 2024

pbrezina commented Aug 16, 2024 • edited Loading

justin-stephenson commented Aug 16, 2024

jakub-vavra-cz commented Aug 19, 2024

justin-stephenson commented Sep 20, 2024

justin-stephenson commented Aug 1, 2024 •

edited

Loading

pbrezina left a comment •

edited

Loading

pbrezina commented Aug 16, 2024 •

edited

Loading