From 15c84fcc940e574a83e6241bc231f0b88d66cca0 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 30 Jul 2024 16:41:50 -0700 Subject: [PATCH] extract helper func in jwtcachefiller and webhookcachefiller --- .../jwtcachefiller/jwtcachefiller.go | 93 +++++++++++-------- .../jwtcachefiller/jwtcachefiller_test.go | 6 +- .../webhookcachefiller/webhookcachefiller.go | 76 +++++++++------ .../webhookcachefiller_test.go | 4 +- 4 files changed, 112 insertions(+), 67 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index e1682d72c..e3eedaeeb 100644 --- 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 := "" 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 diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 8c2e16b08..b69c90137 100644 --- 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 diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index 9112ab55d..1856ab96e 100644 --- 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 { diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index 9d365f7bc..86fc352ac 100644 --- 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:$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:$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)), } }