Switch to avast/retry-go for HTTP retry
#2350
Conversation
saschagrunert
force-pushed
the
retry-go
branch
from
b29bc95 to
a782ddc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2350 +/- ##
===========================================
- Coverage 66.46% 49.86% -16.61%
===========================================
Files 92 192 +100
Lines 9258 24773 +15515
===========================================
+ Hits 6153 12352 +6199
- Misses 2359 11318 +8959
- Partials 746 1103 +357
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
saschagrunert
force-pushed
the
retry-go
branch
6 times, most recently
from
9ae3e8f to
003ab22
Compare
|
Green CI :) |
| func createRoundTripper(inner http.RoundTripper, o *options) http.RoundTripper { | ||
| var tooManyRedirectyRe = regexp.MustCompile(`stopped after \d+ redirects\z`) | ||
|
|
||
| func shouldRetry(resp *http.Response, err error) error { |
There was a problem hiding this comment.
Is this a change in behavior or is this copied from go-retryable's implementation?
There was a problem hiding this comment.
It's not copied over, but it should reflect the same retry behavior.
pkg/client/rekor_client.go
Outdated
| ForceAttemptHTTP2: true, | ||
| MaxIdleConnsPerHost: -1, | ||
| DisableKeepAlives: !o.NoDisableKeepalives, | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: o.InsecureTLS}, //nolint:gosec |
There was a problem hiding this comment.
Previously this was only set if o.InsecureTLS. Should we maintain that, or is there no change in behavior? I'm not sure if there's a default tls config we're overriding.
There was a problem hiding this comment.
Changed it to only apply if o.InsecureTLS, but I assume it's the same as before in the end.
The hashicorp dependencies are MPL licensed and therefore not allowed in the CNCF. This means we now use the `avast/retry-go` implementation to achieve the same goal. Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
force-pushed
the
retry-go
branch
from
003ab22 to
3f3e3b0
Compare
| @@ -132,7 +131,9 @@ require ( | |||
| github.com/google/pprof v0.0.0-20240727154555-813a5fbdbec8 // indirect | |||
| github.com/google/s2a-go v0.1.9 // indirect | |||
| github.com/hashicorp/errwrap v1.1.0 // indirect | |||
| github.com/hashicorp/go-cleanhttp v0.5.2 // indirect | |||
There was a problem hiding this comment.
looks like these are still indirects (probably because of us supporting hashivault)... is that still an issue for CNCF?
There was a problem hiding this comment.
I think so, but the code does not seem to be imported from kubernetes-sigs/release-sdk, we may follow-up on that, though.
Summary
The hashicorp dependencies are MPL licensed and therefore not allowed in the CNCF. This means we now use the
avast/retry-goimplementation to achieve the same goal.Release Note
Use
github.com/avast/retry-goinstead ofgithub.com/hashicorp/go-retryablehttpfor HTTP request retry.Documentation
None
Fixes #2342