-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
agent: Use an in-process listener with cache #12762
Conversation
Uses a bufconn listener between consul-template and vault-agent, when cache and templates are in use. This means no listeners need to be defined in vault-agent for just templating. Always routes consul-template through the vault-agent cache (instead of only when persistent cache is enabled).
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.
Looks good! I love that there are no config options for it - super clean UX
Co-authored-by: Tom Proctor <[email protected]>
import ( | ||
"net" | ||
|
||
ctconfig "github.com/hashicorp/consul-template/config" |
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 wasn't sure if there was a reason to avoid including this dependency, because it's only there for the _ ctconfig.TransportDialer = (*BufConnListenerDialer)(nil)
line. But I see no harm in having it personally.
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.
An option would be to define a local (internal?) interface that is also satisfied by net.Dialer
, especially if we also need to port this over to go-secure-stdlib/listenerutil
.
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 attempted to do something like this in 8e26033. Let me know if that looks better.
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.
LGTM! Nice work 🎉
…e-in-process-listener
// TransportDialer, to serve both ends of an in-process connection (Dial and | ||
// Accept). | ||
type BufConnListenerDialer struct { | ||
listener *bufconn.Listener |
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.
Do we need to wrap this, or can we use buffconn.Listener directly?
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 think we will still need to wrap it for the Dial()
function, since bufconn's Dial is
func (l *Listener) Dial() (net.Conn, error)
and net.Dialer is
func (d *Dialer) Dial(network, address string) (Conn, error)
But you're right, bufconn.Listener implements all the net.Listener interface, so we could reduce this interface to just implement TransportDialer. I'll see what that looks like.
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 made some changes to that effect in 8e26033.
if lnConfig.Type == "bufconn" { | ||
inProcListener := listenerutil.NewBufConnListenerDialer() | ||
config.Cache.InProcDialer = inProcListener | ||
ln = inProcListener |
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.
Isln.Addr().String()
empty at this point? Asking since we're populating info[infoKey]
with this value further below.
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.
No it comes back as bufconn
at this point.
Uses a local transportDialer interface in config.Cache{}. Adds DialContext() to BufConnListenerDialer.
Remove the pointer to my fork
Use a "bufconn" const, remove unused parameters.
…e-in-process-listener
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.
Very nice!
Co-authored-by: Ben Ash <[email protected]>
Uses a bufconn listener between consul-template and vault-agent, when caching is enabled and either templates or a listener is defined. This means no listeners need to be defined in vault-agent for just templating. Always routes consul-template through the vault-agent cache (instead of only when persistent cache is enabled).
Depends on these config changes in consul-template: hashicorp/consul-template#1520