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.

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
Open

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Feb 25, 2025

Description

This commit adds a new option to specify the client timeout to the CA client.

Fixes #2176

@maraino maraino requested a review from hslatman February 25, 2025 19:31
@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Feb 25, 2025
This commit adds to the ca Client a new option to specify the client
timeout.

Fixes #2176
@hslatman hslatman added this to the v0.28.3 milestone Feb 27, 2025
Comment on lines 584 to 591
return &Client{
client: newClient(tr),
client: newClient(tr, o.timeout),
endpoint: u,
retryFunc: o.retryFunc,
timeout: o.timeout,
opts: opts,
}, nil
}
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

@hslatman hslatman self-assigned this Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ca.NewClient don't allow setting a timeout for the http client
2 participants