reverseproxy: add lb_retry_match condition on response status#7569
reverseproxy: add lb_retry_match condition on response status#7569seroperson wants to merge 2 commits intocaddyserver:masterfrom
lb_retry_match condition on response status#7569Conversation
|
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
caddytest/integration/caddyfile_adapt/reverse_proxy_retry_match_oneline.caddyfiletest
Show resolved
Hide resolved
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
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: