From 203da7250478fac497c7fd04c6c4ade509aac552 Mon Sep 17 00:00:00 2001 From: Cory Cooper Date: Fri, 23 Sep 2022 23:50:15 -0700 Subject: [PATCH 1/5] httploader: Add max_retries --- caddyconfig/httploader.go | 25 +++++++++++++++++++++++-- go.mod | 2 ++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/caddyconfig/httploader.go b/caddyconfig/httploader.go index 80eab446978..7de9a8bad19 100644 --- a/caddyconfig/httploader.go +++ b/caddyconfig/httploader.go @@ -15,6 +15,7 @@ package caddyconfig import ( + "context" "crypto/tls" "crypto/x509" "fmt" @@ -24,6 +25,7 @@ import ( "time" "github.com/caddyserver/caddy/v2" + "github.com/hashicorp/go-retryablehttp" ) func init() { @@ -45,6 +47,9 @@ type HTTPLoader struct { // Maximum time allowed for a complete connection and request. Timeout caddy.Duration `json:"timeout,omitempty"` + // Maximum number of retries for a successful call to URL. Defaults to 0. + MaxRetries int `json:"max_retries,omitempty"` + TLS *struct { // Present this instance's managed remote identity credentials to the server. UseServerIdentity bool `json:"use_server_identity,omitempty"` @@ -119,9 +124,25 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { return result, nil } +func getRetryClient(ctx caddy.Context, maxRetries int, timeout caddy.Duration) (*http.Client, error) { + retryClient := retryablehttp.NewClient() + retryClient.RetryMax = maxRetries + + retryClient.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) { + if err != nil || (resp.StatusCode < 200 || resp.StatusCode > 499) { + return true, err + } + return false, err + } + retryClient.HTTPClient.Timeout = time.Duration(timeout) + + return retryClient.StandardClient(), nil +} + func (hl HTTPLoader) makeClient(ctx caddy.Context) (*http.Client, error) { - client := &http.Client{ - Timeout: time.Duration(hl.Timeout), + client, err := getRetryClient(ctx, hl.MaxRetries, hl.Timeout) + if err != nil { + return nil, err } if hl.TLS != nil { diff --git a/go.mod b/go.mod index afcd268b669..008090e1f75 100644 --- a/go.mod +++ b/go.mod @@ -41,6 +41,7 @@ require ( require ( github.com/golang/mock v1.6.0 // indirect + github.com/hashicorp/go-cleanhttp v0.5.2 // indirect golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect gopkg.in/yaml.v2 v2.4.0 // indirect ) @@ -73,6 +74,7 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/golang/snappy v0.0.4 // indirect github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect + github.com/hashicorp/go-retryablehttp v0.7.1 github.com/huandu/xstrings v1.3.2 // indirect github.com/imdario/mergo v0.3.12 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect From 6a795e26e572707bae94c31594233bc83a5b5f94 Mon Sep 17 00:00:00 2001 From: Cory Cooper Date: Sat, 24 Sep 2022 23:35:32 -0700 Subject: [PATCH 2/5] caddyconfig: dependency-free http config loading retries --- caddyconfig/httploader.go | 43 +++++++++++++++++++++++---------------- go.mod | 2 -- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/caddyconfig/httploader.go b/caddyconfig/httploader.go index 7de9a8bad19..5e79a9ef984 100644 --- a/caddyconfig/httploader.go +++ b/caddyconfig/httploader.go @@ -15,17 +15,16 @@ package caddyconfig import ( - "context" "crypto/tls" "crypto/x509" "fmt" "io" + "math" "net/http" "os" "time" "github.com/caddyserver/caddy/v2" - "github.com/hashicorp/go-retryablehttp" ) func init() { @@ -47,7 +46,8 @@ type HTTPLoader struct { // Maximum time allowed for a complete connection and request. Timeout caddy.Duration `json:"timeout,omitempty"` - // Maximum number of retries for a successful call to URL. Defaults to 0. + // Maximum number of retries for a successful call to URL. + // Does exponential back-offs (2...4...8...16... seconds) between retries. Defaults to 0. MaxRetries int `json:"max_retries,omitempty"` TLS *struct { @@ -99,7 +99,7 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { } } - resp, err := client.Do(req) + resp, err := doHttpCallWithRetries(client, req, hl.MaxRetries) if err != nil { return nil, err } @@ -124,25 +124,34 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { return result, nil } -func getRetryClient(ctx caddy.Context, maxRetries int, timeout caddy.Duration) (*http.Client, error) { - retryClient := retryablehttp.NewClient() - retryClient.RetryMax = maxRetries - - retryClient.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) { - if err != nil || (resp.StatusCode < 200 || resp.StatusCode > 499) { - return true, err +// Reattempts the http call using exponential back off when maxRetries is greater than 0. +func doHttpCallWithRetries(client *http.Client, request *http.Request, maxRetries int) (*http.Response, error) { + var resp *http.Response + var err error + + for retry := 0; retry <= maxRetries; retry++ { + if retry > 0 { + caddy.Log().Error(err.Error()) + exponentialBackOff := math.Pow(2, float64(retry)) + caddy.Log().Sugar().Infof("reattempting http load in %g seconds (%v retries remain)", exponentialBackOff, maxRetries-retry) + time.Sleep(time.Duration(exponentialBackOff) * time.Second) + } + resp, err = client.Do(request) + if err != nil { + err = fmt.Errorf("problem calling http loader url: %v", err) + } else if resp.StatusCode < 200 || resp.StatusCode > 499 { + err = fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode) + } else { + return resp, nil } - return false, err } - retryClient.HTTPClient.Timeout = time.Duration(timeout) - return retryClient.StandardClient(), nil + return resp, err } func (hl HTTPLoader) makeClient(ctx caddy.Context) (*http.Client, error) { - client, err := getRetryClient(ctx, hl.MaxRetries, hl.Timeout) - if err != nil { - return nil, err + client := &http.Client{ + Timeout: time.Duration(hl.Timeout), } if hl.TLS != nil { diff --git a/go.mod b/go.mod index 008090e1f75..afcd268b669 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,6 @@ require ( require ( github.com/golang/mock v1.6.0 // indirect - github.com/hashicorp/go-cleanhttp v0.5.2 // indirect golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect gopkg.in/yaml.v2 v2.4.0 // indirect ) @@ -74,7 +73,6 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/golang/snappy v0.0.4 // indirect github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect - github.com/hashicorp/go-retryablehttp v0.7.1 github.com/huandu/xstrings v1.3.2 // indirect github.com/imdario/mergo v0.3.12 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect From 9f257ea89d9ee838faf02467c9e86f3615ec67bc Mon Sep 17 00:00:00 2001 From: Cory Cooper Date: Mon, 3 Oct 2022 20:44:13 -0700 Subject: [PATCH 3/5] caddyconfig: support `retry_delay` in http loader --- caddyconfig/httploader.go | 55 ++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/caddyconfig/httploader.go b/caddyconfig/httploader.go index 5e79a9ef984..db218a98fa3 100644 --- a/caddyconfig/httploader.go +++ b/caddyconfig/httploader.go @@ -19,7 +19,6 @@ import ( "crypto/x509" "fmt" "io" - "math" "net/http" "os" "time" @@ -46,9 +45,9 @@ type HTTPLoader struct { // Maximum time allowed for a complete connection and request. Timeout caddy.Duration `json:"timeout,omitempty"` - // Maximum number of retries for a successful call to URL. - // Does exponential back-offs (2...4...8...16... seconds) between retries. Defaults to 0. - MaxRetries int `json:"max_retries,omitempty"` + // The number of seconds to wait in between attempts to call `URL`. + // Defaults to 0, indicating no retries. + RetryDelay time.Duration `json:"retry_delay,omitempty"` TLS *struct { // Present this instance's managed remote identity credentials to the server. @@ -99,7 +98,7 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { } } - resp, err := doHttpCallWithRetries(client, req, hl.MaxRetries) + resp, err := doHttpCallWithRetries(ctx, client, req, hl.RetryDelay) if err != nil { return nil, err } @@ -124,29 +123,31 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { return result, nil } -// Reattempts the http call using exponential back off when maxRetries is greater than 0. -func doHttpCallWithRetries(client *http.Client, request *http.Request, maxRetries int) (*http.Response, error) { - var resp *http.Response - var err error - - for retry := 0; retry <= maxRetries; retry++ { - if retry > 0 { - caddy.Log().Error(err.Error()) - exponentialBackOff := math.Pow(2, float64(retry)) - caddy.Log().Sugar().Infof("reattempting http load in %g seconds (%v retries remain)", exponentialBackOff, maxRetries-retry) - time.Sleep(time.Duration(exponentialBackOff) * time.Second) - } - resp, err = client.Do(request) - if err != nil { - err = fmt.Errorf("problem calling http loader url: %v", err) - } else if resp.StatusCode < 200 || resp.StatusCode > 499 { - err = fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode) - } else { - return resp, nil - } +// Reattempts the http call, waiting `retryDelay` seconds in between attempts. +func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http.Request, retryDelay time.Duration) (*http.Response, error) { + // make attempt + resp, err := client.Do(request) + if err != nil { + err = fmt.Errorf("problem calling http loader url: %v", err) + } else if resp.StatusCode < 200 || resp.StatusCode > 499 { + err = fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode) + } else { + return resp, nil + } + // skip retries if valid retryDelay not provided + if retryDelay <= 0 { + return resp, err + } + // log the error + caddy.Log().Error(err.Error()) + // wait for the retry delay to lapse before reattempting, + // or return when context is done + select { + case <-time.After(time.Second * retryDelay): + return doHttpCallWithRetries(ctx, client, request, retryDelay) + case <-ctx.Done(): + return nil, err } - - return resp, err } func (hl HTTPLoader) makeClient(ctx caddy.Context) (*http.Client, error) { From 5eb13cc6b5785582cc0239ca71a6aba2294fdd50 Mon Sep 17 00:00:00 2001 From: Cory Cooper Date: Wed, 5 Oct 2022 12:55:58 -0700 Subject: [PATCH 4/5] httploader: Implement retries --- caddyconfig/httploader.go | 50 +++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/caddyconfig/httploader.go b/caddyconfig/httploader.go index db218a98fa3..a9e954b79de 100644 --- a/caddyconfig/httploader.go +++ b/caddyconfig/httploader.go @@ -45,10 +45,6 @@ type HTTPLoader struct { // Maximum time allowed for a complete connection and request. Timeout caddy.Duration `json:"timeout,omitempty"` - // The number of seconds to wait in between attempts to call `URL`. - // Defaults to 0, indicating no retries. - RetryDelay time.Duration `json:"retry_delay,omitempty"` - TLS *struct { // Present this instance's managed remote identity credentials to the server. UseServerIdentity bool `json:"use_server_identity,omitempty"` @@ -98,7 +94,8 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { } } - resp, err := doHttpCallWithRetries(ctx, client, req, hl.RetryDelay) + caddy.Log().Info("attempting to load config via http loader url") + resp, err := doHttpCallWithRetries(ctx, client, req) if err != nil { return nil, err } @@ -123,31 +120,38 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { return result, nil } -// Reattempts the http call, waiting `retryDelay` seconds in between attempts. -func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http.Request, retryDelay time.Duration) (*http.Response, error) { - // make attempt +func attemptHttpCall(client *http.Client, request *http.Request) (*http.Response, error) { resp, err := client.Do(request) if err != nil { - err = fmt.Errorf("problem calling http loader url: %v", err) + return nil, fmt.Errorf("problem calling http loader url: %v", err) } else if resp.StatusCode < 200 || resp.StatusCode > 499 { - err = fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode) + return nil, fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode) } else { + // the http call was successful return resp, nil } - // skip retries if valid retryDelay not provided - if retryDelay <= 0 { - return resp, err - } - // log the error - caddy.Log().Error(err.Error()) - // wait for the retry delay to lapse before reattempting, - // or return when context is done - select { - case <-time.After(time.Second * retryDelay): - return doHttpCallWithRetries(ctx, client, request, retryDelay) - case <-ctx.Done(): - return nil, err +} + +func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http.Request) (*http.Response, error) { + var resp *http.Response + var err error + const maxAttempts = 10 + + // attempt up to 10 times + for i := 0; i < maxAttempts; i++ { + resp, err = attemptHttpCall(client, request) + if err != nil && i < maxAttempts-1 { + // wait 500ms before reattempting, or until context is done + select { + case <-time.After(time.Millisecond * 500): + continue + case <-ctx.Done(): + return resp, ctx.Err() + } + } } + + return resp, err } func (hl HTTPLoader) makeClient(ctx caddy.Context) (*http.Client, error) { From 18b82045dd20935c6ebf5f6744f333ca31da9b21 Mon Sep 17 00:00:00 2001 From: Cory Cooper Date: Wed, 5 Oct 2022 19:56:08 -0700 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Matt Holt --- caddyconfig/httploader.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/caddyconfig/httploader.go b/caddyconfig/httploader.go index a9e954b79de..f1992199809 100644 --- a/caddyconfig/httploader.go +++ b/caddyconfig/httploader.go @@ -94,7 +94,6 @@ func (hl HTTPLoader) LoadConfig(ctx caddy.Context) ([]byte, error) { } } - caddy.Log().Info("attempting to load config via http loader url") resp, err := doHttpCallWithRetries(ctx, client, req) if err != nil { return nil, err @@ -126,10 +125,8 @@ func attemptHttpCall(client *http.Client, request *http.Request) (*http.Response return nil, fmt.Errorf("problem calling http loader url: %v", err) } else if resp.StatusCode < 200 || resp.StatusCode > 499 { return nil, fmt.Errorf("bad response status code from http loader url: %v", resp.StatusCode) - } else { - // the http call was successful - return resp, nil } + return resp, nil } func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http.Request) (*http.Response, error) { @@ -144,7 +141,6 @@ func doHttpCallWithRetries(ctx caddy.Context, client *http.Client, request *http // wait 500ms before reattempting, or until context is done select { case <-time.After(time.Millisecond * 500): - continue case <-ctx.Done(): return resp, ctx.Err() }