From be7abda7d498272e360fe6fae18984c309533de3 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Sat, 5 Oct 2019 16:22:05 -0600 Subject: reverse_proxy: Implement retry_match; by default only retry GET requests See https://caddy.community/t/http-proxy-and-non-get-retries/6304 --- modules/caddyhttp/reverseproxy/httptransport.go | 9 +++- modules/caddyhttp/reverseproxy/reverseproxy.go | 69 ++++++++++++++++++------- 2 files changed, 58 insertions(+), 20 deletions(-) (limited to 'modules') diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go index 00b58a4..4ff9989 100644 --- a/modules/caddyhttp/reverseproxy/httptransport.go +++ b/modules/caddyhttp/reverseproxy/httptransport.go @@ -79,7 +79,14 @@ func (h *HTTPTransport) Provision(_ caddy.Context) error { network = dialInfo.Network address = dialInfo.Address } - return dialer.DialContext(ctx, network, address) + conn, err := dialer.DialContext(ctx, network, address) + if err != nil { + // identify this error as one that occurred during + // dialing, which can be important when trying to + // decide whether to retry a request + return nil, DialError{err} + } + return conn, nil }, MaxConnsPerHost: h.MaxConnsPerHost, ResponseHeaderTimeout: time.Duration(h.ResponseHeaderTimeout), diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 485d14e..a733d68 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -115,6 +115,12 @@ 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 := h.LoadBalancing.RetryMatchRaw.Setup(ctx) + if err != nil { + return err + } + h.LoadBalancing.RetryMatch = lbMatcherSets + h.LoadBalancing.RetryMatchRaw = nil // allow GC to deallocate // if active health checks are enabled, configure them and start a worker if h.HealthChecks != nil && @@ -270,7 +276,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht if proxyErr == nil { proxyErr = fmt.Errorf("no upstreams available") } - if !h.tryAgain(start, proxyErr) { + if !h.LoadBalancing.tryAgain(start, proxyErr, r) { break } continue @@ -306,7 +312,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht proxyErr = h.reverseProxy(w, r, upstream) if proxyErr == nil || proxyErr == context.Canceled { // context.Canceled happens when the downstream client - // cancels the request; we don't have to worry about that + // cancels the request, which is not our failure return nil } @@ -314,7 +320,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht h.countFailure(upstream) // if we've tried long enough, break - if !h.tryAgain(start, proxyErr) { + if !h.LoadBalancing.tryAgain(start, proxyErr, r) { break } } @@ -510,22 +516,39 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, upstre } // tryAgain takes the time that the handler was initially invoked -// as well as any error currently obtained and returns true if -// another attempt should be made at proxying the request. If -// true is returned, it has already blocked long enough before -// the next retry (i.e. no more sleeping is needed). If false is -// returned, the handler should stop trying to proxy the request. -func (h Handler) tryAgain(start time.Time, proxyErr error) bool { - // if downstream has canceled the request, break - if proxyErr == context.Canceled { - return false - } +// as well as any error currently obtained, and the request being +// tried, and returns true if another attempt should be made at +// proxying the request. If true is returned, it has already blocked +// long enough before the next retry (i.e. no more sleeping is +// needed). If false is returned, the handler should stop trying to +// proxy the request. +func (lb LoadBalancing) tryAgain(start time.Time, proxyErr error, req *http.Request) bool { // if we've tried long enough, break - if time.Since(start) >= time.Duration(h.LoadBalancing.TryDuration) { + if time.Since(start) >= time.Duration(lb.TryDuration) { return false } + + // if the error occurred while dialing (i.e. a connection + // could not even be established to the upstream), then it + // should be safe to retry, since without a connection, no + // HTTP request can be transmitted; but if the error is not + // specifically a dialer error, we need to be careful + if _, ok := proxyErr.(DialError); proxyErr != nil && !ok { + // if the error occurred after a connection was established, + // we have to assume the upstream received the request, and + // retries need to be carefully decided, because some requests + // are not idempotent + if lb.RetryMatch == nil && req.Method != "GET" { + // by default, don't retry requests if they aren't GET + return false + } + if !lb.RetryMatch.AnyMatch(req) { + return false + } + } + // otherwise, wait and try the next available host - time.Sleep(time.Duration(h.LoadBalancing.TryInterval)) + time.Sleep(time.Duration(lb.TryInterval)) return true } @@ -621,11 +644,13 @@ func removeConnectionHeaders(h http.Header) { // LoadBalancing has parameters related to load balancing. type LoadBalancing struct { - SelectionPolicyRaw json.RawMessage `json:"selection_policy,omitempty"` - TryDuration caddy.Duration `json:"try_duration,omitempty"` - TryInterval caddy.Duration `json:"try_interval,omitempty"` + SelectionPolicyRaw json.RawMessage `json:"selection_policy,omitempty"` + TryDuration caddy.Duration `json:"try_duration,omitempty"` + TryInterval caddy.Duration `json:"try_interval,omitempty"` + RetryMatchRaw caddyhttp.RawMatcherSets `json:"retry_match,omitempty"` - SelectionPolicy Selector `json:"-"` + SelectionPolicy Selector `json:"-"` + RetryMatch caddyhttp.MatcherSets `json:"-"` } // Selector selects an available upstream from the pool. @@ -650,6 +675,12 @@ var hopHeaders = []string{ "Upgrade", } +// DialError is an error that specifically occurs +// in a call to Dial or DialContext. +type DialError struct { + error +} + // TODO: see if we can use this // var bufPool = sync.Pool{ // New: func() interface{} { -- cgit v1.2.3