Bug 1431153 - SSSD ships a drop-in configuration snippet in /etc/systemd/system
Summary: SSSD ships a drop-in configuration snippet in /etc/systemd/system
Keywords:
Status: CLOSED EOL
Alias: None
Product: Fedora
Classification: Fedora
Component: sssd
Version: 25
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Stephen Gallagher
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-03-10 13:40 UTC by Martin Babinsky
Modified: 2017-12-12 10:49 UTC (History)
11 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1432010 (view as bug list)
Environment:
Last Closed: 2017-12-12 10:49:16 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Martin Babinsky 2017-03-10 13:40:15 UTC
Description of problem:

SSSD ships the following configuration snippet for the systemd service unit file:

/etc/systemd/system/sssd.service.d/journal.conf

Strictly speaking, /etc/systemd/system should only host whatever local configuration employed by system administrator and vendor-supplied configuration should go to /usr/lib/systemd/system or /run/systemd/system (see systemd.unit man page for more detials).

Apart from no adhering to these guidelines, these packaging issues also complicate containerization of services such as FreeIPA, which have to do a lot of unnecessary /etc/ scrubbing in order to provide a sane volume persistence model.

Version-Release number of selected component (if applicable):

sssd-1.15.1-1.fc25.x86_64

How reproducible:

Always.

Steps to Reproduce:
1. dnf install -y sssd-1.15.1-1.fc25.x86_64
2. rpm -ql sssd-common | grep -i /etc/systemd/system

Actual results:

/etc/systemd/system/sssd.service.d
/etc/systemd/system/sssd.service.d/journal.conf

Expected results:

The RPM should provide no configuration in /etc/systemd/system

Comment 1 Lukas Slebodnik 2017-03-10 14:59:15 UTC
This file does not change any behaviour.
It is a template for users who want to enable logging to journald.

sh$ grep journal.conf sssd.spec 
%config(noreplace) %{_sysconfdir}/systemd/system/sssd.service.d/journal.conf

sh$ cat /etc/systemd/system/sssd.service.d/journal.conf
[Service]
# Uncomment *both* of the following lines to enable debug logging
# to go to journald instead of /var/log/sssd. You will need to
# run 'systemctl daemon-reload' and then restart the SSSD service
# for this to take effect
#ExecStart=
#ExecStart=/usr/sbin/sssd -i

Comment 2 Jan Pazdziora 2017-03-10 15:39:30 UTC
Per systemd.unit(5):

       Table 1.  Load path when running in system mode (--system).
       ┌────────────────────────┬─────────────────────┐
       │Path                    │ Description         │
       ├────────────────────────┼─────────────────────┤
       │/etc/systemd/system     │ Local configuration │
       ├────────────────────────┼─────────────────────┤
       │/run/systemd/system     │ Runtime units       │
       ├────────────────────────┼─────────────────────┤
       │/usr/lib/systemd/system │ Units of installed  │
       │                        │ packages            │
       └────────────────────────┴─────────────────────┘

Files owned by installed packages should not be in /etc/systemd/system.

If it's just an example, please put it to the documentation directory.

Comment 3 Lukas Slebodnik 2017-03-10 15:40:31 UTC
It is not against packaging guidelines

Comment 4 Lukas Slebodnik 2017-03-10 15:42:12 UTC
And BTW this files is aimed for local configuration; (to change default logging from files to journald). So location is correct.

Comment 5 Jan Pazdziora 2017-03-10 15:52:05 UTC
It is very bad packaging engineering.

For the user to enable something, they should make a symlink to existing file, not edit existing file, and deal with .rpmnew when the next version of SSSD changes the recommendation.

Comment 6 Lukas Slebodnik 2017-03-10 16:03:35 UTC
(In reply to Jan Pazdziora from comment #5)
> It is very bad packaging engineering.

But you proposal for symlink to %config file in /usr is even worse.
It violates fedora packaging guidelines.

http://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files
"Don't use %config or %config(noreplace) under /usr. /usr is deemed to not contain configuration files in Fedora."

Comment 7 Simo Sorce 2017-03-10 17:07:14 UTC
The packaging guidelines in fact says systemd unit files must not be marked %config[1], so I assume this file also should not be marked config and be moved elsewhere.
Packages are not supposed to drop files in /etc/systemd/system

[1] http://fedoraproject.org/wiki/Packaging:Systemd
"Systemd unit .service files must not be marked as %config files."

This is something that either go in %{_unitdir} or in a doc directory.
Please move it.

Comment 8 Lukas Slebodnik 2017-03-10 17:36:43 UTC
It is not ".service" file it is a configuration snippet and all sssd service files are properly stored in %{_unitdir}.

sh$ rpm -ql sssd-common | grep service$
/usr/lib/systemd/system/sssd-autofs.service
/usr/lib/systemd/system/sssd-ifp.service
/usr/lib/systemd/system/sssd-nss.service
/usr/lib/systemd/system/sssd-pac.service
/usr/lib/systemd/system/sssd-pam.service
/usr/lib/systemd/system/sssd-secrets.service
/usr/lib/systemd/system/sssd-ssh.service
/usr/lib/systemd/system/sssd-sudo.service
/usr/lib/systemd/system/sssd.service

Comment 9 Jan Pazdziora 2017-03-13 14:07:36 UTC
(In reply to Lukas Slebodnik from comment #6)
> (In reply to Jan Pazdziora from comment #5)
> > It is very bad packaging engineering.
> 
> But you proposal for symlink to %config file in /usr is even worse.
> It violates fedora packaging guidelines.

I did not say where that symlink should point to.

Comment 10 Jan Pazdziora 2017-03-13 14:09:21 UTC
(In reply to Lukas Slebodnik from comment #8)
> It is not ".service" file it is a configuration snippet and all sssd service

It is .service file snippet.

Comment 11 Simo Sorce 2017-03-13 20:01:45 UTC
To be honest I am not even sure why we have this file which just contains comments.
Add the comments to the main service file that is in /lib/systemd/system and let users create a unit file based on the comments if they need.

Also please stop closing this bug until the discussion is settled.
Having a package dropping files in /etc/systemd *is* an issue.

Comment 12 Lukas Slebodnik 2017-03-13 20:29:15 UTC
(In reply to Simo Sorce from comment #11)
> To be honest I am not even sure why we have this file which just contains
> comments.
Explained in comment#1

> Add the comments to the main service file that is in /lib/systemd/system and
> let users create a unit file based on the comments if they need.
> 
Current approach is much user-friendly.
Because users will not notice change in /usr/lib/systemd/system/sssd.service
like in commit https://github.com/SSSD/sssd/commit/d4063e9a21a4e203bee7e0a0144fa8cabb14cc46

It's much convenient to compare journald.conf and journald.conf.rpmnew

> Also please stop closing this bug until the discussion is settled.
> Having a package dropping files in /etc/systemd *is* an issue.

I cannot see a reason to have opened ticket which is not a bug.
And discussion can keep going even in closed ticket.

Comment 13 Simo Sorce 2017-03-14 11:13:09 UTC
I started a thread on fedora-devel to sort this out, so far 2 responses both agree that rpms should drop nothing in /etc/systemd/system:

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org
/message/J4KJEC25HZ7BCZPR4UIY2TMXQPV5MX2Y/

Comment 14 Lukas Slebodnik 2017-03-14 12:56:53 UTC
I do not consider empty file in /etc/systemd/system as a unit file.

Reassigning to sgallagh who is author of this feature.
NOTE: removing file is not a solution; it will break upgrades. from "Type=forking"  to "Type=notify"
https://pagure.io/SSSD/sssd/c/d4063e9a21a4e203bee7e0a0144fa8cabb14cc46
https://pagure.io/SSSD/sssd/c/7b4704a10958bb7d3390db9eff863875d2b643f7

Comment 15 Stephen Gallagher 2017-03-14 13:35:41 UTC
This ticket is definitely a bug, for the record. I made a bad choice when I landed this the way I did and it's biting us now.

The proper approach here should probably have been to change the SSSD unit file so that it would look something like:


```
[Unit]
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

[Service]
EnvironmentFile=-/etc/sysconfig/sssd
ExecStart=/usr/sbin/sssd -i ${DEBUG_LOGGER}
Type=notify
NotifyAccess=main
CapabilityBoundingSet=CAP_IPC_LOCK CAP_CHOWN CAP_DAC_READ_SEARCH CAP_DAC_OVERRIDE CAP_KILL CAP_NET_ADMIN CAP_SYS_NICE CAP_FOWNER CAP_SETGID CAP_SETUID CAP_SYS_ADMIN CAP_SYS_RESOURCE CAP_BLOCK_SUSPEND

[Install]
WantedBy=multi-user.target

```

/etc/sysconfig/sssd

```
# SSSD debug logs go to local files
DEBUG_LOGGER=-f

```


Now, with that being said, I'm not sure how we would want to handle the migration. One idea: detect in %post if /etc/systemd/system/sssd.service.d/journal.conf hashes to one of two known results: either the original commented-out version or the version that contains just the two lines uncommented. If it's the original version, just delete it. If it's the uncommented version, `echo "DEBUG_LOGGER=" >> /etc/sysconfig/sssd` (this will override the earlier value in the file, then delete the override file. If it doesn't match to either, someone made manual edits and we should leave it alone.

Comment 16 Simo Sorce 2017-03-14 14:54:25 UTC
This sounds like a reasonable idea, with the caveat that you should remove also any .rpmnew .rpmsave or whatever may have been created during upgrades where a changed file was present.

Comment 17 Jan Kurik 2017-08-15 09:03:03 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 27 development cycle.
Changing version to '27'.

Comment 18 Fedora End Of Life 2017-12-12 10:49:16 UTC
Fedora 25 changed to end-of-life (EOL) status on 2017-12-12. Fedora 25 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.


Note You need to log in before you can comment on or make changes to this bug.