Skip to content

Commit b63ac63

Browse files
ximpanzximpanz
committed
Add per-attempt timeout and idempotency-aware retries to the REST client
The upstream SDK uses http.DefaultClient (no timeout) and a single Do() with no retries, so a stalled NetBird management API hangs plan/apply indefinitely, and transient 429/5xx/connection errors fail the whole run. The SDK also leaks the response body on its error path, which can wedge the connection pool after a burst of error responses. - internal/provider/httpclient.go: retryTransport, an http.RoundTripper with a per-attempt context timeout and bounded exponential backoff. 429 retries any method; 5xx and transport/timeout errors retry only idempotent methods (GET/HEAD/PUT/DELETE/OPTIONS) to avoid duplicate POST/PATCH side effects. Drains+closes bodies between attempts, honors Retry-After, and stops on outer context cancellation. - internal/provider/provider.go: pass the client via WithHttpClient(); new optional request_timeout (NB_REQUEST_TIMEOUT, default 30s) and max_retries (NB_MAX_RETRIES, default 4) provider settings. - internal/provider/httpclient_test.go: retry-policy unit tests. - internal/provider/settings_test.go: intSetting precedence tests (config > env > default, negative/invalid fallback). - internal/provider/provider_test.go: include the two new attributes in TestProviderUserAgent's config so it matches the updated schema. - docs/index.md: document request_timeout and max_retries.
1 parent f865f5c commit b63ac63

6 files changed

Lines changed: 415 additions & 7 deletions

File tree

‎docs/index.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,7 @@ provider "netbird" {
4040
### Optional
4141

4242
- `management_url` (String) NetBird Management API URL, can be also set through NB_MANAGEMENT_URL Environment Variable, value defined in Terraform files takes precedence
43+
- `max_retries` (Number) Maximum number of retries for transient Management API failures (429, 5xx, and connection errors; 5xx and connection retries are limited to idempotent methods), can be also set through NB_MAX_RETRIES Environment Variable. Defaults to 4, set to 0 to disable retries.
44+
- `request_timeout` (Number) Per-attempt timeout in seconds for Management API requests, can be also set through NB_REQUEST_TIMEOUT Environment Variable. Defaults to 30.
4345
- `tenant_account` (String) Account ID to impersonate, can be also set through NB_ACCOUNT Environment Variable, value defined in Terraform files takes precedence
4446
- `token` (String, Sensitive) Admin PAT for NetBird Management Server, can be also set through NB_PAT Environment Variable, value defined in Terraform files takes precedence

‎internal/provider/httpclient.go‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
// Copyright (c) HashiCorp, Inc.
2+
3+
package provider
4+
5+
import (
6+
"bytes"
7+
"context"
8+
"io"
9+
"math/rand"
10+
"net/http"
11+
"strconv"
12+
"time"
13+
)
14+
15+
// The upstream NetBird REST SDK defaults to http.DefaultClient (no timeout)
16+
// and performs a single Do() with no retries. Against a flaky management API
17+
// that means a stalled response blocks forever (hanging plan/apply), and
18+
// transient 429/5xx/connection errors fail the whole run. This transport adds
19+
// a per-attempt timeout plus bounded, idempotency-aware retries with backoff.
20+
type retryTransport struct {
21+
inner http.RoundTripper
22+
maxRetries int
23+
perTryTimeout time.Duration
24+
waitMin time.Duration
25+
waitMax time.Duration
26+
}
27+
28+
// newRetryingHTTPClient builds an *http.Client (satisfies the SDK's HttpClient
29+
// interface) backed by retryTransport. No client-level Timeout is set: each
30+
// attempt is bounded by perTryTimeout via context, so the retry budget is not
31+
// cut short by a single overall deadline.
32+
func newRetryingHTTPClient(perTryTimeout time.Duration, maxRetries int) *http.Client {
33+
// Guard against a non-positive timeout, which would make every attempt
34+
// expire immediately and trigger a retry storm. Callers normally enforce
35+
// this (request_timeout >= 1), but keep the constructor safe on its own.
36+
if perTryTimeout <= 0 {
37+
perTryTimeout = defaultRequestTimeoutSeconds * time.Second
38+
}
39+
return &http.Client{
40+
Transport: &retryTransport{
41+
inner: http.DefaultTransport,
42+
maxRetries: maxRetries,
43+
perTryTimeout: perTryTimeout,
44+
waitMin: 1 * time.Second,
45+
waitMax: 15 * time.Second,
46+
},
47+
}
48+
}
49+
50+
// idempotent reports whether a method is safe to retry after a 5xx or a
51+
// transport error. POST/PATCH are excluded: a request that succeeded
52+
// server-side but failed on the response could be double-applied (e.g. a
53+
// duplicate group/peer/setup-key) if retried.
54+
func idempotent(method string) bool {
55+
switch method {
56+
case http.MethodGet, http.MethodHead, http.MethodPut, http.MethodDelete, http.MethodOptions:
57+
return true
58+
default:
59+
return false
60+
}
61+
}
62+
63+
func (t *retryTransport) shouldRetry(method string, resp *http.Response, err error) bool {
64+
if err != nil {
65+
// Transport error or per-attempt timeout. 429 not reached here.
66+
return idempotent(method)
67+
}
68+
if resp == nil {
69+
return false
70+
}
71+
switch {
72+
case resp.StatusCode == http.StatusTooManyRequests:
73+
// Rate limited: the request was not processed, safe for any method.
74+
return true
75+
case resp.StatusCode >= 500 && resp.StatusCode <= 599:
76+
return idempotent(method)
77+
default:
78+
return false
79+
}
80+
}
81+
82+
// backoff returns the wait before the next attempt: exponential (waitMin<<n)
83+
// capped at waitMax, with jitter. A Retry-After header (429/503) takes
84+
// precedence when present.
85+
func (t *retryTransport) backoff(attempt int, resp *http.Response) time.Duration {
86+
if resp != nil {
87+
if ra := resp.Header.Get("Retry-After"); ra != "" {
88+
if secs, err := strconv.Atoi(ra); err == nil && secs >= 0 {
89+
d := time.Duration(secs) * time.Second
90+
if d > t.waitMax {
91+
d = t.waitMax
92+
}
93+
return d
94+
}
95+
}
96+
}
97+
wait := t.waitMin << uint(attempt)
98+
if wait <= 0 || wait > t.waitMax {
99+
wait = t.waitMax
100+
}
101+
// Full jitter in [wait/2, wait].
102+
half := wait / 2
103+
return half + time.Duration(rand.Int63n(int64(half)+1))
104+
}
105+
106+
func drain(resp *http.Response) {
107+
if resp != nil && resp.Body != nil {
108+
_, _ = io.Copy(io.Discard, resp.Body)
109+
_ = resp.Body.Close()
110+
}
111+
}
112+
113+
func (t *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) {
114+
// Buffer the body once so each attempt can replay it.
115+
var bodyBytes []byte
116+
if req.Body != nil {
117+
b, err := io.ReadAll(req.Body)
118+
_ = req.Body.Close()
119+
if err != nil {
120+
return nil, err
121+
}
122+
bodyBytes = b
123+
}
124+
125+
var resp *http.Response
126+
var err error
127+
128+
for attempt := 0; ; attempt++ {
129+
attemptReq := req.Clone(req.Context())
130+
if bodyBytes != nil {
131+
attemptReq.Body = io.NopCloser(bytes.NewReader(bodyBytes))
132+
attemptReq.ContentLength = int64(len(bodyBytes))
133+
}
134+
135+
// Per-attempt timeout derived from the caller's context, so Terraform
136+
// cancellation (Ctrl-C) still aborts immediately.
137+
ctx, cancel := context.WithTimeout(req.Context(), t.perTryTimeout)
138+
resp, err = t.inner.RoundTrip(attemptReq.WithContext(ctx))
139+
140+
retry := t.shouldRetry(attemptReq.Method, resp, err) &&
141+
attempt < t.maxRetries &&
142+
req.Context().Err() == nil // outer cancel/deadline -> stop
143+
144+
if !retry {
145+
if err != nil {
146+
cancel()
147+
return nil, err
148+
}
149+
// Defer cancel until the caller closes the body, otherwise the
150+
// per-attempt timer would abort an in-progress body read.
151+
resp.Body = &cancelOnCloseBody{ReadCloser: resp.Body, cancel: cancel}
152+
return resp, nil
153+
}
154+
155+
// Retrying: free this attempt's resources, then wait.
156+
drain(resp)
157+
wait := t.backoff(attempt, resp)
158+
cancel()
159+
160+
timer := time.NewTimer(wait)
161+
select {
162+
case <-req.Context().Done():
163+
timer.Stop()
164+
return nil, req.Context().Err()
165+
case <-timer.C:
166+
}
167+
}
168+
}
169+
170+
// cancelOnCloseBody ties the per-attempt context's cancel to body Close so the
171+
// context is released once the caller is done reading the response.
172+
type cancelOnCloseBody struct {
173+
io.ReadCloser
174+
cancel context.CancelFunc
175+
}
176+
177+
func (b *cancelOnCloseBody) Close() error {
178+
err := b.ReadCloser.Close()
179+
b.cancel()
180+
return err
181+
}

‎internal/provider/httpclient_test.go‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
package provider
2+
3+
import (
4+
"context"
5+
"net/http"
6+
"net/http/httptest"
7+
"strings"
8+
"sync/atomic"
9+
"testing"
10+
"time"
11+
)
12+
13+
func newRetryTestClient(maxRetries int, perTry time.Duration) *http.Client {
14+
return &http.Client{
15+
Transport: &retryTransport{
16+
inner: http.DefaultTransport,
17+
maxRetries: maxRetries,
18+
perTryTimeout: perTry,
19+
waitMin: time.Millisecond,
20+
waitMax: 5 * time.Millisecond,
21+
},
22+
}
23+
}
24+
25+
func doReq(t *testing.T, c *http.Client, method, url string) *http.Response {
26+
t.Helper()
27+
req, err := http.NewRequestWithContext(context.Background(), method, url, strings.NewReader("{}"))
28+
if err != nil {
29+
t.Fatalf("new request: %v", err)
30+
}
31+
resp, err := c.Do(req)
32+
if err != nil {
33+
return nil
34+
}
35+
t.Cleanup(func() { _ = resp.Body.Close() })
36+
return resp
37+
}
38+
39+
func TestRetry_GET5xxThenOK(t *testing.T) {
40+
var n int32
41+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
42+
if atomic.AddInt32(&n, 1) == 1 {
43+
w.WriteHeader(http.StatusServiceUnavailable)
44+
return
45+
}
46+
w.WriteHeader(http.StatusOK)
47+
}))
48+
defer srv.Close()
49+
50+
resp := doReq(t, newRetryTestClient(3, 2*time.Second), http.MethodGet, srv.URL)
51+
if resp == nil || resp.StatusCode != http.StatusOK {
52+
t.Fatalf("expected 200 after retry, got %v", resp)
53+
}
54+
if got := atomic.LoadInt32(&n); got != 2 {
55+
t.Fatalf("expected 2 attempts, got %d", got)
56+
}
57+
}
58+
59+
func TestRetry_POST5xxNotRetried(t *testing.T) {
60+
var n int32
61+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
62+
atomic.AddInt32(&n, 1)
63+
w.WriteHeader(http.StatusInternalServerError)
64+
}))
65+
defer srv.Close()
66+
67+
resp := doReq(t, newRetryTestClient(3, 2*time.Second), http.MethodPost, srv.URL)
68+
if resp == nil || resp.StatusCode != http.StatusInternalServerError {
69+
t.Fatalf("expected 500 returned, got %v", resp)
70+
}
71+
if got := atomic.LoadInt32(&n); got != 1 {
72+
t.Fatalf("POST 5xx must not retry: expected 1 attempt, got %d", got)
73+
}
74+
}
75+
76+
func TestRetry_POST429Retried(t *testing.T) {
77+
var n int32
78+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
79+
if atomic.AddInt32(&n, 1) == 1 {
80+
w.WriteHeader(http.StatusTooManyRequests)
81+
return
82+
}
83+
w.WriteHeader(http.StatusCreated)
84+
}))
85+
defer srv.Close()
86+
87+
resp := doReq(t, newRetryTestClient(3, 2*time.Second), http.MethodPost, srv.URL)
88+
if resp == nil || resp.StatusCode != http.StatusCreated {
89+
t.Fatalf("expected 201 after 429 retry, got %v", resp)
90+
}
91+
if got := atomic.LoadInt32(&n); got != 2 {
92+
t.Fatalf("429 should retry (not processed): expected 2 attempts, got %d", got)
93+
}
94+
}
95+
96+
func TestRetry_PerAttemptTimeoutThenBounded(t *testing.T) {
97+
var n int32
98+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
99+
atomic.AddInt32(&n, 1)
100+
time.Sleep(200 * time.Millisecond) // exceeds per-attempt timeout
101+
w.WriteHeader(http.StatusOK)
102+
}))
103+
defer srv.Close()
104+
105+
resp := doReq(t, newRetryTestClient(2, 30*time.Millisecond), http.MethodGet, srv.URL)
106+
if resp != nil {
107+
t.Fatalf("expected timeout error, got status %d", resp.StatusCode)
108+
}
109+
if got := atomic.LoadInt32(&n); got != 3 { // 1 initial + 2 retries
110+
t.Fatalf("expected 3 bounded attempts, got %d", got)
111+
}
112+
}

