From 1a46dfdf5769ae72e35e14749746e064cddf691b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Martins?= Date: Mon, 8 Aug 2022 23:27:45 +0200 Subject: [PATCH] Revert "client-go: remove no longer used finalURLTemplate" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The functionality provided by the finalURLTemplate is still used by certain external projects to track the request latency for requests performed to kube-apiserver. Using a template of the URL, instead of the URL itself, prevents the explosion of label cardinality in exposed metrics since it aggregates the URLs in a way that common URLs requests are reported as being the same. This reverts commit bebf5a608f68523fc430a44f6db26b16022dc862. Signed-off-by: André Martins Kubernetes-commit: 11e16c63c87f07e9e68977c74b937989983754c1 --- rest/request.go | 82 +++++++++++++++++- rest/request_test.go | 200 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 280 insertions(+), 2 deletions(-) diff --git a/rest/request.go b/rest/request.go index dba933f7d6..0c2c5abe2b 100644 --- a/rest/request.go +++ b/rest/request.go @@ -508,6 +508,84 @@ func (r *Request) URL() *url.URL { return finalURL } +// finalURLTemplate is similar to URL(), but will make all specific parameter values equal +// - instead of name or namespace, "{name}" and "{namespace}" will be used, and all query +// parameters will be reset. This creates a copy of the url so as not to change the +// underlying object. +func (r Request) finalURLTemplate() url.URL { + newParams := url.Values{} + v := []string{"{value}"} + for k := range r.params { + newParams[k] = v + } + r.params = newParams + url := r.URL() + + segments := strings.Split(url.Path, "/") + groupIndex := 0 + index := 0 + trimmedBasePath := "" + if url != nil && r.c.base != nil && strings.Contains(url.Path, r.c.base.Path) { + p := strings.TrimPrefix(url.Path, r.c.base.Path) + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + // store the base path that we have trimmed so we can append it + // before returning the URL + trimmedBasePath = r.c.base.Path + segments = strings.Split(p, "/") + groupIndex = 1 + } + if len(segments) <= 2 { + return *url + } + + const CoreGroupPrefix = "api" + const NamedGroupPrefix = "apis" + isCoreGroup := segments[groupIndex] == CoreGroupPrefix + isNamedGroup := segments[groupIndex] == NamedGroupPrefix + if isCoreGroup { + // checking the case of core group with /api/v1/... format + index = groupIndex + 2 + } else if isNamedGroup { + // checking the case of named group with /apis/apps/v1/... format + index = groupIndex + 3 + } else { + // this should not happen that the only two possibilities are /api... and /apis..., just want to put an + // outlet here in case more API groups are added in future if ever possible: + // https://kubernetes.io/docs/concepts/overview/kubernetes-api/#api-groups + // if a wrong API groups name is encountered, return the {prefix} for url.Path + url.Path = "/{prefix}" + url.RawQuery = "" + return *url + } + //switch segLength := len(segments) - index; segLength { + switch { + // case len(segments) - index == 1: + // resource (with no name) do nothing + case len(segments)-index == 2: + // /$RESOURCE/$NAME: replace $NAME with {name} + segments[index+1] = "{name}" + case len(segments)-index == 3: + if segments[index+2] == "finalize" || segments[index+2] == "status" { + // /$RESOURCE/$NAME/$SUBRESOURCE: replace $NAME with {name} + segments[index+1] = "{name}" + } else { + // /namespace/$NAMESPACE/$RESOURCE: replace $NAMESPACE with {namespace} + segments[index+1] = "{namespace}" + } + case len(segments)-index >= 4: + segments[index+1] = "{namespace}" + // /namespace/$NAMESPACE/$RESOURCE/$NAME: replace $NAMESPACE with {namespace}, $NAME with {name} + if segments[index+3] != "finalize" && segments[index+3] != "status" { + // /$RESOURCE/$NAME/$SUBRESOURCE: replace $NAME with {name} + segments[index+3] = "{name}" + } + } + url.Path = path.Join(trimmedBasePath, path.Join(segments...)) + return *url +} + func (r *Request) tryThrottleWithInfo(ctx context.Context, retryInfo string) error { if r.rateLimiter == nil { return nil @@ -537,7 +615,7 @@ func (r *Request) tryThrottleWithInfo(ctx context.Context, retryInfo string) err // but we use a throttled logger to prevent spamming. globalThrottledLogger.Infof("%s", message) } - metrics.RateLimiterLatency.Observe(ctx, r.verb, *r.URL(), latency) + metrics.RateLimiterLatency.Observe(ctx, r.verb, r.finalURLTemplate(), latency) return err } @@ -826,7 +904,7 @@ func (r *Request) request(ctx context.Context, fn func(*http.Request, *http.Resp // Metrics for total request latency start := time.Now() defer func() { - metrics.RequestLatency.Observe(ctx, r.verb, *r.URL(), time.Since(start)) + metrics.RequestLatency.Observe(ctx, r.verb, r.finalURLTemplate(), time.Since(start)) }() if r.err != nil { diff --git a/rest/request_test.go b/rest/request_test.go index ddde5f767a..2773d6aa35 100644 --- a/rest/request_test.go +++ b/rest/request_test.go @@ -338,6 +338,206 @@ func TestResultIntoWithNoBodyReturnsErr(t *testing.T) { } } +func TestURLTemplate(t *testing.T) { + uri, _ := url.Parse("http://localhost/some/base/url/path") + uriSingleSlash, _ := url.Parse("http://localhost/") + testCases := []struct { + Request *Request + ExpectedFullURL string + ExpectedFinalURL string + }{ + { + // non dynamic client + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("POST"). + Prefix("api", "v1").Resource("r1").Namespace("ns").Name("nm").Param("p0", "v0"), + ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1/nm?p0=v0", + ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D?p0=%7Bvalue%7D", + }, + { + // non dynamic client with wrong api group + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("POST"). + Prefix("pre1", "v1").Resource("r1").Namespace("ns").Name("nm").Param("p0", "v0"), + ExpectedFullURL: "http://localhost/some/base/url/path/pre1/v1/namespaces/ns/r1/nm?p0=v0", + ExpectedFinalURL: "http://localhost/%7Bprefix%7D", + }, + { + // dynamic client with core group + namespace + resourceResource (with name) + // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/api/v1/namespaces/ns/r1/name1"), + ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1/name1", + ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D", + }, + { + // dynamic client with named group + namespace + resourceResource (with name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/g1/v1/namespaces/ns/r1/name1"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/ns/r1/name1", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D", + }, + { + // dynamic client with core group + namespace + resourceResource (with NO name) + // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/api/v1/namespaces/ns/r1"), + ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1", + ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1", + }, + { + // dynamic client with named group + namespace + resourceResource (with NO name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/g1/v1/namespaces/ns/r1"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/ns/r1", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/%7Bnamespace%7D/r1", + }, + { + // dynamic client with core group + resourceResource (with name) + // /api/$RESOURCEVERSION/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/api/v1/r1/name1"), + ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/r1/name1", + ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/r1/%7Bname%7D", + }, + { + // dynamic client with named group + resourceResource (with name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/g1/v1/r1/name1"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/r1/name1", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/r1/%7Bname%7D", + }, + { + // dynamic client with named group + namespace + resourceResource (with name) + subresource + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME/$SUBRESOURCE + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/%7Bname%7D/finalize", + }, + { + // dynamic client with named group + namespace + resourceResource (with name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/%7Bname%7D", + }, + { + // dynamic client with named group + namespace + resourceResource (with NO name) + subresource + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%SUBRESOURCE + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/finalize", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/finalize", + }, + { + // dynamic client with named group + namespace + resourceResource (with NO name) + subresource + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%SUBRESOURCE + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/status"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/status", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/status", + }, + { + // dynamic client with named group + namespace + resourceResource (with no name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces", + }, + { + // dynamic client with named group + resourceResource (with name) + subresource + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/finalize"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/finalize", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/finalize", + }, + { + // dynamic client with named group + resourceResource (with name) + subresource + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces/status"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/status", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/status", + }, + { + // dynamic client with named group + resourceResource (with name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces/namespaces"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D", + }, + { + // dynamic client with named group + resourceResource (with no name) + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/apis/namespaces/namespaces/namespaces"), + ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces", + ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces", + }, + { + // dynamic client with wrong api group + namespace + resourceResource (with name) + subresource + // /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME/$SUBRESOURCE + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/pre1/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"), + ExpectedFullURL: "http://localhost/some/base/url/path/pre1/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize", + ExpectedFinalURL: "http://localhost/%7Bprefix%7D", + }, + { + // dynamic client with core group + namespace + resourceResource (with name) where baseURL is a single / + // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/api/v1/namespaces/ns/r2/name1"), + ExpectedFullURL: "http://localhost/api/v1/namespaces/ns/r2/name1", + ExpectedFinalURL: "http://localhost/api/v1/namespaces/%7Bnamespace%7D/r2/%7Bname%7D", + }, + { + // dynamic client with core group + namespace + resourceResource (with name) where baseURL is 'some/base/url/path' + // /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME + Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/api/v1/namespaces/ns/r3/name1"), + ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r3/name1", + ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r3/%7Bname%7D", + }, + { + // dynamic client where baseURL is a single / + // / + Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/"), + ExpectedFullURL: "http://localhost/", + ExpectedFinalURL: "http://localhost/", + }, + { + // dynamic client where baseURL is a single / + // /version + Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE"). + Prefix("/version"), + ExpectedFullURL: "http://localhost/version", + ExpectedFinalURL: "http://localhost/version", + }, + } + for i, testCase := range testCases { + r := testCase.Request + full := r.URL() + if full.String() != testCase.ExpectedFullURL { + t.Errorf("%d: unexpected initial URL: %s %s", i, full, testCase.ExpectedFullURL) + } + actualURL := r.finalURLTemplate() + actual := actualURL.String() + if actual != testCase.ExpectedFinalURL { + t.Errorf("%d: unexpected URL template: %s %s", i, actual, testCase.ExpectedFinalURL) + } + if r.URL().String() != full.String() { + t.Errorf("%d, creating URL template changed request: %s -> %s", i, full.String(), r.URL().String()) + } + } +} + func TestTransformResponse(t *testing.T) { invalid := []byte("aaaaa") uri, _ := url.Parse("http://localhost")