-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat: add generic response interceptors #6232
Conversation
Nice feature! Thanks @dunglas ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool change. I haven't thoroughly reviewed it yet but my first pass is liking where this is going. Just a couple nit picky thoughts for starters.
7cce44a
to
8aa881f
Compare
Looking good here. @francislavoie any thoughts on the Caddyfile implementation? |
// delegate the parsing of handle_response to the caller, | ||
// since we need the httpcaddyfile.Helper to parse subroutes. | ||
// See h.FinalizeUnmarshalCaddyfile | ||
i.handleResponseSegments = append(i.handleResponseSegments, d.NewFromNextSegment()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this isn't embedded within another directive, I think we don't need the finalize
thing here. I think we can immediately parse the current segment as a handler and add that to the config. Then we can delete handleResponseSegments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an idea of how we can do that @francislavoie? We need a httpcaddyfile.Helper
to call ParseSegmentAsSubroute()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh right. I'll think about it.
@dunglas I'll probably tag 2.8 beta 1 this week; it'd be nice to get this in 2.8 beta 1, but if not, maybe we can get it in beta 2 or the RC. |
3138fa3
to
8e39b5a
Compare
@dunglas Before we merge this, can we mark the exported types/methods as "EXPERIMENTAL: Subject to change or removal" (or something like that)? It sounds like there are still decisions to be made to finalize APIs to accommodate expanded functionality later, and by marking it as experimental we can at least merge it in sooner and then change it, as opposed to making painful changes later on. |
Sounds good to me to mark the public API as experimental. Should I update the code? |
Yeah, if you have a chance to do that could you push a commit? I'm wrapping up a few things here. |
Co-authored-by: Francis Lavoie <lavofr@gmail.com>
c7aedf3
to
184654a
Compare
@mholt done (the public type was already marked as experimental). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay thanks. I was mobile this weekend so thanks for double-checking.
In proxy mode, Caddy allows intercepting and modifying responses coming from upstream servers.
This is especially useful for implementing
X-Sendfile
/X-Accel-Redirect
-like features.However, the current implementation of this feature is only available in proxy mode and doesn't allow to intercept and modify responses created by modules.
This makes it impossible to use features like
X-Sendfile
with FrankenPHP, https://github.com/mliezun/caddy-snake, and other similar potential modules (LUA module, etc).This patch implements a generic implementation of response interceptors that behaves similarly to the
reverse_proxy
handle_response
.It currently does not support the
copy_response
andcopy_response_headers
subdirectives, but it should be easy to add support for these features in future Pull Requests.It should also be possible, in the future, to replace the current
reverse_proxy
specific implementation with this new more generic one, but it is out of scope for this PR.Closes dunglas/frankenphp#365. Closes dunglas/frankenphp#396.