reverseproxy: Add handle_response blocks to reverse_proxy (#3710)

[maxatome]

No description provided.

[mholt]

Thanks for working on a PR!

I just did a quick high-level pass for starters. We shouldn't have to change the dispenser or remove the CaddyfileUnmarshaler interface...

I also noticed that with this approach -- and maybe I'm wrong, feel free to correct me -- only filters on a single status code or header value. That seems like a severe limitation; I think adding more status codes is easily doable but giving proper control over headers is going to need more consideration.

[mholt]

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

[francislavoie]

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.

[maxatome]

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.

[francislavoie]

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).

[mholt]

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.

[francislavoie]

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).

[maxatome]

Any news on that topic?

[mholt]

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

[francislavoie]

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.

[maxatome]

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

[maxatome]

I also noticed that with this approach -- and maybe I'm wrong, feel free to correct me -- only filters on a single status code or header value. That seems like a severe limitation; I think adding more status codes is easily doable but giving proper control over headers is going to need more consideration.

I pushed a change in reverse_proxy_handle_response.txt where several headers and statuses are used: each case generates a new handle_response block.

But perhaps you want to be able to combine several headers/statuses for one handle_response?

I applied the format you proposed in the original issue #3707 but maybe I misunderstood your proposition.

Do you want a handle_response block be like:

reverse_proxy ... {
	...
	handle_response header X-Accel-Redirect header X-Another status 401 {
		... 
	}
}

? Meaning both X-Accel-Redirect and (or "or"?) X-Another headers be present with the status 401. When you say several statuses, of course you mean a "or" between them, but for headers it is a "and". I am not sure to fully understand, and so the future caddyfile author.

Feel free to provide me a complete spec of what you want so we can go further.

[francislavoie]

Just a heads up, #3740 may change the way the parsing of handle_response would work; you might need make sure to consume the args on the handle_response subdirective line before calling ParseSegmentAsSubroute

[maxatome]

Just a heads up, #3740 may change the way the parsing of handle_response would work; you might need make sure to consume the args on the handle_response subdirective line before calling ParseSegmentAsSubroute

Aren't the args consumed by d.RemainingArgs()? I thought they were. If not, I'll fix this.

[francislavoie]

Sorry to come back so late on this.

Do you want a handle_response block be like:

reverse_proxy ... {
	...
	handle_response header X-Accel-Redirect header X-Another status 401 {
		... 
	}
}

? Meaning both X-Accel-Redirect and (or "or"?) X-Another headers be present with the status 401. When you say several statuses, of course you mean a "or" between them, but for headers it is a "and". I am not sure to fully understand, and so the future caddyfile author.

Feel free to provide me a complete spec of what you want so we can go further.

@maxatome So I had a bit of a think 🤔 and I believe what we should do to make this more natural is to reuse the matcher syntax that we already have for this, so like:

reverse_proxy ... {
	@accel {
		header X-Accel-Redirect
		header X-Another
		status 401
	}
	handle_response @accel {
		...
	}
}

This would be done by storing the named matchers in a map which is then referenced when parsing the handle_response subdirective.

I think we could keep the existing approach where if you just need one matcher, you can put it inline, but for anything more complex you'd use a named matcher.

One annoyance is that since response matchers aren't the same as request matchers, we can't quite share that parsing code.

[francislavoie]

Also - if you don't have the time to finish this off, let me know and I'd be glad to take it over (you'll still get author credit)

[maxatome]

Hi @francislavoie thanks for your reply.
I do not think I will have the time to work on this in the short-term, so if you are ready to take it over, be my guest :)

[francislavoie]

Thanks for your work on this @maxatome, opened #4021 to replace this PR!