webhookcachefiller and jwtcachefiller always update status when needed

Even when the authenticator is found in the cache, try to update its status. Failing to do so would mean that the actual status will not be overwritten by the controller's newly computed desired status. Co-authored-by: Ashish Amarnath <ashish.amarnath@broadcom.com>
2026-09-06 16:17:08 +00:00 · 2024-08-05 11:32:20 -07:00
parent a0c259ffbc
commit ed502949dd
6 changed files with 399 additions and 121 deletions
@@ -169,6 +169,7 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co

 	var errs []error
 	conditions := make([]*metav1.Condition, 0)
+	var newWebhookAuthenticatorForCache *cachedWebhookAuthenticator

 	caBundle, conditions, tlsBundleOk := c.validateTLSBundle(webhookAuthenticator.Spec.TLS, conditions)

@@ -176,36 +177,33 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 	okSoFar := tlsBundleOk && endpointOk

 	// Only revalidate and update the cache if the cached authenticator is different from the desired authenticator.
-	// There is no need to repeat validations for a spec that was already successfully validated. We are making a
-	// design decision to avoid repeating the validation which dials the server, even though the server's TLS
-	// configuration could have changed, because it is also possible that the network could be flaky. We are choosing
-	// to prefer to keep the authenticator cached (available for end-user auth attempts) during times of network flakes
-	// 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.
-	foundAuthenticatorInCache, alreadyValidatedTheseSettings := c.havePreviouslyValidated(
+	// There is no need to repeat connection probe validations for a URL and CA bundle combination that was already
+	// successfully validated. We are making a design decision to avoid repeating the validation which dials the server,
+	// even though the server's TLS configuration could have changed, because it is also possible that the network
+	// could be flaky. We are choosing to prefer to keep the authenticator cached (available for end-user auth attempts)
+	// during times of network flakes 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.
+	foundAuthenticatorInCache, previouslyValidatedWithSameEndpointAndBundle := c.havePreviouslyValidated(
 		cacheKey, webhookAuthenticator.Spec.Endpoint, tlsBundleOk, caBundle.Hash(), logger)
-	if alreadyValidatedTheseSettings {
-		// Stop, no more work to be done. This authenticator is already validated and cached.
-		// TODO: Append hardcoded list of remaining success conditions and skip redoing the remaining validations below.
-		//       Make sure that the conditions array is still checked for any errors, and the status and cache are still updated, as below.
-		return nil
+	if previouslyValidatedWithSameEndpointAndBundle {
+		// Because the authenticator was previously cached, that implies that the following conditions were
+		// previously validated. These are the expensive validations to repeat, so skip them this time.
+		// However, the status may be lagging behind due to the informer cache being slow to catch up
+		// after previous status updates, so always calculate the new status conditions again and check
+		// if they need to be updated.
+		conditions = append(conditions,
+			successfulWebhookConnectionValidCondition(),
+			successfulAuthenticatorValidCondition(),
+		)
+	} else {
+		// Run all remaining validations.
+		a, moreConditions, moreErrs := c.doExpensiveValidations(webhookAuthenticator, endpointHostPort, caBundle, okSoFar, logger)
+		newWebhookAuthenticatorForCache = a
+		conditions = append(conditions, moreConditions...)
+		errs = append(errs, moreErrs...)
 	}

-	conditions, tlsNegotiateErr := c.validateConnection(caBundle.CertPool(), endpointHostPort, conditions, okSoFar, logger)
-	errs = append(errs, tlsNegotiateErr)
-	okSoFar = okSoFar && tlsNegotiateErr == nil
-
-	newWebhookAuthenticatorForCache, conditions, err := newWebhookAuthenticator(
-		// Note that we use the whole URL when constructing the webhook client,
-		// not just the host and port that we validated above. We need the path, etc.
-		webhookAuthenticator.Spec.Endpoint,
-		caBundle.PEMBytes(),
-		conditions,
-		okSoFar,
-	)
-	errs = append(errs, err)
-
 	authenticatorValid := !conditionsutil.HadErrorCondition(conditions)

 	// If we calculated a failed status condition, then remove it from the cache even before we try to write
@@ -219,18 +217,13 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 			"removedFromCache", foundAuthenticatorInCache)
 	}

+	// Always try to update the status, even when we found it in the authenticator cache.
 	updateErr := c.updateStatus(ctx, webhookAuthenticator, conditions, logger)
 	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,
-			endpoint:     webhookAuthenticator.Spec.Endpoint,
-			caBundleHash: caBundle.Hash(),
-		})
+	// Only add/update this authenticator to the cache when we have a new one and the status update succeeded.
+	if newWebhookAuthenticatorForCache != nil && authenticatorValid && updateErr == nil {
+		c.cache.Store(cacheKey, newWebhookAuthenticatorForCache)
 		logger.Info("added or updated webhook authenticator in cache",
 			"isOverwrite", foundAuthenticatorInCache)
 	}
@@ -243,6 +236,41 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 	return utilerrors.NewAggregate(errs)
 }

+func (c *webhookCacheFillerController) doExpensiveValidations(
+	webhookAuthenticator *authenticationv1alpha1.WebhookAuthenticator,
+	endpointHostPort *endpointaddr.HostPort,
+	caBundle *tlsconfigutil.CABundle,
+	okSoFar bool,
+	logger plog.Logger,
+) (*cachedWebhookAuthenticator, []*metav1.Condition, []error) {
+	var newWebhookAuthenticatorForCache *cachedWebhookAuthenticator
+	var conditions []*metav1.Condition
+	var errs []error
+
+	conditions, tlsNegotiateErr := c.validateConnection(caBundle.CertPool(), endpointHostPort, conditions, okSoFar, logger)
+	errs = append(errs, tlsNegotiateErr)
+	okSoFar = okSoFar && tlsNegotiateErr == nil
+
+	newAuthenticator, conditions, err := newWebhookAuthenticator(
+		// Note that we use the whole URL when constructing the webhook client,
+		// not just the host and port that we validated above. We need the path, etc.
+		webhookAuthenticator.Spec.Endpoint,
+		caBundle.PEMBytes(),
+		conditions,
+		okSoFar,
+	)
+	errs = append(errs, err)
+
+	if newAuthenticator != nil {
+		newWebhookAuthenticatorForCache = &cachedWebhookAuthenticator{
+			Token:        newAuthenticator,
+			endpoint:     webhookAuthenticator.Spec.Endpoint,
+			caBundleHash: caBundle.Hash(),
+		}
+	}
+	return newWebhookAuthenticatorForCache, conditions, errs
+}
+
 func (c *webhookCacheFillerController) havePreviouslyValidated(
 	cacheKey authncache.Key,
 	endpoint string,
@@ -294,6 +322,15 @@ func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *authentication
 	return caBundle, conditions, condition.Status == metav1.ConditionTrue
 }

+func successfulAuthenticatorValidCondition() *metav1.Condition {
+	return &metav1.Condition{
+		Type:    typeAuthenticatorValid,
+		Status:  metav1.ConditionTrue,
+		Reason:  conditionsutil.ReasonSuccess,
+		Message: "authenticator initialized",
+	}
+}
+
 // newWebhookAuthenticator creates a webhook from the provided API server url and caBundle
 // used to validate TLS connections.
 func newWebhookAuthenticator(
@@ -362,17 +399,20 @@ func newWebhookAuthenticator(
 		return nil, conditions, fmt.Errorf("%s: %w", errText, err)
 	}

-	msg := "authenticator initialized"
-	conditions = append(conditions, &metav1.Condition{
-		Type:    typeAuthenticatorValid,
-		Status:  metav1.ConditionTrue,
-		Reason:  conditionsutil.ReasonSuccess,
-		Message: msg,
-	})
+	conditions = append(conditions, successfulAuthenticatorValidCondition())

 	return webhookAuthenticator, conditions, nil
 }

+func successfulWebhookConnectionValidCondition() *metav1.Condition {
+	return &metav1.Condition{
+		Type:    typeWebhookConnectionValid,
+		Status:  metav1.ConditionTrue,
+		Reason:  conditionsutil.ReasonSuccess,
+		Message: "successfully dialed webhook server",
+	}
+}
+
 func (c *webhookCacheFillerController) validateConnection(
 	certPool *x509.CertPool,
 	endpointHostPort *endpointaddr.HostPort,
@@ -411,12 +451,7 @@ func (c *webhookCacheFillerController) validateConnection(
 		logger.Error("error closing dialer", err)
 	}

-	conditions = append(conditions, &metav1.Condition{
-		Type:    typeWebhookConnectionValid,
-		Status:  metav1.ConditionTrue,
-		Reason:  conditionsutil.ReasonSuccess,
-		Message: "successfully dialed webhook server",
-	})
+	conditions = append(conditions, successfulWebhookConnectionValidCondition())
 	return conditions, nil
 }

@@ -769,6 +769,7 @@ func TestController(t *testing.T) {
 			},
 			wantLogLines: []string{
 				fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:<line>$webhookcachefiller.(*webhookCacheFillerController).havePreviouslyValidated","message":"cached webhook authenticator and desired webhook authenticator are the same: already cached, so skipping validations","webhookAuthenticator":"test-name","endpoint":"%s"}`, goodWebhookDefaultServingCertEndpoint),
+				fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:<line>$webhookcachefiller.(*webhookCacheFillerController).updateStatus","message":"choosing to not update the webhookauthenticator status since there is no update to make","webhookAuthenticator":"test-name","endpoint":"%s","phase":"Ready"}`, goodWebhookDefaultServingCertEndpoint),
 			},
 			wantActions: func() []coretesting.Action {
 				return []coretesting.Action{
@@ -950,6 +951,66 @@ func TestController(t *testing.T) {
 			wantSyncErr:                             testutil.WantExactErrorString("error for WebhookAuthenticator test-name: some update error"),
 			wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"}, // keeps the old entry in the cache
 		},
+		{
+			name: "Sync: previously cached valid authenticator with unchanged endpoint URL and CA bundle hash has invalid status conditions in informer cache, as can happen on subsequent sync soon after multiple quick status updates (when the informer cache finally catches up): should update status in current sync",
+			cache: func(t *testing.T, cache *authncache.Cache) {
+				oldCA, err := base64.StdEncoding.DecodeString(goodWebhookAuthenticatorSpecWithCA.TLS.CertificateAuthorityData)
+				require.NoError(t, err)
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "WebhookAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					newCacheValue(t, goodWebhookAuthenticatorSpecWithCA, string(oldCA)),
+				)
+			},
+			webhookAuthenticators: []runtime.Object{
+				&authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test-name",
+						Generation: 1234,
+					},
+					Spec: goodWebhookAuthenticatorSpecWithCA,
+					Status: authenticationv1alpha1.WebhookAuthenticatorStatus{
+						Conditions: conditionstestutil.Replace(
+							allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0),
+							[]metav1.Condition{
+								sadTLSConfigurationValid(frozenMetav1Now, 0),
+								unknownWebhookConnectionValid(frozenMetav1Now, 0),
+								unknownAuthenticatorValid(frozenMetav1Now, 0),
+								sadReadyCondition(frozenMetav1Now, 0),
+							},
+						),
+						Phase: "Error",
+					},
+				},
+			},
+			wantLogLines: []string{
+				fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:<line>$webhookcachefiller.(*webhookCacheFillerController).havePreviouslyValidated","message":"cached webhook authenticator and desired webhook authenticator are the same: already cached, so skipping validations","webhookAuthenticator":"test-name","endpoint":"%s"}`, goodWebhookAuthenticatorSpecWithCA.Endpoint),
+				fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"webhookcachefiller-controller","caller":"webhookcachefiller/webhookcachefiller.go:<line>$webhookcachefiller.(*webhookCacheFillerController).updateStatus","message":"webhookauthenticator status successfully updated","webhookAuthenticator":"test-name","endpoint":"%s","phase":"Ready"}`, goodWebhookAuthenticatorSpecWithCA.Endpoint),
+			},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test-name",
+						Generation: 1234,
+					},
+					Spec: goodWebhookAuthenticatorSpecWithCA,
+					Status: authenticationv1alpha1.WebhookAuthenticatorStatus{ // updates the status to ready
+						Conditions: allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 1234),
+						Phase:      "Ready",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantNamesOfWebhookAuthenticatorsInCache: []string{"test-name"}, // keeps the old entry in the cache
+		},
 		{
 			name: "Sync: valid WebhookAuthenticator with CA: will complete sync loop successfully with success conditions and ready phase",
 			webhookAuthenticators: []runtime.Object{