bump apiserver_request_total to STABLE status

We've dropped the content-type field since it is effectively unbounded (we had a sec-vuln about this before actually). We retain all other fields, despite their unboundedness due to the fact that we can now explicitly set bounds on label values. Change-Id: Icc483fc6a17ea6382928f4448643cda6f3e21adb
2025-11-04 04:08:16 +00:00 · 2021-03-04 07:52:00 -08:00
parent 7791be16eb
commit cfd00de686
5 changed files with 38 additions and 97 deletions
--- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go
@@ -50,7 +50,6 @@ type resettableCollector interface {

 const (
 	APIServerComponent string = "apiserver"
-	OtherContentType   string = "other"
 	OtherRequestMethod string = "other"
 )

@@ -77,14 +76,10 @@ var (
 	requestCounter = compbasemetrics.NewCounterVec(
 		&compbasemetrics.CounterOpts{
 			Name:           "apiserver_request_total",
-			Help:           "Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, and HTTP response contentType and code.",
-			StabilityLevel: compbasemetrics.ALPHA,
+			Help:           "Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, and HTTP response code.",
+			StabilityLevel: compbasemetrics.STABLE,
 		},
-		// The label_name contentType doesn't follow the label_name convention defined here:
-		// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/instrumentation.md
-		// But changing it would break backwards compatibility. Future label_names
-		// should be all lowercase and separated by underscores.
-		[]string{"verb", "dry_run", "group", "version", "resource", "subresource", "scope", "component", "contentType", "code"},
+		[]string{"verb", "dry_run", "group", "version", "resource", "subresource", "scope", "component", "code"},
 	)
 	longRunningRequestGauge = compbasemetrics.NewGaugeVec(
 		&compbasemetrics.GaugeOpts{
@@ -238,19 +233,6 @@ var (
 		requestAbortsTotal,
 	}

-	// 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(
@@ -395,7 +377,7 @@ func RecordLongRunning(req *http.Request, requestInfo *request.RequestInfo, comp

 // MonitorRequest handles standard transformations for client and the reported verb and then invokes Monitor to record
 // a request. verb must be uppercase to be backwards compatible with existing monitoring tooling.
-func MonitorRequest(req *http.Request, verb, group, version, resource, subresource, scope, component string, deprecated bool, removedRelease string, contentType string, httpCode, respSize int, elapsed time.Duration) {
+func MonitorRequest(req *http.Request, verb, group, version, resource, subresource, scope, component string, deprecated bool, removedRelease string, httpCode, respSize int, elapsed time.Duration) {
 	// We don't use verb from <requestInfo>, as this may be propagated from
 	// InstrumentRouteFunc which is registered in installer.go with predefined
 	// list of verbs (different than those translated to RequestInfo).
@@ -404,8 +386,7 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour

 	dryRun := cleanDryRun(req.URL)
 	elapsedSeconds := elapsed.Seconds()
-	cleanContentType := cleanContentType(contentType)
-	requestCounter.WithContext(req.Context()).WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, cleanContentType, codeToString(httpCode)).Inc()
+	requestCounter.WithContext(req.Context()).WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, codeToString(httpCode)).Inc()
 	// MonitorRequest happens after authentication, so we can trust the username given by the request
 	info, ok := request.UserFrom(req.Context())
 	if ok && info.GetName() == user.APIServerUser {
@@ -449,7 +430,7 @@ func InstrumentRouteFunc(verb, group, version, resource, subresource, scope, com

 		routeFunc(req, response)

-		MonitorRequest(req.Request, verb, group, version, resource, subresource, scope, component, deprecated, removedRelease, delegate.Header().Get("Content-Type"), delegate.Status(), delegate.ContentLength(), time.Since(requestReceivedTimestamp))
+		MonitorRequest(req.Request, verb, group, version, resource, subresource, scope, component, deprecated, removedRelease, delegate.Status(), delegate.ContentLength(), time.Since(requestReceivedTimestamp))
 	})
 }

@@ -474,23 +455,10 @@ func InstrumentHandlerFunc(verb, group, version, resource, subresource, scope, c

 		handler(w, req)

-		MonitorRequest(req, verb, group, version, resource, subresource, scope, component, deprecated, removedRelease, delegate.Header().Get("Content-Type"), delegate.Status(), delegate.ContentLength(), time.Since(requestReceivedTimestamp))
+		MonitorRequest(req, verb, group, version, resource, subresource, scope, component, deprecated, removedRelease, delegate.Status(), delegate.ContentLength(), time.Since(requestReceivedTimestamp))
 	}
 }

-// 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 != "" {
--- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go
@@ -17,7 +17,6 @@ limitations under the License.
 package metrics

 import (
-	"fmt"
 	"net/http"
 	"net/url"
 	"testing"
@@ -110,44 +109,3 @@ func TestCleanVerb(t *testing.T) {
 		})
 	}
 }
-
-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)
-			}
-		})
-	}
-}
--- a/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go
@@ -252,11 +252,11 @@ 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.
+        # HELP apiserver_request_total [STABLE] Counter of apiserver requests broken out for each verb, dry run value, group, version, resource, scope, component, and HTTP response 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="",dry_run="",group="",resource="",scope="",subresource="/healthz",verb="GET",version=""} 1
+        apiserver_request_total{code="200",component="",dry_run="",group="",resource="",scope="",subresource="/livez",verb="GET",version=""} 1
+        apiserver_request_total{code="200",component="",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)
--- a/test/instrumentation/testdata/stable-metrics-list.yaml
+++ b/test/instrumentation/testdata/stable-metrics-list.yaml
@@ -0,0 +1,15 @@
+- name: apiserver_request_total
+  help: Counter of apiserver requests broken out for each verb, dry run value, group,
+    version, resource, scope, component, and HTTP response code.
+  type: Counter
+  stabilityLevel: STABLE
+  labels:
+  - code
+  - component
+  - dry_run
+  - group
+  - resource
+  - scope
+  - subresource
+  - verb
+  - version
--- a/test/integration/metrics/metrics_test.go
+++ b/test/integration/metrics/metrics_test.go
@@ -287,42 +287,42 @@ func TestApiserverMetricsPods(t *testing.T) {
 			executor: func() {
 				callOrDie(c.Create(context.TODO(), makePod("foo"), metav1.CreateOptions{}))
 			},
-			want: `apiserver_request_total{code="201", component="apiserver", contentType="application/json", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="POST", version="v1"}`,
+			want: `apiserver_request_total{code="201", component="apiserver", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="POST", version="v1"}`,
 		},
 		{
 			name: "update pod",
 			executor: func() {
 				callOrDie(c.Update(context.TODO(), makePod("bar"), metav1.UpdateOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="PUT", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="PUT", version="v1"}`,
 		},
 		{
 			name: "update pod status",
 			executor: func() {
 				callOrDie(c.UpdateStatus(context.TODO(), makePod("bar"), metav1.UpdateOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="pods", scope="resource", subresource="status", verb="PUT", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="pods", scope="resource", subresource="status", verb="PUT", version="v1"}`,
 		},
 		{
 			name: "get pod",
 			executor: func() {
 				callOrDie(c.Get(context.TODO(), "foo", metav1.GetOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="GET", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="GET", version="v1"}`,
 		},
 		{
 			name: "list pod",
 			executor: func() {
 				callOrDie(c.List(context.TODO(), metav1.ListOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="pods", scope="namespace", subresource="", verb="LIST", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="pods", scope="namespace", subresource="", verb="LIST", version="v1"}`,
 		},
 		{
 			name: "delete pod",
 			executor: func() {
 				callOrDie(nil, c.Delete(context.TODO(), "foo", metav1.DeleteOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="DELETE", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="pods", scope="resource", subresource="", verb="DELETE", version="v1"}`,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -393,42 +393,42 @@ func TestApiserverMetricsNamespaces(t *testing.T) {
 			executor: func() {
 				callOrDie(c.Create(context.TODO(), makeNamespace("foo"), metav1.CreateOptions{}))
 			},
-			want: `apiserver_request_total{code="201", component="apiserver", contentType="application/json", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="POST", version="v1"}`,
+			want: `apiserver_request_total{code="201", component="apiserver", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="POST", version="v1"}`,
 		},
 		{
 			name: "update namespace",
 			executor: func() {
 				callOrDie(c.Update(context.TODO(), makeNamespace("bar"), metav1.UpdateOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="PUT", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="PUT", version="v1"}`,
 		},
 		{
 			name: "update namespace status",
 			executor: func() {
 				callOrDie(c.UpdateStatus(context.TODO(), makeNamespace("bar"), metav1.UpdateOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="namespaces", scope="resource", subresource="status", verb="PUT", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="namespaces", scope="resource", subresource="status", verb="PUT", version="v1"}`,
 		},
 		{
 			name: "get namespace",
 			executor: func() {
 				callOrDie(c.Get(context.TODO(), "foo", metav1.GetOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="GET", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="GET", version="v1"}`,
 		},
 		{
 			name: "list namespace",
 			executor: func() {
 				callOrDie(c.List(context.TODO(), metav1.ListOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="namespaces", scope="cluster", subresource="", verb="LIST", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="namespaces", scope="cluster", subresource="", verb="LIST", version="v1"}`,
 		},
 		{
 			name: "delete namespace",
 			executor: func() {
 				callOrDie(nil, c.Delete(context.TODO(), "foo", metav1.DeleteOptions{}))
 			},
-			want: `apiserver_request_total{code="200", component="apiserver", contentType="application/json", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="DELETE", version="v1"}`,
+			want: `apiserver_request_total{code="200", component="apiserver", dry_run="", group="", resource="namespaces", scope="resource", subresource="", verb="DELETE", version="v1"}`,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {