Port providers to New Failover WIP by justin-stephenson · Pull Request #8532 · SSSD/sssd

justin-stephenson · 2026-03-17T19:28:01Z

No description provided.

(cherry picked from commit 0f5f3b6)

so it can be directly modified

So it can be modified later.

This crafts and implements the new failover interface, it does not provide complete implementation of the failover mechanism yet. It brings the code to a state were the public and private interfaces are stable, working and testable so the following tasks can be split and work on in parallel. What is missing at this state: - server configuration and discovery (failover_server_group/batch/vtable_op) - server selection mechanism (sss_failover_vtable_op_server_next) - kerberos authentication - sharing servers between IPA/AD LDAP and KDC - online/offline callbacks (resolve callback should not be needed) But especially it is possible to start refactoring SSSD code to start using the new failover implementation.

authentication

gemini-code-assist

Code Review

This pull request introduces a new failover mechanism for SSSD providers, exemplified by a new 'minimal' provider. The changes involve significant refactoring of error handling in existing LDAP and AD providers, removing dp_error parameters and related macros, and integrating a new sss_failover_transaction for connection management. The new failover logic is also integrated into various LDAP and IPA components for tasks like account info, enumeration, and sudo rules. However, several critical issues were identified: test hooks that intentionally trigger server failures are present in production code paths (ldap_id.c, minimal_id_services.c) and should be removed or guarded. Additionally, key failover options, DNS discovery domains, and server URIs are hardcoded (failover.c, failover_group.c, failover_srv.c, ldap_init.c), limiting configurability and dynamic behavior, which is unsuitable for a production-ready feature.

src/providers/ldap/ldap_id.c

src/providers/minimal/minimal_id_services.c

src/providers/failover/failover.c

src/providers/failover/failover_group.c

src/providers/failover/failover_srv.c

src/providers/ldap/ldap_init.c

src/providers/failover/failover_group.c

+
+    count = talloc_array_length(fctx->groups);
+
+    for (slot = 0; fctx->groups[slot] != NULL && slot < count; slot++) {


src/providers/ldap/ldap_id.c

    }

