Skip to content

Commit d9fa833

Browse files
committed
test(e2e): retry transient network errors on idempotent requests
The TestEGUpgrade teardown intermittently fails with a transient connection-level error when the gateway-api conformance suite's cleanup deletes the upgrade GatewayClass: Delete ".../gatewayclasses/upgrade": EOF This is a test-infra flake, not a regression in the code under test: it shows up on unrelated PRs, and the failing DELETE runs in the suite's registered t.Cleanup after the test body already succeeded. Root cause: client-go's rest.Request only retries GET on transient errors (IsProbableEOF/IsConnectionReset/IsHTTP2ConnectionLost); DELETE and other write verbs surface the error immediately. So when a DELETE reaches the apiserver but the response is lost (EOF / connection reset / HTTP/2 GOAWAY / timeout), teardown fails even though the resource was already deleted. Fix: install a retrying http.RoundTripper via rest.Config.WrapTransport in test/utils/kubernetes NewClient. WrapTransport is the only hook inherited by the conformance suite's per-test client rebuild (setClientsetForTest calls client.New(suite.RestConfig, ...)), so a client.Client wrapper would be bypassed while the transport wrapper is not. Retry policy (conservative): - Methods: GET, HEAD, DELETE only. Never POST/PATCH/PUT. - Body: only retry when replayable (nil / http.NoBody / GetBody != nil). client-go builds DELETE bodies from bytes.NewReader, and the stdlib auto-populates GetBody for *bytes.Reader, so DeleteOptions bodies are replayable. http.NoBody must be checked explicitly because the stdlib substitutes it for a zero-length body instead of nil. - Before each retry after the first: re-derive the body via GetBody and assign to a Clone (Clone copies GetBody but does not reset Body). - Errors retried: EOF, ErrUnexpectedEOF, connection reset, HTTP/2 GOAWAY/connection-lost, net.Error timeouts. - HTTP status codes are NOT retried (5xx/429/404 returned as-is), so a 404 from a retried DELETE flows through to MustApplyWithCleanup's existing IsNotFound -> success handling. - Close any partial resp.Body before retrying; honor context cancellation in the backoff select. - Compose with any pre-existing cfg.WrapTransport via transport.Wrappers. Adds unit tests covering: EOF retry on GET, DELETE-with-body replay via GetBody, non-retry of POST/non-replayable bodies, non-retry of HTTP status codes (incl. 404 pass-through), attempt exhaustion, context cancellation, and timeout (net.Error) retry. Fixes #9467 Signed-off-by: liuhy <liuhongyu@apache.org>
1 parent 69e072f commit d9fa833

2 files changed

Lines changed: 565 additions & 0 deletions

File tree

test/utils/kubernetes/client.go

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,17 @@
66
package kubernetes
77

