From e0433c8468c5a4e598cb34180532072c18e2a876 Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Wed, 8 Jan 2025 09:11:47 -0500 Subject: [PATCH 1/5] Remove all direct logrus usage from teleport module The only remaining use of logrus is from integrations, which is unfortunately imported by teleport.e, and prevents logrus from being moved to an indirect dependency. The logrus formatter and initialization of the logrus logger will remain in place until integrations is using slog. To prevent any accidental inclusions of logrus within the teleport module the depguard rules have been updated to prohibit importing logrus. The rules also include prohibit a few common log packages that tools like gopls might automatically import. --- .golangci.yml | 29 ++++++--- e | 2 +- integration/helpers/instance.go | 9 --- integration/hostuser_test.go | 4 +- lib/client/api.go | 20 +++++- lib/service/service.go | 5 -- lib/service/service_test.go | 1 - lib/service/servicecfg/config.go | 15 ----- lib/service/servicecfg/config_test.go | 23 ++----- lib/utils/log/formatter_test.go | 2 +- lib/utils/log/handle_state.go | 8 --- lib/utils/log/levels.go | 54 ---------------- lib/utils/log/logrus_formatter.go | 61 ------------------ lib/utils/log/slog.go | 90 +++++++++++++++++++++++++++ lib/utils/log/slog_text_handler.go | 7 --- lib/utils/syslog.go | 42 ------------- lib/utils/syslog_windows.go | 6 -- tool/tbot/spiffe.go | 32 +++++++++- tool/tctl/common/resource_command.go | 22 +++---- tool/teleport/common/teleport.go | 2 +- tool/teleport/testenv/test_server.go | 2 +- tool/tsh/common/tsh_helper_test.go | 4 +- tool/tsh/common/tsh_test.go | 4 +- 23 files changed, 183 insertions(+), 261 deletions(-) delete mode 100644 lib/utils/log/levels.go diff --git a/.golangci.yml b/.golangci.yml index 229a5838f2462..98859bad6c7d9 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -99,10 +99,6 @@ linters-settings: desc: 'use "github.com/google/uuid" instead' - pkg: github.com/pborman/uuid desc: 'use "github.com/google/uuid" instead' - - pkg: github.com/siddontang/go-log/log - desc: 'use "github.com/sirupsen/logrus" instead' - - pkg: github.com/siddontang/go/log - desc: 'use "github.com/sirupsen/logrus" instead' - pkg: github.com/tj/assert desc: 'use "github.com/stretchr/testify/assert" instead' - pkg: go.uber.org/atomic @@ -117,16 +113,29 @@ linters-settings: desc: 'use "github.com/gravitational/teleport/lib/msgraph" instead' - pkg: github.com/cloudflare/cfssl desc: 'use "crypto" or "x/crypto" instead' - # Prevent logrus from being imported by api and e. Once everything in teleport has been converted - # to use log/slog this should be moved into the main block above. - logrus: + # Prevent importing any additional logging libraries. + logging: files: - - '**/api/**' - - '**/e/**' - - '**/lib/srv/**' + # Integrations are still allowed to use logrus becuase they haven't + # been converted to slog yet. Once they use slog, remove this exception. + - '!**/integrations/**' + # The log package still contains the logrus formatter consumed by the integrations. + # Remove this exception when said formatter is deleted. + - '!**/lib/utils/log/**' + - '!**/lib/utils/cli.go' deny: - pkg: github.com/sirupsen/logrus desc: 'use "log/slog" instead' + - pkg: github.com/siddontang/go-log/log + desc: 'use "log/slog" instead' + - pkg: github.com/siddontang/go/log + desc: 'use "log/slog" instead' + - pkg: github.com/mailgun/log + desc: 'use "log/slog" instead' + - pkg: github.com/saferwall/pe/log + desc: 'use "log/slog" instead' + - pkg: golang.org/x/exp/slog + desc: 'use "log/slog" instead' # Prevent importing internal packages in client tools or packages containing # common interfaces consumed by them that are known to bloat binaries or break builds # because they only support a single platform. diff --git a/e b/e index b486de24a443a..498f643ea9033 160000 --- a/e +++ b/e @@ -1 +1 @@ -Subproject commit b486de24a443a9f8eb3e349009af14d11814ff5c +Subproject commit 498f643ea9033b1235359d83c310caadb18305d2 diff --git a/integration/helpers/instance.go b/integration/helpers/instance.go index 5f652c77b4eea..6d375387a02f6 100644 --- a/integration/helpers/instance.go +++ b/integration/helpers/instance.go @@ -40,7 +40,6 @@ import ( "github.com/gorilla/websocket" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" @@ -327,10 +326,6 @@ type InstanceConfig struct { Priv []byte // Pub is SSH public key of the instance Pub []byte - // Log specifies the logger - // Deprecated: Use Logger instead - // TODO(tross): Delete when e is updated - Log utils.Logger // Logger specifies the logger Logger *slog.Logger // Ports is a collection of instance ports. @@ -354,10 +349,6 @@ func NewInstance(t *testing.T, cfg InstanceConfig) *TeleInstance { cfg.Listeners = StandardListenerSetup(t, &cfg.Fds) } - if cfg.Log == nil { - cfg.Log = logrus.New() - } - if cfg.Logger == nil { cfg.Logger = slog.New(logutils.DiscardHandler{}) } diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index b5b045c2840b3..f917a95f872a5 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -637,7 +637,7 @@ func TestRootLoginAsHostUser(t *testing.T) { NodeName: Host, Priv: privateKey, Pub: publicKey, - Log: utils.NewLoggerForTests(), + Logger: utils.NewSlogLoggerForTests(), }) // Create a user that can create a host user. @@ -735,7 +735,7 @@ func TestRootStaticHostUsers(t *testing.T) { NodeName: Host, Priv: privateKey, Pub: publicKey, - Log: utils.NewLoggerForTests(), + Logger: utils.NewSlogLoggerForTests(), }) require.NoError(t, instance.Create(t, nil, false, nil)) diff --git a/lib/client/api.go b/lib/client/api.go index 68084d4833089..ed94462aa9c73 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -35,6 +35,7 @@ import ( "slices" "strconv" "strings" + "sync" "sync/atomic" "time" "unicode/utf8" @@ -2850,6 +2851,21 @@ type execResult struct { exitStatus int } +// sharedWriter is an [io.Writer] implementation that protects +// writes with a mutex. This allows a single [io.Writer] to be shared +// by both logrus and slog without their output clobbering each other. +type sharedWriter struct { + mu sync.Mutex + io.Writer +} + +func (s *sharedWriter) Write(p []byte) (int, error) { + s.mu.Lock() + defer s.mu.Unlock() + + return s.Writer.Write(p) +} + // runCommandOnNodes executes a given bash command on a bunch of remote nodes. func (tc *TeleportClient) runCommandOnNodes(ctx context.Context, clt *ClusterClient, nodes []TargetNode, command []string) error { cluster := clt.ClusterName() @@ -2909,10 +2925,10 @@ func (tc *TeleportClient) runCommandOnNodes(ctx context.Context, clt *ClusterCli } } - stdout := logutils.NewSharedWriter(tc.Stdout) + stdout := &sharedWriter{Writer: tc.Stdout} stderr := stdout if tc.Stdout != tc.Stderr { - stderr = logutils.NewSharedWriter(tc.Stderr) + stderr = &sharedWriter{Writer: tc.Stderr} } for _, node := range nodes { diff --git a/lib/service/service.go b/lib/service/service.go index 51d171f3737b3..7638ee5e85caf 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -56,7 +56,6 @@ import ( "github.com/jonboulle/clockwork" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/quic-go/quic-go" - "github.com/sirupsen/logrus" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "go.opentelemetry.io/otel/attribute" "golang.org/x/crypto/acme" @@ -992,10 +991,6 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) { } processID := fmt.Sprintf("%v", nextProcessID()) - cfg.Log = utils.WrapLogger(cfg.Log.WithFields(logrus.Fields{ - teleport.ComponentKey: teleport.Component(teleport.ComponentProcess, processID), - "pid": fmt.Sprintf("%v.%v", os.Getpid(), processID), - })) cfg.Logger = cfg.Logger.With( teleport.ComponentKey, teleport.Component(teleport.ComponentProcess, processID), "pid", fmt.Sprintf("%v.%v", os.Getpid(), processID), diff --git a/lib/service/service_test.go b/lib/service/service_test.go index 16309ed59ac72..c66fb56ef20e5 100644 --- a/lib/service/service_test.go +++ b/lib/service/service_test.go @@ -948,7 +948,6 @@ func TestTeleportProcess_reconnectToAuth(t *testing.T) { cfg.Testing.ConnectFailureC = make(chan time.Duration, 5) cfg.Testing.ClientTimeout = time.Millisecond cfg.InstanceMetadataClient = imds.NewDisabledIMDSClient() - cfg.Log = utils.NewLoggerForTests() cfg.Logger = utils.NewSlogLoggerForTests() process, err := NewTeleport(cfg) require.NoError(t, err) diff --git a/lib/service/servicecfg/config.go b/lib/service/servicecfg/config.go index 6a14f1ceba5d0..a89e79a8f6302 100644 --- a/lib/service/servicecfg/config.go +++ b/lib/service/servicecfg/config.go @@ -34,7 +34,6 @@ import ( "github.com/ghodss/yaml" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" - "github.com/sirupsen/logrus" "golang.org/x/crypto/ssh" "github.com/gravitational/teleport" @@ -52,7 +51,6 @@ import ( "github.com/gravitational/teleport/lib/sshca" usagereporter "github.com/gravitational/teleport/lib/usagereporter/teleport" "github.com/gravitational/teleport/lib/utils" - logutils "github.com/gravitational/teleport/lib/utils/log" ) // Config contains the configuration for all services that Teleport can run. @@ -223,10 +221,6 @@ type Config struct { // Kube is a Kubernetes API gateway using Teleport client identities. Kube KubeConfig - // Log optionally specifies the logger. - // Deprecated: use Logger instead. - Log utils.Logger - // Logger outputs messages using slog. The underlying handler respects // the user supplied logging config. Logger *slog.Logger @@ -518,10 +512,6 @@ func ApplyDefaults(cfg *Config) { cfg.Version = defaults.TeleportConfigVersionV1 - if cfg.Log == nil { - cfg.Log = utils.NewLogger() - } - if cfg.Logger == nil { cfg.Logger = slog.Default() } @@ -698,10 +688,6 @@ func applyDefaults(cfg *Config) { cfg.Console = io.Discard } - if cfg.Log == nil { - cfg.Log = logrus.StandardLogger() - } - if cfg.Logger == nil { cfg.Logger = slog.Default() } @@ -799,7 +785,6 @@ func verifyEnabledService(cfg *Config) error { // If called after `config.ApplyFileConfig` or `config.Configure` it will also // change the global loggers. func (c *Config) SetLogLevel(level slog.Level) { - c.Log.SetLevel(logutils.SlogLevelToLogrusLevel(level)) c.LoggerLevel.Set(level) } diff --git a/lib/service/servicecfg/config_test.go b/lib/service/servicecfg/config_test.go index e9a6be2df4056..8ed785c0998f9 100644 --- a/lib/service/servicecfg/config_test.go +++ b/lib/service/servicecfg/config_test.go @@ -28,7 +28,6 @@ import ( "testing" "github.com/gravitational/trace" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "github.com/gravitational/teleport/api/types" @@ -651,44 +650,34 @@ func TestWebPublicAddr(t *testing.T) { func TestSetLogLevel(t *testing.T) { for _, test := range []struct { - logLevel slog.Level - expectedLogrusLevel logrus.Level + logLevel slog.Level }{ { - logLevel: logutils.TraceLevel, - expectedLogrusLevel: logrus.TraceLevel, + logLevel: logutils.TraceLevel, }, { - logLevel: slog.LevelDebug, - expectedLogrusLevel: logrus.DebugLevel, + logLevel: slog.LevelDebug, }, { - logLevel: slog.LevelInfo, - expectedLogrusLevel: logrus.InfoLevel, + logLevel: slog.LevelInfo, }, { - logLevel: slog.LevelWarn, - expectedLogrusLevel: logrus.WarnLevel, + logLevel: slog.LevelWarn, }, { - logLevel: slog.LevelError, - expectedLogrusLevel: logrus.ErrorLevel, + logLevel: slog.LevelError, }, } { t.Run(test.logLevel.String(), func(t *testing.T) { // Create a configuration with local loggers to avoid modifying the // global instances. c := &Config{ - Log: logrus.New(), Logger: slog.New(logutils.NewSlogTextHandler(io.Discard, logutils.SlogTextHandlerConfig{})), } ApplyDefaults(c) c.SetLogLevel(test.logLevel) require.Equal(t, test.logLevel, c.LoggerLevel.Level()) - require.IsType(t, &logrus.Logger{}, c.Log) - l, _ := c.Log.(*logrus.Logger) - require.Equal(t, test.expectedLogrusLevel, l.GetLevel()) }) } } diff --git a/lib/utils/log/formatter_test.go b/lib/utils/log/formatter_test.go index e11a9f63620fb..9abb0310ba0be 100644 --- a/lib/utils/log/formatter_test.go +++ b/lib/utils/log/formatter_test.go @@ -51,7 +51,7 @@ var ( logErr = errors.New("the quick brown fox jumped really high") addr = fakeAddr{addr: "127.0.0.1:1234"} - fields = logrus.Fields{ + fields = map[string]any{ "local": &addr, "remote": &addr, "login": "llama", diff --git a/lib/utils/log/handle_state.go b/lib/utils/log/handle_state.go index c8ac9913781ca..3f88e100933ac 100644 --- a/lib/utils/log/handle_state.go +++ b/lib/utils/log/handle_state.go @@ -14,7 +14,6 @@ import ( "time" "github.com/gravitational/trace" - "github.com/sirupsen/logrus" "github.com/gravitational/teleport" ) @@ -114,13 +113,6 @@ func (s *handleState) appendAttr(a slog.Attr) bool { } } return nonEmpty - case logrus.Fields: - for k, v := range fields { - if s.appendAttr(slog.Any(k, v)) { - nonEmpty = true - } - } - return nonEmpty } } diff --git a/lib/utils/log/levels.go b/lib/utils/log/levels.go deleted file mode 100644 index 747561ffb155b..0000000000000 --- a/lib/utils/log/levels.go +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Teleport - * Copyright (C) 2023 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -package log - -import ( - "log/slog" - - "github.com/sirupsen/logrus" -) - -// SupportedLevelsText lists the supported log levels in their text -// representation. All strings are in uppercase. -var SupportedLevelsText = []string{ - TraceLevelText, - slog.LevelDebug.String(), - slog.LevelInfo.String(), - slog.LevelWarn.String(), - slog.LevelError.String(), -} - -// SlogLevelToLogrusLevel converts a [slog.Level] to its equivalent -// [logrus.Level]. -func SlogLevelToLogrusLevel(level slog.Level) logrus.Level { - switch level { - case TraceLevel: - return logrus.TraceLevel - case slog.LevelDebug: - return logrus.DebugLevel - case slog.LevelInfo: - return logrus.InfoLevel - case slog.LevelWarn: - return logrus.WarnLevel - case slog.LevelError: - return logrus.ErrorLevel - default: - return logrus.FatalLevel - } -} diff --git a/lib/utils/log/logrus_formatter.go b/lib/utils/log/logrus_formatter.go index a21d922adf809..14ad8441da7cc 100644 --- a/lib/utils/log/logrus_formatter.go +++ b/lib/utils/log/logrus_formatter.go @@ -25,7 +25,6 @@ import ( "slices" "strconv" "strings" - "unicode" "github.com/gravitational/trace" "github.com/sirupsen/logrus" @@ -76,27 +75,6 @@ func (w *writer) Bytes() []byte { return *w.b } -const ( - noColor = -1 - red = 31 - yellow = 33 - blue = 36 - gray = 37 - // LevelField is the log field that stores the verbosity. - LevelField = "level" - // ComponentField is the log field that stores the calling component. - ComponentField = "component" - // CallerField is the log field that stores the calling file and line number. - CallerField = "caller" - // TimestampField is the field that stores the timestamp the log was emitted. - TimestampField = "timestamp" - messageField = "message" - // defaultComponentPadding is a default padding for component field - defaultComponentPadding = 11 - // defaultLevelPadding is a default padding for level field - defaultLevelPadding = 4 -) - // NewDefaultTextFormatter creates a TextFormatter with // the default options set. func NewDefaultTextFormatter(enableColors bool) *TextFormatter { @@ -304,15 +282,6 @@ func (w *writer) writeError(value interface{}) { } } -func padMax(in string, chars int) string { - switch { - case len(in) < chars: - return in + strings.Repeat(" ", chars-len(in)) - default: - return in[:chars] - } -} - func (w *writer) writeField(value interface{}, color int) { if w.Len() > 0 { w.WriteByte(' ') @@ -456,33 +425,3 @@ func frameToTrace(frame runtime.Frame) trace.Trace { Line: frame.Line, } } - -var defaultFormatFields = []string{LevelField, ComponentField, CallerField, TimestampField} - -var knownFormatFields = map[string]struct{}{ - LevelField: {}, - ComponentField: {}, - CallerField: {}, - TimestampField: {}, -} - -func ValidateFields(formatInput []string) (result []string, err error) { - for _, component := range formatInput { - component = strings.TrimSpace(component) - if _, ok := knownFormatFields[component]; !ok { - return nil, trace.BadParameter("invalid log format key: %q", component) - } - result = append(result, component) - } - return result, nil -} - -// needsQuoting returns true if any non-printable characters are found. -func needsQuoting(text string) bool { - for _, r := range text { - if !unicode.IsPrint(r) { - return true - } - } - return false -} diff --git a/lib/utils/log/slog.go b/lib/utils/log/slog.go index b1b0678ec5487..9b074ff56abad 100644 --- a/lib/utils/log/slog.go +++ b/lib/utils/log/slog.go @@ -24,7 +24,10 @@ import ( "log/slog" "reflect" "strings" + "unicode" + "github.com/gravitational/trace" + "github.com/sirupsen/logrus" oteltrace "go.opentelemetry.io/otel/trace" ) @@ -34,8 +37,56 @@ const ( // TraceLevelText is the text representation of Trace verbosity. TraceLevelText = "TRACE" + + noColor = -1 + red = 31 + yellow = 33 + blue = 36 + gray = 37 + // LevelField is the log field that stores the verbosity. + LevelField = "level" + // ComponentField is the log field that stores the calling component. + ComponentField = "component" + // CallerField is the log field that stores the calling file and line number. + CallerField = "caller" + // TimestampField is the field that stores the timestamp the log was emitted. + TimestampField = "timestamp" + messageField = "message" + // defaultComponentPadding is a default padding for component field + defaultComponentPadding = 11 + // defaultLevelPadding is a default padding for level field + defaultLevelPadding = 4 ) +// SupportedLevelsText lists the supported log levels in their text +// representation. All strings are in uppercase. +var SupportedLevelsText = []string{ + TraceLevelText, + slog.LevelDebug.String(), + slog.LevelInfo.String(), + slog.LevelWarn.String(), + slog.LevelError.String(), +} + +// SlogLevelToLogrusLevel converts a [slog.Level] to its equivalent +// [logrus.Level]. +func SlogLevelToLogrusLevel(level slog.Level) logrus.Level { + switch level { + case TraceLevel: + return logrus.TraceLevel + case slog.LevelDebug: + return logrus.DebugLevel + case slog.LevelInfo: + return logrus.InfoLevel + case slog.LevelWarn: + return logrus.WarnLevel + case slog.LevelError: + return logrus.ErrorLevel + default: + return logrus.FatalLevel + } +} + // DiscardHandler is a [slog.Handler] that discards all messages. It // is more efficient than a [slog.Handler] which outputs to [io.Discard] since // it performs zero formatting. @@ -68,6 +119,45 @@ func addTracingContextToRecord(ctx context.Context, r *slog.Record) { } } +var defaultFormatFields = []string{LevelField, ComponentField, CallerField, TimestampField} + +var knownFormatFields = map[string]struct{}{ + LevelField: {}, + ComponentField: {}, + CallerField: {}, + TimestampField: {}, +} + +func ValidateFields(formatInput []string) (result []string, err error) { + for _, component := range formatInput { + component = strings.TrimSpace(component) + if _, ok := knownFormatFields[component]; !ok { + return nil, trace.BadParameter("invalid log format key: %q", component) + } + result = append(result, component) + } + return result, nil +} + +// needsQuoting returns true if any non-printable characters are found. +func needsQuoting(text string) bool { + for _, r := range text { + if !unicode.IsPrint(r) { + return true + } + } + return false +} + +func padMax(in string, chars int) string { + switch { + case len(in) < chars: + return in + strings.Repeat(" ", chars-len(in)) + default: + return in[:chars] + } +} + // getCaller retrieves source information from the attribute // and returns the file and line of the caller. The file is // truncated from the absolute path to package/filename. diff --git a/lib/utils/log/slog_text_handler.go b/lib/utils/log/slog_text_handler.go index b3bc4900ac64c..7f93a388977bb 100644 --- a/lib/utils/log/slog_text_handler.go +++ b/lib/utils/log/slog_text_handler.go @@ -27,7 +27,6 @@ import ( "sync" "github.com/gravitational/trace" - "github.com/sirupsen/logrus" "github.com/gravitational/teleport" ) @@ -324,12 +323,6 @@ func (s *SlogTextHandler) WithAttrs(attrs []slog.Attr) slog.Handler { nonEmpty = true } } - case logrus.Fields: - for k, v := range fields { - if state.appendAttr(slog.Any(k, v)) { - nonEmpty = true - } - } } default: if state.appendAttr(a) { diff --git a/lib/utils/syslog.go b/lib/utils/syslog.go index 86123bda5e1c0..d028563349cfe 100644 --- a/lib/utils/syslog.go +++ b/lib/utils/syslog.go @@ -24,52 +24,10 @@ package utils import ( "io" "log/syslog" - "os" "github.com/gravitational/trace" - "github.com/sirupsen/logrus" - logrusSyslog "github.com/sirupsen/logrus/hooks/syslog" ) -// SwitchLoggingToSyslog configures the default logger to send output to syslog. -func SwitchLoggingToSyslog() error { - logger := logrus.StandardLogger() - - w, err := NewSyslogWriter() - if err != nil { - logger.Errorf("Failed to switch logging to syslog: %v.", err) - logger.SetOutput(os.Stderr) - return trace.Wrap(err) - } - - hook, err := NewSyslogHook(w) - if err != nil { - logger.Errorf("Failed to switch logging to syslog: %v.", err) - logger.SetOutput(os.Stderr) - return trace.Wrap(err) - } - - logger.ReplaceHooks(make(logrus.LevelHooks)) - logger.AddHook(hook) - logger.SetOutput(io.Discard) - - return nil -} - -// NewSyslogHook provides a [logrus.Hook] that sends output to syslog. -func NewSyslogHook(w io.Writer) (logrus.Hook, error) { - if w == nil { - return nil, trace.BadParameter("syslog writer must not be nil") - } - - sw, ok := w.(*syslog.Writer) - if !ok { - return nil, trace.BadParameter("expected a syslog writer, got %T", w) - } - - return &logrusSyslog.SyslogHook{Writer: sw}, nil -} - // NewSyslogWriter creates a writer that outputs to the local machine syslog. func NewSyslogWriter() (io.Writer, error) { writer, err := syslog.Dial("", "", syslog.LOG_WARNING, "") diff --git a/lib/utils/syslog_windows.go b/lib/utils/syslog_windows.go index 7812dddabb237..27f4d45e3762f 100644 --- a/lib/utils/syslog_windows.go +++ b/lib/utils/syslog_windows.go @@ -22,14 +22,8 @@ import ( "io" "github.com/gravitational/trace" - "github.com/sirupsen/logrus" ) -// NewSyslogHook always returns an error on Windows. -func NewSyslogHook(io.Writer) (logrus.Hook, error) { - return nil, trace.NotImplemented("cannot use syslog on Windows") -} - // NewSyslogWriter always returns an error on Windows. func NewSyslogWriter() (io.Writer, error) { return nil, trace.NotImplemented("cannot use syslog on Windows") diff --git a/tool/tbot/spiffe.go b/tool/tbot/spiffe.go index f319505de8caa..9ba58f3c94df0 100644 --- a/tool/tbot/spiffe.go +++ b/tool/tbot/spiffe.go @@ -21,22 +21,48 @@ package main import ( "context" "fmt" + "log/slog" "time" "github.com/gravitational/trace" "github.com/spiffe/go-spiffe/v2/svid/jwtsvid" "github.com/spiffe/go-spiffe/v2/workloadapi" - - "github.com/gravitational/teleport/lib/utils" ) +// TODO(tross/noah): Remove once go-spiff has a slog<->workloadapi.Logger adapter. +// https://github.com/spiffe/go-spiffe/issues/281 +type logger struct { + l *slog.Logger +} + +func (l logger) Debugf(format string, args ...interface{}) { + //nolint:sloglint // msg cannot be constant + l.l.DebugContext(context.Background(), fmt.Sprintf(format, args...)) +} + +func (l logger) Infof(format string, args ...interface{}) { + //nolint:sloglint // msg cannot be constant + l.l.InfoContext(context.Background(), fmt.Sprintf(format, args...)) +} + +func (l logger) Warnf(format string, args ...interface{}) { + //nolint:sloglint // msg cannot be constant + l.l.WarnContext(context.Background(), fmt.Sprintf(format, args...)) +} + +func (l logger) Errorf(format string, args ...interface{}) { + //nolint:sloglint // msg cannot be constant + l.l.ErrorContext(context.Background(), fmt.Sprintf(format, args...)) +} + func onSPIFFEInspect(ctx context.Context, path string) error { log.InfoContext(ctx, "Inspecting SPIFFE Workload API Endpoint", "path", path) source, err := workloadapi.New( ctx, // TODO(noah): Upstream PR to add slog<->workloadapi.Logger adapter. - workloadapi.WithLogger(utils.NewLogger()), + // https://github.com/spiffe/go-spiffe/issues/281 + workloadapi.WithLogger(logger{l: slog.Default()}), workloadapi.WithAddr(path), ) if err != nil { diff --git a/tool/tctl/common/resource_command.go b/tool/tctl/common/resource_command.go index 23749bc14c528..7ea28fc994402 100644 --- a/tool/tctl/common/resource_command.go +++ b/tool/tctl/common/resource_command.go @@ -507,7 +507,7 @@ func (rc *ResourceCommand) createRole(ctx context.Context, client *authclient.Cl return trace.Wrap(err) } - warnAboutKubernetesResources(rc.config.Log, role) + warnAboutKubernetesResources(ctx, rc.config.Logger, role) roleName := role.GetName() _, err = client.GetRole(ctx, roleName) @@ -536,8 +536,8 @@ func (rc *ResourceCommand) updateRole(ctx context.Context, client *authclient.Cl return trace.Wrap(err) } - warnAboutKubernetesResources(rc.config.Log, role) - warnAboutDynamicLabelsInDenyRule(rc.config.Log, role) + warnAboutKubernetesResources(ctx, rc.config.Logger, role) + warnAboutDynamicLabelsInDenyRule(ctx, rc.config.Logger, role) if _, err := client.UpdateRole(ctx, role); err != nil { return trace.Wrap(err) @@ -548,21 +548,21 @@ func (rc *ResourceCommand) updateRole(ctx context.Context, client *authclient.Cl // warnAboutKubernetesResources warns about kubernetes resources // if kubernetes_labels are set but kubernetes_resources are not. -func warnAboutKubernetesResources(logger utils.Logger, r types.Role) { +func warnAboutKubernetesResources(ctx context.Context, logger *slog.Logger, r types.Role) { role, ok := r.(*types.RoleV6) // only warn about kubernetes resources for v6 roles if !ok || role.Version != types.V6 { return } if len(role.Spec.Allow.KubernetesLabels) > 0 && len(role.Spec.Allow.KubernetesResources) == 0 { - logger.Warningf("role %q has allow.kubernetes_labels set but no allow.kubernetes_resources, this is probably a mistake. Teleport will restrict access to pods.", role.Metadata.Name) + logger.WarnContext(ctx, "role has allow.kubernetes_labels set but no allow.kubernetes_resources, this is probably a mistake - Teleport will restrict access to pods", "role", role.Metadata.Name) } if len(role.Spec.Allow.KubernetesLabels) == 0 && len(role.Spec.Allow.KubernetesResources) > 0 { - logger.Warningf("role %q has allow.kubernetes_resources set but no allow.kubernetes_labels, this is probably a mistake. kubernetes_resources won't be effective.", role.Metadata.Name) + logger.WarnContext(ctx, "role has allow.kubernetes_resources set but no allow.kubernetes_labels, this is probably a mistake - kubernetes_resources won't be effective", "role", role.Metadata.Name) } if len(role.Spec.Deny.KubernetesLabels) > 0 && len(role.Spec.Deny.KubernetesResources) > 0 { - logger.Warningf("role %q has deny.kubernetes_labels set but also has deny.kubernetes_resources set, this is probably a mistake. deny.kubernetes_resources won't be effective.", role.Metadata.Name) + logger.WarnContext(ctx, "role has deny.kubernetes_labels set but also has deny.kubernetes_resources set, this is probably a mistake - deny.kubernetes_resources won't be effective", "role", role.Metadata.Name) } } @@ -574,13 +574,13 @@ func dynamicLabelWarningMessage(r types.Role) string { // warnAboutDynamicLabelsInDenyRule warns about using dynamic/ labels in deny // rules. Only applies to existing roles as adding dynamic/ labels to deny // rules in a new role is not allowed. -func warnAboutDynamicLabelsInDenyRule(logger utils.Logger, r types.Role) { +func warnAboutDynamicLabelsInDenyRule(ctx context.Context, logger *slog.Logger, r types.Role) { if err := services.CheckDynamicLabelsInDenyRules(r); err == nil { return } else if trace.IsBadParameter(err) { - logger.Warningf(dynamicLabelWarningMessage(r)) + logger.WarnContext(ctx, "existing role has labels with the a dynamic prefix in its deny rules, this is not recommended due to the volatility of dynamic labels and is not allowed for new roles", "role", r.GetName()) } else { - logger.WithError(err).Warningf("error checking deny rules labels") + logger.WarnContext(ctx, "error checking deny rules labels", "error", err) } } @@ -2357,7 +2357,7 @@ func (rc *ResourceCommand) getCollection(ctx context.Context, client *authclient if err != nil { return nil, trace.Wrap(err) } - warnAboutDynamicLabelsInDenyRule(rc.config.Log, role) + warnAboutDynamicLabelsInDenyRule(ctx, rc.config.Logger, role) return &roleCollection{roles: []types.Role{role}}, nil case types.KindNamespace: if rc.ref.Name == "" { diff --git a/tool/teleport/common/teleport.go b/tool/teleport/common/teleport.go index 8318fcf68a45f..225e492a6c60c 100644 --- a/tool/teleport/common/teleport.go +++ b/tool/teleport/common/teleport.go @@ -1017,7 +1017,7 @@ func dumpConfigFile(outputURI, contents, comment string) (string, error) { func onSCP(scpFlags *scp.Flags) (err error) { // when 'teleport scp' is executed, it cannot write logs to stderr (because // they're automatically replayed by the scp client) - utils.SwitchLoggingToSyslog() + slog.SetDefault(slog.New(logutils.DiscardHandler{})) if len(scpFlags.Target) == 0 { return trace.BadParameter("teleport scp: missing an argument") } diff --git a/tool/teleport/testenv/test_server.go b/tool/teleport/testenv/test_server.go index 3e034d9fdf0d6..e4c9245c478ce 100644 --- a/tool/teleport/testenv/test_server.go +++ b/tool/teleport/testenv/test_server.go @@ -150,7 +150,7 @@ func MakeTestServer(t *testing.T, opts ...TestServerOptFunc) (process *service.T cfg.Hostname = "server01" cfg.DataDir = t.TempDir() - cfg.Log = utils.NewLoggerForTests() + cfg.Logger = utils.NewSlogLoggerForTests() authAddr := utils.NetAddr{AddrNetwork: "tcp", Addr: NewTCPListener(t, service.ListenerAuth, &cfg.FileDescriptors)} cfg.SetToken(StaticToken) cfg.SetAuthServerAddress(authAddr) diff --git a/tool/tsh/common/tsh_helper_test.go b/tool/tsh/common/tsh_helper_test.go index 85ec86097b3bd..35250d6f54e4b 100644 --- a/tool/tsh/common/tsh_helper_test.go +++ b/tool/tsh/common/tsh_helper_test.go @@ -97,7 +97,7 @@ func (s *suite) setupRootCluster(t *testing.T, options testSuiteOptions) { cfg := servicecfg.MakeDefaultConfig() cfg.CircuitBreakerConfig = breaker.NoopBreakerConfig() - cfg.Log = utils.NewLoggerForTests() + cfg.Logger = utils.NewSlogLoggerForTests() err := config.ApplyFileConfig(fileConfig, cfg) require.NoError(t, err) cfg.FileDescriptors = dynAddr.Descriptors @@ -194,7 +194,7 @@ func (s *suite) setupLeafCluster(t *testing.T, options testSuiteOptions) { cfg := servicecfg.MakeDefaultConfig() cfg.CircuitBreakerConfig = breaker.NoopBreakerConfig() - cfg.Log = utils.NewLoggerForTests() + cfg.Logger = utils.NewSlogLoggerForTests() err := config.ApplyFileConfig(fileConfig, cfg) require.NoError(t, err) cfg.FileDescriptors = dynAddr.Descriptors diff --git a/tool/tsh/common/tsh_test.go b/tool/tsh/common/tsh_test.go index 2ffa313d42cb1..61dda435b127d 100644 --- a/tool/tsh/common/tsh_test.go +++ b/tool/tsh/common/tsh_test.go @@ -3856,7 +3856,7 @@ func makeTestSSHNode(t *testing.T, authAddr *utils.NetAddr, opts ...testServerOp cfg.SSH.Addr = *utils.MustParseAddr("127.0.0.1:0") cfg.SSH.PublicAddrs = []utils.NetAddr{cfg.SSH.Addr} cfg.SSH.DisableCreateHostUser = true - cfg.Log = utils.NewLoggerForTests() + cfg.Logger = utils.NewSlogLoggerForTests() // Disabling debug service for tests so that it doesn't break if the data // directory path is too long. cfg.DebugService.Enabled = false @@ -3905,7 +3905,7 @@ func makeTestServers(t *testing.T, opts ...testServerOptFunc) (auth *service.Tel cfg.Proxy.SSHAddr = utils.NetAddr{AddrNetwork: "tcp", Addr: net.JoinHostPort("127.0.0.1", ports.Pop())} cfg.Proxy.ReverseTunnelListenAddr = utils.NetAddr{AddrNetwork: "tcp", Addr: net.JoinHostPort("127.0.0.1", ports.Pop())} cfg.Proxy.DisableWebInterface = true - cfg.Log = utils.NewLoggerForTests() + cfg.Logger = utils.NewSlogLoggerForTests() // Disabling debug service for tests so that it doesn't break if the data // directory path is too long. cfg.DebugService.Enabled = false From 95ca8b202fa53133c4fc5aae6c5a6bea05c0e0af Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Wed, 8 Jan 2025 18:26:12 -0500 Subject: [PATCH 2/5] Refactor logger initialization Consolidates configuring of global loggers to a single function. This is mainly to facilitate configuring the logger for teleport scp, but will also allow us to remove the copy of logger initialization that currently exists in integrations/lib/logger. --- integrations/lib/logger/logger.go | 21 ++--- lib/config/configuration.go | 81 ++-------------- lib/utils/log/log.go | 128 ++++++++++++++++++++++++++ lib/utils/{ => log}/syslog.go | 2 +- lib/utils/{ => log}/syslog_windows.go | 2 +- tool/teleport/common/teleport.go | 16 +++- 6 files changed, 162 insertions(+), 88 deletions(-) create mode 100644 lib/utils/log/log.go rename lib/utils/{ => log}/syslog.go (98%) rename lib/utils/{ => log}/syslog_windows.go (98%) diff --git a/integrations/lib/logger/logger.go b/integrations/lib/logger/logger.go index a101727ef31e6..7422f03ff906c 100644 --- a/integrations/lib/logger/logger.go +++ b/integrations/lib/logger/logger.go @@ -34,15 +34,6 @@ import ( logutils "github.com/gravitational/teleport/lib/utils/log" ) -// These values are meant to be kept in sync with teleport/lib/config. -// (We avoid importing that package here because integrations must not require CGo) -const ( - // logFileDefaultMode is the preferred permissions mode for log file. - logFileDefaultMode fs.FileMode = 0o644 - // logFileDefaultFlag is the preferred flags set to log file. - logFileDefaultFlag = os.O_WRONLY | os.O_CREATE | os.O_APPEND -) - type Config struct { Output string `toml:"output"` Severity string `toml:"severity"` @@ -108,8 +99,16 @@ func Setup(conf Config) error { } // NewSLogLogger builds a slog.Logger from the logger.Config. -// TODO: this code is adapted from `config.applyLogConfig`, we'll want to deduplicate the logic next time we refactor the logging setup +// TODO(tross): Defer logging initialization to logutils.Initialize and use the +// global slog loggers once integrations has been updated to use slog. func (conf Config) NewSLogLogger() (*slog.Logger, error) { + const ( + // logFileDefaultMode is the preferred permissions mode for log file. + logFileDefaultMode fs.FileMode = 0o644 + // logFileDefaultFlag is the preferred flags set to log file. + logFileDefaultFlag = os.O_WRONLY | os.O_CREATE | os.O_APPEND + ) + var w io.Writer switch conf.Output { case "": @@ -120,7 +119,7 @@ func (conf Config) NewSLogLogger() (*slog.Logger, error) { w = logutils.NewSharedWriter(os.Stdout) case teleport.Syslog: w = os.Stderr - sw, err := utils.NewSyslogWriter() + sw, err := logutils.NewSyslogWriter() if err != nil { slog.Default().ErrorContext(context.Background(), "Failed to switch logging to syslog", "error", err) break diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 546a48700d4fe..dda2ac6859cf4 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -27,7 +27,6 @@ import ( "crypto/x509" "errors" "io" - "io/fs" "log/slog" "maps" "net" @@ -72,13 +71,6 @@ import ( logutils "github.com/gravitational/teleport/lib/utils/log" ) -const ( - // logFileDefaultMode is the preferred permissions mode for log file. - logFileDefaultMode fs.FileMode = 0o644 - // logFileDefaultFlag is the preferred flags set to log file. - logFileDefaultFlag = os.O_WRONLY | os.O_CREATE | os.O_APPEND -) - // CommandLineFlags stores command line flag values, it's a much simplified subset // of Teleport configuration (which is fully expressed via YAML config file) type CommandLineFlags struct { @@ -789,79 +781,22 @@ func applyAuthOrProxyAddress(fc *FileConfig, cfg *servicecfg.Config) error { } func applyLogConfig(loggerConfig Log, cfg *servicecfg.Config) error { - // TODO: this code is copied in the access plugin logging setup `logger.Config.NewSLogLogger` - // We'll want to deduplicate the logic next time we refactor the logging setup - var w io.Writer switch loggerConfig.Output { - case "": - w = os.Stderr - case "stderr", "error", "2": - w = os.Stderr - cfg.Console = io.Discard // disable console printing - case "stdout", "out", "1": - w = os.Stdout + case "stderr", "error", "2", "stdout", "out", "1": cfg.Console = io.Discard // disable console printing - case teleport.Syslog: - var err error - w, err = utils.NewSyslogWriter() - if err != nil { - slog.ErrorContext(context.Background(), "Failed to switch logging to syslog", "error", err) - break - } - default: - // Assume this is a file path. - sharedWriter, err := logutils.NewFileSharedWriter(loggerConfig.Output, logFileDefaultFlag, logFileDefaultMode) - if err != nil { - return trace.Wrap(err, "failed to init the log file shared writer") - } - w = logutils.NewWriterFinalizer[*logutils.FileSharedWriter](sharedWriter) - if err := sharedWriter.RunWatcherReopen(context.Background()); err != nil { - return trace.Wrap(err) - } - } - - level := new(slog.LevelVar) - switch strings.ToLower(loggerConfig.Severity) { - case "", "info": - level.Set(slog.LevelInfo) - case "err", "error": - level.Set(slog.LevelError) - case teleport.DebugLevel: - level.Set(slog.LevelDebug) - case "warn", "warning": - level.Set(slog.LevelWarn) - case "trace": - level.Set(logutils.TraceLevel) - default: - return trace.BadParameter("unsupported logger severity: %q", loggerConfig.Severity) } - configuredFields, err := logutils.ValidateFields(loggerConfig.Format.ExtraFields) + logger, level, err := logutils.Initialize(logutils.Config{ + Output: loggerConfig.Output, + Severity: loggerConfig.Severity, + Format: loggerConfig.Format.Output, + ExtraFields: loggerConfig.Format.ExtraFields, + EnableColors: utils.IsTerminal(os.Stderr), + }) if err != nil { return trace.Wrap(err) } - var logger *slog.Logger - switch strings.ToLower(loggerConfig.Format.Output) { - case "": - fallthrough // not set. defaults to 'text' - case "text": - logger = slog.New(logutils.NewSlogTextHandler(w, logutils.SlogTextHandlerConfig{ - Level: level, - EnableColors: utils.IsTerminal(os.Stderr), - ConfiguredFields: configuredFields, - })) - slog.SetDefault(logger) - case "json": - logger = slog.New(logutils.NewSlogJSONHandler(w, logutils.SlogJSONHandlerConfig{ - Level: level, - ConfiguredFields: configuredFields, - })) - slog.SetDefault(logger) - default: - return trace.BadParameter("unsupported log output format : %q", loggerConfig.Format.Output) - } - cfg.Logger = logger cfg.LoggerLevel = level return nil diff --git a/lib/utils/log/log.go b/lib/utils/log/log.go new file mode 100644 index 0000000000000..c058e518c80d9 --- /dev/null +++ b/lib/utils/log/log.go @@ -0,0 +1,128 @@ +// Teleport +// Copyright (C) 2025 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package log + +import ( + "context" + "io" + "io/fs" + "log/slog" + "os" + "strings" + + "github.com/gravitational/trace" + + "github.com/gravitational/teleport" +) + +// Config configures teleport logging +type Config struct { + // Output defines where logs go. It can be one of the following: "stderr", "stdout" or + // a path to a log file + Output string `yaml:"output,omitempty"` + // Severity defines how verbose the log will be. Possible values are "error", "info", "warn" + Severity string `yaml:"severity,omitempty"` + // Format defines the output format. Possible values are 'text' and 'json'. + Format string `yaml:"output,omitempty"` + // ExtraFields lists the output fields from KnownFormatFields. Example format: [timestamp, component, caller] + ExtraFields []string `yaml:"extra_fields,omitempty"` + // EnableColors dictates if output should be colored. + EnableColors bool +} + +// Initialize configures the default global logger based on the +// provided configuration. The [slog.Logger] and [slog.LevelVar] +func Initialize(loggerConfig Config) (*slog.Logger, *slog.LevelVar, error) { + const ( + // logFileDefaultMode is the preferred permissions mode for log file. + logFileDefaultMode fs.FileMode = 0o644 + // logFileDefaultFlag is the preferred flags set to log file. + logFileDefaultFlag = os.O_WRONLY | os.O_CREATE | os.O_APPEND + ) + + var w io.Writer + level := new(slog.LevelVar) + switch loggerConfig.Output { + case "": + w = os.Stderr + case "stderr", "error", "2": + w = os.Stderr + case "stdout", "out", "1": + w = os.Stdout + case teleport.Syslog: + var err error + w, err = NewSyslogWriter() + if err != nil { + slog.ErrorContext(context.Background(), "Failed to switch logging to syslog", "error", err) + slog.SetDefault(slog.New(DiscardHandler{})) + return slog.Default(), level, nil + } + default: + // Assume this is a file path. + sharedWriter, err := NewFileSharedWriter(loggerConfig.Output, logFileDefaultFlag, logFileDefaultMode) + if err != nil { + return nil, nil, trace.Wrap(err, "failed to init the log file shared writer") + } + w = NewWriterFinalizer(sharedWriter) + if err := sharedWriter.RunWatcherReopen(context.Background()); err != nil { + return nil, nil, trace.Wrap(err) + } + } + + switch strings.ToLower(loggerConfig.Severity) { + case "", "info": + level.Set(slog.LevelInfo) + case "err", "error": + level.Set(slog.LevelError) + case teleport.DebugLevel: + level.Set(slog.LevelDebug) + case "warn", "warning": + level.Set(slog.LevelWarn) + case "trace": + level.Set(TraceLevel) + default: + return nil, nil, trace.BadParameter("unsupported logger severity: %q", loggerConfig.Severity) + } + + configuredFields, err := ValidateFields(loggerConfig.ExtraFields) + if err != nil { + return nil, nil, trace.Wrap(err) + } + + var logger *slog.Logger + switch strings.ToLower(loggerConfig.Format) { + case "": + fallthrough // not set. defaults to 'text' + case "text": + logger = slog.New(NewSlogTextHandler(w, SlogTextHandlerConfig{ + Level: level, + EnableColors: loggerConfig.EnableColors, + ConfiguredFields: configuredFields, + })) + slog.SetDefault(logger) + case "json": + logger = slog.New(NewSlogJSONHandler(w, SlogJSONHandlerConfig{ + Level: level, + ConfiguredFields: configuredFields, + })) + slog.SetDefault(logger) + default: + return nil, nil, trace.BadParameter("unsupported log output format : %q", loggerConfig.Format) + } + + return logger, level, nil +} diff --git a/lib/utils/syslog.go b/lib/utils/log/syslog.go similarity index 98% rename from lib/utils/syslog.go rename to lib/utils/log/syslog.go index d028563349cfe..40896ebedc767 100644 --- a/lib/utils/syslog.go +++ b/lib/utils/log/syslog.go @@ -19,7 +19,7 @@ * along with this program. If not, see . */ -package utils +package log import ( "io" diff --git a/lib/utils/syslog_windows.go b/lib/utils/log/syslog_windows.go similarity index 98% rename from lib/utils/syslog_windows.go rename to lib/utils/log/syslog_windows.go index 27f4d45e3762f..3d359bdc1d437 100644 --- a/lib/utils/syslog_windows.go +++ b/lib/utils/log/syslog_windows.go @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -package utils +package log import ( "io" diff --git a/tool/teleport/common/teleport.go b/tool/teleport/common/teleport.go index 225e492a6c60c..15e96fc949346 100644 --- a/tool/teleport/common/teleport.go +++ b/tool/teleport/common/teleport.go @@ -1014,10 +1014,22 @@ func dumpConfigFile(outputURI, contents, comment string) (string, error) { // user's privileges // // This is the entry point of "teleport scp" call (the parent process is the teleport daemon) -func onSCP(scpFlags *scp.Flags) (err error) { +func onSCP(scpFlags *scp.Flags) error { // when 'teleport scp' is executed, it cannot write logs to stderr (because // they're automatically replayed by the scp client) - slog.SetDefault(slog.New(logutils.DiscardHandler{})) + var verbosity string + if scpFlags.Verbose { + verbosity = teleport.DebugLevel + } + _, _, err := logutils.Initialize(logutils.Config{ + Output: teleport.Syslog, + Severity: verbosity, + }) + if err != nil { + // If something went wrong, discard all logs and continue command execution. + slog.SetDefault(slog.New(logutils.DiscardHandler{})) + } + if len(scpFlags.Target) == 0 { return trace.BadParameter("teleport scp: missing an argument") } From 839b31bdaaa4163189b8f4914949dea2dafe828a Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Wed, 8 Jan 2025 18:29:13 -0500 Subject: [PATCH 3/5] fix: document ValidateFields --- lib/utils/log/slog.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/utils/log/slog.go b/lib/utils/log/slog.go index 9b074ff56abad..46f0e13627b3e 100644 --- a/lib/utils/log/slog.go +++ b/lib/utils/log/slog.go @@ -128,6 +128,8 @@ var knownFormatFields = map[string]struct{}{ TimestampField: {}, } +// ValidateFields ensures the provided fields map to the allowed fields. An error +// is returned if any of the fields are invalid. func ValidateFields(formatInput []string) (result []string, err error) { for _, component := range formatInput { component = strings.TrimSpace(component) From d7f25f08f3d2135a94fa56fbe961d682c8fdc2d0 Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Thu, 9 Jan 2025 11:10:15 -0500 Subject: [PATCH 4/5] fix: remove copied yaml tags --- lib/utils/log/log.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/utils/log/log.go b/lib/utils/log/log.go index c058e518c80d9..8e3a12c80c3f0 100644 --- a/lib/utils/log/log.go +++ b/lib/utils/log/log.go @@ -33,13 +33,13 @@ import ( type Config struct { // Output defines where logs go. It can be one of the following: "stderr", "stdout" or // a path to a log file - Output string `yaml:"output,omitempty"` + Output string // Severity defines how verbose the log will be. Possible values are "error", "info", "warn" - Severity string `yaml:"severity,omitempty"` + Severity string // Format defines the output format. Possible values are 'text' and 'json'. - Format string `yaml:"output,omitempty"` + Format string // ExtraFields lists the output fields from KnownFormatFields. Example format: [timestamp, component, caller] - ExtraFields []string `yaml:"extra_fields,omitempty"` + ExtraFields []string // EnableColors dictates if output should be colored. EnableColors bool } From e71186ea7e95e6bf7342727ae0415c4e90be2cb1 Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Thu, 9 Jan 2025 11:10:57 -0500 Subject: [PATCH 5/5] fix: update file path comment --- lib/utils/log/log.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/utils/log/log.go b/lib/utils/log/log.go index 8e3a12c80c3f0..2f16b902e3df6 100644 --- a/lib/utils/log/log.go +++ b/lib/utils/log/log.go @@ -72,7 +72,7 @@ func Initialize(loggerConfig Config) (*slog.Logger, *slog.LevelVar, error) { return slog.Default(), level, nil } default: - // Assume this is a file path. + // Assume a file path for all other provided output values. sharedWriter, err := NewFileSharedWriter(loggerConfig.Output, logFileDefaultFlag, logFileDefaultMode) if err != nil { return nil, nil, trace.Wrap(err, "failed to init the log file shared writer")