From eab5e62fccc48d6b6a9b224c380db524b41d61a5 Mon Sep 17 00:00:00 2001 From: Hans Feldt <2808287+hafe@users.noreply.github.com> Date: Wed, 29 May 2024 17:51:21 +0200 Subject: [PATCH] Make access_log in http context configurable Closes #5625 Signed-off-by: Hans Feldt <2808287+hafe@users.noreply.github.com> --- .../configmap-resource.md | 2 + internal/configs/config_params.go | 3 +- internal/configs/configmaps.go | 14 +++- internal/configs/configmaps_test.go | 79 +++++++++++++++++++ .../version1/__snapshots__/template_test.snap | 53 ++++++++----- internal/configs/version1/config.go | 2 +- internal/configs/version1/nginx-plus.tmpl | 6 +- internal/configs/version1/nginx.tmpl | 6 +- internal/configs/version1/template_test.go | 9 +++ 9 files changed, 139 insertions(+), 35 deletions(-) diff --git a/docs/content/configuration/global-configuration/configmap-resource.md b/docs/content/configuration/global-configuration/configmap-resource.md index 04473a969b..d6fffd9d33 100644 --- a/docs/content/configuration/global-configuration/configmap-resource.md +++ b/docs/content/configuration/global-configuration/configmap-resource.md @@ -113,6 +113,8 @@ For more information, view the [VirtualServer and VirtualServerRoute resources]( |ConfigMap Key | Description | Default | Example | | ---| ---| ---| --- | |*error-log-level* | Sets the global [error log level](https://nginx.org/en/docs/ngx_core_module.html#error_log) for NGINX. | *notice* | | +|*access-log* | Sets the directive [access log](https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log). A syslog destination is the only valid value. The value will be set to its default in-case user tries to configure it with location other than a syslog. +| ``/dev/stdout main`` | ``syslog:server=localhost:514`` | |*access-log-off* | Disables the [access log](https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log). | *False* | | |*default-server-access-log-off* | Disables the [access log](https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log) for the default server. If access log is disabled globally (*access-log-off: "True"*), then the default server access log is always disabled. | *False* | | |*log-format* | Sets the custom [log format](https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format) for HTTP and HTTPS traffic. For convenience, it is possible to define the log format across multiple lines (each line separated by *\n*). In that case, the Ingress Controller will replace every *\n* character with a space character. All *'* characters must be escaped. | See the [template file](https://github.com/nginxinc/kubernetes-ingress/blob/v{{< nic-version >}}/internal/configs/version1/nginx.tmpl) for the access log. | [Custom Log Format](https://github.com/nginxinc/kubernetes-ingress/tree/v{{< nic-version >}}/examples/shared-examples/custom-log-format). | diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index f124a08473..620c6dbcf0 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -24,7 +24,7 @@ type ConfigParams struct { Keepalive int LBMethod string LocationSnippets []string - MainAccessLogOff bool + MainAccessLog string MainErrorLogLevel string MainHTTPSnippets []string MainKeepaliveRequests int64 @@ -188,6 +188,7 @@ func NewDefaultConfigParams(isPlus bool) *ConfigParams { ProxySendTimeout: "60s", ClientMaxBodySize: "1m", SSLRedirect: true, + MainAccessLog: "/dev/stdout main", MainServerNamesHashBucketSize: "256", MainServerNamesHashMaxSize: "1024", MainMapHashBucketSize: "256", diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index e372be286f..09e1f641b1 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -207,11 +207,21 @@ func ParseConfigMap(cfgm *v1.ConfigMap, nginxPlus bool, hasAppProtect bool, hasA cfgParams.MainErrorLogLevel = errorLogLevel } + if accessLog, exists := cfgm.Data["access-log"]; exists { + if !strings.HasPrefix(accessLog, "syslog:") { + glog.Warningf("Configmap %s/%s: Invalid value for key access-log: %q", cfgm.GetNamespace(), cfgm.GetName(), accessLog) + } else { + cfgParams.MainAccessLog = accessLog + } + } + if accessLogOff, exists, err := GetMapKeyAsBool(cfgm.Data, "access-log-off", cfgm); exists { if err != nil { glog.Error(err) } else { - cfgParams.MainAccessLogOff = accessLogOff + if accessLogOff { + cfgParams.MainAccessLog = "off" + } } } @@ -514,7 +524,7 @@ func ParseConfigMap(cfgm *v1.ConfigMap, nginxPlus bool, hasAppProtect bool, hasA // GenerateNginxMainConfig generates MainConfig. func GenerateNginxMainConfig(staticCfgParams *StaticConfigParams, config *ConfigParams) *version1.MainConfig { nginxCfg := &version1.MainConfig{ - AccessLogOff: config.MainAccessLogOff, + AccessLog: config.MainAccessLog, DefaultServerAccessLogOff: config.DefaultServerAccessLogOff, DefaultServerReturn: config.DefaultServerReturn, DisableIPV6: staticCfgParams.DisableIPV6, diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index fc5c4fb205..3713f3ecb7 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -203,3 +203,82 @@ func TestParseConfigMapWithoutTLSPassthroughProxyProtocol(t *testing.T) { }) } } + +func TestParseConfigMapAccessLog(t *testing.T) { + t.Parallel() + tests := []struct { + accessLog string + accessLogOff string + want string + msg string + }{ + { + accessLogOff: "False", + accessLog: "syslog:server=localhost:514", + want: "syslog:server=localhost:514", + msg: "Non default access_log", + }, + { + accessLogOff: "False", + accessLog: "/tmp/nginx main", + want: "/dev/stdout main", + msg: "access_log to file is not allowed", + }, + { + accessLogOff: "True", + accessLog: "/dev/stdout main", + want: "off", + msg: "Disabled access_log", + }, + } + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + cm := &v1.ConfigMap{ + Data: map[string]string{ + "access-log": test.accessLog, + "access-log-off": test.accessLogOff, + }, + } + result := ParseConfigMap(cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + if result.MainAccessLog != test.want { + t.Errorf("want %q, got %q", test.want, result.MainAccessLog) + } + }) + } +} + +func TestParseConfigMapAccessLogDefault(t *testing.T) { + t.Parallel() + tests := []struct { + accessLog string + accessLogOff string + want string + msg string + }{ + { + want: "/dev/stdout main", + msg: "Default access_log", + }, + } + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + cm := &v1.ConfigMap{ + Data: map[string]string{ + "access-log-off": "False", + }, + } + result := ParseConfigMap(cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + if result.MainAccessLog != test.want { + t.Errorf("want %q, got %q", test.want, result.MainAccessLog) + } + }) + } +} diff --git a/internal/configs/version1/__snapshots__/template_test.snap b/internal/configs/version1/__snapshots__/template_test.snap index f56992a046..ec16b28c1a 100644 --- a/internal/configs/version1/__snapshots__/template_test.snap +++ b/internal/configs/version1/__snapshots__/template_test.snap @@ -34,7 +34,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -164,7 +164,8 @@ http { ' "$http_referer" "$http_user_agent"' ; app_protect_dos_arb_fqdn arb.test.server.com; - access_log /dev/stdout main; + + access_log /dev/stdout main; app_protect_failure_mode_action pass; app_protect_compressed_requests_action pass; app_protect_cookie_seed ABCDEFGHIJKLMNOP; @@ -306,7 +307,8 @@ http { '' $sent_http_grpc_status; } app_protect_enforcer_address enforcer.svc.local; - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -1711,7 +1713,8 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -1848,7 +1851,8 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -1985,7 +1989,8 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -2122,7 +2127,8 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -2259,7 +2265,8 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -2422,7 +2429,8 @@ http { ' "$http_referer" "$http_user_agent"' ; app_protect_dos_arb_fqdn arb.test.server.com; - access_log /dev/stdout main; + + access_log /dev/stdout main; app_protect_failure_mode_action pass; app_protect_compressed_requests_action pass; app_protect_cookie_seed ABCDEFGHIJKLMNOP; @@ -2569,7 +2577,8 @@ http { 'outcome=$app_protect_dos_outcome, reason=$app_protect_dos_outcome_reason, ' 'ip_tls=$remote_addr:$app_protect_dos_tls_fp, '; app_protect_dos_arb_fqdn arb.test.server.com; - access_log /dev/stdout main; + + access_log /dev/stdout main; app_protect_failure_mode_action pass; app_protect_compressed_requests_action pass; app_protect_cookie_seed ABCDEFGHIJKLMNOP; @@ -2722,7 +2731,8 @@ http { ' "$http_referer" "$http_user_agent"' ; app_protect_dos_arb_fqdn arb.test.server.com; - access_log /dev/stdout main; + + access_log /dev/stdout main; app_protect_failure_mode_action pass; app_protect_compressed_requests_action pass; app_protect_cookie_seed ABCDEFGHIJKLMNOP; @@ -2863,7 +2873,8 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3017,7 +3028,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3138,7 +3149,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3259,7 +3270,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3380,7 +3391,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3501,7 +3512,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3641,7 +3652,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3762,7 +3773,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -3884,7 +3895,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; @@ -4005,7 +4016,7 @@ http { default $upstream_trailer_grpc_status; '' $sent_http_grpc_status; } - access_log /dev/stdout main; + access_log /dev/stdout main; sendfile on; #tcp_nopush on; diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index 6677bde3f6..22fa313857 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -191,7 +191,7 @@ type Location struct { // MainConfig describe the main NGINX configuration file. type MainConfig struct { - AccessLogOff bool + AccessLog string DefaultServerAccessLogOff bool DefaultServerReturn string DisableIPV6 bool diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl index da4f18a65b..01b2b7d144 100644 --- a/internal/configs/version1/nginx-plus.tmpl +++ b/internal/configs/version1/nginx-plus.tmpl @@ -90,11 +90,7 @@ http { app_protect_enforcer_address {{ .AppProtectV5EnforcerAddr }}; {{- end}} - {{- if .AccessLogOff}} - access_log off; - {{- else}} - access_log /dev/stdout main; - {{- end}} + access_log {{.AccessLog}}; {{- if .LatencyMetrics}} log_format response_time '{"upstreamAddress":"$upstream_addr", "upstreamResponseTime":"$upstream_response_time", "proxyHost":"$proxy_host", "upstreamStatus": "$upstream_status"}'; diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index f4f46243ee..04677d943f 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -62,11 +62,7 @@ http { default "{{ .StaticSSLPath }}"; } {{- end }} - {{- if .AccessLogOff}} - access_log off; - {{- else}} - access_log /dev/stdout main; - {{- end}} + access_log {{.AccessLog}}; {{- if .LatencyMetrics}} log_format response_time '{"upstreamAddress":"$upstream_addr", "upstreamResponseTime":"$upstream_response_time", "proxyHost":"$proxy_host", "upstreamStatus": "$upstream_status"}'; diff --git a/internal/configs/version1/template_test.go b/internal/configs/version1/template_test.go index bc620d8f42..f3f089ab21 100644 --- a/internal/configs/version1/template_test.go +++ b/internal/configs/version1/template_test.go @@ -2039,6 +2039,7 @@ var ( }, AppProtectDosLogFormatEscaping: "json", AppProtectDosArbFqdn: "arb.test.server.com", + AccessLog: "/dev/stdout main", } mainCfgR31 = MainConfig{ @@ -2067,6 +2068,7 @@ var ( NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), AppProtectV5LoadModule: true, AppProtectV5EnforcerAddr: "enforcer.svc.local", + AccessLog: "/dev/stdout main", } mainCfgHTTP2On = MainConfig{ @@ -2106,6 +2108,7 @@ var ( AppProtectDosLoadModule: true, AppProtectDosLogFormat: []string{}, AppProtectDosArbFqdn: "arb.test.server.com", + AccessLog: "/dev/stdout main", } mainCfgCustomTLSPassthroughPort = MainConfig{ @@ -2132,6 +2135,7 @@ var ( TLSPassthrough: true, TLSPassthroughPort: 8443, NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), + AccessLog: "/dev/stdout main", } mainCfgWithoutTLSPassthrough = MainConfig{ @@ -2158,6 +2162,7 @@ var ( TLSPassthrough: false, TLSPassthroughPort: 8443, NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), + AccessLog: "/dev/stdout main", } mainCfgDefaultTLSPassthroughPort = MainConfig{ @@ -2184,6 +2189,7 @@ var ( TLSPassthrough: true, TLSPassthroughPort: 443, NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), + AccessLog: "/dev/stdout main", } mainCfgCustomDefaultHTTPAndHTTPSListenerPorts = MainConfig{ @@ -2210,6 +2216,7 @@ var ( VariablesHashBucketSize: 256, VariablesHashMaxSize: 1024, NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), + AccessLog: "/dev/stdout main", } mainCfgCustomDefaultHTTPListenerPort = MainConfig{ @@ -2236,6 +2243,7 @@ var ( VariablesHashBucketSize: 256, VariablesHashMaxSize: 1024, NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), + AccessLog: "/dev/stdout main", } mainCfgCustomDefaultHTTPSListenerPort = MainConfig{ @@ -2262,6 +2270,7 @@ var ( VariablesHashBucketSize: 256, VariablesHashMaxSize: 1024, NginxVersion: nginx.NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)"), + AccessLog: "/dev/stdout main", } // Vars for Mergable Ingress Master - Minion tests