extract helper func in jwtcachefiller and webhookcachefiller

2026-01-03 11:45:45 +00:00 · 2024-07-30 16:41:50 -07:00
parent 1438f06c12
commit 15c84fcc94
4 changed files with 112 additions and 67 deletions
--- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
+++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
@@ -111,7 +111,7 @@ type tokenAuthenticatorCloser interface {

 type cachedJWTAuthenticator struct {
 	authenticator.Token
-	spec         *authenticationv1alpha1.JWTAuthenticatorSpec
+	issuer       string
 	caBundleHash tlsconfigutil.CABundleHash
 	cancel       context.CancelFunc
 }
@@ -220,9 +220,18 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 		Name:     jwtAuthenticator.Name,
 	}

+	logger := c.log.WithValues(
+		"jwtAuthenticator", klog.KObj(jwtAuthenticator),
+		"issuer", jwtAuthenticator.Spec.Issuer)
+
+	var errs []error
 	conditions := make([]*metav1.Condition, 0)
+
 	caBundle, conditions, tlsBundleOk := c.validateTLSBundle(jwtAuthenticator.Spec.TLS, conditions)

+	conditions, issuerOk := c.validateIssuer(jwtAuthenticator.Spec.Issuer, conditions)
+	okSoFar := tlsBundleOk && issuerOk
+
 	// 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
@@ -231,24 +240,15 @@ 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 oldJWTAuthenticatorFromCache *cachedJWTAuthenticator
-	if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil {
-		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
-			oldJWTAuthenticatorFromCache.caBundleHash.Equal(caBundle.Hash()) {
-			c.log.WithValues("jwtAuthenticator", klog.KObj(jwtAuthenticator), "issuer", jwtAuthenticator.Spec.Issuer).
-				Info("cached jwt authenticator and desired jwt authenticator are the same: already cached, so skipping validations")
-			// Stop, no more work to be done. This authenticator is already validated and cached.
-			return nil
-		}
+	foundAuthenticatorInCache, alreadyValidatedTheseSettings := c.havePreviouslyValidated(
+		cacheKey, jwtAuthenticator.Spec.Issuer, 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
 	}

-	var errs []error
-	_, conditions, issuerOk := c.validateIssuer(jwtAuthenticator.Spec.Issuer, conditions)
-	okSoFar := tlsBundleOk && issuerOk
-
 	client := phttp.Default(caBundle.CertPool())
 	client.Timeout = 30 * time.Second // copied from Kube OIDC code
 	coreOSCtx := coreosoidc.ClientContext(context.Background(), client)
@@ -283,11 +283,8 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 		// validated and cached. Do not allow an old, previously validated spec of the authenticator to continue
 		// being used for authentication.
 		c.cache.Delete(cacheKey)
-		c.log.WithValues(
-			"jwtAuthenticator", klog.KObj(jwtAuthenticator),
-			"issuer", jwtAuthenticator.Spec.Issuer,
-			"removedFromCache", oldJWTAuthenticatorFromCache != nil,
-		).Info("invalid jwt authenticator")
+		logger.Info("invalid jwt authenticator",
+			"removedFromCache", foundAuthenticatorInCache)
 	}

 	updateErr := c.updateStatus(ctx, jwtAuthenticator, conditions)
@@ -298,11 +295,8 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 	// 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,
-			"isOverwrite", oldJWTAuthenticatorFromCache != nil,
-		).Info("added or updated jwt authenticator in cache")
+		logger.Info("added or updated jwt authenticator in cache",
+			"isOverwrite", foundAuthenticatorInCache)
 	}

 	// Sync loop errors:
@@ -313,14 +307,39 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 	return utilerrors.NewAggregate(errs)
 }

