[sssd PR#132][opened] Add "Wants=" to sssd unit and avoid PAC responder to be always running
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running Action: opened
PR body: """ The first patch changes the current logic of having the services' sockets disabled by default as it adds a "Wants=" to the sssd unit file, making all the services' sockets enabled by the moment sssd service is enabled.
The second patch takes advantage of the first patch and avoids running PAC responder in case its socket is active, leaving the service to be socket-activated when needed. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running
lslebodn commented: """ I am sorry I do not agree with such solution. As I wrote few times on IRC. We should simplify monitor code and not add new code there. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-274771546
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running
fidencio commented: """ @lslebodn: Okay, you don't agree with the second patch and I'm totally fine about that (in case other developers don't agree as well). So, we can just drop it without any problem.
However, this PR also inclyudes the first patch, which is important and I'd like to have it reviewed. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-274772147
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running
lslebodn commented: """ On (24/01/17 02:58), fidencio wrote:
@lslebodn: Okay, you don't agree with the second patch and I'm totally fine about that (in case other developers don't agree as well). So, we can just drop it without any problem.
However, this PR also inclyudes the first patch, which is important and I'd like to have it reviewed.
PR should be reviewd as whole patch set and not just some parts. We can continue with review after droping problematic patch.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-274774278
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit and avoid PAC responder to be always running Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: edited
Changed field: title Original value: """ Add "Wants=" to sssd unit and avoid PAC responder to be always running """
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ The patch changes the current logic of having the services' sockets disabled by default as it adds a "Wants=" to the sssd unit file, making all the services' sockets enabled by the moment sssd service is enabled.
The second patch was dropped for now and I will send an email to SSSD ML asking for opinions and trying to expose both mine and @lslebodn ideas. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-274781056
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
lslebodn commented: """ BTW the second patch had to be also removed because required functions were in newer versions of systemd which are not in CentOS """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-274812500
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ CI: http://sssd-ci.duckdns.org/logs/job/61/27/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-274844018
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
Label: +postponed until sssd 2.0
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ Just rebased atop of git master. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-411114188
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ Sockets for secrets and kcm responders are avoided on purpose? """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-411698413
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ Yes, they are. Theoretically sssd and kcm responders' sockets are (or should be) enabled by default by the distro. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-411701824
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ Yes, they are. Theoretically secrets and kcm responders' sockets are (or should be) enabled by default by the distro. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-411701824
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ OK. In that case the manpage change should be rephrase because the term `all services` is misleading. ``` By default, all services are enabled. In case the Administrator wants to persistently disable one of them, it can be done by running: "systemctl mask sssd-@service@.socket" ``` """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-411714943
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ Patch set has been updated and is ready for review. I'm also removing the "postponing until sssd 2.0" label. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-413358685
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
Label: -postponed until sssd 2.0
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ I don't really understand this line:
``` + "However, SSSD automatically pulls in the "%s" socket(s) and " + "relies on systemd to start and manage the responder's " + "process.\n" ```
Otherwise patches looks good. As you mentioned on IRC, systemd will not start the socket if reposonder is already enabled in sssd.conf per 9c0c83eecf, so we are good here. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-413486215
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ @pbrezina: what I tried to say is that sssd.service has a Wants=sssd-@responder.socket, which will make the sssd-@responder.socket to be started as soon as sssd.service is started ... relying then on systemd. I'm totally open for suggestions on how to rewrite this part to make ir more clear for our users. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-413491333
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ ``` [ffidenci@pessoa sssd]$ git diff diff --git a/src/tools/sssd_check_socket_activated_responders.c b/src/tools/sssd_check_socket_activated_responders.c index e83de622b..0b1a4df94 100644 --- a/src/tools/sssd_check_socket_activated_responders.c +++ b/src/tools/sssd_check_socket_activated_responders.c @@ -192,9 +192,9 @@ int main(int argc, const char *argv[]) "The "services" line contains "%s", meaning that the " "responder's process will be started and managed by SSSD's " "monitor. " - "However, SSSD automatically pulls in the "%s" socket(s) and " - "relies on systemd to start and manage the responder's " - "process.\n" + "However, SSSD relies on systemd to start " + "sssd-%s.socket and then manage the responder's process, " + "causing then a configuration conflict.\n" "In order to solve this misconfiguration, please, either " "remove "%s" from the "services" line in "%s" or call " "`systemctl mask sssd-%s.socket`\n" ```
@pbrezina, does it look better? """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-413800580
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ This sounds better. I'm having some issues when testing this patch, but it may be unrelated. I will let you know later. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-414258216
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ OK. It was not related. Patches works as expected so ACK to the patches but...
Services that are enabled through sssd.conf yields the following warning, which is correct: ``` [root /var/lib/sss/db]# systemctl status sssd-nss.socket ● sssd-nss.socket - SSSD NSS Service responder socket Loaded: loaded (/usr/lib/systemd/system/sssd-nss.socket; disabled; vendor preset: disabled) Active: failed (Result: exit-code) since Mon 2018-08-20 11:57:03 CEST; 6min ago Docs: man:sssd.conf(5) Listen: /var/lib/sss/pipes/nss (Stream)
Aug 20 11:57:03 pbrezina.my systemd[1]: Starting SSSD NSS Service responder socket. Aug 20 11:57:03 pbrezina.my sssd[9458]: There's a misconfiguration in the "services" line of "/etc/sssd/sssd.conf"! The "services" line contains "nss", meaning that the responder's process will be started and managed by SSSD's monitor. However, SSSD relies on s In order to solve this misconfiguration, please, either remove "nss" from the "services" line in "/etc/sssd/sssd.conf" or call `systemctl mask ss Please, refer to "sssd.conf" man page for more info and mind that the recommended way to go is to take advantage of systemd, as much as possible, Aug 20 11:57:03 pbrezina.my systemd[1]: sssd-nss.socket: Control process exited, code=exited status=17 Aug 20 11:57:03 pbrezina.my systemd[1]: Failed to listen on SSSD NSS Service responder socket. Aug 20 11:57:03 pbrezina.my systemd[1]: sssd-nss.socket: Unit entered failed state. ``` However, I'm not sure if this is something desirable for all responders. Everyone have `nss` and `pam` services enabled through `sssd.conf` from its beginning and socket activation use case is not really desirable for them as you want to authenticate and id fast very often.
I already mentioned it on the meeting and I will mention it again just so the idea gets proper thoughts. If we are going to enable all responders/sockets and rely on systemd by default maybe it is time to drop this functionality from monitor (also we are now in 2.0 so we can do such change).
* We can rely completely on systemd to manage socket and services start and monitor. * If a service is present in sssd.conf, it will have no idle timeout and it will run indefinitely. * If a service is not present in sssd.conf, it will terminated itself after some time as it is now. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-414268841
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ That being said, I do not oppose your work Fabiano, it would be used in that case as well. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-414269114
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ @pbrezina, I totally agree and the steps described can be taken as a continuation of my patch. I'd like to: - Have my patch merged; - Have a bug opened with your proposal;
So, any contributor could take the issue to work on. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-414285015
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
pbrezina commented: """ Fair enough. I'm adding accepted label. """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-414299547
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
jhrozek commented: """ I still found two issues: - with no sssd.conf, the nss service is still enabled in confdb, so it's started by the monitor and then also the socket-activated service is started - for some reason, the autofs (I picked this service arbitrarily) service didn't start for me with sssd.conf which didn't enable the autofs service """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-415761243
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ @jhrozek, patch set has been updated.
Now it also checks the confdb in order to now what we should do.
Please, give it a try, all the issues you have seen should be resolved.
In case something is still missing, I'd like to leave this patches here and someone else can keep working on this as I'm not part of SSSD team anymore. Of course, please, **keep my authorship on my patches** or at least mention your work has been based on them.
Unfortunately this PR took way too long to get the attention it needed. :-( """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-415790746
URL: https://github.com/SSSD/sssd/pull/132 Title: #132: Add "Wants=" to sssd unit
fidencio commented: """ @jhrozek, patch set has been updated.
Now it also checks the confdb in order to now what we should do.
Please, give it a try, all the issues you have seen should be resolved.
In case something is still missing, I'd like to leave these patches here and someone else can keep working on this as I'm not part of SSSD team anymore. Of course, please, **keep my authorship on my patches** or at least mention your work has been based on them.
Unfortunately this PR took way too long to get the attention it needed. :-( """
See the full comment at https://github.com/SSSD/sssd/pull/132#issuecomment-415790746
URL: https://github.com/SSSD/sssd/pull/132 Author: fidencio Title: #132: Add "Wants=" to sssd unit Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/132/head:pr132 git checkout pr132
sssd-devel@lists.fedorahosted.org
-
fidencio -
jhrozek
-
pbrezina