From be609bf065b4432d2628541b05429d449bf26953 Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Wed, 27 Aug 2025 11:04:48 +0200 Subject: [PATCH] server/embed: Log EOF on DEBUG in TLS handshake When EOF is encountered during TLS handshake, it is still logged as a warning. This seems excessive and not really useful. This patch ensures EOF is logged on debug only. Unit tests are also added, but since the handler doesn't use an interface for passing the TLS connection in, mocking that is hard. It's achieved by using a private global variable, which is really not ideal, but given the situation, it's the best tradeoff between the code being pretty and complex. Signed-off-by: Ondra Kupka --- server/embed/config_logging.go | 30 +++++++- server/embed/config_logging_test.go | 106 ++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 server/embed/config_logging_test.go diff --git a/server/embed/config_logging.go b/server/embed/config_logging.go index c9da6260d519..438342ad349e 100644 --- a/server/embed/config_logging.go +++ b/server/embed/config_logging.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io" + "net" "net/url" "os" @@ -33,6 +34,17 @@ import ( "go.etcd.io/etcd/client/pkg/v3/logutil" ) +// tlsConn is the minimal interface for mocking *tls.Conn for the TLS handshake failure handler. +// Only used for testing as we cannot really chang the public interface using *tls.Conn. +type tlsConn interface { + ConnectionState() tls.ConnectionState + RemoteAddr() net.Addr +} + +// testingHandshakeFailureTLSConn is used to replace the real TLS conn when testing the TLS handshake failure handler. +// Only used for testing. +var testingHandshakeFailureTLSConn tlsConn + // GetLogger returns the logger. func (cfg *Config) GetLogger() *zap.Logger { cfg.loggerMu.RLock() @@ -166,7 +178,19 @@ func (cfg *Config) setupLogging() error { } logTLSHandshakeFailureFunc := func(msg string) func(conn *tls.Conn, err error) { - return func(conn *tls.Conn, err error) { + return func(c *tls.Conn, err error) { + // This is seriously ugly, but it's the only simply way to mock the connection for testing. + var conn tlsConn = c + if testingHandshakeFailureTLSConn != nil { + conn = testingHandshakeFailureTLSConn + } + + // Log EOF errors on DEBUG not to spam logs too much. + log := cfg.logger.Warn + if errors.Is(err, io.EOF) { + log = cfg.logger.Debug + } + state := conn.ConnectionState() remoteAddr := conn.RemoteAddr().String() serverName := state.ServerName @@ -176,7 +200,7 @@ func (cfg *Config) setupLogging() error { for i := range cert.IPAddresses { ips[i] = cert.IPAddresses[i].String() } - cfg.logger.Warn( + log( msg, zap.String("remote-addr", remoteAddr), zap.String("server-name", serverName), @@ -185,7 +209,7 @@ func (cfg *Config) setupLogging() error { zap.Error(err), ) } else { - cfg.logger.Warn( + log( msg, zap.String("remote-addr", remoteAddr), zap.String("server-name", serverName), diff --git a/server/embed/config_logging_test.go b/server/embed/config_logging_test.go new file mode 100644 index 000000000000..673e595e287e --- /dev/null +++ b/server/embed/config_logging_test.go @@ -0,0 +1,106 @@ +// Copyright 2025 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package embed + +import ( + "crypto/tls" + "errors" + "io" + "net" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest" +) + +type mockTLSConn struct { + state tls.ConnectionState + remoteAddr net.Addr +} + +func (m *mockTLSConn) ConnectionState() tls.ConnectionState { + return m.state +} + +func (m *mockTLSConn) RemoteAddr() net.Addr { + return m.remoteAddr +} + +func TestConfig_SetupLogging_TLSHandshakeFailureFunc(t *testing.T) { + conn := mockTLSConn{ + remoteAddr: &net.TCPAddr{ + IP: net.IPv4(127, 0, 0, 1), + Port: 8000, + }, + } + + testingHandshakeFailureTLSConn = &conn + defer func() { + testingHandshakeFailureTLSConn = nil + }() + + testCases := []struct { + name string + err error + expectedLogEntryPrefix string + }{ + { + name: "custom error", + err: errors.New("nuked"), + expectedLogEntryPrefix: "WARN", + }, + { + name: "EOF error", + err: io.EOF, + expectedLogEntryPrefix: "DEBUG", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var out zaptest.Buffer + + encoderConfig := zap.NewDevelopmentEncoderConfig() + encoderConfig.EncodeTime = nil + + logger := zap.New(zapcore.NewCore( + zapcore.NewConsoleEncoder(encoderConfig), + &out, + zapcore.DebugLevel, + )) + + c := NewConfig() + c.ZapLoggerBuilder = NewZapLoggerBuilder(logger) + if err := c.Validate(); err != nil { + t.Fatal("Failed to validate config:", err) + } + + out.Reset() + c.ClientTLSInfo.HandshakeFailure(nil, tc.err) + + got := out.Lines() + if len(got) != 1 { + t.Fatal("Expected 1 log entry, got log entries:", got) + } + + line := got[0] + if !strings.HasPrefix(line, tc.expectedLogEntryPrefix) { + t.Errorf("Expected log entry prefix %q, got log entry: %s", tc.expectedLogEntryPrefix, got[0]) + } + }) + } +}