Finish first implementation of generic secret generator controller

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

cmd/pinniped-supervisor/main.go

@@ -5,6 +5,7 @@ package main
 
 import (
 	"context"
+	"crypto/rand"
 	"crypto/tls"
 	"fmt"
 	"net"
@@ -17,6 +18,7 @@ import (
 	"go.pinniped.dev/internal/secret"
 
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/clock"
 	kubeinformers "k8s.io/client-go/informers"
@@ -28,11 +30,13 @@ import (
 	"k8s.io/klog/v2"
 	"k8s.io/klog/v2/klogr"
 
+	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
 	pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned"
 	pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions"
 	"go.pinniped.dev/internal/config/supervisor"
 	"go.pinniped.dev/internal/controller/supervisorconfig"
 	"go.pinniped.dev/internal/controller/supervisorconfig/generator"
+	"go.pinniped.dev/internal/controller/supervisorconfig/generator/symmetricsecrethelper"
 	"go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/downward"
@@ -88,6 +92,9 @@ func startControllers(
 	kubeInformers kubeinformers.SharedInformerFactory,
 	pinnipedInformers pinnipedinformers.SharedInformerFactory,
 ) {
+	opInformer := pinnipedInformers.Config().V1alpha1().OIDCProviders()
+	secretInformer := kubeInformers.Core().V1().Secrets()
+
 	// Create controller manager.
 	controllerManager := controllerlib.
 		NewManager().
@@ -96,7 +103,7 @@ func startControllers(
 				issuerManager,
 				clock.RealClock{},
 				pinnipedClient,
-				pinnipedInformers.Config().V1alpha1().OIDCProviders(),
+				opInformer,
 				controllerlib.WithInformer,
 			),
 			singletonWorker,
@@ -106,8 +113,8 @@ func startControllers(
 				cfg.Labels,
 				kubeClient,
 				pinnipedClient,
-				kubeInformers.Core().V1().Secrets(),
+				secretInformer,
-				pinnipedInformers.Config().V1alpha1().OIDCProviders(),
+				opInformer,
 				controllerlib.WithInformer,
 			),
 			singletonWorker,
@@ -115,8 +122,8 @@ func startControllers(
 		WithController(
 			supervisorconfig.NewJWKSObserverController(
 				dynamicJWKSProvider,
-				kubeInformers.Core().V1().Secrets(),
+				secretInformer,
-				pinnipedInformers.Config().V1alpha1().OIDCProviders(),
+				opInformer,
 				controllerlib.WithInformer,
 			),
 			singletonWorker,
@@ -125,8 +132,8 @@ func startControllers(
 			supervisorconfig.NewTLSCertObserverController(
 				dynamicTLSCertProvider,
 				cfg.NamesConfig.DefaultTLSCertificateSecret,
-				kubeInformers.Core().V1().Secrets(),
+				secretInformer,
-				pinnipedInformers.Config().V1alpha1().OIDCProviders(),
+				opInformer,
 				controllerlib.WithInformer,
 			),
 			singletonWorker,
@@ -135,7 +142,7 @@ func startControllers(
 			generator.NewSupervisorSecretsController(
 				supervisorDeployment,
 				kubeClient,
-				kubeInformers.Core().V1().Secrets(),
+				secretInformer,
 				func(secret []byte) {
 					plog.Debug("setting csrf cookie secret")
 					secretCache.SetCSRFCookieEncoderHashKey(secret)
@@ -143,6 +150,63 @@ func startControllers(
 			),
 			singletonWorker,
 		).
+		WithController(
+			generator.NewOIDCProviderSecretsController(
+				symmetricsecrethelper.New(
+					"pinniped-oidc-provider-hmac-key-",
+					cfg.Labels,
+					rand.Reader,
+					func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) {
+						plog.Debug("setting hmac secret", "issuer", parent.Spec.Issuer)
+						secretCache.GetOIDCProviderCacheFor(parent.Spec.Issuer).
+							SetTokenHMACKey(child.Data[symmetricsecrethelper.SecretDataKey])
+					},
+				),
+				kubeClient,
+				secretInformer,
+				opInformer,
+				controllerlib.WithInformer,
+			),
+			singletonWorker,
+		).
+		WithController(
+			generator.NewOIDCProviderSecretsController(
+				symmetricsecrethelper.New(
+					"pinniped-oidc-provider-upstream-state-signature-key-",
+					cfg.Labels,
+					rand.Reader,
+					func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) {
+						plog.Debug("setting state signature key", "issuer", parent.Spec.Issuer)
+						secretCache.GetOIDCProviderCacheFor(parent.Spec.Issuer).
+							SetStateEncoderHashKey(child.Data[symmetricsecrethelper.SecretDataKey])
+					},
+				),
+				kubeClient,
+				secretInformer,
+				opInformer,
+				controllerlib.WithInformer,
+			),
+			singletonWorker,
+		).
+		WithController(
+			generator.NewOIDCProviderSecretsController(
+				symmetricsecrethelper.New(
+					"pinniped-oidc-provider-upstream-state-encryption-key-",
+					cfg.Labels,
+					rand.Reader,
+					func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) {
+						plog.Debug("setting state encryption key", "issuer", parent.Spec.Issuer)
+						secretCache.GetOIDCProviderCacheFor(parent.Spec.Issuer).
+							SetStateEncoderHashKey(child.Data[symmetricsecrethelper.SecretDataKey])
+					},
+				),
+				kubeClient,
+				secretInformer,
+				opInformer,
+				controllerlib.WithInformer,
+			),
+			singletonWorker,
+		).
 		WithController(
 			upstreamwatcher.New(
 				dynamicUpstreamIDPProvider,

internal/controller/supervisorconfig/generator/oidc_provider_secrets.go

@@ -16,7 +16,6 @@ import (
 	"k8s.io/klog/v2"
 
 	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
-	pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned"
 	configinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions/config/v1alpha1"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
 	"go.pinniped.dev/internal/controllerlib"
@@ -28,43 +27,47 @@ const (
 	opcKind = "OIDCProvider" // TODO: deduplicate - internal/controller/supervisorconfig/jwks_writer.go
 )
 
-// jwkController holds the fields necessary for the JWKS controller to communicate with OPC's and
+// SecretHelper describes an object that can Generate() a Secret and determine whether a Secret
-// secrets, both via a cache and via the API.
+// IsValid(). It can also be Notify()'d about a Secret being persisted.
+//
+// A SecretHelper has a Name() that can be used to identify it from other SecretHelper instances.
+type SecretHelper interface {
+	Name() string
+	Generate(*configv1alpha1.OIDCProvider) (*corev1.Secret, error)
+	IsValid(*configv1alpha1.OIDCProvider, *corev1.Secret) bool
+	Notify(*configv1alpha1.OIDCProvider, *corev1.Secret)
+}
+
 type oidcProviderSecretsController struct {
-	secretNameFunc func(*configv1alpha1.OIDCProvider) string
+	secretHelper   SecretHelper
-	secretLabels   map[string]string
-	secretDataFunc func() (map[string][]byte, error)
-	pinnipedClient pinnipedclientset.Interface
 	kubeClient     kubernetes.Interface
 	opcInformer    configinformers.OIDCProviderInformer
 	secretInformer corev1informers.SecretInformer
 }
 
+// NewOIDCProviderSecretsController returns a controllerlib.Controller that ensures a child Secret
+// always exists for a parent OIDCProvider. It does this using the provided secretHelper, which
+// provides the parent/child mapping logic.
 func NewOIDCProviderSecretsController(
-	secretNameFunc func(*configv1alpha1.OIDCProvider) string,
+	secretHelper SecretHelper,
-	secretLabels map[string]string,
-	secretDataFunc func() (map[string][]byte, error),
 	kubeClient kubernetes.Interface,
-	pinnipedClient pinnipedclientset.Interface,
 	secretInformer corev1informers.SecretInformer,
 	opcInformer configinformers.OIDCProviderInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {
 	return controllerlib.New(
 		controllerlib.Config{
-			Name: "JWKSController",
+			Name: fmt.Sprintf("%s%s", secretHelper.Name(), "controller"),
 			Syncer: &oidcProviderSecretsController{
-				secretNameFunc: secretNameFunc,
+				secretHelper:   secretHelper,
-				secretLabels:   secretLabels,
-				secretDataFunc: secretDataFunc,
 				kubeClient:     kubeClient,
-				pinnipedClient: pinnipedClient,
 				secretInformer: secretInformer,
 				opcInformer:    opcInformer,
 			},
 		},
 		// We want to be notified when a OPC's secret gets updated or deleted. When this happens, we
 		// should get notified via the corresponding OPC key.
+		// TODO: de-dup me (jwks_writer.go).
 		withInformer(
 			secretInformer,
 			controllerlib.FilterFuncs{
@@ -96,7 +99,7 @@ func NewOIDCProviderSecretsController(
 }
 
 func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error {
-	opc, err := c.opcInformer.Lister().OIDCProviders(ctx.Key.Namespace).Get(ctx.Key.Name)
+	op, err := c.opcInformer.Lister().OIDCProviders(ctx.Key.Namespace).Get(ctx.Key.Name)
 	notFound := k8serrors.IsNotFound(err)
 	if err != nil && !notFound {
 		return fmt.Errorf(
@@ -108,8 +111,8 @@ func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error {
 	}
 
 	if notFound {
-		// The corresponding secret to this OPC should have been garbage collected since it should have
+		// The corresponding secret to this OP should have been garbage collected since it should have
-		// had this OPC as its owner.
+		// had this OP as its owner.
 		plog.Debug(
 			"oidcprovider deleted",
 			"oidcprovider",
@@ -118,83 +121,99 @@ func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error {
 		return nil
 	}
 
-	secretNeedsUpdate, err := c.secretNeedsUpdate(opc)
+	newSecret, err := c.secretHelper.Generate(op)
 	if err != nil {
-		return fmt.Errorf("cannot determine secret status: %w", err)
+		return fmt.Errorf("failed to generate secret: %w", err)
+	}
+
+	secretNeedsUpdate, existingSecret, err := c.secretNeedsUpdate(op, newSecret.Name)
+	if err != nil {
+		return fmt.Errorf("failed to determine secret status: %w", err)
 	}
 	if !secretNeedsUpdate {
 		// Secret is up to date - we are good to go.
 		plog.Debug(
 			"secret is up to date",
 			"oidcprovider",
-			klog.KRef(ctx.Key.Namespace, ctx.Key.Name),
+			klog.KObj(op),
+			"secret",
+			klog.KObj(existingSecret),
 		)
+		c.secretHelper.Notify(op, existingSecret)
 		return nil
 	}
 
-	// If the OPC does not have a secret associated with it, that secret does not exist, or the secret
+	// If the OP does not have a secret associated with it, that secret does not exist, or the secret
-	// is invalid, we will generate a new secret (i.e., a JWKS).
+	// is invalid, we will create a new secret.
-	secret, err := generateSecret(opc.Namespace, c.secretNameFunc(opc), c.secretDataFunc, opc)
+	if err := c.createOrUpdateSecret(ctx.Context, op, &newSecret); err != nil {
-	if err != nil {
+		return fmt.Errorf("failed to create or update secret: %w", err)
-		return fmt.Errorf("cannot generate secret: %w", err)
 	}
+	plog.Debug("created/updated secret", "oidcprovider", klog.KObj(op), "secret", klog.KObj(newSecret))
 
-	if err := c.createOrUpdateSecret(ctx.Context, secret); err != nil {
+	c.secretHelper.Notify(op, newSecret)
-		return fmt.Errorf("cannot create or update secret: %w", err)
-	}
-	plog.Debug("created/updated secret", "secret", klog.KObj(secret))
 
 	return nil
 }
 
-func (c *oidcProviderSecretsController) secretNeedsUpdate(opc *configv1alpha1.OIDCProvider) (bool, error) {
+// secretNeedsUpdate returns whether or not the Secret, with name secretName, for OIDCProvider op
+// needs to be updated. It returns the existing secret as its second argument.
+func (c *oidcProviderSecretsController) secretNeedsUpdate(
+	op *configv1alpha1.OIDCProvider,
+	secretName string,
+) (bool, *corev1.Secret, error) {
 	// This OPC says it has a secret associated with it. Let's try to get it from the cache.
-	secret, err := c.secretInformer.Lister().Secrets(opc.Namespace).Get(c.secretNameFunc(opc))
+	secret, err := c.secretInformer.Lister().Secrets(op.Namespace).Get(secretName)
 	notFound := k8serrors.IsNotFound(err)
 	if err != nil && !notFound {
-		return false, fmt.Errorf("cannot get secret: %w", err)
+		return false, nil, fmt.Errorf("cannot get secret: %w", err)
 	}
 	if notFound {
 		// If we can't find the secret, let's assume we need to create it.
-		return true, nil
+		return true, nil, nil
 	}
 
-	if !isValid(secret) {
+	if !c.secretHelper.IsValid(op, secret) {
 		// If this secret is invalid, we need to generate a new one.
-		return true, nil
+		return true, secret, nil
 	}
 
-	return false, nil
+	return false, secret, nil
 }
 
 func (c *oidcProviderSecretsController) createOrUpdateSecret(
 	ctx context.Context,
-	newSecret *corev1.Secret,
+	op *configv1alpha1.OIDCProvider,
+	newSecret **corev1.Secret,
 ) error {
-	secretClient := c.kubeClient.CoreV1().Secrets(newSecret.Namespace)
+	secretClient := c.kubeClient.CoreV1().Secrets((*newSecret).Namespace)
 	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		oldSecret, err := secretClient.Get(ctx, newSecret.Name, metav1.GetOptions{})
+		oldSecret, err := secretClient.Get(ctx, (*newSecret).Name, metav1.GetOptions{})
 		notFound := k8serrors.IsNotFound(err)
 		if err != nil && !notFound {
-			return fmt.Errorf("cannot get secret: %w", err)
+			return fmt.Errorf("failed to get secret %s/%s: %w", (*newSecret).Namespace, (*newSecret).Name, err)
 		}
 
 		if notFound {
 			// New secret doesn't exist, so create it.
-			_, err := secretClient.Create(ctx, newSecret, metav1.CreateOptions{})
+			_, err := secretClient.Create(ctx, *newSecret, metav1.CreateOptions{})
 			if err != nil {
-				return fmt.Errorf("cannot create secret: %w", err)
+				return fmt.Errorf("failed to create secret %s/%s: %w", (*newSecret).Namespace, (*newSecret).Name, err)
 			}
 			return nil
 		}
 
 		// New secret already exists, so ensure it is up to date.
-		if isValid(oldSecret) {
+		if c.secretHelper.IsValid(op, oldSecret) {
-			// If the secret already has valid JWK's, then we are good to go and we don't need an update.
+			// If the secret already has valid a valid Secret, then we are good to go and we don't need an
+			// update.
+			*newSecret = oldSecret
 			return nil
 		}
 
-		oldSecret.Data = newSecret.Data
+		oldSecret.Labels = (*newSecret).Labels
+		oldSecret.Type = (*newSecret).Type
+		oldSecret.Data = (*newSecret).Data
+		*newSecret = oldSecret
 		_, err = secretClient.Update(ctx, oldSecret, metav1.UpdateOptions{})
 		return err
 	})

internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go

@@ -7,11 +7,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"sync"
 	"testing"
 	"time"
 
+	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -23,6 +26,7 @@ import (
 	pinnipedfake "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned/fake"
 	pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions"
 	"go.pinniped.dev/internal/controllerlib"
+	"go.pinniped.dev/internal/mocks/mocksecrethelper"
 	"go.pinniped.dev/internal/testutil"
 )
 
@@ -137,6 +141,11 @@ func TestOIDCProviderControllerFilterSecret(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
+			ctrl := gomock.NewController(t)
+			t.Cleanup(ctrl.Finish)
+			secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl)
+			secretHelper.EXPECT().Name().Times(1).Return("some-name")
+
 			secretInformer := kubeinformers.NewSharedInformerFactory(
 				kubernetesfake.NewSimpleClientset(),
 				0,
@@ -147,11 +156,8 @@ func TestOIDCProviderControllerFilterSecret(t *testing.T) {
 			).Config().V1alpha1().OIDCProviders()
 			withInformer := testutil.NewObservableWithInformerOption()
 			_ = NewOIDCProviderSecretsController(
-				secretNameFunc,
+				secretHelper,
-				nil, // labels, not needed
-				fakeSecretDataFunc,
 				nil, // kubeClient, not needed
-				nil, // pinnipedClient, not needed
 				secretInformer,
 				opcInformer,
 				withInformer.WithInformer,
@@ -193,6 +199,11 @@ func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
+			ctrl := gomock.NewController(t)
+			t.Cleanup(ctrl.Finish)
+			secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl)
+			secretHelper.EXPECT().Name().Times(1).Return("some-name")
+
 			secretInformer := kubeinformers.NewSharedInformerFactory(
 				kubernetesfake.NewSimpleClientset(),
 				0,
@@ -203,11 +214,8 @@ func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) {
 			).Config().V1alpha1().OIDCProviders()
 			withInformer := testutil.NewObservableWithInformerOption()
 			_ = NewOIDCProviderSecretsController(
-				secretNameFunc,
+				secretHelper,
-				nil, // labels, not needed
-				fakeSecretDataFunc,
 				nil, // kubeClient, not needed
-				nil, // pinnipedClient, not needed
 				secretInformer,
 				opcInformer,
 				withInformer.WithInformer,
@@ -224,307 +232,271 @@ func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) {
 	}
 }
 
-func TestNewOIDCProviderSecretsControllerSync(t *testing.T) {
+func TestOIDCProviderSecretsControllerSync(t *testing.T) {
-	// We shouldn't run this test in parallel since it messes with a global function (generateKey).
+	t.Parallel()
 
-	const namespace = "tuna-namespace"
+	const (
+		namespace = "some-namespace"
 
-	opcGVR := schema.GroupVersionResource{
+		opName = "op-name"
+		opUID  = "op-uid"
+
+		secretName = "secret-name"
+		secretUID  = "secret-uid"
+	)
+
+	opGVR := schema.GroupVersionResource{
 		Group:    configv1alpha1.SchemeGroupVersion.Group,
 		Version:  configv1alpha1.SchemeGroupVersion.Version,
 		Resource: "oidcproviders",
 	}
 
-	goodOPC := &configv1alpha1.OIDCProvider{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "good-opc",
-			Namespace: namespace,
-			UID:       "good-opc-uid",
-		},
-		Spec: configv1alpha1.OIDCProviderSpec{
-			Issuer: "https://some-issuer.com",
-		},
-	}
-
-	expectedSecretName := secretNameFunc(goodOPC)
-
 	secretGVR := schema.GroupVersionResource{
 		Group:    corev1.SchemeGroupVersion.Group,
 		Version:  corev1.SchemeGroupVersion.Version,
 		Resource: "secrets",
 	}
 
-	newSecret := func(secretData map[string][]byte) *corev1.Secret {
+	goodOP := &configv1alpha1.OIDCProvider{
-		s := corev1.Secret{
-			Type: symmetricKeySecretType,
 		ObjectMeta: metav1.ObjectMeta{
-				Name:      expectedSecretName,
+			Name:      opName,
 			Namespace: namespace,
-				Labels: map[string]string{
+			UID:       opUID,
-					"myLabelKey1": "myLabelValue1",
-					"myLabelKey2": "myLabelValue2",
 		},
+	}
+
+	goodSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretName,
+			Namespace: namespace,
+			UID:       secretUID,
 			OwnerReferences: []metav1.OwnerReference{
 				{
-						APIVersion:         opcGVR.GroupVersion().String(),
+					APIVersion:         opGVR.GroupVersion().String(),
 					Kind:               "OIDCProvider",
-						Name:               goodOPC.Name,
+					Name:               opName,
-						UID:                goodOPC.UID,
+					UID:                opUID,
+					BlockOwnerDeletion: boolPtr(true),
+					Controller:         boolPtr(true),
+				},
+			},
+			Labels: map[string]string{
+				"some-key-0": "some-value-0",
+				"some-key-1": "some-value-1",
+			},
+		},
+		Type: "some-secret-type",
+		Data: map[string][]byte{
+			"some-key": []byte("some-value"),
+		},
+	}
+
+	invalidSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretName,
+			Namespace: namespace,
+			UID:       secretUID,
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion:         opGVR.GroupVersion().String(),
+					Kind:               "OIDCProvider",
+					Name:               opName,
+					UID:                opUID,
 					BlockOwnerDeletion: boolPtr(true),
 					Controller:         boolPtr(true),
 				},
 			},
 		},
-			Data: secretData,
 	}
 
-		return &s
+	once := sync.Once{}
-	}
-
-	secretData, err := fakeSecretDataFunc()
-	require.NoError(t, err)
-
-	goodSecret := newSecret(secretData)
 
 	tests := []struct {
 		name              string
-		key                  controllerlib.Key
+		storage           func(**configv1alpha1.OIDCProvider, **corev1.Secret)
-		secrets              []*corev1.Secret
+		client            func(*kubernetesfake.Clientset)
-		configKubeClient     func(*kubernetesfake.Clientset)
+		secretHelper      func(*mocksecrethelper.MockSecretHelper)
-		configPinnipedClient func(*pinnipedfake.Clientset)
-		opcs                 []*configv1alpha1.OIDCProvider
-		generateKeyErr       error
-		wantGenerateKeyCount int
 		wantSecretActions []kubetesting.Action
-		wantOPCActions       []kubetesting.Action
+		wantOPActions     []kubetesting.Action
 		wantError         string
 	}{
 		{
-			name: "new opc with no secret",
+			name: "OIDCProvider exists and secret does not exist",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				*op = nil
-				goodOPC,
+				*s = nil
+			},
+		},
+		{
+			name: "OIDCProvider exists and secret exists",
+			storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) {
+				*op = nil
+			},
+		},
+		{
+			name: "OIDCProvider exists and secret does not exist",
+			storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) {
+				*s = nil
+			},
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
+				secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1)
 			},
-			wantGenerateKeyCount: 1,
 			wantSecretActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
 				kubetesting.NewCreateAction(secretGVR, namespace, goodSecret),
 			},
-			wantOPCActions: []kubetesting.Action{
+		},
-				kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name),
+		{
+			name: "OIDCProvider exists and valid secret exists",
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
+				secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(true)
+				secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1)
 			},
 		},
 		{
-			name: "opc without status with existing secret",
+			name: "OIDCProvider exists and invalid secret exists",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				*s = invalidSecret.DeepCopy()
-				goodOPC,
 			},
-			secrets: []*corev1.Secret{
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
-				goodSecret,
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
+				secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(2).Return(false)
+				secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1)
 			},
-			wantGenerateKeyCount: 1,
-			wantSecretActions: []kubetesting.Action{
-				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
-			},
-			wantOPCActions: []kubetesting.Action{
-				kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name),
-			},
-		},
-		{
-			name: "existing opc with no secret",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			opcs: []*configv1alpha1.OIDCProvider{
-				goodOPC,
-			},
-			wantGenerateKeyCount: 1,
-			wantSecretActions: []kubetesting.Action{
-				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
-				kubetesting.NewCreateAction(secretGVR, namespace, goodSecret),
-			},
-			wantOPCActions: []kubetesting.Action{
-				kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name),
-			},
-		},
-		{
-			name: "existing opc with existing secret",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			opcs: []*configv1alpha1.OIDCProvider{
-				goodOPC,
-			},
-			secrets: []*corev1.Secret{
-				goodSecret,
-			},
-		},
-		{
-			name: "deleted opc",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			// Nothing to do here since Kube will garbage collect our child secret via its OwnerReference.
-		},
-		{
-			name: "secret data is empty",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			opcs: []*configv1alpha1.OIDCProvider{
-				goodOPC,
-			},
-			secrets: []*corev1.Secret{
-				newSecret(map[string][]byte{}),
-			},
-			wantGenerateKeyCount: 1,
 			wantSecretActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
 				kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret),
 			},
-			wantOPCActions: []kubetesting.Action{
-				kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name),
-			},
 		},
 		{
-			name: fmt.Sprintf("secret missing key %s", symmetricKeySecretDataKey),
+			name: "OIDCProvider exists and generating a secret fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(nil, errors.New("some generate error"))
-				goodOPC,
 			},
-			secrets: []*corev1.Secret{
+			wantError: "failed to generate secret: some generate error",
-				newSecret(map[string][]byte{"badKey": []byte("some secret - must have at least 32 bytes")}),
+		},
+		{
+			name: "OIDCProvider exists and invalid secret exists and upon update we learn of a valid secret",
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
+				otherSecret := goodSecret.DeepCopy()
+				otherSecret.UID = "other-secret-uid"
+
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(otherSecret, nil)
+				secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(false)
+				secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(true)
+				secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1)
 			},
-			wantGenerateKeyCount: 1,
 			wantSecretActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
-				kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret),
-			},
-			wantOPCActions: []kubetesting.Action{
-				kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name),
 			},
 		},
 		{
-			name: fmt.Sprintf("secret data value for key %s", symmetricKeySecretDataKey),
+			name: "OIDCProvider exists and invalid secret exists and get fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
-				goodOPC,
+				secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(false)
 			},
