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 a91136599c6..6ce0a3eab24 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 8de05fd42f4..ad9492a5868 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 750e8b4eae4..1da12201175 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 d8e18345545..74e583a2af4 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 ec5af39d8bc..46b05a2f8e6 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 e92e957a4d3..e87303d4549 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 fd4af99aa55..fc926d1f75c 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 90104df5494..f462276f5bb 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 bd2d00bf722..2544e2429d1 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 a4825b52a6e..fa1136f5fb7 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 51954a6bbce..d989a7b799e 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 6d4d735bdab..16fbf4f01b9 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 067fb0b6ebe..89ece409cf8 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 {