Skip to content

Commit

Permalink
Remove all direct logrus usage from teleport module
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rosstimothy committed Jan 8, 2025
1 parent e1dfb94 commit 0a31c37
Show file tree
Hide file tree
Showing 22 changed files with 180 additions and 260 deletions.
29 changes: 19 additions & 10 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 exeception 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.
Expand Down
2 changes: 1 addition & 1 deletion e
Submodule e updated from 1bc4a6 to ca8a1b
9 changes: 0 additions & 9 deletions integration/helpers/instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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.
Expand All @@ -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{})
}
Expand Down
4 changes: 2 additions & 2 deletions integration/hostuser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))
Expand Down
20 changes: 18 additions & 2 deletions lib/client/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"slices"
"strconv"
"strings"
"sync"
"sync/atomic"
"time"
"unicode/utf8"
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 0 additions & 5 deletions lib/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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),
Expand Down
1 change: 0 additions & 1 deletion lib/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
15 changes: 0 additions & 15 deletions lib/service/servicecfg/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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)
}

Expand Down
23 changes: 6 additions & 17 deletions lib/service/servicecfg/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
})
}
}
Expand Down
8 changes: 0 additions & 8 deletions lib/utils/log/handle_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"time"

"github.com/gravitational/trace"
"github.com/sirupsen/logrus"

"github.com/gravitational/teleport"
)
Expand Down Expand Up @@ -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
}
}

Expand Down
54 changes: 0 additions & 54 deletions lib/utils/log/levels.go

This file was deleted.

Loading

0 comments on commit 0a31c37

Please sign in to comment.