[sssd PR#319][opened] sudo: add a threshold option to reduce size of rules refresh filter

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/319
Author: pbrezina
 Title: #319: sudo: add a threshold option to reduce size of rules refresh filter
Action: opened

PR body:
"""
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.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/319/head:pr319
git checkout pr319

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

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

Basically, what does **exactly** mean "too big 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 big to be processed by
the server" actually means).

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

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314430250

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

fidencio commented:
"""
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 big 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 big to be processed by
the server" actually means).

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

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314430250

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

fidencio commented:
"""
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 big to be processed by
the server" actually means).

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

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314430250

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

fidencio commented:
"""
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.
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314430250

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

pbrezina commented:
"""
On 07/11/2017 02:38 PM, fidencio wrote:
>  While I'm not assigning myself as a review of this patch, there
> are a
>  few questions that came to my mind while reading it.
>
>  Basically, what does *exactly* mean "too big 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? 
It may be a server limitation (query too big) or it may be just very 
slow processing. A customer hit a bug when he had over 2k rules 
refreshed in rules filter which is no good. So we need to limit it somehow.

>  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 big to be processed by the
>  server" actually means). 

"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314435624

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

fidencio commented:
"""
On Tue, Jul 11, 2017 at 3:00 PM, Pavel Březina <notifications(a)github.com&gt;
wrote:

>  On 07/11/2017 02:38 PM, fidencio wrote:
>  > While I'm not assigning myself as a review of this patch, there are a
>  > few questions that came to my mind while reading it.
>  >
>  > Basically, what does *exactly* mean "too big 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?
>
>  It may be a server limitation (query too big) or it may be just very
>  slow processing. A customer hit a bug when he had over 2k rules
>  refreshed in rules filter which is no good. So we need to limit it somehow.
>  
Yep. I'm totally fine about limiting it. But do we really need to expose an
option for this?
Couldn't we go for 1024 rules being the maximum supported number and that's
it? (Of course, we still should have it documented).


>  > 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 big to be processed by the
>  > server" actually means).
>
>  —
>  You are receiving this because you commented.
>  Reply to this email directly, view it on GitHub
>  <https://github.com/SSSD/sssd/pull/319#issuecomment-314435624>;, or mute
>  the thread
>
> <https://github.com/notifications/unsubscribe-auth/AAG4ep87hegrpdx6ICqz5_l...
>  .
>  
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314437974

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

pbrezina commented:
"""
We can leave the option ondocumented in man pages.
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314695481

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

fidencio commented:
"""
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?
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-314716422

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

pbrezina commented:
"""
I honestly do not know what the default should be, 50 is something that came out in the
[bz](https://bugzilla.redhat.com/show_bug.cgi?id=1468610#c10) 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.
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-315317653

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

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

"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-315319103

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

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

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-322191224

[sssd PR#319][synchronized] sudo: add a threshold option to reduce size of rules refresh filter

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/319
Author: pbrezina
 Title: #319: sudo: add a threshold option to reduce size of rules refresh filter
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/319/head:pr319
git checkout pr319

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

pbrezina commented:
"""
Resubmitted.
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-322469689

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[Jakub Hrozek]

On Fri, Jul 14, 2017 at 11:47:32AM +0200, fidencio wrote:
>    URL: https://github.com/SSSD/sssd/pull/319
>  Title: #319: sudo: add a threshold option to reduce size of rules refresh filter
>  
>  fidencio commented:
>  """
>  Okay. Please, let me know when you hear back from the customer and I'll do the review
> of these patches.
>  
>  """ 
The customer reported that the patch works fine. I will run a couple of
downstream tests before marking the patch as Accepted.

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

jhrozek commented:
"""
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.
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-323178440

[sssd PR#319][+Accepted] sudo: add a threshold option to reduce size of rules refresh filter

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

Label: +Accepted

[sssd PR#319][closed] sudo: add a threshold option to reduce size of rules refresh filter

[lslebodn]

   URL: https://github.com/SSSD/sssd/pull/319
Author: pbrezina
 Title: #319: sudo: add a threshold option to reduce size of rules refresh filter
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/319/head:pr319
git checkout pr319

[sssd PR#319][comment] sudo: add a threshold option to reduce size of rules refresh filter

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

lslebodn commented:
"""
master:
* a5f300adf19ec9c3087c62bd93a5175db799687a
"""

See the full comment at https://github.com/SSSD/sssd/pull/319#issuecomment-323275255

[sssd PR#319][+Pushed] sudo: add a threshold option to reduce size of rules refresh filter

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/319
Title: #319: sudo: add a threshold option to reduce size of rules refresh filter

Label: +Pushed