88
import (
9+
"errors"
10+
"net"
11+
"net/http"
912
"testing"
13+
"time"
1014

1115
"github.com/stretchr/testify/require"
1216
batchv1 "k8s.io/api/batch/v1"
17+
netutil "k8s.io/apimachinery/pkg/util/net"
1318
"k8s.io/client-go/rest"
19+
"k8s.io/client-go/transport"
1420
"sigs.k8s.io/controller-runtime/pkg/client"
1521
"sigs.k8s.io/controller-runtime/pkg/client/config"
1622
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
@@ -25,6 +31,23 @@ func NewClient(t *testing.T) (client.Client, *rest.Config) {
2531
cfg, err := config.GetConfig()
2632
require.NoError(t, err)
2733

34+
// Install a transport wrapper that retries transient network errors
35+
// (e.g. EOF, connection reset, HTTP/2 GOAWAY, timeout) for idempotent
36+
// requests whose body can be replayed. This is necessary because the
37+
// gateway-api conformance suite rebuilds its client per test via
38+
// client.New(suite.RestConfig, ...) (see setClientsetForTest), so a
39+
// controller-runtime client wrapper returned from this function would be
40+
// bypassed. The transport wrapper is the only hook inherited by those
41+
// rebuilt clients, and it also covers the raw client returned here.
42+
//
43+
// Notably, client-go only retries GET on transient errors; DELETE and other
44+
// write verbs surface the error immediately (e.g. "Delete ... : EOF"),
45+
// which is the source of teardown flakes. Retrying replayable DELETEs here
46+
// closes that gap: if the first DELETE reached the apiserver but the
47+
// response was lost, the retry may return 404, which conformance cleanup
48+
// already treats as success.
49+
cfg.Wrap(transport.Wrappers(cfg.WrapTransport, newRetryTransportWrapper))
50+
2851
c, err := client.New(cfg, client.Options{})
2952
require.NoError(t, err)
3053

@@ -42,3 +65,126 @@ func CheckInstallScheme(t *testing.T, c client.Client) {
4265
require.NoError(t, egv1a1.AddToScheme(c.Scheme()))
4366
require.NoError(t, batchv1.AddToScheme(c.Scheme()))
4467
}
68+
69+
// retryTransportMaxAttempts bounds the total attempts (1 initial + retries).
70+
const retryTransportMaxAttempts = 4
71+
72+
// retryTransportBaseBackoff is the initial backoff between retries; it grows
73+
// by retryTransportBackoffFactor on each attempt.
74+
const (
75+
retryTransportBaseBackoff = 100 * time.Millisecond
76+
retryTransportBackoffFactor = 2
77+
)
78+
79+
// newRetryTransportWrapper returns a transport.WrapperFunc that layers a
80+
// retrying RoundTripper over the base transport.
81+
func newRetryTransportWrapper(base http.RoundTripper) http.RoundTripper {
82+
if base == nil {
83+
base = http.DefaultTransport
84+
}
85+
return &retryTransport{base: base}
86+
}
87+
88+
// retryTransport retries transient, connection-level errors for idempotent
89+
// requests whose body can be safely replayed.
90+
type retryTransport struct {
91+
base http.RoundTripper
92+
}
93+
94+
func (r *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) {
95+
// Only retry idempotent verbs with a replayable body. POST/PATCH/PUT are
96+
// never retried; a request with a body and no GetBody cannot be replayed
97+
// safely after the body has been (partially) consumed.
98+
if !retryableMethod(req.Method) || !replayableBody(req) {
99+
return r.base.RoundTrip(req)
100+
}
101+
102+
var (
103+
lastErr error
104+
backoff = retryTransportBaseBackoff
105+
attemptReq = req
106+
)
107+
108+
for attempt := 0; attempt < retryTransportMaxAttempts; attempt++ {
109+
// On every attempt after the first, back off before retrying. The sleep
110+
// is done BEFORE creating a fresh body so that a context cancellation
111+
// observed here does not leak a body ReadCloser obtained from GetBody.
112+
if attempt > 0 {
113+
select {
114+
case <-req.Context().Done():
115+
return nil, req.Context().Err()
116+
case <-time.After(backoff):
117+
}
118+
backoff *= retryTransportBackoffFactor
119+
120+
// Reset the body from GetBody so the base transport reads a fresh
121+
// copy; the original req.Body may have been consumed by the previous
122+
// attempt.
123+
if req.GetBody != nil {
124+
body, err := req.GetBody()
125+
if err != nil {
126+
return nil, err
127+
}
128+
attemptReq = req.Clone(req.Context())
129+
attemptReq.Body = body
130+
}
131+
}
132+
133+
resp, err := r.base.RoundTrip(attemptReq)
134+
if !isTransientNetworkError(err) {
135+
// Either success (err == nil) or a non-retryable error: return as-is.
136+
return resp, err
137+
}
138+
139+
// Transient error: drain and close any partial response before retrying
140+
// to avoid leaking the underlying connection.
141+
if resp != nil {
142+
_ = resp.Body.Close()
143+
}
144+
lastErr = err
145+
}
146+
147+
return nil, lastErr
148+
}
149+
150+
// retryableMethod reports whether the HTTP method is safe to retry without
151+
// risking duplicate side effects.
152+
func retryableMethod(method string) bool {
153+
switch method {
154+
case http.MethodGet, http.MethodHead, http.MethodDelete:
155+
return true
156+
default:
157+
return false
158+
}
159+
}
160+
161+
// replayableBody reports whether the request body can be re-sent on a retry.
162+
// A request with no body, an explicit http.NoBody, or a body that can be
163+
// obtained again via GetBody is replayable.
164+
func replayableBody(req *http.Request) bool {
165+
return req.Body == nil || req.Body == http.NoBody || req.GetBody != nil
166+
}
167+
168+
// isTransientNetworkError reports whether err is a connection-level error that
169+
// may have occurred before or during the response, making a retry safe. It does
170+
// not treat HTTP status codes (e.g. 5xx, 429) as retryable: those are returned
171+
// to the caller, preserving the apiserver's status semantics (e.g. a 404 on a
172+
// retried DELETE flows through to conformance cleanup's IsNotFound handling).
173+
func isTransientNetworkError(err error) bool {
174+
if err == nil {
175+
return false
176+
}
177+
if netutil.IsProbableEOF(err) || netutil.IsConnectionReset(err) || netutil.IsHTTP2ConnectionLost(err) {
178+
return true
179+
}
180+
// net.Error covers request timeouts (DeadlineExceeded), which are safe to
181+
// retry for idempotent verbs.
182+
var netErr net.Error
183+
if errors.As(err, &netErr) && netErr.Timeout() {
184+
return true
185+
}
186+
return false
187+
}
188+
189+
// compile-time assertion that retryTransport satisfies http.RoundTripper.
190+
var _ http.RoundTripper = &retryTransport{}

0 commit comments

Comments
 (0)