Sync garbage collector controller less often by adjusting its filters

- Only sync on add/update of secrets in the same namespace which
  have the "storage.pinniped.dev/garbage-collect-after" annotation, and
  also during a full resync of the informer whenever secrets in the
  same namespace with that annotation exist.
- Ignore deleted secrets to avoid having this controller trigger itself
  unnecessarily when it deletes a secret. This controller is never
  interested in deleted secrets, since its only job is to delete
  existing secrets.
- No change to the self-imposed rate limit logic. That still applies
  because secrets with this annotation will be created and updated
  regularly while the system is running (not just during rare system
  configuration steps).

internal/controller/supervisorstorage/garbage_collector.go

@@ -34,6 +34,14 @@ func GarbageCollectorController(
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {
+	isSecretWithGCAnnotation := func(obj metav1.Object) bool {
+		secret, ok := obj.(*v1.Secret)
+		if !ok {
+			return false
+		}
+		_, ok = secret.Annotations[crud.SecretLifetimeAnnotationKey]
+		return ok
+	}
 	return controllerlib.New(
 		controllerlib.Config{
 			Name: "garbage-collector-controller",
@@ -45,7 +53,14 @@ func GarbageCollectorController(
 		},
 		withInformer(
 			secretInformer,
-			pinnipedcontroller.MatchAnythingFilter(nil),
+			controllerlib.FilterFuncs{
+				AddFunc: isSecretWithGCAnnotation,
+				UpdateFunc: func(oldObj, newObj metav1.Object) bool {
+					return isSecretWithGCAnnotation(oldObj) || isSecretWithGCAnnotation(newObj)
+				},
+				DeleteFunc: func(obj metav1.Object) bool { return false }, // ignore all deletes
+				ParentFunc: nil,
+			},
 			controllerlib.InformerOption{},
 		),
 	)

internal/controller/supervisorstorage/garbage_collector_test.go

@@ -49,21 +49,46 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) {
 		when("watching Secret objects", func() {
 			var (
 				subject                           controllerlib.Filter
-				secret, otherSecret *corev1.Secret
+				secretWithAnnotation, otherSecret *corev1.Secret
 			)
 
 			it.Before(func() {
 				subject = secretsInformerFilter
-				secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}}
+				secretWithAnnotation = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace", Annotations: map[string]string{
-				otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}}
+					"storage.pinniped.dev/garbage-collect-after": "some timestamp",
+				}}}
+				otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-namespace"}}
 			})
 
-			when("any Secret changes", func() {
+			when("any Secret with the required annotation is added or updated", func() {
-				it("returns true to trigger the sync function for all secrets", func() {
+				it("returns true to trigger the sync function", func() {
-					r.True(subject.Add(secret))
+					r.True(subject.Add(secretWithAnnotation))
-					r.True(subject.Update(secret, otherSecret))
+					r.True(subject.Update(secretWithAnnotation, otherSecret))
-					r.True(subject.Update(otherSecret, secret))
+					r.True(subject.Update(otherSecret, secretWithAnnotation))
-					r.True(subject.Delete(secret))
+				})
+			})
+
+			when("any Secret with the required annotation is deleted", func() {
+				it("returns false to skip the sync function because it does not need to worry about secrets that are already gone", func() {
+					r.False(subject.Delete(secretWithAnnotation))
+				})
+			})
+
+			when("any Secret without the required annotation changes", func() {
+				it("returns false to skip the sync function", func() {
+					r.False(subject.Add(otherSecret))
+					r.False(subject.Update(otherSecret, otherSecret))
+					r.False(subject.Delete(otherSecret))
+				})
+			})
+
+			when("any other type is passed", func() {
+				it("returns false to skip the sync function", func() {
+					wrongType := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "some-ns", Namespace: "some-ns"}}
+
+					r.False(subject.Add(wrongType))
+					r.False(subject.Update(wrongType, wrongType))
+					r.False(subject.Delete(wrongType))
 				})
 			})
 		})

test/integration/supervisor_storage_garbage_collection_test.go

@@ -44,11 +44,12 @@ func TestStorageGarbageCollection(t *testing.T) {
 	}
 
 	// Start a background goroutine which will end as soon as the test ends.
-	// Keep updating a secret in the same namespace just to get the controller to respond faster.
+	// Keep updating a secret which has the "storage.pinniped.dev/garbage-collect-after" annotation
-	// This is just a performance optimization because otherwise this test has to wait
+	// in the same namespace just to get the controller to respond faster.
-	// ~3 minutes for the controller's next full-resync.
+	// This is just a performance optimization to make this test pass faster because otherwise
+	// this test has to wait ~3 minutes for the controller's next full-resync.
 	stopCh := make(chan bool, 1) // It is important that this channel be buffered.
-	go createAndUpdateSecretEveryTwoSeconds(t, stopCh, secrets)
+	go updateSecretEveryTwoSeconds(t, stopCh, secrets, secretNotYetExpired)
 	t.Cleanup(func() {
 		stopCh <- true
 	})
@@ -68,12 +69,10 @@ func TestStorageGarbageCollection(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface) {
+func updateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface, secret *v1.Secret) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
 	defer cancel()
 
-	unrelatedSecret := createSecret(ctx, t, secrets, "unrelated-to-gc", time.Time{})
-
 	i := 0
 	for {
 		select {
@@ -87,9 +86,9 @@ func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secret
 		time.Sleep(2 * time.Second)
 
 		i++
-		unrelatedSecret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i))
+		secret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i))
 		var updateErr error
-		unrelatedSecret, updateErr = secrets.Update(ctx, unrelatedSecret, metav1.UpdateOptions{})
+		secret, updateErr = secrets.Update(ctx, secret, metav1.UpdateOptions{})
 		require.NoError(t, updateErr)
 	}
 }
@@ -125,6 +124,6 @@ func newSecret(namePrefix string, expiresAt time.Time) *v1.Secret {
 			Annotations:  annotations,
 		},
 		Data: map[string][]byte{"some-key": []byte("fake-data")},
-		Type: "storage.pinniped.dev/gc-test-integration-test",
+		Type: "storage.pinniped.dev/gc-test-integration-test", // the garbage collector controller doesn't care about the type
 	}
 }