Skip to content

Commit

Permalink
fix a number of unbounded dimensions in request metrics
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@logicalhan
logicalhan committed May 29, 2020
1 parent c9d7581 commit 813fcfb
Show file tree
Hide file tree
Showing 3 changed files with 197 additions and 10 deletions.
12 changes: 6 additions & 6 deletions staging/src/k8s.io/apiserver/pkg/endpoints/metrics/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@ load(
"go_test",
)

go_test(
name = "go_default_test",
srcs = ["metrics_test.go"],
embed = [":go_default_library"],
)

go_library(
name = "go_default_library",
srcs = ["metrics.go"],
Expand Down Expand Up @@ -43,3 +37,9 @@ filegroup(
srcs = [":package-srcs"],
tags = ["automanaged"],
)

go_test(
name = "go_default_test",
srcs = ["metrics_test.go"],
embed = [":go_default_library"],
)
60 changes: 57 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ type resettableCollector interface {

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

/*
Expand Down Expand Up @@ -214,6 +216,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 (
Expand Down Expand Up @@ -261,6 +294,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 {
Expand Down Expand Up @@ -301,8 +338,9 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour
client := ""
elapsedMicroseconds := float64(elapsed / time.Microsecond)
elapsedSeconds := elapsed.Seconds()
requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, client, contentType, codeToString(httpCode)).Inc()
deprecatedRequestCounter.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component, client, contentType, codeToString(httpCode)).Inc()
cleanedContentType := cleanContentType(contentType)
requestCounter.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component, client, cleanedContentType, codeToString(httpCode)).Inc()
deprecatedRequestCounter.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component, client, cleanedContentType, codeToString(httpCode)).Inc()
requestLatencies.WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds)
deprecatedRequestLatencies.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component).Observe(elapsedMicroseconds)
deprecatedRequestLatenciesSummary.WithLabelValues(reportedVerb, group, version, resource, subresource, scope, component).Observe(elapsedMicroseconds)
Expand Down Expand Up @@ -359,6 +397,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 != "" {
Expand Down Expand Up @@ -403,7 +454,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 {
Expand Down
135 changes: 134 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ limitations under the License.

package metrics

import "testing"
import (
"fmt"
"net/http"
"net/url"
"testing"
)

func TestCleanUserAgent(t *testing.T) {
panicBuf := []byte{198, 73, 129, 133, 90, 216, 104, 29, 13, 134, 209, 233, 30, 0, 22}
Expand Down Expand Up @@ -52,3 +57,131 @@ func TestCleanUserAgent(t *testing.T) {
}
}
}
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)
}
})
}
}

0 comments on commit 813fcfb

Please sign in to comment.