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

Enable x-accel-redirect #396

Closed
wants to merge 16 commits into from
Closed

Enable x-accel-redirect #396

wants to merge 16 commits into from

Conversation

withinboredom
Copy link
Collaborator

@withinboredom withinboredom commented Dec 17, 2023

closes #365

This adds x-accel-redirect and friends by passing a "filter" to Frankenphp from Caddy. This "filter" allows us to halt output if we have a rewriting matcher that should take over the request. For backward compatibility, context strings are kept as http.reverse_proxy.*.

This is a refactoring of the reverse proxy functionality built into Caddy but adapted for FrankenPHP.

        php_server {
            @accel header X-Accel-Redirect *
            handle_response @accel {
              root * /
              rewrite {http.reverse_proxy.header.X-Accel-Redirect}
              file_server
            }
        }

Todo:

  • worker support
  • tests
  • clean up debug log lines
  • return docker files to a pristine condition
  • refactoring
  • documentation (for caddyfile)
  • documentation (for frankenphp -- hooks)

Thoughts:

  • Anything other than handle_response is probably out of scope for this PR.

frankenphp.go Outdated Show resolved Hide resolved
request_options.go Outdated Show resolved Hide resolved
@withinboredom withinboredom marked this pull request as ready for review January 2, 2024 10:42
@withinboredom withinboredom changed the title WIP: Enable x-accel-redirect Enable x-accel-redirect Jan 2, 2024
logger *zap.Logger
responseMatchers map[string]caddyhttp.ResponseMatcher
handleResponseSegments []*caddyfile.Dispenser
HandleResponse []caddyhttp.ResponseHandler `json:"handle_response,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: maybe can you group all public props?

Comment on lines +247 to +248
err := rh.Provision(ctx)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
err := rh.Provision(ctx)
if err != nil {
if err := rh.Provision(ctx); err != nil {

// FrankenPHPHooks You can pass this to FrankenPHP to "hook into" various processes and direct how FrankenPHP will
// proceed. For example, hooking into responses and redirecting based on headers; detecting db credentials in output
// and sending a white page instead; or, ensuring a custom error page is always shown even if it is a 500 error.
type FrankenPHPHooks struct {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this new type really necessary? I wonder if we cannot use a more generic signature to replace this, such as what does Caddy with its next handler passed as parameter of its HTTP middleware functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be candid, that seems like a pre-generalization (an architectural pre-optimization that might pigeonhole what things we can support in the future). As this is the only hook that we know of, we have no way to know what parameters can be generalized because other hooks would exist during different phases of the request/response cycle.

It's code, so we can technically do whatever we want with it, but I would advise waiting to generalize until there is a need for it.

More technically, the hook is called after all headers have been written, but before a response code is set, which guarantees the underlying writer hasn't flushed the headers to the client yet (since, generally, the status code must be the first line/frame in an HTTP response). We can't know what optimizations the writer makes, but we can know the http spec and use it to our advantage. Maybe this is the wrong way to look at it, but it's the assumption I took with me when writing the code.

@@ -70,3 +70,12 @@ func WithRequestLogger(logger *zap.Logger) RequestOption {
return nil
}
}

// WithHooks sets a hook on the request that FrankenPHP will call back
func WithHooks(hooks *FrankenPHPHooks) RequestOption {
Copy link
Owner

Choose a reason for hiding this comment

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

Why not just WithRequestFilter then? No need for an extra layer of indirection IMHO.

handled = fc.hooks.ResponseFilter(int(status), fc.responseWriter.Header(), r)
}

if handled {
Copy link
Owner

@dunglas dunglas Jan 6, 2024

Choose a reason for hiding this comment

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

I still fail to understand why we need that.
If I understand correctly https://www.nginx.com/resources/wiki/start/topics/examples/xsendfile/ and https://tn123.org/mod_xsendfile/, this feature is entirely in the scope of the web server and shouldn't leak in this module.

From my understanding, we should let PHP return the full HTTP response, and only after that post-process it in the Caddy module.

That's why, for instance, Symfony, Laravel, and the like will not generate any response body if this feature is enabled: https://github.com/symfony/symfony/blob/7.0/src/Symfony/Component/HttpFoundation/BinaryFileResponse.php#L208

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, interesting. I was under the assumption output may still be sent from PHP and being a bit defensive. To be honest, I am also looking into a totally different approach that doesn't need any changes at all in FrankenPHP. I've just been fighting a dying HD on a server for the last couple of days so I haven't had much time to look into it.

But yeah, if we don't need to be defensive, then this can be much simpler.

@dunglas
Copy link
Owner

dunglas commented Feb 26, 2024

Hey @withinboredom, do you mind if I push in your branch to finish this feature?

@withinboredom
Copy link
Collaborator Author

Yeah, go for it. I just haven't had much time to get to this.

@dunglas
Copy link
Owner

dunglas commented Apr 10, 2024

Here is my alternative PR (directly in Caddy): caddyserver/caddy#6232

@dunglas
Copy link
Owner

dunglas commented Apr 23, 2024

Closing in favor of caddyserver/caddy#6232 that is likely to be merged. Thanks for paving the way @withinboredom!!

@dunglas dunglas closed this Apr 23, 2024
@withinboredom withinboredom deleted the add/x-accel branch April 23, 2024 10:13
@withinboredom
Copy link
Collaborator Author

🥳

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.

Intercept responses for nginx-like X-Accel-Redirect handling
2 participants