Caddy: Improve Caddyfile parsing to disambiguate when braces are missing

[francislavoie]

Just an issue to track the sub-discussion that was had in https://github.com/mholt/caddy/pull/1973, also relating to https://github.com/mholt/caddy/issues/550.

See https://caddyserver.com/docs/caddyfile#structure

Caddyfiles allow omitting { } when there's only a single label (domain/url identifier to serve on). A problem arises when an import might be used (either by the new glob feature recently merged in, or explicitly in the main Caddyfile). The imported files might not be using { }, so this would cause parsing errors.

The idea is for the fix would be that the parser could artificially wrap the blocks that don't have { } with those, to avoid the parsing problems.

The tricky bit is figuring out when the label definitions end, because it's allowed for them to be multi-line. For multi-line, each line must end with a , (some whitespace following it is allowed) for the label parsing to continue. While label parsing, we should detect the first newline without a , and end parsing the label there - which also means we can insert a { at that time, remember that we did so, and continue parsing. When we reach the end of the file (when importing), then we can add a } if we added a { earlier.

I think that covers everything. @mholt ?

[mholt]

Yep, basically! Not really a bug though. :) Just a discussion point and possible enhancement for convenience in the future. Thanks for bringing it up!

[francislavoie]

Right - I was thinking that we could consider the parsing errors in those cases errors, but it could also be a feature request too... I guess. Wasn't sure so I just slapped bug on it :man_shrugging:

[whitestrake]

I'm thinking we'd need to look out for instances where the imported tokens either:

• Don't form a full site definition (i.e. partial configuration, to be imported within a site block), or;
• Are already properly braced, or;
• Consist of multiple properly-braced sites.

If we can check for the presence of/requirement for a site label list, and then check whether that list already ends with an opening brace, we could rule out those cases and move on to adding the required braces.

I have no idea how we could determine whether the imported file even needs a label list or not, though - this could be totally ambiguous... If we know there must be one, @francislavoie's solution for finding the end of the list is good, but if we are unsure, I guess we could check for starting tokens that aren't directives and otherwise conform to valid site labels?

[tobya]

I would be in favour of changing the caddyfile structure to require {} always! I know this is a philisophical point for Matt but I think from a simplicity point of view requiring curly braces makes things much much simpler and really does not add any complexity to the caddyfile syntax.

If we were to do this then we would have to do it befor 1.0

[francislavoie]

@tobya one reason to keep that is for the CLI short caddyfile, I think. https://caddyserver.com/docs/cli#short-caddyfile

[mholt]

It's just SO nice to be able to write a (simple) Caddyfile without necessarily needing punctuation or syntax marks like a programmer is used to, for the times that you have just a single site you need to get running, especially if it's your first time. You're right, it'll be simpler to implement if you always use curly braces. For now, as a matter of "good practice" if you operate in scaling or advanced scenarios often, I just recommend putting curly braces around site definitions.

I agree though if we want to require curly braces (* cringe *) we should do it sooner rather than later. I'm not sure I wanna do that though.

[whitestrake]

If we were to decide that site blocks always need braces:

1. We'd have far fewer instances of this kind of bracing ambiguity anyway, because it's unlikely for the imported file to be a flat Caddyfile if a flat Caddyfile wouldn't work on its own
2. We could determine rather easily whether the imported file is an actual site definition requiring bracing, or if it's just partial configuration, based on context - keeping track of whether the import statement itself was inside a braced block or at the top level
3. We might end up with a lot of angry users with broken Caddyfiles

FWIW I always brace my site definitions, even if there's only one. I don't think that removing support for flat Caddyfiles is warranted, though.

[tobya]

We could specify that site definitions must have braces if your use the include directive. That gives us best of both worlds. A short simple caddyfile is still allowable but when you get complicated and start using import you have to use braces

[mholt]

^ I think I could get on board with that idea.

[whitestrake]

I guess we can remove the necessity for disambiguation entirely by requiring users to be unambiguous with advanced setups at the outset?

Wouldn't be required where a user imports partial configuration into a single-site flat Caddyfile... But I can't imagine that's common enough that the inconvenience of needing to brace that configuration comes anywhere near the reduced complexity of implementing brace requirements rather than disambiguation.

[tobya]

So the decision here seems to be (in case someone out there fancies submitting a PR!)

• if a caddyfile includes the •import• keyword then ALL host declarations require curly braces

[francislavoie]

This was a good discussion, but ultimately I don't think there's actually anything actionable here for Caddy v2. This issue hasn't really come into play since we discussed it, no real user complaints happened.