From 7dffc11abc37c4bd750a27553b6d983894bf865c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20Zi=C4=99ba?= Date: Mon, 8 Mar 2021 13:34:55 +0000 Subject: [PATCH] Fix incorret authentication metrics --- .../pkg/endpoints/filters/authentication.go | 12 ++- .../endpoints/filters/authentication_test.go | 86 +++++++++++++++++++ .../pkg/endpoints/filters/metrics.go | 4 +- 3 files changed, 99 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go index 54e77467c0b..d69cfef32d1 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go @@ -17,6 +17,7 @@ limitations under the License. package filters import ( + "context" "errors" "fmt" "net/http" @@ -31,11 +32,17 @@ import ( "k8s.io/klog/v2" ) +type recordMetrics func(context.Context, *authenticator.Response, bool, error, authenticator.Audiences, time.Time, time.Time) + // WithAuthentication creates an http handler that tries to authenticate the given request as a user, and then // stores any such user found onto the provided context for the request. If authentication fails or returns an error // the failed handler is used. On success, "Authorization" header is removed from the request and handler // is invoked to serve the request. func WithAuthentication(handler http.Handler, auth authenticator.Request, failed http.Handler, apiAuds authenticator.Audiences) http.Handler { + return withAuthentication(handler, auth, failed, apiAuds, recordAuthMetrics) +} + +func withAuthentication(handler http.Handler, auth authenticator.Request, failed http.Handler, apiAuds authenticator.Audiences, metrics recordMetrics) http.Handler { if auth == nil { klog.Warning("Authentication is disabled") return handler @@ -47,7 +54,10 @@ func WithAuthentication(handler http.Handler, auth authenticator.Request, failed req = req.WithContext(authenticator.WithAudiences(req.Context(), apiAuds)) } resp, ok, err := auth.AuthenticateRequest(req) - defer recordAuthMetrics(req.Context(), resp, ok, err, apiAuds, authenticationStart) + authenticationFinish := time.Now() + defer func() { + metrics(req.Context(), resp, ok, err, apiAuds, authenticationStart, authenticationFinish) + }() if err != nil || !ok { if err != nil { klog.ErrorS(err, "Unable to authenticate the request") diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication_test.go index 2873c67cb3d..2fd4f9dfe93 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication_test.go @@ -17,6 +17,7 @@ limitations under the License. package filters import ( + "context" "errors" "github.com/stretchr/testify/assert" "k8s.io/apiserver/pkg/authentication/authenticator" @@ -25,6 +26,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" ) func TestAuthenticateRequestWithAud(t *testing.T) { @@ -104,6 +106,90 @@ func TestAuthenticateRequestWithAud(t *testing.T) { } } +func TestAuthenticateMetrics(t *testing.T) { + testcases := []struct { + name string + header bool + apiAuds []string + respAuds []string + expectMetric bool + expectOk bool + expectError bool + }{ + { + name: "no api audience and no audience in response", + header: true, + apiAuds: nil, + respAuds: nil, + expectOk: true, + expectError: false, + }, + { + name: "api audience matching response audience", + header: true, + apiAuds: authenticator.Audiences([]string{"other"}), + respAuds: []string{"other"}, + expectOk: true, + expectError: false, + }, + { + name: "no intersection results in error", + header: true, + apiAuds: authenticator.Audiences([]string{"other"}), + respAuds: []string{"some"}, + expectOk: true, + expectError: true, + }, + { + name: "no header results in error", + header: false, + apiAuds: authenticator.Audiences([]string{"other"}), + respAuds: []string{"some"}, + expectOk: false, + expectError: true, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + called := 0 + auth := withAuthentication( + http.HandlerFunc(func(_ http.ResponseWriter, req *http.Request) { + }), + authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) { + if req.Header.Get("Authorization") == "Something" { + return &authenticator.Response{User: &user.DefaultInfo{Name: "user"}, Audiences: authenticator.Audiences(tc.respAuds)}, true, nil + } + return nil, false, errors.New("Authorization header is missing.") + }), + http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + }), + tc.apiAuds, + func(ctx context.Context, resp *authenticator.Response, ok bool, err error, apiAudiences authenticator.Audiences, authStart time.Time, authFinish time.Time) { + called = 1 + if tc.expectOk != ok { + t.Errorf("unexpected value of ok argument: %t", ok) + } + if tc.expectError { + if err == nil { + t.Errorf("unexpected value of err argument: %s", err) + } + } else { + if err != nil { + t.Errorf("unexpected value of err argument: %s", err) + } + } + }, + ) + if tc.header { + auth.ServeHTTP(httptest.NewRecorder(), &http.Request{Header: map[string][]string{"Authorization": {"Something"}}}) + } else { + auth.ServeHTTP(httptest.NewRecorder(), &http.Request{}) + } + assert.Equal(t, 1, called) + }) + } +} + func TestAuthenticateRequest(t *testing.T) { success := make(chan struct{}) auth := WithAuthentication( diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go index 23bd46fe973..31ead93d51a 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go @@ -76,7 +76,7 @@ func init() { legacyregistry.MustRegister(authenticationLatency) } -func recordAuthMetrics(ctx context.Context, resp *authenticator.Response, ok bool, err error, apiAudiences authenticator.Audiences, authStart time.Time) { +func recordAuthMetrics(ctx context.Context, resp *authenticator.Response, ok bool, err error, apiAudiences authenticator.Audiences, authStart time.Time, authFinish time.Time) { var resultLabel string switch { @@ -90,7 +90,7 @@ func recordAuthMetrics(ctx context.Context, resp *authenticator.Response, ok boo } authenticatedAttemptsCounter.WithContext(ctx).WithLabelValues(resultLabel).Inc() - authenticationLatency.WithContext(ctx).WithLabelValues(resultLabel).Observe(time.Since(authStart).Seconds()) + authenticationLatency.WithContext(ctx).WithLabelValues(resultLabel).Observe(authFinish.Sub(authStart).Seconds()) } // compressUsername maps all possible usernames onto a small set of categories