Skip to content

Commit 60c0b0a

Browse files
hslatmanhslatman
authored
Merge pull request #2191 from smallstep/mariano/timeout
Add option to specify a client timeout
2 parents dd8376c + 3135a2c commit 60c0b0a

File tree

5 files changed

+79
-22
lines changed

5 files changed

+79
-22
lines changed

‎ca/acmeClient.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,11 @@ import (
1111
"strings"
1212

1313
"github.com/pkg/errors"
14+
15+
"go.step.sm/crypto/jose"
16+
1417
"github.com/smallstep/certificates/acme"
1518
acmeAPI "github.com/smallstep/certificates/acme/api"
16-
"go.step.sm/crypto/jose"
1719
)
1820

1921
// ACMEClient implements an HTTP client to an ACME API.
@@ -29,7 +31,7 @@ type ACMEClient struct {
2931
// NewACMEClient initializes a new ACMEClient.
3032
func NewACMEClient(endpoint string, contact []string, opts ...ClientOption) (*ACMEClient, error) {
3133
// Retrieve transport from options.
32-
o := new(clientOptions)
34+
o := defaultClientOptions()
3335
if err := o.apply(opts); err != nil {
3436
return nil, err
3537
}

‎ca/acmeClient_test.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,14 @@ import (
1212
"time"
1313

1414
"github.com/pkg/errors"
15+
1516
"github.com/smallstep/assert"
17+
"go.step.sm/crypto/jose"
18+
"go.step.sm/crypto/pemutil"
19+
1620
"github.com/smallstep/certificates/acme"
1721
acmeAPI "github.com/smallstep/certificates/acme/api"
1822
"github.com/smallstep/certificates/api/render"
19-
"go.step.sm/crypto/jose"
20-
"go.step.sm/crypto/pemutil"
2123
)
2224

2325
func TestNewACMEClient(t *testing.T) {
@@ -169,7 +171,7 @@ func TestACMEClient_GetNonce(t *testing.T) {
169171
NewNonce: srv.URL + "/foo",
170172
}
171173
// Retrieve transport from options.
172-
o := new(clientOptions)
174+
o := defaultClientOptions()
173175
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
174176
tr, err := o.getTransport(srv.URL)
175177
assert.FatalError(t, err)
@@ -241,7 +243,7 @@ func TestACMEClient_post(t *testing.T) {
241243
NewNonce: srv.URL + "/foo",
242244
}
243245
// Retrieve transport from options.
244-
o := new(clientOptions)
246+
o := defaultClientOptions()
245247
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
246248
tr, err := o.getTransport(srv.URL)
247249
assert.FatalError(t, err)
@@ -372,7 +374,7 @@ func TestACMEClient_NewOrder(t *testing.T) {
372374
NewOrder: srv.URL + "/bar",
373375
}
374376
// Retrieve transport from options.
375-
o := new(clientOptions)
377+
o := defaultClientOptions()
376378
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
377379
tr, err := o.getTransport(srv.URL)
378380
assert.FatalError(t, err)
@@ -507,7 +509,7 @@ func TestACMEClient_GetOrder(t *testing.T) {
507509
NewNonce: srv.URL + "/foo",
508510
}
509511
// Retrieve transport from options.
510-
o := new(clientOptions)
512+
o := defaultClientOptions()
511513
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
512514
tr, err := o.getTransport(srv.URL)
513515
assert.FatalError(t, err)
@@ -629,7 +631,7 @@ func TestACMEClient_GetAuthz(t *testing.T) {
629631
NewNonce: srv.URL + "/foo",
630632
}
631633
// Retrieve transport from options.
632-
o := new(clientOptions)
634+
o := defaultClientOptions()
633635
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
634636
tr, err := o.getTransport(srv.URL)
635637
assert.FatalError(t, err)
@@ -751,7 +753,7 @@ func TestACMEClient_GetChallenge(t *testing.T) {
751753
NewNonce: srv.URL + "/foo",
752754
}
753755
// Retrieve transport from options.
754-
o := new(clientOptions)
756+
o := defaultClientOptions()
755757
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
756758
tr, err := o.getTransport(srv.URL)
757759
assert.FatalError(t, err)
@@ -874,7 +876,7 @@ func TestACMEClient_ValidateChallenge(t *testing.T) {
874876
NewNonce: srv.URL + "/foo",
875877
}
876878
// Retrieve transport from options.
877-
o := new(clientOptions)
879+
o := defaultClientOptions()
878880
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
879881
tr, err := o.getTransport(srv.URL)
880882
assert.FatalError(t, err)
@@ -1087,7 +1089,7 @@ func TestACMEClient_FinalizeOrder(t *testing.T) {
10871089
NewNonce: srv.URL + "/foo",
10881090
}
10891091
// Retrieve transport from options.
1090-
o := new(clientOptions)
1092+
o := defaultClientOptions()
10911093
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
10921094
tr, err := o.getTransport(srv.URL)
10931095
assert.FatalError(t, err)
@@ -1214,7 +1216,7 @@ func TestACMEClient_GetAccountOrders(t *testing.T) {
12141216
NewNonce: srv.URL + "/foo",
12151217
}
12161218
// Retrieve transport from options.
1217-
o := new(clientOptions)
1219+
o := defaultClientOptions()
12181220
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
12191221
tr, err := o.getTransport(srv.URL)
12201222
assert.FatalError(t, err)
@@ -1347,7 +1349,7 @@ func TestACMEClient_GetCertificate(t *testing.T) {
13471349
NewNonce: srv.URL + "/foo",
13481350
}
13491351
// Retrieve transport from options.
1350-
o := new(clientOptions)
1352+
o := defaultClientOptions()
13511353
assert.FatalError(t, o.apply([]ClientOption{WithTransport(http.DefaultTransport)}))
13521354
tr, err := o.getTransport(srv.URL)
13531355
assert.FatalError(t, err)

‎ca/adminClient.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,14 +60,23 @@ func (e *AdminClientError) Error() string {
6060
return e.Message
6161
}
6262

63+
// defaultClientOptions returns a new [clientOptions] with a
64+
// default timeout set.
65+
func defaultClientOptions() clientOptions {
66+
return clientOptions{
67+
timeout: 15 * time.Second,
68+
}
69+
}
70+
6371
// NewAdminClient creates a new AdminClient with the given endpoint and options.
6472
func NewAdminClient(endpoint string, opts ...ClientOption) (*AdminClient, error) {
6573
u, err := parseEndpoint(endpoint)
6674
if err != nil {
6775
return nil, err
6876
}
77+
6978
// Retrieve transport from options.
70-
o := new(clientOptions)
79+
o := defaultClientOptions()
7180
if err := o.apply(opts); err != nil {
7281
return nil, err
7382
}
@@ -77,7 +86,7 @@ func NewAdminClient(endpoint string, opts ...ClientOption) (*AdminClient, error)
7786
}
7887

7988
return &AdminClient{
80-
client: newClient(tr),
89+
client: newClient(tr, o.timeout),
8190
endpoint: u,
8291
retryFunc: o.retryFunc,
8392
opts: opts,
@@ -124,7 +133,7 @@ func (c *AdminClient) generateAdminToken(aud *url.URL) (string, error) {
124133
func (c *AdminClient) retryOnError(r *http.Response) bool {
125134
if c.retryFunc != nil {
126135
if c.retryFunc(r.StatusCode) {
127-
o := new(clientOptions)
136+
o := defaultClientOptions()
128137
if err := o.apply(c.opts); err != nil {
129138
return false
130139
}

‎ca/client.go

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"path/filepath"
2323
"strconv"
2424
"strings"
25+
"time"
2526

2627
"github.com/pkg/errors"
2728
"golang.org/x/net/http2"
@@ -53,10 +54,11 @@ type uaClient struct {
5354
Client *http.Client
5455
}
5556

56-
func newClient(transport http.RoundTripper) *uaClient {
57+
func newClient(transport http.RoundTripper, timeout time.Duration) *uaClient {
5758
return &uaClient{
5859
Client: &http.Client{
5960
Transport: transport,
61+
Timeout: timeout,
6062
},
6163
}
6264
}
@@ -149,6 +151,7 @@ type ClientOption func(o *clientOptions) error
149151

150152
type clientOptions struct {
151153
transport http.RoundTripper
154+
timeout time.Duration
152155
rootSHA256 string
153156
rootFilename string
154157
rootBundle []byte
@@ -388,6 +391,16 @@ func WithRetryFunc(fn RetryFunc) ClientOption {
388391
}
389392
}
390393

394+
// WithTimeout defines the time limit for requests made by this client. The
395+
// timeout includes connection time, any redirects, and reading the response
396+
// body.
397+
func WithTimeout(d time.Duration) ClientOption {
398+
return func(o *clientOptions) error {
399+
o.timeout = d
400+
return nil
401+
}
402+
}
403+
391404
func getTransportFromFile(filename string) (http.RoundTripper, error) {
392405
data, err := os.ReadFile(filename)
393406
if err != nil {
@@ -548,6 +561,7 @@ type Client struct {
548561
client *uaClient
549562
endpoint *url.URL
550563
retryFunc RetryFunc
564+
timeout time.Duration
551565
opts []ClientOption
552566
}
553567

@@ -557,8 +571,9 @@ func NewClient(endpoint string, opts ...ClientOption) (*Client, error) {
557571
if err != nil {
558572
return nil, err
559573
}
574+
560575
// Retrieve transport from options.
561-
o := new(clientOptions)
576+
o := defaultClientOptions()
562577
if err := o.apply(opts); err != nil {
563578
return nil, err
564579
}
@@ -568,17 +583,18 @@ func NewClient(endpoint string, opts ...ClientOption) (*Client, error) {
568583
}
569584

570585
return &Client{
571-
client: newClient(tr),
586+
client: newClient(tr, o.timeout),
572587
endpoint: u,
573588
retryFunc: o.retryFunc,
589+
timeout: o.timeout,
574590
opts: opts,
575591
}, nil
576592
}
577593

578594
func (c *Client) retryOnError(r *http.Response) bool {
579595
if c.retryFunc != nil {
580596
if c.retryFunc(r.StatusCode) {
581-
o := new(clientOptions)
597+
o := defaultClientOptions()
582598
if err := o.apply(c.opts); err != nil {
583599
return false
584600
}
@@ -890,7 +906,7 @@ func (c *Client) RevokeWithContext(ctx context.Context, req *api.RevokeRequest,
890906
var uaClient *uaClient
891907
retry:
892908
if tr != nil {
893-
uaClient = newClient(tr)
909+
uaClient = newClient(tr, c.timeout)
894910
} else {
895911
uaClient = c.client
896912
}

‎ca/client_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,6 +1017,34 @@ func TestClient_GetCaURL(t *testing.T) {
10171017
}
10181018
}
10191019

1020+
func TestClient_WithTimeout(t *testing.T) {
1021+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1022+
time.Sleep(100 * time.Millisecond)
1023+
render.JSONStatus(w, r, api.HealthResponse{Status: "ok"}, 200)
1024+
}))
1025+
defer srv.Close()
1026+
1027+
tests := []struct {
1028+
name string
1029+
options []ClientOption
1030+
assertion assert.ErrorAssertionFunc
1031+
}{
1032+
{"ok", []ClientOption{WithTransport(http.DefaultTransport)}, assert.NoError},
1033+
{"ok with timeout", []ClientOption{WithTransport(http.DefaultTransport), WithTimeout(time.Second)}, assert.NoError},
1034+
{"fail with timeout", []ClientOption{WithTransport(http.DefaultTransport), WithTimeout(100 * time.Millisecond)}, assert.Error},
1035+
}
1036+
1037+
for _, tt := range tests {
1038+
t.Run(tt.name, func(t *testing.T) {
1039+
c, err := NewClient(srv.URL, tt.options...)
1040+
require.NoError(t, err)
1041+
assert.NotZero(t, c.timeout)
1042+
_, err = c.Health()
1043+
tt.assertion(t, err)
1044+
})
1045+
}
1046+
}
1047+
10201048
func Test_enforceRequestID(t *testing.T) {
10211049
set := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody)
10221050
set.Header.Set("X-Request-Id", "already-set")

0 commit comments

Comments
 (0)