From 6e8d5640135fab6addbee86b7cd0aba2c5d5cd4f Mon Sep 17 00:00:00 2001 From: aram price Date: Mon, 14 Dec 2020 16:08:48 -0800 Subject: [PATCH] Test filters in SupervisorSecretsController --- cmd/pinniped-supervisor/main.go | 1 + .../generator/supervisor_secrets.go | 49 ++--- .../generator/supervisor_secrets_test.go | 187 +++++++++++++++--- 3 files changed, 192 insertions(+), 45 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 0f2a9665..6c87b43e 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -159,6 +159,7 @@ func startControllers( plog.Debug("setting csrf cookie secret") secretCache.SetCSRFCookieEncoderHashKey(secret) }, + controllerlib.WithInformer, ), singletonWorker, ). diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets.go b/internal/controller/supervisorconfig/generator/supervisor_secrets.go index 61fb0140..e27166ae 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets.go @@ -27,36 +27,39 @@ import ( var generateKey = generateSymmetricKey type supervisorSecretsController struct { - owner *appsv1.Deployment - labels map[string]string - client kubernetes.Interface - secrets corev1informers.SecretInformer - setCache func(secret []byte) + owner *appsv1.Deployment + labels map[string]string + kubeClient kubernetes.Interface + secretInformer corev1informers.SecretInformer + setCacheFunc func(secret []byte) } // NewSupervisorSecretsController instantiates a new controllerlib.Controller which will ensure existence of a generated secret. func NewSupervisorSecretsController( // TODO: generate the name for the secret and label the secret with the UID of the owner? So that we don't have naming conflicts if the user has already created a Secret with that name. - // TODO: add tests for the filter like we do in the JWKSWriterController? owner *appsv1.Deployment, labels map[string]string, - client kubernetes.Interface, - secrets corev1informers.SecretInformer, - setCache func(secret []byte), + kubeClient kubernetes.Interface, + secretInformer corev1informers.SecretInformer, + setCacheFunc func(secret []byte), + withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { c := supervisorSecretsController{ - owner: owner, - labels: labels, - client: client, - secrets: secrets, - setCache: setCache, + owner: owner, + labels: labels, + kubeClient: kubeClient, + secretInformer: secretInformer, + setCacheFunc: setCacheFunc, } - filter := pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool { - return metav1.IsControlledBy(obj, owner) - }, nil) return controllerlib.New( controllerlib.Config{Name: owner.Name + "-secret-generator", Syncer: &c}, - controllerlib.WithInformer(secrets, filter, controllerlib.InformerOption{}), + withInformer( + secretInformer, + pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool { + return metav1.IsControlledBy(obj, owner) + }, nil), + controllerlib.InformerOption{}, + ), controllerlib.WithInitialEvent(controllerlib.Key{ Namespace: owner.Namespace, Name: owner.Name + "-key", @@ -66,7 +69,7 @@ func NewSupervisorSecretsController( // Sync implements controllerlib.Syncer.Sync(). func (c *supervisorSecretsController) Sync(ctx controllerlib.Context) error { - secret, err := c.secrets.Lister().Secrets(ctx.Key.Namespace).Get(ctx.Key.Name) + secret, err := c.secretInformer.Lister().Secrets(ctx.Key.Namespace).Get(ctx.Key.Name) isNotFound := k8serrors.IsNotFound(err) if !isNotFound && err != nil { return fmt.Errorf("failed to list secret %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) @@ -75,7 +78,7 @@ func (c *supervisorSecretsController) Sync(ctx controllerlib.Context) error { secretNeedsUpdate := isNotFound || !isValid(secret) if !secretNeedsUpdate { plog.Debug("secret is up to date", "secret", klog.KObj(secret)) - c.setCache(secret.Data[symmetricKeySecretDataKey]) + c.setCacheFunc(secret.Data[symmetricKeySecretDataKey]) return nil } @@ -93,18 +96,18 @@ func (c *supervisorSecretsController) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to create/update secret %s/%s: %w", newSecret.Namespace, newSecret.Name, err) } - c.setCache(newSecret.Data[symmetricKeySecretDataKey]) + c.setCacheFunc(newSecret.Data[symmetricKeySecretDataKey]) return nil } func (c *supervisorSecretsController) createSecret(ctx context.Context, newSecret *corev1.Secret) error { - _, err := c.client.CoreV1().Secrets(newSecret.Namespace).Create(ctx, newSecret, metav1.CreateOptions{}) + _, err := c.kubeClient.CoreV1().Secrets(newSecret.Namespace).Create(ctx, newSecret, metav1.CreateOptions{}) return err } func (c *supervisorSecretsController) updateSecret(ctx context.Context, newSecret **corev1.Secret, secretName string) error { - secrets := c.client.CoreV1().Secrets((*newSecret).Namespace) + secrets := c.kubeClient.CoreV1().Secrets((*newSecret).Namespace) return retry.RetryOnConflict(retry.DefaultBackoff, func() error { currentSecret, err := secrets.Get(ctx, secretName, metav1.GetOptions{}) isNotFound := k8serrors.IsNotFound(err) diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go index e3ae8905..04ffbeda 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go @@ -21,10 +21,163 @@ import ( kubernetesfake "k8s.io/client-go/kubernetes/fake" kubetesting "k8s.io/client-go/testing" + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/testutil" ) -func TestController(t *testing.T) { +var ( + owner = &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-owner-name", + UID: "some-owner-uid", + }, + } + + ownerGVK = schema.GroupVersionKind{ + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, + Kind: "Deployment", + } + + labels = map[string]string{ + "some-label-key-1": "some-label-value-1", + "some-label-key-2": "some-label-value-2", + } +) + +func TestSupervisorSecretsControllerFilterSecret(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + secret corev1.Secret + wantAdd bool + wantUpdate bool + wantDelete bool + }{ + { + name: "no owner reference", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{}, + }, + }, + { + name: "owner reference without controller set to true", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Name: "some-name", + Kind: ownerGVK.Kind, + UID: owner.GetUID(), + }, + }, + }, + }, + }, + { + name: "owner reference without correct APIVersion", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Name: "some-name", + Kind: ownerGVK.Kind, + Controller: boolPtr(true), + UID: owner.GetUID(), + }}, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "owner reference without correct Kind", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Name: "some-name", + Kind: "IncorrectKind", + Controller: boolPtr(true), + UID: owner.GetUID(), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "correct owner reference", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "multiple owner references", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "UnrelatedKind", + }, + *metav1.NewControllerRef(owner, ownerGVK), + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + secretInformer := kubeinformers.NewSharedInformerFactory( + kubernetesfake.NewSimpleClientset(), + 0, + ).Core().V1().Secrets() + withInformer := testutil.NewObservableWithInformerOption() + _ = NewSupervisorSecretsController( + owner, + labels, + nil, // kubeClient, not needed + secretInformer, + nil, // setCache, not needed + withInformer.WithInformer, + ) + + unrelated := corev1.Secret{} + filter := withInformer.GetFilterForInformer(secretInformer) + require.Equal(t, test.wantAdd, filter.Add(&test.secret)) + require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.secret)) + require.Equal(t, test.wantUpdate, filter.Update(&test.secret, &unrelated)) + require.Equal(t, test.wantDelete, filter.Delete(&test.secret)) + }) + } +} + +func TestSupervisorSecretsControllerSync(t *testing.T) { const ( generatedSecretNamespace = "some-namespace" generatedSecretName = "some-name-abc123" @@ -37,26 +190,9 @@ func TestController(t *testing.T) { Resource: "secrets", } - owner = &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "some-owner-name", - UID: "some-owner-uid", - }, - } - ownerGVK = schema.GroupVersionKind{ - Group: appsv1.SchemeGroupVersion.Group, - Version: appsv1.SchemeGroupVersion.Version, - Kind: "Deployment", - } - generatedSymmetricKey = []byte("some-neato-32-byte-generated-key") otherGeneratedSymmetricKey = []byte("some-funio-32-byte-generated-key") - labels = map[string]string{ - "some-label-key-1": "some-label-value-1", - "some-label-key-2": "some-label-value-2", - } - generatedSecret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: generatedSecretName, @@ -313,10 +449,17 @@ func TestController(t *testing.T) { secrets := informers.Core().V1().Secrets() var callbackSecret []byte - c := NewSupervisorSecretsController(owner, labels, apiClient, secrets, func(secret []byte) { - require.Nil(t, callbackSecret, "callback was called twice") - callbackSecret = secret - }) + c := NewSupervisorSecretsController( + owner, + labels, + apiClient, + secrets, + func(secret []byte) { + require.Nil(t, callbackSecret, "callback was called twice") + callbackSecret = secret + }, + testutil.NewObservableWithInformerOption().WithInformer, + ) // Must start informers before calling TestRunSynchronously(). informers.Start(ctx.Done())