Addressing review

Author: seroperson

caddytest/integration/caddyfile_adapt/reverse_proxy_retry_match_oneline.caddyfiletest

@@ -3,8 +3,10 @@
 reverse_proxy 127.0.0.1:65535 {
 	lb_retries 3
 	lb_retry_match expression `{rp.status_code} in [502, 503]`
-	lb_retry_match expression `isTransportError() || {rp.status_code} == 502`
+	lb_retry_match expression `{rp.is_transport_error} || {rp.status_code} == 502`
 	lb_retry_match expression `method('POST') && {rp.status_code} == 503`
+	lb_retry_match `{rp.status_code} == 504`
+	lb_retry_match `{rp.is_transport_error} && method('PUT')`
 }
 ----------
 {
@@ -27,10 +29,16 @@ reverse_proxy 127.0.0.1:65535 {
 												"expression": "{http.reverse_proxy.status_code} in [502, 503]"
 											},
 											{
-												"expression": "isTransportError() || {http.reverse_proxy.status_code} == 502"
+												"expression": "{http.reverse_proxy.is_transport_error} || {http.reverse_proxy.status_code} == 502"
 											},
 											{
 												"expression": "method('POST') \u0026\u0026 {http.reverse_proxy.status_code} == 503"
+											},
+											{
+												"expression": "{http.reverse_proxy.status_code} == 504"
+											},
+											{
+												"expression": "{http.reverse_proxy.is_transport_error} \u0026\u0026 method('PUT')"
 											}
 										]
 									},

caddytest/integration/caddyfile_adapt/reverse_proxy_retry_match_response.caddyfiletest

@@ -50,12 +50,12 @@ reverse_proxy 127.0.0.1:65535 {
 		expression `header_regexp('Content-Type', '^application/json') && {rp.status_code} == 502`
 	}
 
-	# isTransportError() for transport error handling
+	# transport error handling via placeholder
 	lb_retry_match {
-		expression `isTransportError() || {rp.status_code} in [502, 503]`
+		expression `{rp.is_transport_error} || {rp.status_code} in [502, 503]`
 	}
 	lb_retry_match {
-		expression `isTransportError() && method('POST')`
+		expression `{rp.is_transport_error} && method('POST')`
 	}
 }
 ----------
@@ -124,10 +124,10 @@ reverse_proxy 127.0.0.1:65535 {
 												"expression": "header_regexp('Content-Type', '^application/json') \u0026\u0026 {http.reverse_proxy.status_code} == 502"
 											},
 											{
-												"expression": "isTransportError() || {http.reverse_proxy.status_code} in [502, 503]"
+												"expression": "{http.reverse_proxy.is_transport_error} || {http.reverse_proxy.status_code} in [502, 503]"
 											},
 											{
-												"expression": "isTransportError() \u0026\u0026 method('POST')"
+												"expression": "{http.reverse_proxy.is_transport_error} \u0026\u0026 method('POST')"
 											}
 										]
 									},

caddytest/integration/reverseproxy_test.go

@@ -738,7 +738,7 @@ func TestReverseProxyRetryMatchCombined(t *testing.T) {
 }
 
 // TestReverseProxyRetryMatchIsTransportError verifies that the
-// isTransportError() CEL function correctly identifies transport errors
+// {rp.is_transport_error} == true CEL function correctly identifies transport errors
 // and allows retrying them alongside response-based matching
 func TestReverseProxyRetryMatchIsTransportError(t *testing.T) {
 	// Good upstream: returns 200
@@ -784,7 +784,7 @@ func TestReverseProxyRetryMatchIsTransportError(t *testing.T) {
 			lb_policy round_robin
 			lb_retries 1
 			lb_retry_match {
-				expression `+"`isTransportError() || {rp.status_code} in [502, 503]`"+`
+				expression `+"`{rp.is_transport_error} || {rp.status_code} in [502, 503]`"+`
 			}
 		}
 	}

modules/caddyhttp/celmatcher.go

@@ -126,10 +126,6 @@ func (m *MatchExpression) Provision(ctx caddy.Context) error {
 	// light (and possibly naïve) syntactic sugar
 	m.expandedExpr = placeholderRegexp.ReplaceAllString(m.Expr, placeholderExpansion)
 
-	// rewrite isTransportError() to isTransportError(req) so the
-	// function receives the request context
-	m.expandedExpr = transportErrorRegexp.ReplaceAllString(m.expandedExpr, transportErrorExpansion)
-
 	// as a second pass, we'll strip the escape character from an escaped
 	// placeholder, so that it can be used as an input to other CEL functions
 	m.expandedExpr = escapedPlaceholderRegexp.ReplaceAllString(m.expandedExpr, escapedPlaceholderExpansion)
@@ -172,11 +168,6 @@ func (m *MatchExpression) Provision(ctx caddy.Context) error {
 			[]*cel.Type{httpRequestObjectType, cel.StringType},
 			cel.AnyType,
 		)),
