From 3c0aea990751c2925aaebf188b56ceef14d9454a Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Wed, 25 Sep 2024 11:01:03 +0100 Subject: [PATCH 01/14] refactor to prepare for struct logs --- cmd/nginx-ingress/flags.go | 136 +++++++++++++++++++------------------ cmd/nginx-ingress/main.go | 1 + 2 files changed, 70 insertions(+), 67 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 587eca2cd6..cb59f064cc 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -220,77 +220,12 @@ func parseFlags() { if *versionFlag { os.Exit(0) } +} +func initValidation() { mustValidateInitialChecks() mustValidateWatchedNamespaces() mustValidateFlags() - - if *enableTLSPassthrough && !*enableCustomResources { - glog.Fatal("enable-tls-passthrough flag requires -enable-custom-resources") - } - - if *appProtect && !*nginxPlus { - glog.Fatal("NGINX App Protect support is for NGINX Plus only") - } - - if *appProtectLogLevel != appProtectLogLevelDefault && !*appProtect && !*nginxPlus { - glog.Fatal("app-protect-log-level support is for NGINX Plus only and App Protect is enable") - } - - if *appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos support is for NGINX Plus only") - } - - if *appProtectDosDebug && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos debug support is for NGINX Plus only and App Protect Dos is enable") - } - - 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") - } - - if *appProtectDosMaxWorkers != 0 && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos max workers support is for NGINX Plus and App Protect Dos is enable") - } - - if *appProtectDosMemory != 0 && !*appProtectDos && !*nginxPlus { - glog.Fatal("NGINX App Protect Dos memory support is for NGINX Plus and App Protect Dos is enable") - } - - if *enableInternalRoutes && *spireAgentAddress == "" { - glog.Fatal("enable-internal-routes flag requires spire-agent-address") - } - - if *enableLatencyMetrics && !*enablePrometheusMetrics { - glog.Warning("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") - *enableServiceInsight = false - } - - if *enableCertManager && !*enableCustomResources { - glog.Fatal("enable-cert-manager flag requires -enable-custom-resources") - } - - if *enableExternalDNS && !*enableCustomResources { - glog.Fatal("enable-external-dns flag requires -enable-custom-resources") - } - - if *ingressLink != "" && *externalService != "" { - glog.Fatal("ingresslink and external-service cannot both be set") - } - - if *enableDynamicWeightChangesReload && !*nginxPlus { - glog.Warning("weight-changes-dynamic-reload flag support is for NGINX Plus, Dynamic Weight Changes will not be enabled") - *enableDynamicWeightChangesReload = false - } - - if *agent && !*appProtect { - glog.Fatal("NGINX Agent is used to enable the Security Monitoring dashboard and requires NGINX App Protect to be enabled") - } } func mustValidateInitialChecks() { @@ -402,6 +337,73 @@ func mustValidateFlags() { glog.Fatalf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel) } } + + if *enableTLSPassthrough && !*enableCustomResources { + glog.Fatal("enable-tls-passthrough flag requires -enable-custom-resources") + } + + if *appProtect && !*nginxPlus { + glog.Fatal("NGINX App Protect support is for NGINX Plus only") + } + + if *appProtectLogLevel != appProtectLogLevelDefault && !*appProtect && !*nginxPlus { + glog.Fatal("app-protect-log-level support is for NGINX Plus only and App Protect is enable") + } + + if *appProtectDos && !*nginxPlus { + glog.Fatal("NGINX App Protect Dos support is for NGINX Plus only") + } + + if *appProtectDosDebug && !*appProtectDos && !*nginxPlus { + glog.Fatal("NGINX App Protect Dos debug support is for NGINX Plus only and App Protect Dos is enable") + } + + 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") + } + + if *appProtectDosMaxWorkers != 0 && !*appProtectDos && !*nginxPlus { + glog.Fatal("NGINX App Protect Dos max workers support is for NGINX Plus and App Protect Dos is enable") + } + + if *appProtectDosMemory != 0 && !*appProtectDos && !*nginxPlus { + glog.Fatal("NGINX App Protect Dos memory support is for NGINX Plus and App Protect Dos is enable") + } + + if *enableInternalRoutes && *spireAgentAddress == "" { + glog.Fatal("enable-internal-routes flag requires spire-agent-address") + } + + if *enableLatencyMetrics && !*enablePrometheusMetrics { + glog.Warning("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") + *enableServiceInsight = false + } + + if *enableCertManager && !*enableCustomResources { + glog.Fatal("enable-cert-manager flag requires -enable-custom-resources") + } + + if *enableExternalDNS && !*enableCustomResources { + glog.Fatal("enable-external-dns flag requires -enable-custom-resources") + } + + if *ingressLink != "" && *externalService != "" { + glog.Fatal("ingresslink and external-service cannot both be set") + } + + if *enableDynamicWeightChangesReload && !*nginxPlus { + glog.Warning("weight-changes-dynamic-reload flag support is for NGINX Plus, Dynamic Weight Changes will not be enabled") + *enableDynamicWeightChangesReload = false + } + + if *agent && !*appProtect { + glog.Fatal("NGINX Agent is used to enable the Security Monitoring dashboard and requires NGINX App Protect to be enabled") + } } // validateNamespaceNames validates the namespaces are in the correct format diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index ccb6dd77f9..a9e432548e 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -64,6 +64,7 @@ func main() { fmt.Printf("NGINX Ingress Controller Version=%v Commit=%v Date=%v DirtyState=%v Arch=%v/%v Go=%v\n", version, commitHash, commitTime, dirtyBuild, runtime.GOOS, runtime.GOARCH, runtime.Version()) parseFlags() + initValidation() parsedFlags := os.Args[1:] buildOS := os.Getenv("BUILD_OS") From 1a2dd82445774d3a61db8a8f4d4665a1448f0c72 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Wed, 25 Sep 2024 11:56:57 +0100 Subject: [PATCH 02/14] refactor to solve comments --- cmd/nginx-ingress/flags.go | 35 ++++++++++++++++++----------------- cmd/nginx-ingress/main.go | 3 +-- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index cb59f064cc..61e779c28b 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -217,12 +217,28 @@ var ( func parseFlags() { flag.Parse() - if *versionFlag { + if *versionFlag { // printed in main os.Exit(0) } } -func initValidation() { +func initValidate() { + + if *enableLatencyMetrics && !*enablePrometheusMetrics { + glog.Warning("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") + *enableServiceInsight = false + } + + if *enableDynamicWeightChangesReload && !*nginxPlus { + glog.Warning("weight-changes-dynamic-reload flag support is for NGINX Plus, Dynamic Weight Changes will not be enabled") + *enableDynamicWeightChangesReload = false + } + mustValidateInitialChecks() mustValidateWatchedNamespaces() mustValidateFlags() @@ -374,16 +390,6 @@ func mustValidateFlags() { glog.Fatal("enable-internal-routes flag requires spire-agent-address") } - if *enableLatencyMetrics && !*enablePrometheusMetrics { - glog.Warning("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") - *enableServiceInsight = false - } - if *enableCertManager && !*enableCustomResources { glog.Fatal("enable-cert-manager flag requires -enable-custom-resources") } @@ -396,11 +402,6 @@ func mustValidateFlags() { glog.Fatal("ingresslink and external-service cannot both be set") } - if *enableDynamicWeightChangesReload && !*nginxPlus { - glog.Warning("weight-changes-dynamic-reload flag support is for NGINX Plus, Dynamic Weight Changes will not be enabled") - *enableDynamicWeightChangesReload = false - } - if *agent && !*appProtect { glog.Fatal("NGINX Agent is used to enable the Security Monitoring dashboard and requires NGINX App Protect to be enabled") } diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index a9e432548e..fe70d76eb1 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -62,9 +62,8 @@ const ( func main() { commitHash, commitTime, dirtyBuild := getBuildInfo() fmt.Printf("NGINX Ingress Controller Version=%v Commit=%v Date=%v DirtyState=%v Arch=%v/%v Go=%v\n", version, commitHash, commitTime, dirtyBuild, runtime.GOOS, runtime.GOARCH, runtime.Version()) - parseFlags() - initValidation() + initValidate() parsedFlags := os.Args[1:] buildOS := os.Getenv("BUILD_OS") From 402891c6c3fe5f190907aa6e75ec2db16b929bb0 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Wed, 25 Sep 2024 13:36:11 +0100 Subject: [PATCH 03/14] fix lint --- cmd/nginx-ingress/flags.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 61e779c28b..a3b6acf53b 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -223,7 +223,6 @@ func parseFlags() { } func initValidate() { - if *enableLatencyMetrics && !*enablePrometheusMetrics { glog.Warning("enable-latency-metrics flag requires enable-prometheus-metrics, latency metrics will not be collected") *enableLatencyMetrics = false From f05bdb9d1c9014864e5d1876157b51410a8b6be8 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 26 Sep 2024 11:59:44 +0100 Subject: [PATCH 04/14] Add Flags for loglevel and logformat. --- charts/nginx-ingress/templates/_helpers.tpl | 3 +- charts/nginx-ingress/values.schema.json | 20 ++++++----- charts/nginx-ingress/values.yaml | 5 ++- cmd/nginx-ingress/flags.go | 17 ++++++++++ cmd/nginx-ingress/main.go | 37 +++++++++++++++++++++ 5 files changed, 71 insertions(+), 11 deletions(-) diff --git a/charts/nginx-ingress/templates/_helpers.tpl b/charts/nginx-ingress/templates/_helpers.tpl index 301c64d51e..01c268420f 100644 --- a/charts/nginx-ingress/templates/_helpers.tpl +++ b/charts/nginx-ingress/templates/_helpers.tpl @@ -212,6 +212,7 @@ Build the args for the service binary. - -nginx-plus={{ .Values.controller.nginxplus }} - -nginx-reload-timeout={{ .Values.controller.nginxReloadTimeout }} - -enable-app-protect={{ .Values.controller.appprotect.enable }} +- -log-format={{ .Values.controller.logFormat }} {{- if and .Values.controller.appprotect.enable .Values.controller.appprotect.logLevel }} - -app-protect-log-level={{ .Values.controller.appprotect.logLevel }} {{ end }} @@ -244,7 +245,7 @@ Build the args for the service binary. - -health-status={{ .Values.controller.healthStatus }} - -health-status-uri={{ .Values.controller.healthStatusURI }} - -nginx-debug={{ .Values.controller.nginxDebug }} -- -v={{ .Values.controller.logLevel }} +- -log-level={{ .Values.controller.logLevel }} - -nginx-status={{ .Values.controller.nginxStatus.enable }} {{- if .Values.controller.nginxStatus.enable }} - -nginx-status-port={{ .Values.controller.nginxStatus.port }} diff --git a/charts/nginx-ingress/values.schema.json b/charts/nginx-ingress/values.schema.json index 37297c6bbc..a8a2760474 100644 --- a/charts/nginx-ingress/values.schema.json +++ b/charts/nginx-ingress/values.schema.json @@ -486,17 +486,19 @@ ] }, "logLevel": { - "type": "integer", - "default": 1, + "type": "string", + "default": "info", "title": "The logLevel of the Ingress Controller", "enum": [ - 0, - 1, - 2, - 3 + "trace", + "debug", + "info", + "warning", + "error", + "fatal" ], "examples": [ - 1 + "info" ] }, "customPorts": { @@ -1674,7 +1676,7 @@ "hostNetwork": false, "nginxDebug": false, "shareProcessNamespace": false, - "logLevel": 1, + "logLevel": "info", "customPorts": [], "image": { "repository": "nginx/nginx-ingress", @@ -2214,7 +2216,7 @@ }, "hostNetwork": false, "nginxDebug": false, - "logLevel": 1, + "logLevel": "info", "customPorts": [], "image": { "repository": "nginx/nginx-ingress", diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index c8892f0223..66f80f9dc7 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -124,7 +124,7 @@ controller: shareProcessNamespace: false ## The log level of the Ingress Controller. - logLevel: 1 + logLevel: info ## A list of custom ports to expose on the NGINX Ingress Controller pod. Follows the conventional Kubernetes yaml syntax for container ports. customPorts: [] @@ -552,6 +552,9 @@ controller: ## variable_hash_bucket_size, and variable_hash_max_size in the ConfigMap based on the number of two-way splits. enableWeightChangesDynamicReload: false + logFormat: glog + + rbac: ## Configures RBAC. create: true diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index a3b6acf53b..6ed3cb0bb9 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -208,6 +208,11 @@ var ( enableTelemetryReporting = flag.Bool("enable-telemetry-reporting", true, "Enable gathering and reporting of product related telemetry.") + logFormat = flag.String("log-format", "glog", "Set log format to either glog, text, or json.") + + logLevel = flag.String("log-level", "info", + `Sets log level for Ingress Controller. Allowed values: fatal, error, warning, info, debug, trace.`) + enableDynamicWeightChangesReload = flag.Bool(dynamicWeightChangesParam, false, "Enable changing weights of split clients without reloading NGINX. Requires -nginx-plus") startupCheckFn func() error @@ -310,6 +315,18 @@ func mustValidateWatchedNamespaces() { // mustValidateFlags checks the values for various flags // and calls os.Exit if any of the flags is invalid. func mustValidateFlags() { + validLogFormats := map[string]bool{"glog": true, "text": true, "json": true} + if !validLogFormats[*logFormat] { + glog.Fatalf("Invalid log format: %s. Valid options are: glog, text, json", *logFormat) + os.Exit(1) + } + + validLogLevel := map[string]bool{"info": true, "trace": true, "debug": true, "warning": true, "error": true, "fatal": true} + if !validLogLevel[*logLevel] { + glog.Fatalf("Invalid log level: %s. Valid options are: trace, debug, info, warning, error, fatal", *logFormat) + os.Exit(1) + } + healthStatusURIValidationError := validateLocation(*healthStatusURI) if healthStatusURIValidationError != nil { glog.Fatalf("Invalid value for health-status-uri: %v", healthStatusURIValidationError) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index fe70d76eb1..a9f8619b55 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "log/slog" "net" "net/http" "os" @@ -41,12 +42,23 @@ import ( kitlog "github.com/go-kit/log" "github.com/go-kit/log/level" + + nic_logger "github.com/nginxinc/kubernetes-ingress/internal/logger" + nic_glog "github.com/nginxinc/kubernetes-ingress/internal/logger/glog" ) // Injected during build 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, + } ) const ( @@ -63,6 +75,11 @@ func main() { commitHash, commitTime, dirtyBuild := getBuildInfo() fmt.Printf("NGINX Ingress Controller Version=%v Commit=%v Date=%v DirtyState=%v Arch=%v/%v Go=%v\n", version, commitHash, commitTime, dirtyBuild, runtime.GOOS, runtime.GOARCH, runtime.Version()) parseFlags() + ctx := initLogger(logLevels[*logLevel]) + l := nic_logger.LoggerFromContext(ctx) + l.Debug(fmt.Sprintf("LOGFORMAT = %s", *logFormat)) + l.Log(ctx, nic_glog.LevelTrace, fmt.Sprintf("LOGLEVEL = %s", *logLevel)) + initValidate() parsedFlags := os.Args[1:] @@ -877,3 +894,23 @@ func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, appPro glog.Errorf("Failed to update pod labels after %d attempts", maxRetries) } } + +func initLogger(level slog.Level) context.Context { + programLevel := new(slog.LevelVar) // Info by default + var h slog.Handler + switch { + case *logFormat == "json": + h = slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: programLevel}) + case *logFormat == "text": + h = slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: programLevel}) + default: + h = nic_glog.New(os.Stdout, &nic_glog.Options{Level: programLevel}) + } + l := slog.New(h) + slog.SetDefault(l) + c := context.Background() + + programLevel.Set(level) + + return nic_logger.ContextWithLogger(c, l) +} From 8c5a5f9c29c02839bf69f7f5c01c485db9331a99 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 26 Sep 2024 15:07:55 +0100 Subject: [PATCH 05/14] Add tests --- cmd/nginx-ingress/flags.go | 27 ++++++++++++++++-------- cmd/nginx-ingress/flags_test.go | 37 ++++++++++++++++++++++++++++----- cmd/nginx-ingress/main.go | 15 ++++++------- 3 files changed, 58 insertions(+), 21 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 6ed3cb0bb9..9163c2de0f 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -315,14 +315,14 @@ func mustValidateWatchedNamespaces() { // mustValidateFlags checks the values for various flags // and calls os.Exit if any of the flags is invalid. func mustValidateFlags() { - validLogFormats := map[string]bool{"glog": true, "text": true, "json": true} - if !validLogFormats[*logFormat] { + logFormatValidationError := validateLogFormat(*logFormat) + if logFormatValidationError != nil { glog.Fatalf("Invalid log format: %s. Valid options are: glog, text, json", *logFormat) os.Exit(1) } - validLogLevel := map[string]bool{"info": true, "trace": true, "debug": true, "warning": true, "error": true, "fatal": true} - if !validLogLevel[*logLevel] { + logLevelValidationError := validateLogLevel(*logLevel) + if logLevelValidationError != nil { glog.Fatalf("Invalid log level: %s. Valid options are: trace, debug, info, warning, error, fatal", *logFormat) os.Exit(1) } @@ -364,8 +364,8 @@ func mustValidateFlags() { } if *appProtectLogLevel != appProtectLogLevelDefault && *appProtect && *nginxPlus { - logLevelValidationError := validateAppProtectLogLevel(*appProtectLogLevel) - if logLevelValidationError != nil { + appProtectlogLevelValidationError := validateLogLevel(*appProtectLogLevel) + if appProtectlogLevelValidationError != nil { glog.Fatalf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel) } } @@ -460,8 +460,8 @@ func validatePort(port int) error { return nil } -// validateAppProtectLogLevel makes sure a given logLevel is one of the allowed values -func validateAppProtectLogLevel(logLevel string) error { +// validateLogLevel makes sure a given logLevel is one of the allowed values +func validateLogLevel(logLevel string) error { switch strings.ToLower(logLevel) { case "fatal", @@ -472,7 +472,16 @@ func validateAppProtectLogLevel(logLevel string) error { "trace": return nil } - return fmt.Errorf("invalid App Protect log level: %v", logLevel) + return fmt.Errorf("invalid log level: %v", logLevel) +} + +// validateLogFormat makes sure a given logFormat is one of the allowed values +func validateLogFormat(logFormat string) error { + switch strings.ToLower(logFormat) { + case "glog", "json", "text": + return nil + } + return fmt.Errorf("invalid log format: %v", logFormat) } // parseNginxStatusAllowCIDRs converts a comma separated CIDR/IP address string into an array of CIDR/IP addresses. diff --git a/cmd/nginx-ingress/flags_test.go b/cmd/nginx-ingress/flags_test.go index 1715d2a3e8..eeb6c2b597 100644 --- a/cmd/nginx-ingress/flags_test.go +++ b/cmd/nginx-ingress/flags_test.go @@ -125,7 +125,7 @@ func TestValidateLocation(t *testing.T) { } } -func TestValidateAppProtectLogLevel(t *testing.T) { +func TestValidateLogLevel(t *testing.T) { badLogLevels := []string{ "", "critical", @@ -133,9 +133,9 @@ func TestValidateAppProtectLogLevel(t *testing.T) { "info;", } for _, badLogLevel := range badLogLevels { - err := validateAppProtectLogLevel(badLogLevel) + err := validateLogLevel(badLogLevel) if err == nil { - t.Errorf("validateAppProtectLogLevel(%v) returned no error when it should have returned an error", badLogLevel) + t.Errorf("validateLogLevel(%v) returned no error when it should have returned an error", badLogLevel) } } @@ -148,9 +148,9 @@ func TestValidateAppProtectLogLevel(t *testing.T) { "trace", } for _, goodLogLevel := range goodLogLevels { - err := validateAppProtectLogLevel(goodLogLevel) + err := validateLogLevel(goodLogLevel) if err != nil { - t.Errorf("validateAppProtectLogLevel(%v) returned an error when it should have returned no error: %v", goodLogLevel, err) + t.Errorf("validateLogLevel(%v) returned an error when it should have returned no error: %v", goodLogLevel, err) } } } @@ -172,3 +172,30 @@ func TestValidateNamespaces(t *testing.T) { } } } + +func TestValidateLogFormat(t *testing.T) { + badLogFormats := []string{ + "", + "jason", + "txt", + "gloog", + } + for _, badLogFormat := range badLogFormats { + err := validateLogFormat(badLogFormat) + if err == nil { + t.Errorf("validateLogFormat(%v) returned no error when it should have returned an error", badLogFormat) + } + } + + goodLogFormats := []string{ + "json", + "text", + "glog", + } + for _, goodLogFormat := range goodLogFormats { + err := validateLogFormat(goodLogFormat) + if err != nil { + t.Errorf("validateLogFormat(%v) returned an error when it should have returned no error: %v", goodLogFormat, err) + } + } +} diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index a9f8619b55..2c00024f0e 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "io" "log/slog" "net" "net/http" @@ -75,7 +76,7 @@ func main() { commitHash, commitTime, dirtyBuild := getBuildInfo() fmt.Printf("NGINX Ingress Controller Version=%v Commit=%v Date=%v DirtyState=%v Arch=%v/%v Go=%v\n", version, commitHash, commitTime, dirtyBuild, runtime.GOOS, runtime.GOARCH, runtime.Version()) parseFlags() - ctx := initLogger(logLevels[*logLevel]) + ctx := initLogger(*logFormat, logLevels[*logLevel], os.Stdout) l := nic_logger.LoggerFromContext(ctx) l.Debug(fmt.Sprintf("LOGFORMAT = %s", *logFormat)) l.Log(ctx, nic_glog.LevelTrace, fmt.Sprintf("LOGLEVEL = %s", *logLevel)) @@ -895,16 +896,16 @@ func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, appPro } } -func initLogger(level slog.Level) context.Context { +func initLogger(logFormat string, level slog.Level, out io.Writer) context.Context { programLevel := new(slog.LevelVar) // Info by default var h slog.Handler switch { - case *logFormat == "json": - h = slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: programLevel}) - case *logFormat == "text": - h = slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: programLevel}) + case logFormat == "json": + h = slog.NewJSONHandler(out, &slog.HandlerOptions{Level: programLevel}) + case logFormat == "text": + h = slog.NewTextHandler(out, &slog.HandlerOptions{Level: programLevel}) default: - h = nic_glog.New(os.Stdout, &nic_glog.Options{Level: programLevel}) + h = nic_glog.New(out, &nic_glog.Options{Level: programLevel}) } l := slog.New(h) slog.SetDefault(l) From 90feb34e606f93295ea3571f67e4bb01f84b1be3 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Thu, 26 Sep 2024 15:13:54 +0100 Subject: [PATCH 06/14] add tests for log format --- cmd/nginx-ingress/main_test.go | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 cmd/nginx-ingress/main_test.go diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go new file mode 100644 index 0000000000..95c5c1d2ba --- /dev/null +++ b/cmd/nginx-ingress/main_test.go @@ -0,0 +1,48 @@ +package main + +import ( + "bytes" + "regexp" + "testing" + + nic_logger "github.com/nginxinc/kubernetes-ingress/internal/logger" + nic_glog "github.com/nginxinc/kubernetes-ingress/internal/logger/glog" +) + +func TestLogFormats(t *testing.T) { + testCases := []struct { + name string + format string + wantre string + }{ + { + name: "glog level format message", + format: "glog", + wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, + }, + { + name: "json level format message", + format: "json", + wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, + }, + { + name: "text level format message", + format: "text", + wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, + }, + } + t.Parallel() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + ctx := initLogger(tc.format, nic_glog.LevelInfo, &buf) + l := nic_logger.LoggerFromContext(ctx) + l.Log(ctx, nic_glog.LevelInfo, "test") + got := buf.String() + re := regexp.MustCompile(tc.wantre) + if !re.MatchString(got) { + t.Errorf("\ngot:\n%q\nwant:\n%q", got, tc.wantre) + } + }) + } +} From 7187be05f02cc3be3905186dd3793f660057a2bd Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 26 Sep 2024 16:24:06 +0100 Subject: [PATCH 07/14] fix tests --- cmd/nginx-ingress/main_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 95c5c1d2ba..f7a75cc2ed 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -23,12 +23,12 @@ func TestLogFormats(t *testing.T) { { name: "json level format message", format: "json", - wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, + wantre: `^{"time":"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d+.*","level":"INFO","msg":".*}`, }, { name: "text level format message", format: "text", - wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, + wantre: `^time=\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d+.*level=\w+\smsg=\w+`, }, } t.Parallel() From 63eb0cc8f2c8071f2181563372cb0bab50cbb087 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 26 Sep 2024 16:32:01 +0100 Subject: [PATCH 08/14] add logformat comments and in schema --- charts/nginx-ingress/values.schema.json | 13 +++++++++++++ charts/nginx-ingress/values.yaml | 1 + 2 files changed, 14 insertions(+) diff --git a/charts/nginx-ingress/values.schema.json b/charts/nginx-ingress/values.schema.json index a8a2760474..dd73359489 100644 --- a/charts/nginx-ingress/values.schema.json +++ b/charts/nginx-ingress/values.schema.json @@ -501,6 +501,19 @@ "info" ] }, + "logFormat": { + "type": "string", + "default": "glog", + "title": "The logFormat of the Ingress Controller", + "enum": [ + "glog", + "json", + "text" + ], + "examples": [ + "json" + ] + }, "customPorts": { "type": "array", "default": [], diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index 66f80f9dc7..a15b88b1a1 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -552,6 +552,7 @@ controller: ## variable_hash_bucket_size, and variable_hash_max_size in the ConfigMap based on the number of two-way splits. enableWeightChangesDynamicReload: false + ## Sets the log format of Ingress Controller. Options include: glog, json, text logFormat: glog From 0956ac4618cf040c79dcac5f6a3ee8f16f0f8fb3 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 26 Sep 2024 16:33:25 +0100 Subject: [PATCH 09/14] fix spacing --- charts/nginx-ingress/values.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index a15b88b1a1..a9de47480e 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -555,7 +555,6 @@ controller: ## Sets the log format of Ingress Controller. Options include: glog, json, text logFormat: glog - rbac: ## Configures RBAC. create: true From 27c6075279a3043224fb4438eafbcd29100b307a Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 26 Sep 2024 16:35:35 +0100 Subject: [PATCH 10/14] remove debug --- cmd/nginx-ingress/main.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 2c00024f0e..5379e78d71 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -77,9 +77,7 @@ func main() { fmt.Printf("NGINX Ingress Controller Version=%v Commit=%v Date=%v DirtyState=%v Arch=%v/%v Go=%v\n", version, commitHash, commitTime, dirtyBuild, runtime.GOOS, runtime.GOARCH, runtime.Version()) parseFlags() ctx := initLogger(*logFormat, logLevels[*logLevel], os.Stdout) - l := nic_logger.LoggerFromContext(ctx) - l.Debug(fmt.Sprintf("LOGFORMAT = %s", *logFormat)) - l.Log(ctx, nic_glog.LevelTrace, fmt.Sprintf("LOGLEVEL = %s", *logLevel)) + _ = nic_logger.LoggerFromContext(ctx) initValidate() parsedFlags := os.Args[1:] From a049cf2be26e6c0a5b8905afe0ada86b86239528 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Fri, 27 Sep 2024 08:43:56 +0100 Subject: [PATCH 11/14] move logformat to go near loglevel as they are related - address comment --- charts/nginx-ingress/templates/_helpers.tpl | 2 +- charts/nginx-ingress/values.yaml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/charts/nginx-ingress/templates/_helpers.tpl b/charts/nginx-ingress/templates/_helpers.tpl index 01c268420f..b842c8015c 100644 --- a/charts/nginx-ingress/templates/_helpers.tpl +++ b/charts/nginx-ingress/templates/_helpers.tpl @@ -212,7 +212,6 @@ Build the args for the service binary. - -nginx-plus={{ .Values.controller.nginxplus }} - -nginx-reload-timeout={{ .Values.controller.nginxReloadTimeout }} - -enable-app-protect={{ .Values.controller.appprotect.enable }} -- -log-format={{ .Values.controller.logFormat }} {{- if and .Values.controller.appprotect.enable .Values.controller.appprotect.logLevel }} - -app-protect-log-level={{ .Values.controller.appprotect.logLevel }} {{ end }} @@ -246,6 +245,7 @@ Build the args for the service binary. - -health-status-uri={{ .Values.controller.healthStatusURI }} - -nginx-debug={{ .Values.controller.nginxDebug }} - -log-level={{ .Values.controller.logLevel }} +- -log-format={{ .Values.controller.logFormat }} - -nginx-status={{ .Values.controller.nginxStatus.enable }} {{- if .Values.controller.nginxStatus.enable }} - -nginx-status-port={{ .Values.controller.nginxStatus.port }} diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index a9de47480e..88dd548b22 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -126,6 +126,9 @@ controller: ## The log level of the Ingress Controller. logLevel: info + ## Sets the log format of Ingress Controller. Options include: glog, json, text + logFormat: glog + ## A list of custom ports to expose on the NGINX Ingress Controller pod. Follows the conventional Kubernetes yaml syntax for container ports. customPorts: [] @@ -552,9 +555,6 @@ controller: ## variable_hash_bucket_size, and variable_hash_max_size in the ConfigMap based on the number of two-way splits. enableWeightChangesDynamicReload: false - ## Sets the log format of Ingress Controller. Options include: glog, json, text - logFormat: glog - rbac: ## Configures RBAC. create: true From 23b4e495a2c5963aeb0d3a0e216b15f08e479cb7 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Fri, 27 Sep 2024 09:18:43 +0100 Subject: [PATCH 12/14] update logFormat test case names --- cmd/nginx-ingress/main_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index f7a75cc2ed..7ba896c028 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -16,17 +16,17 @@ func TestLogFormats(t *testing.T) { wantre string }{ { - name: "glog level format message", + name: "glog format message", format: "glog", wantre: `^I\d{8}\s\d+:\d+:\d+.\d{6}\s+\d+\s\w+\.go:\d+\]\s.*\s$`, }, { - name: "json level format message", + name: "json format message", format: "json", wantre: `^{"time":"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d+.*","level":"INFO","msg":".*}`, }, { - name: "text level format message", + name: "text format message", format: "text", wantre: `^time=\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d+.*level=\w+\smsg=\w+`, }, From b97840b34a31a58ea2e0b8744beb60ab5afd1631 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Fri, 27 Sep 2024 09:58:31 +0100 Subject: [PATCH 13/14] add docs --- .../command-line-arguments.md | 20 +++++++++++++++++++ .../installing-nic/installation-with-helm.md | 3 ++- .../troubleshooting/troubleshoot-common.md | 10 +++++----- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md index 617fe3ceb6..713cef15b0 100644 --- a/docs/content/configuration/global-configuration/command-line-arguments.md +++ b/docs/content/configuration/global-configuration/command-line-arguments.md @@ -324,6 +324,26 @@ Path to the TransportServer NGINX configuration template for a TransportServer r Log level for V logs. + + +--- + +### -log-level `` + +Log level for Ingress Controller logs. Allowed values: fatal, error, warn, info, debug, trace. + +- Default is `info`. + + + +--- + +### -log-format `` + +Log format for Ingress Controller logs. Allowed values: glog, json, text. + +- Default is `glog`. + --- diff --git a/docs/content/installation/installing-nic/installation-with-helm.md b/docs/content/installation/installing-nic/installation-with-helm.md index 5c4511083b..56a73df565 100644 --- a/docs/content/installation/installing-nic/installation-with-helm.md +++ b/docs/content/installation/installing-nic/installation-with-helm.md @@ -309,7 +309,8 @@ The following tables lists the configurable parameters of the NGINX Ingress Cont | **controller.hostNetwork** | Enables the Ingress Controller pods to use the host's network namespace. | false | | **controller.dnsPolicy** | DNS policy for the Ingress Controller pods. | ClusterFirst | | **controller.nginxDebug** | Enables debugging for NGINX. Uses the `nginx-debug` binary. Requires `error-log-level: debug` in the ConfigMap via `controller.config.entries`. | false | -| **controller.logLevel** | The log level of the Ingress Controller. | 1 | +| **controller.logLevel** | The log level of the Ingress Controller. | info | +| **controller.logFormat** | The log format of the Ingress Controller. | glog | | **controller.image.digest** | The image digest of the Ingress Controller. | None | | **controller.image.repository** | The image repository of the Ingress Controller. | nginx/nginx-ingress | | **controller.image.tag** | The tag of the Ingress Controller image. | {{< nic-version >}} | diff --git a/docs/content/troubleshooting/troubleshoot-common.md b/docs/content/troubleshooting/troubleshoot-common.md index 7a99f26761..887addfea9 100644 --- a/docs/content/troubleshooting/troubleshoot-common.md +++ b/docs/content/troubleshooting/troubleshoot-common.md @@ -85,7 +85,7 @@ There are two places to configure more verbose logging for NGINX Ingress Control When using `manifest` for deployment, use the command line argument `-nginx-debug` in your deployment or daemonset. -You can add the `-v` parameter to increase the verbosity of the NGINX Ingress Controller process. +You can add the `-log-level` parameter to increase the verbosity of the NGINX Ingress Controller process. Here is a small snippet of setting these command line arguments in the `args` section of a deployment: @@ -94,7 +94,7 @@ args: - -nginx-configmaps=$(POD_NAMESPACE)/nginx-config - -enable-cert-manager - -nginx-debug - - -v=3 + - -log-level=debug ``` **ConfigMap settings** @@ -116,7 +116,7 @@ If you are using `helm`, you can adjust these two settings: ```none controller.nginxDebug = true or false -controller.loglevel = 1 to 3 value +controller.loglevel = fatal, error, warn, info, debug or trace ``` For example, if using a `values.yaml` file: @@ -126,7 +126,7 @@ For example, if using a `values.yaml` file: nginxDebug: true ## The log level of the Ingress Controller. - logLevel: 3 + logLevel: debug ``` This is a more complete example `values.yaml` file: @@ -135,7 +135,7 @@ This is a more complete example `values.yaml` file: controller: kind: Deployment nginxDebug: true - logLevel: 3 + logLevel: debug annotations: nginx: ingress-prod pod: From b2a3a7909b3c2e4173c2b40e96abd941f6b43734 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Fri, 27 Sep 2024 10:17:51 +0100 Subject: [PATCH 14/14] change fatal messages to warning, update values comment, add glog as option for future proof --- charts/nginx-ingress/values.yaml | 2 +- cmd/nginx-ingress/flags.go | 28 ++++++++++++++-------------- cmd/nginx-ingress/main.go | 2 ++ 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index 88dd548b22..2252d656d2 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -123,7 +123,7 @@ controller: ## Share process namespace between containers in the Ingress Controller pod. shareProcessNamespace: false - ## The log level of the Ingress Controller. + ## The log level of the Ingress Controller. Options include: trace, debug, info, warning, error, fatal logLevel: info ## Sets the log format of Ingress Controller. Options include: glog, json, text diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 9163c2de0f..4f75da672d 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -19,6 +19,8 @@ const ( dynamicWeightChangesParam = "weight-changes-dynamic-reload" appProtectLogLevelDefault = "fatal" appProtectEnforcerAddrDefault = "127.0.0.1:50000" + logLevelDefault = "info" + logFormatDefault = "glog" ) var ( @@ -208,9 +210,9 @@ var ( enableTelemetryReporting = flag.Bool("enable-telemetry-reporting", true, "Enable gathering and reporting of product related telemetry.") - logFormat = flag.String("log-format", "glog", "Set log format to either glog, text, or json.") + logFormat = flag.String("log-format", logFormatDefault, "Set log format to either glog, text, or json.") - logLevel = flag.String("log-level", "info", + logLevel = flag.String("log-level", logLevelDefault, `Sets log level for Ingress Controller. Allowed values: fatal, error, warning, info, debug, trace.`) enableDynamicWeightChangesReload = flag.Bool(dynamicWeightChangesParam, false, "Enable changing weights of split clients without reloading NGINX. Requires -nginx-plus") @@ -228,6 +230,16 @@ func parseFlags() { } func initValidate() { + 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) + } + + 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) + } + if *enableLatencyMetrics && !*enablePrometheusMetrics { glog.Warning("enable-latency-metrics flag requires enable-prometheus-metrics, latency metrics will not be collected") *enableLatencyMetrics = false @@ -315,18 +327,6 @@ func mustValidateWatchedNamespaces() { // mustValidateFlags checks the values for various flags // and calls os.Exit if any of the flags is invalid. func mustValidateFlags() { - logFormatValidationError := validateLogFormat(*logFormat) - if logFormatValidationError != nil { - glog.Fatalf("Invalid log format: %s. Valid options are: glog, text, json", *logFormat) - os.Exit(1) - } - - logLevelValidationError := validateLogLevel(*logLevel) - if logLevelValidationError != nil { - glog.Fatalf("Invalid log level: %s. Valid options are: trace, debug, info, warning, error, fatal", *logFormat) - os.Exit(1) - } - healthStatusURIValidationError := validateLocation(*healthStatusURI) if healthStatusURIValidationError != nil { glog.Fatalf("Invalid value for health-status-uri: %v", healthStatusURIValidationError) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 5379e78d71..569e2e550e 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -898,6 +898,8 @@ func initLogger(logFormat string, level slog.Level, out io.Writer) context.Conte programLevel := new(slog.LevelVar) // Info by default var h slog.Handler switch { + case logFormat == "glog": + h = nic_glog.New(out, &nic_glog.Options{Level: programLevel}) case logFormat == "json": h = slog.NewJSONHandler(out, &slog.HandlerOptions{Level: programLevel}) case logFormat == "text":