Merge pull request #87631 from immutableT/extract-auth-metrics

Factor-out metrics related logic from authentication logic.

staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD

@@ -15,6 +15,7 @@ go_test(
         "authorization_test.go",
         "cachecontrol_test.go",
         "impersonation_test.go",
+        "metrics_test.go",
         "requestinfo_test.go",
     ],
     embed = [":go_default_library"],
@@ -33,6 +34,8 @@ go_test(
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
+        "//staging/src/k8s.io/component-base/metrics/legacyregistry:go_default_library",
+        "//staging/src/k8s.io/component-base/metrics/testutil:go_default_library",
         "//vendor/github.com/google/uuid:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
     ],
@@ -48,6 +51,7 @@ go_library(
         "cachecontrol.go",
         "doc.go",
         "impersonation.go",
+        "metrics.go",
         "requestinfo.go",
     ],
     importmap = "k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/endpoints/filters",

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

@@ -18,8 +18,8 @@ package filters
 
 import (
 	"errors"
+	"fmt"
 	"net/http"
-	"strings"
 	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -28,61 +28,9 @@ import (
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
-	"k8s.io/component-base/metrics"
-	"k8s.io/component-base/metrics/legacyregistry"
 	"k8s.io/klog"
 )
 
-/*
- * By default, all the following metrics are defined as falling under
- * ALPHA stability level https://github.com/kubernetes/enhancements/blob/master/keps/sig-instrumentation/20190404-kubernetes-control-plane-metrics-stability.md#stability-classes)
- *
- * Promoting the stability level of the metric is a responsibility of the component owner, since it
- * involves explicitly acknowledging support for the metric across multiple releases, in accordance with
- * the metric stability policy.
- */
-const (
-	successLabel = "success"
-	failureLabel = "failure"
-	errorLabel   = "error"
-)
-
-var (
-	authenticatedUserCounter = metrics.NewCounterVec(
-		&metrics.CounterOpts{
-			Name:           "authenticated_user_requests",
-			Help:           "Counter of authenticated requests broken out by username.",
-			StabilityLevel: metrics.ALPHA,
-		},
-		[]string{"username"},
-	)
-
-	authenticatedAttemptsCounter = metrics.NewCounterVec(
-		&metrics.CounterOpts{
-			Name:           "authentication_attempts",
-			Help:           "Counter of authenticated attempts.",
-			StabilityLevel: metrics.ALPHA,
-		},
-		[]string{"result"},
-	)
-
-	authenticationLatency = metrics.NewHistogramVec(
-		&metrics.HistogramOpts{
-			Name:           "authentication_duration_seconds",
-			Help:           "Authentication duration in seconds broken out by result.",
-			Buckets:        metrics.ExponentialBuckets(0.001, 2, 15),
-			StabilityLevel: metrics.ALPHA,
-		},
-		[]string{"result"},
-	)
-)
-
-func init() {
-	legacyregistry.MustRegister(authenticatedUserCounter)
-	legacyregistry.MustRegister(authenticatedAttemptsCounter)
-	legacyregistry.MustRegister(authenticationLatency)
-}
-
 // 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
@@ -99,22 +47,18 @@ 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(resp, ok, err, apiAuds, authenticationStart)
 		if err != nil || !ok {
 			if err != nil {
 				klog.Errorf("Unable to authenticate the request due to an error: %v", err)
-				authenticatedAttemptsCounter.WithLabelValues(errorLabel).Inc()
-				authenticationLatency.WithLabelValues(errorLabel).Observe(time.Since(authenticationStart).Seconds())
-			} else if !ok {
-				authenticatedAttemptsCounter.WithLabelValues(failureLabel).Inc()
-				authenticationLatency.WithLabelValues(failureLabel).Observe(time.Since(authenticationStart).Seconds())
 			}
-
 			failed.ServeHTTP(w, req)
 			return
 		}
 
-		if len(apiAuds) > 0 && len(resp.Audiences) > 0 && len(authenticator.Audiences(apiAuds).Intersect(resp.Audiences)) == 0 {
+		if !audiencesAreAcceptable(apiAuds, resp.Audiences) {
-			klog.Errorf("Unable to match the audience: %v , accepted: %v", resp.Audiences, apiAuds)
+			err = fmt.Errorf("unable to match the audience: %v , accepted: %v", resp.Audiences, apiAuds)
+			klog.Error(err)
 			failed.ServeHTTP(w, req)
 			return
 		}
@@ -123,11 +67,6 @@ func WithAuthentication(handler http.Handler, auth authenticator.Request, failed
 		req.Header.Del("Authorization")
 
 		req = req.WithContext(genericapirequest.WithUser(req.Context(), resp.User))
-
-		authenticatedUserCounter.WithLabelValues(compressUsername(resp.User.GetName())).Inc()
-		authenticatedAttemptsCounter.WithLabelValues(successLabel).Inc()
-		authenticationLatency.WithLabelValues(successLabel).Observe(time.Since(authenticationStart).Seconds())
-
 		handler.ServeHTTP(w, req)
 	})
 }
