reverse_proxy: deprecate RetryMatchRaw in favor of RetryConditionsRaw

Author: seroperson

modules/caddyhttp/reverseproxy/caddyfile.go

@@ -333,14 +333,13 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 			if err != nil {
 				return err
 			}
-			if condSet != nil {
-				if matcherSet != nil {
-					condSet.MatchRaw = matcherSet
-				}
-				h.LoadBalancing.RetryConditionsRaw = append(h.LoadBalancing.RetryConditionsRaw, condSet)
-			} else if matcherSet != nil {
-				h.LoadBalancing.RetryMatchRaw = append(h.LoadBalancing.RetryMatchRaw, matcherSet)
+			if condSet == nil {
+				condSet = new(RetryConditionSet)
+			}
+			if matcherSet != nil {
+				condSet.MatchRaw = matcherSet
 			}
+			h.LoadBalancing.RetryConditionsRaw = append(h.LoadBalancing.RetryConditionsRaw, condSet)
 
 		case "health_uri":
 			if !d.NextArg() {

modules/caddyhttp/reverseproxy/retries_test.go

@@ -499,3 +499,72 @@ func TestLbRetryMatchCaddyfileParsing(t *testing.T) {
 		})
 	}
 }
+
+func TestLbRetryMatchBackwardCompatibility(t *testing.T) {
+	// Plain lb_retry_match blocks (without status) should go into
+	// RetryConditionsRaw with matchers but no status codes, preserving
+	// the pre-existing behavior.
+	t.Run("plain matcher block goes into RetryConditionsRaw", func(t *testing.T) {
+		d := caddyfile.NewTestDispenser(`reverse_proxy localhost:8080 {
+			lb_retry_match {
+				method POST
+			}
+		}`)
+		h := new(Handler)
+		if err := h.UnmarshalCaddyfile(d); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if h.LoadBalancing == nil {
+			t.Fatal("LoadBalancing is nil")
+		}
+		if len(h.LoadBalancing.RetryMatchRaw) != 0 {
+			t.Errorf("RetryMatchRaw should be empty, got %d entries", len(h.LoadBalancing.RetryMatchRaw))
+		}
+		if len(h.LoadBalancing.RetryConditionsRaw) != 1 {
+			t.Fatalf("RetryConditionsRaw length: got %d, want 1", len(h.LoadBalancing.RetryConditionsRaw))
+		}
+		rc := h.LoadBalancing.RetryConditionsRaw[0]
+		if len(rc.Status) != 0 {
+			t.Errorf("Status should be empty, got %v", rc.Status)
+		}
+		if len(rc.MatchRaw) == 0 {
+			t.Fatal("MatchRaw should contain the method matcher")
+		}
+		if _, ok := rc.MatchRaw["method"]; !ok {
+			t.Errorf("MatchRaw should have 'method' key, got keys: %v", rc.MatchRaw)
+		}
+	})
+
+	// Multiple blocks: plain + status, all go into RetryConditionsRaw
+	t.Run("mixed blocks all go into RetryConditionsRaw", func(t *testing.T) {
+		d := caddyfile.NewTestDispenser(`reverse_proxy localhost:8080 {
+			lb_retry_match {
+				method PUT
+			}
+			lb_retry_match {
+				status 503
+			}
+		}`)
+		h := new(Handler)
+		if err := h.UnmarshalCaddyfile(d); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(h.LoadBalancing.RetryMatchRaw) != 0 {
+			t.Errorf("RetryMatchRaw should be empty, got %d entries", len(h.LoadBalancing.RetryMatchRaw))
+		}
+		if len(h.LoadBalancing.RetryConditionsRaw) != 2 {
+			t.Fatalf("RetryConditionsRaw length: got %d, want 2", len(h.LoadBalancing.RetryConditionsRaw))
+		}
+		// Block 1: plain matcher, no status
+		if len(h.LoadBalancing.RetryConditionsRaw[0].Status) != 0 {
+			t.Errorf("block 0 should have no status codes")
+		}
+		if _, ok := h.LoadBalancing.RetryConditionsRaw[0].MatchRaw["method"]; !ok {
+			t.Errorf("block 0 should have method matcher")
+		}
+		// Block 2: status, no matchers
+		if len(h.LoadBalancing.RetryConditionsRaw[1].Status) != 1 || h.LoadBalancing.RetryConditionsRaw[1].Status[0] != 503 {
+			t.Errorf("block 1 should have status [503], got %v", h.LoadBalancing.RetryConditionsRaw[1].Status)
+		}
+	})
+}

modules/caddyhttp/reverseproxy/reverseproxy.go

