URL: https://github.com/SSSD/sssd/pull/5485 Author: pbrezina Title: #5485: sudo: do not search by low usn value to improve performance Action: opened
PR body: """ This is a follow up on these two commits.
- 819d70ef6e6fa0e736ebd60a7f8a26f672927d57 - 6815844daa7701c76e31addbbdff74656cd30bea
The first one improved the search filter little bit to achieve better performance, however it also changed the behavior: we started to search for `usn >= 1` in the filter if no usn number was known.
This caused issues on OpenLDAP server which was fixed by the second patch. However, the fix was wrong and searching by this meaningfully low number can cause performance issues depending on how the filter is optimized and evaluated on the server.
Now we omit the usn attribute from the filter if there is no meaningful value.
How to test: 1. Setup LDAP with no sudo rules defined 2. Make sure that the LDAP server does not support USN or use the following diff to enforce modifyTimestamp (last USN is always available from rootDSE) ```diff diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 32c0144b9..c853e4dc1 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -1391,7 +1391,7 @@ int sdap_get_server_opts_from_rootdse(TALLOC_CTX *memctx, last_usn_name = opts->gen_map[SDAP_AT_LAST_USN].name; entry_usn_name = opts->gen_map[SDAP_AT_ENTRY_USN].name; if (rootdse) { - if (last_usn_name) { + if (false) { ret = sysdb_attrs_get_string(rootdse, last_usn_name, &last_usn_value); if (ret != EOK) { @@ -1500,7 +1500,7 @@ int sdap_get_server_opts_from_rootdse(TALLOC_CTX *memctx, } }
- if (!last_usn_name) { + if (true) { DEBUG(SSSDBG_FUNC_DATA, "No known USN scheme is supported by this server!\n"); if (!entry_usn_name) { ``` 3. Run SSSD with sudo and check that smart refresh filter does not contain modifyTimestamp 4. Add new sudo rule, check that the filter does contain it after the rules is cached
Resolves: https://github.com/SSSD/sssd/issues/5483 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5485/head:pr5485 git checkout pr5485
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: +Bugzilla
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
alexey-tikhonov commented: """ Overall looks good to me.
For some reason covscan fails to build: https://cov01.lab.eng.brq.redhat.com/covscanhub/task/202033/log/stdout.log (sorry for internal links)
Most probably issue isn't related to the patch, as "vanilla" 2.4.1/2.4.2 fails to build the same way (https://cov01.lab.eng.brq.redhat.com/covscanhub/task/202034/log/stdout.log), but this blocks covscan as a part of review process.
"""
See the full comment at https://github.com/SSSD/sssd/pull/5485#issuecomment-776101448
URL: https://github.com/SSSD/sssd/pull/5485 Author: pbrezina Title: #5485: sudo: do not search by low usn value to improve performance Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5485/head:pr5485 git checkout pr5485
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
pbrezina commented: """ ```diff diff --git a/src/providers/ldap/sdap_sudo_refresh.c b/src/providers/ldap/sdap_sudo_refresh.c index 3441dd8fd..83f944ccf 100644 --- a/src/providers/ldap/sdap_sudo_refresh.c +++ b/src/providers/ldap/sdap_sudo_refresh.c @@ -183,7 +183,8 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx, /* Download all rules from LDAP that are newer than usn */ if (srv_opts == NULL || srv_opts->max_sudo_value == NULL || strcmp(srv_opts->max_sudo_value, "0") == 0) { - DEBUG(SSSDBG_TRACE_FUNC, "USN value is unknown, assuming zero.\n"); + DEBUG(SSSDBG_TRACE_FUNC, "USN value is unknown, assuming zero and " + "omitting it from the filter.\n"); usn = "0"; search_filter = talloc_asprintf(state, "(%s=%s)", map[SDAP_AT_SUDO_OC].name, ``` """
See the full comment at https://github.com/SSSD/sssd/pull/5485#issuecomment-776636412
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
alexey-tikhonov commented: """ Thank you. Covscan is clean. ACK. """
See the full comment at https://github.com/SSSD/sssd/pull/5485#issuecomment-776650874
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
pbrezina commented: """ Pushed PR: https://github.com/SSSD/sssd/pull/5485
* `master` * b100efbfabd96dcfb2825777b75b9a9dfaacb937 - sudo: do not search by low usn value to improve performance
"""
See the full comment at https://github.com/SSSD/sssd/pull/5485#issuecomment-777365728
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/5485 Author: pbrezina Title: #5485: sudo: do not search by low usn value to improve performance Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5485/head:pr5485 git checkout pr5485
URL: https://github.com/SSSD/sssd/pull/5485 Title: #5485: sudo: do not search by low usn value to improve performance
alexey-tikhonov commented: """ RHBZs: https://bugzilla.redhat.com/show_bug.cgi?id=1926454 https://bugzilla.redhat.com/show_bug.cgi?id=1922244 """
See the full comment at https://github.com/SSSD/sssd/pull/5485#issuecomment-778190494
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
pbrezina