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

Improve http.Client usage #1891

Open
jentfoo opened this issue Jan 30, 2025 · 2 comments
Open

Improve http.Client usage #1891

jentfoo opened this issue Jan 30, 2025 · 2 comments
Assignees

Comments

@jentfoo
Copy link
Contributor

jentfoo commented Jan 30, 2025

We should improve our usage with http.Client. We don't currently reuse http clients, or define them in a consistent way in our Go code. This leads to several potential concerns:

  • A lack of reuse prevents the ability to gain potential performance gains through connection pooling. SSL handshakes are notably slow, we likely are able to achieve real performance gains by being able to reuse already established connections.
  • A lack of timeout configuration could be a point of DoS risk.
  • A lack of redirect limitations could result in a trusted source initiating a blind SSRF.

I plan to address this by creating a public function for returning a well configured http.Client from the sdk. Then utilize this client from within the SDK, platform, and backend broadly. If anyone has feedback on this design let me know in Slack or once I submit the PR.

@jentfoo jentfoo self-assigned this Jan 30, 2025
@dmihalcik
Copy link

Would callers to the sdk want to be able to configure this? Maybe it could be a configuration option to sdk.New

@jentfoo
Copy link
Contributor Author

jentfoo commented Feb 5, 2025

@dmihalcik What configuration were you thinking? For pooling I was just going to use DefaultTransport, which only pools 100 connections. That should be fine for even systems with relatively tight memory limits, while still providing a significant benefit over what we are doing today. Let me know your thoughts, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants