reverseproxy: Fix dial placeholders, SRV, active health checks

[mholt]

Supersedes #3776
Partially reverts or updates #3756, #3693, and #3695

Please take a look @mohammed90 @francislavoie and users we know to have been affected previously: @waiyip-aquabyte, @danlsgiga, @markuswustenberg, and @yflau. Build artifacts are available in the CI / actions area.

My understanding is that fixing #3753 introduced regressions fixed by #3693 and #3695, which broke dynamic upstreams (using placeholders in dial addresses) as reported in #3768. Instead of piling more code on top to patch the regressions, this change causes the recently-added integration tests to pass without adding more complexity. I think the key is to just not parse the network address at provision-time, which we can't reliably do because we don't know whether it is a usable address (because it supports placeholders, the address might not be able to be evaluated until request-time).

@mohammed90 I did bring over the new tests you wrote for #3776 and they pass, but I did not add them to this commit because I want you to get credit for those, so if you would like to push them to this branch before we merge, that would be great. One test did not pass (TestDialWithPlaceholderActiveHealthcheck) but I think that test is not really valid anyway, since we don't parse the addresses up front.

[francislavoie]

I think it helps to review from this point, only focusing on the changes in the reverseproxy package:

b95b87381a282e1fe57295d145b71645d7801f07...aeceb32e976cca48496e5c051ae113ef5a4b362a#diff-142e2362ff

This is everything from this PR back to just before the first reverseproxy change @mohammed90 introduced.

[francislavoie]

This looks good to me. Quick summary of the diff I linked above, from what I'm reading:

• Caddyfile adapter adds default port of 80 if the port is empty
• During provisioning, SRV compat is checked (disallow active health checks, disallow mixing with non-SRV dial)
• During provisioning, the active health check port is set on every upstream (override if health_port is set, basically), new field in Upstream struct for it
• The upstream dial is now passed through the replacer when active health checks happen
• If the active health check port is non-zero, then the dial address is set to that port (and the network set to TCP is unix socket)
• If the upstream is not SRV and has more than one port, errors out.

Seems all logical to me.

[mohammed90]

Francis' summary is on-point!

[mholt]

Excellent, thanks @mohammed90 !

Can any of the affected users please re-test with this PR then? It's ready to merge. Would be nice to have confirmation that it works in real use cases.

[danlsgiga]

Going to check shortly and give a feedback here! Thanks for working on this @mohammed90

[danlsgiga]

Alright, according to my internal tests, all is working as expected! ;) Thanks once again everyone that worked on this! ❤️

[mholt]

Excellent, thanks @danlsgiga -- normally I would wait for more affected users to test but we've already waited about a week and in the interest of getting the bug fixes out I think I'll merge this in and tag a release today.