idp: do not update cache timeout if member is added by sumit-bose · Pull Request #8545 · SSSD/sssd

sumit-bose · 2026-03-23T18:46:29Z

If only a single member is added to a group, e.g. during an initgroups
request, do not increment the cache timeout because it is not clear if
the list of members is complete or not.

Resolve: #8330

conf: add avoid_id_lookups domain option

If this new option option is set to 'true' SSSD will try to avoid
sending lookups by ID to the backend and will switch to a lookup by name
if a cached object with a matching ID can be found. This option can
e.g. be used in cases where searches by ID are expensive on the server
side because of missing indexes or are not even possible.

:config: New option 'avoid_id_lookups' to tell the SSSD responders to
use a lookup by name instead of by id where possible

Resolves: #7668

sumit-bose · 2026-03-23T18:47:46Z

Hi,

with respect to #8330 this PR only solves the id/getent group interaction. The handling of nested groups will be part of a different PR.

bye,
Sumit

gemini-code-assist

Code Review

This pull request introduces a new configuration option, avoid_id_lookups, for SSSD domains. This option, which defaults to true for IdP providers, enables SSSD to optimize lookups by attempting to fall back to name-based searches when an ID-based lookup is performed and a cached object with a matching ID is found, thereby avoiding potentially expensive ID lookups on the backend. The changes include adding the configuration option to the domain structure, implementing the fallback logic in cache request processing, updating configuration files and documentation, and adding a new system test for IdP provider behavior. Additionally, an optimization was made in the IdP provider to prevent resetting group cache timeouts when only a single member is added. There are no review comments to address.

alexey-tikhonov · 2026-03-23T19:38:28Z

python-system-tests fails.

alexey-tikhonov · 2026-03-24T09:09:51Z

Imo, option name avoid_id_lookups is unfortunate as it relates to 'id_provider' in general.
Maybe avoid_by_id_lookups?

alexey-tikhonov · 2026-03-24T09:14:10Z

src/man/sssd.conf.5.xml

+                        </para>
+                        <para>
+                            This option can e.g. be used in cases where
+                            searches by ID are expensive on the server side


Is this examlpe - "searches by ID are expensive" - realistic?
Maybe would be better to give (vague enough) examples for "not even possible" - non-reversible id-mapping (IIUC)?

This would explain "True for IdP provider" exception.

I updated the man page entry.

alexey-tikhonov · 2026-03-24T09:41:35Z

src/confdb/confdb.c

+        goto done;
+    }
+
+


Why double new line?

alexey-tikhonov · 2026-03-24T09:45:43Z

src/confdb/confdb.c

        }
    }

