reverseproxy: Add `handle_response` blocks to `reverse_proxy` (#3710) by maxatome · Pull Request #3712 · caddyserver/caddy

maxatome · 2020-09-06T16:17:52Z

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 · 2020-09-16T19:03:35Z

modules/caddyhttp/reverseproxy/caddyfile.go


 // Interface guards
 var (
-	_ caddyfile.Unmarshaler = (*Handler)(nil)


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

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.

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.

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

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.

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

Any news on that topic?

mholt · 2020-09-16T19:06:42Z

caddyconfig/httpcaddyfile/directives.go

+// 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
+}


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

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.

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

maxatome · 2020-09-19T10:54:44Z

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 · 2020-09-21T00:28:19Z

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

modules/caddyhttp/reverseproxy/caddyfile.go

maxatome · 2020-09-21T07:06:28Z

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.

…erver#3710)

francislavoie · 2021-02-11T04:35:55Z

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 · 2021-02-11T21:33:24Z

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 · 2021-02-11T22:15:31Z

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 · 2021-02-16T03:11:47Z

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

maxatome mentioned this pull request Sep 6, 2020

caddyfile: add handle_response blocks to reverse_proxy #3707

Closed

maxatome force-pushed the reverse_proxy-handle_response branch from f47fbf4 to 62eb124 Compare September 14, 2020 07:35

mholt requested changes Sep 16, 2020

View reviewed changes

mholt added the under review 🧐 Review is pending before merging label Sep 16, 2020

mholt added this to the 2.x milestone Sep 16, 2020

maxatome force-pushed the reverse_proxy-handle_response branch from 6ef80a9 to f4eace2 Compare September 19, 2020 14:12

francislavoie mentioned this pull request Sep 19, 2020

Modify status code of reverse proxy response #2920

Closed

francislavoie reviewed Sep 21, 2020

View reviewed changes

modules/caddyhttp/reverseproxy/caddyfile.go Show resolved Hide resolved

maxatome force-pushed the reverse_proxy-handle_response branch from f4eace2 to 165dcf7 Compare September 27, 2020 15:15

maxatome force-pushed the reverse_proxy-handle_response branch from 165dcf7 to 5b5b63c Compare October 30, 2020 11:36

francislavoie mentioned this pull request Nov 4, 2020

Feature Request: Sanitize/reformat/remap content retrieved via reverse_proxy #3848

Closed

maxatome added 2 commits January 5, 2021 23:28

reverseproxy: Add handle_response blocks to reverse_proxy (caddys…

7dcdeed

…erver#3710)

reverseproxy: complete handle_response test

4a48c44

maxatome force-pushed the reverse_proxy-handle_response branch from 5b5b63c to 4a48c44 Compare January 5, 2021 22:33

mholt mentioned this pull request Feb 1, 2021

Support for nginx style X-Accel headers in reverse_proxy #3828

Closed

francislavoie mentioned this pull request Feb 16, 2021

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

Merged

francislavoie closed this Feb 16, 2021

mholt removed the under review 🧐 Review is pending before merging label May 2, 2021

Uh oh!

Conversation

maxatome commented Sep 6, 2020

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

mholt Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

francislavoie Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxatome Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

francislavoie Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mholt Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francislavoie Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxatome Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

mholt Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

francislavoie Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxatome Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

maxatome commented Sep 19, 2020

Uh oh!

francislavoie commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maxatome commented Sep 21, 2020

Uh oh!

francislavoie commented Feb 11, 2021

Uh oh!

francislavoie commented Feb 11, 2021

Uh oh!

maxatome commented Feb 11, 2021

Uh oh!

francislavoie commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

francislavoie Sep 16, 2020 •

edited

Loading

francislavoie Sep 21, 2020 •

edited

Loading

mholt Sep 21, 2020 •

edited

Loading

francislavoie Sep 21, 2020 •

edited

Loading

francislavoie Sep 16, 2020 •

edited

Loading

francislavoie commented Sep 21, 2020 •

edited

Loading