@@ -382,14 +382,14 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 		// defaulting to a sane wait period between attempts
 		h.LoadBalancing.TryInterval = caddy.Duration(250 * time.Millisecond)
 	}
-	lbMatcherSets, err := ctx.LoadModule(h.LoadBalancing, "RetryMatchRaw")
-	if err != nil {
-		return err
-	}
-	err = h.LoadBalancing.RetryMatch.FromInterface(lbMatcherSets)
-	if err != nil {
-		return err
+	// convert legacy retry_match entries into retry_conditions for
+	// backward compatibility with the JSON API
+	for _, matcherSetRaw := range h.LoadBalancing.RetryMatchRaw {
+		h.LoadBalancing.RetryConditionsRaw = append(h.LoadBalancing.RetryConditionsRaw, &RetryConditionSet{
+			MatchRaw: matcherSetRaw,
+		})
 	}
+	h.LoadBalancing.RetryMatchRaw = nil
 
 	// provision retry conditions
 	for i, rc := range h.LoadBalancing.RetryConditionsRaw {
@@ -1325,16 +1325,10 @@ func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int
 		if !isDialError && (!isHandlerError || !errors.Is(herr, errNoUpstream)) {
 			// retryableStatusError is always safe to retry (we control when it's generated)
 			if _, ok := proxyErr.(retryableStatusError); !ok {
-				// default behavior: retry GET or RetryMatch
-				if lb.RetryMatch == nil && req.Method != "GET" {
+				if len(lb.retryConditions) == 0 && req.Method != "GET" {
 					return false
 				}
-				match, err := lb.RetryMatch.AnyMatchWithError(req)
-				if err != nil {
-					logger.Error("error matching request for retry", zap.Error(err))
-					return false
-				}
-				if !match {
+				if len(lb.retryConditions) > 0 && !lb.matchesRetryCondition(req, logger) {
 					return false
 				}
 			}
@@ -1360,6 +1354,26 @@ func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int
 	}
 }
 
+// matchesRetryCondition checks whether any configured retry condition's
+// request matchers match the given request. If a condition has no matchers,
+// any request matches it.
+func (lb LoadBalancing) matchesRetryCondition(req *http.Request, logger *zap.Logger) bool {
+	for _, rc := range lb.retryConditions {
+		if len(rc.matchers) == 0 {
+			return true
+		}
+		match, err := rc.matchers.MatchWithError(req)
+		if err != nil {
+			logger.Error("error matching request for retry condition", zap.Error(err))
+			continue
+		}
+		if match {
+			return true
+		}
+	}
+	return false
+}
+
 // directRequest modifies only req.URL so that it points to the upstream
 // in the given DialInfo. It must modify ONLY the request URL.
 func (h *Handler) directRequest(req *http.Request, di DialInfo) {
@@ -1610,23 +1624,23 @@ type LoadBalancing struct {
 	// to spin if all backends are down and latency is very low.
 	TryInterval caddy.Duration `json:"try_interval,omitempty"`
 
-	// A list of matcher sets that restricts with which requests retries are
-	// allowed. A request must match any of the given matcher sets in order
-	// to be retried if the connection to the upstream succeeded but the
-	// subsequent round-trip failed. If the connection to the upstream failed,
-	// a retry is always allowed. If unspecified, only GET requests will be
-	// allowed to be retried. Note that a retry is done with the next available
-	// host according to the load balancing policy.
+	// Deprecated: Use RetryConditionsRaw instead. Kept for backward
+	// compatibility with the JSON API. Entries are converted to
+	// RetryConditionsRaw during provisioning.
 	RetryMatchRaw caddyhttp.RawMatcherSets `json:"retry_match,omitempty" caddy:"namespace=http.matchers"`
 
 	// A list of retry condition sets. Each set bundles request matchers
-	// with optional status codes. A retry is triggered if the upstream
-	// responds with a matching status code. Within a set, the request
-	// must also match the matchers (if any).
+	// with optional status codes. A request must match any of the given
+	// condition sets in order to be retried if the connection to the
+	// upstream succeeded but the subsequent round-trip failed. If the
+	// connection to the upstream failed, a retry is always allowed. If
+	// a condition has status codes, the upstream's response status must
+	// match one of them for a retry to be triggered. If unspecified,
+	// only GET requests will be retried. Note that a retry is done with
+	// the next available host according to the load balancing policy.
 	RetryConditionsRaw []*RetryConditionSet `json:"retry_conditions,omitempty"`
 
-	SelectionPolicy Selector              `json:"-"`
-	RetryMatch      caddyhttp.MatcherSets `json:"-"`
+	SelectionPolicy Selector `json:"-"`
 	retryConditions []*retryCondition
 }