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

Support custom transport dialer #1520

Merged
merged 4 commits into from Oct 14, 2021

Conversation

tvoran
Copy link
Member

@tvoran tvoran commented Oct 12, 2021

Adds a TransportDialer interface to TransportConfig, to allow using a custom Dial() in clients such as Vault.

Vault's case is illustrated in this PR, where an in-process connection is setup between consul-template and vault-agent: hashicorp/vault#12762

Adds a TransportDialer interface to support using a custom Dial() in
the Vault client.
// TransportDialer is an interface that allows passing a custom dialer to an
// HTTP client's transport config
type TransportDialer interface {
Dial(network, address string) (net.Conn, error)
Copy link
Member

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? Both net.Dialer and bufconn.Listener should still satisfy this.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like Dial on http.Transport is deprecated, so it might be good to have this interface satisfy both in case we switch over.

Copy link
Member Author

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.

Copy link
Contributor

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.

"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}},
Copy link
Member

Choose a reason for hiding this comment

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

Does the second one or the one with the highest timeout win?

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

// Merge combines all values in this configuration with the values in the other
// configuration, with values in the other configuration taking precedence.
// Maps and slices are merged, most other values are overwritten. Complex
// structs define their own merge functionality.
func (c *TransportConfig) Merge(o *TransportConfig) *TransportConfig {

and add some doc strings
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! Is 03d3cad what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would totally work. Thanks!

@eikenb eikenb added enhancement hashicat-update-required Changes that need to be ported to hashicat vault Related to the Vault integration labels Oct 13, 2021
Copy link
Contributor

@eikenb eikenb left a comment

Choose a reason for hiding this comment

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

This looks good to me now. Let me know if you have anything else in mind.
If not I'll merge it.

@tvoran
Copy link
Member Author

tvoran commented Oct 14, 2021

@eikenb Nothing else in mind here, thanks!

@eikenb eikenb merged commit 4ff5538 into hashicorp:master Oct 14, 2021
@eikenb eikenb added this to the 0.28.0 milestone Oct 14, 2021
@eikenb eikenb modified the milestones: v0.28.0, v0.27.2 Oct 27, 2021
@eikenb eikenb added the hashicat-update-complete Completed porting changes to hashicat label Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement hashicat-update-complete Completed porting changes to hashicat hashicat-update-required Changes that need to be ported to hashicat vault Related to the Vault integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants