fix a number of unbounded dimensions in request metrics (#89451)

* fix a number of unbounded dimensions in request metrics

* add test suite for cleanVerb and cleanContentType

* Properly validate that the content-type and charset (if applicable) are RFC compliant

* add additional test case

* truncate list of content-types

Change-Id: Ia5fe0d2e2c602e4def4b8e0849cc19f3f9251818

Kubernetes-commit: 6c588c3f441252f42fd37526297ed92d1e1f3acf

Godeps/Godeps.json

@@ -672,7 +672,7 @@
 		},
 		{
 			"ImportPath": "k8s.io/apimachinery",
-			"Rev": "e0747e0da69d"
+			"Rev": "17bacc71f57a"
 		},
 		{
 			"ImportPath": "k8s.io/client-go",
@@ -680,7 +680,7 @@
 		},
 		{
 			"ImportPath": "k8s.io/component-base",
-			"Rev": "80bc5fe191e9"
+			"Rev": "ee971924d913"
 		},
 		{
 			"ImportPath": "k8s.io/gengo",

go.mod

@@ -43,9 +43,9 @@ require (
 	gopkg.in/square/go-jose.v2 v2.2.2
 	gopkg.in/yaml.v2 v2.2.8
 	k8s.io/api v0.0.0-20200526202119-6f652b6ce59c
-	k8s.io/apimachinery v0.0.0-20200525041908-e0747e0da69d
+	k8s.io/apimachinery v0.0.0-20200528161915-17bacc71f57a
 	k8s.io/client-go v0.0.0-20200527002520-f099a72e140a
-	k8s.io/component-base v0.0.0-20200527003234-80bc5fe191e9
+	k8s.io/component-base v0.0.0-20200529003043-ee971924d913
 	k8s.io/klog/v2 v2.0.0
 	k8s.io/kube-openapi v0.0.0-20200427153329-656914f816f9
 	k8s.io/utils v0.0.0-20200414100711-2df71ebbae66
@@ -58,7 +58,7 @@ replace (
 	golang.org/x/sys => golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a // pinned to release-branch.go1.13
 	golang.org/x/tools => golang.org/x/tools v0.0.0-20190821162956-65e3620a7ae7 // pinned to release-branch.go1.13
 	k8s.io/api => k8s.io/api v0.0.0-20200526202119-6f652b6ce59c
-	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20200525041908-e0747e0da69d
+	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20200528161915-17bacc71f57a
 	k8s.io/client-go => k8s.io/client-go v0.0.0-20200527002520-f099a72e140a
-	k8s.io/component-base => k8s.io/component-base v0.0.0-20200527003234-80bc5fe191e9
+	k8s.io/component-base => k8s.io/component-base v0.0.0-20200529003043-ee971924d913
 )

go.sum

@@ -448,9 +448,9 @@ honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 k8s.io/api v0.0.0-20200526202119-6f652b6ce59c/go.mod h1:SrtLvWo/N06luJcHPmYFDOnMrJk8U2Cr6Ir1liACC+0=
-k8s.io/apimachinery v0.0.0-20200525041908-e0747e0da69d/go.mod h1:x4z2+k1N0YTBvV8PmaVs4/hSmKVVENZmTqI8gBygpLA=
+k8s.io/apimachinery v0.0.0-20200528161915-17bacc71f57a/go.mod h1:x4z2+k1N0YTBvV8PmaVs4/hSmKVVENZmTqI8gBygpLA=
 k8s.io/client-go v0.0.0-20200527002520-f099a72e140a/go.mod h1:Jdh8KHY0T/tsn3wzb16Rx/7b5J8deuWFAaQ3ys+sNtw=
-k8s.io/component-base v0.0.0-20200527003234-80bc5fe191e9/go.mod h1:XcbDDOI89vzfqFamX0jn8R50zju0wlGcpv08+UPDoPo=
+k8s.io/component-base v0.0.0-20200529003043-ee971924d913/go.mod h1:+xL2xoK5MN1rAG3Uxf7IBBwc70/0GwbxpOYf2lckPdg=
 k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/klog/v2 v2.0.0 h1:Foj74zO6RbjjP4hBEKjnYtjjAhGg4jNynUdYF6fJrok=
 k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=

pkg/endpoints/metrics/metrics.go

@@ -28,7 +28,6 @@ import (
 	"time"
 
 	restful "github.com/emicklei/go-restful"
-
 	"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/types"
 	utilsets "k8s.io/apimachinery/pkg/util/sets"
@@ -48,6 +47,8 @@ type resettableCollector interface {
 
 const (
 	APIServerComponent string = "apiserver"
+	OtherContentType   string = "other"
+	OtherRequestMethod string = "other"
 )
 
 /*
@@ -172,6 +173,37 @@ var (
 		currentInflightRequests,
 		requestTerminationsTotal,
 	}
+
+	// these are the known (e.g. whitelisted/known) content types which we will report for
+	// request metrics. Any other RFC compliant content types will be aggregated under 'unknown'
+	knownMetricContentTypes = utilsets.NewString(
+		"application/apply-patch+yaml",
+		"application/json",
+		"application/json-patch+json",
+		"application/merge-patch+json",
+		"application/strategic-merge-patch+json",
+		"application/vnd.kubernetes.protobuf",
+		"application/vnd.kubernetes.protobuf;stream=watch",
+		"application/yaml",
+		"text/plain",
+		"text/plain;charset=utf-8")
+	// these are the valid request methods which we report in our metrics. Any other request methods
+	// will be aggregated under 'unknown'
+	validRequestMethods = utilsets.NewString(
+		"APPLY",
+		"CONNECT",
+		"CREATE",
+		"DELETE",
+		"DELETECOLLECTION",
+		"GET",
+		"LIST",
+		"PATCH",
+		"POST",
+		"PROXY",
+		"PUT",
+		"UPDATE",
+		"WATCH",
+		"WATCHLIST")
 )
 
 const (
@@ -219,6 +251,10 @@ func RecordRequestTermination(req *http.Request, requestInfo *request.RequestInf
 	// translated to RequestInfo).
 	// However, we need to tweak it e.g. to differentiate GET from LIST.
 	verb := canonicalVerb(strings.ToUpper(req.Method), scope)
+	// set verbs to a bounded set of known and expected verbs
+	if !validRequestMethods.Has(verb) {
+		verb = OtherRequestMethod
+	}
 	if requestInfo.IsResourceRequest {
 		requestTerminationsTotal.WithLabelValues(cleanVerb(verb, req), requestInfo.APIGroup, requestInfo.APIVersion, requestInfo.Resource, requestInfo.Subresource, scope, component, codeToString(code)).Inc()
 	} else {
@@ -256,7 +292,8 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour
 	reportedVerb := cleanVerb(verb, req)
 	dryRun := cleanDryRun(req.URL)
 	elapsedSeconds := elapsed.Seconds()
-	requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, contentType, codeToString(httpCode)).Inc()
+	cleanContentType := cleanContentType(contentType)
+	requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, cleanContentType, codeToString(httpCode)).Inc()
 	requestLatencies.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds)
 	// We are only interested in response sizes of read requests.
 	if verb == "GET" || verb == "LIST" {
@@ -311,6 +348,19 @@ func InstrumentHandlerFunc(verb, group, version, resource, subresource, scope, c
 	}
 }
 
+// cleanContentType binds the contentType (for metrics related purposes) to a
+// bounded set of known/expected content-types.
+func cleanContentType(contentType string) string {
+	normalizedContentType := strings.ToLower(contentType)
+	if strings.HasSuffix(contentType, " stream=watch") || strings.HasSuffix(contentType, " charset=utf-8") {
+		normalizedContentType = strings.ReplaceAll(contentType, " ", "")
+	}
+	if knownMetricContentTypes.Has(normalizedContentType) {
+		return normalizedContentType
+	}
+	return OtherContentType
+}
+
 // CleanScope returns the scope of the request.
 func CleanScope(requestInfo *request.RequestInfo) string {
 	if requestInfo.Namespace != "" {
@@ -355,7 +405,10 @@ func cleanVerb(verb string, request *http.Request) string {
 	if verb == "PATCH" && request.Header.Get("Content-Type") == string(types.ApplyPatchType) && utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {
 		reportedVerb = "APPLY"
 	}
-	return reportedVerb
+	if validRequestMethods.Has(reportedVerb) {
+		return reportedVerb
+	}
+	return OtherRequestMethod
 }
 
 func cleanDryRun(u *url.URL) string {

pkg/endpoints/metrics/metrics_test.go

@@ -0,0 +1,153 @@
+/*
+Copyright 2019 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 metrics
+
+import (
+	"fmt"
+	"net/http"
+	"net/url"
+	"testing"
+)
+
+func TestCleanVerb(t *testing.T) {
+	testCases := []struct {
+		desc         string
+		initialVerb  string
+		request      *http.Request
+		expectedVerb string
+	}{
+		{
+			desc:         "An empty string should be designated as unknown",
+			initialVerb:  "",
+			request:      nil,
+			expectedVerb: "other",
+		},
+		{
+			desc:         "LIST should normally map to LIST",
+			initialVerb:  "LIST",
+			request:      nil,
+			expectedVerb: "LIST",
+		},
+		{
+			desc:        "LIST should be transformed to WATCH if we have the right query param on the request",
+			initialVerb: "LIST",
+			request: &http.Request{
+				URL: &url.URL{
+					RawQuery: "watch=true",
+				},
+			},
+			expectedVerb: "WATCH",
+		},
+		{
+			desc:        "LIST isn't transformed to WATCH if we have query params that do not include watch",
+			initialVerb: "LIST",
+			request: &http.Request{
+				URL: &url.URL{
+					RawQuery: "blah=asdf&something=else",
+				},
+			},
+			expectedVerb: "LIST",
+		},
+		{
+			desc:         "WATCHLIST should be transformed to WATCH",
+			initialVerb:  "WATCHLIST",
+			request:      nil,
+			expectedVerb: "WATCH",
+		},
+		{
+			desc:        "PATCH should be transformed to APPLY with the right content type",
+			initialVerb: "PATCH",
+			request: &http.Request{
+				Header: http.Header{
+					"Content-Type": []string{"application/apply-patch+yaml"},
+				},
+			},
+			expectedVerb: "APPLY",
+		},
+		{
+			desc:         "PATCH shouldn't be transformed to APPLY without the right content type",
+			initialVerb:  "PATCH",
+			request:      nil,
+			expectedVerb: "PATCH",
+		},
+		{
+			desc:         "WATCHLIST should be transformed to WATCH",
+			initialVerb:  "WATCHLIST",
+			request:      nil,
+			expectedVerb: "WATCH",
+		},
+		{
+			desc:         "unexpected verbs should be designated as unknown",
+			initialVerb:  "notValid",
+			request:      nil,
+			expectedVerb: "other",
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.initialVerb, func(t *testing.T) {
+			req := &http.Request{URL: &url.URL{}}
+			if tt.request != nil {
+				req = tt.request
+			}
+			cleansedVerb := cleanVerb(tt.initialVerb, req)
+			if cleansedVerb != tt.expectedVerb {
+				t.Errorf("Got %s, but expected %s", cleansedVerb, tt.expectedVerb)
+			}
+		})
+	}
+}
+
+func TestContentType(t *testing.T) {
+	testCases := []struct {
+		rawContentType      string
+		expectedContentType string
+	}{
+		{
+			rawContentType:      "application/json",
+			expectedContentType: "application/json",
+		},
+		{
+			rawContentType:      "image/svg+xml",
+			expectedContentType: "other",
+		},
+		{
+			rawContentType:      "text/plain; charset=utf-8",
+			expectedContentType: "text/plain;charset=utf-8",
+		},
+		{
+			rawContentType:      "application/json;foo=bar",
+			expectedContentType: "other",
+		},
+		{
+			rawContentType:      "application/json;charset=hancoding",
+			expectedContentType: "other",
+		},
+		{
+			rawContentType:      "unknownbutvalidtype",
+			expectedContentType: "other",
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(fmt.Sprintf("parse %s", tt.rawContentType), func(t *testing.T) {
+			cleansedContentType := cleanContentType(tt.rawContentType)
+			if cleansedContentType != tt.expectedContentType {
+				t.Errorf("Got %s, but expected %s", cleansedContentType, tt.expectedContentType)
+			}
+		})
+	}
+}

pkg/server/healthz/healthz_test.go

@@ -256,9 +256,9 @@ func TestMetrics(t *testing.T) {
 	expected := strings.NewReader(`
         # HELP apiserver_request_total [ALPHA] Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, and HTTP response contentType and code.
         # TYPE apiserver_request_total counter
-        apiserver_request_total{code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/healthz",verb="GET",version=""} 1
+        apiserver_request_total{code="200",component="",contentType="text/plain;charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/healthz",verb="GET",version=""} 1
-        apiserver_request_total{code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/livez",verb="GET",version=""} 1
+        apiserver_request_total{code="200",component="",contentType="text/plain;charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/livez",verb="GET",version=""} 1
-        apiserver_request_total{code="200",component="",contentType="text/plain; charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/readyz",verb="GET",version=""} 1
+        apiserver_request_total{code="200",component="",contentType="text/plain;charset=utf-8",dry_run="",group="",resource="",scope="",subresource="/readyz",verb="GET",version=""} 1
 `)
 	if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, expected, "apiserver_request_total"); err != nil {
 		t.Error(err)