-
Notifications
You must be signed in to change notification settings - Fork 783
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
Support custom transport dialer #1520
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ package config | |||||||||||
|
||||||||||||
import ( | ||||||||||||
"fmt" | ||||||||||||
"net" | ||||||||||||
"reflect" | ||||||||||||
"testing" | ||||||||||||
"time" | ||||||||||||
|
@@ -33,6 +34,17 @@ func TestTransportConfig_Copy(t *testing.T) { | |||||||||||
TLSHandshakeTimeout: TimeDuration(30 * time.Second), | ||||||||||||
}, | ||||||||||||
}, | ||||||||||||
{ | ||||||||||||
"same_enabled_custom_dialer", | ||||||||||||
&TransportConfig{ | ||||||||||||
CustomDialer: &net.Dialer{Timeout: 10 * time.Second}, | ||||||||||||
DisableKeepAlives: Bool(true), | ||||||||||||
IdleConnTimeout: TimeDuration(40 * time.Second), | ||||||||||||
MaxIdleConns: Int(150), | ||||||||||||
MaxIdleConnsPerHost: Int(15), | ||||||||||||
TLSHandshakeTimeout: TimeDuration(30 * time.Second), | ||||||||||||
}, | ||||||||||||
}, | ||||||||||||
} | ||||||||||||
|
||||||||||||
for i, tc := range cases { | ||||||||||||
|
@@ -245,6 +257,30 @@ func TestTransportConfig_Merge(t *testing.T) { | |||||||||||
&TransportConfig{TLSHandshakeTimeout: TimeDuration(10 * time.Second)}, | ||||||||||||
&TransportConfig{TLSHandshakeTimeout: TimeDuration(10 * time.Second)}, | ||||||||||||
}, | ||||||||||||
{ | ||||||||||||
"custom_transport_dialer", | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 20 * time.Second}}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 20 * time.Second}}, | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the second one or the one with the highest timeout win? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this test it's the second one, i.e. the "other" takes precedence in Merge: consul-template/config/transport.go Lines 89 to 93 in 799d656
|
||||||||||||
}, | ||||||||||||
{ | ||||||||||||
"custom_transport_dialer_empty_one", | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
&TransportConfig{}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
}, | ||||||||||||
{ | ||||||||||||
"custom_transport_dialer_empty_two", | ||||||||||||
&TransportConfig{}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
}, | ||||||||||||
{ | ||||||||||||
"custom_transport_dialer_same", | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
&TransportConfig{CustomDialer: &net.Dialer{Timeout: 10 * time.Second}}, | ||||||||||||
}, | ||||||||||||
} | ||||||||||||
|
||||||||||||
for i, tc := range cases { | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ import ( | |
consulapi "github.com/hashicorp/consul/api" | ||
rootcerts "github.com/hashicorp/go-rootcerts" | ||
vaultapi "github.com/hashicorp/vault/api" | ||
|
||
"github.com/hashicorp/consul-template/config" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you rework this to not import hashicorp/consul-template/config. I'll need to port this to hashicat (the library replacing consul-template's library functionality) and it doesn't have a config module like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! Is 03d3cad what you had in mind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would totally work. Thanks! |
||
) | ||
|
||
// ClientSet is a collection of clients that dependencies use to communicate | ||
|
@@ -74,6 +76,7 @@ type CreateVaultClientInput struct { | |
SSLCAPath string | ||
ServerName string | ||
|
||
TransportCustomDialer config.TransportDialer | ||
TransportDialKeepAlive time.Duration | ||
TransportDialTimeout time.Duration | ||
TransportDisableKeepAlives bool | ||
|
@@ -202,12 +205,19 @@ func (c *ClientSet) CreateVaultClient(i *CreateVaultClientInput) error { | |
} | ||
|
||
// This transport will attempt to keep connections open to the Vault server. | ||
var dialer config.TransportDialer | ||
dialer = &net.Dialer{ | ||
Timeout: i.TransportDialTimeout, | ||
KeepAlive: i.TransportDialKeepAlive, | ||
} | ||
|
||
if i.TransportCustomDialer != nil { | ||
dialer = i.TransportCustomDialer | ||
} | ||
|
||
transport := &http.Transport{ | ||
Proxy: http.ProxyFromEnvironment, | ||
Dial: (&net.Dialer{ | ||
Timeout: i.TransportDialTimeout, | ||
KeepAlive: i.TransportDialKeepAlive, | ||
}).Dial, | ||
Proxy: http.ProxyFromEnvironment, | ||
Dial: dialer.Dial, | ||
DisableKeepAlives: i.TransportDisableKeepAlives, | ||
MaxIdleConns: i.TransportMaxIdleConns, | ||
IdleConnTimeout: i.TransportIdleConnTimeout, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the interface also cover
DialContext
? Bothnet.Dialer
andbufconn.Listener
should still satisfy this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it could, but
DialContext
isn't called anywhere in consul-template that I see.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
Dial
onhttp.Transport
is deprecated, so it might be good to have this interface satisfy both in case we switch over.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Added in 9860065.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for including DialContext. There are plans on the roadmap for updating CT to use Context.