From 0e2cd58306343281ed0cd6766c740199df4fa64b Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 17 Jun 2020 15:30:31 +0100 Subject: [PATCH] Skip misconfigured vts entries (#227) * Extra filtering for VTS zones Misconfigured ingress resources have the ability to create invalid VTS entries. These are long-lived and are not rectified againt the running nginx config. Because of this, it is possible that the metrics exported by feed can flip between the valid and invalid VTS data causing erroneous metrics to be exported. This adds an extra filter to ensure that metrics are not generated for misconfigured ingress resources. * Update Changelog --- CHANGELOG.md | 3 + nginx/nginx_metrics.go | 4 +- nginx/nginx_test.go | 208 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea4a875c..ba49059f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,6 @@ +# v3.1.7 +* [BUGFIX] Do not generate Prometheus metrics for invalid nginx-vts entries + # v3.1.6 * [BUGFIX] Reduce the number of DescribeTargetGroups to avoid reaching the API limits * [BUGFIX] Do not set the nlb updater as initialised if it failed to describe the target groups diff --git a/nginx/nginx_metrics.go b/nginx/nginx_metrics.go index 8490e5cb..cc95789d 100644 --- a/nginx/nginx_metrics.go +++ b/nginx/nginx_metrics.go @@ -142,8 +142,8 @@ func updateIngressMetrics(metrics VTSMetrics) { } strs := strings.Split(zone, "::") - if len(strs) != 2 { - log.Warnf("filter name not formatted as expected for %s, got %s without a '::' split", host, zone) + if len(strs) != 2 || strs[1] == "" { + log.Warnf("filter name not formatted as expected for %s, got %s without a valid '::' split", host, zone) continue } path := strs[0] diff --git a/nginx/nginx_test.go b/nginx/nginx_test.go index 0bfa88c0..1592e484 100644 --- a/nginx/nginx_test.go +++ b/nginx/nginx_test.go @@ -2047,6 +2047,15 @@ func TestUpdatesMetricsFromNginxStatusPage(t *testing.T) { assertEndpointRequestCounters(t, "kube-system.10.254.201.199.80", "10.254.201.199:80", 2910.0, 1570.0, 1.0, 10.0, 9.0, 2.0, 3.0) + + // Assert that hosts with both valid and invalid entries for the same path generate metrics for the correct, valid VTS entry + assertIngressRequestCounters(t, + "ingress-with-valid-duplicate-path.sandbox.cosmic.sky", "/path/", + 5000.0, 2000.0, 0.0, 5.0, 0.0, 0.0, 0.0) + // Assert that invalid paths do not generate metrics, even if the VTS data shows hits (e.g. 3xx's) + assertIngressRequestCounters(t, + "ingress-with-invalid-path.sandbox.cosmic.sky", "/bad/", + 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0) } func assertIngressRequestCounters(t *testing.T, host, path string, in, out, ones, twos, threes, fours, fives float64) { @@ -2267,6 +2276,84 @@ var statusResponseBody = []byte(`{ "scarce": 0 } }, + "ingress-with-valid-duplicate-path.sandbox.cosmic.sky": { + "requestCounter": 5, + "inBytes": 5000, + "outBytes": 2000, + "responses": { + "1xx": 0, + "2xx": 5, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + }, + "overCounts": { + "maxIntegerSize": 18446744073709551615, + "requestCounter": 0, + "inBytes": 0, + "outBytes": 0, + "1xx": 0, + "2xx": 0, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + } + }, + "ingress-with-invalid-path.sandbox.cosmic.sky": { + "requestCounter": 10, + "inBytes": 10, + "outBytes": 5, + "responses": { + "1xx": 0, + "2xx": 0, + "3xx": 10, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + }, + "overCounts": { + "maxIntegerSize": 18446744073709551615, + "requestCounter": 0, + "inBytes": 0, + "outBytes": 0, + "1xx": 0, + "2xx": 0, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + } + }, "*": { "requestCounter": 10, "inBytes": 2910, @@ -2389,6 +2476,127 @@ var statusResponseBody = []byte(`{ "scarce": 0 } } + }, + "ingress-with-valid-duplicate-path.sandbox.cosmic.sky": { + "/path/::some-app.10.254.204.100.8080": { + "requestCounter": 10, + "inBytes": 5000, + "outBytes": 2000, + "responses": { + "1xx": 0, + "2xx": 5, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + }, + "overCounts": { + "maxIntegerSize": 18446744073709551615, + "requestCounter": 0, + "inBytes": 0, + "outBytes": 0, + "1xx": 0, + "2xx": 0, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + } + }, + "/path/::": { + "requestCounter": 50, + "inBytes": 50, + "outBytes": 50, + "responses": { + "1xx": 50, + "2xx": 50, + "3xx": 50, + "4xx": 50, + "5xx": 50, + "miss": 50, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + }, + "overCounts": { + "maxIntegerSize": 18446744073709551615, + "requestCounter": 0, + "inBytes": 0, + "outBytes": 0, + "1xx": 0, + "2xx": 0, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + } + } + }, + "ingress-with-invalid-path.sandbox.cosmic.sky": { + "/bad::": { + "requestCounter": 10, + "inBytes": 10, + "outBytes": 5, + "responses": { + "1xx": 0, + "2xx": 10, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + }, + "overCounts": { + "maxIntegerSize": 18446744073709551615, + "requestCounter": 0, + "inBytes": 0, + "outBytes": 0, + "1xx": 0, + "2xx": 0, + "3xx": 0, + "4xx": 0, + "5xx": 0, + "miss": 0, + "bypass": 0, + "expired": 0, + "stale": 0, + "updating": 0, + "revalidated": 0, + "hit": 0, + "scarce": 0 + } + } } }, "upstreamZones": {