Repository: caddyserver/caddy (language: go)

Issue:
Configurable autosave behaviour from Caddyfile
## 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](https://github.com/caddyserver/caddy/blob/6bad878a22e048762262d6fabe2144cefaf4ca81/caddy.go#L350) 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.


Discussion / hints:
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.
> What are the log messages you are seeing?

This one here, if I remember correctly: https://github.com/caddyserver/caddy/blob/ee7c92ec9b57c671c9091ff993b1a24251020c25/caddy.go#L367-L369
Right, but what's the error?
The error was:

```
"msg":"unable to create folder for config autosave"..."error":"mkdir /var/lib/caddy/.config: read-only file system"
```
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.
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)
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.
> 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.
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!
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 https://github.com/caddyserver/caddy/blob/762b02789ac0ef79e92de7c58dac19d24a104587/caddyconfig/httpcaddyfile/options.go#L48 and https://github.com/caddyserver/caddy/blob/762b02789ac0ef79e92de7c58dac19d24a104587/caddyconfig/httpcaddyfile/httptype.go#L286 which wire this all up for the `admin` global option.
Would `admin` be the correct configuration section for this?
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.
@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.
https://github.com/caddyserver/caddy/blob/e9d95ab29f6cbf24fd3f7f7fd439ab31e2797f93/admin.go#L80
https://github.com/caddyserver/caddy/blob/e9d95ab29f6cbf24fd3f7f7fd439ab31e2797f93/admin.go#L103-L107

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.
https://github.com/caddyserver/caddy/blob/e9d95ab29f6cbf24fd3f7f7fd439ab31e2797f93/caddyconfig/httpcaddyfile/options.go#L311-L320

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.
> 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`](https://caddyserver.com/docs/caddyfile/options#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.

Fix patch:
diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index 43d756f60cf..50e98ac60c9 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -286,6 +286,17 @@ func (st ServerType) Setup(inputServerBlocks []caddyfile.ServerBlock,
 	if adminConfig, ok := options["admin"].(*caddy.AdminConfig); ok && adminConfig != nil {
 		cfg.Admin = adminConfig
 	}
+
+	if pc, ok := options["persist_config"].(string); ok && pc == "off" {
+		if cfg.Admin == nil {
+			cfg.Admin = new(caddy.AdminConfig)
+		}
+		if cfg.Admin.Config == nil {
+			cfg.Admin.Config = new(caddy.ConfigSettings)
+		}
+		cfg.Admin.Config.Persist = new(bool)
+	}
+
 	if len(customLogs) > 0 {
 		if cfg.Logging == nil {
 			cfg.Logging = &caddy.Logging{
diff --git a/caddyconfig/httpcaddyfile/options.go b/caddyconfig/httpcaddyfile/options.go
index 1775c93b6b5..4e5212bf1e8 100644
--- a/caddyconfig/httpcaddyfile/options.go
+++ b/caddyconfig/httpcaddyfile/options.go
@@ -54,6 +54,7 @@ func init() {
 	RegisterGlobalOption("ocsp_stapling", parseOCSPStaplingOptions)
 	RegisterGlobalOption("log", parseLogOptions)
 	RegisterGlobalOption("preferred_chains", parseOptPreferredChains)
+	RegisterGlobalOption("persist_config", parseOptPersistConfig)
 }
 
 func parseOptTrue(d *caddyfile.Dispenser, _ any) (any, error) { return true, nil }
@@ -386,6 +387,21 @@ func parseOptOnDemand(d *caddyfile.Dispenser, _ any) (any, error) {
 	return ond, nil
 }
 
+func parseOptPersistConfig(d *caddyfile.Dispenser, _ any) (any, error) {
+	d.Next() // consume parameter name
+	if !d.Next() {
+		return "", d.ArgErr()
+	}
+	val := d.Val()
+	if d.Next() {
+		return "", d.ArgErr()
+	}
+	if val != "off" {
+		return "", d.Errf("persist_config must be 'off'")
+	}
+	return val, nil
+}
+
 func parseOptAutoHTTPS(d *caddyfile.Dispenser, _ any) (any, error) {
 	d.Next() // consume parameter name
 	if !d.Next() {


Test patch:
diff --git a/caddytest/integration/caddyfile_adapt/global_options_admin_with_persist_config_off.txt b/caddytest/integration/caddyfile_adapt/global_options_admin_with_persist_config_off.txt
new file mode 100644
index 00000000000..998fe2234bf
--- /dev/null
+++ b/caddytest/integration/caddyfile_adapt/global_options_admin_with_persist_config_off.txt
@@ -0,0 +1,36 @@
+{
+	http_port 8080
+	persist_config off
+	admin {
+		origins localhost:2019 [::1]:2019 127.0.0.1:2019 192.168.10.128
+	}
+}
+
+:80
+----------
+{
+	"admin": {
+		"listen": "localhost:2019",
+		"origins": [
+			"localhost:2019",
+			"[::1]:2019",
+			"127.0.0.1:2019",
+			"192.168.10.128"
+		],
+		"config": {
+			"persist": false
+		}
+	},
+	"apps": {
+		"http": {
+			"http_port": 8080,
+			"servers": {
+				"srv0": {
+					"listen": [
+						":80"
+					]
+				}
+			}
+		}
+	}
+}
diff --git a/caddytest/integration/caddyfile_adapt/global_options_persist_config.txt b/caddytest/integration/caddyfile_adapt/global_options_persist_config.txt
new file mode 100644
index 00000000000..c905b476eec
--- /dev/null
+++ b/caddytest/integration/caddyfile_adapt/global_options_persist_config.txt
@@ -0,0 +1,25 @@
+{
+	persist_config off
+}
+
+:8881 {
+}
+----------
+{
+	"admin": {
+		"config": {
+			"persist": false
+		}
+	},
+	"apps": {
+		"http": {
+			"servers": {
+				"srv0": {
+					"listen": [
+						":8881"
+					]
+				}
+			}
+		}
+	}
+}