URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: opened
PR body: """ This patch set has been written on top of PR#141, thus PR#141 should be merged before this one and also the following patches* are don't have to be reviewed as part of this PR.
*: - CACHE_REQ: Add cache_req_data_set_bypass_cache() - PAM: Use cache_req to perform initgroups lookups - TESTS: Adapt pam-srv-tests to deal with cache_req related changes
The other 2 patches, the important ones. are basically introducing a new configurable option to define whether the responder should query all the caches before querying the data providers and reasonable debug info for the work.
I'm not 100% sure those changes are the only changes needed (but seems so), so I'd like to ask @jhrozek and @pbrezina for a first review just in order to be sure this series is according to their ideas.
Those 2 important patches could be squashed, in case it's the preference of the reviewers. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
pbrezina commented: """ Hi, thank you for the patch.
```c @@ -469,6 +476,8 @@ static void cache_req_input_parsed(struct tevent_req *subreq) return; }
+ cache_req_setup_search_approach(state); + ret = cache_req_select_domains(req, domain); if (ret != EAGAIN) { tevent_req_error(req, ret); ``` The search approach needs to be set in `cache_req_create()`, since you don't always parse the username. I also prefer the search approach to be set as a return values instead of setting it inside function. Somethink like this:
```c static enum cache_req_search_approach cache_req_get_search_approach(bool confront_cache_first, bool plugin_bypass_cache, bool request_bypass_cache)
cr->search_approach = cache_req_get_search_approach(rctx->confront_cache, plugin->bypass_cache, data->bypass_cache) ``` If we search cache first and we have a cache hit you just return it regardless its expiration status. I also don't like this procedural style here at all such as `cache_req_evaluate_search_approach()` etc. It is quite difficult to understand what is happening there.
What I would like to see and I personally think that it is much cleaner is something like this:
1) `cache_req_search()` will get two parameters: `bool skip_cache`, `bool skip_dp` or similar in meaning. This way you can select whichever search approach you want: * (1a) `skip_cache := false`, `skip_dp := false` => current approach * (1b) `skip_cache := true`, `skip_dp := false` => bypass_cache is true * (1c) `skip_cache := false`, `skip_dp := true` => search cache first (first iteration) * (1d) `skip_cache := true`, `skip_dp := false` => now search dp (second iteration)
2) Decide whether `confront_cache_first` and `bypass_cache` is set and store it into `cr` in `cache_req_create()`
3) If `confront_cache_first` is `false`, you will use `cache_req_search_send(..., skip_cache := bypass_cache, skip_dp := false)`, i.e. (1a) or (1b).
4) If `confront_cache_first` is `true` you need to perform two iterations first with (1c) and then with (1d). With (1c) you need to return `ENOENT` when the object is expired.
5) To support two iterations over all domains I highly recommend to move the whole iteration logic into separate tevent request, e.g. `cache_req_search_domains(ev, cr, domains, domain, skip_cache, skip_dp)` where domains is list of `rctx->domains` and `domain` is domain from input if specified. This way you will gain better control over the whole logic.
I know there is some glue code required for tevent but don't be scared of those requests. It make the code quite clean and sequential when done right.
"""
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-281068933
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ @pbrezina: So, firstly sorry for the long time taken for updating this PR. I've been slapped in the face by tevent when trying to have a code reasonable clean.
So, the first patch* moves the iteration logic into a separate tevent request. The second one** adds the confront_caches_first option (and here you were right, the changes became quite simple after the first patch).
This patchset is rebased on top of #141 (which is still pending a review after the rebasing on top of @sumit-bose pacthes).
*: CACHE_REQ: Move cache_req_next_domain() into a new tevent request **: CACHE_REQ: Confront caches first """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-282475120
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
pbrezina commented: """ Hi, this version looks much better. `cache_req: cinfront caches first` has some smaller issues that will be easily fix. Let's focus on the first patch to get the tevent request good and then we willl fix the second patch. In general, this change is good. We just tent to stick to few coding styles guidelines to make it simpler to read and understand.
1. Each tevent request has to have its own state structure that share name with the request, do not reuse state of another request please. That is: ```c struct tevent_req *cache_req_search_domains_send(TALLOC_CTX *mem_ctx, ...
struct tevent_req *req; struct cache_req_state *state = NULL; errno_t ret;
req = tevent_req_create(mem_ctx, &state, struct cache_req_state); if (req == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n"); return NULL; } ``` Instead of reusing state of `cache_req_send`, create and use a new `struct cache_req_search_domains_state`. Put fields that are used by this request to this state and remove fields that are not used by `cache_req` requests from `cache_req_state`.
2. Never reuse `_recv` function of another request, even though they are the same. ```c static void cache_req_done(struct tevent_req *subreq) { struct cache_req_state *state; struct tevent_req *req; errno_t ret;
req = tevent_req_callback_data(subreq, struct tevent_req); state = tevent_req_data(req, struct cache_req_state);
ret = cache_req_recv(state, subreq, &state->results); talloc_zfree(subreq); ``` should be ```c static void cache_req_done(struct tevent_req *subreq) { struct cache_req_state *state; struct tevent_req *req; errno_t ret;
req = tevent_req_callback_data(subreq, struct tevent_req); state = tevent_req_data(req, struct cache_req_state);
ret = cache_req__search_domains_recv(state, subreq, &state->results); talloc_zfree(subreq); ```
3. Each tevent requests should be a stand-alone working block of code that can be read from the beginning of `_send` to the end of `_recv` without any jumps. I.e. whole code should be together: ```c struct request_state {}
struct tevent_req *request_send() errno_t request_step1() errno_t request_step2() ... void request_done() errno_t request_recv() ```
Please move `cache_req_search_domains` out of `cache_req` request code. The current code read like: ```c struct tevent_req *cache_req_send() static errno_t cache_req_process_input() static void cache_req_input_parsed() static errno_t cache_req_select_domains() static errno_t cache_req_search_domains() struct tevent_req *cache_req_search_domains_send() static errno_t cache_req_next_domain() static errno_t cache_req_add_result() static errno_t cache_req_create_and_add_result() static void cache_req_search_domains_done() static void cache_req_done() ``` It should read as: ```c static errno_t cache_req_search_domains_add_result() static errno_t cache_req_search_domains_create_and_add_result() struct tevent_req *cache_req_search_domains_send() static errno_t cache_req_search_domains_next() static void cache_req_search_domains_done() static void cache_req_search_domains_recv()
struct tevent_req *cache_req_send() static errno_t cache_req_process_input() static void cache_req_input_parsed() static errno_t cache_req_select_domains() static errno_t cache_req_search_domains() static void cache_req_done() ``` 4. `dp_success` parameter is not an input parameter that is required. It is a result of `cache_req_search` and used only internally here. ```c struct tevent_req *cache_req_search_domains_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct cache_req *cr, struct sss_domain_info *domain, bool check_next, bool dp_success, ```
5. I know this seems like nitpicking and sometimes even unnecessary, but tevent requests are not done right it become chaos that is hard to read, understand and work with if not done right, so please stick with me. I had troubles learning it myself. :-) """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-284717277
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ @pbrezina: Patch set update. I've tried to address all your comments.
I'm removing the "Changes request" label. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-285148038
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
pbrezina commented: """ Hi, thank you. This version looks much better. There where few logic issues in the code, I started to write the review but then I had a need to actually try to code my suggestions to see if it may work. So I'm sorry that I won't write it down and let you implement it your way this time. Please, see my patches and include them/inspire yourself in writing your own.
I fixed the logic of confront cache in a way that it now contains several shortcut on phases where we know we don't have to iterate to next domain. I also removed code duplication in function dealing with new results. And finally I made the selection of proper search schema more explicit. I did not test these patches besides our available automated tests.
Repo: https://github.com/pbrezina/sssd/commits/cache_req """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-285342833
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ Pavel,
I've taken your patches, reworked in order to have then sorted out in a way that makes sense and have pushed a new series.
Some changes have been done in order to have them working properly. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286029636
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
pbrezina commented: """ Ack. Thank you for your patience. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286064545
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
jhrozek commented: """ I'm sorry I have one more question before pushing the patches. Could we please ask some native English speaker if the "confront_caches" option sounds good to them? To me it sounds a bit strange, but I can't tell if that's my Czenglish or if we should rename the option.
btw the patches look OK to me even though I only spot-checked them. My question is really only about the option name. And if we end up changing the option, I'm fine with just changing the option name, the internal flag can remain the same. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286231134
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
justin-stephenson commented: """ I agree with that the term `confront caches` sounds a bit unusual, perhaps something like `cache_first`, `cache_direct`, or `direct_to_cache` may be more clear. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286276608
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
justin-stephenson commented: """ I agree with that the term `confront caches` sounds a bit unusual, perhaps something like `cache_first`, `cache_direct`, or `direct_to_cache` may be more clear. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286276608
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ @jhrozek: make your choice and I'll do the sed """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286277686
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
jhrozek commented: """ I vote for `cache_first` :) """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286345259
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ @jhrozek, pushed a new version.
Here's the diff in order to make your review easier:
``` diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 6e54b72..c05b1ce 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -88,7 +88,7 @@ #define CONFDB_RESPONDER_LOCAL_NEG_TIMEOUT "local_negative_timeout" #define CONFDB_RESPONDER_IDLE_TIMEOUT "responder_idle_timeout" #define CONFDB_RESPONDER_IDLE_DEFAULT_TIMEOUT 300 -#define CONFDB_RESPONDER_CONFRONT_CACHES_FIRST "confront_caches_first" +#define CONFDB_RESPONDER_CACHE_FIRST "cache_first"
/* NSS */ #define CONFDB_NSS_CONF_ENTRY "config/nss" diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index 0cb8ef3..03a1a43 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -51,7 +51,7 @@ option_strings = { 'fd_limit' : _('The number of file descriptors that may be opened by this responder'), 'client_idle_timeout' : _('Idle time before automatic disconnection of a client'), 'responder_idle_timeout' : _('Idle time before automatic shutdown of the responder'), - 'confront_caches_first': _('Always query all the caches before querying the Data Providers'), + 'cache_first': _('Always query all the caches before querying the Data Providers'),
# [sssd] 'services' : _('SSSD Services to start'), diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index b6d3a18..457a6f0 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -309,7 +309,7 @@ class SSSDConfigTestSSSDService(unittest.TestCase): 'fd_limit', 'client_idle_timeout', 'responder_idle_timeout', - 'confront_caches_first', + 'cache_first', 'description', 'certificate_verification', 'override_space', diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 78e62b3..c287328 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -59,7 +59,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# Name service option = user_attributes @@ -98,7 +98,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# Authentication service option = offline_credentials_expiration @@ -133,7 +133,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# sudo service option = sudo_timed @@ -155,7 +155,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# autofs service option = autofs_negative_timeout @@ -176,7 +176,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# ssh service option = ssh_hash_known_hosts @@ -199,7 +199,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# PAC responder option = allowed_uids @@ -221,7 +221,7 @@ option = fd_limit option = client_idle_timeout option = description option = responder_idle_timeout -option = confront_caches_first +option = cache_first
# InfoPipe responder option = allowed_uids diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index a786483..08cecf0 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -14,7 +14,7 @@ reconnection_retries = int, None, false fd_limit = int, None, false client_idle_timeout = int, None, false responder_idle_timeout = int, None, false -confront_caches_first = int, None, false +cache_first = int, None, false description = str, None, false
[sssd] diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 429d896..e179964 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -663,7 +663,7 @@ </listitem> </varlistentry> <varlistentry> - <term>confront_caches_first</term> + <term>cache_first</term> <listitem> <para> This option specifies whether the responder should diff --git a/src/responder/common/cache_req/cache_req.c b/src/responder/common/cache_req/cache_req.c index efbce5e..aca150d 100644 --- a/src/responder/common/cache_req/cache_req.c +++ b/src/responder/common/cache_req/cache_req.c @@ -118,7 +118,7 @@ cache_req_create(TALLOC_CTX *mem_ctx, return NULL; }
- cr->confront_caches_first = rctx->confront_caches_first; + cr->cache_first = rctx->cache_first; cr->bypass_cache = cr->plugin->bypass_cache || cr->data->bypass_cache;
return cr; @@ -580,7 +580,7 @@ static bool cache_req_search_schema(struct cache_req *cr, if (!first_iteration) { return false; } - } else if (cr->confront_caches_first == false) { + } else if (!cr->cache_first) { /* We will search cache and on cache-miss * contact domain provider sequentially. */ bypass_cache = false; @@ -829,8 +829,8 @@ static errno_t cache_req_search_domains(struct tevent_req *req,
CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, state->cr, "Search will %s the cache and %s the data provider\n", - bypass_cache ? "bypass" : "confront", - bypass_dp ? "bypass" : "confront"); + bypass_cache ? "bypass" : "check", + bypass_dp ? "bypass" : "check");
subreq = cache_req_search_domains_send(state, state->ev, state->cr, domain, check_next, bypass_cache, bypass_dp); diff --git a/src/responder/common/cache_req/cache_req_private.h b/src/responder/common/cache_req/cache_req_private.h index b5e2dcf..2d3c187 100644 --- a/src/responder/common/cache_req/cache_req_private.h +++ b/src/responder/common/cache_req/cache_req_private.h @@ -40,7 +40,7 @@ struct cache_req {
/* Domain related informations. */ struct sss_domain_info *domain; - bool confront_caches_first; + bool cache_first; bool bypass_cache;
/* Debug information */ diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h index a22de7d..4d1048a 100644 --- a/src/responder/common/responder.h +++ b/src/responder/common/responder.h @@ -138,7 +138,7 @@ struct resp_ctx { bool shutting_down; bool socket_activated; bool dbus_activated; - bool confront_caches_first; + bool cache_first; };
struct cli_creds; diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index 006853b..76f4360 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -1102,11 +1102,11 @@ int sss_process_init(TALLOC_CTX *mem_ctx, }
ret = confdb_get_bool(rctx->cdb, rctx->confdb_service_path, - CONFDB_RESPONDER_CONFRONT_CACHES_FIRST, - false, &rctx->confront_caches_first); + CONFDB_RESPONDER_CACHE_FIRST, + false, &rctx->cache_first); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, - "Cannot get "confront_caches_option".\n" + "Cannot get "cache_first_option".\n" "Querying the caches first before querying the " "Data Providers will not be enforced [%d]: %s.\n", ret, sss_strerror(ret));
``` """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286349367
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ I'm running our internal CI and I'll post the link when it passes. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286351873
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
jhrozek commented: """ oops, I also submitted to the internal CI, I guess more checks don't hurt. let me reschedule the centos CI though to see if there was just a fluke """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286352399
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286352432
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
fidencio commented: """ CI: http://sssd-ci.duckdns.org/logs/job/64/75/summary.html
There's a failure on f25 mock build but it's not related to this series. """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286372830
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
jhrozek commented: """ I knew there's a reason for me to send a CI as well, because mine passed :-)
http://sssd-ci.duckdns.org/logs/job/64/74/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286375262
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
jhrozek commented: """ 606015a4f71d8ee809347188667d268f73110483 8bb6680637ead03e24a38d15ec5265d11a920a1d 9286d0d4143bcb4e36b91022107e307998122bbb 7cd226414c7bcdd32f05416df64ebda3ac869bd7 828fe7528fbe6971701d16aed87ba12dd91da55f """
See the full comment at https://github.com/SSSD/sssd/pull/154#issuecomment-286395764
URL: https://github.com/SSSD/sssd/pull/154 Author: fidencio Title: #154: Confront caches first Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/154/head:pr154 git checkout pr154
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/154 Title: #154: Confront caches first
Label: +Pushed
sssd-devel@lists.fedorahosted.org