-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdmain: tls listener MUST be at the outer layer of all listeners #4153
Conversation
return nil, err | ||
} | ||
|
||
func NewKeepAliveListener(l net.Listener, scheme string, info TLSInfo) (net.Listener, error) { |
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.
So this patch is doing the same thing as the old code but it's passing a Listener
into NewKeepAliveListener
instead of expecting it to create one for you? There should be a comment here about why this is important.
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.
@heyitsanthony Right. The TLS one must be the most outside listener. So we need to prepare inner listener and then pass in that listener. I will document it.
lgtm aside from documentation (and the typo in the commit message); it'd probably be good to type assert all custom wrappers to error out or panic if wrapping tls listeners but that might be overkill |
cc524f8
to
605a6b1
Compare
@heyitsanthony All fixed. Add a TODO for type assertion before passing the listener to HTTP server. |
ok LGTM, if CI is giving false positives. please fix the commit title though |
go HTTP library uses type assertion to determine if a connection is a TLS connection. If we wrapper TLS Listener with any customized Listener that can create customized Conn, HTTPs will be broken. This commit fixes the issue.
etcdmain: tls listener MUST be at the outer layer of all listeners
go HTTP library uses type assertion to determine if a connection
is a TLS connection. If we wrapper TLS Listener with any customized
Listener that can create customized Conn, HTTPs will be broken.
This commit fixes the issue.
/cc @heyitsanthony I want to keep NewListener behaviors the same although we do not wrap it with any other listener now. While you were on it today, can you help to change it?