reverseproxy: restore allowing upstream to contain replaceable placeholders #3776
Conversation
| @@ -129,8 +129,12 @@ func (h *Handler) Provision(ctx caddy.Context) error { | |||
| h.ctx = ctx | |||
| h.logger = ctx.Logger(h) | |||
|
|
|||
| re := regexp.MustCompile(`\{[[:graph:]]+\}`) | |||
There was a problem hiding this comment.
Not gonna lie, I do hate this 😆 Let me think on this for a bit and see if we can come up with something more elegant...
This didn't used to be a problem, right? Did we add code that broke this use case? Can we not remove that code? Or at least change it?
There was a problem hiding this comment.
Yeah, the code added to fix the healthcheck on alternate port requires parsing the network address, which errors out for not having a port when using placeholders. I've moved that part into later in the Provision method where it only sets up the health checks. Now if the upstream contains a placeholder dial address, it will not accept active healthcheck config because it's like SRV and owned by another entity.
There was a problem hiding this comment.
Do you think there's a way we can fix the original health checking bug without requiring host/port information up front? That would simplify the code, rather than complicate it.
| @@ -101,6 +102,7 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { | |||
| // to write tests before making any more changes to it | |||
| upstreamDialAddress := func(upstreamAddr string) (string, error) { | |||
| var network, scheme, host, port string | |||
| placeholder := regexp.MustCompile(`\{[[:graph:]]+\}`) | |||
There was a problem hiding this comment.
Why does this logic/check have to be repeated in the Caddyfile adapter?
(If we keep it, this should be brought out into a package-level variable like var placeholderPattern = ... or something, then reused...)
|
Thanks so much for working on this. But the growing complexity resulting from the original fix is starting to make me nervous / give me a headache. 😅 I'm hoping we can revisit the original problem and find a better solution. I'll try taking a look at it later today. |
* reverse_proxy: ensure upstream address has port range of only 1 * reverse_proxy: don't log the error if upstream range size is more than 1
|
I think this will be mostly superseded by #3780, but not before we bring over the new integration tests this PR has (except for one, see linked PR). |
Another regression caused by d55d50b
Updates #3768