From 09724cfa712bd7aced2854540a0d0ebe39cd5529 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 23 Jul 2024 13:40:13 -0700 Subject: [PATCH] Add unit test: when discovery is already cached for OIDCIdentityProvider --- .../oidc_upstream_watcher.go | 13 +- .../oidc_upstream_watcher_test.go | 111 ++++++++++++++++++ internal/supervisor/server/server.go | 1 + 3 files changed, 119 insertions(+), 6 deletions(-) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 9a73aea91..a2a85f21c 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -109,8 +109,8 @@ type oidcDiscoveryCacheValue struct { // oidcDiscoveryCache caches the discovered provider along with the http Client to use for making calls to that provider, // for a particular combination OIDC issuer and CA bundle for that issuer. type oidcDiscoveryCache interface { - getProvider(*oidcDiscoveryCacheKey) *oidcDiscoveryCacheValue - putProvider(*oidcDiscoveryCacheKey, *oidcDiscoveryCacheValue) + getProvider(oidcDiscoveryCacheKey) *oidcDiscoveryCacheValue + putProvider(oidcDiscoveryCacheKey, *oidcDiscoveryCacheValue) } // ttlProviderCache caches the *coreosoidc.Provider associated with a particular issuer/TLS configuration, @@ -121,7 +121,7 @@ type ttlProviderCache struct{ cache *cache.Expiring } var _ oidcDiscoveryCache = (*ttlProviderCache)(nil) // getProvider gets an entry from the ttlProviderCache. -func (c *ttlProviderCache) getProvider(key *oidcDiscoveryCacheKey) *oidcDiscoveryCacheValue { +func (c *ttlProviderCache) getProvider(key oidcDiscoveryCacheKey) *oidcDiscoveryCacheValue { if result, ok := c.cache.Get(key); ok { entry := result.(*oidcDiscoveryCacheValue) return entry @@ -130,7 +130,7 @@ func (c *ttlProviderCache) getProvider(key *oidcDiscoveryCacheKey) *oidcDiscover } // putProvider adds to the ttlProviderCache for a limited period of time. -func (c *ttlProviderCache) putProvider(key *oidcDiscoveryCacheKey, value *oidcDiscoveryCacheValue) { +func (c *ttlProviderCache) putProvider(key oidcDiscoveryCacheKey, value *oidcDiscoveryCacheValue) { c.cache.Set(key, value, oidcValidatorCacheTTL) } @@ -153,6 +153,7 @@ func New( configMapInformer corev1informers.ConfigMapInformer, log plog.Logger, withInformer pinnipedcontroller.WithInformerOptionFunc, + validatorCache *cache.Expiring, ) controllerlib.Controller { c := oidcWatcherController{ cache: idpCache, @@ -161,7 +162,7 @@ func New( oidcIdentityProviderInformer: oidcIdentityProviderInformer, secretInformer: secretInformer, configMapInformer: configMapInformer, - validatorCache: &ttlProviderCache{cache: cache.NewExpiring()}, + validatorCache: &ttlProviderCache{cache: validatorCache}, } return controllerlib.New( controllerlib.Config{Name: oidcControllerName, Syncer: &c}, @@ -357,7 +358,7 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *id var httpClient *http.Client // Get the discovered provider and HTTP client from cache, if they are found in the cache. - cacheKey := &oidcDiscoveryCacheKey{ + cacheKey := oidcDiscoveryCacheKey{ issuer: upstream.Spec.Issuer, caBundleHash: sha256.Sum256(caBundlePEM), // note that this will always return the same hash for nil input } diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index c3ea5fda2..ed5968185 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -6,6 +6,8 @@ package oidcupstreamwatcher import ( "bytes" "context" + "crypto/sha256" + "crypto/x509" "encoding/base64" "encoding/json" "net/http" @@ -15,11 +17,13 @@ import ( "testing" "time" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + expiringcache "k8s.io/apimachinery/pkg/util/cache" "k8s.io/apimachinery/pkg/util/net" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" @@ -31,6 +35,7 @@ import ( "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/federationdomain/dynamicupstreamprovider" "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/net/phttp" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" @@ -119,6 +124,7 @@ func TestOIDCUpstreamWatcherControllerFilterSecret(t *testing.T) { configMapInformer, logger, withInformer.WithInformer, + expiringcache.NewExpiring(), ) unrelated := corev1.Secret{} @@ -178,6 +184,7 @@ func TestOIDCUpstreamWatcherControllerFilterConfigMaps(t *testing.T) { configMapInformer, logger, withInformer.WithInformer, + expiringcache.NewExpiring(), ) unrelated := corev1.ConfigMap{} @@ -237,6 +244,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { name string inputUpstreams []runtime.Object inputResources []runtime.Object + inputValidatorCache func(*testing.T) map[oidcDiscoveryCacheKey]*oidcDiscoveryCacheValue wantErr string wantLogs []string wantResultingCache []*oidctestutil.TestUpstreamOIDCIdentityProvider @@ -1054,6 +1062,99 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }, }}, }, + { + name: "valid upstream which already exists in the OIDC discovery validation cache, should skip performing OIDC discovery again and just use cached discovery results", + inputUpstreams: []runtime.Object{&idpv1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Spec: idpv1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/this-path-does-not-exist", + TLS: &idpv1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: idpv1alpha1.OIDCClient{SecretName: testSecretName}, + Claims: idpv1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim}, + }, + Status: idpv1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + // Was previously validated, so already has conditions. + Conditions: []metav1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "discovered issuer configuration", ObservedGeneration: 1234}, + {Type: "TLSConfigurationValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "spec.tls is valid: loaded TLS configuration", ObservedGeneration: 1234}, + }, + }, + }}, + inputResources: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + inputValidatorCache: func(t *testing.T) map[oidcDiscoveryCacheKey]*oidcDiscoveryCacheValue { + // Create a working OIDC discovery validator cache entry for the working issuer and CA bundle. + certPool := x509.NewCertPool() + require.True(t, certPool.AppendCertsFromPEM([]byte(testIssuerCA))) + httpClient := phttp.Default(certPool) + httpClient.Timeout = time.Minute // same timeout as in the production code + testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + // Really do OIDC discovery, so we can put the real result into the cache. + discoveredProvider, err := coreosoidc.NewProvider(coreosoidc.ClientContext(testCtx, httpClient), testIssuerURL) + require.NoError(t, err) + cacheValue := &oidcDiscoveryCacheValue{ + provider: discoveredProvider, + client: httpClient, + } + // Create the cache key to use with the above entry, and cache it at the issuer value that was + // configured in the OIDCIdentityProvider. If the production code tries to perform OIDC discovery + // on that URL, it will fail with a 404. But if the production code correctly reads the pre-cached + // discovery result from this cache, then it should skip discovery and use the value from this cache + // without encountering any errors. + cacheKey := oidcDiscoveryCacheKey{ + issuer: testIssuerURL + "/this-path-does-not-exist", + caBundleHash: sha256.Sum256([]byte(testIssuerCA)), + } + // Put it into the initial cache for this test. + return map[oidcDiscoveryCacheKey]*oidcDiscoveryCacheValue{ + cacheKey: cacheValue, + } + }, + wantLogs: []string{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { + Name: testName, + ClientID: testClientID, + AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, + Scopes: testDefaultExpectedScopes, + UsernameClaim: testUsernameClaim, + GroupsClaim: testGroupsClaim, + AllowPasswordGrant: false, + AdditionalAuthcodeParams: map[string]string{}, + AdditionalClaimMappings: nil, // Does not default to empty map + ResourceUID: testUID, + }, + }, + wantResultingUpstreams: []idpv1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Status: idpv1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + // Conditions are unchanged. + Conditions: []metav1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, + {Type: "ClientCredentialsSecretValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "discovered issuer configuration", ObservedGeneration: 1234}, + {Type: "TLSConfigurationValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", + Message: "spec.tls is valid: loaded TLS configuration", ObservedGeneration: 1234}, + }, + }, + }}, + }, { name: "valid upstream with CA bundle read from a Secret", inputUpstreams: []runtime.Object{&idpv1alpha1.OIDCIdentityProvider{ @@ -1550,6 +1651,15 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { var log bytes.Buffer logger := plog.TestLogger(t, &log) + validatorCache := expiringcache.NewExpiring() + if tt.inputValidatorCache != nil { + oidcValidatorCache := &ttlProviderCache{cache: validatorCache} + // add to the underlying validatorCache using oidcValidatorCache which wraps it + for key, value := range tt.inputValidatorCache(t) { + oidcValidatorCache.putProvider(key, value) + } + } + controller := New( cache, fakePinnipedClient, @@ -1558,6 +1668,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { kubeInformers.Core().V1().ConfigMaps(), logger, controllerlib.WithInformer, + validatorCache, ) ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 15c75e378..c2fdceb4b 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -308,6 +308,7 @@ func prepareControllers( configMapInformer, plog.New(), controllerlib.WithInformer, + cache.NewExpiring(), ), singletonWorker). WithController(