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

reverseproxy: Improve error message when using scheme+placeholder #3393

Merged
merged 2 commits into from May 26, 2020

Conversation

francislavoie
Copy link
Member

See the discussion here: https://caddy.community/t/proxy-upstream-placeholders/8085

So as it turns out, if you try to use a placeholder as a proxy address, it works fine, unless you also specify a scheme (i.e. the address containing ://), which uses a different code path.

For example with this Caddyfile:

:8080
reverse_proxy http://{host}:8081

The error message when adapting looks like this:

adapt: parsing caddyfile tokens for 'reverse_proxy': Caddyfile:2 - Error during parsing: parsing upstream URL: parse "http://{http.request.host}:8081": invalid character "{" in host name

This doesn't really explain why it doesn't work. This PR adds a condition ahead of the call to url.Parse (which triggers that error) which gives a more useful message:

adapt: parsing caddyfile tokens for 'reverse_proxy': Caddyfile:2 - Error during parsing: for now, placeholders are not allowed when specifying a scheme; instead, omit the scheme and use the transport subdirective if you need tls support

Essentially this just means "omit http:// and it'll work, or omit https:// and use transport { tls } if you need HTTPS".

Also as a note, currently it's impossible to use a SRV address from the Caddyfile and also use placeholders. We could add a srv subdirective that basically skips the parsing checks. Currently, we must parse the scheme to allow SRV because it's set as a different field in the JSON than non-SRV addresses (i.e. dial for non-SRV, and lookup_srv for SRV addresses). With a separate subdirective we could skip the parse step to allow placeholders to pass though... but the use-case for that is so thin that I don't think it's worth the bother for now unless someone specifically shows a valid use-case for it.

@francislavoie francislavoie added this to the 2.1 milestone May 11, 2020
@francislavoie francislavoie requested a review from mholt May 11, 2020 05:47
@mholt
Copy link
Member

mholt commented May 11, 2020

Thanks for this. Hmm, given the limited scope of URLs we expect to see here, I wonder if we could and should just write our own simple URL parser to split scheme, host, port (and path if any), which allows placeholders... of course, I guess we'd have to assume that a placeholder would never comprise more than one piece of the URL.

@francislavoie
Copy link
Member Author

Yeah I thought about it but that doesn't seem worth it considering it's completely possible to work around it by just not specifying the scheme.

@mholt mholt added the under review 🧐 Review is pending before merging label May 11, 2020
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.

The whole situation is a bit unfortunate, but as you note, it's easy enough to work around. I don't know a better solution, so I guess this is fine. I just don't want to get too speculative in the error message as to what the user's intention was.

modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
@francislavoie francislavoie requested a review from mholt May 23, 2020 02:53
@mholt mholt removed the under review 🧐 Review is pending before merging label May 26, 2020
@mholt mholt merged commit c1e5c09 into caddyserver:master May 26, 2020
@francislavoie francislavoie deleted the proxy-placeholder-errormsg branch May 26, 2020 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants