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

Remove logrus #50829

Merged
merged 5 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 10 additions & 11 deletions integrations/lib/logger/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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 "":
Expand All @@ -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
Expand Down
81 changes: 8 additions & 73 deletions lib/config/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"crypto/x509"
"errors"
"io"
"io/fs"
"log/slog"
"maps"
"net"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time understanding what's going on here. Why do we disable console printing when the user wants to print to the console? I know it wasn't commented in the original code either, but I still think it warrants a word or two.

The documentation comments of Config.Console and Config.Logger don't help too much, either, as they both sound like serving roughly the same purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at Console more closely, I don't actually see it being used anywhere. For now I'm going to leave this as is and in a follow up PR I will remove it entirely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keep the cases for "" and teleport.Syslog in the meantime? Is the case for teleport.Syslog obsolete too?

Copy link
Contributor Author

@rosstimothy rosstimothy Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"" and teleport.Syslog are handled by log.Initialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Expand Down
128 changes: 128 additions & 0 deletions lib/utils/log/log.go
Original file line number Diff line number Diff line change
@@ -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 <http://www.gnu.org/licenses/>.

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
}
2 changes: 1 addition & 1 deletion lib/utils/syslog.go → lib/utils/log/syslog.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package utils
package log

import (
"io"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package utils
package log

import (
"io"
Expand Down
16 changes: 14 additions & 2 deletions tool/teleport/common/teleport.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down