Configurable autosave behaviour from Caddyfile

[jayaddison]

What would you like to have changed?

A Caddyfile configuration option to enable/disable autosaving the server's configuration state could be useful to reduce some log noise.

Why is this feature a useful, necessary, and/or important addition to this project?

This is low priority -- it's not hugely necessary or important -- but I noticed some log messages relating to caddy attempting to autosave configuration to disk, and from there noticed that the relevant allowPersist variable is hardcoded in v2.6.2.

For sites where configuration is static and/or configuration writes are not expected/possible, it could make sense to allow that variable to be configurable.

What alternatives are there, or what are you doing in the meantime to work around the lack of this feature?

Failure to write the autosaved configuration is not a fatal error, so the service continues to run.

[mholt]

Thanks for opening an issue!

but I noticed some log messages relating to caddy attempting to autosave configuration to disk,

What are the log messages you are seeing?

Failing to autosave the config will result in errors in the logs but nothing that should stop the server.

In general, we advise that users simply ignore unneeded log messages, rather than trying to control everything.

[jayaddison]

What are the log messages you are seeing?

This one here, if I remember correctly:

caddy/caddy.go

Lines 367 to 369 in ee7c92e

	Log().Error("unable to autosave config",
	zap.String("file", ConfigAutosavePath),
	zap.Error(err))

[mholt]

Right, but what's the error?

[jayaddison]

The error was:

"msg":"unable to create folder for config autosave"..."error":"mkdir /var/lib/caddy/.config: read-only file system"

[mholt]

Thanks. So, that error is not fatal, actually. We only log it, we don't stop the server because of it.

What are your full logs from process start to end? I'd like to know why the server is stopping.

[jayaddison]

Thanks for the quick reply - yep, the server does start despite the error, so it's not of critical importance. The main impact would be for any downstream systems that alert on error-level log messages.

(in other words: the rest of the logs all look fairly standard - the error stood out as a bit noisy, though)

[mholt]

Oh, ok, I guess I took this literally:

Failure to write the autosaved configuration is not a fatal error, so the service continues to run.

I disagree with that error being "noisy" though -- 99% of the time it's a legitimate problem, because people have misconfigured their permissions or file systems, which leads to further problems with cert storing, etc.

[jayaddison]

I disagree with that error being "noisy" though

Yep, a totally fair perspective - and in many cases it probably does indicate a configuration issue to be addressed.

If a significant number of read-only containers run with caddy in future though, then the noise sum would increase, making for unnecessary log output that could require after-the-fact filtering in various log-handling environments.

I feel like the path that would allow the minimal CPU, storage, filtering rules, puzzled developers/admins, and so on could be to allow configuration of that variable.

[mholt]

To clarify, it is currently configurable, just not via Caddyfile. Shouldn't be too hard to add if someone would like to submit a proposal; I'll leave this open until then, as it's not really a priority for me right now. Thanks!

[francislavoie]

This should be very easy to set up as a global option. Thinking persist_config off. It would just set admin > config > persist to false.

Just need to follow the lead of other global options. Take a look at

caddy/caddyconfig/httpcaddyfile/options.go

Line 48 in 762b027

RegisterGlobalOption("admin", parseOptAdmin)

and

caddy/caddyconfig/httpcaddyfile/httptype.go

Line 286 in 762b027

if adminConfig, ok := options["admin"].(*caddy.AdminConfig); ok && adminConfig != nil {

which wire this all up for the admin global option.

[jayaddison]

Would admin be the correct configuration section for this?

[francislavoie]

I don't think it needs to be within admin, it can go beside it. You might not necessarily want to configure admin at the same time as turning off persisting the config.

[u5surf]

@francislavoie @jayaddison

Hi, I would like to work on this issue.
I consider that it should specified an attribute into admin directive because the present Admin.Config has the Persist attribute. Moreover it is more intuitive than we make a new global options.

caddy/admin.go

Line 80 in e9d95ab

Config *ConfigSettings `json:"config,omitempty"`

caddy/admin.go

Lines 103 to 107 in e9d95ab

	type ConfigSettings struct {
	// Whether to keep a copy of the active config on disk. Default is true.
	// Note that "pulled" dynamic configs (using the neighboring "load" module)
	// are not persisted; only configs that are pushed to Caddy get persisted.
	Persist *bool `json:"persist,omitempty"`

Thus for example it is suitable something like disable_persist_config which is sub-attribute of the admin directive.

admin 127.0.0.1:2018 {
   disable_persist_config
}

It is quite simple to implement because it can only add the case into switch of parseOptAdmin.

caddy/caddyconfig/httpcaddyfile/options.go

Lines 311 to 320 in e9d95ab

	switch d.Val() {
	case "enforce_origin":
	adminCfg.EnforceOrigin = true
	case "origins":
	adminCfg.Origins = d.RemainingArgs()
	default:
	return nil, d.Errf("unrecognized parameter '%s'", d.Val())
	}

If there's one concerned about this, it is the other attributes of Admin.Config like LoadRaw and LoadDelay. We are not able to specified explicitly these. But we seems that they are experimental, cannot specify with Caddyfile at present.

[francislavoie]

I consider that it should specified an attribute into admin directive because the present Admin.Config has the Persist attribute.

The Caddyfile doesn't necessarily need to be a 1 to 1 match to JSON.

I don't think there's overlap in configuring an admin endpoint and wanting to disable persisting config. Those are pretty much mutually exclusive configurations.

It just happens to be there in the Go struct because it's the admin code which manages the config and writes a copy of the config to file. But that's a technical reason that users don't need to be concerned about.

Thus for example it is suitable something like disable_persist_config

One thing we try to avoid is "negative" config options, i.e. starting its name with "disable".

Instead, I suggest persist_config off. For now, off would be the only argument, but that's fine. It would leave the door open to later add other flags for it or a sub-options block to it if it ever becomes necessary.

Compare with auto_https for example, it started by only having auto_https off, but we later added a couple other modes like disable_redirects and more because there were different subsets of the features that could be turned off in parts.

If there's one concerned about this, it is the other attributes of Admin.Config like LoadRaw and LoadDelay. We are not able to specified explicitly these. But we seems that they are experimental, cannot specify with Caddyfile at present.

That's fine -- those only make sense if you're using JSON config since they're about loading config via the admin API dynamically. It doesn't really make sense to use this as a user of the Caddyfile, since you're probably loading config directly from a file via CLI commands, instead of directly via the API.

That should be split into a separate issue to implement later on if we have a good reason to add it. But right now, I don't think it's necessary.

[jayaddison]

Thank you, @u5surf @francislavoie!