‎internal/provider/provider.go‎

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import (
66
"context"
77
"fmt"
88
"os"
9+
"strconv"
10+
"time"
911

1012
"github.com/hashicorp/terraform-plugin-framework/datasource"
1113
"github.com/hashicorp/terraform-plugin-framework/ephemeral"
@@ -18,6 +20,11 @@ import (
1820
netbird "github.com/netbirdio/netbird/shared/management/client/rest"
1921
)
2022

23+
const (
24+
defaultRequestTimeoutSeconds = 30
25+
defaultMaxRetries = 4
26+
)
27+
2128
// Ensure NetBirdProvider satisfies various provider interfaces.
2229
var _ provider.Provider = &NetBirdProvider{}
2330
var _ provider.ProviderWithFunctions = &NetBirdProvider{}
@@ -32,9 +39,11 @@ type NetBirdProvider struct {
3239

3340
// NetBirdProviderModel describes the provider data model.
3441
type NetBirdProviderModel struct {
35-
ManagementURL types.String `tfsdk:"management_url"`
36-
Token types.String `tfsdk:"token"`
37-
TenantAccount types.String `tfsdk:"tenant_account"`
42+
ManagementURL types.String `tfsdk:"management_url"`
43+
Token types.String `tfsdk:"token"`
44+
TenantAccount types.String `tfsdk:"tenant_account"`
45+
RequestTimeout types.Int64 `tfsdk:"request_timeout"`
46+
MaxRetries types.Int64 `tfsdk:"max_retries"`
3847
}
3948

4049
func (p *NetBirdProvider) Metadata(ctx context.Context, req provider.MetadataRequest, resp *provider.MetadataResponse) {
@@ -59,10 +68,38 @@ func (p *NetBirdProvider) Schema(ctx context.Context, req provider.SchemaRequest
5968
MarkdownDescription: "Account ID to impersonate, can be also set through NB_ACCOUNT Environment Variable, value defined in Terraform files takes precedence",
6069
Optional: true,
6170
},
71+
"request_timeout": schema.Int64Attribute{
72+
MarkdownDescription: "Per-attempt timeout in seconds for Management API requests, can be also set through NB_REQUEST_TIMEOUT Environment Variable. Defaults to 30.",
73+
Optional: true,
74+
},
75+
"max_retries": schema.Int64Attribute{
76+
MarkdownDescription: "Maximum number of retries for transient Management API failures (429, 5xx, and connection errors; 5xx and connection retries are limited to idempotent methods), can be also set through NB_MAX_RETRIES Environment Variable. Defaults to 4, set to 0 to disable retries.",
77+
Optional: true,
78+
},
6279
},
6380
}
6481
}
6582

