Reject configuration when placeholders are missing

[pascalgn]

1. What would you like to have changed?

When placeholders are used in the configuration, for example {env.NO_SUCH_VAR}, caddy should fail during startup, to indicate that something is wrong.

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

IMO it's better usability to fail early, because it allows users to detect the root cause of the problem more easily.

Having an error like 2019/10/14 17:02:45 [ERROR][{env.DOMAIN_NAME}] failed to obtain certificate: acme: error. is a bit harder to understand than (for example) ERROR: Placeholders not found in configuration value: {env.DOMAIN_NAME}

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

The alternative is to just leave it as is and accept the slightly worse user experience.

4. Please link to any relevant issues, pull requests, or other discussions.

Extra: Implementation details

For example, here we do some replacement:

caddy/modules/caddyhttp/caddyhttp.go

Lines 297 to 305 in acf7dea

	if hm, ok := m.(*MatchHost); ok {
	for _, d := range *hm {
	d = repl.ReplaceAll(d, "")
	if certmagic.HostQualifies(d) &&
	!srv.AutoHTTPS.Skipped(d, srv.AutoHTTPS.Skip) {
	domainSet[d] = struct{}{}
	}
	}
	}

The repl.ReplaceAll method only returns string, so we cannot know if the replacement was successful. An easy solution would be to introduce another method, like ReplaceAllOrFail, but there might be better ways...

[mholt]

I like the idea of failing early, but there's a few complications with this.

Placeholders aren't evaluated until they are used. In the example you gave, that replacement happens when the server is starting up, but in other cases a replacement doesn't happen until an HTTP request. Would we treat the whole HTTP request as an error then?

I agree though that it would be useful to make a ReplaceAll method that could return an error value if a placeholder is not recognized.

Another question: do we treat an empty placeholder as an error, or make that an option as well? i.e. what is the difference between an env variable being set and being empty? Empty environment variables are indistinguishable from unset/nil ones, but other placeholders (like properties of the HTTP request) can distinguish empty from unset/nil.

Do you think this kind of check would be better handled by a linter?

[mholt]

Related to #2313

[mholt]

@pascalgn Alrighty, I had to update the replacers to make logging work in #2831 so you can find these enhancements in 755d442.

• ReplaceAll() now replaces all placeholders, even if unrecognized.
• ReplaceKnown() preserves the previous behavior of ReplaceAll(), which is that only recognized (known) placeholders are replaced.
• ReplaceOrErr() optionally returns an error if a placeholder is unrecognized or an evaluated value is empty, or both.

In that PR I've also updated the examples you mentioned, of placeholders in automatic HTTPS and in listener addresses, so those yield errors now.

It's a bit tricky though. The listener address one was pretty straightforward: if a placeholder isn't recognized or is empty, that should be an error.

The host matcher one is more nuanced, because those are meant to be evaluated at HTTP-request-time, not at server startup like we're doing for automatic HTTPS. So the placeholder could be one that exists only during an HTTP request. We don't want to error completely for that at startup, but we should error if the placeholder is recognized and still empty. While I don't love a signature like repl.ReplaceOrErr(input, true, false), it works.

Env variables make things even tricker because I don't think there's a super-efficient way in Go to know whether an env variable is set: we can only know whether it is empty (and unset == empty, although maybe by iterating all known env variables we could determine the difference, but that doesn't seem efficient).

Anyway, I hope that is sufficient. Will close this now. Let me know how it works for you!

[pascalgn]

I'm having a look at #2831 and it's looking quite good!

Regarding the ReplaceOrErr and also the question about environment variables, I thought about it a bit, but I still cannot think of a use case where nonexistent/unknown (nil) should be treated different from empty (""), so I think we can simplify that method a little bit.

To also support use cases where a variable can be empty, we will need a way to specify a default value for missing parameters, though, as already requested in #2313 (I would also support the suggested syntax from bash, {env.TEST:-default}). Then, the previous behaviour (unknown variables will be expanded to "") can be easily restored by e.g. using {env.CAN_BE_EMPTY:-} (default value is "", also adds a nice smiley :-} to the config).

Technically, for both environment variables and e.g. HTTP headers we could treat nonexistent and empty differently, but I think then we would also need to provide a different default-value-syntax and IMO it would make things unnecessarily complicated. So I think treating empty and missing the same should be fine.

[mholt]

I still cannot think of a use case where nonexistent/unknown (nil) should be treated different from empty ("")

Ah, I'll give you one:

{
    "handler": "templates"
},
{
    "handler": "static_response",
    "body": "Hello! It is the year {{now | date \"2006\"}}."
}

Actually two -- here's another:

"json": "{\"field\": 0}

In the first case, we have a template string that is evaluated by the templates handler above it, but static response bodies also support placeholders. Thus, {{now}} (which actually, I believe would be parsed by the replacer as {{now} -- maybe I could improve on that) is an unrecognized placeholder, because it is not a placeholder, so it needs to be preserved.

Similarly for JSON: {"field": 0} is not a recognized placeholder, so we should preserve it.

These only happen a few places in the current code base, I think: pretty much everywhere else doesn't expect template- or JSON-style values, so it is rare that it happens, but it happens.

To also support use cases where a variable can be empty, we will need a way to specify a default value for missing parameters, though, as already requested in #2313 (I would also support the suggested syntax from bash, {env.TEST:-default}). Then, the previous behaviour (unknown variables will be expanded to "") can be easily restored by e.g. using {env.CAN_BE_EMPTY:-} (default value is "", also adds a nice smiley :-} to the config)

Aha 😄 I do like smileys. We can move the discussion about default placeholder values to that issue. If we expand the functionality of placeholders, I want to be careful how we do it. (There's also some talk of giving placeholders the ability to be wrapped in a simple function, like for formatting time, etc.)

[pascalgn]

Agree, I will continue the discussion about default values in #2313!

However, regarding your examples, I think we need another solution. Usually this should be solved by escaping, I think (although it would increase the replacer complexity).

So IMO it would be hard to explain to users that the following works (system.hostnames is unknown, so would be kept)

{
    "handler": "static_response",
    "body": "To get the hostnames, run 'echo ${system.hostnames}'"
}

But the following will not work correctly (system.hostname is a valid replacement and would be replaced)

{
    "handler": "static_response",
    "body": "To get the hostname, run 'echo ${system.hostname}'"
}

(Especially considering that even the first example could break in the future, when for some reason a new replacement system.hostnames would be introduced)

I think what we really need is some kind of escaping, like

{
    "handler": "static_response",
    "body": "To get the hostname, run 'echo $\\{system.hostname\\}'"
}

[mholt]

Yeah I also considered that, but I just hate how tedious escaping is...

You have a good point though, the inconsistency here isn't great. It's something we should figure out before the stable release.

[pascalgn]

I had some thoughts on this and I think escaping isn't the best solution right now.

The suggestion from some days ago was escaping, like "body": "To get the hostname, run 'echo $\\{system.hostname\\}'"

However, consider for example the following config:

{
    "handler": "static_response",
    "body": "You can find the files in Z:\Shared\Some_Folder"
}

As we use \ as escape character, that config would need to be changed to "body": "You can find the files in Z:\\Shared\\Some_Folder", causing more effort for users who don't even use placeholders.

What I thought now is to maybe make the beginning and end of placeholders configurable, so users which need to use {} somewhere, could have a config like

"placeholder_open": "<<",
"placeholder_close": ">>"

This has some advantages, some disadvantages, but I think we could consider it as an alternative to escaping...

[mholt]

Going to close this since we have addressed the idea of replacers returning errors; other things are for different issues I think.

[mholt]

@pascalgn I think #1793 is relevant at this point. Maybe I will reopen that one.