Page MenuHomeFreeBSD

rc: also run NAME_setup on NAME_reload
Needs ReviewPublic

Authored by franco_opnsense.org on Aug 19 2022, 6:24 AM.
Tags
Referenced Files
Unknown Object (File)
Fri, Feb 23, 4:29 PM
Unknown Object (File)
Fri, Feb 23, 6:53 AM
Unknown Object (File)
Sat, Feb 17, 7:28 PM
Unknown Object (File)
Sat, Feb 17, 4:02 PM
Unknown Object (File)
Sat, Feb 17, 2:46 PM
Unknown Object (File)
Dec 30 2023, 2:46 PM
Unknown Object (File)
Dec 20 2023, 9:41 PM
Unknown Object (File)
Dec 20 2023, 7:47 AM

Details

Reviewers
oshogbo
0mp
Summary

Reload is used for service reconfiguration as well
and lacks a NAME_prepend-like mechanism so it makes
sense to extend the NAME_reload hook into this
action.

precmd may use configuration checks and blocks setup
from doing its designated work (e.g. nginx). In moving
the invoke of the setup script in front allows us to
provide custom scripts for config file generation and
fixing prior to precmd checking configuration integrity.

Also introduce _run_rc_setup to separate the launcher
from the main one. Let it run correctly in the case
of restart_precmd and block further execution as
would be the case in start due to the internal plumbing
of restart being split into calling stop and start
afterwards.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 54544
Build 51433: arc lint + arc unit

Event Timeline

rc: extend NAME_setup, redefining commands escapes all structure

change setup/precmd order so when precmd checks config file it won't fail

Sorry for so late review.
Should we also add it to rc.subr(8) man page?

  • update documentation on internals and caveats

@oshogbo I've updated the documentation and also described the caveats with the current implementation of restart_precmd which is pretty dangerous when not using restart_cmd ... but running the setup there prior to running it again during start seems silly just to pass a potential failure of restart_precmd. A number of ports seem to be using this override but a "proper" config file should be present if we assume that start was ran successfully first?

@oshogbo I've updated this again fixing a remaining issue with restart_precmd -- if you can find the time to review I'd appreciate it.

can you show me an example of this in action?
how are you using it in your ports?

Set up script variable:

https://github.com/opnsense/core/blob/137e7af193e43/src/opnsense/service/templates/OPNsense/Monit/rc.conf.d#L3

Script to run:

https://github.com/opnsense/core/blob/137e7af193e4321/src/opnsense/scripts/OPNsense/Monit/setup.sh

This is a simple use case to fix the template generation that has a limitation on our side to create files in 0644 mode. Monit blocks faulty permission due to restart_precmd custom script in rc file which prevents restart for us.

A complex example is:

https://github.com/opnsense/core/blob/137e7af193e4/src/opnsense/scripts/proxy/setup.sh

This one ensures that directories required are set up correctly (pkg-upgrade only does this during a reinstall but directories might be missing for other reasons) and collects certificates for Squid to use. You can even generate the configuration files from here.

We have been using the "setup.sh" type automation in OPNsense since 2015 and built it up to this rc system integration. The current version except the restart handling is what we are shipping in releases so this is being run out in the wild already.

Maybe to add to that: the main motivation was a user-side precmd type hook support since the precmd is hard or impossible to overwrite (being used in the rc scripts defined by the ports) and name_setup=path/to/file seemed to be the easiest solution.