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

Add "Wants=" to sssd unit #132

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@fidencio
Copy link
Contributor

commented Jan 24, 2017

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.

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from 7fb0577 to fa069cc Jan 24, 2017

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

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.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

@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.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from fa069cc to 2bf284c Jan 24, 2017

@fidencio fidencio changed the title Add "Wants=" to sssd unit and avoid PAC responder to be always running Add "Wants=" to sssd unit Jan 24, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

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.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

BTW the second patch had to be also removed because required functions were in newer versions of systemd which are not in CentOS

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from 2bf284c to 06a5fbf Jan 25, 2017

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from 06a5fbf to e730392 Aug 7, 2018

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Just rebased atop of git master.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Sockets for secrets and kcm responders are avoided on purpose?

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Yes, they are. Theoretically secrets and kcm responders' sockets are (or should be) enabled by default by the distro.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

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"

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from e730392 to a6b6966 Aug 15, 2018

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

Patch set has been updated and is ready for review.
I'm also removing the "postponing until sssd 2.0" label.

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from a6b6966 to 5824df0 Aug 15, 2018

@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

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 9c0c83e, so we are good here.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

@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.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

[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?

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from 5824df0 to 55ddb4d Aug 17, 2018

@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

This sounds better. I'm having some issues when testing this patch, but it may be unrelated. I will let you know later.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

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.
@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

That being said, I do not oppose your work Fabiano, it would be used in that case as well.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

@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.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Fair enough. I'm adding accepted label.

@pbrezina pbrezina added the Accepted label Aug 20, 2018

@pbrezina pbrezina self-assigned this Aug 20, 2018

fidencio added some commits Jan 24, 2017

sssd: add a list of dependent services to sssd.service
Let's add a list of dependent services to the sssd unit file so we can
have all those services enable by default when enabling sssd unit.

As it differs from our first approach were all services were disabled by
default, the manuals have also been updated.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
monitor: remove add_implicit_services()
As the services are socket-activated and enabled by default by SSSD,
there's no need to keep this code which has the only purpose to add the
PAC responder to the services list when needed.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
socket_activated_responders: improve tool's error log
Improve the sssd_check_socket_activated_responders' error log in case of
misconfiguration and also start to log it into the syslog instead of
logging it only into the responders' log files.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
socket_activated_responders: deal with conf less setup
In case there's no sssd.conf, let's just start the services' sockets.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

@fidencio fidencio force-pushed the fidencio:wip/sssd_wants branch from 55ddb4d to 570dae9 Aug 22, 2018

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

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

@jhrozek jhrozek removed the Accepted label Aug 24, 2018

fidencio added some commits Aug 24, 2018

socket_activated_responders: check also confdb
We also have to check confdb as the implicit files domain brings in the
nss responder by default.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
socket_activated_responders: updated debug message
Mention that the service may be implicitly started.

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

@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. :-(

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.