Repository navigation
reverseproxy: add lb_retry_match condition on response status - #7569
Conversation
|
I've revisited this PR to make it simpler. Shortly, the only thing left is retrying based on status, which I moved to the I've removed retrying based on conditions as it messes with default behavior too much and it makes everything too complicated and unclear. I still can implement them in this PR, but feels like you should point me on how they must be defined and handled. Updated the PR's description, previous one is now hidden under "Initial version" spoiler. |
b8aaa26 to
4e31ada
Compare
|
@mohammed90 Hello! Sorry for ping, but would you mind reviewing this? |
eeb231c to
d106d8c
Compare
|
I find this quite awkward. As documented, the config layer says Could we use response matchers https://caddyserver.com/docs/caddyfile/response-matchers directly instead of This also needs Caddyfile adapt tests. See |
lb_retry_match condition on response status
|
@francislavoie yea, the parsing looks weird in such scenario. The reason why I did it exactly like this is that I wanted to preserve the ability to write the spec like this: This way it looks understandable for an end user and reads exactly like "retry on match method It will be still the same mix, but with re-used parsing logic. OR we can split the matchers, so it will look like: This way the code is cleaner and there is a strict separation of matching requests and responses, but this approach a bit less flexible because of how we treat conditions:
But then we will be unable to implement something like "retry on ( Maybe I'm missing something. @francislavoie @mohammed90 What do you think? Which approach to implement then? |
|
Hmm. I think we can just reframe this. What we want is to match on response status or headers. We can actually do this using CEL expressions already, as long as the Replacer is filled with properties of the response, placeholders can be used in the CEL expression to do complex conditions involving those. We document in https://caddyserver.com/docs/caddyfile/directives/reverse_proxy#intercepting-responses that lb_retry_match `path('/foo*') && {rp.status_code} in [502, 503]`The backtick syntax might not work as-is in We'll also need some |
03fcc72 to
8b1f9c6
Compare
|
@francislavoie thank you for the review and pointing in the right direction! I've revisited changes and re-implemented them. Now:
It wasn't possible to implement it like Everything is covered with unit-tests, e2e tests and caddyfile tests.
For example, the previous logic:
Just to remind what transport error is (roughly):
Now examples of how the new
|
8b1f9c6 to
f4ec226
Compare
|
I'm confused. That looks like way too many changes for this. Why do we need "isTransportError" and such? That seems very over engineered. It's also changing how the placeholders are being set outside of the retries, I don't understand why. And why is it blocking other matchers to be alongside expression? That's very arbitrary, nothing else does that. In my mind, the change should've just been "make sure placeholders are set during the retry flow, and add tests". That's it really. Possibly also "allow omitting |
|
@francislavoie thank you for a quick response
It's now set earlier because the old place executes after the place where we decide to retry. Without the moving these variables would be unset in retry logic, which must go before Also I've added
Everything else ( Then in case of transport error, that's what happening in retry logic: caddy/modules/caddyhttp/reverseproxy/reverseproxy.go Lines 1270 to 1282 in acf8d6a We don't get into the very first if because It makes all transport error non-retriable if
I didn't found easy ways of fixing this + not break the existing logic + not code workarounds + let API remain simple and not confusing for user.
That's why I separated
If we somehow deal with transport error handling, then I guess there is no point to restrict mixing. Actually if we stick to
I also thought that initially, but it's not that simple. Besides things described above, I've also added additional handling of failure counting for health checks (so upstream marked unhealthy only in case of dial/transport errors), and the actual response matcher handling in I can revert the |
f4ec226 to
0283a77
Compare
And updated all the tests, comments and docs everywhere. |
8f170bb to
61c00e9
Compare
francislavoie
left a comment
There was a problem hiding this comment.
Much better! I think I'm happy where this is at now. We'll just need to spend a bit of effort on documentation for the website as well (if you'd like to help with that after merging) to make it clear how the different states work, especially because of the hidden magic regarding branching logic if there's an expression vs not.
61c00e9 to
6087447
Compare
|
I've been trying to use this and am having some issues. My setup is a Caddy load balancer with Caddy upstreams that reverse proxy to localhost Rails apps. For context, I'm trying to use this so passive health checks to upstreams where Rails might be down can retry automatically. Upstream Caddy would potentially return a 502, 503, or 504 in that case (I believe). I'm getting I discovered this because using the combined expression with status codes was giving a {"level":"error","ts":1783036764.2413948,"logger":"http.handlers.reverse_proxy","msg":"error matching request for retry","error":"no such overload"}Using: Is this a bug? My full reverse proxy config is the following for context: A few other questions:
|
caddy 2.11.3 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`. - [ ] `resource` blocks have been checked for updates. <details> <summary>release notes</summary> <pre>This release improves several aspects of Caddy with minor features, bug fixes, and security patches. Thank you to everyone and their bots who contributed to help make this release the best one yet! **Security patches:** - fastcgi: Carrying over a patch from FrankenPHP for a bug that could allow non-PHP files to be executed; collaborated on by @dunglas, @KC1zs4, and @chenjj. - vars: A more thorough fix for GHSA-m2w3-8f23-hxxf, collaborated by @everping and @vnxme. - admin: Array index normalization to prevent remote admin socket auth bypass, by @Amemoyoi and bot. - admin: More rigorous path prefix matching to prevent remote admin socket auth bypass, by @Amemoyoi and bot. We've also merged a couple PRs that fix upstream security bugs in other projects like quic-go and CertMagic. Thank you to @marten-seemann for maintaining quic-go so diligently! ## What's Changed * caddyhttp: Sync placeholder expansion in `vars` and `vars_regexp` by @vnxme in caddyserver/caddy#7573 * caddytls: Avoid ACME fallback for implicit Tailscale *.ts.net policies by @steadytao in caddyserver/caddy#7577 * chore: Resolve recent CI failures by @mholt in caddyserver/caddy#7593 * caddytls: Consolidate empty APs more smartly by @mholt in caddyserver/caddy#7567 * rewrite: skip query rename when source key is absent by @steadytao in caddyserver/caddy#7599 * root: introduce down-propagating Helper.BlockState for other directives/plugins to use by @henderkes in caddyserver/caddy#7594 * http: make zstd checksum configurable by @ottenhoff in caddyserver/caddy#7586 * notify: Always send "READY=1" even after an error by @francislavoie in caddyserver/caddy#7597 * reverseproxy: Fix check for `header_up Host {upstream_hostport}` redundancy by @yubiuser in caddyserver/caddy#7564 * caddytls: Expand placeholders in dns_challenge override_domain tls parameter by @pberkel in caddyserver/caddy#7609 * tls: add system and combined CA pool modules by @HarshPatel5940 in caddyserver/caddy#7406 * vars: Don't expand placeholders in values by @vnxme in caddyserver/caddy#7629 * build(deps): bump go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp from 1.42.0 to 1.43.0 by @dependabot[bot] in caddyserver/caddy#7637 * build(deps): bump the all-updates group across 1 directory with 11 updates by @dependabot[bot] in caddyserver/caddy#7641 * reverseproxy: make stream copy buffer size configurable by @steadytao in caddyserver/caddy#7627 * vars: Add matcher placeholder handling tests by @steadytao in caddyserver/caddy#7640 * build(deps): bump github.com/go-jose/go-jose/v4 from 4.1.3 to 4.1.4 by @dependabot[bot] in caddyserver/caddy#7621 * logging: Add journald encoder wrapper by @steadytao in caddyserver/caddy#7623 * caddyfile: Improve import/global options UX for imports before global options by @steadytao in caddyserver/caddy#7642 * chore: replace `interface{}` with `any` for modernization by @tsinglua in caddyserver/caddy#7571 * chore: bump timberjack to v1.4.1 by @DeRuina in caddyserver/caddy#7618 * logging: Preserve ts for journald-wrapped JSON logs by @steadytao in caddyserver/caddy#7644 * fileserver: show symlink targets verbatim (#7476) by @maxtruxa in caddyserver/caddy#7579 * fix(caddyfile): {block} in snippet by @prettysunflower in caddyserver/caddy#7558 * caddyhttp: Document missing placeholders for escaped URI and prefixed query by @steffenbusch in caddyserver/caddy#7659 * chore: add `AGENTS.md` by @mohammed90 in caddyserver/caddy#7652 * build(deps): bump github.com/jackc/pgx/v5 from 5.8.0 to 5.9.0 by @dependabot[bot] in caddyserver/caddy#7655 * admin: Redact sensitive request headers in API logs by @steadytao in caddyserver/caddy#7578 * reverseproxy: add `lb_retry_match` condition on response status by @seroperson in caddyserver/caddy#7569 * caddyhttp: prefer port 443 in auto-HTTPS and add tests by @mholt in caddyserver/caddy#7666 * fix: Propagate ECH keys to the QUIC listener by @steadytao in caddyserver/caddy#7670 * chore: Use atomics where appropriate by @francislavoie in caddyserver/caddy#7648 * metrics: Implement pushing via OLTP by @dunglas in caddyserver/caddy#7664 * logging: Add regression coverage for rotated file mode by @steadytao in caddyserver/caddy#7620 * httpcaddyfile: Inherit global ACME issuer settings in tls shortcuts by @steadytao in caddyserver/caddy#7617 * build(deps): bump github.com/jackc/pgx/v5 from 5.9.0 to 5.9.2 by @dependabot[bot] in caddyserver/caddy#7668 * admin: require path segment boundary in remote access control by @Amemoyoi in caddyserver/caddy#7673 * reverseproxy: Add ability to clear dynamic upstreams cache during retries by @mholt in caddyserver/caddy#7662 * listeners: clean up stale Unix socket files on Windows by @mfrischknecht in caddyserver/caddy#7676 * admin: reject non-canonical config array indices by @Amemoyoi in caddyserver/caddy#7592 * caddytls: Expand ACME credentials by @tribut in caddyserver/caddy#7554 * caddyauth: set user placeholders before auth rejection by @cyphercodes in caddyserver/caddy#7685 * caddyauth: revert user placeholders on auth rejection by @steadytao in caddyserver/caddy#7688 * chore: Fix golangci-lint 2.12.1 findings by @steadytao in caddyserver/caddy#7690 * httpcaddyfile: accept duration strings for log sampling interval by @tomholford in caddyserver/caddy#7694 * tls: Add alpn to managed HTTPS records by @steadytao in caddyserver/caddy#7653 * caddytls: avoid duplicate automation for wildcard-covered hosts by @Rijul-A in caddyserver/caddy#7697 * docs: add documentation for fileExists and fileStat template functions by @steffenbusch in caddyserver/caddy#7700 * rewrite: escape file matcher paths before rewriting by @cyphercodes in caddyserver/caddy#7683 * metrics: Add nil check for metricsHandler in AdminMetrics.serveHTTP by @Br1an67 in caddyserver/caddy#7553 ## New Contributors * @steadytao made their first contribution in caddyserver/caddy#7577 * @henderkes made their first contribution in caddyserver/caddy#7594 * @yubiuser made their first contribution in caddyserver/caddy#7564 * @pberkel made their first contribution in caddyserver/caddy#7609 * @HarshPatel5940 made their first contribution in caddyserver/caddy#7406 * @tsinglua made their first contribution in caddyserver/caddy#7571 * @maxtruxa made their first contribution in caddyserver/caddy#7579 * @seroperson made their first contribution in caddyserver/caddy#7569 * @Amemoyoi made their first contribution in caddyserver/caddy#7673 * @mfrischknecht made their first contribution in caddyserver/caddy#7676 * @tribut made their first contribution in caddyserver/caddy#7554 * @cyphercodes made their first contribution in caddyserver/caddy#7685 * @tomholford made their first contribution in caddyserver/caddy#7694 * @Rijul-A made their first contribution in caddyserver/caddy#7697 * @Br1an67 made their first contribution in caddyserver/caddy#7553 **Full Changelog**: https://github.com/caddyserver/caddy/compare/v2.11.2...v2.11.3</pre> <p>View the full release notes at <a href="https://github.com/caddyserver/caddy/releases/tag/v2.11.3">https://github.com/caddyserver/caddy/releases/tag/v2.11.3</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!842
…data (#8168) * reverseproxy: Only retry responses for expressions that use response data Since #7569 every lb_retry_match set with an expression is evaluated against upstream responses. An expression that only tests the request, such as {http.request.method} == "POST", then matched every successful response: the request was sent again to the next upstreams and the client got a 502 once retries ran out. Only treat expressions referencing {rp.*} placeholders as response conditions. * reverseproxy: Only count placeholders set from the response as response data * reverseproxy: Read response placeholders from the checked expression The expression matcher records the placeholder names its checked CEL expression reads, so retry_match finds direct ph() calls and ignores escaped placeholder text in string literals. A placeholder name only known at evaluation time counts as response data. * reverseproxy: Match response placeholder names exactly Only the header placeholders are matched by prefix now, and both upstream latency placeholders are listed. MatchExpression.Placeholders returns a copy, so callers cannot change the provisioned matcher.
…data (caddyserver#8168) * reverseproxy: Only retry responses for expressions that use response data Since caddyserver#7569 every lb_retry_match set with an expression is evaluated against upstream responses. An expression that only tests the request, such as {http.request.method} == "POST", then matched every successful response: the request was sent again to the next upstreams and the client got a 502 once retries ran out. Only treat expressions referencing {rp.*} placeholders as response conditions. * reverseproxy: Only count placeholders set from the response as response data * reverseproxy: Read response placeholders from the checked expression The expression matcher records the placeholder names its checked CEL expression reads, so retry_match finds direct ph() calls and ignores escaped placeholder text in string literals. A placeholder name only known at evaluation time counts as response data. * reverseproxy: Match response placeholder names exactly Only the header placeholders are matched by prefix now, and both upstream latency placeholders are listed. MatchExpression.Placeholders returns a copy, so callers cannot change the provisioned matcher. (cherry picked from commit 7c74563)
Closes #6267
lb_retry_matchnow handlesexpressionoption which evaluates against the upstream response via{rp.status_code},{rp.status_text}, and{rp.header.X-Header}placeholders in CEL expressionsisTransportError()expression+ old syntax (I mean just a plain oldlb_retry_matchoptions) isn't allowedIt wasn't possible to implement it like
lb_retry_match `path('/foo*') && {rp.status_code} in [502, 503]`because of howlb_retry_matchis handled, but you can use a oneliner likelb_retry_match expression `path('/foo*') && {rp.status_code} in [502, 503]`Everything is covered with unit-tests, e2e tests and caddyfile tests.
lb_retry_matchoptions, some things not, and sometimes it's unclear whether something would be retried or not.For example, the previous logic:
GETloses its' default transport error retry becauseRetryMatchis non-nil and onlyPUTis defined. Addingexpressionhandling would result into even more unclear logic. That's why usingexpressionwith plain options isn't allowed.Just to remind what transport error is (roughly):
Now examples of how the new
expressioncan be used: