Several more controllers Sync less often by adjusting their filters

- JWKSWriterController
- JWKSObserverController
- FederationDomainSecretsController for HMAC keys
- FederationDomainSecretsController for state signature key
- FederationDomainSecretsController for state encryption key

Signed-off-by: Ryan Richard <richardry@vmware.com>

internal/controller/supervisorconfig/generator/federation_domain_secrets.go

@@ -65,16 +65,7 @@ func NewFederationDomainSecretsController(
 		// should get notified via the corresponding FederationDomain key.
 		withInformer(
 			secretInformer,
-			pinnipedcontroller.SimpleFilter(isFederationDomainControllee, func(obj metav1.Object) controllerlib.Key {
-				if isFederationDomainControllee(obj) {
-					controller := metav1.GetControllerOf(obj)
-					return controllerlib.Key{
-						Name:      controller.Name,
-						Namespace: obj.GetNamespace(),
-					}
-				}
-				return controllerlib.Key{}
-			}),
+			pinnipedcontroller.SimpleFilter(secretHelper.Handles, pinnipedcontroller.SecretIsControlledByParentFunc(secretHelper.Handles)),
 			controllerlib.InformerOption{},
 		),
 		// We want to be notified when anything happens to an FederationDomain.
@@ -240,11 +231,3 @@ func (c *federationDomainSecretsController) updateFederationDomain(
 		return err
 	})
 }
-
-// isFederationDomainControllee returns whether the provided obj is controlled by an FederationDomain.
-func isFederationDomainControllee(obj metav1.Object) bool {
-	controller := metav1.GetControllerOf(obj)
-	return controller != nil &&
-		controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() &&
-		controller.Kind == federationDomainKind
-}

internal/controller/supervisorconfig/generator/federation_domain_secrets_test.go

@@ -5,6 +5,7 @@ package generator
 
 import (
 	"context"
+	"crypto/rand"
 	"errors"
 	"fmt"
 	"sync"
@@ -35,7 +36,7 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 
 	tests := []struct {
 		name       string
-		secret     corev1.Secret
+		secret     metav1.Object
 		wantAdd    bool
 		wantUpdate bool
 		wantDelete bool
@@ -43,13 +44,15 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 	}{
 		{
 			name: "no owner reference",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type:       "secrets.pinniped.dev/federation-domain-token-signing-key",
 				ObjectMeta: metav1.ObjectMeta{},
 			},
 		},
 		{
 			name: "owner reference without correct APIVersion",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -64,7 +67,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "owner reference without correct Kind",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -79,7 +83,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "owner reference without controller set to true",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -94,7 +99,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "correct owner reference",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -114,7 +120,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "multiple owner references",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -135,16 +142,44 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 			wantDelete: true,
 			wantParent: controllerlib.Key{Namespace: "some-namespace", Name: "some-name"},
 		},
+		{
+			name: "correct owner reference but wrong secret type",
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/this-is-the-wrong-type",
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "some-namespace",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: configv1alpha1.SchemeGroupVersion.String(),
+							Kind:       "FederationDomain",
+							Name:       "some-name",
+							Controller: boolPtr(true),
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "resource of wrong data type",
+			secret: &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "some-namespace",
+				},
+			},
+		},
 	}
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
 
-			ctrl := gomock.NewController(t)
-			t.Cleanup(ctrl.Finish)
-			secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl)
-			secretHelper.EXPECT().NamePrefix().Times(1).Return("some-name")
+			secretHelper := NewSymmetricSecretHelper(
+				"some-name",
+				map[string]string{},
+				rand.Reader,
+				SecretUsageTokenSigningKey,
+				func(cacheKey string, cacheValue []byte) {},
+			)
 
 			secretInformer := kubeinformers.NewSharedInformerFactory(
 				kubernetesfake.NewSimpleClientset(),
@@ -167,11 +202,11 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
 
 			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))
-			require.Equal(t, test.wantParent, filter.Parent(&test.secret))
+			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))
+			require.Equal(t, test.wantParent, filter.Parent(test.secret))
 		})
 	}
 }
