Merge pull request #99944 from marseel/fix/fix_incorrect_authenticato…

…r_metrics

Fix incorrect authentication latency metric

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")

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(

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