Page MenuHomeFreeBSD

rc: also run NAME_setup on NAME_reload
ClosedPublic

Authored by franco_opnsense.org on Aug 19 2022, 6:24 AM.
Tags
Referenced Files
Unknown Object (File)
Thu, Oct 17, 2:58 AM
Unknown Object (File)
Tue, Oct 15, 5:09 AM
Unknown Object (File)
Mon, Oct 14, 12:01 AM
Unknown Object (File)
Sun, Oct 13, 3:54 PM
Unknown Object (File)
Sat, Oct 12, 9:12 AM
Unknown Object (File)
Fri, Oct 11, 1:14 AM
Unknown Object (File)
Thu, Oct 10, 6:43 AM
Unknown Object (File)
Oct 8 2024, 11:37 PM

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 47871
Build 44758: 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.

No takers? This has been in a half-working state in FreeBSD 14 for quite some time. :/

This revision is now accepted and ready to land.May 28 2024, 2:06 PM

Oh and I was going to try merge it :P
So we can close this review?

Yes I just need to wrap up testing for the updated GH PR

This won't merge, I'm talking to @imp at https://github.com/freebsd/freebsd-src/pull/1258

I'm just waiting for confirmation it works with the latest.

In my work for the automatic service jails (committed last week), I stumbled upon cases (in the base system) where some kind of functions were used to setup variables before starting the actual service, which was not necessary. When the automatic service jail is enabled for such a service, some of this stuff was run outside the service jail and didn't had the desired effect. After changing those scripts to not setup variables like that, it worked.

I would be interested to know about ports with start scripts where the change of this review makes a difference, to determine what kind of impact this has for service jails.

In principle this only changes the user side setup (rc.conf), not the ports side. There is no direct ports consumer of this feature and it tries to hook into all services in order to automate configuration changes when service commands are dispatched.

I followed your lead in staying close to the precmd calls which do a similar thing, but from the ports side.