-func (c *jwtCacheFillerController) cacheValueAsJWTAuthenticator(value authncache.Value) *cachedJWTAuthenticator {
+func (c *jwtCacheFillerController) havePreviouslyValidated(
+	cacheKey authncache.Key,
+	issuer string,
+	tlsBundleOk bool,
+	caBundleHash tlsconfigutil.CABundleHash,
+	logger plog.Logger,
+) (bool, bool) {
+	var authenticatorFromCache *cachedJWTAuthenticator
+	valueFromCache := c.cache.Get(cacheKey)
+	if valueFromCache == nil {
+		return false, false
+	}
+	authenticatorFromCache = c.cacheValueAsJWTAuthenticator(valueFromCache, logger)
+	if authenticatorFromCache == nil {
+		return false, false
+	}
+	if authenticatorFromCache.issuer == issuer &&
+		tlsBundleOk && // if there was any error while validating the latest CA bundle, then do not consider it previously validated
+		authenticatorFromCache.caBundleHash.Equal(caBundleHash) {
+		logger.Info("cached jwt authenticator and desired jwt authenticator are the same: already cached, so skipping validations")
+		return true, true
+	}
+	return true, false // found the authenticator, but it had not been previously validated with these same settings
+}
+
+func (c *jwtCacheFillerController) cacheValueAsJWTAuthenticator(value authncache.Value, logger plog.Logger) *cachedJWTAuthenticator {
 	jwtAuthenticator, ok := value.(*cachedJWTAuthenticator)
 	if !ok {
 		actualType := "<nil>"
 		if t := reflect.TypeOf(value); t != nil {
 			actualType = t.String()
 		}
-		c.log.WithValues("actualType", actualType).Info("wrong JWT authenticator type in cache")
+		logger.WithValues("actualType", actualType).Info("wrong JWT authenticator type in cache")
 		return nil
 	}
 	return jwtAuthenticator
@@ -338,7 +357,7 @@ func (c *jwtCacheFillerController) validateTLSBundle(tlsSpec *authenticationv1al
 	return caBundle, conditions, condition.Status == metav1.ConditionTrue
 }

-func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*metav1.Condition) (*url.URL, []*metav1.Condition, bool) {
+func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*metav1.Condition) ([]*metav1.Condition, bool) {
 	issuerURL, err := url.Parse(issuer)
 	if err != nil {
 		msg := fmt.Sprintf("%s: %s", "spec.issuer URL is invalid", err.Error())
@@ -348,7 +367,7 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
 			Reason:  conditionsutil.ReasonInvalidIssuerURL,
 			Message: msg,
 		})
-		return nil, conditions, false
+		return conditions, false
 	}

 	if issuerURL.Scheme != "https" {
@@ -359,7 +378,7 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
 			Reason:  reasonInvalidIssuerURLScheme,
 			Message: msg,
 		})
-		return nil, conditions, false
+		return conditions, false
 	}

 	if strings.HasSuffix(issuerURL.Path, "/.well-known/openid-configuration") {
@@ -370,7 +389,7 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
 			Reason:  reasonInvalidIssuerURLContainsWellKnownEndpoint,
 			Message: msg,
 		})
-		return nil, conditions, false
+		return conditions, false
 	}

 	if len(issuerURL.Query()) != 0 {
@@ -381,7 +400,7 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
 			Reason:  reasonInvalidIssuerURLQueryParams,
 			Message: msg,
 		})
-		return nil, conditions, false
+		return conditions, false
 	}

 	if issuerURL.Fragment != "" {
@@ -392,7 +411,7 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
 			Reason:  reasonInvalidIssuerURLFragment,
 			Message: msg,
 		})
-		return nil, conditions, false
+		return conditions, false
 	}

 	conditions = append(conditions, &metav1.Condition{
@@ -401,7 +420,7 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m
 		Reason:  conditionsutil.ReasonSuccess,
 		Message: "issuer is a valid URL",
 	})
