Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CACHE: SSSD doesn't clear cache entries #716

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@thalman
Copy link
Contributor

commented Dec 17, 2018

Once object is in cache it is refreshed when it is expired and
requested by the system. Object ID is not checked before refresh,
but config parameter ldap_(min|max)_id could be changed by admin.
We should check object ID and not refresh objects outside min/max
ID interval.

Resolves:
https://pagure.io/SSSD/sssd/issue/3905

@thalman thalman force-pushed the thalman:min_id_cache branch from 7be7277 to 8bfdebb Dec 18, 2018

id = ldb_msg_find_attr_as_uint(result->msgs[0], SYSDB_UIDNUM, 0);
} else if (strcasecmp(object_class, SYSDB_GROUP_CLASS) == 0) {
id = ldb_msg_find_attr_as_uint(result->msgs[0], SYSDB_GIDNUM, 0);
}

This comment has been minimized.

Copy link
@jhrozek

jhrozek Dec 18, 2018

Contributor

Looking at sdap_save_user() we check whether both UID and GID are within the domain limits. Therefore wouldn't it be more consistent to always check the ID if present (then, UID won't be present for groups and the check will be skipped..)

You can also use the OUT_OF_ID_RANGE helper macro.

This comment has been minimized.

Copy link
@thalman

thalman Jan 3, 2019

Author Contributor

Good idea, thanks. PR updated to be consistent with sdap_user_save ().

CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
"Object found, but needs to be refreshed.\n");
bypass_dp = false;
if (cache_req_should_be_in_cache (cr, state->result)) {

This comment has been minimized.

Copy link
@jhrozek

jhrozek Dec 18, 2018

Contributor

I would personally suggest to add this check to cache_req_search_cache directly and if the check "matches" (so, if the entry is supposed to be filtrered out), I would just pass ERR_ID_OUTSIDE_RANGE to the big switch in cache_req_search_cache.

And another potential idea: some cache_req searches might return more than one entry. The extreme case is enumeration which returns all. Should we in that case filter out only a subset those entries that match the ID constrains? If yes, then maybe it would be even better to have two or three common "checks" and assign them to each cache_req plugin as a new cache_req plugin property.

This comment has been minimized.

Copy link
@thalman

thalman Jan 3, 2019

Author Contributor

I would personally suggest to add this check to cache_req_search_cache directly and if the check "matches" (so, if the entry is supposed to be filtrered out), I would just pass ERR_ID_OUTSIDE_RANGE to the big switch in cache_req_search_cache.

If we move this check into cache_req_search_cache it will be performed on every request.
This way it is performed only on expired items => less often. Returning ERR_ID_OUTSIDE_RANGE is good idea, PR updated.

And another potential idea: some cache_req searches might return more than one entry. The extreme case is enumeration which returns all. Should we in that case filter out only a subset those entries that match the ID constrains? If yes, then maybe it would be even better to have two or three common "checks" and assign them to each cache_req plugin as a new cache_req plugin property.

Few lines above we call cache_req_expiration_status which should add expiration timestamp. This function also assumes that there is only one (or none) message in the response. I guess it is OK and it is handled somewhere else. But I'm not sure about this.

@pbrezina what do you think?

This comment has been minimized.

Copy link
@pbrezina

pbrezina Jan 4, 2019

Member

Shouldn't we return ERR_ID_OUTSIDE_RANGE for both expired and not expired objects?

This comment has been minimized.

Copy link
@thalman

thalman Jan 7, 2019

Author Contributor

Shouldn't we return ERR_ID_OUTSIDE_RANGE for both expired and not expired objects?

Yes, we can.

    • we will react on config change immediately.
    • we will be checking always all requested objects

Just what is more important. I thing that looking inside every object is not so expensive but have no experience with sssd in large scale deployment.
@pbrezina, @jhrozek - would it be fine to check all the time? In the bug customer expects that object will expire after its TTL so it will be fine for him like this.

@jhrozek
Copy link
Contributor

left a comment

The comments are something to discuss and I would welcome @pbrezina's opinion. But even more importantly the tests don't pass:

[  FAILED  ] test_nss_getgrgid_ex_no_members

It looks to me like the test needs to be adjusted (IOW, the test was taking advantage of this bug :-)) but nonetheless, the tests must pass.