-			secrets: []*corev1.Secret{
+			client: func(c *kubernetesfake.Clientset) {
-				newSecret(map[string][]byte{symmetricKeySecretDataKey: {}}),
+				c.PrependReactor("get", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
-			},
-			wantGenerateKeyCount: 1,
-			wantSecretActions: []kubetesting.Action{
-				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
-				kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret),
-			},
-			wantOPCActions: []kubetesting.Action{
-				kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name),
-			},
-		},
-		{
-			name: "generate key fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			opcs: []*configv1alpha1.OIDCProvider{
-				goodOPC,
-			},
-			generateKeyErr: errors.New("some generate error"),
-			wantError:      "cannot generate secret: cannot generate key: some generate error",
-		},
-		{
-			name: "get secret fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			opcs: []*configv1alpha1.OIDCProvider{
-				goodOPC,
-			},
-			configKubeClient: func(client *kubernetesfake.Clientset) {
-				client.PrependReactor("get", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
 					return true, nil, errors.New("some get error")
 				})
 			},
-			wantError: "cannot create or update secret: cannot get secret: some get error",
+			wantSecretActions: []kubetesting.Action{
+				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
+			},
+			wantError: fmt.Sprintf("failed to create or update secret: failed to get secret %s/%s: some get error", namespace, goodSecret.Name),
 		},
 		{
-			name: "create secret fails",
+			name: "OIDCProvider exists and secret does not exist and create fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				*s = nil
-				goodOPC,
 			},
-			configKubeClient: func(client *kubernetesfake.Clientset) {
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
-				client.PrependReactor("create", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
+			},
+			client: func(c *kubernetesfake.Clientset) {
+				c.PrependReactor("create", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
 					return true, nil, errors.New("some create error")
 				})
 			},
-			wantError: "cannot create or update secret: cannot create secret: some create error",
+			wantSecretActions: []kubetesting.Action{
+				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
+				kubetesting.NewCreateAction(secretGVR, namespace, goodSecret),
+			},
+			wantError: fmt.Sprintf("failed to create or update secret: failed to create secret %s/%s: some create error", namespace, goodSecret.Name),
 		},
 		{
-			name: "update secret fails",
+			name: "OIDCProvider exists and invalid secret exists and update fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
-				goodOPC,
+				secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(2).Return(false)
 			},
-			secrets: []*corev1.Secret{
+			client: func(c *kubernetesfake.Clientset) {
-				newSecret(map[string][]byte{}),
+				c.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
-			},
-			configKubeClient: func(client *kubernetesfake.Clientset) {
-				client.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
 					return true, nil, errors.New("some update error")
 				})
 			},
-			wantError: "cannot create or update secret: some update error",
+			wantSecretActions: []kubetesting.Action{
+				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
+				kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret),
+			},
+			wantError: "failed to create or update secret: some update error",
 		},
 		{
-			name: "get opc fails",
+			name: "OIDCProvider exists and invalid secret exists and update fails due to conflict",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
+			storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) {
-			opcs: []*configv1alpha1.OIDCProvider{
+				*s = invalidSecret.DeepCopy()
-				goodOPC,
 			},
-			configPinnipedClient: func(client *pinnipedfake.Clientset) {
+			secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) {
-				client.PrependReactor("get", "oidcproviders", func(_ kubetesting.Action) (bool, runtime.Object, error) {
+				secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil)
-					return true, nil, errors.New("some get error")
+				secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(3).Return(false)
+				secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1)
+			},
+			client: func(c *kubernetesfake.Clientset) {
+				c.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
+					var err error
+					once.Do(func() { err = k8serrors.NewConflict(secretGVR.GroupResource(), namespace, errors.New("some error")) })
+					return true, nil, err
 				})
 			},
-			wantError: "cannot update opc: cannot get opc: some get error",
+			wantSecretActions: []kubetesting.Action{
+				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
+				kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret),
+				kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name),
+				kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret),
 			},
-		{
-			name: "update opc fails",
-			key:  controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name},
-			opcs: []*configv1alpha1.OIDCProvider{
-				goodOPC,
-			},
-			configPinnipedClient: func(client *pinnipedfake.Clientset) {
-				client.PrependReactor("update", "oidcproviders", func(_ kubetesting.Action) (bool, runtime.Object, error) {
-					return true, nil, errors.New("some update error")
-				})
-			},
-			wantError: "cannot update opc: some update error",
 		},
 	}
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
-			// We shouldn't run this test in parallel since it messes with a global function (generateKey).
+			t.Parallel()
-			generateKeyCount := 0
+
-			generateKey := func() (map[string][]byte, error) {
-				generateKeyCount++
-				return map[string][]byte{
-					symmetricKeySecretDataKey: []byte("some secret - must have at least 32 bytes"),
-				}, nil
-			}
 			ctx, cancel := context.WithTimeout(context.Background(), time.Second*3)
 			defer cancel()
 
+			pinnipedInformerClient := pinnipedfake.NewSimpleClientset()
+
 			kubeAPIClient := kubernetesfake.NewSimpleClientset()
 			kubeInformerClient := kubernetesfake.NewSimpleClientset()
