diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 56c84a7e4..93b752e7a 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -119,6 +119,9 @@ type cachedJWTAuthenticator struct { } func (c *cachedJWTAuthenticator) Close() { + if c == nil { + return + } c.cancel() } @@ -162,12 +165,12 @@ type jwtCacheFillerController struct { // Sync implements controllerlib.Syncer. func (c *jwtCacheFillerController) Sync(ctx controllerlib.Context) error { obj, err := c.jwtAuthenticators.Lister().Get(ctx.Key.Name) - if err != nil && apierrors.IsNotFound(err) { c.log.Info("Sync() found that the JWTAuthenticator does not exist yet or was deleted") return nil } if err != nil { + // no unit test for this failure return fmt.Errorf("failed to get JWTAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) } @@ -179,33 +182,29 @@ func (c *jwtCacheFillerController) Sync(ctx controllerlib.Context) error { // If this authenticator already exists, then only recreate it if is different from the desired // authenticator. We don't want to be creating a new authenticator for every resync period. - // - // If we do need to recreate the authenticator, then make sure cancel the old one to avoid - // goroutine leaks. - if value := c.cache.Get(cacheKey); value != nil { - jwtAuthenticator := c.extractValueAsJWTAuthenticator(value) - if jwtAuthenticator != nil { - if reflect.DeepEqual(jwtAuthenticator.spec, &obj.Spec) { - c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer).Info("actual jwt authenticator and desired jwt authenticator are the same") - return nil - } - jwtAuthenticator.Close() + var jwtAuthenticatorFromCache *cachedJWTAuthenticator + if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil { + jwtAuthenticatorFromCache = c.cacheValueAsJWTAuthenticator(valueFromCache) + if jwtAuthenticatorFromCache != nil && reflect.DeepEqual(jwtAuthenticatorFromCache.spec, &obj.Spec) { + c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.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. + return nil } } conditions := make([]*metav1.Condition, 0) - specCopy := obj.Spec.DeepCopy() var errs []error - rootCAs, conditions, tlsOk := c.validateTLS(specCopy.TLS, conditions) - _, conditions, issuerOk := c.validateIssuer(specCopy.Issuer, conditions) + rootCAs, conditions, tlsOk := c.validateTLSBundle(obj.Spec.TLS, conditions) + _, conditions, issuerOk := c.validateIssuer(obj.Spec.Issuer, conditions) okSoFar := tlsOk && issuerOk client := phttp.Default(rootCAs) client.Timeout = 30 * time.Second // copied from Kube OIDC code coreOSCtx := coreosoidc.ClientContext(context.Background(), client) - pJSON, provider, conditions, providerErr := c.validateProviderDiscovery(coreOSCtx, specCopy.Issuer, conditions, okSoFar) + pJSON, provider, conditions, providerErr := c.validateProviderDiscovery(coreOSCtx, obj.Spec.Issuer, conditions, okSoFar) errs = append(errs, providerErr) okSoFar = okSoFar && providerErr == nil @@ -217,21 +216,29 @@ func (c *jwtCacheFillerController) Sync(ctx controllerlib.Context) error { errs = append(errs, jwksFetchErr) okSoFar = okSoFar && jwksFetchErr == nil - // Make a deep copy of the spec so we aren't storing pointers to something that the informer cache - // may mutate! We don't store status as status is derived from spec. - cachedAuthenticator, conditions, err := c.newCachedJWTAuthenticator( + newJWTAuthenticatorForCache, conditions, err := c.newCachedJWTAuthenticator( client, - obj.Spec.DeepCopy(), + obj.Spec.DeepCopy(), // deep copy to avoid caching original object keySet, conditions, okSoFar) errs = append(errs, err) - if !conditionsutil.HadErrorCondition(conditions) { - c.cache.Store(cacheKey, cachedAuthenticator) - c.log.Info("added new jwt authenticator", "jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer) + if conditionsutil.HadErrorCondition(conditions) { + // 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 { + c.cache.Store(cacheKey, newJWTAuthenticatorForCache) + c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.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. + jwtAuthenticatorFromCache.Close() + err = c.updateStatus(ctx.Context, obj, conditions) errs = append(errs, err) @@ -243,7 +250,7 @@ func (c *jwtCacheFillerController) Sync(ctx controllerlib.Context) error { return utilerrors.NewAggregate(errs) } -func (c *jwtCacheFillerController) extractValueAsJWTAuthenticator(value authncache.Value) *cachedJWTAuthenticator { +func (c *jwtCacheFillerController) cacheValueAsJWTAuthenticator(value authncache.Value) *cachedJWTAuthenticator { jwtAuthenticator, ok := value.(*cachedJWTAuthenticator) if !ok { actualType := "" @@ -256,7 +263,7 @@ func (c *jwtCacheFillerController) extractValueAsJWTAuthenticator(value authncac return jwtAuthenticator } -func (c *jwtCacheFillerController) validateTLS(tlsSpec *authenticationv1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) { +func (c *jwtCacheFillerController) validateTLSBundle(tlsSpec *authenticationv1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) { rootCAs, _, err := pinnipedcontroller.BuildCertPoolAuth(tlsSpec) if err != nil { msg := fmt.Sprintf("%s: %s", "invalid TLS configuration", err.Error()) diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index e0fd73af5..1f0ab2c19 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -10,6 +10,7 @@ import ( "crypto/x509" "fmt" "net/url" + "reflect" "time" k8sauthv1beta1 "k8s.io/api/authentication/v1beta1" @@ -56,6 +57,11 @@ const ( msgUnableToValidate = "unable to validate; see other conditions for details" ) +type cachedWebhookAuthenticator struct { + authenticator.Token + spec *authenticationv1alpha1.WebhookAuthenticatorSpec +} + // New instantiates a new controllerlib.Controller which will populate the provided authncache.Cache. func New( cache *authncache.Cache, @@ -103,19 +109,38 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to get WebhookAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) } + cacheKey := authncache.Key{ + APIGroup: authenticationv1alpha1.GroupName, + Kind: "WebhookAuthenticator", + Name: ctx.Key.Name, + } + + // If this authenticator already exists, then only recreate it if is different from the desired + // authenticator. We don't want to be creating a new authenticator for every resync period. + if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil { + webhookAuthenticatorFromCache := c.cacheValueAsWebhookAuthenticator(valueFromCache) + if webhookAuthenticatorFromCache != nil && reflect.DeepEqual(webhookAuthenticatorFromCache.spec, &obj.Spec) { + c.log.WithValues("webhookAuthenticator", klog.KObj(obj), "endpoint", obj.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. + return nil + } + } + conditions := make([]*metav1.Condition, 0) var errs []error certPool, pemBytes, conditions, tlsBundleOk := c.validateTLSBundle(obj.Spec.TLS, conditions) endpointHostPort, conditions, endpointOk := c.validateEndpoint(obj.Spec.Endpoint, conditions) okSoFar := tlsBundleOk && endpointOk + conditions, tlsNegotiateErr := c.validateConnection(certPool, endpointHostPort, conditions, okSoFar) errs = append(errs, tlsNegotiateErr) okSoFar = okSoFar && tlsNegotiateErr == nil - webhookAuthenticator, conditions, err := newWebhookAuthenticator( + newWebhookAuthenticatorForCache, conditions, err := newWebhookAuthenticator( // Note that we use the whole URL when constructing the webhook client, - // not just the host and port that ew validated above. We need the path, etc. + // not just the host and port that we validated above. We need the path, etc. obj.Spec.Endpoint, pemBytes, conditions, @@ -123,13 +148,18 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { ) errs = append(errs, err) - if !conditionsutil.HadErrorCondition(conditions) { - c.cache.Store(authncache.Key{ - APIGroup: authenticationv1alpha1.GroupName, - Kind: "WebhookAuthenticator", - Name: ctx.Key.Name, - }, webhookAuthenticator) - c.log.WithValues("webhook", klog.KObj(obj), "endpoint", obj.Spec.Endpoint).Info("added new webhook authenticator") + if conditionsutil.HadErrorCondition(conditions) { + // 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 { + c.cache.Store(cacheKey, &cachedWebhookAuthenticator{ + Token: newWebhookAuthenticatorForCache, + spec: obj.Spec.DeepCopy(), // deep copy to avoid caching original object + }) + c.log.WithValues("webhook", klog.KObj(obj), "endpoint", obj.Spec.Endpoint). + Info("added new webhook authenticator") } err = c.updateStatus(ctx.Context, obj, conditions) @@ -143,6 +173,19 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { return utilerrors.NewAggregate(errs) } +func (c *webhookCacheFillerController) cacheValueAsWebhookAuthenticator(value authncache.Value) *cachedWebhookAuthenticator { + webhookAuthenticator, ok := value.(*cachedWebhookAuthenticator) + if !ok { + actualType := "" + if t := reflect.TypeOf(value); t != nil { + actualType = t.String() + } + c.log.WithValues("actualType", actualType).Info("wrong webhook authenticator type in cache") + return nil + } + return webhookAuthenticator +} + // newWebhookAuthenticator creates a webhook from the provided API server url and caBundle // used to validate TLS connections. func newWebhookAuthenticator(