On Wed, Jul 30, 2014 at 08:39:56AM -0400, Yassir Elley wrote: > > > ----- Original Message ----- > > On Thu, Jul 24, 2014 at 06:53:24AM -0400, Yassir Elley wrote: > > > Hi, > > > > > > The attached patch adds support for an "entry_cache_gpo_timeout" option, > > > which allows an admin to specify how many seconds the backend should > > > consider locally stored gpo policy files to be valid before asking the > > > backend again. This is an additional performance enhancement (along with > > > comparing gpo versions to avoid unnecessary downloads). The timeout is set > > > when a gpo entry is stored in the cache (which only happens after policy > > > files have been downloaded). > > > > > > During the processing of a gpo, if the gpo's cache timeout has not > > > elapsed, > > > the backend does not interact with the gpo_child at all (and therefore > > > doesn't download any files from the smb server). Rather, it re-uses the > > > policy files that are already stored locally. Obviously, no gpo version > > > comparisons need to be made in this case. If the timeout has elapsed (or > > > there is no gpo entry in the cache), then the backend does interact with > > > the gpo child in the usual manner (including comparing gpo versions to > > > avoid unnecessary downloads). Note that the entry_cache_gpo_timeout option > > > is *not* an expiration for the data in the gpo cache entry (which includes > > > the cached_gpt_version). Indeed, the cached_gpt_version is never > > > considered to be "expired". > > > > > > Note that this patch does not add support for offline mode, which will be > > > implemented in a subsequent patch. > > > > > > Regards, > > > Yassir. > > > > Hi, > > > > I was wondering if it's a good idea to introduce the option as a global > > one, present in sssd.conf or a sssd-ad option? See for instance > > ipa_hbac_refresh in man sssd-ipa, I think that's quite natural > > counterpart. > > > > Also, the default timeout is way too big, for access control, I think we > > should go with a lower timeout, the one used is for identity lookups > > only where we care more about speed than precision. > > > > Some more comments inline: > > > > > From 8aff4445df5cf42e09cd8b521cbd5ff4d6e1a666 Mon Sep 17 00:00:00 2001 > > > From: Yassir Elley <[email protected]> > > > Date: Tue, 22 Jul 2014 14:19:35 -0400 > > > Subject: [PATCH] AD-GPO: add entry_cache_gpo_timeout option > > > > > > --- > > > src/confdb/confdb.c | 11 ++ > > > src/confdb/confdb.h | 2 + > > > src/config/SSSDConfig/__init__.py.in | 1 + > > > src/config/SSSDConfigTest.py | 2 + > > > src/config/etc/sssd.api.conf | 1 + > > > src/db/sysdb.h | 14 +- > > > src/db/sysdb_gpo.c | 46 +++++- > > > src/man/sssd.conf.5.xml | 13 ++ > > > > [...] > > > > > @@ -1305,6 +1311,12 @@ ad_gpo_cse_step(struct tevent_req *req) > > > struct tevent_req *subreq; > > > struct ad_gpo_access_state *state; > > > int i = 0; > > > + struct ldb_result *res; > > > + errno_t ret; > > > + bool found_in_cache = false; > > > + bool send_to_child = true; > > > + int cached_gpt_version = 0; > > > + time_t policy_file_timeout = 0; > > > > > > state = tevent_req_data(req, struct ad_gpo_access_state); > > > > > > @@ -1327,14 +1339,69 @@ ad_gpo_cse_step(struct tevent_req *req) > > > DEBUG(SSSDBG_TRACE_FUNC, "smb_path: %s\n", > > > cse_filtered_gpo->smb_path); > > > DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n", > > > cse_filtered_gpo->gpo_guid); > > > > > > + /* retrieve gpo cache entry; set cached_gpt_version to -1 if > > > unavailable */ > > > + DEBUG(SSSDBG_TRACE_FUNC, "retrieving GPO from cache [%s]\n", > > > + cse_filtered_gpo->gpo_guid); > > > + ret = sysdb_gpo_get_gpo_by_guid(state, > > > + state->domain, > > > + cse_filtered_gpo->gpo_guid, > > > + &res); > > > + if (ret == EOK) { > > > + found_in_cache = true; > > > + } else { > > > + switch (ret) { > > > + case ENOENT: > > > + DEBUG(SSSDBG_TRACE_FUNC, "ENOENT\n"); > > > + cached_gpt_version = -1; > > > + break; > > > + default: > > > + DEBUG(SSSDBG_FATAL_FAILURE, "Could not read GPO from cache: > > > [%s]\n", > > > + strerror(ret)); > > > + return ret; > > > + } > > > + } > > > > I think the above is too complex, what about: > > > > if (ret == EOK) { > > /* > > * Note: if the timeout is valid, then we can later avoid > > downloading > > * the GPT.INI file, as well as any policy files (i.e. we don't > > need > > * to interact with the gpo_child at all). However, even if the > > timeout > > * is not valid, while we will have to interact with the gpo child > > to > > * download the GPT.INI file, we may still be able to avoid > > downloading > > * the policy files (if the cached_gpt_version is the same as the > > * GPT.INI version). In other words, the timeout is *not* an > > expiration > > * for the entire cache entry; the cached_gpt_version never > > expires. > > _ */ > > cached_gpt_version = ldb_msg_find_attr_as_int(res->msgs[0], > > > > SYSDB_GPO_VERSION_ATTR, > > 0); > > > > > > > > policy_file_timeout = ldb_msg_find_attr_as_uint64 > > (res->msgs[0], SYSDB_GPO_TIMEOUT_ATTR, 0); > > > > > > > > if (policy_file_timeout >= time(NULL)) { > > send_to_child = false; > > } > > } else if (ret == ENOENT) { > > DEBUG(SSSDBG_TRACE_FUNC, "ENOENT\n"); > > cached_gpt_version = -1; > > } else { > > DEBUG(SSSDBG_FATAL_FAILURE, "Could not read GPO from cache: > > [%s]\n", > > strerror(ret)); > > return ret; > > } > > > > That way you don't need the found_in_cache variable at all. > > > > > + > > > + DEBUG(SSSDBG_TRACE_FUNC, "found_in_cache: %d\n", found_in_cache); > > > + if (found_in_cache) { > > > + > > > + /* > > > + * Note: if the timeout is valid, then we can later avoid > > > downloading > > > + * the GPT.INI file, as well as any policy files (i.e. we don't > > > need > > > + * to interact with the gpo_child at all). However, even if the > > > timeout > > > + * is not valid, while we will have to interact with the gpo > > > child > > > to > > > + * download the GPT.INI file, we may still be able to avoid > > > downloading > > > + * the policy files (if the cached_gpt_version is the same as the > > > + * GPT.INI version). In other words, the timeout is *not* an > > > expiration > > > + * for the entire cache entry; the cached_gpt_version never > > > expires. > > > + */ > > > + > > > + cached_gpt_version = ldb_msg_find_attr_as_int(res->msgs[0], > > > + > > > SYSDB_GPO_VERSION_ATTR, > > > + 0); > > > + > > > + policy_file_timeout = ldb_msg_find_attr_as_uint64 > > > + (res->msgs[0], SYSDB_GPO_TIMEOUT_ATTR, 0); > > > + > > > + if (policy_file_timeout >= time(NULL)) { > > > + send_to_child = false; > > > + } > > > + } > > > + > > > + DEBUG(SSSDBG_TRACE_FUNC, "send_to_child: %d\n", send_to_child); > > > + DEBUG(SSSDBG_TRACE_FUNC, "cached_gpt_version: %d\n", > > > cached_gpt_version); > > > + > > > + cse_filtered_gpo->send_to_child = send_to_child; > > > + > > > subreq = ad_gpo_process_cse_send(state, > > > state->ev, > > > - state->domain, > > > + send_to_child, > > > + state->domain->name, > > > + cse_filtered_gpo->gpo_guid, > > > cse_filtered_gpo->smb_server, > > > cse_filtered_gpo->smb_share, > > > cse_filtered_gpo->smb_path, > > > GP_EXT_GUID_SECURITY_SUFFIX, > > > - cse_filtered_gpo->gpo_guid); > > > + cached_gpt_version); > > > > > > tevent_req_set_callback(subreq, ad_gpo_cse_done, req); > > > return EAGAIN; > > > @@ -1356,10 +1423,12 @@ ad_gpo_cse_done(struct tevent_req *subreq) > > > struct ad_gpo_access_state *state; > > > int ret; > > > int sysvol_gpt_version; > > > + const char *policy_filename = NULL; > > > char **allowed_sids; > > > int allowed_size; > > > char **denied_sids; > > > int denied_size; > > > + time_t now; > > > > > > req = tevent_req_callback_data(subreq, struct tevent_req); > > > state = tevent_req_data(req, struct ad_gpo_access_state); > > > @@ -1372,8 +1441,7 @@ ad_gpo_cse_done(struct tevent_req *subreq) > > > DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n", gpo_guid); > > > > > > ret = ad_gpo_process_cse_recv(subreq, state, &sysvol_gpt_version, > > > - &allowed_size, &allowed_sids, > > > - &denied_size, &denied_sids); > > > + &policy_filename); > > > > > > talloc_zfree(subreq); > > > > > > @@ -1383,17 +1451,35 @@ ad_gpo_cse_done(struct tevent_req *subreq) > > > goto done; > > > } > > > > > > - DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n", > > > sysvol_gpt_version); > > > + /* only store to cache (and refresh timeout) if we interacted with > > > child */ > > > + if (cse_filtered_gpo->send_to_child) { > > > > > > - ret = sysdb_gpo_store_gpo(state->domain, gpo_guid, > > > sysvol_gpt_version); > > > + DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n", > > > sysvol_gpt_version); > > > + > > > + now = time(NULL); > > > + ret = sysdb_gpo_store_gpo(state->domain, gpo_guid, > > > sysvol_gpt_version, > > > + state->domain->gpo_timeout, now); > > > + if (ret != EOK) { > > > + DEBUG(SSSDBG_OP_FAILURE, "Unable to store gpo cache entry: > > > [%d](%s}\n", > > > + ret, sss_strerror(ret)); > > > + goto done; > > > + } > > > + } > > > > What about moving the above to the ad_gpo_process_cse* request? That way > > you could always store the GPO data after the child has finished and could > > avoid branching in the caller. > > > > The rest looks good to me and the patches seem to work fine. > > I agree with all your comments and have attached a revised patch. With regard > to the timeout option, I have changed this to be an sssd-ad option. I have > changed the name to ad_gpo_cache_timeout and have modelled it after > ipa_hbac_refresh (including the 5 second default timeout). > > Regards, > Yassir.
The code now looks good to me and I've verified that if you log in twice within the ad_gpo_cache_timeout interval, the gpo_child is not called at all. ACK _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