-		cel.Function(CELTransportErrorFuncName, cel.SingletonUnaryBinding(isTransportErrorFunc), cel.Overload(
-			CELTransportErrorFuncName+"_httpRequest",
-			[]*cel.Type{httpRequestObjectType},
-			cel.BoolType,
-		)),
 		cel.Variable(CELRequestVarName, httpRequestObjectType),
 		cel.CustomTypeAdapter(m.ta),
 		ext.Strings(),
@@ -218,10 +209,6 @@ func (m MatchExpression) Match(r *http.Request) bool {
 	return match
 }
 
-// CanMatchResponse is a marker indicating that expression matchers
-// may reference response data via placeholders like {rp.status_code}
-func (m MatchExpression) CanMatchResponse() {}
-
 // MatchWithError returns true if r matches m.
 func (m MatchExpression) MatchWithError(r *http.Request) (bool, error) {
 	celReq := celHTTPRequest{r}
@@ -293,29 +280,6 @@ func (m MatchExpression) caddyPlaceholderFunc(lhs, rhs ref.Val) ref.Val {
 	return m.ta.NativeToValue(val)
 }
 
-// isTransportErrorFunc implements the isTransportError() CEL function
-// that returns true when the evaluation is happening during a transport
-// error retry decision (as opposed to a response-based retry decision)
-func isTransportErrorFunc(val ref.Val) ref.Val {
-	celReq, ok := val.(celHTTPRequest)
-	if !ok {
-		return types.NewErr(
-			"invalid request of type '%v' to %s(request)",
-			val.Type(),
-			CELTransportErrorFuncName,
-		)
-	}
-	repl, ok := celReq.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
-	if !ok || repl == nil {
-		return types.Bool(false)
-	}
-	v, _ := repl.Get("http.reverse_proxy.is_transport_error")
-	if b, ok := v.(bool); ok {
-		return types.Bool(b)
-	}
-	return types.Bool(false)
-}
-
 // httpRequestCELType is the type representation of a native HTTP request.
 var httpRequestCELType = cel.ObjectType("http.Request", traits.ReceiverType)
 
@@ -850,10 +814,6 @@ var (
 	placeholderRegexp    = regexp.MustCompile(`([^\\]|^){([a-zA-Z][\w.-]+)}`)
 	placeholderExpansion = `${1}ph(req, "${2}")`
 
-	// rewrite isTransportError() calls to pass the request object
-	transportErrorRegexp    = regexp.MustCompile(`isTransportError\(\)`)
-	transportErrorExpansion = `isTransportError(req)`
-
 	// As a second pass, we need to strip the escape character in front of
 	// the placeholder, if it exists.
 	escapedPlaceholderRegexp    = regexp.MustCompile(`\\{([a-zA-Z][\w.-]+)}`)
@@ -867,10 +827,6 @@ var httpRequestObjectType = cel.ObjectType("http.Request")
 // The name of the CEL function which accesses Replacer values.
 const CELPlaceholderFuncName = "ph"
 
-// The name of the CEL function that checks if the current evaluation
-// context is a transport error (connection succeeded but roundtrip failed)
-const CELTransportErrorFuncName = "isTransportError"
-
 // The name of the CEL request variable.
 const CELRequestVarName = "req"
 

modules/caddyhttp/matchers.go

@@ -1562,6 +1562,14 @@ func ParseCaddyfileNestedMatcherSet(d *caddyfile.Dispenser) (caddy.ModuleMap, er
 	// instances of the matcher in this set
 	tokensByMatcherName := make(map[string][]caddyfile.Token)
 	for nesting := d.Nesting(); d.NextArg() || d.NextBlock(nesting); {
+		// if the token is quoted (backtick), treat it as a shorthand
+		// for an expression matcher, same as @named matcher parsing
+		if d.Token().Quoted() {
+			expressionToken := d.Token().Clone()
+			expressionToken.Text = "expression"
+			tokensByMatcherName["expression"] = append(tokensByMatcherName["expression"], expressionToken, d.Token())
+			continue
+		}
 		matcherName := d.Val()
 		tokensByMatcherName[matcherName] = append(tokensByMatcherName[matcherName], d.NextSegment()...)
 	}

modules/caddyhttp/reverseproxy/caddyfile.go

@@ -327,13 +327,6 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 			if err != nil {
 				return d.Errf("failed to parse lb_retry_match: %v", err)
 			}
-			// expression matchers and non-expression matchers cannot be
-			// mixed in the same block - expression matchers evaluate
-			// against responses while non-expression matchers evaluate
-			// against transport errors
-			if _, hasExpr := matcherSet["expression"]; hasExpr && len(matcherSet) > 1 {
-				return d.Errf("lb_retry_match: expression cannot be mixed with other matchers in the same block; use either an expression or request matchers, not both")
-			}
 			if h.LoadBalancing == nil {
 				h.LoadBalancing = new(LoadBalancing)
 			}