martian: move TLS handshake from forwarder.Listener #941
Conversation
|
The question is what do we do with the tls handshake metric? |
Listener is not the right place to do the handshake. It may block main accept loop.
|
I rebased and added a test.
That approach resigns from having a tls error metrics in forwarder. We don't have any metrics directly in martian and I see no point in adding it only for that. |
internal/martian/proxy.go
Outdated
| pc := newProxyConn(p, conn) | ||
| pc, err := newProxyConn(p, conn) | ||
| if err != nil { | ||
| log.Errorf(context.TODO(), "failed to create proxy connection: %v", err) |
There was a problem hiding this comment.
Maybe initialize would be better?
internal/martian/proxy_conn.go
Outdated
| ctx := context.Background() | ||
| if p.TLSHandshakeTimeout > 0 { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(context.Background(), p.TLSHandshakeTimeout) | ||
| defer cancel() | ||
| } | ||
| if err := tconn.HandshakeContext(ctx); err != nil { | ||
| return nil, fmt.Errorf("failed to do TLS handshake: %w", err) | ||
| } |
There was a problem hiding this comment.
Perhaps this should be extracted to method so that newProxyConn does not return error.
This would give us the opportunity to provide better errors.
There was a problem hiding this comment.
A separate method to do the handshake has been added.
internal/martian/proxy_conn.go
Outdated
| if tconn, ok := conn.(*tls.Conn); ok { | ||
| v.secure = true | ||
| v.cs = tconn.ConnectionState() | ||
| func (p *proxyConn) tlsHandshake() error { |
There was a problem hiding this comment.
I'd call that maybeHandshakeTLS
internal/martian/proxy_conn.go
Outdated
| v.secure = true | ||
| v.cs = tconn.ConnectionState() | ||
| func (p *proxyConn) tlsHandshake() error { | ||
| if tconn, ok := p.conn.(*tls.Conn); ok { |
Martian Proxy will explicitly do the handshake if accepted connection is tls.Conn.
|
LGTM |
Listener is not the right place to do the handshake. It may block main accept loop.
Martian Proxy will explicitly do the handshake if accepted connection is tls.Conn.
Consequences:
Fixes #917