-			for _, secret := range test.secrets {
+
+			op := goodOP.DeepCopy()
+			secret := goodSecret.DeepCopy()
+			if test.storage != nil {
+				test.storage(&op, &secret)
+			}
+			if op != nil {
+				require.NoError(t, pinnipedInformerClient.Tracker().Add(op))
+			}
+			if secret != nil {
 				require.NoError(t, kubeAPIClient.Tracker().Add(secret))
 				require.NoError(t, kubeInformerClient.Tracker().Add(secret))
 			}
-			if test.configKubeClient != nil {
-				test.configKubeClient(kubeAPIClient)
-			}
 
-			pinnipedAPIClient := pinnipedfake.NewSimpleClientset()
+			if test.client != nil {
-			pinnipedInformerClient := pinnipedfake.NewSimpleClientset()
+				test.client(kubeAPIClient)
-			for _, opc := range test.opcs {
-				require.NoError(t, pinnipedAPIClient.Tracker().Add(opc))
-				require.NoError(t, pinnipedInformerClient.Tracker().Add(opc))
-			}
-			if test.configPinnipedClient != nil {
-				test.configPinnipedClient(pinnipedAPIClient)
 			}
 
 			kubeInformers := kubeinformers.NewSharedInformerFactory(
@@ -536,15 +508,17 @@ func TestNewOIDCProviderSecretsControllerSync(t *testing.T) {
 				0,
 			)
 
+			ctrl := gomock.NewController(t)
+			t.Cleanup(ctrl.Finish)
+			secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl)
+			secretHelper.EXPECT().Name().Times(1).Return("some-name")
+			if test.secretHelper != nil {
+				test.secretHelper(secretHelper)
+			}
+
 			c := NewOIDCProviderSecretsController(
-				secretNameFunc,
+				secretHelper,
-				map[string]string{
-					"myLabelKey1": "myLabelValue1",
-					"myLabelKey2": "myLabelValue2",
-				},
-				generateKey,
 				kubeAPIClient,
-				pinnipedAPIClient,
 				kubeInformers.Core().V1().Secrets(),
 				pinnipedInformers.Config().V1alpha1().OIDCProviders(),
 				controllerlib.WithInformer,
@@ -557,7 +531,10 @@ func TestNewOIDCProviderSecretsControllerSync(t *testing.T) {
 
 			err := controllerlib.TestSync(t, c, controllerlib.Context{
 				Context: ctx,
-				Key:     test.key,
+				Key: controllerlib.Key{
+					Namespace: namespace,
+					Name:      opName,
+				},
 			})
 			if test.wantError != "" {
 				require.EqualError(t, err, test.wantError)
@@ -565,26 +542,12 @@ func TestNewOIDCProviderSecretsControllerSync(t *testing.T) {
 			}
 			require.NoError(t, err)
 
-			require.Equal(t, test.wantGenerateKeyCount, generateKeyCount)
+			if test.wantSecretActions == nil {
-
+				test.wantSecretActions = []kubetesting.Action{}
-			if test.wantSecretActions != nil {
+			}
 			require.Equal(t, test.wantSecretActions, kubeAPIClient.Actions())
-			}
-			if test.wantOPCActions != nil {
-				require.Equal(t, test.wantOPCActions, pinnipedAPIClient.Actions())
-			}
 		})
 	}
 }
 
-func secretNameFunc(opc *configv1alpha1.OIDCProvider) string {
-	return fmt.Sprintf("pinniped-%s-%s-test_secret", opc.Kind, opc.UID)
-}
-
-func fakeSecretDataFunc() (map[string][]byte, error) {
-	return map[string][]byte{
-		symmetricKeySecretDataKey: []byte("some secret - must have at least 32 bytes"),
-	}, nil
-}
-
 func boolPtr(b bool) *bool { return &b }

internal/controller/supervisorconfig/generator/supervisor_secrets.go

@@ -24,6 +24,7 @@ import (
 	"go.pinniped.dev/internal/plog"
 )
 
+// TODO: de-dup me when we abstract these controllers.
 const (
 	symmetricKeySecretType    = "secrets.pinniped.dev/symmetric"
 	symmetricKeySecretDataKey = "key"

internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper.go

@@ -0,0 +1,110 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+// Package symmetricsecrethelper provides a type that can generate and validate symmetric keys as
+// Secret's.
+package symmetricsecrethelper
+
+import (
+	"fmt"
+	"io"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
+	"go.pinniped.dev/internal/controller/supervisorconfig/generator"
+)
+
+const (
+	// SecretType is corev1.Secret.Type of all corev1.Secret's generated by this helper.
+	SecretType = "secrets.pinniped.dev/symmetric"
+	// SecretDataKey is the corev1.Secret.Data key for the symmetric key value generated by this helper.
+	SecretDataKey = "key"
+
+	// keySize is the default length, in bytes, of generated keys. It is set to 32 since this
+	// seems like reasonable entropy for our keys, and a 32-byte key will allow for AES-256
+	// to be used in our codecs (see dynamiccodec.Codec).
+	keySize = 32
+)
+
+type helper struct {
+	namePrefix string
+	labels     map[string]string
+	rand       io.Reader
+	notifyFunc func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret)
+}
+
+var _ generator.SecretHelper = &helper{}
+
+// New returns a SecretHelper that has been parameterized with common symmetric secret generation
+// knobs.
+func New(
+	namePrefix string,
+	labels map[string]string,
+	rand io.Reader,
+	notifyFunc func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret),
+) generator.SecretHelper {
+	return &helper{
+		namePrefix: namePrefix,
+		labels:     labels,
+		rand:       rand,
+		notifyFunc: notifyFunc,
+	}
+}
+
+func (s *helper) Name() string { return s.namePrefix }
+
+// Generate implements SecretHelper.Generate().
+func (s *helper) Generate(parent *configv1alpha1.OIDCProvider) (*corev1.Secret, error) {
+	key := make([]byte, keySize)
+	if _, err := s.rand.Read(key); err != nil {
+		return nil, err
+	}
+
+	return &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("%s%s", s.namePrefix, parent.UID),
+			Namespace: parent.Namespace,
+			Labels:    s.labels,
+			OwnerReferences: []metav1.OwnerReference{
+				*metav1.NewControllerRef(parent, schema.GroupVersionKind{
+					Group:   configv1alpha1.SchemeGroupVersion.Group,
+					Version: configv1alpha1.SchemeGroupVersion.Version,
+					Kind:    "OIDCProvider",
+				}),
+			},
+		},
+		Type: SecretType,
+		Data: map[string][]byte{
+			SecretDataKey: key,
+		},
+	}, nil
+}
+
+// IsValid implements SecretHelper.IsValid().
+func (s *helper) IsValid(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) bool {
+	if !metav1.IsControlledBy(child, parent) {
+		return false
+	}
+
+	if child.Type != SecretType {
+		return false
+	}
+
+	key, ok := child.Data[SecretDataKey]
+	if !ok {
+		return false
+	}
+	if len(key) != keySize {
+		return false
+	}
+
+	return true
+}
+
+// Notify implements SecretHelper.Notify().
+func (s *helper) Notify(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) {
+	s.notifyFunc(parent, child)
+}

internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper_test.go

@@ -0,0 +1,154 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package symmetricsecrethelper
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
+)
+
+const keyWith32Bytes = "0123456789abcdef0123456789abcdef"
+
+func TestHelper(t *testing.T) {
+	labels := map[string]string{
+		"some-label-key-1": "some-label-value-1",
+		"some-label-key-2": "some-label-value-2",
+	}
+	randSource := strings.NewReader(keyWith32Bytes)
+	var notifyParent *configv1alpha1.OIDCProvider
+	var notifyChild *corev1.Secret
+	h := New("some-name-prefix-", labels, randSource, func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) {
+		require.True(t, notifyParent == nil && notifyChild == nil, "expected notify func not to have been called yet")
+		notifyParent = parent
+		notifyChild = child
+	})
+
+	parent := &configv1alpha1.OIDCProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			UID:       "some-uid",
+			Namespace: "some-namespace",
+		},
+	}
+	child, err := h.Generate(parent)
+	require.NoError(t, err)
+	require.Equal(t, child, &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "some-name-prefix-some-uid",
+			Namespace: "some-namespace",
+			Labels:    labels,
+			OwnerReferences: []metav1.OwnerReference{
+				*metav1.NewControllerRef(parent, schema.GroupVersionKind{
+					Group:   configv1alpha1.SchemeGroupVersion.Group,
+					Version: configv1alpha1.SchemeGroupVersion.Version,
+					Kind:    "OIDCProvider",
+				}),
+			},
+		},
+		Type: "secrets.pinniped.dev/symmetric",
+		Data: map[string][]byte{
+			"key": []byte(keyWith32Bytes),
+		},
+	})
+
+	require.True(t, h.IsValid(parent, child))
+
+	h.Notify(parent, child)
+	require.Equal(t, parent, notifyParent)
+	require.Equal(t, child, notifyChild)
+}
+
+func TestHelperIsValid(t *testing.T) {
+	tests := []struct {
+		name   string
+		child  func(*corev1.Secret)
+		parent func(*configv1alpha1.OIDCProvider)
+		want   bool
+	}{
+		{
+			name: "wrong type",
+			child: func(s *corev1.Secret) {
+				s.Type = "wrong"
+			},
+			want: false,
+		},
+		{
+			name: "empty type",
+			child: func(s *corev1.Secret) {
+				s.Type = ""
+			},
+			want: false,
+		},
+		{
+			name: "data key is too short",
+			child: func(s *corev1.Secret) {
+				s.Data["key"] = []byte("short")
+			},
+			want: false,
+		},
+		{
+			name: "data key does not exist",
+			child: func(s *corev1.Secret) {
+				delete(s.Data, "key")
+			},
+			want: false,
+		},
+		{
+			name: "child not owned by parent",
+			parent: func(op *configv1alpha1.OIDCProvider) {
+				op.UID = "wrong"
+			},
+			want: false,
+		},
+		{
+			name: "happy path",
+			want: true,
+		},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			h := New("none of these args matter", nil, nil, nil)
+
+			parent := &configv1alpha1.OIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "some-parent-name",
+					Namespace: "some-namespace",
+					UID:       "some-parent-uid",
+				},
+			}
+			child := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "some-name-prefix-some-uid",
+					Namespace: "some-namespace",
+					OwnerReferences: []metav1.OwnerReference{
+						*metav1.NewControllerRef(parent, schema.GroupVersionKind{
+							Group:   configv1alpha1.SchemeGroupVersion.Group,
+							Version: configv1alpha1.SchemeGroupVersion.Version,
+							Kind:    "OIDCProvider",
+						}),
+					},
+				},
+				Type: "secrets.pinniped.dev/symmetric",
+				Data: map[string][]byte{
+					"key": []byte(keyWith32Bytes),
+				},
+			}
+			if test.child != nil {
+				test.child(child)
+			}
+			if test.parent != nil {
+				test.parent(parent)
+			}
+
+			require.Equalf(t, test.want, h.IsValid(parent, child), "child: %#v", child)
+		})
+	}
+}

