feat: Add Caddy placeholders #1901
Conversation
|
Would this only be used for formatting access logs? Here it says that part of the original response will be used for routing decisions, not sure though what that means if you're not working with a reverse proxy. Reverse proxies often modify the response, so these placeholders can help if you want to log values from the original response. But in the FrankenPHP case the response is not modified and all original status/headers are already present in the access logs. |
|
I'll give you the full context of what I'm trying to achieve since that's not entirely reflected in the generic example I added: I'm migrating from Apache with mod_php which has a apache_note() function to pass strings over to Apache which can then be used for logging (and maybe other things?). In my use case I used this to log the userId and tenantId to my access log. However there is no such mechanism in Caddy/FrankenPHP but the way I've seen nginx and Caddy in reverse proxy mode handle this requirement has been by adding that information in response headers which then get stripped before being sent out to the client. I don't want to use caddy's default response headers logging because:
|
|
IMHO we should add a function similar to |
|
I agree, we probably shouldn't replicate the hacky solution. Ideally a function call would add that context to the logger/replacers. Not sure though how to most elegantly forward that info to caddy. |
|
That's what I've also been considering, I've gone ahead and implemented that function. I believe the remaining linter error is a bug: |
| } | ||
|
|
||
| c := context.WithValue(r.Context(), contextKey, fc) | ||
| c = context.WithValue(c, PlaceholdersContextKey, make(map[string]string)) |
There was a problem hiding this comment.
Can you defer context creation to go_set_caddy_placeholder? Otherwise this map will most of the time end up being unused
There was a problem hiding this comment.
I've been trying to work on this but I'm stuck and don't know how to go about it since this is only my second time ever working with Go.
I've been able to defer creation to the first call to go_set_caddy_placeholder but I can't access the updated request context from caddy in ServeHTTP after the request is handled by FrankenPHP. That is because frankenPHPContext is private so I can't access the updated request and it's context.
I feel like my only way forward would be to make frankenPHPContext public and store the placeholders there directly instead of the request context.
What do you think?
There was a problem hiding this comment.
I think that probably makes sense. If you were to use this feature outside Caddy (i.e. frankenphp as a library), then it’d also be useful.
There was a problem hiding this comment.
If we plan to expose other things about the request, then making parts of FrankenPHPContext public probably is the way to go. What else would you like to expose @withinboredom ?
There was a problem hiding this comment.
Should I go ahead and make FrankenPHPContext public?
If so, is there anything I should be aware of before mass replacing frankenPHPContext by FrankenPHPContext? Is it safe to proceed like that? (asking because it's a relatively large diff)
There was a problem hiding this comment.
Hmm not sure, since there aren't other obvious use cases, I'd rather not make it public. I think this might still be the best option, I also can't think of a better way right now.
c = context.WithValue(c, PlaceholdersContextKey, make(map[string]string))| return | ||
| } | ||
|
|
||
| placeholders[C.GoString(key)] = C.GoString(value) |
There was a problem hiding this comment.
Maybe these keys should always be prefixed with 'frankenphp', otherwise devs might end up replacing existing placeholders
There was a problem hiding this comment.
I’m partial to what @dunglas said. It’s possibly better to reimplement apache_note (similar to what we did with apache_headers IIRC) with our own flavour instead of a new function.
| } | ||
|
|
||
| c := context.WithValue(r.Context(), contextKey, fc) | ||
| c = context.WithValue(c, PlaceholdersContextKey, make(map[string]string)) |
There was a problem hiding this comment.
I think that probably makes sense. If you were to use this feature outside Caddy (i.e. frankenphp as a library), then it’d also be useful.
4 participants
This pull request introduces a mechanism to expose HTTP response details from FrankenPHP as placeholders for use in Caddy directives, improving observability and configurability. The core of the change is a new
responseWriterInterceptorthat captures response status and headers, making them available as placeholders. Documentation has also been updated to describe the new placeholders.This is inspired by Caddy's native reverse proxy module:
https://github.com/caddyserver/caddy/blob/65e0ddc22137bbbaa68c842ae0b98d0548504545/modules/caddyhttp/reverseproxy/reverseproxy.go#L955-L961