diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD index 8abb3d1a611..fee49f24acb 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD +++ b/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"], +) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index c79efdef4e3..6f1a836e347 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/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 { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go new file mode 100644 index 00000000000..7f8791f6e2d --- /dev/null +++ b/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) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go b/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go index 321d5bd809c..4f99739aecd 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz_test.go +++ b/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)