@thalman thalman force-pushed the thalman:min_id_cache branch 2 times, most recently from 986220b to f031c7d Jan 2, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

I think this commit is mostly good. I'll just leave some very minor nitpics inline using the github review tool.

@@ -169,6 +170,52 @@ static errno_t cache_req_search_ncache_filter(TALLOC_CTX *mem_ctx,
return ret;
}

static int
cache_req_should_be_in_cache (struct cache_req *cr,

This comment has been minimized.

Copy link
@jhrozek

jhrozek Jan 8, 2019

Contributor

We don't put space after function name and before the opening ( in sssd

cache_req_should_be_in_cache (struct cache_req *cr,
struct ldb_result *result)
{
unsigned int id = 0;

This comment has been minimized.

Copy link
@jhrozek

jhrozek Jan 8, 2019

Contributor

It is better to use id_t

{
unsigned int id = 0;
const char *object_class;

This comment has been minimized.

Copy link
@jhrozek

jhrozek Jan 8, 2019

Contributor

Extra blank line


object_class = ldb_msg_find_attr_as_string(result->msgs[0],
SYSDB_OBJECTCATEGORY, NULL);
if (! object_class) {

This comment has been minimized.

Copy link
@jhrozek

jhrozek Jan 8, 2019

Contributor

Extra space after !

This comment has been minimized.

Copy link
@jhrozek

jhrozek Jan 8, 2019

Contributor

FWIW I personally prefer to compare against NULL directly..

@jhrozek
Copy link
Contributor

left a comment

Some minor stuff, mostly formatting needs to be fixed, otherwise LGTM.

@thalman thalman force-pushed the thalman:min_id_cache branch from f031c7d to 60237f7 Jan 8, 2019

@thalman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@jhrozek I fixed all minor comments. There is still question whether we move the check as you suggested.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Wait, I'm not sure how should the check be moved now? Currently based on both my reading of the code and a quick test, the check is executed always, even if an entry is not expired. I think this is good.

About the scalability concerns, all the patch does is one ldb_msg_find_attr_as_string and two ldb_msg_find_attr_as_uint. ldb_msg_find_whatever is basically a for loop that does a strcmp until it finds the requested attribute, so I don't think this is too bad.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

btw one thing that can be changed is to simplify the code to throw away the objectclass check and just have:

95         id = ldb_msg_find_attr_as_uint(result->msgs[0], SYSDB_UIDNUM, 0);
196         if (id && OUT_OF_ID_RANGE(id, cr->domain->id_min, cr->domain->id_max)) { 
197             return ERR_ID_OUTSIDE_RANGE;
198         }
199 
200         id = ldb_msg_find_attr_as_uint(result->msgs[0], SYSDB_GIDNUM, 0);
201         if (id && OUT_OF_ID_RANGE(id, cr->domain->id_min, cr->domain->id_max)) { 
202             return ERR_ID_OUTSIDE_RANGE;
203         }

For both users and groups. Because for a group object, the SYSDB_UIDNUM check would return the default value of 0 which then skips the check. But it's up to you which way you find more readable.

CACHE: SSSD doesn't clear cache entries
Once object is in cache it is refreshed when it is expired and
requested by the system. Object ID is not checked before refresh,
but config parameter ldap_(min|max)_id could be changed by admin.
We should check object ID and not refresh objects outside min/max
ID interval.

Resolves:
https://pagure.io/SSSD/sssd/issue/3905

@thalman thalman force-pushed the thalman:min_id_cache branch from 60237f7 to 4123f8d Jan 9, 2019

@thalman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@jhrozek Updated, I moved the test up to cache_req_search_cache. Also improved the cache_req_should_be_in_cache to check number of results and handle only result->count == 1.

Tested on my VM works well. Seems that checks on github have some issues.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

I re-run the tests, it passed ok this time. It seems that it was not able to contact some dnf repository.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.