Bug 255106
| Summary: | www/caddy: rc.d/caddy requires admin API for stopping | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Thijs van Dien <thijs> | ||||||
| Component: | Individual Port(s) | Assignee: | Adam Weinberger <adamw> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Some People | CC: | feld, scf | ||||||
| Priority: | --- | Flags: | bugzilla:
maintainer-feedback?
(adamw) |
||||||
| Version: | Latest | ||||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
|
Description
Thijs van Dien
2021-04-16 04:36:05 UTC
I was wondering about that. It's certainly not ideal; I'd far prefer to have the admin API off. Caddy doesn't document which signals it responds to. It'd be nice to have a graceful shutdown signal, a reload signal, and a reopen logs signal, but I have no idea which (if any) of those it responds to. Here's a comment I found in the source: "trapSignalsCrossPlatform captures SIGINT or interrupt (depending on the OS), which initiates a graceful shutdown. A second SIGINT or interrupt will forcefully exit the process immediately." The reload signal I brought up here: https://github.com/caddyserver/caddy/issues/3967#issuecomment-820943864. Not sure what you mean by reloading logs? Correction: reopening logs. *** Bug 266769 has been marked as a duplicate of this bug. *** The change in https://cgit.freebsd.org/ports/commit/www/caddy/files/caddy.in?id=9e47fe9f65ed633d956ee2f6e19ea9ff6e8a95ff does not fix it as Adam suggested in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266769#c1 It still just hangs and hangs. But we can fix this by using wait_for_pids. I'll send a patch. Created attachment 237027 [details]
caddy shutdown improvement
Here's a patch that provides pretty good behavior. It tries the caddy command to stop it but if that hangs for 2 seconds it sends a SIGTERM, and then a SIGKilL at 5 seconds after which it manually kills caddy and waits for the PID to exit
> sends a SIGTERM, and then a SIGKILL at 5 seconds
to clarify:
to the "caddy stop" process, not the running service
also note that /usr/bin/timeout can't reuse your caddy_execute() function so that's why I had to duplicate the "/usr/bin/su ..." Given comment #2, is it worth doing two SIGTERMs instead of a SIGTERM + SIGKILL? I can't replicate this locally with just turning the admin API off, so I need to lean on you for testing things out. It quits right away for me with `admin off` so something else is going on for you. I'm also looping in scf, the author of the rc.d/caddy script. I'm not clear whether we are fixing a problem or papering over it here. Created attachment 247686 [details]
Fix caddy rc script prestop function
Here is a patch to fix stopping caddy without the admin interface being available. I have been running with this for quite awhile and forgot about it. Basically, there are two possible error messages that show that the admin interface is not available. This checks for both.
(In reply to Sean Farley from comment #10) To be clear, this just fixes shutdowns, right? It doesn't fix reload, reloadssl, configtest, etc.? Either way, please commit your patch, Sean. Would the same approach also work for fixing the `reload` command when the admin API is off? (In reply to Adam Weinberger from comment #11) This fixes the stop and restart commands. configtest already works for me. Sadly, you are correct about the reload commands; they are not fixed by my patch since caddy no longer has signal handlers that would allow a way for a non-admin method to reload the configuration. I doubt they will be returning even if I submitted a patch upstream. I have no commit bits, so I approve you for committing it. :) A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=2d925f150358bf061d4fec9bb8caa009e0a56442 commit 2d925f150358bf061d4fec9bb8caa009e0a56442 Author: Adam Weinberger <adamw@FreeBSD.org> AuthorDate: 2024-01-17 11:41:24 +0000 Commit: Adam Weinberger <adamw@FreeBSD.org> CommitDate: 2024-01-17 11:49:24 +0000 www/caddy{,-custom}: Fix start/stop with admin API disabled Caddy largely relies on the admin API for state control, like start/stop/reload. However, the admin API endpoints are inherently a security risk. Although the admin API is enabled by default, many users may choose to disable it. However, the rc(8) script then needs an alternative approach to controlling the daemon. We already sortof supported signal-based control, but it didn't always work, because there are multiple error messages that indicate that the admin API is disabled (and none of them actually say that in a clear way). This commit fixes start and stop with the admin API disabled. The reload command (and reloadssl) still require the admin API to be enabled and will fail if the admin API is disabled. PR: 255106 Submitted by: scf www/caddy-custom/Makefile | 2 +- www/caddy-custom/files/caddy.in | 4 +++- www/caddy/Makefile | 1 + www/caddy/files/caddy.in | 4 +++- 4 files changed, 8 insertions(+), 3 deletions(-) I committed your patch, Sean, but I couldn't fix the real problem, which is you not having a commit bit. Thank you for identifying and solving this! Mark and Thijs, hopefully this fixes the underlying issue in a satisfactory way. I'm going to close out this ticket, but please reach out if the behaviour is still not right. |
