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

caddyhttp: Add support for triggering errors from try_files #4346

Merged
merged 3 commits into from
Sep 17, 2021

Conversation

francislavoie
Copy link
Member

See #4344, this is a first step towards completing that issue.

Nginx supports a couple pieces of functionality in its implementation of try_files that allow handling the request either as an HTTP error, or by routing to a named location block. http://nginx.org/en/docs/http/ngx_http_core_module.html#try_files

For Caddy, I think it would be quite beneficial to add support to try_files for triggering an error as a fallback case, to short-circuit the request handling early. In many cases, it's not simply the case of "try_files matches, or doesn't".

The one potentially contentious bit here, implementation-wise, is the usage of a placeholder http.matchers.error as a way to carry the error from the matcher logic up to the HTTP handler chain logic. I'm hoping that this would have a minimal performance impact, but I'm not totally certain, haven't benchmarked. It's also I guess kinda cross-cutting concerns. But I don't really see a cleaner way to do this. We can't use r.Context() directly, because that's immutable at this point, so the replacer, being mutable, is the mechanism that would let us do it.

Anyways, an example of how this would look:

:8080 {
	root * /srv
	try_files {path} {path}/index.php =404
	php_fastcgi localhost:9000
	file_server

	handle_errors {
		respond "Error!"
	}
}

Given the files: /srv/foo.js, /srv/index.php, /srv/bar.php

$ curl localhost:8080/foo.js
foo.js contents

$ curl localhost:8080/
index.php executed

$ curl localhost:8080/index.php
index.php executed

$ curl localhost:8080/bar.php
bar.php executed

$ curl localhost:8080/foo.php
Error!

Whereas without this, curl localhost:8080/foo.php would be sent through fastcgi when the file is already known not to exist.

@francislavoie
Copy link
Member Author

While reading through the code, I was reminded that GetVar(ctx, key) and SetVar(ctx, key, val) exist, which seems more appropriate for this than using the replacer. Made the change.

@ttys3
Copy link
Contributor

ttys3 commented Sep 13, 2021

The first example in the description has a typo,

is it try_files {path} {path}/index.php =404 should be put in a block? (like #4347 (comment)) :

:8080 {
	root * /srv
	php_fastcgi localhost:9000 {
       try_files {path} {path}/index.php =404
    }
	file_server

	handle_errors {
		respond "Error!"
	}
}

@francislavoie
Copy link
Member Author

francislavoie commented Sep 13, 2021

No, I did that on purpose because this PR doesn't add try_files support to php_fastcgi. That's the job of the other PR.

They're subtly different. Using try_files outside doesn't modify php_fastcgi, but instead just does a try_files redirect before php_fastcgi gets a chance to run. Somewhat similar effect, but has some minor edgecase differences.

@ttys3
Copy link
Contributor

ttys3 commented Sep 13, 2021

Got it. I like the implementation. This make try_files has more power ( and the php_fastcgi also benefit from it).

try_files, if matched, rewrite it, otherwise trigger an error with code after the = char

Am I correct in this way?

@francislavoie
Copy link
Member Author

francislavoie commented Sep 13, 2021

Yeah, pretty much. It will rewrite to the first argument that matches a file that exists on disk, or immediately emit an error if it reaches an argument that looks like =[0-9]{3}, essentially

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.

Ok, this actually looks alright. Thanks for the contribution!

modules/caddyhttp/matchers.go Show resolved Hide resolved
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.

LGTM. I still think it's good that matchers don't return errors, especially since returning an error can be an optional utility like in this case, this approach seems better than actual error return value.

@mholt mholt merged commit 907e2d8 into master Sep 17, 2021
@mholt mholt deleted the try-files-error-trigger branch September 17, 2021 06:52
@mholt mholt removed the under review 🧐 Review is pending before merging label Sep 17, 2021
@francislavoie francislavoie modified the milestones: v2.5.0, v2.4.6 Nov 9, 2021
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.

3 participants