Bug 1471056 - Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon
Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Robert-André Mauchin
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-14 06:27 EDT by Marc Dequènes (Duck)
Modified: 2017-10-04 13:59 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-10-04 13:59:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zebob.m: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Marc Dequènes (Duck) 2017-07-14 06:27:15 EDT
Spec URL: https://gitlab.com/osas/osas-infra-team-rpm-pkg/blob/master/postsrsd/postsrsd.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/duck/osas-infra-team-rpm-repo/epel-7-x86_64/00573099-postsrsd/postsrsd-1.4_a77bf99-2.el7.centos.src.rpm
Description: PostSRSd is used to mitigate strict SPF settings (like done by Red Hat) in order to allow forwarding from a custom domain. For example myname@example.com aliases to example@redhat.com would fail. PostSRSd rewrites addresses similarly to mailing-lists softwares to allow this.
Fedora Account System Username: duck


Quack,

This package is not present in the distribution ans no other ticket was found.

The package builds well and works well too. Also despite the fact the last build failed on Copr, the previous one worked fine and the spec file did not change between the two (just added documentation in the repo), so there is most probably a bug on Copr's side.

This software relies on a modified version of libsrs2 (http://www.libsrs2.org/download.html) which is unmaintained since 2014. I did not reach out postsrsd upstream yet. Having a new maintainer for libsrs2 and building postsrsd with it would be much better but that's a lot of work outside the scope of this package.

Regards.

\_o<
Comment 1 Marc Dequènes (Duck) 2017-07-21 03:12:53 EDT
Quack,

As for the build problems, it was partially solved in ticket #1471066 and is being continued in #1473361 and #1473364. The build was relaunched and works well on all F25/F26/EL7 distros and related architectures.

Could someone haz a look pleaze?
\_o<
Comment 2 Robert-André Mauchin 2017-08-21 12:40:47 EDT
Hello/Quack,

Several issues:

 - Please don't use tabs, use spaces

 - What do you package a snapshot of the git, instead of the latest stable version? Please justify this.

 - If you do want to package the snapshot, the Version and Release tag are wrong. First you should define a %commitdate

%global commit date 20170118

  Then fix you Version and Release:

Version:    1.4
Release:    2.%{commitdate}git%{shortcommit0}%{?dist}

  Don't forget to also change the changelog to reflect this:

* Fri Apr 14 2017 Marc Dequènes (Duck) <duck@redhat.com> - 1.4-2.20170118gita77bf99

 - The Group: tag is not used in Fedora. See https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
If you also package for RHEL, put it inside a condition.


 - The Source0 format is wrong, change it to:

Source0:    https://github.com/roehling/%{name}/archive/%{commit0}/%{name}-%{commit0}.tar.gz

  - Requires:	/usr/bin/base64 is not needed I think, since it is provided by coreutils

  - Don't use:

cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS="$(RPM_OPT_FLAGS)" -DCMAKE_INSTALL_PREFIX=/usr -DGENERATE_SRS_SECRET=OFF -DUSE_SELINUX=ON

  Instead please use the %cmake macro which takes care of the path and add your custom option afterwards:

cd build && %cmake .. -DGENERATE_SRS_SECRET=OFF -DUSE_SELINUX=ON

  - Similarly, please use %make_build and %make_install macro:

%make_build -C build

%make_install -C build

  - # proper location for systemd config ⇒ This is not the propre location, service file must be placed in the special dir %{_unitdir}:

mkdir -p %{buildroot}/%{_unitdir}
mv %{buildroot}/etc/systemd/system/postsrsd.service %{buildroot}/%{_unitdir}/postsrsd.service


  You also need to change the path in your sed patch:

sed -ri -e 's/postsrsd\/default/postsrsd.default/' \
        -e "s/(\[Install\])/RuntimeDirectory=postsrsd\nRuntimeDirectoryMode=0750\n\n\1/" %{buildroot}/%{_unitdir}/postsrsd.service


  And fix it in %files too:

%{_unitdir}/postsrsd.service

  - systemd service files requires special scriplets to be run in %post, %preun and %postun. See: https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Systemd


%post
%systemd_post %{name}.service

%preun
%systemd_preun %{name}.service

%postun
%systemd_postun_with_restart %{name}.service

  You also need to add a BR with a special macro:

%{?systemd_requires}
BuildRequires: systemd

  - You *must* add the LICENSE file with %license in %files:

%license LICENSE

  - %doc should contain the relevant README files:

%doc README.md README_UPGRADE.md README.exim.md

  - Don't use /usr/sbin, use %{_sbindir}

%{_sbindir}/postsrsd


  - Don't use /usr/share/doc but:

%{_docdir}/postsrsd

  - Same with man:

%{_mandir}/man8/postsrsd.8*

  - The whole SELinux part looks fishy. Let's do it the Fedora way. First add the Requires:

Requires(post): policycoreutils, initscripts
Requires(preun): policycoreutils, initscripts
Requires(postun): policycoreutils

  Then we add the correct scriplets in %post, %preun and %postun:

%post
if [ "$1" -le "1" ]; then # Fist install
    semodule -i %{_datadir}/selinux/packages/%{name}/postsrsd.pp 2>/dev/null || :
    fixfiles -R postsrsd restore || :
    /sbin/service postsrsd condrestart > /dev/null 2>&1  || :
fi

%preun
if [ "$1" -lt "1" ]; then # Final removal
    semodule -r postsrsd 2>/dev/null || :
    fixfiles -R postsrsd restore || :
    /sbin/service postsrsd condrestart > /dev/null 2>&1 || :
fi

%postun
if [ "$1" -ge "1" ]; then # Upgrade
    # Replaces the module if it is already loaded
    semodule -i %{_datadir}/selinux/packages/%{name}/postsrsd.pp 2>/dev/null || :
    # no need to restart the daemon
fi


  Once you fix all this, please check if the package is working as expected as I'm not well versed in SELinux.
Comment 3 Marc Dequènes (Duck) 2017-08-22 00:57:43 EDT
Thanks Robert-André for the review.

As for the version, the following fix was needed to make the service usable at all: https://github.com/roehling/postsrsd/pull/65
I should have made it clearer in the changelog and will fix this.

You're right, base64 is part of coreutils. The following page tells me a BuildRequires on it is unnecessary: https://fedoraproject.org/wiki/HOWTOFindMissingBuildRequires
It is ok for this package, nevertheless this is really not satisfying. How are we supposed to know the list of these packages. Someone on the Internet suggested it could be packages in group 'Core' but got no interesting reply. I can't find more documentation on this, could point at the canonical source of info?


As for SELinux, could you tell me where these information come from, it is not in the Fedora Packaging guidelines?

So the fishy part was about allowing the scritlets to work on machines without SELinux activated. Is Fedora officially not supporting such configurations?

Are these scriplets requires really necessary, it looks to me packages from a base installation, and also in the 'Core' group?

I did not know fixfiles, really practical; I would still need to call restorecon manually for /etc/postsrsd.secret as it is generated and not in %files. Or is there another way?

Also calling semodule only at first install seems not a good idea. The daemon might change and require new rules, so it's safer to rebuild after each upgrade to avoid breaking the service. Same for fixfiles. And then same for restarting the service. Am I wrong?

Also what is "|| :"? Seems to behave like "|| true" but I don't find it very readable (might depend on your font size though).

So I'm going to push fixes which are well understood. The rest would come soon.
Regards.
\_o<
Comment 4 Robert-André Mauchin 2017-08-22 02:59:44 EDT
>As for the version, the following fix was needed to make the service usable at all

All right then.


>As for SELinux, could you tell me where these information come from, it is not in the Fedora Packaging guidelines?

The SELinux packaging draft. See https://fedoraproject.org/wiki/PackagingDrafts/SELinux at the bottom.

> So the fishy part was about allowing the scritlets to work on machines without SELinux activated

SELinux is enabled by default on Fedora. As you see above in the TODO, the case where SELinux is disabled is not considered.
Comment 5 Marc Dequènes (Duck) 2017-08-22 06:14:12 EDT
It leaves a few questions unanswered but I went on.

Also I dug some more around the selinux example and looked more in details at the scriptlets scheduling. It's very peculiar to see upgrade actions launched at the postun stage (supposedly "uninstall" stuff) because at this point the old package possible leftover files are no more and it is safe to restart the service and so on…

Anyway, considering previous comments and adaptations due to the fact we don't need 'service' as we're relying on systemd, I made changes to the package.

I'm currently running this version in production and it seem to work fine.

Would you please have another look?
Comment 6 Robert-André Mauchin 2017-08-22 07:18:06 EDT
You also need to remove the %clean section which is not needed in Fedora: https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
Comment 7 Marc Dequènes (Duck) 2017-08-22 09:54:19 EDT
Done Sir :-).
Comment 8 Robert-André Mauchin 2017-08-22 13:58:11 EDT
I forgot a few thing:

 - make is not needed as a BR
 - /etc/default/ → %{_sysconfdir}/default in %files
 - Contrary to what I've told you before, remove:
 