83+
// intSetting resolves an optional int provider setting: explicit config value
84+
// wins, then the environment variable, then the supplied default. Values below
85+
// minVal (from either source) are rejected and fall back to the default.
86+
// minVal lets callers forbid nonsensical values, e.g. request_timeout must be
87+
// >= 1 (a 0 per-attempt timeout would expire immediately and cause a retry
88+
// storm), while max_retries allows 0 to disable retries.
89+
func intSetting(v types.Int64, envVar string, def, minVal int) int {
90+
if !v.IsNull() && !v.IsUnknown() {
91+
if n := int(v.ValueInt64()); n >= minVal {
92+
return n
93+
}
94+
}
95+
if s, ok := os.LookupEnv(envVar); ok {
96+
if n, err := strconv.Atoi(s); err == nil && n >= minVal {
97+
return n
98+
}
99+
}
100+
return def
101+
}
102+
66103
func (p *NetBirdProvider) Configure(ctx context.Context, req provider.ConfigureRequest, resp *provider.ConfigureResponse) {
67104
var data NetBirdProviderModel
68105

@@ -89,10 +126,16 @@ func (p *NetBirdProvider) Configure(ctx context.Context, req provider.ConfigureR
89126
if resp.Diagnostics.HasError() {
90127
return
91128
}
129+
// request_timeout must be >= 1s; max_retries may be 0 to disable retries.
130+
requestTimeout := intSetting(data.RequestTimeout, "NB_REQUEST_TIMEOUT", defaultRequestTimeoutSeconds, 1)
131+
maxRetries := intSetting(data.MaxRetries, "NB_MAX_RETRIES", defaultMaxRetries, 0)
132+
httpClient := newRetryingHTTPClient(time.Duration(requestTimeout)*time.Second, maxRetries)
133+
92134
client := netbird.NewWithOptions(
93135
netbird.WithManagementURL(managementURL),
94136
netbird.WithPAT(token),
95-
netbird.WithUserAgent(fmt.Sprintf("terraform-provider-netbird/%s Terraform/%s", p.version, req.TerraformVersion)))
137+
netbird.WithUserAgent(fmt.Sprintf("terraform-provider-netbird/%s Terraform/%s", p.version, req.TerraformVersion)),
138+
netbird.WithHttpClient(httpClient))
96139
if !data.TenantAccount.IsNull() && !data.TenantAccount.IsUnknown() {
97140
client = client.Impersonate(data.TenantAccount.ValueString())
98141
} else if v, ok := os.LookupEnv("NB_ACCOUNT"); ok {

0 commit comments

Comments
 (0)