From 83bb1e3ff26c9b30cc3050049f54dafc27e132c4 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 17 Feb 2022 16:51:55 +0100 Subject: [PATCH] client-go: remove no longer used finalURLTemplate The restclient metrics were updated to track only the host field of the url, the finalURLTemplate is not longer needed, its only goal was to replace name and namespace in the path to avoid cardinality. Kubernetes-commit: bebf5a608f68523fc430a44f6db26b16022dc862 --- rest/request.go | 82 +----------------- rest/request_test.go | 200 ------------------------------------------- 2 files changed, 2 insertions(+), 280 deletions(-) diff --git a/rest/request.go b/rest/request.go index 5cc9900b..93b6e057 100644 --- a/rest/request.go +++ b/rest/request.go @@ -496,84 +496,6 @@ 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 @@ -601,7 +523,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.finalURLTemplate(), latency) + metrics.RateLimiterLatency.Observe(ctx, r.verb, *r.URL(), latency) return err } @@ -929,7 +851,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.finalURLTemplate(), time.Since(start)) + metrics.RequestLatency.Observe(ctx, r.verb, *r.URL(), time.Since(start)) }() if r.err != nil { diff --git a/rest/request_test.go b/rest/request_test.go index 8e9bb92b..f2130cbf 100644 --- a/rest/request_test.go +++ b/rest/request_test.go @@ -337,206 +337,6 @@ 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")