internal/mocks/mocksecrethelper/generate.go

@@ -0,0 +1,6 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package mocksecrethelper
+
+//go:generate go run -v github.com/golang/mock/mockgen  -destination=mocksecrethelper.go -package=mocksecrethelper -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/controller/supervisorconfig/generator SecretHelper

internal/mocks/mocksecrethelper/mocksecrethelper.go

@@ -0,0 +1,94 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+//
+
+// Code generated by MockGen. DO NOT EDIT.
+// Source: go.pinniped.dev/internal/controller/supervisorconfig/generator (interfaces: SecretHelper)
+
+// Package mocksecrethelper is a generated GoMock package.
+package mocksecrethelper
+
+import (
+	gomock "github.com/golang/mock/gomock"
+	v1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
+	v1 "k8s.io/api/core/v1"
+	reflect "reflect"
+)
+
+// MockSecretHelper is a mock of SecretHelper interface
+type MockSecretHelper struct {
+	ctrl     *gomock.Controller
+	recorder *MockSecretHelperMockRecorder
+}
+
+// MockSecretHelperMockRecorder is the mock recorder for MockSecretHelper
+type MockSecretHelperMockRecorder struct {
+	mock *MockSecretHelper
+}
+
+// NewMockSecretHelper creates a new mock instance
+func NewMockSecretHelper(ctrl *gomock.Controller) *MockSecretHelper {
+	mock := &MockSecretHelper{ctrl: ctrl}
+	mock.recorder = &MockSecretHelperMockRecorder{mock}
+	return mock
+}
+
+// EXPECT returns an object that allows the caller to indicate expected use
+func (m *MockSecretHelper) EXPECT() *MockSecretHelperMockRecorder {
+	return m.recorder
+}
+
+// Generate mocks base method
+func (m *MockSecretHelper) Generate(arg0 *v1alpha1.OIDCProvider) (*v1.Secret, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "Generate", arg0)
+	ret0, _ := ret[0].(*v1.Secret)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// Generate indicates an expected call of Generate
+func (mr *MockSecretHelperMockRecorder) Generate(arg0 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Generate", reflect.TypeOf((*MockSecretHelper)(nil).Generate), arg0)
+}
+
+// IsValid mocks base method
+func (m *MockSecretHelper) IsValid(arg0 *v1alpha1.OIDCProvider, arg1 *v1.Secret) bool {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "IsValid", arg0, arg1)
+	ret0, _ := ret[0].(bool)
+	return ret0
+}
+
+// IsValid indicates an expected call of IsValid
+func (mr *MockSecretHelperMockRecorder) IsValid(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsValid", reflect.TypeOf((*MockSecretHelper)(nil).IsValid), arg0, arg1)
+}
+
+// Name mocks base method
+func (m *MockSecretHelper) Name() string {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "Name")
+	ret0, _ := ret[0].(string)
+	return ret0
+}
+
+// Name indicates an expected call of Name
+func (mr *MockSecretHelperMockRecorder) Name() *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockSecretHelper)(nil).Name))
+}
+
+// Notify mocks base method
+func (m *MockSecretHelper) Notify(arg0 *v1alpha1.OIDCProvider, arg1 *v1.Secret) {
+	m.ctrl.T.Helper()
+	m.ctrl.Call(m, "Notify", arg0, arg1)
+}
+
+// Notify indicates an expected call of Notify
+func (mr *MockSecretHelperMockRecorder) Notify(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Notify", reflect.TypeOf((*MockSecretHelper)(nil).Notify), arg0, arg1)
+}