-	return issuerURL, conditions, true
+	return conditions, true
 }

 func (c *jwtCacheFillerController) validateProviderDiscovery(ctx context.Context, issuer string, conditions []*metav1.Condition, prereqOk bool) (*providerJSON, *coreosoidc.Provider, []*metav1.Condition, error) {
@@ -646,7 +665,7 @@ func (c *jwtCacheFillerController) newCachedJWTAuthenticator(
 	})
 	return &cachedJWTAuthenticator{
 		Token:        oidcAuthenticator,
-		spec:         spec,
+		issuer:       spec.Issuer,
 		caBundleHash: caBundleHash,
 		cancel:       cancel,
 	}, conditions, nil
--- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go
+++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go
@@ -1444,6 +1444,10 @@ func TestController(t *testing.T) {
 					"logger":     "jwtcachefiller-controller",
 					"message":    "wrong JWT authenticator type in cache",
 					"actualType": "*mockcachevalue.MockValue",
+					"issuer":     goodIssuer,
+					"jwtAuthenticator": map[string]any{
+						"name": "test-name",
+					},
 				},
 				{
 					"level":       "info",
@@ -2914,7 +2918,7 @@ func newCacheValue(t *testing.T, spec authenticationv1alpha1.JWTAuthenticatorSpe
 	})

 	return &cachedJWTAuthenticator{
-		spec:         &spec,
+		issuer:       spec.Issuer,
 		caBundleHash: tlsconfigutil.NewCABundleHash([]byte(caBundle)),
 		cancel: func() {
 			wasClosed = true
--- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go
+++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go
@@ -60,7 +60,7 @@ const (

 type cachedWebhookAuthenticator struct {
 	authenticator.Token
-	spec         *authenticationv1alpha1.WebhookAuthenticatorSpec
+	endpoint     string
 	caBundleHash tlsconfigutil.CABundleHash
 }

@@ -163,13 +163,18 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 		Name:     webhookAuthenticator.Name,
 	}

-	conditions := make([]*metav1.Condition, 0)
-	caBundle, conditions, tlsBundleOk := c.validateTLSBundle(webhookAuthenticator.Spec.TLS, conditions)
-
-	webhookSpecificLogger := c.log.WithValues(
+	logger := c.log.WithValues(
 		"webhookAuthenticator", webhookAuthenticator.Name,
 		"endpoint", webhookAuthenticator.Spec.Endpoint)

+	var errs []error
+	conditions := make([]*metav1.Condition, 0)
+
+	caBundle, conditions, tlsBundleOk := c.validateTLSBundle(webhookAuthenticator.Spec.TLS, conditions)
+
+	endpointHostPort, conditions, endpointOk := c.validateEndpoint(webhookAuthenticator.Spec.Endpoint, conditions)
+	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
@@ -178,24 +183,16 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx 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 oldWebhookAuthenticatorFromCache *cachedWebhookAuthenticator
-	if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil {
-		oldWebhookAuthenticatorFromCache = c.cacheValueAsWebhookAuthenticator(valueFromCache, webhookSpecificLogger)
-		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
-			oldWebhookAuthenticatorFromCache.caBundleHash.Equal(caBundle.Hash()) {
-			webhookSpecificLogger.Info("cached webhook authenticator and desired webhook authenticator are the same: already cached, so skipping validations")
-			// Stop, no more work to be done. This authenticator is already validated and cached.
-			return nil
-		}
+	foundAuthenticatorInCache, alreadyValidatedTheseSettings := 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
 	}

-	var errs []error
-	endpointHostPort, conditions, endpointOk := c.validateEndpoint(webhookAuthenticator.Spec.Endpoint, conditions)
-	okSoFar := tlsBundleOk && endpointOk
-
-	conditions, tlsNegotiateErr := c.validateConnection(caBundle.CertPool(), endpointHostPort, conditions, okSoFar, webhookSpecificLogger)
+	conditions, tlsNegotiateErr := c.validateConnection(caBundle.CertPool(), endpointHostPort, conditions, okSoFar, logger)
 	errs = append(errs, tlsNegotiateErr)
 	okSoFar = okSoFar && tlsNegotiateErr == nil

@@ -218,11 +215,11 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 		// validated and cached. Do not allow an old, previously validated spec of the authenticator to continue
 		// being used for authentication.
 		c.cache.Delete(cacheKey)
-		webhookSpecificLogger.Info("invalid webhook authenticator",
-			"removedFromCache", oldWebhookAuthenticatorFromCache != nil)
+		logger.Info("invalid webhook authenticator",
+			"removedFromCache", foundAuthenticatorInCache)
 	}

-	updateErr := c.updateStatus(ctx, webhookAuthenticator, conditions, webhookSpecificLogger)
+	updateErr := c.updateStatus(ctx, webhookAuthenticator, conditions, logger)
 	errs = append(errs, updateErr)

 	// Only add this WebhookAuthenticator to the cache if the status update succeeds.
@@ -231,11 +228,11 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 	if authenticatorValid && updateErr == nil {
 		c.cache.Store(cacheKey, &cachedWebhookAuthenticator{
 			Token:        newWebhookAuthenticatorForCache,
-			spec:         webhookAuthenticator.Spec.DeepCopy(), // deep copy to avoid caching original object
+			endpoint:     webhookAuthenticator.Spec.Endpoint,
 			caBundleHash: caBundle.Hash(),
 		})
-		webhookSpecificLogger.Info("added or updated webhook authenticator in cache",
-			"isOverwrite", oldWebhookAuthenticatorFromCache != nil)
+		logger.Info("added or updated webhook authenticator in cache",
+			"isOverwrite", foundAuthenticatorInCache)
 	}

 	// Sync loop errors:
@@ -246,6 +243,31 @@ func (c *webhookCacheFillerController) syncIndividualWebhookAuthenticator(ctx co
 	return utilerrors.NewAggregate(errs)
 }

+func (c *webhookCacheFillerController) havePreviouslyValidated(
+	cacheKey authncache.Key,
+	endpoint string,
+	tlsBundleOk bool,
+	caBundleHash tlsconfigutil.CABundleHash,
+	logger plog.Logger,
+) (bool, bool) {
+	var authenticatorFromCache *cachedWebhookAuthenticator
+	valueFromCache := c.cache.Get(cacheKey)
+	if valueFromCache == nil {
+		return false, false
+	}
+	authenticatorFromCache = c.cacheValueAsWebhookAuthenticator(valueFromCache, logger)
+	if authenticatorFromCache == nil {
+		return false, false
+	}
+	if authenticatorFromCache.endpoint == endpoint &&
+		tlsBundleOk && // if there was any error while validating the latest CA bundle, then do not consider it previously validated
+		authenticatorFromCache.caBundleHash.Equal(caBundleHash) {
+		logger.Info("cached webhook authenticator and desired webhook authenticator are the same: already cached, so skipping validations")
+		return true, true
+	}
+	return true, false // found the authenticator, but it had not been previously validated with these same settings
+}
+
 func (c *webhookCacheFillerController) cacheValueAsWebhookAuthenticator(value authncache.Value, log plog.Logger) *cachedWebhookAuthenticator {
 	webhookAuthenticator, ok := value.(*cachedWebhookAuthenticator)
 	if !ok {
--- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go
+++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go
@@ -768,7 +768,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).syncIndividualWebhookAuthenticator","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":"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),
 			},
 			wantActions: func() []coretesting.Action {
 				return []coretesting.Action{
@@ -2038,7 +2038,7 @@ func newCacheValue(t *testing.T, spec authenticationv1alpha1.WebhookAuthenticato
 	t.Helper()

 	return &cachedWebhookAuthenticator{
-		spec:         &spec,
+		endpoint:     spec.Endpoint,
 		caBundleHash: tlsconfigutil.NewCABundleHash([]byte(caBundle)),
 	}
 }