URL: https://github.com/SSSD/sssd/pull/142
Author: fidencio
Title: #142: Fix breakage caused by having the NSS responder socket-activated
Action: opened
PR body:
"""
**MONITOR: Don't return an error in case we fail to register a service**
This patch is a pre-requisite for the next one and both patches could be
squashed. Although, I've decided to keep this patch separated as it brings back
a behaviour that has been used before the socket-activation series.
**MONITOR: Avoid starting an already started service**
This patch basically prevents that we end up socket-activating (or systemd
enabling) a service that has been explicitly enabled in the services' line.
**NSS: Make sssd-nss.service a hard requirement**
This patch takes a step back and do not allow the NSS responder to be
socket-activated.
Instead, it's added as a hard requirement for SSSD (in sssd.service) and will
be running whenever SSSD is running. Thankfully to our previous patch we ensure
that the only one instance of the responder will be running. So, if nss is not
present in the services' line, it will be started as a dep. In case it's part
of the services' line, it will be started by the monitor and later on started
by systemd. The second instance will be killed during the services'
registration part.
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/142/head:pr142
git checkout pr142
From fe4543ac1faccc113b8ceb28de6d376113d7c010 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Fri, 3 Feb 2017 18:31:51 +0100
Subject: [PATCH 1/3] MONITOR: Don't return an error in case we fail to
register a service
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This behaviour was mistakenly changed by the {dbus,socket}-activation
series and, as it's now, I've noticed the monitor may end up in some
weird state due to this change, where it doesn't stop properly and leave
some defuncts children processes.
This patch is needed for the coming one (where I've noticed the weird
behaviour) but I've decided to split in a different patch for the
clarity's sake.
Related:
https://fedorahosted.org/sssd/ticket/3298
Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com>
---
src/monitor/monitor.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index b82c6e5..d4c8615 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -363,11 +363,7 @@ static int client_registration(struct sbus_request *dbus_req, void *data)
sbus_request_finish(dbus_req, NULL);
/* FIXME: should we just talloc_zfree(conn) ? */
- if (ret == ENOENT) {
- goto done;
- }
-
- return ret;
+ goto done;
}
/* Fill in svc structure with connection data */
From cb9a5f8a666aa189195da0f50dc6530f60269888 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Fri, 3 Feb 2017 16:29:04 +0100
Subject: [PATCH 2/3] MONITOR: Avoid starting an already started service
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This situation can happen by socket-activating a service that has been
configured as explicitly-activated.
As there's no way, from the service side, to know whether the same
service is running, it'll always contact the monitor which will be
responsible for not having both services running at the same time.
In order to be able to kill the newly started process from the monitor,
now the processes send also their pid when registering and in case the
service's been already started the monitor sends a SIGTERM to the
processes.
Related:
https://fedorahosted.org/sssd/ticket/3298
Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com>
---
src/monitor/monitor.c | 15 +++++++++++++++
src/monitor/monitor_interfaces.h | 1 +
src/monitor/monitor_sbus.c | 7 +++++--
src/providers/data_provider_be.c | 2 +-
src/responder/common/responder_common.c | 2 +-
5 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index d4c8615..b768e44 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -292,6 +292,13 @@ static int get_service_in_the_list(struct mon_init_conn *mini,
for (svc = mini->ctx->svc_list; svc != NULL; svc = svc->next) {
if (strcasecmp(svc->identity, svc_name) == 0) {
+ if (svc->svc_started) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Service \"%s\" is already running but only "
+ "one instance of the service is allowed!\n",
+ svc_name);
+ return EINVAL;
+ }
svc->socket_activated = false;
*_svc = svc;
return EOK;
@@ -322,6 +329,7 @@ static int client_registration(struct sbus_request *dbus_req, void *data)
DBusError dbus_error;
dbus_uint16_t svc_ver;
dbus_uint16_t svc_type;
+ dbus_uint32_t svc_pid;
char *svc_name;
dbus_bool_t dbret;
int ret;
@@ -341,6 +349,7 @@ static int client_registration(struct sbus_request *dbus_req, void *data)
DBUS_TYPE_STRING, &svc_name,
DBUS_TYPE_UINT16, &svc_ver,
DBUS_TYPE_UINT16, &svc_type,
+ DBUS_TYPE_UINT32, &svc_pid,
DBUS_TYPE_INVALID);
if (!dbret) {
DEBUG(SSSDBG_CRIT_FAILURE,
@@ -363,6 +372,12 @@ static int client_registration(struct sbus_request *dbus_req, void *data)
sbus_request_finish(dbus_req, NULL);
/* FIXME: should we just talloc_zfree(conn) ? */
+ if (ret == EINVAL) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Sending a SIGTERM to the process %d\n", svc_pid);
+ kill(svc_pid, SIGTERM);
+ }
+
goto done;
}
diff --git a/src/monitor/monitor_interfaces.h b/src/monitor/monitor_interfaces.h
index 136beb4..ed0307a 100644
--- a/src/monitor/monitor_interfaces.h
+++ b/src/monitor/monitor_interfaces.h
@@ -49,6 +49,7 @@ errno_t sss_monitor_init(TALLOC_CTX *mem_ctx,
const char *svc_name,
uint16_t svc_version,
uint16_t svc_type,
+ uint32_t svc_pid,
void *pvt,
time_t *last_request_time,
struct sbus_connection **mon_conn);
diff --git a/src/monitor/monitor_sbus.c b/src/monitor/monitor_sbus.c
index faf28f7..702f4dd 100644
--- a/src/monitor/monitor_sbus.c
+++ b/src/monitor/monitor_sbus.c
@@ -112,7 +112,8 @@ static void id_callback(DBusPendingCall *pending, void *ptr)
static int monitor_common_send_id(struct sbus_connection *conn,
const char *name,
uint16_t version,
- uint16_t type)
+ uint16_t type,
+ uint32_t pid)
{
DBusMessage *msg;
dbus_bool_t ret;
@@ -134,6 +135,7 @@ static int monitor_common_send_id(struct sbus_connection *conn,
DBUS_TYPE_STRING, &name,
DBUS_TYPE_UINT16, &version,
DBUS_TYPE_UINT16, &type,
+ DBUS_TYPE_UINT32, &pid,
DBUS_TYPE_INVALID);
if (!ret) {
DEBUG(SSSDBG_CRIT_FAILURE, "Failed to build message\n");
@@ -166,6 +168,7 @@ errno_t sss_monitor_init(TALLOC_CTX *mem_ctx,
const char *svc_name,
uint16_t svc_version,
uint16_t svc_type,
+ uint32_t svc_pid,
void *pvt,
time_t *last_request_time,
struct sbus_connection **mon_conn)
@@ -196,7 +199,7 @@ errno_t sss_monitor_init(TALLOC_CTX *mem_ctx,
}
/* Identify ourselves to the monitor */
- ret = monitor_common_send_id(conn, svc_name, svc_version, svc_type);
+ ret = monitor_common_send_id(conn, svc_name, svc_version, svc_type, svc_pid);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, "Failed to identify to the monitor!\n");
return ret;
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 12b5f43..4c85c54 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -408,7 +408,7 @@ errno_t be_process_init(TALLOC_CTX *mem_ctx,
ret = sss_monitor_init(be_ctx, be_ctx->ev, &monitor_be_methods,
be_ctx->identity, DATA_PROVIDER_VERSION,
- MT_SVC_PROVIDER, be_ctx, NULL,
+ MT_SVC_PROVIDER, getpid(), be_ctx, NULL,
&be_ctx->mon_conn);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, "Unable to initialize monitor connection\n");
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 67922bf..664994a 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -1173,7 +1173,7 @@ int sss_process_init(TALLOC_CTX *mem_ctx,
ret = sss_monitor_init(rctx, rctx->ev, monitor_intf,
svc_name, svc_version, MT_SVC_SERVICE,
- rctx, &rctx->last_request_time,
+ getpid(), rctx, &rctx->last_request_time,
&rctx->mon_conn);
if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE, "fatal error setting up message bus\n");
From 862a1326e9940f15a4d082f3233af86629705fe9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Fri, 3 Feb 2017 23:02:43 +0100
Subject: [PATCH 3/3] NSS: Make sssd-nss.service a hard requirement
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
NSS responder seems to be a little bit problematic for having it
socket-activated. Some headaches already happened during the
development of the socket-activation series and now seems that having
sssd-nss,socket enabled makes some services (like systemd-logind) do not
start up, making the boot process to take forecever and making the user
end up with a broken system,
In order to avoid those issues, let's take a step back and make the
sssd-nss.service a hard requirement for sssd.service.
Although it won't be socket-activated, it still may be supressed from
the services' line.
Resolves:
https://fedorahosted.org/sssd/ticket/3298
Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com>
---
Makefile.am | 7 -------
contrib/sssd.spec.in | 4 ----
src/sysv/systemd/sssd-nss.service.in | 6 +-----
src/sysv/systemd/sssd-nss.socket.in | 12 ------------
src/sysv/systemd/sssd.service.in | 1 +
5 files changed, 2 insertions(+), 28 deletions(-)
delete mode 100644 src/sysv/systemd/sssd-nss.socket.in
diff --git a/Makefile.am b/Makefile.am
index 674d328..62e9471 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3967,7 +3967,6 @@ systemdconf_DATA =
if HAVE_SYSTEMD_UNIT
systemdunit_DATA += \
src/sysv/systemd/sssd.service \
- src/sysv/systemd/sssd-nss.socket \
src/sysv/systemd/sssd-nss.service \
src/sysv/systemd/sssd-pam.socket \
src/sysv/systemd/sssd-pam-priv.socket \
@@ -4057,7 +4056,6 @@ replace_script = \
EXTRA_DIST += \
src/sysv/systemd/sssd.service.in \
src/sysv/systemd/journal.conf.in \
- src/sysv/systemd/sssd-nss.socket.in \
src/sysv/systemd/sssd-nss.service.in \
src/sysv/systemd/sssd-pam.socket.in \
src/sysv/systemd/sssd-pam-priv.socket.in \
@@ -4104,10 +4102,6 @@ src/sysv/systemd/journal.conf: src/sysv/systemd/journal.conf.in Makefile
@$(MKDIR_P) src/sysv/systemd/
$(replace_script)
-src/sysv/systemd/sssd-nss.socket: src/sysv/systemd/sssd-nss.socket.in Makefile
- @$(MKDIR_P) src/sysv/systemd/
- $(replace_script)
-
src/sysv/systemd/sssd-nss.service: src/sysv/systemd/sssd-nss.service.in Makefile
@$(MKDIR_P) src/sysv/systemd/
$(replace_script)
@@ -4401,7 +4395,6 @@ endif
rm -f $(builddir)/src/sysv/systemd/sssd-autofs.socket
rm -f $(builddir)/src/sysv/systemd/sssd-autofs.service
rm -f $(builddir)/src/sysv/systemd/sssd-ifp.service
- rm -f $(builddir)/src/sysv/systemd/sssd-nss.socket
rm -f $(builddir)/src/sysv/systemd/sssd-nss.service
rm -f $(builddir)/src/sysv/systemd/sssd-pac.socket
rm -f $(builddir)/src/sysv/systemd/sssd-pac.service
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
index c0c9e36..e690fe5 100644
--- a/contrib/sssd.spec.in
+++ b/contrib/sssd.spec.in
@@ -802,7 +802,6 @@ done
%{_unitdir}/sssd-autofs.socket
%{_unitdir}/sssd-autofs.service
%{_unitdir}/sssd-ifp.service
-%{_unitdir}/sssd-nss.socket
%{_unitdir}/sssd-nss.service
%{_unitdir}/sssd-pac.socket
%{_unitdir}/sssd-pac.service
@@ -1148,7 +1147,6 @@ getent passwd sssd >/dev/null || useradd -r -g sssd -d / -s /sbin/nologin -c "Us
%post common
%systemd_post sssd.service
%systemd_post sssd-autofs.socket
-%systemd_post sssd-nss.socket
%systemd_post sssd-pac.socket
%systemd_post sssd-pam.socket
%systemd_post sssd-pam-priv.socket
@@ -1159,7 +1157,6 @@ getent passwd sssd >/dev/null || useradd -r -g sssd -d / -s /sbin/nologin -c "Us
%preun common
%systemd_preun sssd.service
%systemd_preun sssd-autofs.socket
-%systemd_preun sssd-nss.socket
%systemd_preun sssd-pac.socket
%systemd_preun sssd-pam.socket
%systemd_preun sssd-pam-priv.socket
@@ -1171,7 +1168,6 @@ getent passwd sssd >/dev/null || useradd -r -g sssd -d / -s /sbin/nologin -c "Us
%systemd_postun_with_restart sssd.service
%systemd_postun_with_restart sssd-autofs.socket
%systemd_postun_with_restart sssd-autofs.service
-%systemd_postun_with_restart sssd-nss.socket
%systemd_postun_with_restart sssd-nss.service
%systemd_postun_with_restart sssd-pac.socket
%systemd_postun_with_restart sssd-pac.service
diff --git a/src/sysv/systemd/sssd-nss.service.in b/src/sysv/systemd/sssd-nss.service.in
index 2e6fc79..3596b7b 100644
--- a/src/sysv/systemd/sssd-nss.service.in
+++ b/src/sysv/systemd/sssd-nss.service.in
@@ -4,10 +4,6 @@ Documentation=man:sssd.conf(5)
After=sssd.service
BindsTo=sssd.service
-[Install]
-Also=sssd-nss.socket
-
[Service]
-ExecStartPre=-/bin/chown root:root @logpath@/sssd_nss.log
-ExecStart=@libexecdir@/sssd/sssd_nss --debug-to-files --socket-activated
+ExecStart=@libexecdir@/sssd/sssd_nss --uid 0 --gid 0 --debug-to-files
Restart=on-failure
diff --git a/src/sysv/systemd/sssd-nss.socket.in b/src/sysv/systemd/sssd-nss.socket.in
deleted file mode 100644
index 530fa0c..0000000
--- a/src/sysv/systemd/sssd-nss.socket.in
+++ /dev/null
@@ -1,12 +0,0 @@
-[Unit]
-Description=SSSD NSS Service responder socket
-Documentation=man:sssd.conf(5)
-BindsTo=sssd.service
-
-[Socket]
-ListenStream=@pipepath@/nss
-SocketUser=@SSSD_USER@
-SocketGroup=@SSSD_USER@
-
-[Install]
-WantedBy=sssd.service
diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
index 05cfd37..5632019 100644
--- a/src/sysv/systemd/sssd.service.in
+++ b/src/sysv/systemd/sssd.service.in
@@ -3,6 +3,7 @@ Description=System Security Services Daemon
# SSSD must be running before we permit user sessions
Before=systemd-user-sessions.service nss-user-lookup.target
Wants=nss-user-lookup.target
+Requires=sssd-nss.service
[Service]
EnvironmentFile=-@environment_file@
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
