From 0ec4d6d396f237ccb3ae0e96922a90600befb83d Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Tue, 30 Oct 2018 12:41:46 -0700 Subject: [PATCH] remove webhook cache implementation and replace with the token cache The striped cache used by the token cache is slightly more sophisticated however the simple cache provides about the same exact behavior. I used the striped cache rather than the simple cache because: * It has been used without issue as the primary token cache. * It preforms better under load. * It is already exposed in the public API of the token cache package. --- pkg/kubeapiserver/authenticator/config.go | 6 +-- .../Godeps/Godeps.json | 4 ++ .../authentication/authenticatorfactory/BUILD | 1 + .../authenticatorfactory/delegating.go | 6 ++- .../token/cache/cached_token_authenticator.go | 11 ++-- .../cache/cached_token_authenticator_test.go | 4 +- .../pkg/authenticator/token/webhook/BUILD | 3 +- .../authenticator/token/webhook/webhook.go | 51 +++++++++---------- .../token/webhook/webhook_test.go | 14 +++-- .../k8s.io/kube-aggregator/Godeps/Godeps.json | 4 ++ .../sample-apiserver/Godeps/Godeps.json | 4 ++ test/integration/auth/BUILD | 1 + test/integration/auth/auth_test.go | 5 +- 13 files changed, 68 insertions(+), 46 deletions(-) diff --git a/pkg/kubeapiserver/authenticator/config.go b/pkg/kubeapiserver/authenticator/config.go index a91136599c618..6ce0a3eab241f 100644 --- a/pkg/kubeapiserver/authenticator/config.go +++ b/pkg/kubeapiserver/authenticator/config.go @@ -179,7 +179,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{ @@ -317,10 +317,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 index 8de05fd42f47b..ad9492a58680b 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json +++ b/staging/src/k8s.io/apiextensions-apiserver/Godeps/Godeps.json @@ -1354,6 +1354,10 @@ "ImportPath": "k8s.io/apiserver/pkg/authentication/serviceaccount", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" }, + { + "ImportPath": "k8s.io/apiserver/pkg/authentication/token/cache", + "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + }, { "ImportPath": "k8s.io/apiserver/pkg/authentication/token/tokenfile", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/BUILD b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/BUILD index 750e8b4eae4c6..1da12201175f8 100644 --- 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 index d8e18345545eb..74e583a2af435 100644 --- 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 index ec5af39d8bcec..46b05a2f8e614 100644 --- 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 @@ -36,6 +36,7 @@ type cacheRecord struct { type cachedTokenAuthenticator struct { authenticator authenticator.Token + cacheErrs bool successTTL time.Duration failureTTL time.Duration @@ -52,13 +53,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) }), @@ -75,6 +77,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/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go index e92e957a4d3ac..e87303d454943 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go @@ -42,7 +42,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") @@ -114,7 +114,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 index fd4af99aa5517..fc926d1f75c71 100644 --- 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 index 90104df54943a..f462276f5bbb7 100644 --- 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,28 +44,29 @@ 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. @@ -74,25 +74,20 @@ func (w *WebhookTokenAuthenticator) AuthenticateToken(ctx context.Context, token 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 index bd2d00bf72240..2544e2429d137 100644 --- 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 index a4825b52a6e13..fa1136f5fb761 100644 --- a/staging/src/k8s.io/kube-aggregator/Godeps/Godeps.json +++ b/staging/src/k8s.io/kube-aggregator/Godeps/Godeps.json @@ -1006,6 +1006,10 @@ "ImportPath": "k8s.io/apiserver/pkg/authentication/serviceaccount", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" }, + { + "ImportPath": "k8s.io/apiserver/pkg/authentication/token/cache", + "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + }, { "ImportPath": "k8s.io/apiserver/pkg/authentication/token/tokenfile", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" diff --git a/staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json b/staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json index 51954a6bbceb9..d989a7b799ebd 100644 --- a/staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json +++ b/staging/src/k8s.io/sample-apiserver/Godeps/Godeps.json @@ -970,6 +970,10 @@ "ImportPath": "k8s.io/apiserver/pkg/authentication/serviceaccount", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" }, + { + "ImportPath": "k8s.io/apiserver/pkg/authentication/token/cache", + "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + }, { "ImportPath": "k8s.io/apiserver/pkg/authentication/token/tokenfile", "Rev": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" diff --git a/test/integration/auth/BUILD b/test/integration/auth/BUILD index 6d4d735bdab89..16fbf4f01b998 100644 --- a/test/integration/auth/BUILD +++ b/test/integration/auth/BUILD @@ -63,6 +63,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 index 067fb0b6ebec1..89ece409cf81e 100644 --- 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 {