URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: opened
PR body: """ Added option use_memcache to centrally disable memcache for all clients without the need to specify SSS_NSS_USE_MEMCACHE=NO environment variable.
Resolves: https://pagure.io/SSSD/sssd/issue/3496 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ I am opening this PR even though it is not 100% decided that we want this option. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-330824739
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ I'm sorry memory cache is essential part of sssd and it's already documented in manual page sssd.conf how to disable it. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-330833608
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Rejected
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
jhrozek commented: """ We had this discussion several times. There are people who (at least two weeks ago) wanted this feature. I know you provided a different workaround since then and we should check if that customer uses the workaround.
But please don't close the pull request. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-330938365
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: reopened
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Rejected
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """
We had this discussion several times. There are people who (at least two weeks ago) wanted this feature. I know you provided a different workaround since then and we should check if that customer uses the workaround.
And it was mentioned several times that there are other way how to achieve it without changing sssd code. Therefore this PR should be rejected.
Adding label without closing it for now. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-330958132
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Rejected
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ On Wed, Sep 20, 2017 at 9:37 PM, lslebodn notifications@github.com wrote:
We had this discussion several times. There are people who (at least two weeks ago) wanted this feature. I know you provided a different workaround since then and we should check if that customer uses the workaround.
And it was mentioned several times that there are other way how to achieve it without changing sssd code. Therefore this PR should be rejected.
That's not what was discussed in the last meetings. Otherwise mzidek wouldn't work on this PR.
Adding label without closing it for now.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/390#issuecomment-330958132, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4eueDeI9wnrqI64Cno5hzW1wkl5q9ks5skWl3gaJpZM4Pduvu .
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-330958994
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
jhrozek commented: """ On Wed, Sep 20, 2017 at 07:37:29PM +0000, lslebodn wrote:
We had this discussion several times. There are people who (at least two weeks ago) wanted this feature. I know you provided a different workaround since then and we should check if that customer uses the workaround.
And it was mentioned several times that there are other way how to achieve it without changing sssd code. Therefore this PR should be rejected.
Adding label without closing it for now.
And it was mentioned several times that the workarounds are not considered sufficient. We discussed this at length on our weekly phone meeting.
That was the place and time to argue, not rejecting a PR after we as a team agree to implement this.
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-330962260
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Rejected
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ @pbrezina Thank you for the comments. If the cache is disabled centrally using the use_memcache option, client applications can not use it regardless of SSS_NSS_USE_MEMCACHE env variable. If the memcache is enabled on SSSD side, then the client applications can use SSS_NSS_USE_MEMCACHE=NO to override it.
I added one sentence to the description to make it more clear. Tell me if it is enough. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331406516
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
pbrezina commented: """ I made slight changes: ``` + <para> + Whether to use in-memory cache to improve + performance. If this option is set to False the in-memory cache is not used and + the environment variable SSS_NSS_USE_MEMCACHE + is ignored. + </para> + <para> + Default: True + </para> + <para> + NOTE: If this option is set to true and the environment variable + SSS_NSS_USE_MEMCACHE is set to "NO", client + applications will not use the fast in-memory + cache. ```
Although I would prefer that the environment variable will always override the configuration option. I.e. if it is false and env.var. is set to yes, than memory cache will be used. Opinions? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331409260
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @pbrezina, @mzidek-rh: Although I do believe that Pavel's suggestion is reasonable, I can see some problems with that, which I will try to describe below: - option and env-var are evaluated in different parts of the code: I see this as a possible limitation for @pbrezina's suggestion, although it could be changed; - env-var is only checked for NO: In case we want it to have priority, we have to have the code changed in a way that we also would evaluate YES and "NOT PRESENT" and based on this we could decide whether to use or not the memcache - having two methods for doing the very same thing is not so nice (as then we start dealing with priorities): So, here is more like a question than a suggestion ... can't we just use this option from 2.0 (where we'll be breaking compats anyways)?
Enough "bla bla bla" .... so, summing up I guess @mzidek-rh's approach is simpler while also being functional. So, I'd go for it, at least till some customer complains about the approach. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331416745
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ @pbrezina That would not be a good idea IMO. If the administrator decides that the memcache should be disabled, then the client applications should not be able to override it. Also we would have to refactor how SSS_NSS_USE_MEMCACHE works, because currently we just check in the client code if it is set to 'NO' and we skip the memcache if it is (anything else means it is enabled). With the way you suggested it, we would also have to check for 'YES' and 'not defined' which IMO complicates the whole thing.
Also note that by doing it the way you mentioned, we would not be able to just 'not initialize' the memcache for use_memcache=False and we would need to read the option from client side to decide what to do. Which again is a complication (an unnecessary one IMO) of client code. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331417906
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ @fidencio I did not notice your comment before. But I do agree :) """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331418180
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ Updated the man page to use wording that @pbrezina suggested. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331420300
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
pbrezina commented: """ Ok then. Thank you for these arguments. Ack. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331447961
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ Running sssd without memory cache is significant performance problem which should not be supported by upstream. Therefore it should not be documented. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331450760
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ Moreover there should not be any evidence that such feature exist. This misfeature is even worse then disabling tls for authentication. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331451258
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ @lslebodn I am not sure about that. Maybe we could document the fact that it has significant performance impact? Retrospectively, I think we should do this also for SSS_NSS_USE_MEMCACHE. It may be obvious to some people, but stressing it out is better IMO. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331451687
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (22/09/17 06:48), mzidek-rh wrote:
@lslebodn I am not sure about that. Maybe we could document the fact that it has significant performance impact? Retrospectively, I think we should do this also for SSS_NSS_USE_MEMCACHE. It may be obvious to some people, but stressing it out is better IMO.
SSS_NSS_USE_MEMCACHE is a different case. Because you intentionally disable it per client. Disabling memcache globally on heavy load server (e.g. mailserver) is like jumping from skyscraper without a parachute.
(I know what I am talking about https://pagure.io/SSSD/sssd/issue/3520)
Such misfeature should not have a place in upstream. We should not repeat the same mistake as with allowing authentication without tls. Ideal would be have this PR just as a downstream patch to make one particular customer (with unreasonable request due to workarounds on top of workarounds) happy.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331454131
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ As for not putting this to upstream and only to the requested downstream distro, I do not like it. We may end up backporting it to future versions of that downstream distro as well (after rebases), which is IMO unnecessary burden for downstream maintainers. I do not think it is worth it just to avoid having this in upstream. In general, I would like to avoid "downstream only" patches as much as possible.
As for not documenting the option the same way as we do for the option to disable tls, I do not like this either, but if other developers agree with that, I am OK with it. The difference between the option to disable tls and this one is that disabling tls was added for testing purposes, while this one is added to support actual customer's use case and as such it should be IMO documented. But indeed, the current version of the man page is probably not stressing out enough how severe the performance impact of using this option is, so we can add a warning there.
I did not do any changes to the man page for now, the options are: - do not document the option in man pages (Lukas likes) - stress out that using it can have significant negative performance impact (Michal likes)
I would like to hear from other developers what they think the best approach is before I do any changes. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331477185
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
jhrozek commented: """ I prefer to document this as well. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331692154
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ I think adding to option to the man page is the only way to make sure the negative impact of this option is properly documented and nobody can be hit by the effects of the option by surprise (I know nobody reads man pages but that's a different story). Additionally this way we can underline that this option should not be used in production but only for testing or to temporarily work around an issues until it is solved. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331797883
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (25/09/17 07:23), sumit-bose wrote:
I think adding to option to the man page is the only way to make sure the negative impact of this option is properly documented and nobody can be hit by the effects of the option by surprise (I know nobody reads man pages but that's a different story). Additionally this way we can underline that this option should not be used in production but only for testing or to temporarily work around an issues until it is solved.
I do not agree, If we do not document anything and do not advertise it then people will not use it. This option does not have any reasonable use-case. You can already use `SSS_NSS_USE_MEMCACHE` + systemd for that purpose.
Moreover, problematic customer does not have a problem with memory cache. because it was explained many times that even though some application(postfix) clean environment variables and (thus also SSS_NSS_USE_MEMCACHE) then such application will never cause issue with opened fd to removed mc files. postfix will do many request to sssd and therefore sssd-client will reopen fresh mc files. So customer does not have any reasonable request for disabling memory cache. They just want that because they want and they are big.
However they are fine with systemd workaround but they want to disable memory cache on el6 and el5. And it it obvious that memory cache is not they main problem because they didn't even test it properly. (el5 does not have memory cache). And el6 does not have an init system which would call getpwnam/getgrnam/initgroups for each service user session.
So their main problem is that authentication is denied with full /var.
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331826630
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (22/09/17 15:19), mzidek-rh wrote:
As for not putting this to upstream and only to the requested downstream distro
It is a problem of downstream distribution that it accept unreasonable request. So they should handle it as downstream patch. There is not any technical reason why this misfeature should be in upstream.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331827301
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (24/09/17 07:14), Jakub Hrozek wrote:
I prefer to document this as well.
Then I would prefer to document all hidden options and advertise them in next release.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331827683
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ Firstly, sorry for jumping in in the discussion without having any technical contribution to give.
While is quite clear that @lslebodn is against the solution, I'd just like to quote a message from him in another PR review (https://github.com/SSSD/sssd/pull/250#issuecomment-297701355): "It looks like 2 guys like version with if. I will prepare version with if even though that ternary is more readable :-). Democracy works.".
So, basically, I do have to agree with @lslebodn in the sense that democracy works and the most part of the developers do prefer this issue documented, so I do believe it's the way to go.
About "prefering all hidden options documented and advertised", please, open a ticket for that and we'll discuss that in our next triage meeting (and the decision of the majority wins, as democracy works!). """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331833359
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ As @mzidek-rh, @jhrozek, @sumit-bose, @pbrezina and myself prefer to have the feature documented, I'm adding back the "Accepted" label, which was given by @pbrezina after his review of this patch. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331837488
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ Sure democracy works but there is not a reason why some discouraged options shoudl be documented and other should not be documented. We need to be consistent. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331838899
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ Sure democracy works but there is not a reason why some discouraged options should be documented and other should not be documented. We need to be consistent. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331838899
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ Sure democracy works but there is not a reason why some discouraged options should be documented and other should not be documented. We need to be consistent. And @mzidek already wrote that current version is not good. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331838899
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @lslebodn: issue has been opened https://pagure.io/SSSD/sssd/issue/3524 and would be way more coherent to have the discussion related to it there in the pagure issue.
Also, can you be explicit about which changes are you requesting? Either that or do not add the "Changes requested" label. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331841174
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """
Also, can you be explicit about which changes are you requesting? Either that or do not add the "Changes requested" label.
https://github.com/SSSD/sssd/pull/390#issuecomment-331838899 """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331844048
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ I still don't understand.
"Sure democracy works but there is not a reason why some discouraged options should be documented and other should not be documented. We need to be consistent."
It should be treated in https://pagure.io/SSSD/sssd/issue/3524 and not here.
"And @mzidek already wrote that current version is not good." He didn't write it after the review and re-work of his patch.
In any case ... @mzidek-rh, is there any known issue in your patch? In case there is, would be nice to advert the reviewer about them.
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331844892
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ @fidencio I mentioned that I think we should add warning to the man page, but I was not sure if we end up deleting the man page, so I did not add the change immediately.
Looking at the discussion now I think more people agree that we should document it, so I added one more patch with the warning. The previous patch is the same. Please check if the wording in the new patch is appropriate. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331852770
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (25/09/17 10:46), fidencio wrote:
I still don't understand.
"Sure democracy works but there is not a reason why some discouraged options should be documented and other should not be documented. We need to be consistent."
It should be treated in https://pagure.io/SSSD/sssd/issue/3524 and not here.
I have very bad experience with fixing similar requests later. There are ignored because they are not important.
Firstly wee need to make a decision whether we want to document "discouraged options" and then we can move this forward. At the moment it is not clear.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331863699
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @lslebodn, your request is out of the scope of this patch.
I see your request as exactly the same case as https://github.com/SSSD/sssd/pull/373#issuecomment-330161363. And we're taking exactly the same path as taken there by filing an issue and we really should move the discussion to that issue as it's out of the scope of this patch. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331866559
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ I updated the 'Warning' patch to use wording suggested by @fidencio (I like it more then mine).
Btw. I do not mind documenting other discouraged options, such as the one to disable TLS if they are accompanied with warnings if it is the only way to move this patch forward.
Btw. what other options are we talking about? I can only think of the one to disable TLS. If it is just that, I can add it to this PR as separate patch, it will be quick change. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331869590
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ In retrospect I agree that we should document all options. It is not that hard to find the options but currently it is not easy to understand the side effects to those options because they are not documented.
Documenting them gives us the ability to explain why it is not a good idea to use them, what the side effects are and why they are there at all. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331878234
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """
@lslebodn, your request is out of the scope of this patch.
It is not because if we decide that we do not want to document discouraged options by upstream then this PR need to be changed. BTW discouraged means also that they are not documented and they are is not tested by upstream. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331878670
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (25/09/17 13:19), sumit-bose wrote:
Documenting them gives us the ability to explain why it is not a good idea to use them, what the side effects are and why they are there at all.
Most of people do not care about warnings. They just want working solution even though it is insecure. And if you give them a simple way how to find such things they would like it.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331880216
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ On (25/09/17 13:19), sumit-bose wrote:
Documenting them gives us the ability to explain why it is not a good idea to use them, what the side effects are and why they are there at all.
Most of people do not care about warnings. They just want working solution even though it is insecure. And if you give them a simple way how to find such things they will like it.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-331880216
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
jhrozek commented: """ There is a *really* big difference between an option that leaks user's credentials to anyone sniffing the network and an option that lets you decrease NSS lookups performance. Especially since IIRC at the moment, fully qualified names are not stored in the memory cache at all.
It's really about the damage you can do with the option. If you disable the memory cache and the system performance is slow, well, too bad, worst that could have happened is that your users file a helpdesk ticket. If you let your credentials leak over the network, it's a security problem.
So, let's document this option along with its disadvantages. No sensible person will set this option and if they do, well too bad, their SSSD instance will behave poorly. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-332616726
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ Hi, I just squashed the patch with the warning with the original patch, I only kept the patches separate for review purpose, but as @fidencio mentioned it is better to squash them.
As for documenting the option to disable TLS, I did not add it to this PR, we can discuss it in a different PR or on IRC. I would prefer to not do any changes to this PR at this point. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333044977
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ Just fired a CI build with the last version of this patch and I'll add the "Accepted" label as soon as I have the results.
Thanks for your contribution! """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333052491
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ Although I'm fine with the current version of the patch I'd like to make a suggestion which might help to make this change less controversial.
I recently discussed with Simo that SSS_MC_CACHE_ELEMENTS is current hardcoded to 50000 which might be a git small in special cases, e.g. an IPA server with trust to a large AD forest. So we agreed that it would make sense to make this configurable. If we would add an option like 'memcache_elements' or similar a value of '0' would effectively mean to disable the memory cache. This corner case should of course be discussed as in the current patch in the man page. With this disabling the memory cache would just be a side-effect of an option with a broader usage. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333052808
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ Hmm. I like @sumit-bose and @simo5 idea. We just have to be crystal clear that setting the new option to zero will disable the memcache (or any really small value) and that will have a strong deficit in the performance (and, of course, that it's not recommended).
@mzidek-rh, do you agree with the suggestion? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333053469
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ I agree with the proposal. Will update new version. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333102308
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ I have one question to the implementation. I will add one option for all memcache types, do we also want to have separate options for different types of memcache? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333103147
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
simo5 commented: """ @mzidek-rh yes I think in some cases we may want a much larger group memcache than user memcache """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333122077
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
simo5 commented: """ @mzidek-rh yes I think in some cases we may want a much larger group memcache than user memcache """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333172795
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ I've added an "in-line" comment about keeping the warning from the previous patch. Adding the "Changes Requested" label. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333954657
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ Also, thinking loud here ... IMO would make more sense if memcache_size would override the others and not the opposite (but I don't have a strong opinion on this).
And, please, could you add more details to the commit message? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-333977183
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ @fidencio, so far the more specific option was used, see e.g. the entry_chache_* options. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334072059
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @sumit-bose: Okay, makes a lot of sense to be consistent about the way the options are presented.
Thanks for answer. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334076058
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @mzidek-rh, firstly, sorry for not noticing this before ... your tests are covering only the memcache_size option. Does make sense to expand the tests to also cover the other options? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334160332
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ Yes, the test only covers the original use case. I will add test cases to disable only certain memcache types, thanks for noticing that. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334161841
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ Sorry there is an error in the tests, I will update new version. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334193007
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ I'm sorry but this change is unnecessary complicated and add too many unnecessary new options.
If some downstream distribution want to have a way how to disable it then they can use simple oneliner downstream patch ``` diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index a87ad646f..7ee22557c 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -1218,6 +1218,7 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name, int payload; int ret, dret;
+ if (timeout == 0) return EOK; switch (type) { case SSS_MC_PASSWD: payload = SSS_AVG_PASSWD_PAYLOAD; ```
And if other developers would like to have it in upstream then it can be there with `#ifdef EXPERMIENTAL_FEATURE` around """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334434651
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ @lslebodn, I think your one-liner patch (together with a comment in the memcache_timeout man page entry) should go into upstream as well. Because currently setting 'memcache_timeout = 0' will make them memcache unusable but still use disk and memory space, so it is somewhat ill-defined. Your patch will make this clear and defined behavior.
Neverless I'd like to have the option to configure the memory cache size as well. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334448236
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to disable memcache Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ I changed the tests, because as they were written before they did not work (we probably have a bug in memcache, but it is not related to these patches, because it is reproducible even without them).
I will open a new issue to track that bug. So far it was only reproducible in the pytest environment.
The code itself is the same, only the tests have been updated to work around the mentioned bug. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-334452440
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ I see I forgot to remove the changes requested label. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-335440884
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @sumit-bose, let me try to understand what was the agreement we have for this patch series. - @mzidek-rh's patches are going to be pushed as soon as they're acked, right? - @lslebodn's patch suggestion should also go upstream, is it? Do we want this for 1.16?
I'll review the new tests implemented by @mzidek-rh, fire a few CI builds and ACK this patchset base on the conversations we had before. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-336364498
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @mzidek-rh's patches look good to me and I've fired a couple of CI builds.
I'll add the "Accepted" label after those builds are successfully finished.
@sumit-bose, @lslebodn: IMO does make sense to treat @lslebodn's suggestion as a different PR, do you guys agree on that? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-336784728
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ @fidencio, I'm fine either way. But please note the @lslebodn's suggestion is much easier solution to backport to released versions to allow to disable the memory cache. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-336809583
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ @sumit-bose, I don't have any strong opinions here, but my understanding was that we would like to have **both** solutions merge. Is that the case or I did misunderstand it? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-336811832
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ @fidencio, yes, I think it would be good to implement both to make clear what memchache_timeout=0 means and to allow to configure how many entries the memory caches can hold. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-336813345
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ As CI passed, I'm adding the "Accepted" label. - http://vm-058-233.$%7Babc%7D/logs/job/79/68/summary.html
I've ran the CI a few times and had 2 non-related tests failures. One of those being new (at least to me: http://vm-058-233.$%7Babc%7D/job/79/71/fedora23/ci-build-debug/ci-make-distc...) and an issue will be opened in pagure soon.
Per the conversation in this PR, @lslebodn's patch is also something that should go upstream and will be posted as a different PR. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-337345684
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
lslebodn commented: """ Setting changes requested per comment https://github.com/SSSD/sssd/pull/390#issuecomment-334434651 """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-337543353
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
fidencio commented: """ Although I'm disappointed to see the "Changes Requested" label set, I'm not really surprised.
I've asked a few times whether the decision was to have both approaches merged and the answer was yes and now I'm really failing to understand how it changed ...
Could someone, please, help me to understand this situation and, mainly, help me to understand how to avoid similar situations in the future? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-337545019
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
sumit-bose commented: """ @lslebodn, the patch is not about disabling memcache anymore but about making the size of the memcache configurable. Why do you think this should be labeled as experimental? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-337571690
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to disable memcache
mzidek-rh commented: """ Just for the record. We decided to not include these these patches in the next release.
The options will be more useful once a memcache diagnostic function is implemented in the sssctl tool. So until then, this PR is put on hold. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-338195318
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to tune the memcache size
mzidek-rh commented: """ fidencio wrote:
Do we still want to have those changes merged at some point?
Yes, but there was condition that we need a way to get statistics about the cache usage, so that users know when to make the cache bigger based on some information that we can provide. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-371428333
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to tune the memcache size
mzidek-rh commented: """ fidencio wrote:
Do we still want to have those changes merged at some point?
Yes, but there was condition that we need a way to get statistics about the cache usage, so that users know when to make the cache bigger based on some information that we can provide.
EDIT: Ah, hit the "Comment" button too soon :) I think the statistics were somehow related to the work @sumit-bose was doing in memcache, but I do not remember how. Sumit, do you know? Alternatively we can talk about this on devel meeting today. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-371428333
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to tune the memcache size
fidencio commented: """ I see. It's just my lack of memory kicking me really hard while I'm going through the list of opened PRs. Thanks for the answer @mzidek-rh! """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-371433325
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to tune the memcache size
mzidek-rh commented: """ I will close this PR. We have a ticket to track the RFE for memcache statistics tool. Configurable memcache size can be tracked by that ticket as well, we do not need to keep this open. """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-544450778
URL: https://github.com/SSSD/sssd/pull/390 Author: mzidek-rh Title: #390: NSS: Add option to tune the memcache size Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/390/head:pr390 git checkout pr390
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to tune the memcache size
alexey-tikhonov commented: """
I will close this PR. We have a ticket to track the RFE for memcache statistics tool. Configurable memcache size can be tracked by that ticket as well, we do not need to keep this open.
Could you please provide a link to this RFE? """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-544491942
URL: https://github.com/SSSD/sssd/pull/390 Title: #390: NSS: Add option to tune the memcache size
alexey-tikhonov commented: """
I will close this PR. We have a ticket to track the RFE for memcache statistics tool. Configurable memcache size can be tracked by that ticket as well, we do not need to keep this open.
Could you please provide a link to this RFE?
I guess this is https://pagure.io/SSSD/sssd/issue/3552 """
See the full comment at https://github.com/SSSD/sssd/pull/390#issuecomment-544493378
sssd-devel@lists.fedorahosted.org