search

trusteddomainproject / OpenDKIM

Other
97 stars 52 forks source link

OpenRC service scripts #41

Open rseichter opened 5 years ago

rseichter commented 5 years ago

Having recently inherited the maintainership for Gentoo Linux's OpenDKIM package, I suggest adding our OpenRC service scripts.

orlitzky commented 5 years ago

Thanks Ralph, I was going to put together this list sometime later today but you were too fast.

There's a lot we need to clean up before we adding these... I'll try to list them in a sensible order.

1. Variable replacement

The magic variables like @sbindir@ are replaced at build-time by ./configure, but not unless you tell it to. The usual way is to rename the "source" file with an .in suffix, like opendkim.initd.in, and then that file needs to be added to the bottom of configure.ac so that autoconf knows to process it (it will replace the variables, and save the output without the .in suffix).

2. Use of runstatedir

Then, we should depend on a newer version of autoconf. At the top of configure.ac, there's a line like

AC_PREREQ(2.61)

which says what version needs to used, at a minimum. (This is only for developers). In autoconf-2.70, a new variable "runstatedir" was added:

https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

and that's what we want to use instead of /run everywhere, so I think we should depend on autoconf-2.70.

3. Add a tmpfiles.d entry

The @runstatedir@/opendkim directory needs special permissions, and a tmpfiles.d entry can do that for both OpenRC and systemd:

d @runstatedir@/opendkim 0755 opendkim opendkim

4. Using the new runstatedir to store the pid file and socket.

In the init script (and in upstream's system service file -- but we can worry about that later!), there are some things we can improve now that @runstatedir@ is available. The spirit of these is the same as the suggestions in my comment 5 on https://bugs.gentoo.org/606978, but instead of just declaring /run/opendkim to be the socket path by fiat, here we're constructing it from @runstatedir@ at build time.

pidfile="@runstatedir@/${RC_SVCNAME}.pid"

(Note: RC_SVCNAME is still fine, no need for $prog)

And we can get rid of all of the socket handling stuff:

command_args="-P ${pidfile} -x ${CONFFILE}"
...
start_pre() {
    # If this isn't a restart, make sure that the user's config isn't
    # busted before we try to start the daemon (this will produce
    # better error messages than if we just try to start it blindly).
    #
    # If, on the other hand, this *is* a restart, then the stop_pre
    # action will have ensured that the config is usable and we don't
    # need to do that again.
    if [ "${RC_CMD}" != "restart" ]; then
        checkconfig || return $?
    fi
}

We can get rid of the conf.d file now too; it's not needed if OpenRC/systemd agree to use @runstatedir@/opendkim for local sockets, and if the configuration file is the one source of truth for the socket path.

The stale socket handling has also been removed above, since we don't know the socket path. This is fine: the init system shouldn't be doing that anyway, and systemd doesn't do it. Stale sockets only get created in exceptional circumstances anyway, and when it happens, maybe you don't want your init system just deleting shit to hide the problem.

5. Configuration file updates

Now that "runstatedir" is gospel, we should put an example local socket entry in the config file, telling people that their local sockets should go in @runstatedir@/opendkim. The example configs are already processed by autoconf, so in e.g. opendkim.conf.simple.in we could add

# To use a local socket instead, specify a filename under @runstatedir@/opendkim.
# Socket  local:@runstatedir@/opendkim/opendkim.sock

6. Install process

Rather than document the files that need to be copied and where in a README, we should have e.g. make install-openrc do it.

7. Conclusion / summary of goals

Ultimately, both systemd and OpenRC will have @runstatedir@/opendkim created automatically, with the right permissions, by the tmpfiles.d entry. The socket would be set in the config file (where the correct path based on @runstatedir@ is shown), and then regardless of whether it was inet/local and regardless of the init system, things would just work.

Basically, we should be able to make && make install && make install-openrc on Gentoo after setting the correct --runstatedir=/run at build time, and have the resulting init script work out of the box, and have the systemd service also work out of the box with no changes.

It does seem like a lot of work, but supporting systemd forces us to do a few things simpler, and even though I'm a systemd hater, I think the simplicity is a good thing. With systemd's tmpfiles entry in place, we can just re-use it in OpenRC, greatly simplifying things there, too. Then it's just a matter of undoing some old cruft to bring the two to parity. And then upstreaming everything to get the cool @vars@.

rseichter commented 5 years ago

@orlitzky Please see my email.

orlitzky commented 5 years ago

@mskucherawy

I believe this is ready for serious consideration now. Most of the points I brought up in my earlier comment have been addressed:

  1. We've added backwards-compatible runstatedir support to the build system in anticipation of the next version of autotools.
  2. We've added an example to the simple configuration file, showing people the best place to put their local socket (i.e. one that init systems can anticipate and prepare for).
  3. We've added an OpenRC service script. Gentoo is the primary source of OpenRC users, and Ralph and I are responsible for OpenDKIM on Gentoo and are pretty confident that this script is in good shape. (In other words, I'm asking you to just trust us that it works if you don't run OpenRC yourself.) I promise to fix any bugs!
  4. To create the socket directory automatically at boot time in OpenRC/systemd, we've added a tmpfiles.d entry...
  5. ...and while we were at it, some improvements were made to the systemd service file. Hopefully these are uncontroversial; they are also tested on Gentoo, and happen to fix your issue #38.
  6. For the files this PR touches, we've implemented a better way to do full @variable@ substitution. Using e.g. AC_SUBST in configure.ac doesn't always substitute the full path, and a better way is suggested by the autotools documentation (in the "Installation Directory Variables" section). We haven't gone through and updated all of the other files that were using autoconf-based substitutions, but they are easily fixable now that the right framework is in place.

A few things we didn't fix:

Our Gentoo users are running the patch corresponding to this PR, and so far no one has complained. Ralph and I have also done a lot of testing. Carrying the patch isn't a big deal, but it makes it hard to pile on future improvements, so I hope we can get some feedback soon. Thanks for your time!

rseichter commented 4 years ago

@mskucherawy This pull request has now been open for more than a year. Some form of reaction would be appreciated.

mskucherawy commented 4 years ago

LGTM; are we going to add the RELEASE_NOTES text in here or in a later pull request?

rseichter commented 4 years ago

@mskucherawy I'm not certain if this is a question you are asking your co-reviewers, or @orlitzky and myself? Michael has provided extensive information in a comment above, and I doubt there is much room for improvement left. Could you use Michael's comment as a basis for the release notes?

orlitzky commented 4 years ago

Most of these changes are invisible to end users. How in-depth would you like the release notes to be? Something like,

In other news, autoconf-2.70 is (finally) almost ready. After it's widely available, we can require it in configure.ac to improve the @runstatedir@ user experience.