fix a number of unbounded dimensions in request metrics (kubernetes#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

Author: Han Kang

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD

@@ -3,6 +3,7 @@ package(default_visibility = ["//visibility:public"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
+    "go_test",
 )
 
 go_library(
@@ -35,3 +36,9 @@ filegroup(
     srcs = [":package-srcs"],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["metrics_test.go"],
+    embed = [":go_default_library"],
+)

staging/src/k8s.io/apiserver/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 {

staging/src/k8s.io/apiserver/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)
+			}
+		})
+	}
+}

staging/src/k8s.io/apiserver/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="/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="/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="/readyz",verb="GET",version=""} 1
 `)
 	if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, expected, "apiserver_request_total"); err != nil {
 		t.Error(err)