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

reverseproxy: Add handle_response blocks to reverse_proxy (#3710) #3712

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions caddyconfig/httpcaddyfile/directives.go
Expand Up @@ -263,6 +263,13 @@ func (h Helper) NewBindAddresses(addrs []string) []ConfigValue {
return []ConfigValue{{Class: "bind", Value: addrs}}
}

// WithDispenser returns a new instance based on d. All others Helper
// fields are copied, so typically maps are shared with this new instance.
func (h Helper) WithDispenser(d *caddyfile.Dispenser) Helper {
h.Dispenser = d
return h
}
Comment on lines +266 to +271
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely clear on why this is necessary?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is httpcaddyfile.ParseSegmentAsSubroute takes a Helper rather than a Dispenser, which makes it necessary to chain parsing for other directives. Here, we only want to pass a dispenser that only has the contents of the handle_response block so that it doesn't overrun its parsing. That's why the dispenser is being replaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again @francislavoie, yes this simple method is to simplify code in such cases.


// ParseSegmentAsSubroute parses the segment such that its subdirectives
// are themselves treated as directives, from which a subroute is built
// and returned.
Expand Down
@@ -0,0 +1,130 @@
:8884

reverse_proxy 127.0.0.1:65535 {
handle_response header X-Accel-Redirect {
respond "Header X-Accel-Redirect!"
}
handle_response header X-Another {
respond "Header X-Another!"
}
handle_response status 401 {
respond "Status 401!"
}
handle_response status 403 {
respond "Status 403!"
}
handle_response {
respond "Any!"
}
}
----------
{
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":8884"
],
"routes": [
{
"handle": [
{
"handle_response": [
{
"match": {
"headers": {
"X-Accel-Redirect": []
}
},
"routes": [
{
"handle": [
{
"body": "Header X-Accel-Redirect!",
"handler": "static_response"
}
]
}
]
},
{
"match": {
"headers": {
"X-Another": []
}
},
"routes": [
{
"handle": [
{
"body": "Header X-Another!",
"handler": "static_response"
}
]
}
]
},
{
"match": {
"status_code": [
401
]
},
"routes": [
{
"handle": [
{
"body": "Status 401!",
"handler": "static_response"
}
]
}
]
},
{
"match": {
"status_code": [
403
]
},
"routes": [
{
"handle": [
{
"body": "Status 403!",
"handler": "static_response"
}
]
}
]
},
{
"match": {},
"routes": [
{
"handle": [
{
"body": "Any!",
"handler": "static_response"
}
]
}
]
}
],
"handler": "reverse_proxy",
"upstreams": [
{
"dial": "127.0.0.1:65535"
}
]
}
]
}
]
}
}
}
}
}
53 changes: 49 additions & 4 deletions modules/caddyhttp/reverseproxy/caddyfile.go
Expand Up @@ -38,14 +38,14 @@ func init() {

func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) {
rp := new(Handler)
err := rp.UnmarshalCaddyfile(h.Dispenser)
err := rp.ParseCaddyfileReverseProxy(h)
if err != nil {
return nil, err
}
return rp, nil
}

// UnmarshalCaddyfile sets up the handler from Caddyfile tokens. Syntax:
// ParseCaddyfileReverseProxy sets up the handler from Caddyfile tokens. Syntax:
//
// reverse_proxy [<matcher>] [<upstreams...>] {
// # upstreams
Expand Down Expand Up @@ -86,13 +86,20 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
// transport <name> {
// ...
// }
//
// # handle responses
// handle_response [header <field>|status <status>] {
// ...
// }
// }
//
// Proxy upstream addresses should be network dial addresses such
// as `host:port`, or a URL such as `scheme://host:port`. Scheme
// and port may be inferred from other parts of the address/URL; if
// either are missing, defaults to HTTP.
func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
func (h *Handler) ParseCaddyfileReverseProxy(helper httpcaddyfile.Helper) error {
francislavoie marked this conversation as resolved.
Show resolved Hide resolved
d := helper.Dispenser

// currently, all backends must use the same scheme/protocol (the
// underlying JSON does not yet support per-backend transports)
var commonScheme string
Expand Down Expand Up @@ -578,6 +585,45 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
}
transport = rt

case "handle_response":
var rm caddyhttp.ResponseMatcher
args := d.RemainingArgs()
switch len(args) {
case 2:
switch args[0] {
case "header":
rm = caddyhttp.ResponseMatcher{
Headers: http.Header{args[1]: []string{}},
}
case "status":
statusNum, err := strconv.Atoi(args[1])
if err != nil {
return d.Errf("bad status value '%s': %v", args[1], err)
}
rm = caddyhttp.ResponseMatcher{
StatusCode: []int{statusNum},
}
default:
return d.Err("handle_response only accepts header|status")
}
case 0: // Any status or header
default:
return d.ArgErr()
}
handler, err := httpcaddyfile.ParseSegmentAsSubroute(helper.WithDispenser(d.NewFromNextSegment()))
if err != nil {
return err
}
subroute, ok := handler.(*caddyhttp.Subroute)
if !ok {
return helper.Errf("segment was not parsed as a subroute")
}
h.HandleResponse = append(h.HandleResponse,
caddyhttp.ResponseHandler{
Match: &rm,
Routes: subroute.Routes,
})

default:
return d.Errf("unrecognized subdirective %s", d.Val())
}
Expand Down Expand Up @@ -855,6 +901,5 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {

// Interface guards
var (
_ caddyfile.Unmarshaler = (*Handler)(nil)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this no longer a caddyfile Unmarshaler? It needs to be if it is to be used to unmarshal the Caddyfile.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's not exactly right (because reverse_proxy still need to be a caddyfile.Unmarshaler), but this was done because with those changes, httpcaddyfile.Helper is needed to do the parse chaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @francislavoie, you are right.

@mholt if you tell me how to cope with the absence of httpcaddyfile.Helper in (*Handler).UnmarshalCaddyfile() (as I need this httpcaddyfile.Helper instance to call httpcaddyfile.ParseSegmentAsSubroute() as suggested by @francislavoie in #3707), I will be happy to change this.

Copy link
Member

Choose a reason for hiding this comment

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

I did a bit more digging in the source, and actually, I think it's okay for reverse_proxy to not be a caddyfile.Unmarshaler.

This is because it's never used as a guest module to something else. Regular directives don't strictly need to be Unmarshalers because their parsing is handled by callbacks registered with RegisterHandlerDirective or RegisterDirective, and those are passed Helper.

Most directives that do implement caddyfile.Unmarshaler only do so for the registered callbacks to call UnmarshalCaddyfile, but that's not actually necessary.

It is necessary for guest modules (like proxy transports, log encoders, log filters, matchers, etc.) to be Unmarshaler because they are invoked by the parent module (i.e. actual directive parsers).

Copy link
Member

@mholt mholt Sep 21, 2020

Choose a reason for hiding this comment

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

It's good practice / conventional to implement that, so you can unmarshal it where necessary. But true, all a directive needs is an unmarshaling function, not necessarily to implement an interface. I strongly encourage keeping to that convention, especially for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I understand @mholt but in this case I don't think it's possible since we actually need to use Helper all the way down the callchain for handle_response to work. Consider how handle and route work, this is the same kind of situation (except that it's nested within another directive this time, i.e. reverse_proxy).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any news on that topic?

_ caddyfile.Unmarshaler = (*HTTPTransport)(nil)
)
2 changes: 1 addition & 1 deletion modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
Expand Up @@ -355,7 +355,7 @@ func parsePHPFastCGI(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error
// using the reverse_proxy directive syntax
// TODO: this can overwrite our fcgiTransport that we encoded and
// set on the rpHandler... even with a non-fastcgi transport!
err = rpHandler.UnmarshalCaddyfile(dispenser)
err = rpHandler.ParseCaddyfileReverseProxy(h.WithDispenser(dispenser))
if err != nil {
return nil, err
}
Expand Down