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

Unable to set root inside 'handle_response' block which is in turn inside 'php_fastcgi' directive #4322

Closed
yareg-com opened this issue Sep 1, 2021 · 6 comments · Fixed by #4342
Labels
bug 🐞 Something isn't working
Milestone

Comments

@yareg-com
Copy link

yareg-com commented Sep 1, 2021

Hi, I tried to set up a custom error page based on response status from 'php_fastcgi'. My first attempt was like this:

php_fastcgi app:9000 {
  @error status 4xx
  handle_response @error {
    root * /errors
    rewrite * /{http.reverse_proxy.status_code}.html
    file_server
  }
}

Since php_fastcgi directive is just a wrapper over a reverse proxy and (as documented here) I expected it should work but got error instead:

Error during parsing: unrecognized directive: /errors

Removing the line root * /errors solved the problem, but I needed to point 'file_server' to another directory from web root.

So I decided to expand the 'php_fastcgi' directive into respective 'reverse_proxy':

@index file index.php
rewrite @index {http.matchers.file.relative}

@php path *.php
reverse_proxy @php app:9000 {
    transport fastcgi {
        split .php
    }

    @error status 4xx
    handle_response @error {
        root * /errors
        rewrite * /{http.reverse_proxy.status_code}.html
        file_server
    }
}

This worked as expected. I think 'php_fastcgi' directive has a bug or am I missing something?

@francislavoie
Copy link
Member

Yeah, I think this is a Caddyfile parsing bug.

That said, I don't think this functionality will work the way you expect it to. See #4298 (tl;dr, handle_response doesn't currently deal with responses with non-empty bodies properly).

@francislavoie francislavoie added the bug 🐞 Something isn't working label Sep 1, 2021
@yareg-com
Copy link
Author

Now I am confused. The code is taken from the example found in the documentation (scroll down to the bottom of the page). And it is titled as 'Custom error page for errors from upstream' (this is exactly what I need). If I don't need the response body, just the status to trigger the appropriate error page, will it be ok to do it like this? Or this example is strictly for 5xx?

@francislavoie
Copy link
Member

If your upstream writes a response body at all, it won't work, right now. It's a bug.

The main usecase we tested for was the X-Accel-Redirect case, which typically involves the upstream not writing a response body, and expecting Caddy to intercept it to actually write the response.

@francislavoie francislavoie added this to the v2.5.0 milestone Sep 1, 2021
@yareg-com
Copy link
Author

@francislavoie Thank you for clarification. As I use X-Accel-Redirect too, I thought I could implement it the same way. Glad to see it will be fixed in 2.5.0 :)

@francislavoie
Copy link
Member

That's the plan, anyways. We don't have a fix yet, that's just the target.

@francislavoie
Copy link
Member

Alright, I figured out what the actual bug is -- when parsing the tokens inside of php_fastcgi, it reads root * /errors and treats it as if it was the root option of the php_fastcgi directive (see here https://caddyserver.com/docs/caddyfile/directives/php_fastcgi), so it consumes those options before passing it through to the reverse_proxy options parser 🤦‍♂️

Working on a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants