fix bug in jwtcachefiller caused when status update returns error

Co-authored-by: Ashish Amarnath <ashish.amarnath@broadcom.com>

internal/controller/authenticator/authncache/cache.go

@@ -36,6 +36,7 @@ type Key struct {
 
 type Value interface {
 	authenticator.Token
+	Close()
 }
 
 // New returns an empty cache.
@@ -45,21 +46,31 @@ func New() *Cache {
 
 // Get an authenticator by key.
 func (c *Cache) Get(key Key) Value {
-	res, _ := c.cache.Load(key)
-	if res == nil {
+	v, _ := c.cache.Load(key)
+	if v == nil {
 		return nil
 	}
-	return res.(Value)
+	return v.(Value)
 }
 
-// Store an authenticator into the cache.
+// Store an authenticator into the cache. If overwriting a value in the cache, closes the overwritten value.
 func (c *Cache) Store(key Key, value Value) {
-	c.cache.Store(key, value)
+	previousValue, _ := c.cache.Swap(key, value)
+	// Wait until after it has been overwritten in the cache to close it, to ensure that it is only closed
+	// after it is not available for cache reads anymore.
+	if previousValue != nil {
+		previousValue.(Value).Close()
+	}
 }
 
-// Delete an authenticator from the cache.
+// Delete an authenticator from the cache. Closes the authenticator after removing it from the cache.
 func (c *Cache) Delete(key Key) {
-	c.cache.Delete(key)
+	deletedValue, _ := c.cache.LoadAndDelete(key)
+	// Wait until after it has been removed from the cache to close it, to ensure that it is only closed
+	// after it is not available for cache reads anymore.
+	if deletedValue != nil {
+		deletedValue.(Value).Close()
+	}
 }
 
 // Keys currently stored in the cache.

internal/controller/authenticator/authncache/cache_test.go

@@ -19,35 +19,52 @@ import (
 
 	authenticationv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1"
 	loginapi "go.pinniped.dev/generated/latest/apis/concierge/login"
-	"go.pinniped.dev/internal/mocks/mocktokenauthenticator"
+	"go.pinniped.dev/internal/mocks/mockcachevalue"
 )
 
 func TestCache(t *testing.T) {
 	t.Parallel()
 
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
+	t.Cleanup(func() {
+		ctrl.Finish()
+	})
 
 	cache := New()
 	require.NotNil(t, cache)
 
 	key1 := Key{Name: "authenticator-one"}
-	mockToken1 := mocktokenauthenticator.NewMockToken(ctrl)
-	cache.Store(key1, mockToken1)
-	require.Equal(t, mockToken1, cache.Get(key1))
+	mockValue1 := mockcachevalue.NewMockValue(ctrl)
+	require.Nil(t, cache.Get(key1))
+	cache.Store(key1, mockValue1)
+	require.Equal(t, mockValue1, cache.Get(key1))
 	require.Equal(t, 1, len(cache.Keys()))
 
 	key2 := Key{Name: "authenticator-two"}
-	mockToken2 := mocktokenauthenticator.NewMockToken(ctrl)
-	cache.Store(key2, mockToken2)
-	require.Equal(t, mockToken2, cache.Get(key2))
+	mockValue2 := mockcachevalue.NewMockValue(ctrl)
+	cache.Store(key2, mockValue2)
+	require.Equal(t, mockValue2, cache.Get(key2))
 	require.Equal(t, 2, len(cache.Keys()))
 
+	// Assert that Close() has not been called yet, and it should be called by the end of the test.
+	mockValue1.EXPECT().Close().Times(1)
+	mockValue2.EXPECT().Close().Times(1)
+
 	for _, key := range cache.Keys() {
 		cache.Delete(key)
 	}
 	require.Zero(t, len(cache.Keys()))
 
+	key3 := Key{Name: "authenticator-three"}
+	mockValue3 := mockcachevalue.NewMockValue(ctrl)
+	cache.Store(key3, mockValue3)
+	require.Equal(t, mockValue3, cache.Get(key3))
+	require.Equal(t, 1, len(cache.Keys()))
+	mockValue4 := mockcachevalue.NewMockValue(ctrl)
+	// Assert that Close() has not been called yet, and it should be called by the end of the test.
+	mockValue3.EXPECT().Close().Times(1)
+	cache.Store(key3, mockValue4) // overwrite
+
 	// Fill the cache back up with a fixed set of keys, but inserted in shuffled order.
 	keysInExpectedOrder := []Key{
 		{APIGroup: "a", Kind: "a", Name: "a"},
@@ -92,7 +109,7 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) {
 	mockCache := func(t *testing.T, res *authenticator.Response, authenticated bool, err error) *Cache {
 		ctrl := gomock.NewController(t)
 		t.Cleanup(ctrl.Finish)
-		m := mocktokenauthenticator.NewMockToken(ctrl)
+		m := mockcachevalue.NewMockValue(ctrl)
 		m.EXPECT().AuthenticateToken(audienceFreeContext{}, validRequest.Spec.Token).Return(res, authenticated, err)
 		c := New()
 		c.Store(validRequestKey, m)
@@ -137,7 +154,7 @@ func TestAuthenticateTokenCredentialRequest(t *testing.T) {
 	t.Run("context is cancelled", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
 		t.Cleanup(ctrl.Finish)
-		m := mocktokenauthenticator.NewMockToken(ctrl)
+		m := mockcachevalue.NewMockValue(ctrl)
 		m.EXPECT().AuthenticateToken(gomock.Any(), validRequest.Spec.Token).DoAndReturn(
 			func(ctx context.Context, token string) (*authenticator.Response, bool, error) {
 				select {

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

@@ -231,13 +231,13 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 	// rather than trying to show the most up-to-date status possible. These validations are for administrator
 	// convenience at the time of a configuration change, to catch typos and blatant misconfigurations, rather
 	// than to constantly monitor for external issues.
-	var jwtAuthenticatorFromCache *cachedJWTAuthenticator
+	var oldJWTAuthenticatorFromCache *cachedJWTAuthenticator
 	if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil {
-		jwtAuthenticatorFromCache = c.cacheValueAsJWTAuthenticator(valueFromCache)
-		if jwtAuthenticatorFromCache != nil &&
-			reflect.DeepEqual(jwtAuthenticatorFromCache.spec, &jwtAuthenticator.Spec) &&
+		oldJWTAuthenticatorFromCache = c.cacheValueAsJWTAuthenticator(valueFromCache)
+		if oldJWTAuthenticatorFromCache != nil &&
+			reflect.DeepEqual(oldJWTAuthenticatorFromCache.spec, &jwtAuthenticator.Spec) &&
 			tlsBundleOk && // if there was any error while validating the CA bundle, then run remaining validations and update status
-			jwtAuthenticatorFromCache.caBundleHash.Equal(caBundle.Hash()) {
+			oldJWTAuthenticatorFromCache.caBundleHash.Equal(caBundle.Hash()) {
 			c.log.WithValues("jwtAuthenticator", klog.KObj(jwtAuthenticator), "issuer", jwtAuthenticator.Spec.Issuer).
 				Info("actual jwt authenticator and desired jwt authenticator are the same")
 			// Stop, no more work to be done. This authenticator is already validated and cached.
@@ -274,25 +274,29 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 		okSoFar)
 	errs = append(errs, err)
 
-	if conditionsutil.HadErrorCondition(conditions) {
+	authenticatorValid := !conditionsutil.HadErrorCondition(conditions)
+
+	// If we calculated a failed status condition, then remove it from the cache even before we try to write
+	// the status, because writing the status can fail for various reasons.
+	if !authenticatorValid {
 		// The authenticator was determined to be invalid. Remove it from the cache, in case it was previously
 		// validated and cached. Do not allow an old, previously validated spec of the authenticator to continue
 		// being used for authentication.
 		c.cache.Delete(cacheKey)
-	} else {
+	}
+
+	updateErr := c.updateStatus(ctx, jwtAuthenticator, conditions)
+	errs = append(errs, updateErr)
+
+	// Only add this JWTAuthenticator to the cache if the status update succeeds.
+	// If it were in the cache after failing to update the status, then the next Sync loop would see it in the cache
+	// and skip trying to update its status again, which would leave its old status permanently intact.
+	if authenticatorValid && updateErr == nil {
 		c.cache.Store(cacheKey, newJWTAuthenticatorForCache)
 		c.log.WithValues("jwtAuthenticator", klog.KObj(jwtAuthenticator), "issuer", jwtAuthenticator.Spec.Issuer).
 			Info("added new jwt authenticator")
 	}
 
-	// In case we just overwrote or deleted the authenticator from the cache, clean up the old instance
-	// to avoid leaking goroutines. It's safe to call Close() on nil. We avoid calling Close() until it is
-	// removed from the cache, because we do not want any end-user authentications to use a closed authenticator.
-	jwtAuthenticatorFromCache.Close()
-
-	err = c.updateStatus(ctx, jwtAuthenticator, conditions)
-	errs = append(errs, err)
-
 	// Sync loop errors:
 	// - Should not be configuration errors. Config errors a user must correct belong on the .Status
 	//   object. The controller simply must wait for a user to correct before running again.

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -47,6 +48,7 @@ import (
 	"go.pinniped.dev/internal/controller/tlsconfigutil"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/crypto/ptls"
+	"go.pinniped.dev/internal/mocks/mockcachevalue"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/internal/testutil/conditionstestutil"
@@ -712,7 +714,7 @@ func TestController(t *testing.T) {
 		// Errors such as url.Parse of the issuer are not returned as they imply a user error.
 		// Since these errors trigger a resync, we are careful only to return an error when
 		// something can be automatically corrected on a retry (ie an error that might be networking).
-		wantSyncLoopErr                     testutil.RequireErrorStringFunc
+		wantSyncErr                         testutil.RequireErrorStringFunc
 		wantLogs                            []map[string]any
 		wantActions                         func() []coretesting.Action
 		wantUsernameClaim                   string
@@ -801,7 +803,7 @@ func TestController(t *testing.T) {
 					Spec: *badIssuerJWKSURIJWTAuthenticatorSpec,
 				},
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString("[" +
+			wantSyncErr: testutil.WantExactErrorString("[" +
 				`error for JWTAuthenticator another-invalid-jwt-authenticator: could not parse provider jwks_uri: parse "https://.café   .com/café/café/café/coffee/jwks.json": invalid character " " in host name` +
 				", " +
 				`error for JWTAuthenticator invalid-jwt-authenticator: could not parse provider jwks_uri: parse "https://.café   .com/café/café/café/coffee/jwks.json": invalid character " " in host name` +
@@ -1181,7 +1183,6 @@ func TestController(t *testing.T) {
 					newCacheValue(t, *otherJWTAuthenticatorSpec, string(oldCA), wantClose),
 				)
 			},
-			wantClose: true,
 			jwtAuthenticators: []runtime.Object{
 				&authenticationv1alpha1.JWTAuthenticator{
 					ObjectMeta: metav1.ObjectMeta{
@@ -1219,6 +1220,62 @@ func TestController(t *testing.T) {
 				}
 			},
 			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
+			wantClose:                           true,
+		},
+		{
+			name: "Sync: previously cached JWTAuthenticator gets new spec fields, but status update fails: loop will leave it in the cache and avoid calling close",
+			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
+				oldCA, err := base64.StdEncoding.DecodeString(otherJWTAuthenticatorSpec.TLS.CertificateAuthorityData)
+				require.NoError(t, err)
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "JWTAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					newCacheValue(t, *otherJWTAuthenticatorSpec, string(oldCA), wantClose),
+				)
+			},
+			configClient: func(client *conciergefake.Clientset) {
+				client.PrependReactor(
+					"update",
+					"jwtauthenticators",
+					func(action coretesting.Action) (handled bool, ret runtime.Object, err error) {
+						return true, nil, errors.New("some update error")
+					},
+				)
+			},
+			jwtAuthenticators: []runtime.Object{
+				&authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+				},
+			},
+			wantLogs: []map[string]any{},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+					Status: authenticationv1alpha1.JWTAuthenticatorStatus{
+						Conditions: allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
+						Phase:      "Ready",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			skipTestingCachedAuthenticator:      true,
+			wantSyncErr:                         testutil.WantExactErrorString("error for JWTAuthenticator test-name: some update error"),
+			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"}, // keeps the old entry in the cache
+			wantClose:                           false,
 		},
 		{
 			name: "Sync: JWTAuthenticator with external and changed CA bundle: loop will close previous instance of JWTAuthenticator and complete successfully and update status conditions",
@@ -1232,7 +1289,6 @@ func TestController(t *testing.T) {
 					newCacheValue(t, *someJWTAuthenticatorSpecWithCAInSecret, "some-stale-ca-bundle-pem-content-from-secret", wantClose),
 				)
 			},
-			wantClose: true,
 			jwtAuthenticators: []runtime.Object{
 				&authenticationv1alpha1.JWTAuthenticator{
 					ObjectMeta: metav1.ObjectMeta{
@@ -1272,6 +1328,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
+			wantClose:                           true,
 			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
 		},
 		{
@@ -1288,7 +1345,6 @@ func TestController(t *testing.T) {
 					newCacheValue(t, *someJWTAuthenticatorSpec, string(oldCA), wantClose),
 				)
 			},
-			wantClose: false,
 			jwtAuthenticators: []runtime.Object{
 				&authenticationv1alpha1.JWTAuthenticator{
 					ObjectMeta: metav1.ObjectMeta{
@@ -1313,6 +1369,7 @@ func TestController(t *testing.T) {
 					coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
 				}
 			},
+			wantClose:                           false,
 			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
 			// skip the tests because the authenticator pre-loaded into the cache is the mock version that was added above
 			skipTestingCachedAuthenticator: true,
@@ -1320,6 +1377,12 @@ func TestController(t *testing.T) {
 		{
 			name: "Sync: authenticator update when cached authenticator is the wrong data type, which should never really happen: loop will complete successfully and update status conditions",
 			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
+				ctrl := gomock.NewController(t)
+				t.Cleanup(func() {
+					ctrl.Finish()
+				})
+				mockCacheValue := mockcachevalue.NewMockValue(ctrl)
+				mockCacheValue.EXPECT().Close().Times(1)
 				cache.Store(
 					authncache.Key{
 						Name:     "test-name",
@@ -1329,7 +1392,7 @@ func TestController(t *testing.T) {
 					// Only entries of type cachedJWTAuthenticator are ever put into the cache, so this should never really happen.
 					// This test is to provide coverage on the production code which reads from the cache and casts those entries to
 					// the appropriate data type.
-					struct{ authenticator.Token }{},
+					mockCacheValue,
 				)
 			},
 			jwtAuthenticators: []runtime.Object{
@@ -1346,7 +1409,7 @@ func TestController(t *testing.T) {
 					"timestamp":  "2099-08-08T13:57:36.123456Z",
 					"logger":     "jwtcachefiller-controller",
 					"message":    "wrong JWT authenticator type in cache",
-					"actualType": "struct { authenticator.Token }",
+					"actualType": "*mockcachevalue.MockValue",
 				},
 				{
 					"level":     "info",
@@ -1419,7 +1482,7 @@ func TestController(t *testing.T) {
 			},
 			// no explicit logs, this is an issue of config, the user must provide TLS config for the
 			// custom cert provided for this server.
-			wantSyncLoopErr: testutil.WantSprintfErrorString(`error for JWTAuthenticator test-name: could not perform oidc discovery on provider issuer: Get "%s/.well-known/openid-configuration": tls: failed to verify certificate: x509: certificate signed by unknown authority`, goodIssuer),
+			wantSyncErr: testutil.WantSprintfErrorString(`error for JWTAuthenticator test-name: could not perform oidc discovery on provider issuer: Get "%s/.well-known/openid-configuration": tls: failed to verify certificate: x509: certificate signed by unknown authority`, goodIssuer),
 		},
 		{
 			name: "validateTLS: JWTAuthenticator with invalid CA: loop will fail, will write failed and unknown status conditions, but will not enqueue a resync due to user config error",
@@ -1461,7 +1524,7 @@ func TestController(t *testing.T) {
 			},
 		},
 		{
-			name: "previously valid cached authenticator (which did not specify a CA bundle) changes and becomes invalid due to any problem with the CA bundle: loop will fail sync, will write failed and unknown status conditions, and will remove authenticator from cache",
+			name: "previously valid cached authenticator (which did not specify a CA bundle) changes and becomes invalid due to any problem with the CA bundle: loop will fail sync, will write failed and unknown status conditions, and will remove authenticator from cache and close it",
 			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
 				cache.Store(
 					authncache.Key{
@@ -1825,7 +1888,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString(`error for JWTAuthenticator test-name: could not perform oidc discovery on provider issuer: Get "` + goodIssuer + `/foo/bar/baz/shizzle/.well-known/openid-configuration": tls: failed to verify certificate: x509: certificate signed by unknown authority`),
+			wantSyncErr: testutil.WantExactErrorString(`error for JWTAuthenticator test-name: could not perform oidc discovery on provider issuer: Get "` + goodIssuer + `/foo/bar/baz/shizzle/.well-known/openid-configuration": tls: failed to verify certificate: x509: certificate signed by unknown authority`),
 		},
 		{
 			name: "validateProviderDiscovery: excessively long errors truncated: loop will fail sync, will write failed and unknown conditions, and will enqueue new sync",
@@ -1875,7 +1938,7 @@ func TestController(t *testing.T) {
 				}
 			},
 			// not currently truncating the logged err
-			wantSyncLoopErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: could not perform oidc discovery on provider issuer: 404 Not Found: <html>\n\t\t  \t<head><title>404 not found page</title></head>\n\t\t\t<body>lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz should not reach end of string</body>\n\t\t</html>"),
+			wantSyncErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: could not perform oidc discovery on provider issuer: 404 Not Found: <html>\n\t\t  \t<head><title>404 not found page</title></head>\n\t\t\t<body>lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz lots of text that is at least 300 characters long 0123456789 abcdefghijklmnopqrstuvwxyz should not reach end of string</body>\n\t\t</html>"),
 		},
 		// cannot be tested currently the way the coreos lib works.
 		// the constructor requires an issuer in the payload and validates the issuer matches the actual issuer,
@@ -1918,7 +1981,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString(`error for JWTAuthenticator test-name: could not parse provider jwks_uri: parse "https://.café   .com/café/café/café/coffee/jwks.json": invalid character " " in host name`),
+			wantSyncErr: testutil.WantExactErrorString(`error for JWTAuthenticator test-name: could not parse provider jwks_uri: parse "https://.café   .com/café/café/café/coffee/jwks.json": invalid character " " in host name`),
 		},
 		{
 			name: "validateProviderJWKSURL: invalid scheme, requires 'https': loop will fail sync, will write failed and unknown conditions, and will enqueue new sync",
@@ -1957,7 +2020,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: jwks_uri http://.café.com/café/café/café/coffee/jwks.json has invalid scheme, require 'https'"),
+			wantSyncErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: jwks_uri http://.café.com/café/café/café/coffee/jwks.json has invalid scheme, require 'https'"),
 		},
 		{
 			name: "validateProviderJWKSURL: remote jwks should not have been able to verify hardcoded test jwt token: loop will fail sync, will write failed and unknown conditions, and will enqueue new sync",
@@ -1997,7 +2060,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: remote jwks should not have been able to verify hardcoded test jwt token"),
+			wantSyncErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: remote jwks should not have been able to verify hardcoded test jwt token"),
 		},
 		{
 			name: "validateJWKSFetch: could not fetch keys: loop will fail sync, will write failed and unknown status conditions, and will enqueue a resync",
@@ -2036,7 +2099,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: could not fetch keys: fetching keys oidc: get keys failed: 404 Not Found 404 page not found\n"),
+			wantSyncErr: testutil.WantExactErrorString("error for JWTAuthenticator test-name: could not fetch keys: fetching keys oidc: get keys failed: 404 Not Found 404 page not found\n"),
 		},
 		{
 			name: "updateStatus: called with matching original and updated conditions: will not make request to update conditions",
@@ -2120,22 +2183,13 @@ func TestController(t *testing.T) {
 			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
 		},
 		{
-			name: "updateStatus: when update request fails: error will enqueue a resync",
+			name: "updateStatus: given a valid JWTAuthenticator spec, when update request fails: error will enqueue a resync and the authenticator will not be added to the cache",
 			jwtAuthenticators: []runtime.Object{
 				&authenticationv1alpha1.JWTAuthenticator{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "test-name",
 					},
 					Spec: *someJWTAuthenticatorSpec,
-					Status: authenticationv1alpha1.JWTAuthenticatorStatus{
-						Conditions: conditionstestutil.Replace(
-							allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
-							[]metav1.Condition{
-								sadReadyCondition(frozenMetav1Now, 0),
-							},
-						),
-						Phase: "SomethingThatWontUpdate",
-					},
 				},
 			},
 			configClient: func(client *conciergefake.Clientset) {
@@ -2149,7 +2203,7 @@ func TestController(t *testing.T) {
 			},
 			wantActions: func() []coretesting.Action {
 				// This captures that there was an attempt to update to Ready, allHappyConditions,
-				// but the wantSyncLoopErr indicates that there is a failure, so the JWTAuthenticator
+				// but the wantSyncErr indicates that there is a failure, so the JWTAuthenticator
 				// remains with a bad phase and at least 1 sad condition
 				updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &authenticationv1alpha1.JWTAuthenticator{
 					ObjectMeta: metav1.ObjectMeta{
@@ -2168,18 +2222,9 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantLogs: []map[string]any{{
-				"level":     "info",
-				"timestamp": "2099-08-08T13:57:36.123456Z",
-				"logger":    "jwtcachefiller-controller",
-				"message":   "added new jwt authenticator",
-				"issuer":    goodIssuer,
-				"jwtAuthenticator": map[string]any{
-					"name": "test-name",
-				},
-			}},
-			wantSyncLoopErr:                     testutil.WantExactErrorString("error for JWTAuthenticator test-name: some update error"),
-			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
+			wantLogs:                            []map[string]any{},
+			wantSyncErr:                         testutil.WantExactErrorString("error for JWTAuthenticator test-name: some update error"),
+			wantNamesOfJWTAuthenticatorsInCache: []string{}, // even though the authenticator was valid, do not cache it because the status update failed
 		},
 		// cannot be tested the way we are invoking oidc.New as we don't provide enough configuration
 		// knobs to actually invoke the code in a broken way.  We always give a good client, good keys, and
@@ -2227,8 +2272,8 @@ func TestController(t *testing.T) {
 
 			syncCtx := controllerlib.Context{Context: ctx}
 
-			if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantSyncLoopErr != nil {
-				testutil.RequireErrorStringFromErr(t, err, tt.wantSyncLoopErr)
+			if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantSyncErr != nil {
+				testutil.RequireErrorStringFromErr(t, err, tt.wantSyncErr)
 			} else {
 				require.NoError(t, err)
 			}

internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go

@@ -65,6 +65,10 @@ type cachedWebhookAuthenticator struct {
 	caBundleHash tlsconfigutil.CABundleHash
 }
 
+func (*cachedWebhookAuthenticator) Close() {
+	// no-op, because no cleanup is needed on webhook authenticators
+}
+
 // New instantiates a new controllerlib.Controller which will populate the provided authncache.Cache.
 func New(
 	namespace string,
@@ -171,11 +175,11 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 	// convenience at the time of a configuration change, to catch typos and blatant misconfigurations, rather
 	// than to constantly monitor for external issues.
 	if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil {
-		webhookAuthenticatorFromCache := c.cacheValueAsWebhookAuthenticator(valueFromCache)
-		if webhookAuthenticatorFromCache != nil &&
-			reflect.DeepEqual(webhookAuthenticatorFromCache.spec, &webhookAuthenticator.Spec) &&
+		oldWebhookAuthenticatorFromCache := c.cacheValueAsWebhookAuthenticator(valueFromCache)
+		if oldWebhookAuthenticatorFromCache != nil &&
+			reflect.DeepEqual(oldWebhookAuthenticatorFromCache.spec, &webhookAuthenticator.Spec) &&
 			tlsBundleOk && // if there was any error while validating the CA bundle, then run remaining validations and update status
-			webhookAuthenticatorFromCache.caBundleHash.Equal(caBundle.Hash()) {
+			oldWebhookAuthenticatorFromCache.caBundleHash.Equal(caBundle.Hash()) {
 			c.log.WithValues("webhookAuthenticator", klog.KObj(webhookAuthenticator), "endpoint", webhookAuthenticator.Spec.Endpoint).
 				Info("actual webhook authenticator and desired webhook authenticator are the same")
 			// Stop, no more work to be done. This authenticator is already validated and cached.

internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go

@@ -20,12 +20,12 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	authenticationv1beta1 "k8s.io/api/authentication/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 	k8sinformers "k8s.io/client-go/informers"
 	kubeinformers "k8s.io/client-go/informers"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
@@ -41,6 +41,7 @@ import (
 	"go.pinniped.dev/internal/controller/tlsconfigutil"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/crypto/ptls"
+	"go.pinniped.dev/internal/mocks/mockcachevalue"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/internal/testutil/conditionstestutil"
@@ -410,10 +411,10 @@ func TestController(t *testing.T) {
 		webhookAuthenticators []runtime.Object
 		secretsAndConfigMaps  []runtime.Object
 		// for modifying the clients to hack in arbitrary api responses
-		configClient    func(*conciergefake.Clientset)
-		wantSyncLoopErr testutil.RequireErrorStringFunc
-		wantLogs        []map[string]any
-		wantActions     func() []coretesting.Action
+		configClient func(*conciergefake.Clientset)
+		wantSyncErr  testutil.RequireErrorStringFunc
+		wantLogs     []map[string]any
+		wantActions  func() []coretesting.Action
 		// random comment so lines above don't have huge indents
 		wantNamesOfWebhookAuthenticatorsInCache []string
 	}{
@@ -833,6 +834,12 @@ func TestController(t *testing.T) {
 		{
 			name: "Sync: authenticator update when cached authenticator is the wrong data type, which should never really happen: loop will complete successfully and update status conditions",
 			cache: func(t *testing.T, cache *authncache.Cache) {
+				ctrl := gomock.NewController(t)
+				t.Cleanup(func() {
+					ctrl.Finish()
+				})
+				mockCacheValue := mockcachevalue.NewMockValue(ctrl)
+				mockCacheValue.EXPECT().Close().Times(1)
 				cache.Store(
 					authncache.Key{
 						Name:     "test-name",
@@ -842,7 +849,7 @@ func TestController(t *testing.T) {
 					// Only entries of type cachedWebhookAuthenticator are ever put into the cache, so this should never really happen.
 					// This test is to provide coverage on the production code which reads from the cache and casts those entries to
 					// the appropriate data type.
-					struct{ authenticator.Token }{},
+					mockCacheValue,
 				)
 			},
 			webhookAuthenticators: []runtime.Object{
@@ -859,7 +866,7 @@ func TestController(t *testing.T) {
 					"timestamp":  "2099-08-08T13:57:36.123456Z",
 					"logger":     "webhookcachefiller-controller",
 					"message":    "wrong webhook authenticator type in cache",
-					"actualType": "struct { authenticator.Token }",
+					"actualType": "*mockcachevalue.MockValue",
 				},
 				{
 					"level":     "info",
@@ -1097,7 +1104,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr:                         testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: tls: failed to verify certificate: x509: certificate signed by unknown authority`),
+			wantSyncErr:                             testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: tls: failed to verify certificate: x509: certificate signed by unknown authority`),
 			wantNamesOfWebhookAuthenticatorsInCache: []string{},
 		},
 		{
@@ -1337,7 +1344,7 @@ func TestController(t *testing.T) {
 					Spec: badWebhookAuthenticatorSpecGoodEndpointButUnknownCA,
 				},
 			},
-			wantSyncLoopErr: testutil.WantExactErrorString("error for WebhookAuthenticator test-name: cannot dial server: tls: failed to verify certificate: x509: certificate signed by unknown authority"),
+			wantSyncErr: testutil.WantExactErrorString("error for WebhookAuthenticator test-name: cannot dial server: tls: failed to verify certificate: x509: certificate signed by unknown authority"),
 			wantActions: func() []coretesting.Action {
 				updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{
 					ObjectMeta: metav1.ObjectMeta{
@@ -1495,7 +1502,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr:                         testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: dial tcp [::1]:4242: connect: connection refused`),
+			wantSyncErr:                             testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: dial tcp [::1]:4242: connect: connection refused`),
 			wantNamesOfWebhookAuthenticatorsInCache: []string{},
 		},
 		{
@@ -1543,7 +1550,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr:                         testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: dial tcp [::1]:443: connect: connection refused`),
+			wantSyncErr:                             testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: dial tcp [::1]:443: connect: connection refused`),
 			wantNamesOfWebhookAuthenticatorsInCache: []string{},
 		},
 		{
@@ -1625,7 +1632,7 @@ func TestController(t *testing.T) {
 				}
 			},
 			wantNamesOfWebhookAuthenticatorsInCache: []string{},
-			wantSyncLoopErr:                         testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: tls: failed to verify certificate: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs`),
+			wantSyncErr:                             testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: tls: failed to verify certificate: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs`),
 		},
 		{
 			name: "validateConnection: IPv6 address without port or brackets: should succeed since IPv6 brackets are optional without port",
@@ -1672,7 +1679,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr:                         testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: dial tcp [::1]:443: connect: connection refused`),
+			wantSyncErr:                             testutil.WantExactErrorString(`error for WebhookAuthenticator test-name: cannot dial server: dial tcp [::1]:443: connect: connection refused`),
 			wantNamesOfWebhookAuthenticatorsInCache: []string{},
 		},
 		{
@@ -1818,7 +1825,7 @@ func TestController(t *testing.T) {
 					updateStatusAction,
 				}
 			},
-			wantSyncLoopErr:                         testutil.WantExactErrorString("error for WebhookAuthenticator test-name: some update error"),
+			wantSyncErr:                             testutil.WantExactErrorString("error for WebhookAuthenticator test-name: some update error"),
 			wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"},
 		},
 	}
@@ -1861,8 +1868,8 @@ func TestController(t *testing.T) {
 
 			syncCtx := controllerlib.Context{Context: ctx}
 
-			if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantSyncLoopErr != nil {
-				testutil.RequireErrorStringFromErr(t, err, tt.wantSyncLoopErr)
+			if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantSyncErr != nil {
+				testutil.RequireErrorStringFromErr(t, err, tt.wantSyncErr)
 			} else {
 				require.NoError(t, err)
 			}

internal/mocks/mockcachevalue/generate.go

@@ -0,0 +1,6 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package mockcachevalue
+
+//go:generate go run -v go.uber.org/mock/mockgen  -destination=mockcachevalue.go -package=mockcachevalue -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/controller/authenticator/authncache Value

internal/mocks/mockcachevalue/mockcachevalue.go

@@ -0,0 +1,73 @@
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+//
+
+// Code generated by MockGen. DO NOT EDIT.
+// Source: go.pinniped.dev/internal/controller/authenticator/authncache (interfaces: Value)
+//
+// Generated by this command:
+//
+//	mockgen -destination=mockcachevalue.go -package=mockcachevalue -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/controller/authenticator/authncache Value
+//
+
+// Package mockcachevalue is a generated GoMock package.
+package mockcachevalue
+
+import (
+	context "context"
+	reflect "reflect"
+
+	gomock "go.uber.org/mock/gomock"
+	authenticator "k8s.io/apiserver/pkg/authentication/authenticator"
+)
+
+// MockValue is a mock of Value interface.
+type MockValue struct {
+	ctrl     *gomock.Controller
+	recorder *MockValueMockRecorder
+}
+
+// MockValueMockRecorder is the mock recorder for MockValue.
+type MockValueMockRecorder struct {
+	mock *MockValue
+}
+
+// NewMockValue creates a new mock instance.
+func NewMockValue(ctrl *gomock.Controller) *MockValue {
+	mock := &MockValue{ctrl: ctrl}
+	mock.recorder = &MockValueMockRecorder{mock}
+	return mock
+}
+
+// EXPECT returns an object that allows the caller to indicate expected use.
+func (m *MockValue) EXPECT() *MockValueMockRecorder {
+	return m.recorder
+}
+
+// AuthenticateToken mocks base method.
+func (m *MockValue) 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 *MockValueMockRecorder) AuthenticateToken(arg0, arg1 any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthenticateToken", reflect.TypeOf((*MockValue)(nil).AuthenticateToken), arg0, arg1)
+}
+
+// Close mocks base method.
+func (m *MockValue) Close() {
+	m.ctrl.T.Helper()
+	m.ctrl.Call(m, "Close")
+}
+
+// Close indicates an expected call of Close.
+func (mr *MockValueMockRecorder) Close() *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockValue)(nil).Close))
+}

internal/mocks/mocktokenauthenticator/generate.go

@@ -1,6 +0,0 @@
-// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
-// SPDX-License-Identifier: Apache-2.0
-
-package mocktokenauthenticator
-
-//go:generate go run -v go.uber.org/mock/mockgen  -destination=mocktokenauthenticator.go -package=mocktokenauthenticator -copyright_file=../../../hack/header.txt k8s.io/apiserver/pkg/authentication/authenticator Token

internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go

@@ -1,61 +0,0 @@
-// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
-// SPDX-License-Identifier: Apache-2.0
-//
-
-// Code generated by MockGen. DO NOT EDIT.
-// Source: k8s.io/apiserver/pkg/authentication/authenticator (interfaces: Token)
-//
-// Generated by this command:
-//
-//	mockgen -destination=mocktokenauthenticator.go -package=mocktokenauthenticator -copyright_file=../../../hack/header.txt k8s.io/apiserver/pkg/authentication/authenticator Token
-//
-
-// Package mocktokenauthenticator is a generated GoMock package.
-package mocktokenauthenticator
-
-import (
-	context "context"
-	reflect "reflect"
-
-	gomock "go.uber.org/mock/gomock"
-	authenticator "k8s.io/apiserver/pkg/authentication/authenticator"
-)
-
-// MockToken is a mock of Token interface.
-type MockToken struct {
-	ctrl     *gomock.Controller
-	recorder *MockTokenMockRecorder
-}
-
-// MockTokenMockRecorder is the mock recorder for MockToken.
-type MockTokenMockRecorder struct {
-	mock *MockToken
-}
-
-// NewMockToken creates a new mock instance.
-func NewMockToken(ctrl *gomock.Controller) *MockToken {
-	mock := &MockToken{ctrl: ctrl}
-	mock.recorder = &MockTokenMockRecorder{mock}
-	return mock
-}
-
-// EXPECT returns an object that allows the caller to indicate expected use.
-func (m *MockToken) EXPECT() *MockTokenMockRecorder {
-	return m.recorder
-}
-
-// AuthenticateToken mocks base method.
-func (m *MockToken) 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 *MockTokenMockRecorder) AuthenticateToken(arg0, arg1 any) *gomock.Call {
-	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthenticateToken", reflect.TypeOf((*MockToken)(nil).AuthenticateToken), arg0, arg1)
-}