Merge pull request kubernetes#70449 from mikedanese/simplcache

remove webhook cache implementation and replace with token cache
joerocklin · Nov 6, 2018 · c860465 · c860465
2 parents c86944b + 0ec4d6d
commit c860465
Show file tree

Hide file tree

Showing 13 changed files with 68 additions and 46 deletions.
diff --git a/pkg/kubeapiserver/authenticator/config.go b/pkg/kubeapiserver/authenticator/config.go
@@ -178,7 +178,7 @@ func (config AuthenticatorConfig) New() (authenticator.Request, *spec.SecurityDe
 		tokenAuth := tokenunion.New(tokenAuthenticators...)
 		// Optionally cache authentication results
 		if config.TokenSuccessCacheTTL > 0 || config.TokenFailureCacheTTL > 0 {
-			tokenAuth = tokencache.New(tokenAuth, config.TokenSuccessCacheTTL, config.TokenFailureCacheTTL)
+			tokenAuth = tokencache.New(tokenAuth, true, config.TokenSuccessCacheTTL, config.TokenFailureCacheTTL)
 		}
 		authenticators = append(authenticators, bearertoken.New(tokenAuth), websocket.NewProtocolAuthenticator(tokenAuth))
 		securityDefinitions["BearerToken"] = &spec.SecurityScheme{
@@ -316,10 +316,10 @@ func newAuthenticatorFromClientCAFile(clientCAFile string) (authenticator.Reques
 }
 
 func newWebhookTokenAuthenticator(webhookConfigFile string, ttl time.Duration) (authenticator.Token, error) {
-	webhookTokenAuthenticator, err := webhook.New(webhookConfigFile, ttl)
+	webhookTokenAuthenticator, err := webhook.New(webhookConfigFile)
 	if err != nil {
 		return nil, err
 	}
 
-	return webhookTokenAuthenticator, nil
+	return tokencache.New(webhookTokenAuthenticator, false, ttl, ttl), nil
 }
diff --git a/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json b/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/BUILD b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/BUILD
@@ -23,6 +23,7 @@ go_library(
         "//staging/src/k8s.io/apiserver/pkg/authentication/request/union:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/request/websocket:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/request/x509:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authentication/token/cache:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/token/tokenfile:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook:go_default_library",

diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go
@@ -31,6 +31,7 @@ import (
 	unionauth "k8s.io/apiserver/pkg/authentication/request/union"
 	"k8s.io/apiserver/pkg/authentication/request/websocket"
 	"k8s.io/apiserver/pkg/authentication/request/x509"
+	"k8s.io/apiserver/pkg/authentication/token/cache"
 	webhooktoken "k8s.io/apiserver/plugin/pkg/authenticator/token/webhook"
 	authenticationclient "k8s.io/client-go/kubernetes/typed/authentication/v1beta1"
 	"k8s.io/client-go/util/cert"
@@ -85,11 +86,12 @@ func (c DelegatingAuthenticatorConfig) New() (authenticator.Request, *spec.Secur
 	}
 
 	if c.TokenAccessReviewClient != nil {
-		tokenAuth, err := webhooktoken.NewFromInterface(c.TokenAccessReviewClient, c.CacheTTL)
+		tokenAuth, err := webhooktoken.NewFromInterface(c.TokenAccessReviewClient)
 		if err != nil {
 			return nil, nil, err
 		}
-		authenticators = append(authenticators, bearertoken.New(tokenAuth), websocket.NewProtocolAuthenticator(tokenAuth))
+		cachingTokenAuth := cache.New(tokenAuth, false, c.CacheTTL, c.CacheTTL)
+		authenticators = append(authenticators, bearertoken.New(cachingTokenAuth), websocket.NewProtocolAuthenticator(cachingTokenAuth))
 
 		securityDefinitions["BearerToken"] = &spec.SecurityScheme{
 			SecuritySchemeProps: spec.SecuritySchemeProps{

diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go
@@ -35,6 +35,7 @@ type cacheRecord struct {
 type cachedTokenAuthenticator struct {
 	authenticator authenticator.Token
 
+	cacheErrs  bool
 	successTTL time.Duration
 	failureTTL time.Duration
 
@@ -51,13 +52,14 @@ type cache interface {
 }
 
 // New returns a token authenticator that caches the results of the specified authenticator. A ttl of 0 bypasses the cache.
-func New(authenticator authenticator.Token, successTTL, failureTTL time.Duration) authenticator.Token {
-	return newWithClock(authenticator, successTTL, failureTTL, utilclock.RealClock{})
+func New(authenticator authenticator.Token, cacheErrs bool, successTTL, failureTTL time.Duration) authenticator.Token {
+	return newWithClock(authenticator, cacheErrs, successTTL, failureTTL, utilclock.RealClock{})
 }
 
-func newWithClock(authenticator authenticator.Token, successTTL, failureTTL time.Duration, clock utilclock.Clock) authenticator.Token {
+func newWithClock(authenticator authenticator.Token, cacheErrs bool, successTTL, failureTTL time.Duration, clock utilclock.Clock) authenticator.Token {
 	return &cachedTokenAuthenticator{
 		authenticator: authenticator,
+		cacheErrs:     cacheErrs,
 		successTTL:    successTTL,
 		failureTTL:    failureTTL,
 		cache:         newStripedCache(32, fnvHashFunc, func() cache { return newSimpleCache(128, clock) }),
@@ -74,6 +76,9 @@ func (a *cachedTokenAuthenticator) AuthenticateToken(ctx context.Context, token
 	}
 
 	resp, ok, err := a.authenticator.AuthenticateToken(ctx, token)
+	if !a.cacheErrs && err != nil {
+		return resp, ok, err
+	}
 
 	switch {
 	case ok && a.successTTL > 0:

diff --git a/...ng/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go b/...ng/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go
@@ -41,7 +41,7 @@ func TestCachedTokenAuthenticator(t *testing.T) {
 	})
 	fakeClock := utilclock.NewFakeClock(time.Now())
 
-	a := newWithClock(fakeAuth, time.Minute, 0, fakeClock)
+	a := newWithClock(fakeAuth, true, time.Minute, 0, fakeClock)
 
 	calledWithToken, resultUsers, resultOk, resultErr = []string{}, nil, false, nil
 	a.AuthenticateToken(context.Background(), "bad1")
@@ -113,7 +113,7 @@ func TestCachedTokenAuthenticatorWithAudiences(t *testing.T) {
 	})
 	fakeClock := utilclock.NewFakeClock(time.Now())
 
-	a := newWithClock(fakeAuth, time.Minute, 0, fakeClock)
+	a := newWithClock(fakeAuth, true, time.Minute, 0, fakeClock)
 
 	resultUsers["audAusertoken1"] = &user.DefaultInfo{Name: "user1"}
 	resultUsers["audBusertoken1"] = &user.DefaultInfo{Name: "user1-different"}

diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/BUILD b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/BUILD
@@ -16,6 +16,8 @@ go_test(
     deps = [
         "//staging/src/k8s.io/api/authentication/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authentication/token/cache:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/client-go/tools/clientcmd/api/v1:go_default_library",
     ],
@@ -30,7 +32,6 @@ go_library(
         "//staging/src/k8s.io/api/authentication/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
-        "//staging/src/k8s.io/apimachinery/pkg/util/cache:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library",

diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go
@@ -26,7 +26,6 @@ import (
 	authentication "k8s.io/api/authentication/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/util/cache"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/util/webhook"
@@ -45,54 +44,50 @@ var _ authenticator.Token = (*WebhookTokenAuthenticator)(nil)
 
 type WebhookTokenAuthenticator struct {
 	tokenReview    authenticationclient.TokenReviewInterface
-	responseCache  *cache.LRUExpireCache
-	ttl            time.Duration
 	initialBackoff time.Duration
 }
 
-// NewFromInterface creates a webhook authenticator using the given tokenReview client
-func NewFromInterface(tokenReview authenticationclient.TokenReviewInterface, ttl time.Duration) (*WebhookTokenAuthenticator, error) {
-	return newWithBackoff(tokenReview, ttl, retryBackoff)
+// NewFromInterface creates a webhook authenticator using the given tokenReview
+// client. It is recommend to wrap this authenticator with the token cache
+// authenticator implemented in
+// k8s.io/apiserver/pkg/authentication/token/cache.
+func NewFromInterface(tokenReview authenticationclient.TokenReviewInterface) (*WebhookTokenAuthenticator, error) {
+	return newWithBackoff(tokenReview, retryBackoff)
 }
 
 // New creates a new WebhookTokenAuthenticator from the provided kubeconfig file.
-func New(kubeConfigFile string, ttl time.Duration) (*WebhookTokenAuthenticator, error) {
+func New(kubeConfigFile string) (*WebhookTokenAuthenticator, error) {
 	tokenReview, err := tokenReviewInterfaceFromKubeconfig(kubeConfigFile)
 	if err != nil {
 		return nil, err
 	}
-	return newWithBackoff(tokenReview, ttl, retryBackoff)
+	return newWithBackoff(tokenReview, retryBackoff)
 }
 
 // newWithBackoff allows tests to skip the sleep.
-func newWithBackoff(tokenReview authenticationclient.TokenReviewInterface, ttl, initialBackoff time.Duration) (*WebhookTokenAuthenticator, error) {
-	return &WebhookTokenAuthenticator{tokenReview, cache.NewLRUExpireCache(1024), ttl, initialBackoff}, nil
+func newWithBackoff(tokenReview authenticationclient.TokenReviewInterface, initialBackoff time.Duration) (*WebhookTokenAuthenticator, error) {
+	return &WebhookTokenAuthenticator{tokenReview, initialBackoff}, nil
 }
 
 // AuthenticateToken implements the authenticator.Token interface.
 func (w *WebhookTokenAuthenticator) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) {
 	r := &authentication.TokenReview{
 		Spec: authentication.TokenReviewSpec{Token: token},
 	}
-	if entry, ok := w.responseCache.Get(r.Spec); ok {
-		r.Status = entry.(authentication.TokenReviewStatus)
-	} else {
-		var (
-			result *authentication.TokenReview
-			err    error
-		)
-		webhook.WithExponentialBackoff(w.initialBackoff, func() error {
-			result, err = w.tokenReview.Create(r)
-			return err
-		})
-		if err != nil {
-			// An error here indicates bad configuration or an outage. Log for debugging.
-			glog.Errorf("Failed to make webhook authenticator request: %v", err)
-			return nil, false, err
-		}
-		r.Status = result.Status
-		w.responseCache.Add(r.Spec, result.Status, w.ttl)
+	var (
+		result *authentication.TokenReview
+		err    error
+	)
+	webhook.WithExponentialBackoff(w.initialBackoff, func() error {
+		result, err = w.tokenReview.Create(r)
+		return err
+	})
+	if err != nil {
+		// An error here indicates bad configuration or an outage. Log for debugging.
+		glog.Errorf("Failed to make webhook authenticator request: %v", err)
+		return nil, false, err
 	}
+	r.Status = result.Status
 	if !r.Status.Authenticated {
 		return nil, false, nil
 	}

diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_test.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_test.go
@@ -33,6 +33,8 @@ import (
 
 	"k8s.io/api/authentication/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/apiserver/pkg/authentication/token/cache"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/client-go/tools/clientcmd/api/v1"
 )
@@ -166,7 +168,7 @@ func (m *mockService) HTTPStatusCode() int { return m.statusCode }
 
 // newTokenAuthenticator creates a temporary kubeconfig file from the provided
 // arguments and attempts to load a new WebhookTokenAuthenticator from it.
-func newTokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte, cacheTime time.Duration) (*WebhookTokenAuthenticator, error) {
+func newTokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte, cacheTime time.Duration) (authenticator.Token, error) {
 	tempfile, err := ioutil.TempFile("", "")
 	if err != nil {
 		return nil, err
@@ -194,7 +196,12 @@ func newTokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte, c
 		return nil, err
 	}
 
-	return newWithBackoff(c, cacheTime, 0)
+	authn, err := newWithBackoff(c, 0)
+	if err != nil {
+		return nil, err
+	}
+
+	return cache.New(authn, false, cacheTime, cacheTime), nil
 }
 
 func TestTLSConfig(t *testing.T) {
@@ -549,15 +556,12 @@ func TestWebhookCacheAndRetry(t *testing.T) {
 			_, ok, err := wh.AuthenticateToken(context.Background(), testcase.token)
 			hasError := err != nil
 			if hasError != testcase.expectError {
-				t.Log(testcase.description)
 				t.Errorf("Webhook returned HTTP %d, expected error=%v, but got error %v", testcase.code, testcase.expectError, err)
 			}
 			if serv.called != testcase.expectCalls {
-				t.Log(testcase.description)
 				t.Errorf("Expected %d calls, got %d", testcase.expectCalls, serv.called)
 			}
 			if ok != testcase.expectOk {
-				t.Log(testcase.description)
 				t.Errorf("Expected ok=%v, got %v", testcase.expectOk, ok)
 			}
 		})

diff --git a/staging/src/k8s.io/kube-aggregator/Godeps/Godeps.json b/staging/src/k8s.io/kube-aggregator/Godeps/Godeps.json
diff --git a/staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json b/staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json
diff --git a/test/integration/auth/BUILD b/test/integration/auth/BUILD
@@ -62,6 +62,7 @@ go_test(
         "//staging/src/k8s.io/apiserver/pkg/authentication/group:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/request/bearertoken:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authentication/token/cache:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/token/tokenfile:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",

diff --git a/test/integration/auth/auth_test.go b/test/integration/auth/auth_test.go
@@ -40,6 +40,7 @@ import (
 	"k8s.io/apiserver/pkg/authentication/group"
 	"k8s.io/apiserver/pkg/authentication/request/bearertoken"
 	"k8s.io/apiserver/pkg/authentication/serviceaccount"
+	"k8s.io/apiserver/pkg/authentication/token/cache"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/authorization/authorizerfactory"
@@ -84,11 +85,11 @@ func getTestWebhookTokenAuth(serverURL string) (authenticator.Request, error) {
 	if err := json.NewEncoder(kubecfgFile).Encode(config); err != nil {
 		return nil, err
 	}
-	webhookTokenAuth, err := webhook.New(kubecfgFile.Name(), 2*time.Minute)
+	webhookTokenAuth, err := webhook.New(kubecfgFile.Name())
 	if err != nil {
 		return nil, err
 	}
-	return bearertoken.New(webhookTokenAuth), nil
+	return bearertoken.New(cache.New(webhookTokenAuth, false, 2*time.Minute, 2*time.Minute)), nil
 }
 
 func path(resource, namespace, name string) string {