@@ -149,24 +88,10 @@ func Unauthorized(s runtime.NegotiatedSerializer, supportsBasicAuth bool) http.H
 	})
 }
 
-// compressUsername maps all possible usernames onto a small set of categories
+func audiencesAreAcceptable(apiAuds, responseAudiences authenticator.Audiences) bool {
-// of usernames. This is done both to limit the cardinality of the
+	if len(apiAuds) == 0 || len(responseAudiences) == 0 {
-// authorized_user_requests metric, and to avoid pushing actual usernames in the
+		return true
-// metric.
-func compressUsername(username string) string {
-	switch {
-	// Known internal identities.
-	case username == "admin" ||
-		username == "client" ||
-		username == "kube_proxy" ||
-		username == "kubelet" ||
-		username == "system:serviceaccount:kube-system:default":
-		return username
-	// Probably an email address.
-	case strings.Contains(username, "@"):
-		return "email_id"
-	// Anything else (custom service accounts, custom external identities, etc.)
-	default:
-		return "other"
 	}
+
+	return len(apiAuds.Intersect(responseAudiences)) > 0
 }

staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go

@@ -0,0 +1,115 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package filters
+
+import (
+	"strings"
+	"time"
+
+	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/component-base/metrics"
+	"k8s.io/component-base/metrics/legacyregistry"
+)
+
+/*
+ * By default, all the following metrics are defined as falling under
+ * ALPHA stability level https://github.com/kubernetes/enhancements/blob/master/keps/sig-instrumentation/20190404-kubernetes-control-plane-metrics-stability.md#stability-classes)
+ *
+ * Promoting the stability level of the metric is a responsibility of the component owner, since it
+ * involves explicitly acknowledging support for the metric across multiple releases, in accordance with
+ * the metric stability policy.
+ */
+const (
+	successLabel = "success"
+	failureLabel = "failure"
+	errorLabel   = "error"
+)
+
+var (
+	authenticatedUserCounter = metrics.NewCounterVec(
+		&metrics.CounterOpts{
+			Name:           "authenticated_user_requests",
+			Help:           "Counter of authenticated requests broken out by username.",
+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"username"},
+	)
+
+	authenticatedAttemptsCounter = metrics.NewCounterVec(
+		&metrics.CounterOpts{
+			Name:           "authentication_attempts",
+			Help:           "Counter of authenticated attempts.",
+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"result"},
+	)
+
+	authenticationLatency = metrics.NewHistogramVec(
+		&metrics.HistogramOpts{
+			Name:           "authentication_duration_seconds",
+			Help:           "Authentication duration in seconds broken out by result.",
+			Buckets:        metrics.ExponentialBuckets(0.001, 2, 15),
+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"result"},
+	)
+)
+
+func init() {
+	legacyregistry.MustRegister(authenticatedUserCounter)
+	legacyregistry.MustRegister(authenticatedAttemptsCounter)
+	legacyregistry.MustRegister(authenticationLatency)
+}
+
+func recordAuthMetrics(resp *authenticator.Response, ok bool, err error, apiAudiences authenticator.Audiences, authStart time.Time) {
+	var resultLabel string
+
+	switch {
+	case err != nil || (resp != nil && !audiencesAreAcceptable(apiAudiences, resp.Audiences)):
+		resultLabel = errorLabel
+	case !ok:
+		resultLabel = failureLabel
+	default:
+		resultLabel = successLabel
+		authenticatedUserCounter.WithLabelValues(compressUsername(resp.User.GetName())).Inc()
+	}
+
+	authenticatedAttemptsCounter.WithLabelValues(resultLabel).Inc()
+	authenticationLatency.WithLabelValues(resultLabel).Observe(time.Since(authStart).Seconds())
+}
+
+// compressUsername maps all possible usernames onto a small set of categories
+// of usernames. This is done both to limit the cardinality of the
+// authorized_user_requests metric, and to avoid pushing actual usernames in the
+// metric.
+func compressUsername(username string) string {
+	switch {
+	// Known internal identities.
+	case username == "admin" ||
+		username == "client" ||
+		username == "kube_proxy" ||
+		username == "kubelet" ||
+		username == "system:serviceaccount:kube-system:default":
+		return username
+	// Probably an email address.
+	case strings.Contains(username, "@"):
+		return "email_id"
+	// Anything else (custom service accounts, custom external identities, etc.)
+	default:
+		return "other"
+	}
+}

staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics_test.go

@@ -0,0 +1,159 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package filters
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/component-base/metrics/legacyregistry"
+	"k8s.io/component-base/metrics/testutil"
+)
+
+func TestMetrics(t *testing.T) {
+	// Excluding authentication_duration_seconds since it is difficult to predict its values.
+	metrics := []string{
+		"authenticated_user_requests",
+		"authentication_attempts",
+	}
+
+	testCases := []struct {
+		desc        string
+		response    *authenticator.Response
+		status      bool
+		err         error
+		apiAudience authenticator.Audiences
+		want        string
+	}{
+		{
+			desc: "auth ok",
+			response: &authenticator.Response{
+				User: &user.DefaultInfo{Name: "admin"},
+			},
+			status: true,
+			want: `
+				# HELP authenticated_user_requests [ALPHA] Counter of authenticated requests broken out by username.
+				# TYPE authenticated_user_requests counter
+				authenticated_user_requests{username="admin"} 1
+        # HELP authentication_attempts [ALPHA] Counter of authenticated attempts.
+        # TYPE authentication_attempts counter
+        authentication_attempts{result="success"} 1
+				`,
+		},
+		{
+			desc: "auth failed with error",
+			err:  errors.New("some error"),
+			want: `
+        # HELP authentication_attempts [ALPHA] Counter of authenticated attempts.
+        # TYPE authentication_attempts counter
+        authentication_attempts{result="error"} 1
+				`,
+		},
+		{
+			desc: "auth failed with status false",
+			want: `
+        # HELP authentication_attempts [ALPHA] Counter of authenticated attempts.
+        # TYPE authentication_attempts counter
+        authentication_attempts{result="failure"} 1
+				`,
+		},
+		{
+			desc: "auth failed due to audiences not intersecting",
+			response: &authenticator.Response{
+				User:      &user.DefaultInfo{Name: "admin"},
+				Audiences: authenticator.Audiences{"audience-x"},
+			},
+			status:      true,
+			apiAudience: authenticator.Audiences{"audience-y"},
+			want: `
+        # HELP authentication_attempts [ALPHA] Counter of authenticated attempts.
+        # TYPE authentication_attempts counter
+        authentication_attempts{result="error"} 1
+				`,
+		},
+		{
+			desc: "audiences not supplied in the response",
+			response: &authenticator.Response{
+				User: &user.DefaultInfo{Name: "admin"},
+			},
+			status:      true,
+			apiAudience: authenticator.Audiences{"audience-y"},
+			want: `
+        # HELP authenticated_user_requests [ALPHA] Counter of authenticated requests broken out by username.
+				# TYPE authenticated_user_requests counter
+				authenticated_user_requests{username="admin"} 1
+        # HELP authentication_attempts [ALPHA] Counter of authenticated attempts.
+        # TYPE authentication_attempts counter
+        authentication_attempts{result="success"} 1
+				`,
+		},
+		{
+			desc: "audiences not supplied to the handler",
+			response: &authenticator.Response{
+				User:      &user.DefaultInfo{Name: "admin"},
+				Audiences: authenticator.Audiences{"audience-x"},
+			},
+			status: true,
+			want: `
+        # HELP authenticated_user_requests [ALPHA] Counter of authenticated requests broken out by username.
+				# TYPE authenticated_user_requests counter
+				authenticated_user_requests{username="admin"} 1
+        # HELP authentication_attempts [ALPHA] Counter of authenticated attempts.
+        # TYPE authentication_attempts counter
+        authentication_attempts{result="success"} 1
+				`,
+		},
+	}
+
+	// Since prometheus' gatherer is global, other tests may have updated metrics already, so
+	// we need to reset them prior running this test.
+	// This also implies that we can't run this test in parallel with other auth tests.
+	authenticatedUserCounter.Reset()
+	authenticatedAttemptsCounter.Reset()
+
+	for _, tt := range testCases {
+		t.Run(tt.desc, func(t *testing.T) {
+			defer authenticatedUserCounter.Reset()
+			defer authenticatedAttemptsCounter.Reset()
+			done := make(chan struct{})
+			auth := WithAuthentication(
+				http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
+					close(done)
+				}),
+				authenticator.RequestFunc(func(_ *http.Request) (*authenticator.Response, bool, error) {
+					return tt.response, tt.status, tt.err
+				}),
+				http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
+					close(done)
+				}),
+				tt.apiAudience,
+			)
+
+			auth.ServeHTTP(httptest.NewRecorder(), &http.Request{})
+			<-done
+
+			if err := testutil.GatherAndCompare(legacyregistry.DefaultGatherer, strings.NewReader(tt.want), metrics...); err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}