Skip to content

Commit 1a46dfd

Browse files
aanmk8s-publishing-bot
aanm
authored and
k8s-publishing-bot
committed
Revert "client-go: remove no longer used finalURLTemplate"
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 <[email protected]> Kubernetes-commit: 11e16c63c87f07e9e68977c74b937989983754c1
1 parent b3e4a40 commit 1a46dfd

File tree

2 files changed

+280
-2
lines changed

2 files changed

+280
-2
lines changed

Diff for: ‎rest/request.go

+80-2
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,84 @@ func (r *Request) URL() *url.URL {
508508
return finalURL
509509
}
510510

511+
// finalURLTemplate is similar to URL(), but will make all specific parameter values equal
512+
// - instead of name or namespace, "{name}" and "{namespace}" will be used, and all query
513+
// parameters will be reset. This creates a copy of the url so as not to change the
514+
// underlying object.
515+
func (r Request) finalURLTemplate() url.URL {
516+
newParams := url.Values{}
517+
v := []string{"{value}"}
518+
for k := range r.params {
519+
newParams[k] = v
520+
}
521+
r.params = newParams
522+
url := r.URL()
523+
524+
segments := strings.Split(url.Path, "/")
525+
groupIndex := 0
526+
index := 0
527+
trimmedBasePath := ""
528+
if url != nil && r.c.base != nil && strings.Contains(url.Path, r.c.base.Path) {
529+
p := strings.TrimPrefix(url.Path, r.c.base.Path)
530+
if !strings.HasPrefix(p, "/") {
531+
p = "/" + p
532+
}
533+
// store the base path that we have trimmed so we can append it
534+
// before returning the URL
535+
trimmedBasePath = r.c.base.Path
536+
segments = strings.Split(p, "/")
537+
groupIndex = 1
538+
}
539+
if len(segments) <= 2 {
540+
return *url
541+
}
542+
543+
const CoreGroupPrefix = "api"
544+
const NamedGroupPrefix = "apis"
545+
isCoreGroup := segments[groupIndex] == CoreGroupPrefix
546+
isNamedGroup := segments[groupIndex] == NamedGroupPrefix
547+
if isCoreGroup {
548+
// checking the case of core group with /api/v1/... format
549+
index = groupIndex + 2
550+
} else if isNamedGroup {
551+
// checking the case of named group with /apis/apps/v1/... format
552+
index = groupIndex + 3
553+
} else {
554+
// this should not happen that the only two possibilities are /api... and /apis..., just want to put an
555+
// outlet here in case more API groups are added in future if ever possible:
556+
// https://kubernetes.io/docs/concepts/overview/kubernetes-api/#api-groups
557+
// if a wrong API groups name is encountered, return the {prefix} for url.Path
558+
url.Path = "/{prefix}"
559+
url.RawQuery = ""
560+
return *url
561+
}
562+
//switch segLength := len(segments) - index; segLength {
563+
switch {
564+
// case len(segments) - index == 1:
565+
// resource (with no name) do nothing
566+
case len(segments)-index == 2:
567+
// /$RESOURCE/$NAME: replace $NAME with {name}
568+
segments[index+1] = "{name}"
569+
case len(segments)-index == 3:
570+
if segments[index+2] == "finalize" || segments[index+2] == "status" {
571+
// /$RESOURCE/$NAME/$SUBRESOURCE: replace $NAME with {name}
572+
segments[index+1] = "{name}"
573+
} else {
574+
// /namespace/$NAMESPACE/$RESOURCE: replace $NAMESPACE with {namespace}
575+
segments[index+1] = "{namespace}"
576+
}
577+
case len(segments)-index >= 4:
578+
segments[index+1] = "{namespace}"
579+
// /namespace/$NAMESPACE/$RESOURCE/$NAME: replace $NAMESPACE with {namespace}, $NAME with {name}
580+
if segments[index+3] != "finalize" && segments[index+3] != "status" {
581+
// /$RESOURCE/$NAME/$SUBRESOURCE: replace $NAME with {name}
582+
segments[index+3] = "{name}"
583+
}
584+
}
585+
url.Path = path.Join(trimmedBasePath, path.Join(segments...))
586+
return *url
587+
}
588+
511589
func (r *Request) tryThrottleWithInfo(ctx context.Context, retryInfo string) error {
512590
if r.rateLimiter == nil {
513591
return nil
@@ -537,7 +615,7 @@ func (r *Request) tryThrottleWithInfo(ctx context.Context, retryInfo string) err
537615
// but we use a throttled logger to prevent spamming.
538616
globalThrottledLogger.Infof("%s", message)
539617
}
540-
metrics.RateLimiterLatency.Observe(ctx, r.verb, *r.URL(), latency)
618+
metrics.RateLimiterLatency.Observe(ctx, r.verb, r.finalURLTemplate(), latency)
541619

542620
return err
543621
}
@@ -826,7 +904,7 @@ func (r *Request) request(ctx context.Context, fn func(*http.Request, *http.Resp
826904
// Metrics for total request latency
827905
start := time.Now()
828906
defer func() {
829-
metrics.RequestLatency.Observe(ctx, r.verb, *r.URL(), time.Since(start))
907+
metrics.RequestLatency.Observe(ctx, r.verb, r.finalURLTemplate(), time.Since(start))
830908
}()
831909

832910
if r.err != nil {

Diff for: ‎rest/request_test.go

+200
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,206 @@ func TestResultIntoWithNoBodyReturnsErr(t *testing.T) {
338338
}
339339
}
340340

341+
func TestURLTemplate(t *testing.T) {
342+
uri, _ := url.Parse("http://localhost/some/base/url/path")
343+
uriSingleSlash, _ := url.Parse("http://localhost/")
344+
testCases := []struct {
345+
Request *Request
346+
ExpectedFullURL string
347+
ExpectedFinalURL string
348+
}{
349+
{
350+
// non dynamic client
351+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("POST").
352+
Prefix("api", "v1").Resource("r1").Namespace("ns").Name("nm").Param("p0", "v0"),
353+
ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1/nm?p0=v0",
354+
ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D?p0=%7Bvalue%7D",
355+
},
356+
{
357+
// non dynamic client with wrong api group
358+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("POST").
359+
Prefix("pre1", "v1").Resource("r1").Namespace("ns").Name("nm").Param("p0", "v0"),
360+
ExpectedFullURL: "http://localhost/some/base/url/path/pre1/v1/namespaces/ns/r1/nm?p0=v0",
361+
ExpectedFinalURL: "http://localhost/%7Bprefix%7D",
362+
},
363+
{
364+
// dynamic client with core group + namespace + resourceResource (with name)
365+
// /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
366+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
367+
Prefix("/api/v1/namespaces/ns/r1/name1"),
368+
ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1/name1",
369+
ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D",
370+
},
371+
{
372+
// dynamic client with named group + namespace + resourceResource (with name)
373+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
374+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
375+
Prefix("/apis/g1/v1/namespaces/ns/r1/name1"),
376+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/ns/r1/name1",
377+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/%7Bnamespace%7D/r1/%7Bname%7D",
378+
},
379+
{
380+
// dynamic client with core group + namespace + resourceResource (with NO name)
381+
// /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE
382+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
383+
Prefix("/api/v1/namespaces/ns/r1"),
384+
ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r1",
385+
ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r1",
386+
},
387+
{
388+
// dynamic client with named group + namespace + resourceResource (with NO name)
389+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE
390+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
391+
Prefix("/apis/g1/v1/namespaces/ns/r1"),
392+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/ns/r1",
393+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/namespaces/%7Bnamespace%7D/r1",
394+
},
395+
{
396+
// dynamic client with core group + resourceResource (with name)
397+
// /api/$RESOURCEVERSION/$RESOURCE/%NAME
398+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
399+
Prefix("/api/v1/r1/name1"),
400+
ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/r1/name1",
401+
ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/r1/%7Bname%7D",
402+
},
403+
{
404+
// dynamic client with named group + resourceResource (with name)
405+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME
406+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
407+
Prefix("/apis/g1/v1/r1/name1"),
408+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/g1/v1/r1/name1",
409+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/g1/v1/r1/%7Bname%7D",
410+
},
411+
{
412+
// dynamic client with named group + namespace + resourceResource (with name) + subresource
413+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME/$SUBRESOURCE
414+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
415+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"),
416+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize",
417+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/%7Bname%7D/finalize",
418+
},
419+
{
420+
// dynamic client with named group + namespace + resourceResource (with name)
421+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
422+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
423+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces"),
424+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces",
425+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/%7Bname%7D",
426+
},
427+
{
428+
// dynamic client with named group + namespace + resourceResource (with NO name) + subresource
429+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%SUBRESOURCE
430+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
431+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"),
432+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/finalize",
433+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/finalize",
434+
},
435+
{
436+
// dynamic client with named group + namespace + resourceResource (with NO name) + subresource
437+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%SUBRESOURCE
438+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
439+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces/status"),
440+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces/status",
441+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces/status",
442+
},
443+
{
444+
// dynamic client with named group + namespace + resourceResource (with no name)
445+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
446+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
447+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/namespaces"),
448+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/namespaces",
449+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bnamespace%7D/namespaces",
450+
},
451+
{
452+
// dynamic client with named group + resourceResource (with name) + subresource
453+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
454+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
455+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/finalize"),
456+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/finalize",
457+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/finalize",
458+
},
459+
{
460+
// dynamic client with named group + resourceResource (with name) + subresource
461+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
462+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
463+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces/status"),
464+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces/status",
465+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D/status",
466+
},
467+
{
468+
// dynamic client with named group + resourceResource (with name)
469+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME
470+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
471+
Prefix("/apis/namespaces/namespaces/namespaces/namespaces"),
472+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/namespaces",
473+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces/%7Bname%7D",
474+
},
475+
{
476+
// dynamic client with named group + resourceResource (with no name)
477+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/$RESOURCE/%NAME
478+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
479+
Prefix("/apis/namespaces/namespaces/namespaces"),
480+
ExpectedFullURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces",
481+
ExpectedFinalURL: "http://localhost/some/base/url/path/apis/namespaces/namespaces/namespaces",
482+
},
483+
{
484+
// dynamic client with wrong api group + namespace + resourceResource (with name) + subresource
485+
// /apis/$NAMEDGROUPNAME/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME/$SUBRESOURCE
486+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
487+
Prefix("/pre1/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize"),
488+
ExpectedFullURL: "http://localhost/some/base/url/path/pre1/namespaces/namespaces/namespaces/namespaces/namespaces/namespaces/finalize",
489+
ExpectedFinalURL: "http://localhost/%7Bprefix%7D",
490+
},
491+
{
492+
// dynamic client with core group + namespace + resourceResource (with name) where baseURL is a single /
493+
// /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
494+
Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
495+
Prefix("/api/v1/namespaces/ns/r2/name1"),
496+
ExpectedFullURL: "http://localhost/api/v1/namespaces/ns/r2/name1",
497+
ExpectedFinalURL: "http://localhost/api/v1/namespaces/%7Bnamespace%7D/r2/%7Bname%7D",
498+
},
499+
{
500+
// dynamic client with core group + namespace + resourceResource (with name) where baseURL is 'some/base/url/path'
501+
// /api/$RESOURCEVERSION/namespaces/$NAMESPACE/$RESOURCE/%NAME
502+
Request: NewRequestWithClient(uri, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
503+
Prefix("/api/v1/namespaces/ns/r3/name1"),
504+
ExpectedFullURL: "http://localhost/some/base/url/path/api/v1/namespaces/ns/r3/name1",
505+
ExpectedFinalURL: "http://localhost/some/base/url/path/api/v1/namespaces/%7Bnamespace%7D/r3/%7Bname%7D",
506+
},
507+
{
508+
// dynamic client where baseURL is a single /
509+
// /
510+
Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
511+
Prefix("/"),
512+
ExpectedFullURL: "http://localhost/",
513+
ExpectedFinalURL: "http://localhost/",
514+
},
515+
{
516+
// dynamic client where baseURL is a single /
517+
// /version
518+
Request: NewRequestWithClient(uriSingleSlash, "", ClientContentConfig{GroupVersion: schema.GroupVersion{Group: "test"}}, nil).Verb("DELETE").
519+
Prefix("/version"),
520+
ExpectedFullURL: "http://localhost/version",
521+
ExpectedFinalURL: "http://localhost/version",
522+
},
523+
}
524+
for i, testCase := range testCases {
525+
r := testCase.Request
526+
full := r.URL()
527+
if full.String() != testCase.ExpectedFullURL {
528+
t.Errorf("%d: unexpected initial URL: %s %s", i, full, testCase.ExpectedFullURL)
529+
}
530+
actualURL := r.finalURLTemplate()
531+
actual := actualURL.String()
532+
if actual != testCase.ExpectedFinalURL {
533+
t.Errorf("%d: unexpected URL template: %s %s", i, actual, testCase.ExpectedFinalURL)
534+
}
535+
if r.URL().String() != full.String() {
536+
t.Errorf("%d, creating URL template changed request: %s -> %s", i, full.String(), r.URL().String())
537+
}
538+
}
539+
}
540+
341541
func TestTransformResponse(t *testing.T) {
342542
invalid := []byte("aaaaa")
343543
uri, _ := url.Parse("http://localhost")

0 commit comments

Comments
 (0)