Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
Mirrored from https://github.com/Slazee/caddy.git
Synced Nov 11, 03:30 UTC
reverseproxy: Only retry responses for expressions that use response 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.
Jay Hemnani committed
Oct 10, 2026 at 02:52 UTC
7c7456353e52d1f6056f22538b1eb05b02708e57
3 files changed
+165
-50
modules/caddyhttp/celmatcher.go
+28
@@ -22,6 +22,7 @@ import (
22
"net/http"
23
"reflect"
24
"regexp"
25
+ "slices"
26
"strings"
27
"time"
28
@@ -70,6 +71,7 @@ type MatchExpression struct {
71
Name string `json:"name,omitempty"`
72
73
expandedExpr string
74
+ placeholders []string
75
prg cel.Program
76
ta types.Adapter
77
@@ -192,6 +194,8 @@ func (m *MatchExpression) Provision(ctx caddy.Context) error {
194
return fmt.Errorf("CEL request matcher expects return type of bool, not %s", checked.OutputType())
195
}
196
197
+ m.placeholders = celPlaceholderNames(checked.NativeRep().Expr())
198
+
199
// compile the "program"
200
m.prg, err = env.Program(checked, cel.EvalOptions(cel.OptOptimize))
201
if err != nil {
@@ -200,6 +204,13 @@ func (m *MatchExpression) Provision(ctx caddy.Context) error {
204
return nil
205
}
206
207
+// Placeholders returns the names of the placeholders the provisioned
208
+// expression reads. A name that is only known at evaluation time is
209
+// returned as an empty string.
210
+func (m MatchExpression) Placeholders() []string {
211
+ return slices.Clone(m.placeholders)
212
+}
213
+
214
// Match returns true if r matches m.
215
func (m MatchExpression) Match(r *http.Request) bool {
216
match, err := m.MatchWithError(r)
@@ -729,6 +740,23 @@ func CELValueToMapStrList(data ref.Val) (map[string][]string, error) {
740
return mapStrListStr, nil
741
}
742
743
+// celPlaceholderNames returns the name passed to each placeholder call in e,
744
+// or an empty string where the name is not a string literal.
745
+func celPlaceholderNames(e ast.Expr) []string {
746
+ var names []string
747
+ ast.PreOrderVisit(e, ast.NewExprVisitor(func(e ast.Expr) {
748
+ if !isCELCaddyPlaceholderCall(e) {
749
+ return
750
+ }
751
+ name := ""
752
+ if args := e.AsCall().Args(); len(args) == 2 && isCELStringLiteral(args[1]) {
753
+ name = args[1].AsLiteral().Value().(string)
754
+ }
755
+ names = append(names, name)
756
+ }))
757
+ return names
758
+}
759
+
760
// isCELStringExpr indicates whether the expression is a supported string expression
761
func isCELStringExpr(e ast.Expr) bool {
762
return isCELStringLiteral(e) || isCELCaddyPlaceholderCall(e) || isCELConcatCall(e)
modules/caddyhttp/reverseproxy/retries_test.go
+101
-36
@@ -283,50 +283,57 @@ func minimalHandlerWithRetryMatch(retries int, retryMatch caddyhttp.MatcherSets,
283
// code matching a retry_match expression, the request is retried on the next
284
// upstream
285
func TestResponseRetryStatusCode(t *testing.T) {
286
- // Bad upstream: returns 502
287
- badServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
288
- w.WriteHeader(http.StatusBadGateway)
289
- }))
290
- t.Cleanup(badServer.Close)
286
+ for _, expr := range []string{
287
+ "{http.reverse_proxy.status_code} in [502, 503]",
288
+ `ph(req, "http.reverse_proxy.status_code") in [502, 503]`,
289
+ } {
290
+ t.Run(expr, func(t *testing.T) {
291
+ // Bad upstream: returns 502
292
+ badServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
293
+ w.WriteHeader(http.StatusBadGateway)
294
+ }))
295
+ t.Cleanup(badServer.Close)
296
292
- // Good upstream: returns 200
293
- goodServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
294
- w.WriteHeader(http.StatusOK)
295
- w.Write([]byte("ok"))
296
- }))
297
- t.Cleanup(goodServer.Close)
297
+ // Good upstream: returns 200
298
+ goodServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
299
+ w.WriteHeader(http.StatusOK)
300
+ w.Write([]byte("ok"))
301
+ }))
302
+ t.Cleanup(goodServer.Close)
303
299
- retryMatch := caddyhttp.MatcherSets{
300
- caddyhttp.MatcherSet{
301
- newExpressionMatcher(t, "{http.reverse_proxy.status_code} in [502, 503]"),
302
- },
303
- }
304
+ retryMatch := caddyhttp.MatcherSets{
305
+ caddyhttp.MatcherSet{
306
+ newExpressionMatcher(t, expr),
307
+ },
308
+ }
309
305
- // RoundRobin picks index 1 first, then 0
306
- upstreams := []*Upstream{
307
- {Host: new(Host), Dial: goodServer.Listener.Addr().String()},
308
- {Host: new(Host), Dial: badServer.Listener.Addr().String()},
309
- }
310
+ // RoundRobin picks index 1 first, then 0
311
+ upstreams := []*Upstream{
312
+ {Host: new(Host), Dial: goodServer.Listener.Addr().String()},
313
+ {Host: new(Host), Dial: badServer.Listener.Addr().String()},
314
+ }
315
311
- h := minimalHandlerWithRetryMatch(1, retryMatch, upstreams...)
316
+ h := minimalHandlerWithRetryMatch(1, retryMatch, upstreams...)
317
313
- req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil)
314
- req = prepareTestRequest(req)
315
- rec := httptest.NewRecorder()
318
+ req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil)
319
+ req = prepareTestRequest(req)
320
+ rec := httptest.NewRecorder()
321
317
- err := h.ServeHTTP(rec, req, caddyhttp.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
318
- return nil
319
- }))
322
+ err := h.ServeHTTP(rec, req, caddyhttp.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
323
+ return nil
324
+ }))
325
321
- gotStatus := rec.Code
322
- if err != nil {
323
- if herr, ok := err.(caddyhttp.HandlerError); ok {
324
- gotStatus = herr.StatusCode
325
- }
326
- }
326
+ gotStatus := rec.Code
327
+ if err != nil {
328
+ if herr, ok := err.(caddyhttp.HandlerError); ok {
329
+ gotStatus = herr.StatusCode
330
+ }
331
+ }
332
328
- if gotStatus != http.StatusOK {
329
- t.Errorf("status: got %d, want %d (err=%v)", gotStatus, http.StatusOK, err)
333
+ if gotStatus != http.StatusOK {
334
+ t.Errorf("status: got %d, want %d (err=%v)", gotStatus, http.StatusOK, err)
335
+ }
336
+ })
337
}
338
}
339
@@ -565,6 +572,64 @@ func TestRequestOnlyMatcherDoesNotRetryResponses(t *testing.T) {
572
}
573
}
574
575
+// TestRequestOnlyExpressionDoesNotRetryResponses verifies that an expression
576
+// matcher that only tests the request or the selected upstream (no response
577
+// placeholders) does not cause successful responses to be retried
578
+func TestRequestOnlyExpressionDoesNotRetryResponses(t *testing.T) {
579
+ for _, expr := range []string{
580
+ `{http.request.method} == "POST"`,
581
+ `{http.reverse_proxy.upstream.host} == "127.0.0.1"`,
582
+ `{http.request.uri.path} != "\{http.reverse_proxy.status_code}"`,
583
+ `{http.reverse_proxy.status_codes} == null`,
584
+ } {
585
+ t.Run(expr, func(t *testing.T) {
586
+ var hits atomic.Int32
587
+
588
+ // Server returns 200 OK for all requests
589
+ server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
590
+ hits.Add(1)
591
+ w.WriteHeader(http.StatusOK)
592
+ w.Write([]byte("ok"))
593
+ }))
594
+ t.Cleanup(server.Close)
595
+
596
+ retryMatch := caddyhttp.MatcherSets{
597
+ caddyhttp.MatcherSet{
598
+ newExpressionMatcher(t, expr),
599
+ },
600
+ }
601
+
602
+ upstreams := []*Upstream{
603
+ {Host: new(Host), Dial: server.Listener.Addr().String()},
604
+ {Host: new(Host), Dial: server.Listener.Addr().String()},
605
+ }
606
+
607
+ h := minimalHandlerWithRetryMatch(2, retryMatch, upstreams...)
608
+
609
+ req := httptest.NewRequest(http.MethodPost, "http://example.com/", strings.NewReader("order=1"))
610
+ req = prepareTestRequest(req)
611
+ rec := httptest.NewRecorder()
612
+
613
+ err := h.ServeHTTP(rec, req, caddyhttp.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
614
+ return nil
615
+ }))
616
+ if err != nil {
617
+ t.Fatalf("unexpected error: %v", err)
618
+ }
619
+
620
+ if hits.Load() != 1 {
621
+ t.Errorf("upstream hits: got %d, want 1 (should not retry successful responses)", hits.Load())
622
+ }
623
+ if rec.Code != http.StatusOK {
624
+ t.Errorf("status: got %d, want %d", rec.Code, http.StatusOK)
625
+ }
626
+ if rec.Body.String() != "ok" {
627
+ t.Errorf("body: got %q, want %q", rec.Body.String(), "ok")
628
+ }
629
+ })
630
+ }
631
+}
632
+
633
// brokenUpstreamAddr returns the address of a TCP listener that accepts
634
// connections but immediately closes them, causing a transport error (not
635
// a dial error). This simulates an upstream that is reachable but broken
modules/caddyhttp/reverseproxy/reverseproxy.go
+36
-14
@@ -30,6 +30,7 @@ import (
30
"net/netip"
31
"net/textproto"
32
"net/url"
33
+ "slices"
34
"strconv"
35
"strings"
36
"sync"
@@ -1279,14 +1280,13 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, origRe
1280
// check if the response matches a retry match entry; if so,
1281
// close the body and return a retryable error so the request
1282
// is retried with the next upstream. Only evaluate matcher sets
1282
- // that contain at least one expression matcher, since those are
1283
- // the ones that can reference response data ({rp.status_code},
1284
- // {rp.header.*}). Pure request-only matchers (method, path, etc.)
1285
- // are skipped to avoid retrying every response that matches a
1286
- // request condition
1283
+ // with an expression that references response data ({rp.status_code},
1284
+ // {rp.header.*}). Request-only matchers (method, path, or an
1285
+ // expression without response placeholders) are skipped to avoid
1286
+ // retrying every response that matches a request condition
1287
if h.LoadBalancing != nil && len(h.LoadBalancing.RetryMatch) > 0 {
1288
for _, matcherSet := range h.LoadBalancing.RetryMatch {
1289
- if !matcherSetHasExpressionMatcher(matcherSet) {
1289
+ if !matcherSetReferencesResponse(matcherSet) {
1290
continue
1291
}
1292
match, err := matcherSet.MatchWithError(req)
@@ -2135,15 +2135,37 @@ type RequestHeaderOpsTransport interface {
2135
RequestHeaderOps() *headers.HeaderOps
2136
}
2137
2138
-// matcherSetHasExpressionMatcher reports whether a matcher set contains
2139
-// at least one expression matcher. Expression matchers can reference
2140
-// response data via placeholders like {rp.status_code}. Matcher sets
2141
-// without expression matchers only test request properties and should
2142
-// not be evaluated for response-based retry decisions
2143
-func matcherSetHasExpressionMatcher(matcherSet caddyhttp.MatcherSet) bool {
2138
+// responsePlaceholders are the placeholders that are only set once the
2139
+// upstream round trip completes ({rp.status_code} is adapted to
2140
+// {http.reverse_proxy.status_code}, and so on)
2141
+var responsePlaceholders = []string{
2142
+ "http.reverse_proxy.status_code",
2143
+ "http.reverse_proxy.status_text",
2144
+ "http.reverse_proxy.upstream.latency",
2145
+ "http.reverse_proxy.upstream.latency_ms",
2146
+}
2147
+
2148
+// responseHeaderPlaceholderPrefix is the prefix of the response
2149
+// header placeholders, {http.reverse_proxy.header.*}
2150
+const responseHeaderPlaceholderPrefix = "http.reverse_proxy.header."
2151
+
2152
+// matcherSetReferencesResponse reports whether a matcher set contains an
2153
+// expression matcher that reads response data, or a placeholder whose name
2154
+// is only known at evaluation time. Other matcher sets only test request or
2155
+// upstream selection properties and should not be evaluated for
2156
+// response-based retry decisions
2157
+func matcherSetReferencesResponse(matcherSet caddyhttp.MatcherSet) bool {
2158
for _, m := range matcherSet {
2145
- if _, ok := m.(*caddyhttp.MatchExpression); ok {
2146
- return true
2159
+ expr, ok := m.(*caddyhttp.MatchExpression)
2160
+ if !ok {
2161
+ continue
2162
+ }
2163
+ for _, name := range expr.Placeholders() {
2164
+ if name == "" ||
2165
+ slices.Contains(responsePlaceholders, name) ||
2166
+ strings.HasPrefix(name, responseHeaderPlaceholderPrefix) {
2167
+ return true
2168
+ }
2169
}
2170
}
2171
return false