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

Jump to bottom

[x/net/http] Add Proxy field to HTTPClientOptions #2994

Merged
merged 3 commits into from
Dec 9, 2020
Merged

[x/net/http] Add Proxy field to HTTPClientOptions #2994

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a26a108
[x/net] Add Proxy field to HTTPClientOptions
jeromefroe Dec 8, 2020
815f13d
Set Proxy field if it is not set
jeromefroe Dec 8, 2020
fa43f4e
Ensure code is gofmt'ed
jeromefroe Dec 8, 2020
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
9 changes: 8 additions & 1 deletion src/x/net/http/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@ package xhttp
import (
"net"
"net/http"
"net/url"
"time"
)

// ProxyFunc is a type alias for the proxy function used by the standard
// library's http.Transport struct.
type ProxyFunc func(*http.Request) (*url.URL, error)

// HTTPClientOptions specify HTTP Client options.
type HTTPClientOptions struct {
RequestTimeout time.Duration `yaml:"requestTimeout"`
Expand All @@ -34,14 +39,15 @@ type HTTPClientOptions struct {
IdleConnTimeout time.Duration `yaml:"idleConnTimeout"`
MaxIdleConns int `yaml:"maxIdleConns"`
DisableCompression bool `yaml:"disableCompression"`
Proxy ProxyFunc `yaml:"proxy"`
}

// NewHTTPClient constructs a new HTTP Client.
func NewHTTPClient(o HTTPClientOptions) *http.Client {
return &http.Client{
Timeout: o.RequestTimeout,
Transport: &http.Transport{
Proxy: http.ProxyFromEnvironment,
Proxy: o.Proxy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we keep this as http.ProxyFromEnvironment unless the user specifies a non-nil proxy function?

Copy link
Collaborator Author

@jeromefroe jeromefroe Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So I set the default in DefaultHTTPClientOptions to http.ProxyFromEnvironment, do we want a check in addition to that? I'm happy to add one, it just seems odd that we would check this field only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'd feel more comfortable in case someone constructs HTTPClientOptions from scratch (not with Default...), since we've now changed what used to be an explicit default to a potentially nil value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 fair point, will update.

Dial: (&net.Dialer{
Timeout: o.ConnectTimeout,
KeepAlive: o.KeepAlive,
Expand All @@ -67,5 +73,6 @@ func DefaultHTTPClientOptions() HTTPClientOptions {
// DisableCompression is true by default since we have seen
// a large amount of overhead with compression.
DisableCompression: true,
Proxy: ProxyFunc(http.ProxyFromEnvironment),
}
}