%doc README.md README_UPGRADE.md README.exim.md
 
 It creates conflicts and duplicate files with the %{_docdir} macro, and usage of both in the same spec is disallowed.
Comment 9 Marc Dequènes (Duck) 2017-08-22 22:55:15 EDT
That's right, the buildsys already takes care of installing the doc.

I made the changes.
Comment 10 Robert-André Mauchin 2017-08-23 02:20:58 EDT
We're all good then, package accepted.


Thanks for your work.
Comment 11 Marc Dequènes (Duck) 2017-08-23 03:22:25 EDT
Yeahhhhhh :-)
Thank you for your help.
Comment 12 Matthias Runge 2017-09-26 09:36:54 EDT
A few more remarks:
- please separate changelog entries by empty lines:

* Tue Aug 22 2017 Marc Dequènes (Duck) <duck@redhat.com> - 1.4-5.20170118gita77bf99
- remove %%clean section, not needed in Fedora

* Tue Aug 22 2017 Marc Dequènes (Duck) <duck@redhat.com> - 1.4-4.20170118gita77bf99
- don't remove secret file during upgrade
- start service at the end of post scriptlet
- improve SELinux rules handling (now requires a running SELinux)

* Tue Aug 22 2017 Marc Dequènes (Duck) <duck@redhat.com> - 1.4-3.20170118gita77bf99


you shouldn't remove buildroot and create it again. That can be expected to be clean 
(begin of %install section)

On irc, you mentioned another package waiting for review. Please point me to that, I might be able to get you sponsor you.
Comment 13 Marc Dequènes (Duck) 2017-09-27 10:18:44 EDT
-7 has the needed changes.
Comment 15 Matthias Runge 2017-09-28 04:29:52 EDT
Two more nits: please break the description line < 79 chars. Also: the build requirement on gcc is not necessary.

Since I've already sponsored you into the packager group, you can go ahead and submit a request for a git repo to be created. Feel free to ping me, if there are any questions.
Comment 16 Marc Dequènes (Duck) 2017-09-28 04:44:39 EDT
Taking care of the latest fixes.

Thanks a lot :-).
Comment 17 Gwyn Ciesla 2017-09-28 09:02:53 EDT
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/postsrsd
Comment 18 Andreas Thienemann 2017-09-30 05:45:27 EDT
Thank you for submitting the package and doing all the work to get it into Fedora.

I did notice one thing about the .spec file though I did not like. The handling of the postsrsd.secret file could be improved.

I like that you generate it in the %post script, thus ensuring that every install has a unique file.
But I do not like the way you clean up the file in the %postun scriptlet.

The way it is handled right now is that there's a file in /etc/ that does not belong to any package.

# rpm -ql postsrsd | grep -F postsrsd.secret
# rpm -qf /etc/postsrsd.secret
file /etc/postsrsd.secret is not owned by any package
#

I would expect the postsrsd package to own this file.
You can achieve that by adding the filepath with the %ghost tag to the spec file in the %files section. http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html has some details.

The rm -f command can be removed then.
Please fix that before uploading the package.
Comment 19 Marc Dequènes (Duck) 2017-10-04 12:43:52 EDT
Quack,

I had already made a build. I fixed it in postsrsd-1.4-9.20170118gita77bf9. Thanks for your nice suggestions.

Now I wonder what I might have missed because I guess this ticket should be closed now, and my other one too. Is it not automagically closed when a successful build is done?
Comment 20 Robert-André Mauchin 2017-10-04 13:45:20 EDT
It's not automated, you should close it yourself and select Rawhide as a resolution.
Comment 21 Marc Dequènes (Duck) 2017-10-04 13:59:27 EDT
Thanks Robert-André

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