Billing metrics always contains `anon' user.

dkirillov commented

2024-02-22 14:05:57 +00:00

Collaborator

Billing metrics

$ curl http://localhost:8086/metrics/billing
# HELP frostfs_s3_gw_billing_user_requests Accumulated user requests
# TYPE frostfs_s3_gw_billing_user_requests counter
frostfs_s3_gw_billing_user_requests{bucket="test2",cid="Erv2GCQQq1gkRKchMpe1o4tsH3U6SvFz1PQqRRHoi4pP",operation="PUT",user="anon"} 1

always contains anon instead of actual owner id.

Expected Behavior

Metric contains actual owner ID.

Current Behavior

Metric contains anon for authenticated requests.

Possible Solution

There are two possible solutions I see:

Change order of middlwares (put s3middleware.Metrics after s3middleware.Auth)

diff --git a/api/router.go b/api/router.go
index 1e7cf80e..2606441d 100644
--- a/api/router.go
+++ b/api/router.go
@@ -127,9 +127,9 @@ func NewRouter(cfg Config) *chi.Mux {
                middleware.ThrottleWithOpts(cfg.Throttle),
                middleware.Recoverer,
                s3middleware.Tracing(),
-               s3middleware.Metrics(cfg.Log, cfg.Handler.ResolveBucket, cfg.Metrics, cfg.MiddlewareSettings),
                s3middleware.LogSuccessResponse(cfg.Log),
                s3middleware.Auth(cfg.Center, cfg.Log),
+               s3middleware.Metrics(cfg.Log, cfg.Handler.ResolveBucket, cfg.Metrics, cfg.MiddlewareSettings),
        )
 
        if cfg.FrostFSIDValidation {

But this leads to no counting requests that fail authentication and don't count authentication in result request duration.

Instead of resolving user from ctx get it from ReqInfo (s3middleware.Auth must put proper owner id into it)

diff --git a/api/middleware/auth.go b/api/middleware/auth.go
index b35ef126..76984025 100644
--- a/api/middleware/auth.go
+++ b/api/middleware/auth.go
@@ -46,6 +46,8 @@ func Auth(center Center, log *zap.Logger) Func {
        return func(h http.Handler) http.Handler {
                return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                        ctx := r.Context()
+                       reqInfo := GetReqInfo(ctx)
+                       reqInfo.User = "anon"
                        box, err := center.Authenticate(r)
                        if err != nil {
                                if err == ErrNoAuthorizationHeader {
@@ -64,6 +66,10 @@ func Auth(center Center, log *zap.Logger) Func {
                                        ctx = SetClientTime(ctx, box.ClientTime)
                                }
                                ctx = SetAuthHeaders(ctx, box.AuthHeaders)
+
+                               if box.AccessBox.Gate.BearerToken != nil {
+                                       reqInfo.User = bearer.ResolveIssuer(*box.AccessBox.Gate.BearerToken).String()
+                               }
                        }
 
                        h.ServeHTTP(w, r.WithContext(ctx))
diff --git a/api/middleware/metrics.go b/api/middleware/metrics.go
index 1370c3bc..273b4b96 100644
--- a/api/middleware/metrics.go
+++ b/api/middleware/metrics.go
@@ -80,7 +80,7 @@ func stats(f http.HandlerFunc, resolveCID cidResolveFunc, appMetrics *metrics.Ap
                // simply for the fact that it is not human-readable.
                durationSecs := time.Since(statsWriter.startTime).Seconds()
 
-               user := resolveUser(r.Context())
+               user := reqInfo.User
                cnrID := resolveCID(r.Context(), reqInfo)
                appMetrics.Update(user, reqInfo.BucketName, cnrID, settings.ResolveNamespaceAlias(reqInfo.Namespace),
                        requestTypeFromAPI(reqInfo.API), in.countBytes, out.countBytes)
diff --git a/api/middleware/reqinfo.go b/api/middleware/reqinfo.go
index 1c016079..6b8b2809 100644
--- a/api/middleware/reqinfo.go
+++ b/api/middleware/reqinfo.go
@@ -35,6 +35,7 @@ type (
                RequestID    string   // x-amz-request-id
                API          string   // API name -- GetObject PutObject NewMultipartUpload etc.
                BucketName   string   // Bucket name
+               User         string   // User owner id
                ObjectName   string   // Object name
                TraceID      string   // Trace ID
                URL          *url.URL // Request url

Steps to Reproduce (for bugs)

Create bucket

$ aws   s3api --endpoint http://localhost:8084  create-bucket --bucket test

Put object

$ s3api --endpoint http://localhost:8084  put-object --bucket test --key mtp 
{
    "ETag": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
}

Get metrics

$ curl http://localhost:8086/metrics/billing
# HELP frostfs_s3_gw_billing_user_requests Accumulated user requests
# TYPE frostfs_s3_gw_billing_user_requests counter
frostfs_s3_gw_billing_user_requests{bucket="test2",cid="Erv2GCQQq1gkRKchMpe1o4tsH3U6SvFz1PQqRRHoi4pP",operation="PUT",user="anon"} 1

Context

This metric is used to make billing, so we need to have properly owner id.

Regression

Yes. This #149 pr changed middlewares order.

Your Environment

Version used: 391fc9cbe3
Server setup and configuration: devenv

Billing metrics ``` $ curl http://localhost:8086/metrics/billing # HELP frostfs_s3_gw_billing_user_requests Accumulated user requests # TYPE frostfs_s3_gw_billing_user_requests counter frostfs_s3_gw_billing_user_requests{bucket="test2",cid="Erv2GCQQq1gkRKchMpe1o4tsH3U6SvFz1PQqRRHoi4pP",operation="PUT",user="anon"} 1 ``` always contains `anon` instead of actual owner id. ## Expected Behavior Metric contains actual owner ID. ## Current Behavior Metric contains `anon` for authenticated requests. ## Possible Solution There are two possible solutions I see: * Change [order of middlwares](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/391fc9cbe3c95b26bd5dc684a9c09bed0a6e6865/api/router.go#L130-L132) (put `s3middleware.Metrics` after `s3middleware.Auth`) ```diff diff --git a/api/router.go b/api/router.go index 1e7cf80e..2606441d 100644 --- a/api/router.go +++ b/api/router.go @@ -127,9 +127,9 @@ func NewRouter(cfg Config) *chi.Mux { middleware.ThrottleWithOpts(cfg.Throttle), middleware.Recoverer, s3middleware.Tracing(), - s3middleware.Metrics(cfg.Log, cfg.Handler.ResolveBucket, cfg.Metrics, cfg.MiddlewareSettings), s3middleware.LogSuccessResponse(cfg.Log), s3middleware.Auth(cfg.Center, cfg.Log), + s3middleware.Metrics(cfg.Log, cfg.Handler.ResolveBucket, cfg.Metrics, cfg.MiddlewareSettings), ) if cfg.FrostFSIDValidation { ``` But this leads to no counting requests that fail authentication and don't count authentication in result request duration. * Instead of [resolving user from ctx](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/commit/391fc9cbe3c95b26bd5dc684a9c09bed0a6e6865/api/middleware/metrics.go#L83) get it from `ReqInfo` (`s3middleware.Auth` must put proper owner id into it) ```diff diff --git a/api/middleware/auth.go b/api/middleware/auth.go index b35ef126..76984025 100644 --- a/api/middleware/auth.go +++ b/api/middleware/auth.go @@ -46,6 +46,8 @@ func Auth(center Center, log *zap.Logger) Func { return func(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + reqInfo := GetReqInfo(ctx) + reqInfo.User = "anon" box, err := center.Authenticate(r) if err != nil { if err == ErrNoAuthorizationHeader { @@ -64,6 +66,10 @@ func Auth(center Center, log *zap.Logger) Func { ctx = SetClientTime(ctx, box.ClientTime) } ctx = SetAuthHeaders(ctx, box.AuthHeaders) + + if box.AccessBox.Gate.BearerToken != nil { + reqInfo.User = bearer.ResolveIssuer(*box.AccessBox.Gate.BearerToken).String() + } } h.ServeHTTP(w, r.WithContext(ctx)) diff --git a/api/middleware/metrics.go b/api/middleware/metrics.go index 1370c3bc..273b4b96 100644 --- a/api/middleware/metrics.go +++ b/api/middleware/metrics.go @@ -80,7 +80,7 @@ func stats(f http.HandlerFunc, resolveCID cidResolveFunc, appMetrics *metrics.Ap // simply for the fact that it is not human-readable. durationSecs := time.Since(statsWriter.startTime).Seconds() - user := resolveUser(r.Context()) + user := reqInfo.User cnrID := resolveCID(r.Context(), reqInfo) appMetrics.Update(user, reqInfo.BucketName, cnrID, settings.ResolveNamespaceAlias(reqInfo.Namespace), requestTypeFromAPI(reqInfo.API), in.countBytes, out.countBytes) diff --git a/api/middleware/reqinfo.go b/api/middleware/reqinfo.go index 1c016079..6b8b2809 100644 --- a/api/middleware/reqinfo.go +++ b/api/middleware/reqinfo.go @@ -35,6 +35,7 @@ type ( RequestID string // x-amz-request-id API string // API name -- GetObject PutObject NewMultipartUpload etc. BucketName string // Bucket name + User string // User owner id ObjectName string // Object name TraceID string // Trace ID URL *url.URL // Request url ``` ## Steps to Reproduce (for bugs) 1. Create bucket ``` $ aws s3api --endpoint http://localhost:8084 create-bucket --bucket test ``` 2. Put object ``` $ s3api --endpoint http://localhost:8084 put-object --bucket test --key mtp { "ETag": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" } ``` 3. Get metrics ``` $ curl http://localhost:8086/metrics/billing # HELP frostfs_s3_gw_billing_user_requests Accumulated user requests # TYPE frostfs_s3_gw_billing_user_requests counter frostfs_s3_gw_billing_user_requests{bucket="test2",cid="Erv2GCQQq1gkRKchMpe1o4tsH3U6SvFz1PQqRRHoi4pP",operation="PUT",user="anon"} 1 ``` ## Context This metric is used to make billing, so we need to have properly owner id. ## Regression Yes. This #149 pr changed middlewares order. ## Your Environment  * Version used: https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/commit/391fc9cbe3c95b26bd5dc684a9c09bed0a6e6865 * Server setup and configuration: devenv