Skip to content
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

Convert lib/service to use slog #50578

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions lib/service/certreloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ import (
"context"
"crypto/tls"
"crypto/x509"
"log/slog"
"sync"
"time"

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

"github.com/gravitational/teleport"
"github.com/gravitational/teleport/lib/service/servicecfg"
Expand All @@ -44,7 +44,7 @@ type CertReloaderConfig struct {
// CertReloader periodically reloads a list of cert key-pair paths.
// This allows new certificates to be used without a full reload of Teleport.
type CertReloader struct {
*log.Entry
logger *slog.Logger
// cfg is the certificate reloader configuration.
cfg CertReloaderConfig

Expand All @@ -57,17 +57,15 @@ type CertReloader struct {
// NewCertReloader initializes a new certificate reloader.
func NewCertReloader(cfg CertReloaderConfig) *CertReloader {
return &CertReloader{
Entry: log.WithFields(log.Fields{
teleport.ComponentKey: teleport.Component(teleport.ComponentProxy, "certreloader"),
}),
cfg: cfg,
logger: slog.With(teleport.ComponentKey, teleport.Component(teleport.ComponentProxy, "certreloader")),
cfg: cfg,
}
}

// Run tries to load certificates and then spawns the certificate reloader.
func (c *CertReloader) Run(ctx context.Context) error {
// Synchronously load initially configured certificates.
if err := c.loadCertificates(); err != nil {
if err := c.loadCertificates(ctx); err != nil {
return trace.Wrap(err)
}

Expand All @@ -78,18 +76,18 @@ func (c *CertReloader) Run(ctx context.Context) error {

// Spawn the certificate reloader.
go func() {
c.Infof("Starting periodic reloading of certificate key pairs every %s.", c.cfg.KeyPairsReloadInterval)
c.logger.InfoContext(ctx, "Starting periodic reloading of certificate key pairs", "reload_interval", c.cfg.KeyPairsReloadInterval)
defer func() {
c.Info("Stopped periodic reloading of certificate key pairs.")
c.logger.InfoContext(ctx, "Stopped periodic reloading of certificate key pairs")
}()

t := time.NewTicker(c.cfg.KeyPairsReloadInterval)
defer t.Stop()
for {
select {
case <-t.C:
if err := c.loadCertificates(); err != nil {
c.WithError(err).Warn("Failed to load certificates")
if err := c.loadCertificates(ctx); err != nil {
c.logger.WarnContext(ctx, "Failed to load certificates", "error", err)
}
case <-ctx.Done():
return
Expand All @@ -102,10 +100,13 @@ func (c *CertReloader) Run(ctx context.Context) error {
// loadCertificates loads certificate keys pairs.
// It returns an error if any of the certificate key pairs fails to load.
// If any of the key pairs fails to load, none of the certificates are updated.
func (c *CertReloader) loadCertificates() error {
func (c *CertReloader) loadCertificates(ctx context.Context) error {
certs := make([]tls.Certificate, 0, len(c.cfg.KeyPairs))
for _, pair := range c.cfg.KeyPairs {
c.Debugf("Loading TLS certificate %v and key %v.", pair.Certificate, pair.PrivateKey)
c.logger.DebugContext(ctx, "Loading TLS certificate",
"public_key", pair.Certificate,
"private_key", pair.PrivateKey,
)

certificate, err := tls.LoadX509KeyPair(pair.Certificate, pair.PrivateKey)
if err != nil {
Expand Down
8 changes: 2 additions & 6 deletions lib/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -631,9 +631,6 @@ type TeleportProcess struct {
// during in-process reloads.
id string

// log is a process-local logrus.Entry.
// Deprecated: use logger instead.
log logrus.FieldLogger
// logger is a process-local slog.Logger.
logger *slog.Logger

Expand Down Expand Up @@ -1031,13 +1028,13 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) {
}

if len(cfg.FileDescriptors) == 0 {
cfg.FileDescriptors, err = importFileDescriptors(cfg.Log)
cfg.FileDescriptors, err = importFileDescriptors(cfg.Logger)
if err != nil {
return nil, trace.Wrap(err)
}
}

supervisor := NewSupervisor(processID, cfg.Log)
supervisor := NewSupervisor(processID, cfg.Logger)
storage, err := storage.NewProcessStorage(supervisor.ExitContext(), filepath.Join(cfg.DataDir, teleport.ComponentProcess))
if err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -1183,7 +1180,6 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) {
storage: storage,
rotationCache: rotationCache,
id: processID,
log: cfg.Log,
logger: cfg.Logger,
cloudLabels: cloudLabels,
TracingProvider: tracing.NoopProvider(),
Expand Down
6 changes: 2 additions & 4 deletions lib/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,6 @@ func TestAthenaAuditLogSetup(t *testing.T) {
exitContext: context.Background(),
},
backend: backend,
log: utils.NewLoggerForTests(),
logger: utils.NewSlogLoggerForTests(),
}

Expand Down Expand Up @@ -920,7 +919,7 @@ func TestSetupProxyTLSConfig(t *testing.T) {
process := TeleportProcess{
Config: cfg,
// Setting Supervisor so that `ExitContext` can be called.
Supervisor: NewSupervisor("process-id", cfg.Log),
Supervisor: NewSupervisor("process-id", cfg.Logger),
}
tls, err := process.setupProxyTLSConfig(
&Connector{},
Expand Down Expand Up @@ -1291,15 +1290,14 @@ func TestProxyGRPCServers(t *testing.T) {
// Create a new Teleport process to initialize the gRPC servers with KubeProxy
// enabled.
process := &TeleportProcess{
Supervisor: NewSupervisor(hostID, utils.NewLoggerForTests()),
Supervisor: NewSupervisor(hostID, utils.NewSlogLoggerForTests()),
Config: &servicecfg.Config{
Proxy: servicecfg.ProxyConfig{
Kube: servicecfg.KubeProxyConfig{
Enabled: true,
},
},
},
log: utils.NewLoggerForTests(),
logger: utils.NewSlogLoggerForTests(),
}

Expand Down
6 changes: 3 additions & 3 deletions lib/service/signals.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"encoding/json"
"fmt"
"io"
"log/slog"
"net"
"os"
"os/exec"
Expand All @@ -31,7 +32,6 @@ import (
"time"

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

apidefaults "github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/lib/defaults"
Expand Down Expand Up @@ -414,7 +414,7 @@ func (process *TeleportProcess) ExportFileDescriptors() ([]*servicecfg.FileDescr
}

// importFileDescriptors imports file descriptors from environment if there are any
func importFileDescriptors(log logrus.FieldLogger) ([]*servicecfg.FileDescriptor, error) {
func importFileDescriptors(log *slog.Logger) ([]*servicecfg.FileDescriptor, error) {
// These files may be passed in by the parent process
filesString := os.Getenv(teleportFilesEnvVar)
os.Unsetenv(teleportFilesEnvVar)
Expand All @@ -428,7 +428,7 @@ func importFileDescriptors(log logrus.FieldLogger) ([]*servicecfg.FileDescriptor
}

if len(files) != 0 {
log.Infof("Child has been passed files: %v", files)
log.InfoContext(context.Background(), "Child has been passed files", "file_descriptors", files)
}

return files, nil
Expand Down
37 changes: 19 additions & 18 deletions lib/service/supervisor.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@ import (
"context"
"errors"
"fmt"
"log/slog"
"sync"
"time"

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

"github.com/gravitational/teleport"
"github.com/gravitational/teleport/lib/observability/metrics"
logutils "github.com/gravitational/teleport/lib/utils/log"
)

// Supervisor implements the simple service logic - registering
Expand Down Expand Up @@ -165,11 +166,11 @@ type LocalSupervisor struct {
id string

// log specifies the logger
log logrus.FieldLogger
log *slog.Logger
}

// NewSupervisor returns new instance of initialized supervisor
func NewSupervisor(id string, parentLog logrus.FieldLogger) Supervisor {
func NewSupervisor(id string, parentLog *slog.Logger) Supervisor {
ctx := context.TODO()

closeContext, cancel := context.WithCancel(ctx)
Expand All @@ -196,7 +197,7 @@ func NewSupervisor(id string, parentLog logrus.FieldLogger) Supervisor {
gracefulExitContext: gracefulExitContext,
signalGracefulExit: signalGracefulExit,

log: parentLog.WithField(teleport.ComponentKey, teleport.Component(teleport.ComponentProcess, id)),
log: parentLog.With(teleport.ComponentKey, teleport.Component(teleport.ComponentProcess, id)),
}
go srv.fanOut()
return srv
Expand All @@ -214,7 +215,7 @@ func (e *Event) String() string {
}

func (s *LocalSupervisor) Register(srv Service) {
s.log.WithField("service", srv.Name()).Debug("Adding service to supervisor.")
s.log.DebugContext(s.closeContext, "Adding service to supervisor", "service", srv.Name())
s.Lock()
defer s.Unlock()
s.services = append(s.services, srv)
Expand Down Expand Up @@ -246,17 +247,17 @@ func (s *LocalSupervisor) RegisterCriticalFunc(name string, fn Func) {

// RemoveService removes service from supervisor tracking list
func (s *LocalSupervisor) RemoveService(srv Service) error {
l := s.log.WithField("service", srv.Name())
l := s.log.With("service", srv.Name())
s.Lock()
defer s.Unlock()
for i, el := range s.services {
if el == srv {
s.services = append(s.services[:i], s.services[i+1:]...)
l.Debug("Service is completed and removed.")
l.DebugContext(s.closeContext, "Service is completed and removed")
return nil
}
}
l.Warning("Service is completed but not found.")
l.WarnContext(s.closeContext, "Service is completed but not found")
return trace.NotFound("service %v is not found", srv)
}

Expand Down Expand Up @@ -302,15 +303,15 @@ func (s *LocalSupervisor) serve(srv Service) {
defer metricsServicesRunning.WithLabelValues(label).Dec()
}

l := s.log.WithField("service", srv.Name())
l.Debug("Service has started.")
l := s.log.With("service", srv.Name())
l.DebugContext(s.closeContext, "Service has started")
err := srv.Serve()
if err != nil {
if errors.Is(err, ErrTeleportExited) {
l.Info("Teleport process has shut down.")
} else {
if s.ExitContext().Err() == nil {
l.WithError(err).Warning("Teleport process has exited with error.")
l.WarnContext(s.closeContext, "Teleport process has exited with error", "error", err)
}
s.BroadcastEvent(Event{
Name: ServiceExitedWithErrorEvent,
Expand All @@ -327,7 +328,7 @@ func (s *LocalSupervisor) Start() error {
s.state = stateStarted

if len(s.services) == 0 {
s.log.Warning("Supervisor has no services to run. Exiting.")
s.log.WarnContext(s.closeContext, "Supervisor has no services to run - exiting")
return nil
}

Expand Down Expand Up @@ -408,7 +409,7 @@ func (s *LocalSupervisor) BroadcastEvent(event Event) {
// Log all events other than recovered events to prevent the logs from
// being flooded.
if event.String() != TeleportOKEvent {
s.log.WithField("event", event.String()).Debug("Broadcasting event.")
s.log.DebugContext(s.closeContext, "Broadcasting event", "event", logutils.StringerAttr(&event))
}

go func() {
Expand All @@ -430,12 +431,12 @@ func (s *LocalSupervisor) BroadcastEvent(event Event) {
return
}
}(mappedEvent)
s.log.WithFields(logrus.Fields{
"in": event.String(),
"out": m.String(),
}).Debug("Broadcasting mapped event.")
s.log.DebugContext(s.closeContext, "Broadcasting mapped event",
"in", logutils.StringerAttr(&event),
"out", logutils.StringerAttr(m),
)
} else if err != nil {
s.log.Debugf("Teleport not yet ready: %v", err)
s.log.DebugContext(s.closeContext, "Teleport not yet ready", "error", err)
}
}
}
Expand Down
Loading