@@ -201,10 +236,13 @@ func TestNewFederationDomainSecretsControllerFilterFederationDomain(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().NamePrefix().Times(1).Return("some-name")
+			secretHelper := NewSymmetricSecretHelper(
+				"some-name",
+				map[string]string{},
+				rand.Reader,
+				SecretUsageTokenSigningKey,
+				func(cacheKey string, cacheValue []byte) {},
+			)
 
 			secretInformer := kubeinformers.NewSharedInformerFactory(
 				kubernetesfake.NewSimpleClientset(),
@@ -635,6 +673,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) {
 			if test.secretHelper != nil {
 				test.secretHelper(secretHelper)
 			}
+			secretHelper.EXPECT().Handles(gomock.Any()).AnyTimes().Return(true)
 
 			c := NewFederationDomainSecretsController(
 				secretHelper,

internal/controller/supervisorconfig/generator/secret_helper.go

@@ -23,6 +23,7 @@ type SecretHelper interface {
 	Generate(*configv1alpha1.FederationDomain) (*corev1.Secret, error)
 	IsValid(*configv1alpha1.FederationDomain, *corev1.Secret) bool
 	ObserveActiveSecretAndUpdateParentFederationDomain(*configv1alpha1.FederationDomain, *corev1.Secret) *configv1alpha1.FederationDomain
+	Handles(metav1.Object) bool
 }
 
 const (
@@ -83,6 +84,29 @@ type symmetricSecretHelper struct {
 	updateCacheFunc func(cacheKey string, cacheValue []byte)
 }
 
+func (s *symmetricSecretHelper) Handles(obj metav1.Object) bool {
+	return IsFederationDomainSecretOfType(obj, s.secretType())
+}
+
+func IsFederationDomainSecretOfType(obj metav1.Object, secretType corev1.SecretType) bool {
+	secret, ok := obj.(*corev1.Secret)
+	if !ok {
+		return false
+	}
+	if secret.Type != secretType {
+		return false
+	}
+	return isFederationDomainControllee(secret)
+}
+
+// isFederationDomainControllee returns whether the provided obj is controlled by an FederationDomain.
+func isFederationDomainControllee(obj metav1.Object) bool {
+	controller := metav1.GetControllerOf(obj)
+	return controller != nil &&
+		controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() &&
+		controller.Kind == federationDomainKind
+}
+
 func (s *symmetricSecretHelper) NamePrefix() string { return s.namePrefix }
 
 // Generate implements SecretHelper.Generate().

internal/controller/supervisorconfig/generator/secret_helper_test.go

@@ -108,6 +108,14 @@ func TestSymmetricSecretHelper(t *testing.T) {
 			require.Equal(t, parent.Spec.Issuer, federationDomainIssuerValue)
 			require.Equal(t, child.Name, test.wantSetFederationDomainField(parent))
 			require.Equal(t, child.Data["key"], symmetricKeyValue)
+
+			require.True(t, h.Handles(child))
+			wrongTypedChild := child.DeepCopy()
+			wrongTypedChild.Type = "the-wrong-type"
+			require.False(t, h.Handles(wrongTypedChild))
+			wrongOwnerKindChild := child.DeepCopy()
+			wrongOwnerKindChild.OwnerReferences[0].Kind = "WrongKind"
+			require.False(t, h.Handles(wrongOwnerKindChild))
 		})
 	}
 }

internal/controller/supervisorconfig/jwks_observer.go

@@ -52,7 +52,7 @@ func NewJWKSObserverController(
 		},
 		withInformer(
 			secretInformer,
-			pinnipedcontroller.MatchAnythingFilter(nil),
+			pinnipedcontroller.MatchAnySecretOfTypeFilter(jwksSecretTypeValue),
 			controllerlib.InformerOption{},
 		),
 		withInformer(

internal/controller/supervisorconfig/jwks_observer_test.go

@@ -51,24 +51,33 @@ func TestJWKSObserverControllerInformerFilters(t *testing.T) {
 
 		when("watching Secret objects", func() {
 			var (
-				subject             controllerlib.Filter
-				secret, otherSecret *corev1.Secret
+				subject                 controllerlib.Filter
+				secret, otherTypeSecret *corev1.Secret
 			)
 
 			it.Before(func() {
 				subject = secretsInformerFilter
-				secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}}
-				otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}}
+				secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}, Type: "secrets.pinniped.dev/federation-domain-jwks"}
+				otherTypeSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}, Type: "other"}
 			})
 
