caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
58.12k stars 4.03k forks source link

Feature request: readd reload on USR1 signal #3967

Closed chulkilee closed 3 years ago

chulkilee commented 3 years ago

Caddy < 2 had reload on USR1 signal (#107 / 41c4484222ecd9efc4b5f48c3c66a5e87a2ff532), but it was dropped in v2 (#2242) (see also discussion)

Although there are other ways to reload the config (using --watch option, or making POST /load request) but it would be nice to have the feature back so that ops can determine when to apply the changes without installing http client.

(edit: also caddy reload is there, but it requires passing config again)

francislavoie commented 3 years ago

You're aware you can use the caddy reload command, right?

chulkilee commented 3 years ago

Oh yes (will add it to the list) - but it's still simpler for process managers to send a signal instead of forking a process to run caddy reload.

francislavoie commented 3 years ago

What process manager are you using here? With systemd, we use ExecReload

https://github.com/caddyserver/dist/blob/master/init/caddy.service#L26

mholt commented 3 years ago

Yeah; sorry, I'm not sure what the motivation is for a posix-only signal when we already have a more flexible cross-platform solution for this; one that is the same experience for the user on linux too: systemctl reload caddy

chulkilee commented 3 years ago

I'd like to use make nomad to trigger reload when caddyfile changed via template - using change_signal

I may use consul template directly to use template.command but it requires adding consul-template inside caddy docker image and passing consul secrets down to caddy container, which is not desired.

mholt commented 3 years ago

Can you pass string arguments that way? Because I'm not sure how that's going to work otherwise. Signals aren't powerful enough. Where will Caddy get the config? Without a --config argument, Caddy will have to assume the config is at ./Caddyfile, but that's not often the case except with interactive use.

mr-karan commented 3 years ago

+1 for Nomad usecase. If there's a signal which can be defined to do exactly what caddy reload does, I guess that should solve the problem?

mholt commented 3 years ago

@mr-karan But with what arguments/flags? That's the question. AFAIK you can't pass arguments with signals. caddy reload without any args will try to load a Caddyfile in the current working directory, but that's seldom useful in production... I think.

mr-karan commented 3 years ago

@mholt Not sure if this makes sense but if we hold the config location in memory, we can just reload the existing config with which caddy was running? That's the ideal behaviour of reload IMHO, to basically re-read from the same config file with which it was started.

Prior Art: nginx -s reload and Prometheus Reload work the same way if I'm not wrong.

mholt commented 3 years ago

@mr-karan That doesn't really make sense to me. :)

if we hold the config location in memory, we can just reload the existing config with which caddy was running?

We can, but reloading the exact same config with no changes has limited utility (like, triggering a manual reload of TLS certs from disk).

That's the ideal behaviour of reload IMHO, to basically re-read from the same config file with which it was started.

This is different, now you want to re-read a file. This is typically the purpose of a config reload, but which file? So does it reload the config in memory or read it from disk?

mr-karan commented 3 years ago

but reloading the exact same config with

We're not reloading the exact config. The config contents changed, the config path didn't change.

This is typically the purpose of a config reload, but which file?

same file with which the caddy server was started.

So does it reload the config in memory or read it from disk?

Config is stored on disk. We can just store the config file-path in memory (like /etc/caddy/Caddyfile) and re-read the contents from disk.

Well this is a typical behavior of hot reloading any process.. please correct if I'm wrong :)

mholt commented 3 years ago

We can just store the config file-path in memory (like /etc/caddy/Caddyfile) and re-read the contents from disk.

I guess with most servers that is easy, but with Caddy I'm not sure of a clean way to do it yet. Any way I can think of is pretty hacky. Caddy's config doesn't come from files, it comes through the API. Only the command line interface has any knowledge of files, and only at initial startup. That's why the caddy reload command doesn't attempt to "remember" any config files from before, because it's not really safe to assume that the current config running came from a file.

mr-karan commented 3 years ago

because it's not really safe to assume that the current config running came from a file.

Ah you're right. I totally missed this :thinking: .

mholt commented 3 years ago

Yeah, so that's the main problem: "reload" -- reload what? Do you intend to reload a file even though the config may have changed through the API? (By the way, even configs from files get loaded via the API, the CLI just wraps that up for you.) Or do you intend to reload the currently-running config that was POSTed to the API, without changes (because where would you get the changes from)?

Hence the command line arguments; and why, with a signal, it doesn't really make sense since you can't provide args.

chulkilee commented 3 years ago

Oh, if the server process holds only the final config, not the location of config to look up.. then yes, it has limited utility - only "reevaluating" the existing config.

However, I'm wondering we can use import to mimic the config file reload without passing config again?. For example I can use import sites-enabled/* and, update the file, and then make the reload config.

optiz0r commented 3 years ago

For the nomad use case, I've extended the Caddy container with a bash signal handler that triggers a reload using the Caddyfile which was used when the container was first launched. The code is here (https://github.com/optiz0r/caddy-consul) in case it's useful to anyone else.

chulkilee commented 3 years ago

As there is a workaround and adding the default file location isn't trivial, I'm closing this issue. Thanks all!

optiz0r commented 3 years ago

Is it worth adding the sh script workaround to the pre-built docker image, which is hardcoded to use the Caddyfile on startup? (My workaround script puts non-json text to stdout where it gets mixed up with json-formatted output from caddy, so those echos should probably be dropped).

It embeds the latest version of tini (to avoid the shell script having to bear the pid1 responsibilities), handles SIGHUP, and passes on SIGTERM, SIGINT, SIGQUIT (but could very easily pass on other signals too, e.g. USR1/2 etc).

If you're happy to accept the change, I'll raise it as a PR?

thijsvandien commented 3 years ago

From what I understand, the main argument against keeping/reintroducing the USR1 signal is that it's ambiguous which config to load. I'd say a good default would be doing that what a restart would've done, but without interrupting anything.

In any case, with admin off, I believe no such ambiguity exists—there is a designated config file with no dynamic changes. At the same time, there is no way to reload it on demand then. Could we at least have a signal handler for this simpler scenario?

rawb1 commented 2 years ago

@optiz0r your script work really well, thanks 👍 I ended up creating and publishing a docker image using your script : https://github.com/rawb1/caddy-hcp https://hub.docker.com/r/rawb1/caddy-hcp

For Nomad another solution is to use caddy dynamic reverse proxy with consul service SRV record so you don't need to reload caddy config but it requires more configuration to setup Consul DNS forwarding https://learn.hashicorp.com/tutorials/consul/dns-forwarding ...

cycneuramus commented 1 year ago

For the Nomad use case, we can make use of the change_mode parameter of the template block to achieve a clean reload without having to build custom images or manipulate signals:

template {
  data        = file("Caddyfile.tpl")
  destination = "/local/Caddyfile"
  change_mode = "script"
  change_script {
    command = "caddy"
    args    = ["reload", "--config", "/local/Caddyfile", "--adapter", "caddyfile"]
  }
}

<...>

config {
  image = caddy
  entrypoint = ["caddy", "run", "--config", "/local/Caddyfile", "--adapter", "caddyfile"]

<...>
}

(Sorry for commenting on an old issue; I just thought I might drop this hint for future troubleshooters.)

stronny commented 2 months ago

Sorry for messing things up, in my defense I'm feeling a little under the weather. Below is my hot take for what it's worth.

It's generally not a good idea to assume you can imagine every legitimate use case, even if your software is trivial, which caddy is most certainly not. You, the authors, are happy with caddy reload, however others may have objections that they don't even need to elaborate on. Mine lies primarily with admin API, which I have disabled, which caddy supports, which I give credit for. Now, is it reasonable to run a supported configuration without a way to reload it, not restarting the whole process? If you allow for admin API to be disabled, it stands to reason to provide a way to reload the configuration in some other ways. SIGHUP is traditionally this way, used for config reload in daemons from time immemorial, and not really having any downsides that I'm aware of.

I understand you didn't design the main loop to deal with config files, that's totally fair. I would propose there are elegant ways to support dealing with files nonetheless. First thing that comes to mind is to run a separate app inside caddy that exists for this sole purpose: trap HUP and read the file specified in the command line.

Yes, you need the file specified in the command line. Yes, there are already ways to achieve the same result. Yes, this doesn't cover all cases, but neither does any existing way. Yes, this needs work, I will try to throw something together myself.

Now, some specific points:

Signals aren't available on windows

I fail to see how it invalidates using them where they are available.

SIGHUP is the wrong signal

Debatable. If caddy runs in the foreground, it will indeed signal the terminal going away, so caddy should probably exit cleanly and timely, but if it doesn't have a tty, the only way to receive a HUP would be for someone else to send it.

If you don't like HUP, I'm not married to it, it's just what everyone expects.

automatic reload

https://caddyserver.com/docs/command-line#caddy-run says:

--watch will watch the config file and automatically reload it after it changes. ⚠️ This feature is intended for use only in local development environments!

And I agree, it's not ideal for a production server. In the other issue you say it's impossible to have this functionality without admin API enabled, but I'm pretty sure it works as advertised. I will check again later and report back.

mholt commented 2 months ago

@stronny Sorry you're not feeling well, hope you are back to better health soon.

It's generally not a good idea to assume you can imagine every legitimate use case, even if your software is trivial, which caddy is most certainly not. You, the authors, are happy with caddy reload, however others may have objections that they don't even need to elaborate on.

To clarify, we're not trying to assume anything. The decisions are technical -- and complexity-cost based.

Now, is it reasonable to run a supported configuration without a way to reload it, not restarting the whole process? If you allow for admin API to be disabled, it stands to reason to provide a way to reload the configuration in some other ways.

When you say "reload" do you mean the same configuration or a new/changed one?

Caddy does persist the loaded configuration by default (this can be disabled, however), so I suppose it would be possible to reload the same configuration, but this has little-to-no benefit for most Caddy deployments.

If there's any change to the configuration, though, there has to be a way to get that configuration to Caddy, and signals can't do that.

stronny commented 2 months ago

@mholt thank you, it isn't that bad, I'll be alright.

decisions are technical

Sometimes it doesn't seem so, to be perfectly honest.

When you say "reload" do you mean the same configuration or a new/changed one?

Though a valid question, I can't think of a reason to only ever reload the same config. Of course it's a config as defined at the point of reload, not the point of a process start.

Caddy does persist the loaded configuration

I disable that too. I like my daemons integrated into the distro, not declaring independence of the rules everyone else plays by. I also disable log rotation, that's not caddy's job, and it can't do it meaningfully anyway, rotating by size instead of a timer.

If there's any change to the configuration, though, there has to be a way to get that configuration to Caddy, and signals can't do that.

All that a signal has to do is to indicate the necessity to do an action. The action itself has to be well-defined inside the running process -- of course, yes, we all understand that. Are you saying caddy is incapable of reading a file from the disk? Surely not, what you mean is: caddy, as designed and implemented currently, reads its config from a file descriptor, but it doesn't open an actual file by name. This serves to make the procedure simple and generic, but it carries usage implications that sometimes get in the way.

What I'm saying is: given that a server configuration in a file is such a ubiquitous case, it's worth supporting first-class, making a special exception to the generic rule. It shouldn't even be hard to implement, as all the puzzle pieces are there already, all that needs to happen is a slight rearrangement. To be clear, I haven't looked at relevant code yet, I'm speculating at this point.

Here's a general overview of one example to achieve this, in English:

mholt commented 2 months ago

All that a signal has to do is to indicate the necessity to do an action. The action itself has to be well-defined inside the running process

And that action is defined in the configuration which is being changed :upside_down_face:

This is why the new configuration must be given to the server. And that can't be done if the admin interface is disabled, because that is how the new config gets to the server.

Your proposal is a complex side channel for config loading and works only for static files. Bypassing the admin API is probably not a good idea.

stronny commented 2 months ago

@mholt what's complex about reading a file? Please elaborate.

mholt commented 2 months ago

That is obviously not what I said; please read my post again.

stronny commented 2 months ago

Your proposal is a complex side channel for config loading and works only for static files.

What did you say then? Please explain, I guess I'm failing to understand you here.

stronny commented 2 months ago

@mholt I've just tested it again. --watch works with admin disabled.

stronny commented 2 months ago

signal-reload.go.txt

Here's a very simple module that reloads a config on USR1. I didn't test it beyond the basics.

stronny commented 2 months ago

The same, inline:

package xnt_caddy_signal_reload

import (
        "os"
        "os/signal"
        "syscall"
        "github.com/caddyserver/caddy/v2"
        caddycmd "github.com/caddyserver/caddy/v2/cmd"
        "go.uber.org/zap"
)

func init() {
        caddy.RegisterModule(App{})
}

type App struct {
        File    string `json:"file,omitempty"`
        Adapter string `json:"adapter,omitempty"`
        sigchan chan os.Signal
        logger  *zap.Logger
        ctx     caddy.Context
}

func (App) CaddyModule() caddy.ModuleInfo {
        return caddy.ModuleInfo{
                ID:  "sig",
                New: func() caddy.Module { return new(App) },
        }
}

func (a *App) Provision(ctx caddy.Context) error {
        a.ctx = ctx
        a.logger = ctx.Logger()
        return nil
}

func (a *App) trap() error {
        for sig := range a.sigchan {
                if sig == syscall.SIGSTOP {
                        close(a.sigchan)
                        return nil
                }
                a.logger.Debug("trapped a reload signal")

                // get the config from the disk
                config, file, err := caddycmd.LoadConfig(a.File, a.Adapter)
                if err != nil {
                        a.logger.Error("error reading config", zap.String("file", file), zap.Error(err))
                        continue
                }

                // apply the config into the running process
                err = caddy.Load(config, false)
                if err != nil {
                        a.logger.Error("error loading config", zap.Error(err))
                        continue
                }
        }
        return nil
}

func (a *App) Start() error {
        a.logger.Debug("started")
        a.sigchan = make(chan os.Signal, 1)
        signal.Notify(a.sigchan, syscall.SIGUSR1) // ?unsure? HUP may need a tty check
        go a.trap()
        return nil
}

func (a App) Stop() error {
        a.logger.Debug("stopping")
        signal.Stop(a.sigchan)
        a.sigchan <- syscall.SIGSTOP
        return nil
}

// Interface guard
var _ caddy.App = (*App)(nil)
stronny commented 2 months ago

So... any thoughts so far? I can always go ahead and publish a mod, but it really benefits from being in caddycmd, because there it can see the filename and adapter, rather than requiring a separate configuration.

I wouldn't imagine you exposing these variables for mods either.

mholt commented 2 months ago

Sorry, had a lot on my plate (still 2 pages of my notification backlog left -- this is one of them) but I will try to look at it soon