internal/oidc/provider/manager/manager.go

@@ -86,14 +86,6 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) {
 	for _, incomingProvider := range oidcProviders {
 		providerCache := m.cache.GetOIDCProviderCacheFor(incomingProvider.Issuer())
 
-		if providerCache == nil { // TODO remove when populated from `Secret` values
-			providerCache = &secret.OIDCProviderCache{}
-			providerCache.SetTokenHMACKey([]byte("some secret - must have at least 32 bytes")) // TODO fetch from `Secret`
-			providerCache.SetStateEncoderHashKey([]byte("fake-state-hash-secret"))             // TODO fetch from `Secret`
-			providerCache.SetStateEncoderBlockKey([]byte("16-bytes-STATE01"))                  // TODO fetch from `Secret`
-			m.cache.SetOIDCProviderCacheFor(incomingProvider.Issuer(), providerCache)
-		}
-
 		issuer := incomingProvider.Issuer()
 		issuerHostWithPath := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath()
 		oidcTimeouts := oidc.DefaultOIDCTimeoutsConfiguration()

internal/oidc/provider/manager/manager_test.go

@@ -246,6 +246,16 @@ func TestManager(t *testing.T) {
 			cache := secret.Cache{}
 			cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret"))
 
+			oidcProvider1Cache := cache.GetOIDCProviderCacheFor(issuer1)
+			oidcProvider1Cache.SetStateEncoderHashKey([]byte("some-state-encoder-hash-key-1"))
+			oidcProvider1Cache.SetStateEncoderBlockKey([]byte("16-bytes-STATE01"))
+			oidcProvider1Cache.SetTokenHMACKey([]byte("some secret 1 - must have at least 32 bytes"))
+
+			oidcProvider2Cache := cache.GetOIDCProviderCacheFor(issuer2)
+			oidcProvider2Cache.SetStateEncoderHashKey([]byte("some-state-encoder-hash-key-2"))
+			oidcProvider2Cache.SetStateEncoderBlockKey([]byte("16-bytes-STATE02"))
+			oidcProvider2Cache.SetTokenHMACKey([]byte("some secret 2 - must have at least 32 bytes"))
+
 			subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, &cache, secretsClient)
 		})
 
@@ -309,21 +319,26 @@ func TestManager(t *testing.T) {
 			requireAuthorizationRequestToBeHandled(issuer2, authRequestParams, upstreamIDPAuthorizationURL)
 
 			// Hostnames are case-insensitive, so test that we can handle that.
+			csrfCookieValue1, upstreamStateParam1 :=
 				requireAuthorizationRequestToBeHandled(issuer1DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL)
-			csrfCookieValue, upstreamStateParam :=
+			csrfCookieValue2, upstreamStateParam2 :=
 				requireAuthorizationRequestToBeHandled(issuer2DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL)
 
-			callbackRequestParams := "?" + url.Values{
+			callbackRequestParams1 := "?" + url.Values{
 				"code":  []string{"some-fake-code"},
-				"state": []string{upstreamStateParam},
+				"state": []string{upstreamStateParam1},
+			}.Encode()
+			callbackRequestParams2 := "?" + url.Values{
+				"code":  []string{"some-fake-code"},
+				"state": []string{upstreamStateParam2},
 			}.Encode()
 
-			downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue)
+			downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams1, csrfCookieValue1)
-			downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue)
+			downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams2, csrfCookieValue2)
 
 			// Hostnames are case-insensitive, so test that we can handle that.
-			downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue)
+			downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams1, csrfCookieValue1)
-			downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue)
+			downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams2, csrfCookieValue2)
 
 			requireTokenRequestToBeHandled(issuer1, downstreamAuthCode1, issuer1JWKS, issuer1)
 			requireTokenRequestToBeHandled(issuer2, downstreamAuthCode2, issuer2JWKS, issuer2)

internal/secret/cache.go

@@ -3,6 +3,9 @@
 
 package secret
 
+// TODO: synchronize me.
+// TODO: use SetIssuerXXX() functions instead of returning a struct so that we don't have to worry about reentrancy.
+
 type Cache struct {
 	csrfCookieEncoderHashKey  []byte
 	csrfCookieEncoderBlockKey []byte
@@ -26,7 +29,12 @@ func (c *Cache) SetCSRFCookieEncoderBlockKey(key []byte) {
 }
 
 func (c *Cache) GetOIDCProviderCacheFor(oidcIssuer string) *OIDCProviderCache {
-	return c.oidcProviderCaches()[oidcIssuer]
+	oidcProvider, ok := c.oidcProviderCaches()[oidcIssuer]
+	if !ok {
+		oidcProvider = &OIDCProviderCache{}
+		c.oidcProviderCaches()[oidcIssuer] = oidcProvider
+	}
+	return oidcProvider
 }
 
 func (c *Cache) SetOIDCProviderCacheFor(oidcIssuer string, oidcProviderCache *OIDCProviderCache) {