From cae935d32a0388532ef11b7169aaf435ef9aeecf Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Thu, 9 Nov 2017 12:41:01 -0700 Subject: [PATCH 1/7] Implement loggable map for HTTP status --- docs/user-guide/nginx-configuration/configmap.md | 5 +++++ internal/ingress/controller/config/config.go | 1 + internal/ingress/controller/template/configmap.go | 8 ++++++++ .../ingress/controller/template/configmap_test.go | 2 ++ internal/ingress/defaults/main.go | 6 ++++++ rootfs/etc/nginx/template/nginx.tmpl | 13 ++++++++++++- 6 files changed, 34 insertions(+), 1 deletion(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index c55b7502a..f11231a83 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -194,6 +194,7 @@ The following table shows a configuration option's name, type, and the default v |[denylist-source-range](#denylist-source-range)|[]string|[]string{}|| |[whitelist-source-range](#whitelist-source-range)|[]string|[]string{}|| |[skip-access-log-urls](#skip-access-log-urls)|[]string|[]string{}|| +|[skip-access-log-http-statuses](#skip-access-log-http-statuses)|[]string|[]string{}|| |[limit-rate](#limit-rate)|int|0|| |[limit-rate-after](#limit-rate-after)|int|0|| |[lua-shared-dicts](#lua-shared-dicts)|string|""|| @@ -1172,6 +1173,10 @@ See [ngx_http_access_module](https://nginx.org/en/docs/http/ngx_http_access_modu Sets a list of URLs that should not appear in the NGINX access log. This is useful with urls like `/health` or `health-check` that make "complex" reading the logs. _**default:**_ is empty +## skip-access-log-http-statuses + +Sets a list of HTTP statuses that should not appear in the NGINX access log. This is useful for high volume ingress where turning access logging completely off is undesirable, but not logging things like 2xx and 3xx responses is desirable. _**default:**_ is empty + ## limit-rate Limits the rate of response transmission to a client. The rate is specified in bytes per second. The zero value disables rate limiting. The limit is set per a request, and so if a client simultaneously opens two connections, the overall rate will be twice as much as the specified limit. diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index ec44b08ed..62453fa71 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -962,6 +962,7 @@ func NewDefault() Configuration { DenylistSourceRange: []string{}, WhitelistSourceRange: []string{}, SkipAccessLogURLs: []string{}, + SkipAccessLogHTTPStatuses: []string{}, LimitRate: 0, LimitRateAfter: 0, ProxyBuffering: "off", diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index c73f3b6c0..0e7e69f08 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -40,6 +40,7 @@ import ( const ( customHTTPErrors = "custom-http-errors" skipAccessLogUrls = "skip-access-log-urls" + skipAccessLogHTTPStatuses = "skip-access-log-http-statuses" whitelistSourceRange = "whitelist-source-range" denylistSourceRange = "denylist-source-range" proxyRealIPCIDR = "proxy-real-ip-cidr" @@ -101,6 +102,7 @@ func ReadConfig(src map[string]string) config.Configuration { to := config.NewDefault() errors := make([]int, 0) skipUrls := make([]string, 0) + skipHTTPStatuses := make([]string, 0) denyList := make([]string, 0) whiteList := make([]string, 0) proxyList := make([]string, 0) @@ -171,6 +173,11 @@ func ReadConfig(src map[string]string) config.Configuration { skipUrls = splitAndTrimSpace(val, ",") } + if val, ok := conf[skipAccessLogHTTPStatuses]; ok { + delete(conf, skipAccessLogHTTPStatuses) + skipHTTPStatuses = strings.Split(val, ",") + } + if val, ok := conf[denylistSourceRange]; ok { delete(conf, denylistSourceRange) denyList = append(denyList, splitAndTrimSpace(val, ",")...) @@ -402,6 +409,7 @@ func ReadConfig(src map[string]string) config.Configuration { to.CustomHTTPErrors = filterErrors(errors) to.SkipAccessLogURLs = skipUrls + to.SkipAccessLogHTTPStatuses = skipHTTPStatuses to.DenylistSourceRange = denyList to.WhitelistSourceRange = whiteList to.ProxyRealIPCIDR = proxyList diff --git a/internal/ingress/controller/template/configmap_test.go b/internal/ingress/controller/template/configmap_test.go index dad841694..3184a956a 100644 --- a/internal/ingress/controller/template/configmap_test.go +++ b/internal/ingress/controller/template/configmap_test.go @@ -58,6 +58,7 @@ func TestMergeConfigMapToStruct(t *testing.T) { "proxy-read-timeout": "1", "proxy-send-timeout": "2", "skip-access-log-urls": "/log,/demo,/test", + "skip-access-log-http-statuses": "^[23],204,302,^201", "use-proxy-protocol": "true", "disable-access-log": "true", "access-log-params": "buffer=4k gzip", @@ -85,6 +86,7 @@ func TestMergeConfigMapToStruct(t *testing.T) { def.AccessLogPath = "/var/log/test/access.log" def.ErrorLogPath = "/var/log/test/error.log" def.SkipAccessLogURLs = []string{"/log", "/demo", "/test"} + def.SkipAccessLogHTTPStatuses = []string{"^[23]", "204", "302", "^201"} def.ProxyReadTimeout = 1 def.ProxySendTimeout = 2 def.UseProxyProtocol = true diff --git a/internal/ingress/defaults/main.go b/internal/ingress/defaults/main.go index 0aab2ff47..4c89dfb14 100644 --- a/internal/ingress/defaults/main.go +++ b/internal/ingress/defaults/main.go @@ -107,6 +107,12 @@ type Backend struct { // By default this list is empty SkipAccessLogURLs []string `json:"skip-access-log-urls"` + // SkipAccessLogHTTPStatuses sets a list of HTTP statuses that should not appear in the NGINX access log + // The status strings provided are interpreted by an NGINX map as regex + // This is useful with statuses like 2xx and 3xx that make "complex" reading the logs + // By default this list is empty + SkipAccessLogHTTPStatuses []string `json:"skip-access-log-http-statuses,-"` + // Enables or disables the redirect (301) to the HTTPS port SSLRedirect bool `json:"ssl-redirect"` diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index a1e02aae3..70d739ae7 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -391,9 +391,20 @@ http { {{/* map urls that should not appear in access.log */}} {{/* http://nginx.org/en/docs/http/ngx_http_log_module.html#access_log */}} - map $request_uri $loggable { + map $request_uri $loggable_request_url { {{ range $reqUri := $cfg.SkipAccessLogURLs }} {{ $reqUri }} 0;{{ end }} + } + + {{/* map HTTP statuses that should not appear in access.log */}} + {{/* http://nginx.org/en/docs/http/ngx_http_log_module.html#access_log */}} + map $status $loggable_http_status { + {{ range $reqHTTPStatus := $cfg.SkipAccessLogHTTPStatuses }} + ~{{ $reqHTTPStatus }} 0;{{ end }} + } + + map "${loggable_request_url}${loggable_http_status}" $loggable { + ~0 0; default 1; } From c9f31be9aa8794d936ec49073270b2b0b3d7eb2e Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Mon, 24 Apr 2023 09:06:31 -0600 Subject: [PATCH 2/7] gofmt --- internal/ingress/controller/config/config.go | 54 ++++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 62453fa71..6d2089690 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -942,33 +942,33 @@ func NewDefault() Configuration { ProxyStreamNextUpstreamTimeout: "600s", ProxyStreamNextUpstreamTries: 3, Backend: defaults.Backend{ - ProxyBodySize: bodySize, - ProxyConnectTimeout: 5, - ProxyReadTimeout: 60, - ProxySendTimeout: 60, - ProxyBuffersNumber: 4, - ProxyBufferSize: "4k", - ProxyCookieDomain: "off", - ProxyCookiePath: "off", - ProxyNextUpstream: "error timeout", - ProxyNextUpstreamTimeout: 0, - ProxyNextUpstreamTries: 3, - ProxyRequestBuffering: "on", - ProxyRedirectFrom: "off", - ProxyRedirectTo: "off", - PreserveTrailingSlash: false, - SSLRedirect: true, - CustomHTTPErrors: []int{}, - DenylistSourceRange: []string{}, - WhitelistSourceRange: []string{}, - SkipAccessLogURLs: []string{}, - SkipAccessLogHTTPStatuses: []string{}, - LimitRate: 0, - LimitRateAfter: 0, - ProxyBuffering: "off", - ProxyHTTPVersion: "1.1", - ProxyMaxTempFileSize: "1024m", - ServiceUpstream: false, + ProxyBodySize: bodySize, + ProxyConnectTimeout: 5, + ProxyReadTimeout: 60, + ProxySendTimeout: 60, + ProxyBuffersNumber: 4, + ProxyBufferSize: "4k", + ProxyCookieDomain: "off", + ProxyCookiePath: "off", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: 0, + ProxyNextUpstreamTries: 3, + ProxyRequestBuffering: "on", + ProxyRedirectFrom: "off", + ProxyRedirectTo: "off", + PreserveTrailingSlash: false, + SSLRedirect: true, + CustomHTTPErrors: []int{}, + DenylistSourceRange: []string{}, + WhitelistSourceRange: []string{}, + SkipAccessLogURLs: []string{}, + SkipAccessLogHTTPStatuses: []string{}, + LimitRate: 0, + LimitRateAfter: 0, + ProxyBuffering: "off", + ProxyHTTPVersion: "1.1", + ProxyMaxTempFileSize: "1024m", + ServiceUpstream: false, }, UpstreamKeepaliveConnections: 320, UpstreamKeepaliveTime: "1h", From a352164aaf0d9a5e36eda115c920d1dff4b7e08e Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Mon, 24 Apr 2023 10:17:40 -0600 Subject: [PATCH 3/7] Make doc more useful --- docs/user-guide/nginx-configuration/configmap.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index f11231a83..e3b6b925c 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -1175,7 +1175,7 @@ Sets a list of URLs that should not appear in the NGINX access log. This is usef ## skip-access-log-http-statuses -Sets a list of HTTP statuses that should not appear in the NGINX access log. This is useful for high volume ingress where turning access logging completely off is undesirable, but not logging things like 2xx and 3xx responses is desirable. _**default:**_ is empty +Sets a comma separated list of HTTP statuses, each interpreted as a regular expression, that should not appear in the NGINX access log. This is useful for high volume ingress where turning access logging completely off is undesirable, but not logging things like 2xx and 3xx responses is desirable. _**default:**_ is empty ## limit-rate From f4eaef7e85018604a198354ffbf9731076963baa Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Mon, 24 Apr 2023 18:33:37 +0000 Subject: [PATCH 4/7] e2e test for skip-access-log-http-statuses annotation --- .../annotations/skipaccessloghttpstatuses.go | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 test/e2e/annotations/skipaccessloghttpstatuses.go diff --git a/test/e2e/annotations/skipaccessloghttpstatuses.go b/test/e2e/annotations/skipaccessloghttpstatuses.go new file mode 100644 index 000000000..5de51bc46 --- /dev/null +++ b/test/e2e/annotations/skipaccessloghttpstatuses.go @@ -0,0 +1,101 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package annotations + +import ( + "net/http" + "strings" + + "github.com/onsi/ginkgo/v2" + + "k8s.io/ingress-nginx/test/e2e/framework" + "github.com/stretchr/testify/assert" +) + +var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { + f := framework.NewDefaultFramework("skipaccessloghttpstatuses") + + ginkgo.BeforeEach(func() { + f.NewEchoDeployment() + }) + + ginkgo.It("skip-access-log-http-statuses 200 literal, 200 OK", func() { + host := "skipaccessloghttpstatuses.go.foo.com" + + f.UpdateNginxConfigMapData("skip-access-log-http-statuses", "200") + ing := framework.NewSingleIngress(host, "/prefixOne", host, f.Namespace, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + f.WaitForNginxConfiguration(func(ngx string) bool { + return strings.Contains(ngx, `~200 0;`) + }) + + f.HTTPTestClient(). + GET("/prefixOne"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) + + logs, err := f.NginxLogs() + assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") + assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) + }) + + ginkgo.It("skip-access-log-http-statuses ^2.. regex, 200 OK", func() { + host := "skipaccessloghttpstatuses.go.foo.com" + + f.UpdateNginxConfigMapData("skip-access-log-http-statuses", "^2..") + ing := framework.NewSingleIngress(host, "/prefixOne", host, f.Namespace, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + f.WaitForNginxConfiguration(func(ngx string) bool { + return strings.Contains(ngx, `~^2.. 0;`) + }) + + f.HTTPTestClient(). + GET("/prefixOne"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) + + logs, err := f.NginxLogs() + assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") + assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) + }) + + ginkgo.It("skip-access-log-http-statuses ^2.. regex, 404 Not Found", func() { + host := "skipaccessloghttpstatuses.go.foo.com" + + f.UpdateNginxConfigMapData("skip-access-log-http-statuses", "^2..") + ing := framework.NewSingleIngress(host, "/prefixOne", host, f.Namespace, framework.EchoService, 80, nil) + f.EnsureIngress(ing) + + f.WaitForNginxConfiguration(func(ngx string) bool { + return strings.Contains(ngx, `~^2.. 0;`) + }) + + f.HTTPTestClient(). + GET("/404"). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) + + logs, err := f.NginxLogs() + assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") + assert.Contains(ginkgo.GinkgoT(), logs, `GET /404 HTTP/1.1" 404`) + }) +}) From 0e259ab81fc42aaaf0b0b3691f40b8e4491fe88a Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Mon, 24 Apr 2023 19:17:20 +0000 Subject: [PATCH 5/7] gofmt --- .../annotations/skipaccessloghttpstatuses.go | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/test/e2e/annotations/skipaccessloghttpstatuses.go b/test/e2e/annotations/skipaccessloghttpstatuses.go index 5de51bc46..ef5b23d65 100644 --- a/test/e2e/annotations/skipaccessloghttpstatuses.go +++ b/test/e2e/annotations/skipaccessloghttpstatuses.go @@ -17,21 +17,21 @@ limitations under the License. package annotations import ( - "net/http" + "net/http" "strings" "github.com/onsi/ginkgo/v2" + "github.com/stretchr/testify/assert" "k8s.io/ingress-nginx/test/e2e/framework" - "github.com/stretchr/testify/assert" ) var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { f := framework.NewDefaultFramework("skipaccessloghttpstatuses") - ginkgo.BeforeEach(func() { - f.NewEchoDeployment() - }) + ginkgo.BeforeEach(func() { + f.NewEchoDeployment() + }) ginkgo.It("skip-access-log-http-statuses 200 literal, 200 OK", func() { host := "skipaccessloghttpstatuses.go.foo.com" @@ -44,15 +44,15 @@ var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { return strings.Contains(ngx, `~200 0;`) }) - f.HTTPTestClient(). - GET("/prefixOne"). - WithHeader("Host", host). - Expect(). - Status(http.StatusOK) + f.HTTPTestClient(). + GET("/prefixOne"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) - logs, err := f.NginxLogs() - assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") - assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) + logs, err := f.NginxLogs() + assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") + assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) }) ginkgo.It("skip-access-log-http-statuses ^2.. regex, 200 OK", func() { @@ -66,15 +66,15 @@ var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { return strings.Contains(ngx, `~^2.. 0;`) }) - f.HTTPTestClient(). - GET("/prefixOne"). - WithHeader("Host", host). - Expect(). - Status(http.StatusOK) + f.HTTPTestClient(). + GET("/prefixOne"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) - logs, err := f.NginxLogs() - assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") - assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) + logs, err := f.NginxLogs() + assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") + assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) }) ginkgo.It("skip-access-log-http-statuses ^2.. regex, 404 Not Found", func() { @@ -88,14 +88,14 @@ var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { return strings.Contains(ngx, `~^2.. 0;`) }) - f.HTTPTestClient(). - GET("/404"). - WithHeader("Host", host). - Expect(). - Status(http.StatusNotFound) + f.HTTPTestClient(). + GET("/404"). + WithHeader("Host", host). + Expect(). + Status(http.StatusNotFound) - logs, err := f.NginxLogs() - assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") - assert.Contains(ginkgo.GinkgoT(), logs, `GET /404 HTTP/1.1" 404`) + logs, err := f.NginxLogs() + assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") + assert.Contains(ginkgo.GinkgoT(), logs, `GET /404 HTTP/1.1" 404`) }) }) From 2058a265480b84c8feae1745f5735c7ff34fc3c1 Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Tue, 25 Apr 2023 00:18:19 +0000 Subject: [PATCH 6/7] Make e2e test useful --- test/e2e/annotations/skipaccessloghttpstatuses.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/annotations/skipaccessloghttpstatuses.go b/test/e2e/annotations/skipaccessloghttpstatuses.go index ef5b23d65..76eb7eb72 100644 --- a/test/e2e/annotations/skipaccessloghttpstatuses.go +++ b/test/e2e/annotations/skipaccessloghttpstatuses.go @@ -52,7 +52,7 @@ var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { logs, err := f.NginxLogs() assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") - assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) + assert.NotContains(ginkgo.GinkgoT(), logs, `GET /prefixOne HTTP/1.1" 200`) }) ginkgo.It("skip-access-log-http-statuses ^2.. regex, 200 OK", func() { @@ -74,7 +74,7 @@ var _ = framework.DescribeAnnotation("skip-access-log-http-statuses", func() { logs, err := f.NginxLogs() assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs") - assert.NotContains(ginkgo.GinkgoT(), logs, `GET / HTTP/1.1" 200`) + assert.NotContains(ginkgo.GinkgoT(), logs, `GET /prefixOne HTTP/1.1" 200`) }) ginkgo.It("skip-access-log-http-statuses ^2.. regex, 404 Not Found", func() { From dc730c5e4014b2babd0eabdf7c0057b63f180299 Mon Sep 17 00:00:00 2001 From: Andrew Davidoff Date: Fri, 5 May 2023 13:36:53 +0000 Subject: [PATCH 7/7] Do not omit SkipAccessLogHTTPStatuses from json output --- internal/ingress/defaults/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ingress/defaults/main.go b/internal/ingress/defaults/main.go index 4c89dfb14..20619b3ca 100644 --- a/internal/ingress/defaults/main.go +++ b/internal/ingress/defaults/main.go @@ -111,7 +111,7 @@ type Backend struct { // The status strings provided are interpreted by an NGINX map as regex // This is useful with statuses like 2xx and 3xx that make "complex" reading the logs // By default this list is empty - SkipAccessLogHTTPStatuses []string `json:"skip-access-log-http-statuses,-"` + SkipAccessLogHTTPStatuses []string `json:"skip-access-log-http-statuses"` // Enables or disables the redirect (301) to the HTTPS port SSLRedirect bool `json:"ssl-redirect"`