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

sudo: add a threshold option to reduce size of rules refresh filter #319

Closed
wants to merge 1 commit into from

Conversation

@pbrezina
Copy link
Member

pbrezina commented Jul 11, 2017

If a large number of rules is expired at one time the ldap filter may
become too large to be processed by server. This commits adds a new
option "sudo_threshold" to sudo responder. If the threshold is
exceeded a full refreshed is done instead of rules refresh.

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Jul 11, 2017

While I'm not assigning myself as a reviewer of this patch, there are a few questions that came to my mind while reading it.

Basically, what does exactly mean "too large to be processed by the server"? Is this some limitation encountered on server side? Is this something that differs on different LDAP server's implementation?

The main reason I'm asking this is because I'm not big fond of having this option. While I'm pretty sure it does work, I'd prefer to have something automatically done internally, otherwise we may just end up answering bug reports with "please, try to tune this option to ..." which is not exactly convenient. (Of course, I'm assuming here that we have at least some idea about what "too large to be processed by the server" actually means).

Please, let me know what you think about my suggestion, @pbrezina.

@pbrezina

This comment has been minimized.

Copy link
Member Author

pbrezina commented Jul 11, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Jul 11, 2017

@pbrezina

This comment has been minimized.

Copy link
Member Author

pbrezina commented Jul 12, 2017

We can leave the option ondocumented in man pages.

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Jul 12, 2017

I've talked privately to @pbrezina and as far as I understand it's a common practice to add the option on SSSD instead of having something as a default behaviour in the project (even if well documented) without providing a way for the user to tune it.

So, I'm partially taking back my comments and we can go for @pbrezina's original approach.

Last question about this patch series from me ...

@pbrezina: you said that customers had issues with 2k+ rules. Considering that, a default value as 50 wouldn't be too low? 1k wouldn't be a more realistic value that would avoid a lot of users having to change it?

@pbrezina

This comment has been minimized.

Copy link
Member Author

pbrezina commented Jul 14, 2017

I honestly do not know what the default should be, 50 is something that came out in the bz discussion and I think it is reasonable. The filet looks like (|(cn=rule1)...(cn=ruleN)) + host filters. Even for 50 rules it is really big filter to be evaluated. Customer was provided with a test package, we should wait for their confirmation.

@fidencio

This comment has been minimized.

Copy link
Contributor

fidencio commented Jul 14, 2017

Okay. Please, let me know when you hear back from the customer and I'll do the review of these patches.

@fidencio fidencio self-assigned this Jul 14, 2017
@fidencio fidencio removed their assignment Aug 10, 2017
@jhrozek

This comment has been minimized.

Copy link
Contributor

jhrozek commented Aug 14, 2017

@pbrezina could you resubmit with ticket #3478 linked to the commit message? I'll review the patches in the meantime.

@jhrozek jhrozek self-assigned this Aug 14, 2017
If a large number of rules is expired at one time the ldap filter may
become too large to be processed by server. This commits adds a new
option "sudo_threshold" to sudo responder. If the threshold is
exceeded a full refreshed is done instead of rules refresh.

Resolves:
https://pagure.io/SSSD/sssd/issue/3478
@pbrezina pbrezina force-pushed the pbrezina:sudo branch from 7cd4f78 to 1380806 Aug 15, 2017
@pbrezina

This comment has been minimized.

Copy link
Member Author

pbrezina commented Aug 15, 2017

Resubmitted.

Copy link
Contributor

jhrozek left a comment

Code-wise LGTM and confirmed to work by an affected customer. Will run tests before adding the Accepted label.

@jhrozek

This comment has been minimized.

Copy link
Contributor

jhrozek commented Aug 17, 2017

downstream tests, passed, ACK.

Whoever is pushing the patch (if it's not me), please also add fabiano to reviewed-by since he asked a lot of important questions.

@jhrozek jhrozek added the Accepted label Aug 17, 2017
@lslebodn

This comment has been minimized.

Copy link
Contributor

lslebodn commented Aug 18, 2017

master:

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