From 919ba2d847c9d263578e78eac5a0441c4fa01fdf Mon Sep 17 00:00:00 2001 From: Paul Abel <128620221+pdabelf5@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:27:56 +0100 Subject: [PATCH] refactor: replace glog in flags.go (#6530) --- cmd/nginx-ingress/flags.go | 128 +++++++++++++++++---------- cmd/nginx-ingress/main.go | 15 ++-- cmd/nginx-ingress/main_test.go | 6 +- internal/logger/glog/handler.go | 27 ++---- internal/logger/glog/handler_test.go | 16 ++-- internal/logger/levels/levels.go | 18 ++++ 6 files changed, 127 insertions(+), 83 deletions(-) create mode 100644 internal/logger/levels/levels.go diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 4f75da672d..afb0cd6875 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -1,6 +1,7 @@ package main import ( + "context" "flag" "fmt" "net" @@ -8,10 +9,12 @@ import ( "regexp" "strings" - "github.com/golang/glog" api_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/validation" + + nlog "github.com/nginxinc/kubernetes-ingress/internal/logger" + "github.com/nginxinc/kubernetes-ingress/internal/logger/levels" ) const ( @@ -229,86 +232,95 @@ func parseFlags() { } } -func initValidate() { +func initValidate(ctx context.Context) { + l := nlog.LoggerFromContext(ctx) logFormatValidationError := validateLogFormat(*logFormat) if logFormatValidationError != nil { - glog.Warningf("Invalid log format: %s. Valid options are: glog, text, json. Falling back to default: %s", *logFormat, logFormatDefault) + l.Warn(fmt.Sprintf("Invalid log format: %s. Valid options are: glog, text, json. Falling back to default: %s", *logFormat, logFormatDefault)) } logLevelValidationError := validateLogLevel(*logLevel) if logLevelValidationError != nil { - glog.Warningf("Invalid log level: %s. Valid options are: trace, debug, info, warning, error, fatal. Falling back to default: %s", *logLevel, logLevelDefault) + l.Warn(fmt.Sprintf("Invalid log level: %s. Valid options are: trace, debug, info, warning, error, fatal. Falling back to default: %s", *logLevel, logLevelDefault)) } if *enableLatencyMetrics && !*enablePrometheusMetrics { - glog.Warning("enable-latency-metrics flag requires enable-prometheus-metrics, latency metrics will not be collected") + l.Warn("enable-latency-metrics flag requires enable-prometheus-metrics, latency metrics will not be collected") *enableLatencyMetrics = false } if *enableServiceInsight && !*nginxPlus { - glog.Warning("enable-service-insight flag support is for NGINX Plus, service insight endpoint will not be exposed") + l.Warn("enable-service-insight flag support is for NGINX Plus, service insight endpoint will not be exposed") *enableServiceInsight = false } if *enableDynamicWeightChangesReload && !*nginxPlus { - glog.Warning("weight-changes-dynamic-reload flag support is for NGINX Plus, Dynamic Weight Changes will not be enabled") + l.Warn("weight-changes-dynamic-reload flag support is for NGINX Plus, Dynamic Weight Changes will not be enabled") *enableDynamicWeightChangesReload = false } - mustValidateInitialChecks() - mustValidateWatchedNamespaces() - mustValidateFlags() + mustValidateInitialChecks(ctx) + mustValidateWatchedNamespaces(ctx) + mustValidateFlags(ctx) } -func mustValidateInitialChecks() { +func mustValidateInitialChecks(ctx context.Context) { + l := nlog.LoggerFromContext(ctx) err := flag.Lookup("logtostderr").Value.Set("true") if err != nil { - glog.Fatalf("Error setting logtostderr to true: %v", err) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Error setting logtostderr to true: %v", err)) + os.Exit(1) } err = flag.Lookup("include_year").Value.Set("true") if err != nil { - glog.Fatalf("Error setting include_year flag: %v", err) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Error setting include_year flag: %v", err)) + os.Exit(1) } if startupCheckFn != nil { err := startupCheckFn() if err != nil { - glog.Fatalf("Failed startup check: %v", err) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Failed startup check: %v", err)) + os.Exit(1) } - glog.Info("AWS startup check passed") + l.Info("AWS startup check passed") } - glog.Infof("Starting with flags: %+q", os.Args[1:]) + l.Info(fmt.Sprintf("Starting with flags: %+q", os.Args[1:])) unparsed := flag.Args() if len(unparsed) > 0 { - glog.Warningf("Ignoring unhandled arguments: %+q", unparsed) + l.Warn(fmt.Sprintf("Ignoring unhandled arguments: %+q", unparsed)) } } // mustValidateWatchedNamespaces calls internally os.Exit if it can't validate namespaces. -func mustValidateWatchedNamespaces() { +func mustValidateWatchedNamespaces(ctx context.Context) { + l := nlog.LoggerFromContext(ctx) if *watchNamespace != "" && *watchNamespaceLabel != "" { - glog.Fatal("watch-namespace and -watch-namespace-label are mutually exclusive") + l.Log(ctx, levels.LevelFatal, "watch-namespace and -watch-namespace-label are mutually exclusive") + os.Exit(1) } watchNamespaces = strings.Split(*watchNamespace, ",") if *watchNamespace != "" { - glog.Infof("Namespaces watched: %v", watchNamespaces) + l.Info(fmt.Sprintf("Namespaces watched: %v", watchNamespaces)) namespacesNameValidationError := validateNamespaceNames(watchNamespaces) if namespacesNameValidationError != nil { - glog.Fatalf("Invalid values for namespaces: %v", namespacesNameValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid values for namespaces: %v", namespacesNameValidationError)) + os.Exit(1) } } if len(*watchSecretNamespace) > 0 { watchSecretNamespaces = strings.Split(*watchSecretNamespace, ",") - glog.Infof("Namespaces watched for secrets: %v", watchSecretNamespaces) + l.Debug(fmt.Sprintf("Namespaces watched for secrets: %v", watchSecretNamespaces)) namespacesNameValidationError := validateNamespaceNames(watchSecretNamespaces) if namespacesNameValidationError != nil { - glog.Fatalf("Invalid values for secret namespaces: %v", namespacesNameValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid values for secret namespaces: %v", namespacesNameValidationError)) + os.Exit(1) } } else { // empty => default to watched namespaces @@ -319,107 +331,131 @@ func mustValidateWatchedNamespaces() { var err error _, err = labels.Parse(*watchNamespaceLabel) if err != nil { - glog.Fatalf("Unable to parse label %v for watch namespace label: %v", *watchNamespaceLabel, err) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Unable to parse label %v for watch namespace label: %v", *watchNamespaceLabel, err)) + os.Exit(1) } } } // mustValidateFlags checks the values for various flags // and calls os.Exit if any of the flags is invalid. -func mustValidateFlags() { +// nolint:gocyclo +func mustValidateFlags(ctx context.Context) { + l := nlog.LoggerFromContext(ctx) healthStatusURIValidationError := validateLocation(*healthStatusURI) if healthStatusURIValidationError != nil { - glog.Fatalf("Invalid value for health-status-uri: %v", healthStatusURIValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for health-status-uri: %v", healthStatusURIValidationError)) + os.Exit(1) } statusLockNameValidationError := validateResourceName(*leaderElectionLockName) if statusLockNameValidationError != nil { - glog.Fatalf("Invalid value for leader-election-lock-name: %v", statusLockNameValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for leader-election-lock-name: %v", statusLockNameValidationError)) + os.Exit(1) } statusPortValidationError := validatePort(*nginxStatusPort) if statusPortValidationError != nil { - glog.Fatalf("Invalid value for nginx-status-port: %v", statusPortValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for nginx-status-port: %v", statusPortValidationError)) + os.Exit(1) } metricsPortValidationError := validatePort(*prometheusMetricsListenPort) if metricsPortValidationError != nil { - glog.Fatalf("Invalid value for prometheus-metrics-listen-port: %v", metricsPortValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for prometheus-metrics-listen-port: %v", metricsPortValidationError)) + os.Exit(1) } readyStatusPortValidationError := validatePort(*readyStatusPort) if readyStatusPortValidationError != nil { - glog.Fatalf("Invalid value for ready-status-port: %v", readyStatusPortValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for ready-status-port: %v", readyStatusPortValidationError)) + os.Exit(1) } healthProbePortValidationError := validatePort(*serviceInsightListenPort) if healthProbePortValidationError != nil { - glog.Fatalf("Invalid value for service-insight-listen-port: %v", metricsPortValidationError) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for service-insight-listen-port: %v", metricsPortValidationError)) + os.Exit(1) } var err error allowedCIDRs, err = parseNginxStatusAllowCIDRs(*nginxStatusAllowCIDRs) if err != nil { - glog.Fatalf(`Invalid value for nginx-status-allow-cidrs: %v`, err) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for nginx-status-allow-cidrs: %v", err)) + os.Exit(1) } if *appProtectLogLevel != appProtectLogLevelDefault && *appProtect && *nginxPlus { appProtectlogLevelValidationError := validateLogLevel(*appProtectLogLevel) if appProtectlogLevelValidationError != nil { - glog.Fatalf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel) + l.Log(ctx, levels.LevelFatal, fmt.Sprintf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel)) + os.Exit(1) } } if *enableTLSPassthrough && !*enableCustomResources { - glog.Fatal("enable-tls-passthrough flag requires -enable-custom-resources") + l.Log(ctx, levels.LevelFatal, "enable-tls-passthrough flag requires -enable-custom-resources") + os.Exit(1) } if *appProtect && !*nginxPlus { - glog.Fatal("NGINX App Protect support is for NGINX Plus only") + l.Log(ctx, levels.LevelFatal, "NGINX App Protect support is for NGINX Plus only") + os.Exit(1) } if *appProtectLogLevel != appProtectLogLevelDefault && !*appProtect && !*nginxPlus { - glog.Fatal("app-protect-log-level support is for NGINX Plus only and App Protect is enable") + l.Log(ctx, levels.LevelFatal, "app-protect-log-level support is for NGINX Plus only and App Protect is enable") + os.Exit(1) } if *appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos support is for NGINX Plus only") + l.Log(ctx, levels.LevelFatal, "NGINX App Protect Dos support is for NGINX Plus only") + os.Exit(1) } if *appProtectDosDebug && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos debug support is for NGINX Plus only and App Protect Dos is enable") + l.Log(ctx, levels.LevelFatal, "NGINX App Protect Dos debug support is for NGINX Plus only and App Protect Dos is enable") + os.Exit(1) } if *appProtectDosMaxDaemons != 0 && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos max daemons support is for NGINX Plus only and App Protect Dos is enable") + l.Log(ctx, levels.LevelFatal, "NGINX App Protect Dos max daemons support is for NGINX Plus only and App Protect Dos is enable") + os.Exit(1) } if *appProtectDosMaxWorkers != 0 && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos max workers support is for NGINX Plus and App Protect Dos is enable") + l.Log(ctx, levels.LevelFatal, "NGINX App Protect Dos max workers support is for NGINX Plus and App Protect Dos is enable") + os.Exit(1) } if *appProtectDosMemory != 0 && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos memory support is for NGINX Plus and App Protect Dos is enable") + l.Log(ctx, levels.LevelFatal, "NGINX App Protect Dos memory support is for NGINX Plus and App Protect Dos is enable") + os.Exit(1) } if *enableInternalRoutes && *spireAgentAddress == "" { - glog.Fatal("enable-internal-routes flag requires spire-agent-address") + l.Log(ctx, levels.LevelFatal, "enable-internal-routes flag requires spire-agent-address") + os.Exit(1) } if *enableCertManager && !*enableCustomResources { - glog.Fatal("enable-cert-manager flag requires -enable-custom-resources") + l.Log(ctx, levels.LevelFatal, "enable-cert-manager flag requires -enable-custom-resources") + os.Exit(1) } if *enableExternalDNS && !*enableCustomResources { - glog.Fatal("enable-external-dns flag requires -enable-custom-resources") + l.Log(ctx, levels.LevelFatal, "enable-external-dns flag requires -enable-custom-resources") + os.Exit(1) } if *ingressLink != "" && *externalService != "" { - glog.Fatal("ingresslink and external-service cannot both be set") + l.Log(ctx, levels.LevelFatal, "ingresslink and external-service cannot both be set") + os.Exit(1) } if *agent && !*appProtect { - glog.Fatal("NGINX Agent is used to enable the Security Monitoring dashboard and requires NGINX App Protect to be enabled") + l.Log(ctx, levels.LevelFatal, "NGINX Agent is used to enable the Security Monitoring dashboard and requires NGINX App Protect to be enabled") + os.Exit(1) } } diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 569e2e550e..5e561554dd 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -46,6 +46,7 @@ import ( nic_logger "github.com/nginxinc/kubernetes-ingress/internal/logger" nic_glog "github.com/nginxinc/kubernetes-ingress/internal/logger/glog" + "github.com/nginxinc/kubernetes-ingress/internal/logger/levels" ) // Injected during build @@ -53,12 +54,12 @@ var ( version string telemetryEndpoint string logLevels = map[string]slog.Level{ - "trace": nic_glog.LevelTrace, - "debug": nic_glog.LevelDebug, - "info": nic_glog.LevelInfo, - "warning": nic_glog.LevelWarning, - "error": nic_glog.LevelError, - "fatal": nic_glog.LevelFatal, + "trace": levels.LevelTrace, + "debug": levels.LevelDebug, + "info": levels.LevelInfo, + "warning": levels.LevelWarning, + "error": levels.LevelError, + "fatal": levels.LevelFatal, } ) @@ -79,7 +80,7 @@ func main() { ctx := initLogger(*logFormat, logLevels[*logLevel], os.Stdout) _ = nic_logger.LoggerFromContext(ctx) - initValidate() + initValidate(ctx) parsedFlags := os.Args[1:] buildOS := os.Getenv("BUILD_OS") diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 7ba896c028..f223cbcb67 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -6,7 +6,7 @@ import ( "testing" nic_logger "github.com/nginxinc/kubernetes-ingress/internal/logger" - nic_glog "github.com/nginxinc/kubernetes-ingress/internal/logger/glog" + "github.com/nginxinc/kubernetes-ingress/internal/logger/levels" ) func TestLogFormats(t *testing.T) { @@ -35,9 +35,9 @@ func TestLogFormats(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { var buf bytes.Buffer - ctx := initLogger(tc.format, nic_glog.LevelInfo, &buf) + ctx := initLogger(tc.format, levels.LevelInfo, &buf) l := nic_logger.LoggerFromContext(ctx) - l.Log(ctx, nic_glog.LevelInfo, "test") + l.Log(ctx, levels.LevelInfo, "test") got := buf.String() re := regexp.MustCompile(tc.wantre) if !re.MatchString(got) { diff --git a/internal/logger/glog/handler.go b/internal/logger/glog/handler.go index 335310e403..d5fa0e0459 100644 --- a/internal/logger/glog/handler.go +++ b/internal/logger/glog/handler.go @@ -11,21 +11,8 @@ import ( "strconv" "strings" "sync" -) -const ( - // LevelTrace - Trace Level Logging same as glog.V(3) - LevelTrace = slog.Level(-8) - // LevelDebug - Debug Level Logging same as glog.V(2) - LevelDebug = slog.LevelDebug - // LevelInfo - Info Level Logging same as glog.Info() - LevelInfo = slog.LevelInfo - // LevelWarning - Warn Level Logging same as glog.Warning() - LevelWarning = slog.LevelWarn - // LevelError - Error Level Logging same as glog.Error() - LevelError = slog.LevelError - // LevelFatal - Fatal Level Logging same as glog.Fatal() - LevelFatal = slog.Level(12) + "github.com/nginxinc/kubernetes-ingress/internal/logger/levels" ) // Handler holds all the parameters for the handler @@ -80,17 +67,17 @@ func (h *Handler) Handle(_ context.Context, r slog.Record) error { buf := make([]byte, 0, 1024) // LogLevel switch r.Level { - case LevelTrace: + case levels.LevelTrace: buf = append(buf, "I"...) - case LevelDebug: + case levels.LevelDebug: buf = append(buf, "I"...) - case LevelInfo: + case levels.LevelInfo: buf = append(buf, "I"...) - case LevelWarning: + case levels.LevelWarning: buf = append(buf, "W"...) - case LevelError: + case levels.LevelError: buf = append(buf, "E"...) - case LevelFatal: + case levels.LevelFatal: buf = append(buf, "F"...) } diff --git a/internal/logger/glog/handler_test.go b/internal/logger/glog/handler_test.go index ef99f00092..08bf50e19a 100644 --- a/internal/logger/glog/handler_test.go +++ b/internal/logger/glog/handler_test.go @@ -6,6 +6,8 @@ import ( "log/slog" "regexp" "testing" + + "github.com/nginxinc/kubernetes-ingress/internal/logger/levels" ) func TestGlogFormat(t *testing.T) { @@ -28,32 +30,32 @@ func TestGlogLogLevels(t *testing.T) { }{ { name: "Trace level log message", - level: LevelTrace, + level: levels.LevelTrace, wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, { name: "Debug level log message", - level: LevelDebug, + level: levels.LevelDebug, wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, { name: "Info level log message", - level: LevelInfo, + level: levels.LevelInfo, wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, { name: "Warning level log message", - level: LevelWarning, + level: levels.LevelWarning, wantre: `^W\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, { name: "Error level log message", - level: LevelError, + level: levels.LevelError, wantre: `^E\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, { name: "Fatal level log message", - level: LevelFatal, + level: levels.LevelFatal, wantre: `^F\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, } @@ -84,7 +86,7 @@ func TestGlogDefaultLevel(t *testing.T) { func TestGlogHigherLevel(t *testing.T) { var buf bytes.Buffer - l := slog.New(New(&buf, &Options{Level: LevelError})) + l := slog.New(New(&buf, &Options{Level: levels.LevelError})) l.Info("test") if got := buf.Len(); got != 0 { diff --git a/internal/logger/levels/levels.go b/internal/logger/levels/levels.go new file mode 100644 index 0000000000..ec0f9ee086 --- /dev/null +++ b/internal/logger/levels/levels.go @@ -0,0 +1,18 @@ +package levels + +import "log/slog" + +const ( + // LevelTrace - Trace Level Logging same as glog.V(3) + LevelTrace = slog.Level(-8) + // LevelDebug - Debug Level Logging same as glog.V(2) + LevelDebug = slog.LevelDebug + // LevelInfo - Info Level Logging same as glog.Info() + LevelInfo = slog.LevelInfo + // LevelWarning - Warn Level Logging same as glog.Warning() + LevelWarning = slog.LevelWarn + // LevelError - Error Level Logging same as glog.Error() + LevelError = slog.LevelError + // LevelFatal - Fatal Level Logging same as glog.Fatal() + LevelFatal = slog.Level(12) +)