Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to specify a client timeout #2191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+47 −4
Open

Add option to specify a client timeout #2191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ca/adminClient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func NewAdminClient(endpoint string, opts ...ClientOption) (*AdminClient, error)
}

return &AdminClient{
client: newClient(tr),
client: newClient(tr, o.timeout),
endpoint: u,
retryFunc: o.retryFunc,
opts: opts,
Expand Down
21 changes: 18 additions & 3 deletions ca/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"path/filepath"
"strconv"
"strings"
"time"

"github.com/pkg/errors"
"golang.org/x/net/http2"
Expand Down Expand Up @@ -53,10 +54,11 @@ type uaClient struct {
Client *http.Client
}

func newClient(transport http.RoundTripper) *uaClient {
func newClient(transport http.RoundTripper, timeout time.Duration) *uaClient {
return &uaClient{
Client: &http.Client{
Transport: transport,
Timeout: timeout,
},
}
}
Expand Down Expand Up @@ -149,6 +151,7 @@ type ClientOption func(o *clientOptions) error

type clientOptions struct {
transport http.RoundTripper
timeout time.Duration
rootSHA256 string
rootFilename string
rootBundle []byte
Expand Down Expand Up @@ -388,6 +391,16 @@ func WithRetryFunc(fn RetryFunc) ClientOption {
}
}

// WithTimeout defines the time limit for requests made by this client. The
// timeout includes connection time, any redirects, and reading the response
// body.
func WithTimeout(d time.Duration) ClientOption {
return func(o *clientOptions) error {
o.timeout = d
return nil
}
}

func getTransportFromFile(filename string) (http.RoundTripper, error) {
data, err := os.ReadFile(filename)
if err != nil {
Expand Down Expand Up @@ -548,6 +561,7 @@ type Client struct {
client *uaClient
endpoint *url.URL
retryFunc RetryFunc
timeout time.Duration
opts []ClientOption
}

Expand All @@ -568,9 +582,10 @@ func NewClient(endpoint string, opts ...ClientOption) (*Client, error) {
}

return &Client{
client: newClient(tr),
client: newClient(tr, o.timeout),
endpoint: u,
retryFunc: o.retryFunc,
timeout: o.timeout,
opts: opts,
}, nil
}
Comment on lines 584 to 591
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also set a default timeout? I believe without a default timeout set, it will just wait until the server disconnects based on its timeouts (if any). See the output for different cases below. I know this client will be used with our CA, and we're in control over those timeouts, but it's generally a good practice to set a timeout in any http.Client that is created.

The WithContext methods can also be used, and they seem to behave as expected. However, we're not setting timeouts through the context in places where we use those methods.

# timeout on connection triggered by server; no (default) timeout in the HTTP client or context:
$ go run cmd/step/main.go ca health 
client GET https://127.0.0.1:8443/health failed: stream error: stream ID 1; INTERNAL_ERROR; received from peer

# timeout on the context from the client side:
$ go run cmd/step/main.go ca health
client GET https://127.0.0.1:8443/health failed: context deadline exceeded
exit status 1

# timeout on the client:
$ go run cmd/step/main.go ca health
client GET https://127.0.0.1:8443/health failed: context deadline exceeded (Client.Timeout exceeded while awaiting headers)
exit status 1

Expand Down Expand Up @@ -890,7 +905,7 @@ func (c *Client) RevokeWithContext(ctx context.Context, req *api.RevokeRequest,
var uaClient *uaClient
retry:
if tr != nil {
uaClient = newClient(tr)
uaClient = newClient(tr, c.timeout)
} else {
uaClient = c.client
}
Expand Down
28 changes: 28 additions & 0 deletions ca/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,34 @@ func TestClient_GetCaURL(t *testing.T) {
}
}

func TestClient_WithTimeout(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
time.Sleep(100 * time.Millisecond)
render.JSONStatus(w, r, api.HealthResponse{Status: "ok"}, 200)
}))
defer srv.Close()

tests := []struct {
name string
options []ClientOption
assertion assert.ErrorAssertionFunc
}{
{"ok", []ClientOption{WithTransport(http.DefaultTransport)}, assert.NoError},
{"ok with timeout", []ClientOption{WithTransport(http.DefaultTransport), WithTimeout(time.Second)}, assert.NoError},
{"fail with timeout", []ClientOption{WithTransport(http.DefaultTransport), WithTimeout(100 * time.Millisecond)}, assert.Error},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c, err := NewClient(srv.URL, tt.options...)
require.NoError(t, err)
_, err = c.Health()
tt.assertion(t, err)
})
}

}

func Test_enforceRequestID(t *testing.T) {
set := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody)
set.Header.Set("X-Request-Id", "already-set")
Expand Down