From e0ee18a993c955b068c70216d285ee526dd91319 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Tue, 8 Dec 2020 11:08:53 -0500 Subject: [PATCH] Always close JWTAuthenticator underlying authenticator Otherwise we will leak goroutines. Signed-off-by: Andrew Keesler --- .../controller/authenticator/authenticator.go | 8 +++ .../cachecleaner/cachecleaner.go | 11 +-- .../cachecleaner/cachecleaner_test.go | 51 ++++++-------- .../jwtcachefiller/jwtcachefiller.go | 22 ++++-- .../jwtcachefiller/jwtcachefiller_test.go | 49 +++++++++++++- .../mocktokenauthenticatorcloser/generate.go | 21 ++++++ .../mocktokenauthenticatorcloser.go | 67 +++++++++++++++++++ 7 files changed, 183 insertions(+), 46 deletions(-) create mode 100644 internal/mocks/mocktokenauthenticatorcloser/generate.go create mode 100644 internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go diff --git a/internal/controller/authenticator/authenticator.go b/internal/controller/authenticator/authenticator.go index 16075ab54..0e412fc13 100644 --- a/internal/controller/authenticator/authenticator.go +++ b/internal/controller/authenticator/authenticator.go @@ -10,6 +10,14 @@ import ( auth1alpha1 "go.pinniped.dev/generated/1.19/apis/concierge/authentication/v1alpha1" ) +// Closer is a type that can be closed impotently. +// +// This type is slightly different from io.Closer, because io.Closer can return an error and is not +// necessarily idempotent. +type Closer interface { + Close() +} + // CABundle returns a PEM-encoded CA bundle from the provided spec. If the provided spec is nil, a // nil CA bundle will be returned. If the provided spec contains a CA bundle that is not properly // encoded, an error will be returned. diff --git a/internal/controller/authenticator/cachecleaner/cachecleaner.go b/internal/controller/authenticator/cachecleaner/cachecleaner.go index ab762536a..f400117a6 100644 --- a/internal/controller/authenticator/cachecleaner/cachecleaner.go +++ b/internal/controller/authenticator/cachecleaner/cachecleaner.go @@ -14,16 +14,11 @@ import ( auth1alpha1 "go.pinniped.dev/generated/1.19/apis/concierge/authentication/v1alpha1" authinformers "go.pinniped.dev/generated/1.19/client/concierge/informers/externalversions/authentication/v1alpha1" pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controller/authenticator" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" ) -// closable is used to detect when a cache value has a Close() method on it. We use this to -// determine if we should call the Close() method on a cache value upon deleting it from the cache. -type closable interface { - Close() -} - // New instantiates a new controllerlib.Controller which will garbage collect authenticators from the provided Cache. func New( cache *authncache.Cache, @@ -108,8 +103,8 @@ func (c *controller) Sync(_ controllerlib.Context) error { ).Info("deleting authenticator from cache") value := c.cache.Get(key) - if closable, ok := value.(closable); ok { - closable.Close() + if closer, ok := value.(authenticator.Closer); ok { + closer.Close() } c.cache.Delete(key) diff --git a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go index 9e5cd147a..db1038a73 100644 --- a/internal/controller/authenticator/cachecleaner/cachecleaner_test.go +++ b/internal/controller/authenticator/cachecleaner/cachecleaner_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -17,6 +18,7 @@ import ( pinnipedinformers "go.pinniped.dev/generated/1.19/client/concierge/informers/externalversions" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser" "go.pinniped.dev/internal/testutil/testlogger" ) @@ -57,16 +59,16 @@ func TestController(t *testing.T) { tests := []struct { name string objects []runtime.Object - initialCache map[authncache.Key]authncache.Value + initialCache func(t *testing.T, cache *authncache.Cache) wantErr string wantLogs []string wantCacheKeys []authncache.Key }{ { name: "no change", - initialCache: map[authncache.Key]authncache.Value{ - testWebhookKey1: nil, - testJWTAuthenticatorKey1: nil, + initialCache: func(t *testing.T, cache *authncache.Cache) { + cache.Store(testWebhookKey1, nil) + cache.Store(testJWTAuthenticatorKey1, nil) }, objects: []runtime.Object{ &authv1alpha.WebhookAuthenticator{ @@ -85,8 +87,7 @@ func TestController(t *testing.T) { wantCacheKeys: []authncache.Key{testWebhookKey1, testJWTAuthenticatorKey1}, }, { - name: "authenticators not yet added", - initialCache: nil, + name: "authenticators not yet added", objects: []runtime.Object{ &authv1alpha.WebhookAuthenticator{ ObjectMeta: metav1.ObjectMeta{ @@ -117,12 +118,12 @@ func TestController(t *testing.T) { }, { name: "successful cleanup", - initialCache: map[authncache.Key]authncache.Value{ - testWebhookKey1: nil, - testWebhookKey2: nil, - testJWTAuthenticatorKey1: newClosableCacheValue(t, "closable1", 0), - testJWTAuthenticatorKey2: newClosableCacheValue(t, "closable2", 1), - testKeyUnknownType: nil, + initialCache: func(t *testing.T, cache *authncache.Cache) { + cache.Store(testWebhookKey1, nil) + cache.Store(testWebhookKey2, nil) + cache.Store(testJWTAuthenticatorKey1, newClosableCacheValue(t, 0)) + cache.Store(testJWTAuthenticatorKey2, newClosableCacheValue(t, 1)) + cache.Store(testKeyUnknownType, nil) }, objects: []runtime.Object{ &authv1alpha.WebhookAuthenticator{ @@ -153,8 +154,8 @@ func TestController(t *testing.T) { fakeClient := pinnipedfake.NewSimpleClientset(tt.objects...) informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) cache := authncache.New() - for k, v := range tt.initialCache { - cache.Store(k, v) + if tt.initialCache != nil { + tt.initialCache(t, cache) } testLog := testlogger.New(t) @@ -188,20 +189,10 @@ func TestController(t *testing.T) { } } -func newClosableCacheValue(t *testing.T, name string, wantCloses int) authncache.Value { - t.Helper() - c := &closableCacheValue{} - t.Cleanup(func() { - require.Equalf(t, wantCloses, c.closeCallCount, "expected %s.Close() to be called %d times", name, wantCloses) - }) - return c -} - -type closableCacheValue struct { - authncache.Value - closeCallCount int -} - -func (c *closableCacheValue) Close() { - c.closeCallCount++ +func newClosableCacheValue(t *testing.T, wantCloses int) authncache.Value { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + tac := mocktokenauthenticatorcloser.NewMockTokenAuthenticatorCloser(ctrl) + tac.EXPECT().Close().Times(wantCloses) + return tac } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 3028fbf88..ff53b9c75 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -85,17 +85,27 @@ func (c *controller) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to get JWTAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) } + cacheKey := authncache.Key{ + APIGroup: auth1alpha1.GroupName, + Kind: "JWTAuthenticator", + Namespace: ctx.Key.Namespace, + Name: ctx.Key.Name, + } + + // If this authenticator already exists, then we gotta make sure we close the old authenticator so + // we don't leak goroutines. + if value := c.cache.Get(cacheKey); value != nil { + if closer, ok := value.(authenticator.Closer); ok { + closer.Close() + } + } + jwtAuthenticator, err := newJWTAuthenticator(&obj.Spec) if err != nil { return fmt.Errorf("failed to build jwt authenticator: %w", err) } - c.cache.Store(authncache.Key{ - APIGroup: auth1alpha1.GroupName, - Kind: "JWTAuthenticator", - Namespace: ctx.Key.Namespace, - Name: ctx.Key.Name, - }, jwtAuthenticator) + c.cache.Store(cacheKey, jwtAuthenticator) c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer).Info("added new jwt authenticator") return nil } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index c5d453f27..10823c02e 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -21,6 +21,7 @@ import ( "gopkg.in/square/go-jose.v2/jwt" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,6 +34,7 @@ import ( pinnipedinformers "go.pinniped.dev/generated/1.19/client/concierge/informers/externalversions" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser" "go.pinniped.dev/internal/testutil/testlogger" ) @@ -41,6 +43,7 @@ func TestController(t *testing.T) { tests := []struct { name string + cache func(*authncache.Cache) syncKey controllerlib.Key jwtAuthenticators []runtime.Object wantErr string @@ -75,6 +78,38 @@ func TestController(t *testing.T) { }, wantCacheEntries: 1, }, + { + name: "updating jwt authenticator closes previous instance", + cache: func(cache *authncache.Cache) { + cache.Store( + authncache.Key{ + Name: "test-name", + Namespace: "test-namespace", + Kind: "JWTAuthenticator", + APIGroup: auth1alpha1.SchemeGroupVersion.Group, + }, + newClosableCacheValue(t, 1), + ) + }, + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: auth1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://some-issuer.com", + Audience: "some-audience", + TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURVVENDQWptZ0F3SUJBZ0lWQUpzNStTbVRtaTJXeUI0bGJJRXBXaUs5a1RkUE1BMEdDU3FHU0liM0RRRUIKQ3dVQU1COHhDekFKQmdOVkJBWVRBbFZUTVJBd0RnWURWUVFLREFkUWFYWnZkR0ZzTUI0WERUSXdNRFV3TkRFMgpNamMxT0ZvWERUSTBNRFV3TlRFMk1qYzFPRm93SHpFTE1Ba0dBMVVFQmhNQ1ZWTXhFREFPQmdOVkJBb01CMUJwCmRtOTBZV3d3Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQkR3QXdnZ0VLQW9JQkFRRERZWmZvWGR4Z2NXTEMKZEJtbHB5a0tBaG9JMlBuUWtsVFNXMno1cGcwaXJjOGFRL1E3MXZzMTRZYStmdWtFTGlvOTRZYWw4R01DdVFrbApMZ3AvUEE5N1VYelhQNDBpK25iNXcwRGpwWWd2dU9KQXJXMno2MFRnWE5NSFh3VHk4ME1SZEhpUFVWZ0VZd0JpCmtkNThzdEFVS1Y1MnBQTU1reTJjNy9BcFhJNmRXR2xjalUvaFBsNmtpRzZ5dEw2REtGYjJQRWV3MmdJM3pHZ2IKOFVVbnA1V05DZDd2WjNVY0ZHNXlsZEd3aGc3cnZ4U1ZLWi9WOEhCMGJmbjlxamlrSVcxWFM4dzdpUUNlQmdQMApYZWhKZmVITlZJaTJtZlczNlVQbWpMdnVKaGpqNDIrdFBQWndvdDkzdWtlcEgvbWpHcFJEVm9wamJyWGlpTUYrCkYxdnlPNGMxQWdNQkFBR2pnWU13Z1lBd0hRWURWUjBPQkJZRUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1IKTUI4R0ExVWRJd1FZTUJhQUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1JNQjBHQTFVZEpRUVdNQlFHQ0NzRwpBUVVGQndNQ0JnZ3JCZ0VGQlFjREFUQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BNEdBMVVkRHdFQi93UUVBd0lCCkJqQU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFYbEh4M2tIMDZwY2NDTDlEVE5qTnBCYnlVSytGd2R6T2IwWFYKcmpNaGtxdHVmdEpUUnR5T3hKZ0ZKNXhUR3pCdEtKamcrVU1pczBOV0t0VDBNWThVMU45U2c5SDl0RFpHRHBjVQpxMlVRU0Y4dXRQMVR3dnJIUzIrdzB2MUoxdHgrTEFiU0lmWmJCV0xXQ21EODUzRlVoWlFZekkvYXpFM28vd0p1CmlPUklMdUpNUk5vNlBXY3VLZmRFVkhaS1RTWnk3a25FcHNidGtsN3EwRE91eUFWdG9HVnlkb3VUR0FOdFhXK2YKczNUSTJjKzErZXg3L2RZOEJGQTFzNWFUOG5vZnU3T1RTTzdiS1kzSkRBUHZOeFQzKzVZUXJwNGR1Nmh0YUFMbAppOHNaRkhidmxpd2EzdlhxL3p1Y2JEaHEzQzBhZnAzV2ZwRGxwSlpvLy9QUUFKaTZLQT09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K"}, + }, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="https://some-issuer.com" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + }, { name: "valid jwt authenticator without CA", syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, @@ -124,6 +159,10 @@ func TestController(t *testing.T) { cache := authncache.New() testLog := testlogger.New(t) + if tt.cache != nil { + tt.cache(cache) + } + controller := New(cache, informers.Authentication().V1alpha1().JWTAuthenticators(), testLog) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) @@ -432,7 +471,13 @@ func createJWT( jwt, err := builder.CompactSerialize() require.NoError(t, err) - t.Log("andrew:", jwt) - return jwt } + +func newClosableCacheValue(t *testing.T, wantCloses int) authncache.Value { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + tac := mocktokenauthenticatorcloser.NewMockTokenAuthenticatorCloser(ctrl) + tac.EXPECT().Close().Times(wantCloses) + return tac +} diff --git a/internal/mocks/mocktokenauthenticatorcloser/generate.go b/internal/mocks/mocktokenauthenticatorcloser/generate.go new file mode 100644 index 000000000..878ae5133 --- /dev/null +++ b/internal/mocks/mocktokenauthenticatorcloser/generate.go @@ -0,0 +1,21 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mocktokenauthenticatorcloser + +import ( + "k8s.io/apiserver/pkg/authentication/authenticator" + + pinnipedauthenticator "go.pinniped.dev/internal/controller/authenticator" +) + +//go:generate go run -v github.com/golang/mock/mockgen -destination=mocktokenauthenticatorcloser.go -package=mocktokenauthenticatorcloser -copyright_file=../../../hack/header.txt . TokenAuthenticatorCloser + +// TokenAuthenticatorCloser is a type that can authenticate tokens and be closed idempotently. +// +// This type is slightly different from io.Closer, because io.Closer can return an error and is not +// necessarily idempotent. +type TokenAuthenticatorCloser interface { + authenticator.Token + pinnipedauthenticator.Closer +} diff --git a/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go new file mode 100644 index 000000000..c4ddc1f4e --- /dev/null +++ b/internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go @@ -0,0 +1,67 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser (interfaces: TokenAuthenticatorCloser) + +// Package mocktokenauthenticatorcloser is a generated GoMock package. +package mocktokenauthenticatorcloser + +import ( + context "context" + gomock "github.com/golang/mock/gomock" + authenticator "k8s.io/apiserver/pkg/authentication/authenticator" + reflect "reflect" +) + +// MockTokenAuthenticatorCloser is a mock of TokenAuthenticatorCloser interface +type MockTokenAuthenticatorCloser struct { + ctrl *gomock.Controller + recorder *MockTokenAuthenticatorCloserMockRecorder +} + +// MockTokenAuthenticatorCloserMockRecorder is the mock recorder for MockTokenAuthenticatorCloser +type MockTokenAuthenticatorCloserMockRecorder struct { + mock *MockTokenAuthenticatorCloser +} + +// NewMockTokenAuthenticatorCloser creates a new mock instance +func NewMockTokenAuthenticatorCloser(ctrl *gomock.Controller) *MockTokenAuthenticatorCloser { + mock := &MockTokenAuthenticatorCloser{ctrl: ctrl} + mock.recorder = &MockTokenAuthenticatorCloserMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockTokenAuthenticatorCloser) EXPECT() *MockTokenAuthenticatorCloserMockRecorder { + return m.recorder +} + +// AuthenticateToken mocks base method +func (m *MockTokenAuthenticatorCloser) AuthenticateToken(arg0 context.Context, arg1 string) (*authenticator.Response, bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AuthenticateToken", arg0, arg1) + ret0, _ := ret[0].(*authenticator.Response) + ret1, _ := ret[1].(bool) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// AuthenticateToken indicates an expected call of AuthenticateToken +func (mr *MockTokenAuthenticatorCloserMockRecorder) AuthenticateToken(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthenticateToken", reflect.TypeOf((*MockTokenAuthenticatorCloser)(nil).AuthenticateToken), arg0, arg1) +} + +// Close mocks base method +func (m *MockTokenAuthenticatorCloser) Close() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Close") +} + +// Close indicates an expected call of Close +func (mr *MockTokenAuthenticatorCloserMockRecorder) Close() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockTokenAuthenticatorCloser)(nil).Close)) +}