-			when("any Secret changes", func() {
+			when("any Secret of the JWKS type changes", func() {
 				it("returns true to trigger the sync method", func() {
 					r.True(subject.Add(secret))
-					r.True(subject.Update(secret, otherSecret))
-					r.True(subject.Update(otherSecret, secret))
+					r.True(subject.Update(secret, otherTypeSecret))
+					r.True(subject.Update(otherTypeSecret, secret))
 					r.True(subject.Delete(secret))
 				})
 			})
+
+			when("any Secret of some other type changes", func() {
+				it("returns false to skip the sync method", func() {
+					r.False(subject.Add(otherTypeSecret))
+					r.False(subject.Update(otherTypeSecret, otherTypeSecret))
+					r.False(subject.Update(otherTypeSecret, otherTypeSecret))
+					r.False(subject.Delete(otherTypeSecret))
+				})
+			})
 		})
 
 		when("watching FederationDomain objects", func() {

internal/controller/supervisorconfig/jwks_writer.go

@@ -26,6 +26,7 @@ import (
 	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/controller/supervisorconfig/generator"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/plog"
 )
@@ -76,6 +77,10 @@ func NewJWKSWriterController(
 	federationDomainInformer configinformers.FederationDomainInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {
+	isSecretToSync := func(obj metav1.Object) bool {
+		return generator.IsFederationDomainSecretOfType(obj, jwksSecretTypeValue)
+	}
+
 	return controllerlib.New(
 		controllerlib.Config{
 			Name: "JWKSController",
@@ -91,23 +96,7 @@ func NewJWKSWriterController(
 		// should get notified via the corresponding FederationDomain key.
 		withInformer(
 			secretInformer,
-			controllerlib.FilterFuncs{
-				ParentFunc: func(obj metav1.Object) controllerlib.Key {
-					if isFederationDomainControllee(obj) {
-						controller := metav1.GetControllerOf(obj)
-						return controllerlib.Key{
-							Name:      controller.Name,
-							Namespace: obj.GetNamespace(),
-						}
-					}
-					return controllerlib.Key{}
-				},
-				AddFunc: isFederationDomainControllee,
-				UpdateFunc: func(oldObj, newObj metav1.Object) bool {
-					return isFederationDomainControllee(oldObj) || isFederationDomainControllee(newObj)
-				},
-				DeleteFunc: isFederationDomainControllee,
-			},
+			pinnipedcontroller.SimpleFilter(isSecretToSync, pinnipedcontroller.SecretIsControlledByParentFunc(isSecretToSync)),
 			controllerlib.InformerOption{},
 		),
 		// We want to be notified when anything happens to an FederationDomain.
@@ -316,14 +305,6 @@ func (c *jwksWriterController) updateFederationDomain(
 	})
 }
 
-// isFederationDomainControlle returns whether the provided obj is controlled by a FederationDomain.
-func isFederationDomainControllee(obj metav1.Object) bool {
-	controller := metav1.GetControllerOf(obj)
-	return controller != nil &&
-		controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() &&
-		controller.Kind == federationDomainKind
-}
-
 // isValid returns whether the provided secret contains a valid active JWK and verification JWKS.
 func isValid(secret *corev1.Secret) bool {
 	if secret.Type != jwksSecretTypeValue {

internal/controller/supervisorconfig/jwks_writer_test.go

@@ -35,7 +35,7 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 
 	tests := []struct {
 		name       string
-		secret     corev1.Secret
+		secret     metav1.Object
 		wantAdd    bool
 		wantUpdate bool
 		wantDelete bool
@@ -43,13 +43,15 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 	}{
 		{
 			name: "no owner reference",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type:       "secrets.pinniped.dev/federation-domain-jwks",
 				ObjectMeta: metav1.ObjectMeta{},
 			},
 		},
 		{
 			name: "owner reference without correct APIVersion",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-jwks",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -64,7 +66,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "owner reference without correct Kind",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-jwks",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -79,7 +82,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "owner reference without controller set to true",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-jwks",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -94,7 +98,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "correct owner reference",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-jwks",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -114,7 +119,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 		},
 		{
 			name: "multiple owner references",
-			secret: corev1.Secret{
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/federation-domain-jwks",
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "some-namespace",
 					OwnerReferences: []metav1.OwnerReference{
@@ -135,6 +141,31 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 			wantDelete: true,
 			wantParent: controllerlib.Key{Namespace: "some-namespace", Name: "some-name"},
 		},
+		{
+			name: "correct owner reference but wrong type",
+			secret: &corev1.Secret{
+				Type: "secrets.pinniped.dev/some-other-type",
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "some-namespace",
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: configv1alpha1.SchemeGroupVersion.String(),
+							Kind:       "FederationDomain",
+							Name:       "some-name",
+							Controller: boolPtr(true),
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "resource of wrong data type",
+			secret: &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "some-namespace",
+				},
+			},
+		},
 	}
 	for _, test := range tests {
 		test := test
@@ -161,11 +192,11 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
 
 			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))
-			require.Equal(t, test.wantParent, filter.Parent(&test.secret))
+			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))
+			require.Equal(t, test.wantParent, filter.Parent(test.secret))
 		})
 	}
 }