-    if (state->sdap_ret == ENOENT && state->noexist_delete == true) {
+    if (ret == ENOENT && state->noexist_delete == true) {


src/providers/minimal/minimal_id.c

src/providers/minimal/minimal_ldap_auth.c

src/providers/ldap/sdap_async_initgroups.c

+                                                         /* FIXME: Placeholder to be updated when
+                                                          * AD is moved to new Failover
+                                                          * Set to state->conn */


src/providers/minimal/minimal_id.c

src/providers/minimal/minimal_id_services.c

Assisted by: Claude code (Sonnet 4.6)

The data provider handler methods now return a single output argument. Remove 'dp_error/dp_err' and 'error_message' usage across provider code. The getAccountDomain method still needs to return 'domain_name' string. Assisted by: Claude code (Sonnet 4.6)

- Servers are hardcoded

This will be done differently inside the failover code

To allow system tests to run in upstream PRCI

** Temporary commit as each provider is ported to new failover ** These provider tests will often fail because they have not yet been ported to the new failover code, and they call into ldap functions which are utilizing the new failover. For example crash in : #0 0x00007fdb8d72cb41 in sss_failover_transaction_ex_send () from /usr/lib64/sssd/libsss_ldap_common.so #1 0x00007fdb8d72ccbd in sss_failover_transaction_send () from /usr/lib64/sssd/libsss_ldap_common.so #2 0x00007fdb8d6e367b in groups_by_user_send () from /usr/lib64/sssd/libsss_ldap_common.so #3 0x00007fdb8d6e6a88 in sdap_handle_acct_req_send () from /usr/lib64/sssd/libsss_ldap_common.so #4 0x00007fdb8d799076 in ipa_id_get_account_info_get_original_step () from /usr/lib64/sssd/libsss_ipa.so #5 0x00007fdb8d7a038b in ipa_id_get_account_info_send () from /usr/lib64/sssd/libsss_ipa.so #6 0x00007fdb8d7a2560 in ipa_account_info_send () from /usr/lib64/sssd/libsss_ipa.so #7 0x00007fdb8d7a2877 in ipa_account_info_handler_send () from /usr/lib64/sssd/libsss_ipa.so

* test_failover tests are expected to fail due to failover interface changes. * test_logging 'offline' tests and test_autofs__propagate_offline_status_for_multiple_domains fail because they are asserting offline related log messages which are not in the new failover code. tests/test_autofs.py::test_autofs__propagate_offline_status_for_multiple_domains (ldap) FAILED [ 35%] tests/test_failover.py::test_failover__reactivation_timeout_is_honored[None-31] (ldap) FAILED [ 39%] tests/test_failover.py::test_failover__reactivation_timeout_is_honored[15-31] (ldap) FAILED [ 39%] tests/test_failover.py::test_failover__reactivation_timeout_is_honored[60-60] (ldap) FAILED [ 39%] tests/test_failover.py::test_failover__connect_using_ipv4_second_family (ldap) FAILED [ 40%] tests/test_logging.py::test_logging__default_settings_logs_offline_errors (ldap) FAILED [ 60%] tests/test_logging.py::test_logging__default_settings_logs_to_syslog_when_ldap_is_offline (ldap) FAILED [ 60%]

pbrezina · 2026-03-21T12:40:13Z

I pushed the failover feature branch, can you please rebase and use it as the base branch?

https://github.com/SSSD/sssd/tree/failover

pbrezina

Hi Justin, it's a lot of code and you didn't want a full review yet so I just scrolled over it.

It looks good. I have some concerns about the first two patches that converts the interface to a single return code - it looks like some important logic was removed as well at few places.

And the sdap_id_op code is responsible of terminating the retry logic and bringing SSSD offline. I think it should, at some point, return ERR_NO_MORE_SERVER. And the LDAP search code should return ERR_SERVER_FAILURE so it can be picked up by sss_failover_transaction.

pbrezina · 2026-03-21T12:45:57Z

src/providers/ldap/sdap_access.c

-    ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
+    ret = sdap_id_op_done(state->sdap_op, ret);
    if (ret != EOK) {
-        if (dp_error == DP_ERR_OK) {


This looks like some retry logic was accidentally removed?

Yes, and here the call to sdap_id_op_done() is removed entirely in a later commit in this PR.

To fix this should I create a function that performs the same logic as sdap_id_op_done() but returns the ERR_* codes such as ERR_NO_MORE_SERVER and ERR_SERVER_FAILURE instead?

pbrezina · 2026-03-21T12:46:53Z

src/providers/ldap/sdap_id_op.c

-        dp_err = DP_ERR_FATAL;
-    }
-
-    if (dp_err == DP_ERR_OK && retval != EOK) {


Logic removed?

pbrezina · 2026-03-21T12:49:38Z

src/providers/ldap/ldap_init.c

-                  sdap_account_info_handler_send, sdap_account_info_handler_recv, id_ctx,
-                  struct sdap_id_ctx, struct dp_id_data, struct dp_reply_std);
+                  sdap_account_info_handler_send, sdap_account_info_handler_recv, init_ctx,
+                  struct ldap_init_ctx, struct dp_id_data, struct dp_reply_std);


minimal_init_ctx was created to do minimal changes required for the minimal provider, without completely refactoring it. Here, you should refactor the id_ctx to hold the necessary data (failover contexts) and remove old failover bits.

pbrezina · 2026-03-21T13:57:01Z

The end goal is to get rid of sdap_id_op code completely. Maybe you could remove the file and avoid building ipa and ad providers for now to make sure we do not depend on anything from sdap_id_op?

justin-stephenson · 2026-03-23T18:16:10Z

Hi Justin, it's a lot of code and you didn't want a full review yet so I just scrolled over it.

Thank you.

It looks good. I have some concerns about the first two patches that converts the interface to a single return code - it looks like some important logic was removed as well at few places.

Functions below contain logic checking the values of dp_error or sdap_ret, I could use some suggestion on how to retain this logic alongside the changes in this PR.

 sdap_ad_resolve_sids_done()
 ad_gpo_target_dn_retrieval_done()
 groups_by_user_done()
 get_user_and_group_users_done()

pbrezina and others added 9 commits March 17, 2026 13:45

utils: add TEVENT_REQ_ERROR_TO_ERRNO

02e720c

minimal: Initial implementation of minimal provider

1d90deb

(cherry picked from commit 0f5f3b6)

minimal: copy paste services code

34b8bea

so it can be directly modified

minimal: push init_ctx instead of id_ctx so we can easily pass new fctx

092182b

minimal: add authentication provider

c143fa4

minimal: copy paste authentication code

6ce9c28

So it can be modified later.

minimal: switch to new failover for service lookup and user

70a2014

authentication

minimal: update how to use it notes

6bd83a1

gemini-code-assist bot reviewed Mar 17, 2026

View reviewed changes

justin-stephenson force-pushed the failover_justin_v1_1 branch from e345d10 to 90d41a1 Compare March 17, 2026 19:35

github-advanced-security bot found potential problems Mar 17, 2026

View reviewed changes

justin-stephenson force-pushed the failover_justin_v1_1 branch 8 times, most recently from d56510c to 6d953d4 Compare March 18, 2026 13:27

justin-stephenson added 4 commits March 18, 2026 09:53

failover: minor header fixes

be3d67d

dp: Remove sdap_ret and sdap_error

8e1ce45

Assisted by: Claude code (Sonnet 4.6)

Update LDAP provider initialization for new failover

61f751a

- Servers are hardcoded

justin-stephenson force-pushed the failover_justin_v1_1 branch 4 times, most recently from 8540239 to 410fced Compare March 19, 2026 15:04

justin-stephenson added 2 commits March 19, 2026 11:24

Port LDAP provider code to new failover

6b24f5d

ldap: Dont set DPM_CHECK_ONLINE handler

1fab371

This will be done differently inside the failover code

minimal: Disable building minimal provider

c5bdfde

To allow system tests to run in upstream PRCI

justin-stephenson force-pushed the failover_justin_v1_1 branch from 410fced to 517f6dd Compare March 19, 2026 15:27

justin-stephenson force-pushed the failover_justin_v1_1 branch from 517f6dd to 62796d5 Compare March 19, 2026 15:28

justin-stephenson changed the title ~~Test only WIP~~ Port providers to New Failover WIP Mar 19, 2026

pbrezina reviewed Mar 21, 2026

View reviewed changes


		count = talloc_array_length(fctx->groups);

		for (slot = 0; fctx->groups[slot] != NULL && slot < count; slot++) {

Conversation

justin-stephenson commented Mar 17, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

Uh oh!

Check warning

Uh oh!

Uh oh!

Check notice

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pbrezina commented Mar 21, 2026

Uh oh!

pbrezina left a comment

Choose a reason for hiding this comment

Uh oh!

pbrezina Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

justin-stephenson Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

pbrezina Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

pbrezina Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

pbrezina commented Mar 21, 2026

Uh oh!

justin-stephenson commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants