Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

httpcaddyfile: Deprecate paths in site addresses; use zap everywhere #4728

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

francislavoie
Copy link
Member

In the Caddyfile, site addresses have historically supported paths. This was essentially a carry-over from Caddy v1. The idea is that you could write your sites with a path which would get turned into a subroute with that path as a matcher, and sites with the same domain would get grouped up together:

example.com/first* {
	respond "first"
}

example.com/second* {
	respond "second"
}

example.com {
	respond "anything else"
}

This is kinda fun, it can read nicely if your config is relatively simple. But, keeping this feature has many problems, both from a config UX standpoint and from a code complexity standpoint:

  • Since the site address looks like a proper URL, users may be tempted to write their site address like https://example.com/ which is how the URL might look in the browser.

    The problem though, is that path matchers in Caddy v2 are exact, so this would only match requests to exactly / and nothing else; it would need to be configured as https://example.com/* to match all paths, but that's not really a URL. In Caddy v1, this wasn't a problem because a matcher of / would match all paths since it was always a prefix match (prefix matching has ambiguity though, so we changed to exact matching in v2).

    This has been a common mistake that has caused confusion for users, and we've gotten plenty of questions in the forums relating to this problem.

  • Some directives such as tls and log are not HTTP handlers, but actually special directives that configure some settings for the server according to the domain in the site.

    These directives don't make sense to be scoped by a URL path, because they can only operate on the domain; tls configures connection policies and cert automation policies for the domain(s) in the site address; log enables access logging based on domain (this technically could be extended to match on the path as well, but the effort isn't worth it, there's better ideas to extend this, such as New handler to skip access logs based on request matcher #4689)

    If two site blocks are defined with the same domain and different paths, then which of the tls and log directives should we use? This causes ambiguity, and isn't obvious how it'll be have to users configuring it. Right now, we just sort the sites based on the length of their path matchers, and I think the least specific matcher's config will win in that case (I haven't verified this, but either way, it's bad behaviour).

  • The Caddyfile adapter has to include a bunch of extra code to deal with sorting site blocks based on their path matchers, creating new subroutes matching by the path, merging all of these together in one single set of routes, etc. Some of this code is shared with the logic for merging sites with different domains (reasonable), but it should be possible to significantly simplify certain loops to avoid this extra work, once we remove path support.

The preferred way to write your config if you need to have separate routes based on the request path, is to use handle blocks inside of a single site block. The above Caddyfile example would be better written like this:

example.com {
	handle /first* {
		respond "first"
	}

	handle /second* {
		respond "second"
	}

	handle {
		respond "anything else"
	}
}

This is slightly longer, and has an extra level of indentation, but the behaviour is much clearer, and the domain doesn't need to be repeated for each site block.


Also in this branch, I adjusted a bunch of places where we used fmt.Printf or log.Printf to instead use our zap logger; looks nicer in the terminal, and improves logging consistency in the codebase.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Apr 24, 2022
@francislavoie francislavoie added this to the v2.5.0 milestone Apr 24, 2022
@francislavoie francislavoie requested a review from mholt April 24, 2022 21:24
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Francis, the zap logger is a nice little enhancement.

As discussed in Slack, it's probably a good idea to deprecate path matchers in the site address for the reasons you mentioned here. It is definitely nice sometimes, but I can see how it gets confusing with tls directives and similar.

We're about to release 2.5, but this change is very simple and minimal and non-invasive, does not affect any critical functionality as far as I can tell, and is solely informational. Probably best to deprecate this before the release so we can remove it later.

@mholt mholt merged commit 3a1e0db into master Apr 25, 2022
@mholt mholt deleted the deprecate-site-paths branch April 25, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants