fix bug in webhookcachefiller caused when status update returns error

Also refactor test assertions regarding log statements in
jwtcachefiller_test.go and webhookcachefiller_test.go

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

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go

@@ -32,6 +32,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/user"
@@ -2290,26 +2291,28 @@ func TestController(t *testing.T) {
 			actualLogLines := testutil.SplitByNewline(log.String())
 			require.Equal(t, len(tt.wantLogs), len(actualLogLines), "log line count should be correct")
 
-			for logLineNum, logLine := range actualLogLines {
-				require.NotNil(t, tt.wantLogs[logLineNum], "expected log line should never be empty")
-				var lineStruct map[string]any
-				err := json.Unmarshal([]byte(logLine), &lineStruct)
-				require.NoError(t, err)
+			for actualLogLineNum, actualLogLine := range actualLogLines {
+				wantLine := tt.wantLogs[actualLogLineNum]
+				require.NotNil(t, wantLine, "expected log line should never be empty")
 
-				require.Equal(t, tt.wantLogs[logLineNum]["level"], lineStruct["level"], fmt.Sprintf("log line (%d) log level should be correct (in: %s)", logLineNum, lineStruct))
+				var actualParsedLine map[string]any
+				err := json.Unmarshal([]byte(actualLogLine), &actualParsedLine)
+				require.NoError(t, err)
 
-				require.Equal(t, tt.wantLogs[logLineNum]["timestamp"], lineStruct["timestamp"], fmt.Sprintf("log line (%d) timestamp should be correct (in: %s)", logLineNum, lineStruct))
-				require.Equal(t, tt.wantLogs[logLineNum]["logger"], lineStruct["logger"], fmt.Sprintf("log line (%d) logger should be correct", logLineNum))
-				require.NotEmpty(t, lineStruct["caller"], fmt.Sprintf("log line (%d) caller should not be empty", logLineNum))
-				require.Equal(t, tt.wantLogs[logLineNum]["message"], lineStruct["message"], fmt.Sprintf("log line (%d) message should be correct", logLineNum))
-				if lineStruct["issuer"] != nil {
-					require.Equal(t, tt.wantLogs[logLineNum]["issuer"], lineStruct["issuer"], fmt.Sprintf("log line (%d) issuer should be correct", logLineNum))
-				}
-				if lineStruct["jwtAuthenticator"] != nil {
-					require.Equal(t, tt.wantLogs[logLineNum]["jwtAuthenticator"], lineStruct["jwtAuthenticator"], fmt.Sprintf("log line (%d) jwtAuthenticator should be correct", logLineNum))
-				}
-				if lineStruct["actualType"] != nil {
-					require.Equal(t, tt.wantLogs[logLineNum]["actualType"], lineStruct["actualType"], fmt.Sprintf("log line (%d) actualType should be correct", logLineNum))
+				wantLineAsJSON, err := json.Marshal(wantLine)
+				require.NoError(t, err)
+				wantLine["caller"] = "we don't want to actually make equality comparisons about this"
+				require.Lenf(t, actualParsedLine, len(wantLine), "actual: %s\nwant:   %s", actualLogLine, string(wantLineAsJSON))
+				require.Equal(t, sets.StringKeySet(actualParsedLine), sets.StringKeySet(wantLine))
+
+				for k := range actualParsedLine {
+					if k == "caller" {
+						require.NotEmpty(t, actualParsedLine["caller"])
+					} else {
+						require.Equal(t, wantLine[k], actualParsedLine[k],
+							fmt.Sprintf("log line (%d) key %q was not equal\nactual: %s\nwant:   %s",
+								actualLogLineNum, k, actualParsedLine[k], wantLine[k]))
+					}
 				}
 			}
 

internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go

@@ -205,24 +205,33 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 	)
 	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, webhookAuthenticator, conditions)
+	errs = append(errs, updateErr)
+
+	// Only add this WebhookAuthenticator 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, &cachedWebhookAuthenticator{
 			Token:        newWebhookAuthenticatorForCache,
 			spec:         webhookAuthenticator.Spec.DeepCopy(), // deep copy to avoid caching original object
 			caBundleHash: caBundle.Hash(),
 		})
-		c.log.WithValues("webhook", klog.KObj(webhookAuthenticator), "endpoint", webhookAuthenticator.Spec.Endpoint).
+		c.log.WithValues("webhookAuthenticator", klog.KObj(webhookAuthenticator), "endpoint", webhookAuthenticator.Spec.Endpoint).
 			Info("added new webhook authenticator")
 	}
 
-	err = c.updateStatus(ctx, webhookAuthenticator, 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.