+    if (strcasecmp(domain->provider, "idp") == 0) {


It feels inconsistent that CONFDB_DOMAIN_AUTO_UPG handling checks for domain->provider != NULL and this check doesn't.

Given "Domain [%s] does not specify an ID provider, disabling!\n" above perhaps both aren't needed.

I think the NULL check is not needed and there is already another use of domain->provider without a check. Do you want to get the CONFDB_DOMAIN_AUTO_UPG case fixed here?

alexey-tikhonov · 2026-03-24T09:47:53Z

src/man/sssd.conf.5.xml

                </varlistentry>

+                <varlistentry>
+                    <term>avoid_id_lookups (boolean)</term>


Is it intentional new option can't be listed in subdomain_inherit?

Hi,

currently it is automatically inherited in new_subdomain(). I did this because even if there are sub-domains which can handle lookups by-id the fallback should do no harm and if the fallback is not possible the lookup by-id will be tried anyways.

bye,
Sumit

alexey-tikhonov · 2026-03-24T09:49:31Z

"If this new option option is set ..." -- double 'option' in the commit message.

alexey-tikhonov · 2026-03-24T09:54:54Z

src/responder/common/cache_req/cache_req.c

+
+    name = ldb_msg_find_attr_as_string(result->msgs[0], SYSDB_NAME, NULL);
+    if (name != NULL) {
+        ret = cache_req_set_plugin(cr, CACHE_REQ_GROUP_BY_NAME);


Why does this hardcode CACHE_REQ_GROUP_BY_NAME?

Function can be called for "User by ID" plugin as well.

thanks, fixed

alexey-tikhonov · 2026-03-24T10:00:28Z

src/responder/common/cache_req/cache_req_search.c

+
+            if (cr->domain->avoid_id_lookups
+                    && (strcmp(cr->plugin->name, "Group by ID") == 0
+                        || strcmp(cr->plugin->name, "User by ID") == 0)) {


Better to match against cache_req_data_get_type() - cheaper and less fragile.

cache_req_data_get_type() is used in the latest version.

alexey-tikhonov · 2026-03-24T10:14:53Z

src/providers/idp/idp_id_eval.c

-    cache_timeout = dom->group_timeout;
+    /* If we just add a single member to the group we do not want to change
+     * the cache timeout. */
+    cache_timeout = user_name == NULL ? dom->group_timeout : 0;


Setting '0' explicitly expires group object (if it already exists) -- this doesn't match comment "do not want to change the cache timeout".

Not sure if this is possible though...

I handled this differently in the latest version.

alexey-tikhonov · 2026-03-24T10:17:00Z

src/providers/idp/idp_id_eval.c

+    /* If we just add a single member to the group we do not want to change
+     * the cache timeout. */
+    cache_timeout = user_name == NULL ? dom->group_timeout : 0;
    ret = sysdb_store_group(dom, fqdn, gid, attrs, cache_timeout, 0);


Btw, should this check ret != EOK?

alexey-tikhonov · 2026-03-24T10:17:52Z

Commit message: "Resolve: #8330" -> "Resolves ..."

If this new option is set to 'true' SSSD will try to avoid sending lookups by ID to the backend and will switch to a lookup by name if a cached object with a matching ID can be found. This option can e.g. be used in cases where searches by ID are expensive on the server side because of missing indexes or are not even possible. :config: New option 'avoid_by_id_lookups' to tell the SSSD responders to use a lookup by name instead of by id where possible Resolves: SSSD#7668

If 'avoid_by_id_lookups' is set to 'True' switch to a lookup by name if a user or a group is searched by ID. Resolves: SSSD#7668

If only a single member is added to a group, e.g. during an initgroups request, do not increment the cache timeout because it is not clear if the list of members is complete or not. Resolves: SSSD#8330

sumit-bose · 2026-03-30T08:53:42Z

Imo, option name avoid_id_lookups is unfortunate as it relates to 'id_provider' in general. Maybe avoid_by_id_lookups?

Hi,

I changed this in the latest version.

bye,
Sumit

sumit-bose · 2026-03-30T08:58:33Z

"If this new option option is set ..." -- double 'option' in the commit message.

fixed

sumit-bose · 2026-03-30T09:00:24Z

Commit message: "Resolve: #8330" -> "Resolves ..."

fixed

sumit-bose marked this pull request as ready for review March 23, 2026 18:46

gemini-code-assist bot reviewed Mar 23, 2026

View reviewed changes

alexey-tikhonov added the no-backport This should go to target branch only. label Mar 23, 2026

alexey-tikhonov self-assigned this Mar 23, 2026

alexey-tikhonov self-requested a review March 23, 2026 19:37

sumit-bose force-pushed the idp_group_fixes branch from dffbcd0 to 2cdb7c6 Compare March 24, 2026 08:22

alexey-tikhonov requested a review from pbrezina March 24, 2026 09:08

alexey-tikhonov reviewed Mar 24, 2026

View reviewed changes

alexey-tikhonov added the Changes requested label Mar 24, 2026

alexey-tikhonov assigned pbrezina Mar 25, 2026

sumit-bose added 3 commits March 30, 2026 09:56

cache_req: switch from ID to name lookup

05f218a

If 'avoid_by_id_lookups' is set to 'True' switch to a lookup by name if a user or a group is searched by ID. Resolves: SSSD#7668

idp: do not update cache timeout if member is added

4808cf1

If only a single member is added to a group, e.g. during an initgroups request, do not increment the cache timeout because it is not clear if the list of members is complete or not. Resolves: SSSD#8330

sumit-bose force-pushed the idp_group_fixes branch from 2cdb7c6 to 4808cf1 Compare March 30, 2026 08:51

sumit-bose removed the Changes requested label Mar 31, 2026

sumit-bose added the Waiting for review label Mar 31, 2026

Conversation

sumit-bose commented Mar 23, 2026

Uh oh!

sumit-bose commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

alexey-tikhonov commented Mar 23, 2026

Uh oh!

alexey-tikhonov commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-tikhonov commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-tikhonov commented Mar 24, 2026

Uh oh!

sumit-bose commented Mar 30, 2026

Uh oh!

sumit-bose commented Mar 30, 2026

Uh oh!

sumit-bose commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sumit-bose commented Mar 23, 2026 •

edited

Loading