Always close JWTAuthenticator underlying authenticator

Otherwise we will leak goroutines.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

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.

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 {
+			if closer, ok := value.(authenticator.Closer); ok {
-				closable.Close()
+				closer.Close()
 			}
 
 			c.cache.Delete(key)

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{
+			initialCache: func(t *testing.T, cache *authncache.Cache) {
-				testWebhookKey1:          nil,
+				cache.Store(testWebhookKey1, nil)
-				testJWTAuthenticatorKey1: nil,
+				cache.Store(testJWTAuthenticatorKey1, nil)
 			},
 			objects: []runtime.Object{
 				&authv1alpha.WebhookAuthenticator{
@@ -86,7 +88,6 @@ func TestController(t *testing.T) {
 		},
 		{
 			name: "authenticators not yet added",
-			initialCache: nil,
 			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{
+			initialCache: func(t *testing.T, cache *authncache.Cache) {
-				testWebhookKey1:          nil,
+				cache.Store(testWebhookKey1, nil)
-				testWebhookKey2:          nil,
+				cache.Store(testWebhookKey2, nil)
-				testJWTAuthenticatorKey1: newClosableCacheValue(t, "closable1", 0),
+				cache.Store(testJWTAuthenticatorKey1, newClosableCacheValue(t, 0))
-				testJWTAuthenticatorKey2: newClosableCacheValue(t, "closable2", 1),
+				cache.Store(testJWTAuthenticatorKey2, newClosableCacheValue(t, 1))
-				testKeyUnknownType:       nil,
+				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 {
+			if tt.initialCache != nil {
-				cache.Store(k, v)
+				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 {
+func newClosableCacheValue(t *testing.T, wantCloses int) authncache.Value {
-	t.Helper()
+	ctrl := gomock.NewController(t)
-	c := &closableCacheValue{}
+	t.Cleanup(ctrl.Finish)
-	t.Cleanup(func() {
+	tac := mocktokenauthenticatorcloser.NewMockTokenAuthenticatorCloser(ctrl)
-		require.Equalf(t, wantCloses, c.closeCallCount, "expected %s.Close() to be called %d times", name, wantCloses)
+	tac.EXPECT().Close().Times(wantCloses)
-	})
+	return tac
-	return c
-}
-
-type closableCacheValue struct {
-	authncache.Value
-	closeCallCount int
-}
-
-func (c *closableCacheValue) Close() {
-	c.closeCallCount++
 }

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{
+	c.cache.Store(cacheKey, jwtAuthenticator)
-		APIGroup:  auth1alpha1.GroupName,
-		Kind:      "JWTAuthenticator",
-		Namespace: ctx.Key.Namespace,
-		Name:      ctx.Key.Name,
-	}, jwtAuthenticator)
 	c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer).Info("added new jwt authenticator")
 	return nil
 }

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
+}

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
+}

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))
+}