internal/controller/utils.go

@@ -4,6 +4,7 @@
 package controller
 
 import (
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/internal/controllerlib"
@@ -32,6 +33,30 @@ func SimpleFilter(match func(metav1.Object) bool, parentFunc controllerlib.Paren
 	}
 }
 
+func MatchAnySecretOfTypeFilter(secretType v1.SecretType) controllerlib.Filter {
+	isSecretOfType := func(obj metav1.Object) bool {
+		secret, ok := obj.(*v1.Secret)
+		if !ok {
+			return false
+		}
+		return secret.Type == secretType
+	}
+	return SimpleFilter(isSecretOfType, nil)
+}
+
+func SecretIsControlledByParentFunc(matchFunc func(obj metav1.Object) bool) func(obj metav1.Object) controllerlib.Key {
+	return func(obj metav1.Object) controllerlib.Key {
+		if matchFunc(obj) {
+			controller := metav1.GetControllerOf(obj)
+			return controllerlib.Key{
+				Name:      controller.Name,
+				Namespace: obj.GetNamespace(),
+			}
+		}
+		return controllerlib.Key{}
+	}
+}
+
 // SingletonQueue returns a parent func that treats all events as the same key.
 func SingletonQueue() controllerlib.ParentFunc {
 	return func(_ metav1.Object) controllerlib.Key {

internal/mocks/mocksecrethelper/mocksecrethelper.go

@@ -9,12 +9,11 @@
 package mocksecrethelper
 
 import (
-	reflect "reflect"
-
 	gomock "github.com/golang/mock/gomock"
-	v1 "k8s.io/api/core/v1"
-
 	v1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
+	v1 "k8s.io/api/core/v1"
+	v10 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	reflect "reflect"
 )
 
 // MockSecretHelper is a mock of SecretHelper interface
@@ -55,6 +54,20 @@ func (mr *MockSecretHelperMockRecorder) Generate(arg0 interface{}) *gomock.Call
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Generate", reflect.TypeOf((*MockSecretHelper)(nil).Generate), arg0)
 }
 
+// Handles mocks base method
+func (m *MockSecretHelper) Handles(arg0 v10.Object) bool {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "Handles", arg0)
+	ret0, _ := ret[0].(bool)
+	return ret0
+}
+
+// Handles indicates an expected call of Handles
+func (mr *MockSecretHelperMockRecorder) Handles(arg0 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Handles", reflect.TypeOf((*MockSecretHelper)(nil).Handles), arg0)
+}
+
 // IsValid mocks base method
 func (m *MockSecretHelper) IsValid(arg0 *v1alpha1.FederationDomain, arg1 *v1.Secret) bool {
 	m.ctrl.T.Helper()