From adbe41698d71bcbe3f24d214bd8a48472b5edd7e Mon Sep 17 00:00:00 2001 From: Maxim Date: Wed, 13 Feb 2019 15:13:13 +0100 Subject: [PATCH] API usage monitoring filter improvements (#950) * feat(api-usage-monitoring-filter): use {no-match} for not matched paths (instead of ) (#949) Signed-off-by: Maxim Tschumak * feat(api-usage-monitoring-filter): fail (not create a filter) on invalid config (#949) Signed-off-by: Maxim Tschumak * feat(api-usage-monitoring-filter): path parameters in curly braces (#949) Signed-off-by: Maxim Tschumak * feat(api-usage-monitoring-filter): realms via global config and client pattern on filter level (incompatible interface change) (#949) Signed-off-by: Maxim Tschumak * feat(api-usage-monitoring-filter): improvements proposed by @tkrop (#949) Signed-off-by: Maxim Tschumak * feat(api-usage-monitoring-filter): use regex for realms to track (#949) Signed-off-by: Maxim Tschumak --- cmd/skipper/main.go | 11 +- docs/reference/filters.md | 39 ++-- filters/apiusagemonitoring/doc.go | 4 +- filters/apiusagemonitoring/filter.go | 43 ++--- .../filter_clientmetrics_test.go | 69 ++++--- .../filter_endpointmetrics_test.go | 68 ++----- filters/apiusagemonitoring/pathinfo.go | 5 +- filters/apiusagemonitoring/spec.go | 110 ++++++----- filters/apiusagemonitoring/spec_test.go | 176 +++++++++++------- filters/apiusagemonitoring/testutils_test.go | 2 +- skipper.go | 11 +- 11 files changed, 286 insertions(+), 252 deletions(-) diff --git a/cmd/skipper/main.go b/cmd/skipper/main.go index 317c800975..f36d7c7b0e 100644 --- a/cmd/skipper/main.go +++ b/cmd/skipper/main.go @@ -73,6 +73,7 @@ const ( defaultApiUsageMonitoringRealmKeys = "" defaultApiUsageMonitoringClientKeys = "sub" defaultApiUsageMonitoringDefaultClientTrackingPattern = "" + defaultApiUsageMonitoringRealmsTrackingPattern = "services" // generic: addressUsage = "network address that skipper should listen on" @@ -168,7 +169,8 @@ const ( apiUsageMonitoringEnableUsage = "enables the apiUsageMonitoring filter" apiUsageMonitoringRealmKeysUsage = "name of the property in the JWT payload that contains the authority realm" apiUsageMonitoringClientKeysUsage = "comma separated list of names of the properties in the JWT body that contains the client ID" - apiUsageMonitoringDefaultClientTrackingPatternUsage = "regular expression to default to when API usage monitoring filter configuration does not provide `client_tracking_pattern`" + apiUsageMonitoringDefaultClientTrackingPatternUsage = "*Deprecated*: set `client_tracking_pattern` directly on filter" + apiUsageMonitoringRealmsTrackingPatternUsage = "regular expression used for matching monitored realms (defaults is 'services')" // connections, timeouts: idleConnsPerHostUsage = "maximum idle connections per backend host" @@ -310,6 +312,7 @@ var ( apiUsageMonitoringRealmKeys string apiUsageMonitoringClientKeys string apiUsageMonitoringDefaultClientTrackingPattern string + apiUsageMonitoringRealmsTrackingPattern string // connections, timeouts: idleConnsPerHost int @@ -449,6 +452,7 @@ func init() { flag.StringVar(&apiUsageMonitoringRealmKeys, "api-usage-monitoring-realm-keys", defaultApiUsageMonitoringRealmKeys, apiUsageMonitoringRealmKeysUsage) flag.StringVar(&apiUsageMonitoringClientKeys, "api-usage-monitoring-client-keys", defaultApiUsageMonitoringClientKeys, apiUsageMonitoringClientKeysUsage) flag.StringVar(&apiUsageMonitoringDefaultClientTrackingPattern, "api-usage-monitoring-default-client-tracking-pattern", defaultApiUsageMonitoringDefaultClientTrackingPattern, apiUsageMonitoringDefaultClientTrackingPatternUsage) + flag.StringVar(&apiUsageMonitoringRealmsTrackingPattern, "api-usage-monitoring-realms-tracking-pattern", defaultApiUsageMonitoringRealmsTrackingPattern, apiUsageMonitoringRealmsTrackingPatternUsage) // connections, timeouts: flag.IntVar(&idleConnsPerHost, "idle-conns-num", proxy.DefaultIdleConnsPerHost, idleConnsPerHostUsage) @@ -650,9 +654,10 @@ func main() { // API Monitoring: ApiUsageMonitoringEnable: apiUsageMonitoringEnable, - ApiUsageMonitoringRealmKey: apiUsageMonitoringRealmKeys, - ApiUsageMonitoringClientIdKeyName: apiUsageMonitoringClientKeys, + ApiUsageMonitoringRealmKeys: apiUsageMonitoringRealmKeys, + ApiUsageMonitoringClientKeys: apiUsageMonitoringClientKeys, ApiUsageMonitoringDefaultClientTrackingPattern: apiUsageMonitoringDefaultClientTrackingPattern, + ApiUsageMonitoringRealmsTrackingPattern: apiUsageMonitoringRealmsTrackingPattern, // Auth: OAuthUrl: oauthURL, diff --git a/docs/reference/filters.md b/docs/reference/filters.md index 9a1183a3a3..5b7a8ee7ed 100644 --- a/docs/reference/filters.md +++ b/docs/reference/filters.md @@ -1050,7 +1050,7 @@ For the client based metrics, additional flags need to be specified. |--------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `api-usage-monitoring-realm-keys` | Name of the property in the JWT JSON body that contains the name of the _realm_. | | `api-usage-monitoring-client-keys` | Name of the property in the JWT JSON body that contains the name of the _client_. | -| `api-usage-monitoring-default-client-tracking-pattern` | Optional. Default is empty. Allows to deploy Skipper with a default client tracking pattern. When `apiUsageMonitoring` configuration does not specify one, this default is used instead of disabling the client metrics. | +| `api-usage-monitoring-realms-tracking-pattern` | RegEx of _realms_ to be monitored. Defaults to 'services'. | NOTE: Make sure to activate the metrics flavour proper to your environment using the `metrics-flavour` flag in order to get those metrics. @@ -1058,7 +1058,7 @@ flag in order to get those metrics. Example: ```bash -skipper -metrics-flavour prometheus -enable-api-usage-monitoring -api-usage-monitoring-realm-keys="realm" -api-usage-monitoring-client-keys="managed-id" +skipper -metrics-flavour prometheus -enable-api-usage-monitoring -api-usage-monitoring-realm-keys="realm" -api-usage-monitoring-client-keys="managed-id" api-usage-monitoring-realms-tracking-pattern="services,users" ``` The structure of the metrics is all of those elements, separated by `.` dots: @@ -1085,7 +1085,7 @@ Example: ``` + Realm | -apiUsageMonitoring.custom.orders-backend.orders-api.GET.foo/orders/:order-id.*.*.http_count +apiUsageMonitoring.custom.orders-backend.orders-api.GET.foo/orders/{order-id}.*.*.http_count | | | + Metric Name + Client @@ -1155,6 +1155,7 @@ api-usage-monitoring-configuration: path_templates: description: Endpoints to be monitored. type: array + minLength: 1 items: type: string description: > @@ -1163,23 +1164,19 @@ api-usage-monitoring-configuration: example: /orders/{order-id} client_tracking_pattern: description: > - The pattern that the combination `realm.client` must match in order for the client - based metrics to be tracked, in form of a regular expression. + The pattern that matches client id in form of a regular expression. - By default (if undefined), it is set to `services\\..*`. + By default (if undefined), it is set to `.*`. An empty string disables the client metrics completely. - - IMPORTANT: Avoid patterns that would match too many different values like `.*` or `users\\..*`. Too - many different metric keys would badly affect the performances of the metric systems (e.g.: Prometheus). type: string examples: all_services: - summary: All services are tracked (clients of the realm `services`). - value: "services\\..*" + summary: All services are tracked (for all activated realms). + value: ".*" just_some_services: - summary: Only services `orders` and `shipment` are tracked. - value: "services\\.(orders|shipment)" + summary: Only services `orders-service` and `shipment-service` are tracked. + value: "(orders\-service|shipment\-service)" ``` Configuration Example: @@ -1194,7 +1191,7 @@ apiUsageMonitoring(` "foo/orders/:order-id", "foo/orders/:order-id/order_item/{order-item-id}" ], - "client_tracking_pattern": "users\\.(joe|sabine)" + "client_tracking_pattern": "(shipping\-service|payment\-service)" }`,`{ "application_id": "my-app", "api_id": "customers-api", @@ -1209,13 +1206,13 @@ apiUsageMonitoring(` Based on the previous configuration, here is an example of a counter metric. ``` -apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/:order-id.*.*.http_count +apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/{order-id}.*.*.http_count ``` Here is the _Prometheus_ query to obtain it. ``` -sum(rate(skipper_custom_total{key="apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/:order-id.*.*.http_count"}[60s])) by (key) +sum(rate(skipper_custom_total{key="apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/{order-id}.*.*.http_count"}[60s])) by (key) ``` Here is an example of a histogram metric. @@ -1230,17 +1227,13 @@ Here is the _Prometheus_ query to obtain it. histogram_quantile(0.5, sum(rate(skipper_custom_duration_seconds_bucket{key="apiUsageMonitoring.custom.my-app.orders-api.POST.foo/orders.*.*.latency"}[60s])) by (le, key)) ``` -NOTE: Non configured paths will be tracked with `` application ID, API ID +NOTE: Non configured paths will be tracked with `{unknown}` application ID, API ID and path template. -``` -apiUsageMonitoring.custom...GET..*.*.http_count -``` - However, if all `application_id`s of your configuration refer to the same application, the filter assume that also non configured paths will be directed to this application. E.g.: ``` -apiUsageMonitoring.custom.my-app..GET..*.*.http_count -``` +apiUsageMonitoring.custom.my-app.{unknown}.GET.{no-match}.*.*.http_count +``` \ No newline at end of file diff --git a/filters/apiusagemonitoring/doc.go b/filters/apiusagemonitoring/doc.go index aca8ed8ac2..03063a0ee0 100644 --- a/filters/apiusagemonitoring/doc.go +++ b/filters/apiusagemonitoring/doc.go @@ -35,8 +35,8 @@ Command line example for executing locally: -routes-file "$HOME/temp/test.eskip" \ -enable-api-usage-monitoring \ -api-usage-monitoring-realm-keys="https://identity.zalando.com/realm" \ - -api-usage-monitoring-client-keys="https://identity.zalando.com/managed-id" \ - -api-usage-monitoring-default-client-tracking-pattern="services\\..*" \ + -api-usage-monitoring-client-keys="https://identity.zalando.com/managed-id,sub" \ + -api-usage-monitoring-realmsTrackingPattern-tracking-pattern="services" \ -metrics-flavour prometheus \ -histogram-metric-buckets=".01,.025,.05,.075,.1,.2,.3,.4,.5,.75,1,2,3,4,5,7,10,15,20,30,60,120,300,600" \ -application-log-level=DEBUG diff --git a/filters/apiusagemonitoring/filter.go b/filters/apiusagemonitoring/filter.go index a3483ce1c3..b885dbcaae 100644 --- a/filters/apiusagemonitoring/filter.go +++ b/filters/apiusagemonitoring/filter.go @@ -104,45 +104,40 @@ func (f *apiUsageMonitoringFilter) getClientMetricsNames(realmClientKey string, return prefixes } -// getRealmClientKey generates the proper . part of the -// client metrics name. -func (f *apiUsageMonitoringFilter) getRealmClientKey(r *http.Request, path *pathInfo) string { - const ( - unknownRealmClient = unknownElementPlaceholder + "." + unknownElementPlaceholder - unknownClientAfterKnownRealm = "." + unknownElementPlaceholder - ) +const unknownUnknown = unknownPlaceholder + "." + unknownPlaceholder - // no JWT ==> . +// getRealmClientKey generates the proper . part of the client metrics name. +func (f *apiUsageMonitoringFilter) getRealmClientKey(r *http.Request, path *pathInfo) string { + // no JWT ==> {unknown}.{unknown} jwt := parseJwtBody(r) if jwt == nil { - return unknownRealmClient + return unknownUnknown } - // no realm in JWT ==> . + // no realm in JWT ==> {unknown}.{unknown} realm, ok := jwt.getOneOfString(f.Spec.realmKeys) if !ok { - return unknownRealmClient + return unknownUnknown } - // no matcher ==> realm. - if path.ClientTracking.ClientTrackingMatcher == nil { - return realm + unknownClientAfterKnownRealm + // realm is not one of the realmsTrackingPattern to be tracked ==> realm.{all} + if !path.ClientTracking.RealmsTrackingMatcher.MatchString(realm) { + return realm + ".{all}" } - // no client in JWT ==> realm. + // no client in JWT ==> realm.{unknown} client, ok := jwt.getOneOfString(f.Spec.clientKeys) if !ok { - return realm + unknownClientAfterKnownRealm + return realm + "." + unknownPlaceholder } - // if `realm.client` does not match ==> realm. - realmAndClient := realm + "." + client - if !path.ClientTracking.ClientTrackingMatcher.MatchString(realmAndClient) { - return realm + unknownClientAfterKnownRealm + // if client does not match ==> realm.{no-match} + if !path.ClientTracking.ClientTrackingMatcher.MatchString(client) { + return realm + "." + noMatchPlaceholder } - // all matched ==> realm.client - return realmAndClient + // all matched ==> realm.client + return realm + "." + client } // resolvePath tries to match the request's path with one of the configured path template. @@ -163,7 +158,7 @@ func getEndpointMetricsNames(req *http.Request, path *pathInfo) *endpointMetricN methodIndex, ok := methodToIndex[method] if !ok { methodIndex = methodIndexUnknown - method = unknownElementPlaceholder + method = unknownPlaceholder } if p := path.metricPrefixesPerMethod[methodIndex]; p != nil { @@ -174,7 +169,7 @@ func getEndpointMetricsNames(req *http.Request, path *pathInfo) *endpointMetricN // createAndCacheMetricsNames generates metrics names and cache them. func createAndCacheMetricsNames(path *pathInfo, method string, methodIndex int) *endpointMetricNames { - endpointPrefix := path.CommonPrefix + method + "." + path.PathTemplate + ".*.*." + endpointPrefix := path.CommonPrefix + method + "." + path.PathLabel + ".*.*." prefixes := &endpointMetricNames{ endpointPrefix: endpointPrefix, countAll: endpointPrefix + metricCountAll, diff --git a/filters/apiusagemonitoring/filter_clientmetrics_test.go b/filters/apiusagemonitoring/filter_clientmetrics_test.go index d33b0ea089..9e242167c2 100644 --- a/filters/apiusagemonitoring/filter_clientmetrics_test.go +++ b/filters/apiusagemonitoring/filter_clientmetrics_test.go @@ -15,11 +15,11 @@ type clientMetricsTest struct { expectedEndpointMetricPrefix string expectedClientMetricPrefix string - defaultClientTrackingPattern string + realmsTrackingPattern string } var ( - clientTrackingPatternJustSomeUsers = s(`users\.(joe|sabine)`) + clientTrackingPatternJustSomeUsers = s(`(joe|sabine)`) headerUsersJoe = http.Header{ authorizationHeaderName: { "Bearer " + buildFakeJwtWithBody(map[string]interface{}{ @@ -38,6 +38,7 @@ func Test_Filter_ClientMetrics_ClientTrackingPatternDoesNotCompile(t *testing.T) testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "service", clientTrackingPattern: s("(["), header: headerUsersJoe, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", @@ -50,10 +51,11 @@ func Test_Filter_ClientMetrics_NoMatchingPath_RealmIsKnown(t *testing.T) { realmKeyName: "realm", clientKeyName: "client", clientTrackingPattern: s(".*"), + realmsTrackingPattern: "services", header: headerUsersJoe, url: "https://www.example.com/non/configured/path/template", - expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app..GET..*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app..*.*.users..", + expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.{unknown}.GET.{no-match}.*.*.", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.{unknown}.*.*.users.{all}.", }) } @@ -64,8 +66,8 @@ func Test_Filter_ClientMetrics_NoMatchingPath_RealmIsUnknown(t *testing.T) { clientTrackingPattern: s(".*"), header: headerUsersJoe, url: "https://www.example.com/non/configured/path/template", - expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app..GET..*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app..*.*...", + expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.{unknown}.GET.{no-match}.*.*.", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.{unknown}.*.*.{unknown}.{unknown}.", }) } @@ -77,6 +79,7 @@ func Test_Filter_ClientMetrics_MatchAll(t *testing.T) { header: headerUsersJoe, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users.joe.", + realmsTrackingPattern: "users", }) } @@ -95,6 +98,7 @@ func Test_Filter_ClientMetrics_MatchOneOfClientKeyName(t *testing.T) { }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.services.payments.", + realmsTrackingPattern: "services", }) } @@ -102,6 +106,7 @@ func Test_Filter_ClientMetrics_MatchOneOfClientKeyName_UseFirstMatching(t *testi testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client,sub", + realmsTrackingPattern: "services", clientTrackingPattern: s(".*"), header: http.Header{ authorizationHeaderName: { @@ -121,6 +126,7 @@ func Test_Filter_ClientMetrics_Realm1User1(t *testing.T) { testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "users", clientTrackingPattern: clientTrackingPatternJustSomeUsers, header: headerUsersJoe, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", @@ -132,6 +138,7 @@ func Test_Filter_ClientMetrics_Realm1User0(t *testing.T) { testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "users", clientTrackingPattern: clientTrackingPatternJustSomeUsers, header: http.Header{ authorizationHeaderName: { @@ -142,7 +149,7 @@ func Test_Filter_ClientMetrics_Realm1User0(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users..", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users.{no-match}.", }) } @@ -150,6 +157,7 @@ func Test_Filter_ClientMetrics_Realm0User1(t *testing.T) { testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "services", clientTrackingPattern: clientTrackingPatternJustSomeUsers, header: http.Header{ authorizationHeaderName: { @@ -160,7 +168,7 @@ func Test_Filter_ClientMetrics_Realm0User1(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.nobodies..", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.nobodies.{all}.", }) } @@ -168,6 +176,7 @@ func Test_Filter_ClientMetrics_Realm0User0(t *testing.T) { testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "services", clientTrackingPattern: clientTrackingPatternJustSomeUsers, header: http.Header{ authorizationHeaderName: { @@ -178,7 +187,7 @@ func Test_Filter_ClientMetrics_Realm0User0(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.nobodies..", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.nobodies.{all}.", }) } @@ -186,6 +195,7 @@ func Test_Filter_ClientMetrics_AuthDoesNotHaveBearerPrefix(t *testing.T) { testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "services", clientTrackingPattern: clientTrackingPatternJustSomeUsers, header: http.Header{ authorizationHeaderName: { @@ -196,7 +206,7 @@ func Test_Filter_ClientMetrics_AuthDoesNotHaveBearerPrefix(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*...", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.{unknown}.{unknown}.", }) } @@ -209,7 +219,7 @@ func Test_Filter_ClientMetrics_NoAuthHeader(t *testing.T) { /* no Authorization header */ }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*...", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.{unknown}.{unknown}.", }) } @@ -224,7 +234,7 @@ func Test_Filter_ClientMetrics_JWTIsNot3DotSeparatedString(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*...", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.{unknown}.{unknown}.", }) } @@ -239,7 +249,7 @@ func Test_Filter_ClientMetrics_JWTIsNotBase64Encoded(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*...", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.{unknown}.{unknown}.", }) } @@ -255,7 +265,7 @@ func Test_Filter_ClientMetrics_JWTBodyIsNoJSON(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*...", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.{unknown}.{unknown}.", }) } @@ -273,7 +283,7 @@ func Test_Filter_ClientMetrics_JWTBodyHasNoRealm(t *testing.T) { }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*...", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.{unknown}.{unknown}.", }) } @@ -281,6 +291,7 @@ func Test_Filter_ClientMetrics_JWTBodyHasNoClient_ShouldTrackRealm(t *testing.T) testClientMetrics(t, clientMetricsTest{ realmKeyName: "realm", clientKeyName: "client", + realmsTrackingPattern: "users", clientTrackingPattern: clientTrackingPatternJustSomeUsers, header: http.Header{ authorizationHeaderName: { @@ -291,7 +302,7 @@ func Test_Filter_ClientMetrics_JWTBodyHasNoClient_ShouldTrackRealm(t *testing.T) }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users..", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users.{unknown}.", }) } @@ -319,10 +330,10 @@ func Test_Filter_ClientMetrics_NoFlagClientKeyName(t *testing.T) { func Test_Filter_ClientMetrics_DefaultClientTrackingPattern_NoClientTrackingPatternInRouteFilterJSON_User(t *testing.T) { testClientMetrics(t, clientMetricsTest{ - realmKeyName: "realm", - clientKeyName: "client", - defaultClientTrackingPattern: "services\\..*", - clientTrackingPattern: nil, // no client tracking in route's filter configuration + realmKeyName: "realm", + clientKeyName: "client", + realmsTrackingPattern: "services", + clientTrackingPattern: nil, // no client tracking in route's filter configuration header: http.Header{ authorizationHeaderName: { "Bearer " + buildFakeJwtWithBody(map[string]interface{}{ @@ -332,16 +343,16 @@ func Test_Filter_ClientMetrics_DefaultClientTrackingPattern_NoClientTrackingPatt }, }, expectedEndpointMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.GET.foo/orders.*.*.", - expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users..", + expectedClientMetricPrefix: "apiUsageMonitoring.custom.my_app.my_api.*.*.users.{all}.", }) } func Test_Filter_ClientMetrics_DefaultClientTrackingPattern_NoClientTrackingPatternInRouteFilterJSON_Service(t *testing.T) { testClientMetrics(t, clientMetricsTest{ - realmKeyName: "realm", - clientKeyName: "client", - defaultClientTrackingPattern: "services\\..*", - clientTrackingPattern: nil, // no client tracking in route's filter configuration + realmKeyName: "realm", + clientKeyName: "client", + realmsTrackingPattern: "services", + clientTrackingPattern: nil, // no client tracking in route's filter configuration header: http.Header{ authorizationHeaderName: { "Bearer " + buildFakeJwtWithBody(map[string]interface{}{ @@ -357,10 +368,10 @@ func Test_Filter_ClientMetrics_DefaultClientTrackingPattern_NoClientTrackingPatt func Test_Filter_ClientMetrics_EmptyClientTrackingPatternInRouteFilterJSON(t *testing.T) { testClientMetrics(t, clientMetricsTest{ - realmKeyName: "realm", - clientKeyName: "client", - defaultClientTrackingPattern: "services\\..*", - clientTrackingPattern: s(""), // no client tracking in route's filter configuration + realmKeyName: "realm", + clientKeyName: "client", + realmsTrackingPattern: "services", + clientTrackingPattern: s(""), // no client tracking in route's filter configuration header: http.Header{ authorizationHeaderName: { "Bearer " + buildFakeJwtWithBody(map[string]interface{}{ diff --git a/filters/apiusagemonitoring/filter_endpointmetrics_test.go b/filters/apiusagemonitoring/filter_endpointmetrics_test.go index 67e1cab5d7..323e14b8b3 100644 --- a/filters/apiusagemonitoring/filter_endpointmetrics_test.go +++ b/filters/apiusagemonitoring/filter_endpointmetrics_test.go @@ -20,35 +20,7 @@ func Test_Filter_NoPathTemplate(t *testing.T) { "https://www.example.org/a/b/c", 299, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app..GET..*.*." - // no path matching: tracked as unknown - m.WithCounters(func(counters map[string]int64) { - assert.Equal(t, - map[string]int64{ - pre + "http_count": int64(pass), - pre + "http2xx_count": int64(pass), - }, - counters, - ) - }) - m.WithMeasures(func(measures map[string][]time.Duration) { - assert.Contains(t, measures, pre+"latency") - }) - }) -} - -func Test_Filter_NoConfiguration(t *testing.T) { - testWithFilter( - t, - func() (filters.Filter, error) { - spec := NewApiUsageMonitoring(true, "", "", "") - return spec.CreateFilter([]interface{}{}) - }, - http.MethodGet, - "https://www.example.org/a/b/c", - 200, - func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom...GET..*.*." + pre := "apiUsageMonitoring.custom.my_app.{unknown}.GET.{no-match}.*.*." // no path matching: tracked as unknown m.WithCounters(func(counters map[string]int64) { assert.Equal(t, @@ -97,7 +69,7 @@ func Test_Filter_PathTemplateWithVariablePart(t *testing.T) { "https://www.example.org/foo/orders/1234", 204, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/:order-id.*.*." + pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/{order-id}.*.*." m.WithCounters(func(counters map[string]int64) { assert.Equal(t, map[string]int64{ @@ -121,12 +93,12 @@ func Test_Filter_PathTemplateWithMultipleVariablePart(t *testing.T) { "https://www.example.org/foo/orders/1234/order-items/123", 301, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/:order-id/order-items/:order-item-id.*.*." + pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/{order-id}/order-items/{order-item-id}.*.*." m.WithCounters(func(counters map[string]int64) { assert.NotContains( t, counters, - "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/:order-id.http_count", - "Matched `foo/orders/:order-id` instead of `foo/orders/:order-id`/order-items/:order-item-id") + "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/{order-id}.http_count", + "Matched `foo/orders/{order-id}` instead of `foo/orders/{order-id}`/order-items/{order-item-id}") assert.Equal(t, map[string]int64{ @@ -150,7 +122,7 @@ func Test_Filter_PathTemplateFromSecondConfiguredApi(t *testing.T) { "https://www.example.org/foo/customers/loremipsum", 502, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/customers/:customer-id.*.*." + pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/customers/{customer-id}.*.*." m.WithCounters(func(counters map[string]int64) { assert.Equal(t, map[string]int64{ @@ -238,7 +210,7 @@ func Test_Filter_StatusCodeUnder100IsMonitoredWithoutHttpStatusCount(t *testing. }) } -func Test_Filter_NonConfiguredPathTrackedUnderUnknown(t *testing.T) { +func Test_Filter_NonConfiguredPathTrackedUnderNoMatch(t *testing.T) { testWithFilter( t, createFilterForTest, @@ -246,7 +218,7 @@ func Test_Filter_NonConfiguredPathTrackedUnderUnknown(t *testing.T) { "https://www.example.org/lapin/malin", 200, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app..GET..*.*." + pre := "apiUsageMonitoring.custom.my_app.{unknown}.GET.{no-match}.*.*." m.WithCounters(func(counters map[string]int64) { assert.Equal(t, map[string]int64{ @@ -276,8 +248,8 @@ func Test_Filter_AllHttpMethodsAreSupported(t *testing.T) { {http.MethodConnect, "CONNECT"}, {http.MethodOptions, "OPTIONS"}, {http.MethodTrace, "TRACE"}, - {"", ""}, - {"foo", ""}, + {"", "{unknown}"}, + {"foo", "{unknown}"}, } { t.Run(testCase.method, func(t *testing.T) { testWithFilter( @@ -288,7 +260,7 @@ func Test_Filter_AllHttpMethodsAreSupported(t *testing.T) { 200, func(t *testing.T, pass int, m *metricstest.MockMetrics) { pre := fmt.Sprintf( - "apiUsageMonitoring.custom.my_app..%s..*.*.", + "apiUsageMonitoring.custom.my_app.{unknown}.%s.{no-match}.*.*.", testCase.expectedMethodInMetric) m.WithCounters(func(counters map[string]int64) { assert.Equal(t, @@ -315,7 +287,7 @@ func Test_Filter_PathTemplateMatchesInternalSlashes(t *testing.T) { "https://www.example.org/foo/orders/1/2/3/order-items/123", 204, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/:order-id/order-items/:order-item-id.*.*." + pre := "apiUsageMonitoring.custom.my_app.my_api.POST.foo/orders/{order-id}/order-items/{order-item-id}.*.*." m.WithCounters(func(counters map[string]int64) { assert.Equal(t, map[string]int64{ @@ -346,14 +318,14 @@ func Test_Filter_PathTemplateMatchesInternalSlashesTooFollowingVarPart(t *testin return spec.CreateFilter(args) } for _, c := range []struct { - requestPath string - expectedMatchedPathTemplate string + requestPath string + expectedMatchedPathLabel string }{ - {"foo/1", "foo/:a"}, - {"foo/1/2", "foo/:a/:b"}, - {"foo/1/2/3", "foo/:a/:b/:c"}, - {"foo/1/2/3/4", "foo/:a/:b/:c"}, - {"foo/1/2/3/4/5", "foo/:a/:b/:c"}, + {"foo/1", "foo/{a}"}, + {"foo/1/2", "foo/{a}/{b}"}, + {"foo/1/2/3", "foo/{a}/{b}/{c}"}, + {"foo/1/2/3/4", "foo/{a}/{b}/{c}"}, + {"foo/1/2/3/4/5", "foo/{a}/{b}/{c}"}, } { subTestName := strings.Replace(c.requestPath, "/", "_", -1) t.Run(subTestName, func(t *testing.T) { @@ -364,7 +336,7 @@ func Test_Filter_PathTemplateMatchesInternalSlashesTooFollowingVarPart(t *testin fmt.Sprintf("https://www.example.org/%s", c.requestPath), 204, func(t *testing.T, pass int, m *metricstest.MockMetrics) { - pre := "apiUsageMonitoring.custom.my_app.my_api.GET." + c.expectedMatchedPathTemplate + ".*.*." + pre := "apiUsageMonitoring.custom.my_app.my_api.GET." + c.expectedMatchedPathLabel + ".*.*." m.WithCounters(func(counters map[string]int64) { assert.Equal(t, map[string]int64{ diff --git a/filters/apiusagemonitoring/pathinfo.go b/filters/apiusagemonitoring/pathinfo.go index 42e7f023e1..bd6df55a86 100644 --- a/filters/apiusagemonitoring/pathinfo.go +++ b/filters/apiusagemonitoring/pathinfo.go @@ -10,6 +10,7 @@ type pathInfo struct { ApplicationId string ApiId string PathTemplate string + PathLabel string Matcher *regexp.Regexp ClientTracking *clientTrackingInfo CommonPrefix string @@ -19,7 +20,7 @@ type pathInfo struct { metricPrefixedPerClient map[string]*clientMetricNames } -func newPathInfo(applicationId, apiId, pathTemplate string, clientTracking *clientTrackingInfo) *pathInfo { +func newPathInfo(applicationId, apiId, pathTemplate string, pathLabel string, clientTracking *clientTrackingInfo) *pathInfo { commonPrefix := applicationId + "." + apiId + "." var metricPrefixedPerClient map[string]*clientMetricNames if clientTracking != nil { @@ -29,6 +30,7 @@ func newPathInfo(applicationId, apiId, pathTemplate string, clientTracking *clie ApplicationId: applicationId, ApiId: apiId, PathTemplate: pathTemplate, + PathLabel: pathLabel, metricPrefixedPerClient: metricPrefixedPerClient, ClientTracking: clientTracking, CommonPrefix: commonPrefix, @@ -89,4 +91,5 @@ var ( type clientTrackingInfo struct { ClientTrackingMatcher *regexp.Regexp + RealmsTrackingMatcher *regexp.Regexp } diff --git a/filters/apiusagemonitoring/spec.go b/filters/apiusagemonitoring/spec.go index 157c48f38f..f0cde54777 100644 --- a/filters/apiusagemonitoring/spec.go +++ b/filters/apiusagemonitoring/spec.go @@ -2,6 +2,7 @@ package apiusagemonitoring import ( "encoding/json" + "fmt" "github.com/sirupsen/logrus" "github.com/zalando/skipper/filters" "regexp" @@ -12,7 +13,8 @@ import ( const ( Name = "apiUsageMonitoring" - unknownElementPlaceholder = "" + unknownPlaceholder = "{unknown}" + noMatchPlaceholder = "{no-match}" regexUrlPathPart = `.+` regexOptionalSlashes = `\/*` @@ -30,14 +32,14 @@ func NewApiUsageMonitoring( enabled bool, realmKeys string, clientKeys string, - defaultClientTrackingPattern string, + realmsTrackingPattern string, ) filters.Spec { if !enabled { - log.Debugf("Filter %q is not enabled. Spec returns `noop` filters.", Name) + log.Debugf("filter %q is not enabled. spec returns `noop` filters.", Name) return &noopSpec{&noopFilter{}} } - // Parse realm keys comma separated list + // parse realm keys comma separated list var realmKeyList []string for _, key := range strings.Split(realmKeys, ",") { strippedKey := strings.TrimSpace(key) @@ -45,7 +47,7 @@ func NewApiUsageMonitoring( realmKeyList = append(realmKeyList, strippedKey) } } - // Parse client keys comma separated list + // parse client keys comma separated list var clientKeyList []string for _, key := range strings.Split(clientKeys, ",") { strippedKey := strings.TrimSpace(key) @@ -53,27 +55,39 @@ func NewApiUsageMonitoring( clientKeyList = append(clientKeyList, strippedKey) } } + // compile realms regex + realmsTrackingMatcher, err := regexp.Compile(realmsTrackingPattern) + if err != nil { + log.Errorf( + "api-usage-monitoring-realmsTrackingPattern-tracking-pattern (global config) ignored: error compiling regular expression %q: %v", + realmsTrackingPattern, err) + realmsTrackingMatcher = regexp.MustCompile("services") + log.Warn("defaulting to 'services' as api-usage-monitoring-realmsTrackingPattern-tracking-pattern (global config)") + } // Create the filter Spec var unknownPathClientTracking *clientTrackingInfo = nil // client metrics feature is disabled if realmKeys != "" { unknownPathClientTracking = &clientTrackingInfo{ - ClientTrackingMatcher: nil, // do not match anything (track `realm.`) + ClientTrackingMatcher: nil, // do not match anything (track `realm.{unknown}`) + RealmsTrackingMatcher: realmsTrackingMatcher, } } unknownPath := newPathInfo( - unknownElementPlaceholder, - unknownElementPlaceholder, - unknownElementPlaceholder, + unknownPlaceholder, + unknownPlaceholder, + noMatchPlaceholder, + noMatchPlaceholder, unknownPathClientTracking, ) + spec := &apiUsageMonitoringSpec{ - realmKeys: realmKeyList, - clientKeys: clientKeyList, - unknownPath: unknownPath, - defaultClientTrackingPattern: defaultClientTrackingPattern, + realmKeys: realmKeyList, + clientKeys: clientKeyList, + unknownPath: unknownPath, + realmsTrackingMatcher: realmsTrackingMatcher, } - log.Debugf("Created filter spec: %+v", spec) + log.Debugf("created filter spec: %+v", spec) return spec } @@ -86,10 +100,10 @@ type apiConfig struct { } type apiUsageMonitoringSpec struct { - realmKeys []string - clientKeys []string - unknownPath *pathInfo - defaultClientTrackingPattern string + realmKeys []string + clientKeys []string + realmsTrackingMatcher *regexp.Regexp + unknownPath *pathInfo } func (s *apiUsageMonitoringSpec) Name() string { @@ -99,12 +113,15 @@ func (s *apiUsageMonitoringSpec) Name() string { func (s *apiUsageMonitoringSpec) CreateFilter(args []interface{}) (filter filters.Filter, err error) { apis := s.parseJsonConfiguration(args) paths := s.buildPathInfoListFromConfiguration(apis) - unknownPath := s.buildUnknownPathInfo(apis) + + if len(paths) == 0 { + return nil, fmt.Errorf("no valid configurations") + } filter = &apiUsageMonitoringFilter{ Spec: s, Paths: paths, - UnknownPath: unknownPath, + UnknownPath: s.buildUnknownPathInfo(paths), } return } @@ -118,7 +135,7 @@ func (s *apiUsageMonitoringSpec) parseJsonConfiguration(args []interface{}) []*a continue } config := &apiConfig{ - ClientTrackingPattern: s.defaultClientTrackingPattern, + ClientTrackingPattern: ".*", // track all clients per default } decoder := json.NewDecoder(strings.NewReader(rawJsonConfiguration)) decoder.DisallowUnknownFields() @@ -132,19 +149,20 @@ func (s *apiUsageMonitoringSpec) parseJsonConfiguration(args []interface{}) []*a return apis } -func (s *apiUsageMonitoringSpec) buildUnknownPathInfo(apis []*apiConfig) *pathInfo { +func (s *apiUsageMonitoringSpec) buildUnknownPathInfo(paths []*pathInfo) *pathInfo { var applicationId *string - for _, api := range apis { - if applicationId != nil && *applicationId != api.ApplicationId { + for _, path := range paths { + if applicationId != nil && *applicationId != path.ApplicationId { return s.unknownPath } - applicationId = &api.ApplicationId + applicationId = &path.ApplicationId } if applicationId != nil && *applicationId != "" { return newPathInfo( *applicationId, s.unknownPath.ApiId, + s.unknownPath.PathLabel, s.unknownPath.PathTemplate, s.unknownPath.ClientTracking) } @@ -155,32 +173,27 @@ func (s *apiUsageMonitoringSpec) buildPathInfoListFromConfiguration(apis []*apiC var paths []*pathInfo existingPathTemplates := make(map[string]*pathInfo) existingRegEx := make(map[string]*pathInfo) - for apiIndex, api := range apis { - if api.PathTemplates == nil || len(api.PathTemplates) == 0 { - log.Errorf( - "args[%d] ignored: does not specify any path template", - apiIndex) - continue - } + for apiIndex, api := range apis { applicationId := api.ApplicationId if applicationId == "" { - log.Errorf( - "args[%d] does not specify an application ID, defaulting to %q", - apiIndex, unknownElementPlaceholder) - applicationId = unknownElementPlaceholder + log.Errorf("args[%d] ignored: does not specify an application_id", apiIndex) + continue } apiId := api.ApiId if apiId == "" { - log.Errorf( - "args[%d] does not specify an API ID, defaulting to %q", - apiIndex, unknownElementPlaceholder) - apiId = unknownElementPlaceholder + log.Errorf("args[%d] ignored: does not specify an api_id", apiIndex) + continue } - clientTrackingInfo := s.buildClientTrackingInfo(apiIndex, api) + if api.PathTemplates == nil || len(api.PathTemplates) == 0 { + log.Errorf("args[%d] ignored: does not specify any path template", apiIndex) + continue + } + + clientTrackingInfo := s.buildClientTrackingInfo(apiIndex, api, s.realmsTrackingMatcher) for templateIndex, template := range api.PathTemplates { @@ -193,10 +206,10 @@ func (s *apiUsageMonitoringSpec) buildPathInfoListFromConfiguration(apis []*apiC } // Normalize path template and get regular expression from it - normalisedPathTemplate, regExStr := generateRegExpStringForPathTemplate(template) + normalisedPathTemplate, regExStr, pathLabel := generateRegExpStringForPathTemplate(template) // Create new `pathInfo` with normalized PathTemplate - info := newPathInfo(applicationId, apiId, normalisedPathTemplate, clientTrackingInfo) + info := newPathInfo(applicationId, apiId, normalisedPathTemplate, pathLabel, clientTrackingInfo) // Detect path template duplicates if _, ok := existingPathTemplates[info.PathTemplate]; ok { @@ -237,7 +250,7 @@ func (s *apiUsageMonitoringSpec) buildPathInfoListFromConfiguration(apis []*apiC return paths } -func (s *apiUsageMonitoringSpec) buildClientTrackingInfo(apiIndex int, api *apiConfig) *clientTrackingInfo { +func (s *apiUsageMonitoringSpec) buildClientTrackingInfo(apiIndex int, api *apiConfig, realmsTrackingMatcher *regexp.Regexp) *clientTrackingInfo { if len(s.realmKeys) == 0 { log.Infof( `args[%d]: skipper wide configuration "api-usage-monitoring-realm-keys" not provided, not tracking client metrics`, @@ -267,15 +280,17 @@ func (s *apiUsageMonitoringSpec) buildClientTrackingInfo(apiIndex int, api *apiC return &clientTrackingInfo{ ClientTrackingMatcher: clientTrackingMatcher, + RealmsTrackingMatcher: realmsTrackingMatcher, } } // generateRegExpStringForPathTemplate normalizes the given path template and // creates a regular expression from it. -func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTemplate, matcher string) { +func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTemplate, matcher, pathLabel string) { pathParts := strings.Split(pathTemplate, "/") matcherPathParts := make([]string, 0, len(pathParts)) normalizedPathTemplateParts := make([]string, 0, len(pathParts)) + pathLabelParts := make([]string, 0, len(pathParts)) for _, p := range pathParts { if p == "" { continue @@ -285,10 +300,13 @@ func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTem // this part is not a placeholder: match it exactly matcherPathParts = append(matcherPathParts, p) normalizedPathTemplateParts = append(normalizedPathTemplateParts, p) + pathLabelParts = append(pathLabelParts, p) + } else { // this part is a placeholder: match a wildcard for it matcherPathParts = append(matcherPathParts, regexUrlPathPart) normalizedPathTemplateParts = append(normalizedPathTemplateParts, ":"+placeholderName) + pathLabelParts = append(pathLabelParts, "{"+placeholderName+"}") } } rawRegEx := &strings.Builder{} @@ -300,6 +318,8 @@ func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTem matcher = rawRegEx.String() normalizedPathTemplate = strings.Join(normalizedPathTemplateParts, "/") + pathLabel = strings.Join(pathLabelParts, "/") + return } diff --git a/filters/apiusagemonitoring/spec_test.go b/filters/apiusagemonitoring/spec_test.go index 73ee3b86cc..5b908f16c9 100644 --- a/filters/apiusagemonitoring/spec_test.go +++ b/filters/apiusagemonitoring/spec_test.go @@ -43,9 +43,9 @@ type pathMatcher struct { func unknownPath(applicationId string) pathMatcher { return pathMatcher{ - PathTemplate: "", + PathTemplate: "{no-match}", ApplicationId: applicationId, - ApiId: "", + ApiId: "{unknown}", Matcher: nil, } } @@ -81,91 +81,111 @@ func assertPaths(t *testing.T, paths []*pathInfo, expectedPaths []pathMatcher) { func Test_FeatureNotEnabled_TypeNameAndCreatedFilterAreRight(t *testing.T) { spec := NewApiUsageMonitoring(false, "", "", "") assert.Equal(t, "apiUsageMonitoring", spec.Name()) + filter, err := spec.CreateFilter([]interface{}{}) + assert.NoError(t, err) assert.Equal(t, filter, &noopFilter{}) } -func Test_CreateFilter_NoParam(t *testing.T) { - var args []interface{} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) +func Test_CreateFilter_NoParam_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_EmptyString(t *testing.T) { - args := []interface{}{""} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) +func Test_CreateFilter_EmptyString_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{""}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_NotAString(t *testing.T) { - args := []interface{}{1234} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) +func Test_CreateFilter_NotAString_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{1234}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_NotJson(t *testing.T) { - args := []interface{}{"I am not JSON"} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) +func Test_CreateFilter_NotJson_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{"I am not JSON"}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_EmptyJson(t *testing.T) { - args := []interface{}{"{}"} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) +func Test_CreateFilter_EmptyJson_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{"{}"}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_NoPathTemplate(t *testing.T) { - args := []interface{}{`{ +func Test_CreateFilter_NoPathTemplate_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{`{ + "application_id": "app", + "api_id": "api", "path_templates": [] - }`} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) + }`}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_EmptyPathTemplate(t *testing.T) { - args := []interface{}{`{ +func Test_CreateFilter_EmptyPathTemplate_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{`{ "application_id": "my_app", "api_id": "my_api", "path_templates": [ "" ] - }`} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("my_app")) - }) + }`}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_TypoInPropertyNamesFail(t *testing.T) { +func Test_CreateFilter_TypoInPropertyNames_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + // path_template has no `s` and should cause a JSON decoding error. - args := []interface{}{`{ + _, err := spec.CreateFilter([]interface{}{`{ "application_id": "my_app", "api_id": "my_api", "path_template": [ "" ] - }`} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) + }`}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) } -func Test_CreateFilter_NonParseableParametersShouldBeLoggedAndIgnored(t *testing.T) { +func Test_CreateFilter_NonParsableParametersShouldBeLoggedAndIgnored(t *testing.T) { args := []interface{}{ `{ "application_id": "my_app", @@ -245,23 +265,36 @@ func Test_CreateFilter_FullConfigSingleApi(t *testing.T) { }) } -func Test_CreateFilter_NoApplicationOrApiId(t *testing.T) { - args := []interface{}{`{ +func Test_CreateFilter_NoApplicationId_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{`{ + "api_id": "api", "path_templates": [ "foo/orders" ] - }`} - assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assertPaths(t, filter.Paths, []pathMatcher{ - { - ApplicationId: "", - ApiId: "", - PathTemplate: "foo/orders", - Matcher: matcher("^\\/*foo\\/orders\\/*$"), - }, - }) - assertPath(t, filter.UnknownPath, unknownPath("")) - }) + }`}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) + +} + +func Test_CreateFilter_NoApiId_ShouldReturnError(t *testing.T) { + spec := NewApiUsageMonitoring(true, "", "", "") + + _, err := spec.CreateFilter([]interface{}{`{ + "application_id": "api", + "path_templates": [ + "foo/orders" + ] + }`}) + + assert.NotNil(t, err) + assert.Error(t, err) + assert.Regexp(t, `.*no valid configurations.*`, err.Error()) + } func Test_CreateFilter_FullConfigMultipleApis(t *testing.T) { @@ -358,7 +391,7 @@ func Test_CreateFilter_FullConfigWithApisWithoutPaths(t *testing.T) { Matcher: matcher("^\\/*foo\\/orders\\/*$"), }, }) - assertPath(t, filter.UnknownPath, unknownPath("")) + assertPath(t, filter.UnknownPath, unknownPath("my_order_app")) }) } @@ -414,11 +447,12 @@ func Test_CreateFilter_RegExCompileFailureIgnoresPath(t *testing.T) { "application_id": "my_app", "api_id": "orders_api", "path_templates": [ - "([" + "([", + "orders/" ] }`} assertApiUsageMonitoringFilter(t, args, func(t *testing.T, filter *apiUsageMonitoringFilter) { - assert.Empty(t, filter.Paths) + assert.Equal(t, 1, len(filter.Paths)) assertPath(t, filter.UnknownPath, unknownPath("my_app")) }) } diff --git a/filters/apiusagemonitoring/testutils_test.go b/filters/apiusagemonitoring/testutils_test.go index fc953214bc..9b0af333d6 100644 --- a/filters/apiusagemonitoring/testutils_test.go +++ b/filters/apiusagemonitoring/testutils_test.go @@ -190,7 +190,7 @@ func testClientMetrics(t *testing.T, testCase clientMetricsTest) { t.FailNow() } args := []interface{}{string(js)} - spec := NewApiUsageMonitoring(true, testCase.realmKeyName, testCase.clientKeyName, testCase.defaultClientTrackingPattern) + spec := NewApiUsageMonitoring(true, testCase.realmKeyName, testCase.clientKeyName, testCase.realmsTrackingPattern) return spec.CreateFilter(args) }, } diff --git a/skipper.go b/skipper.go index 891c0574ce..372f144d85 100644 --- a/skipper.go +++ b/skipper.go @@ -492,9 +492,10 @@ type Options struct { // API Monitoring feature is active (feature toggle) ApiUsageMonitoringEnable bool - ApiUsageMonitoringRealmKey string - ApiUsageMonitoringClientIdKeyName string + ApiUsageMonitoringRealmKeys string + ApiUsageMonitoringClientKeys string ApiUsageMonitoringDefaultClientTrackingPattern string + ApiUsageMonitoringRealmsTrackingPattern string // WebhookTimeout sets timeout duration while calling a custom webhook auth service WebhookTimeout time.Duration @@ -761,9 +762,9 @@ func Run(o Options) error { auth.NewOAuthOidcAllClaims(o.OIDCSecretsFile), apiusagemonitoring.NewApiUsageMonitoring( o.ApiUsageMonitoringEnable, - o.ApiUsageMonitoringRealmKey, - o.ApiUsageMonitoringClientIdKeyName, - o.ApiUsageMonitoringDefaultClientTrackingPattern, + o.ApiUsageMonitoringRealmKeys, + o.ApiUsageMonitoringClientKeys, + o.ApiUsageMonitoringRealmsTrackingPattern, ), )