From 5603652d33f3a0390dc3a522c0492e540f49aab6 Mon Sep 17 00:00:00 2001 From: Todd Date: Thu, 21 Apr 2022 14:29:31 -0700 Subject: [PATCH] Make the {id} regexp less strict for identifying the "path" label value. (#2034) Previously we were matching what boundary considers an id, but that is determined after requests are routed by grpc-gateway. This changes the regexp to match any path segment to try to behave the same way that grpc-gateway would when assigning values to the {id} variable declared in the protobuf. --- .../servers/controller/internal/metric/api.go | 7 +++-- .../controller/internal/metric/api_test.go | 29 ++++++++++++++----- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/internal/servers/controller/internal/metric/api.go b/internal/servers/controller/internal/metric/api.go index 7c2a4f5c2..06f542b52 100644 --- a/internal/servers/controller/internal/metric/api.go +++ b/internal/servers/controller/internal/metric/api.go @@ -91,9 +91,10 @@ func apiPathsAndMethods() map[string][]string { } func buildRegexFromPath(p string) *regexp.Regexp { - // a public id in boundary consists of a some prefix, an underscore and - // at least 10 alphanumerical characters like "h_1234567890". - const idRegexp = "[[:alnum:]]{1,}_[[:alnum:]]{10,}" + // We only care about how grpc-gateway will route to specific handlers. + // As long as there is at least 1 character that is part of a path segment + // (not a '/', '?', or ':' we have identified an id for the sake of routing. + const idRegexp = "[^\\/\\?\\:]+" // Replace any tag in the form of {id} or {auth_method_id} with the above // regex so we can match paths to that when measuring requests. diff --git a/internal/servers/controller/internal/metric/api_test.go b/internal/servers/controller/internal/metric/api_test.go index 84ca2a9a0..bbb94f518 100644 --- a/internal/servers/controller/internal/metric/api_test.go +++ b/internal/servers/controller/internal/metric/api_test.go @@ -20,13 +20,15 @@ func TestBuildRegexFromPath(t *testing.T) { match: []string{ "/v1/pathsomething/a4s_aKsdFh723018djsa:test-thing", "/v1/pathsomething/h_1234567890:test-thing", - }, - dont: []string{ "/v1/pathsomething/{id}:test-thing", "/v1/pathsomething/{any_old_id}:test-thing", - "/v1/pathsomething:test-thing", "/v1/pathsomething/not-an-id:test-thing", + }, + dont: []string{ "/v1/pathsomething/a4s_aKsdFh723018djsa:other-thing", + "/v1/pathsomething:test-thing", + "/v1/pathsomething/:test-thing:test-thing", + "/v1/pathsomething/:other-thing", }, }, { @@ -34,11 +36,16 @@ func TestBuildRegexFromPath(t *testing.T) { match: []string{ "/v1/pathsomething/a4s_aKsdFh723018djsa:authenticate", "/v1/pathsomething/am_1234567890:authenticate", - }, - dont: []string{ "/v1/pathsomething/{id}:authenticate", "/v1/pathsomething/{auth_method}:authenticate", }, + dont: []string{ + "/v1/pathsomething:authenticate", + "/v1/pathsomething:authenticate:authenticate", + "/v1/pathsomething/:authenticate:authenticate", + "/v1/pathsomething/am_1234567890/:authenticate", + "/v1/pathsomething/?whatabout=:authenticate", + }, }, { path: "/v1/pathsomething/{non_id_tag}:test", @@ -66,10 +73,10 @@ func TestBuildRegexFromPath(t *testing.T) { t.Run(tc.path, func(t *testing.T) { r := buildRegexFromPath(tc.path) for _, m := range tc.match { - assert.True(t, r.Match([]byte(m))) + assert.True(t, r.Match([]byte(m)), "Couldn't match %q", m) } for _, d := range tc.dont { - assert.False(t, r.Match([]byte(d))) + assert.False(t, r.Match([]byte(d)), "Matched %q", d) } }) } @@ -100,6 +107,14 @@ func TestPathLabel(t *testing.T) { in: "/v1/accounts/a_1234567890", want: "/v1/accounts/{id}", }, + { + in: "/v1/accounts/mistake", + want: "/v1/accounts/{id}", + }, + { + in: "/v1/accounts/mistyped_id", + want: "/v1/accounts/{id}", + }, { in: "v1/accounts/a_1234567890", want: "/v1/accounts/{id}",