Only watch ResourceGroups with the correct owner label (#1439)

* Only watch ResourceGroups with the correct owner label

* Check new object for ownership too

* Refactor test to cover all cases

* Fix more failing checks

* Only filter events for ResourceGroup resources

* Fix typo

* Set GVK on all test objects

* Test that we always include events from non-ResourceGroup resources

Author: tomasaschan

e2e/nomostest/testresourcegroup/resourcegroup.go

@@ -25,6 +25,7 @@ import (
 	"kpt.dev/configsync/e2e/nomostest/testkubeclient"
 	"kpt.dev/configsync/pkg/api/configmanagement"
 	"kpt.dev/configsync/pkg/api/kpt.dev/v1alpha1"
+	"kpt.dev/configsync/pkg/metadata"
 	"kpt.dev/configsync/pkg/resourcegroup/controllers/resourcegroup"
 	"kpt.dev/configsync/pkg/resourcegroup/controllers/status"
 	"sigs.k8s.io/cli-utils/pkg/common"
@@ -45,6 +46,7 @@ func New(nn types.NamespacedName, id string) *v1alpha1.ResourceGroup {
 	if id != "" {
 		rg.SetLabels(map[string]string{
 			common.InventoryLabel: id,
+			metadata.ManagedByKey: metadata.ManagedByValue,
 		})
 	}
 	return rg

pkg/resourcegroup/controllers/root/root_controller.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/util/retry"
 	"kpt.dev/configsync/pkg/api/kpt.dev/v1alpha1"
+	"kpt.dev/configsync/pkg/metadata"
 	"kpt.dev/configsync/pkg/resourcegroup/controllers/handler"
 	"kpt.dev/configsync/pkg/resourcegroup/controllers/resourcegroup"
 	"kpt.dev/configsync/pkg/resourcegroup/controllers/resourcemap"
@@ -211,6 +212,8 @@ func NewController(mgr manager.Manager, channel chan event.GenericEvent,
 		WithEventFilter(ResourceGroupPredicate{}).
 		// skip the Generic events
 		WithEventFilter(NoGenericEventPredicate{}).
+		// only reconcile resource groups owned by Config Sync
+		WithEventFilter(OwnedByConfigSyncPredicate{}).
 		Watches(&apiextensionsv1.CustomResourceDefinition{}, &handler.CRDEventHandler{
 			Mapping: resMap,
 			Channel: channel,
@@ -296,3 +299,37 @@ func isConditionTrue(cType v1alpha1.ConditionType, conditions []v1alpha1.Conditi
 
 	return false
 }
+
+// OwnedByConfigSyncPredicate filters events for objects that have the label "app.kubernetes.io/managed-by=configmanagement.gke.io"
+type OwnedByConfigSyncPredicate struct{}
+
+// Create implements predicate.Predicate
+func (OwnedByConfigSyncPredicate) Create(e event.CreateEvent) bool {
+	return !isResourceGroup(e.Object) || isOwnedByConfigSync(e.Object)
+}
+
+// Delete implements predicate.Predicate
+func (OwnedByConfigSyncPredicate) Delete(e event.DeleteEvent) bool {
+	return !isResourceGroup(e.Object) || isOwnedByConfigSync(e.Object)
+}
+
+// Update implements predicate.Predicate
+func (OwnedByConfigSyncPredicate) Update(e event.UpdateEvent) bool {
+	return !isResourceGroup(e.ObjectOld) || !isResourceGroup(e.ObjectNew) || isOwnedByConfigSync(e.ObjectOld) || isOwnedByConfigSync(e.ObjectNew)
+}
+
+// Generic implements predicate.Predicate
+func (OwnedByConfigSyncPredicate) Generic(e event.GenericEvent) bool {
+	return !isResourceGroup(e.Object) || isOwnedByConfigSync(e.Object)
+}
+
+func isResourceGroup(o client.Object) bool {
+	return o.GetObjectKind().GroupVersionKind().Group == v1alpha1.SchemeGroupVersion.Group &&
+		o.GetObjectKind().GroupVersionKind().Kind == v1alpha1.ResourceGroupKind
+}
+
+func isOwnedByConfigSync(o client.Object) bool {
+	labels := o.GetLabels()
+	owner, ok := labels[metadata.ManagedByKey]
+	return ok && owner == metadata.ManagedByValue
+}

pkg/resourcegroup/controllers/root/root_controller_test.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
@@ -196,3 +197,105 @@ func TestRootReconciler(t *testing.T) {
 	time.Sleep(time.Second)
 	assert.Equal(t, "group0", e.Object.GetName())
 }
+
+func TestOwnedByConfigSyncPredicate(t *testing.T) {
+	type testCase struct {
+		name string
+		got  bool
+		want bool
+	}
+
+	ownedByConfigSync := &v1alpha1.ResourceGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels: map[string]string{
+				"app.kubernetes.io/managed-by": "configmanagement.gke.io",
+			},
+			Namespace: "foo",
+			Name:      "bar",
+		},
+	}
+	ownedByConfigSync.SetGroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("ResourceGroup"))
+
+	ownedBySomeoneElse := &v1alpha1.ResourceGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels: map[string]string{
+				"app.kubernetes.io/managed-by": "someone-else",
+			},
+			Namespace: "foo",
+			Name:      "bar",
+		},
+	}
+	ownedBySomeoneElse.SetGroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("ResourceGroup"))
+
+	notOwned := &v1alpha1.ResourceGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+			Name:      "bar",
+		},
+	}
+	notOwned.SetGroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("ResourceGroup"))
+
+	notResourceGroup := &v1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: "foo"}}
+	notResourceGroup.SetGroupVersionKind(v1.SchemeGroupVersion.WithKind("CustomResourceDefinition"))
+
+	create := func(o client.Object) bool {
+		return OwnedByConfigSyncPredicate{}.Create(
+			event.TypedCreateEvent[client.Object]{Object: o},
+		)
+	}
+	update := func(o client.Object, n client.Object) bool {
+		return OwnedByConfigSyncPredicate{}.Update(
+			event.TypedUpdateEvent[client.Object]{ObjectOld: o, ObjectNew: n},
+		)
+	}
+
+	// not allowed to redefine delete, so we use a different name
+	yeet := func(o client.Object) bool {
+		return OwnedByConfigSyncPredicate{}.Delete(
+			event.TypedDeleteEvent[client.Object]{Object: o},
+		)
+	}
+
+	generic := func(o client.Object) bool {
+		return OwnedByConfigSyncPredicate{}.Generic(
+			event.TypedGenericEvent[client.Object]{Object: o},
+		)
+	}
+
+	testCases := []testCase{
+		{name: "create owned by configsync", got: create(ownedByConfigSync), want: true},
+		{name: "create owned by someone else", got: create(ownedBySomeoneElse), want: false},
+		{name: "create not owned", got: create(notOwned), want: false},
+		{name: "create non-resourcegroup owned by configsync", got: create(notResourceGroup), want: true},
+
+		{name: "update both owned by configsync", got: update(ownedByConfigSync, ownedByConfigSync), want: true},
+		{name: "update old owned by configsync, new owned by someone else", got: update(ownedByConfigSync, ownedBySomeoneElse), want: true},
+		{name: "update old owned by someone else, new owned by configsync", got: update(ownedBySomeoneElse, ownedByConfigSync), want: true},
+		{name: "update both owned by someone else", got: update(ownedBySomeoneElse, ownedBySomeoneElse), want: false},
+		{name: "update old not owned, new owned by configsync", got: update(notOwned, ownedByConfigSync), want: true},
+		{name: "update old owned by configsync, new not owned", got: update(ownedByConfigSync, notOwned), want: true},
+		{name: "update both not owned", got: update(notOwned, notOwned), want: false},
+		{name: "update old not owned, new owned by someone else", got: update(notOwned, ownedBySomeoneElse), want: false},
+		{name: "update old owned by someone else, new not owned", got: update(ownedBySomeoneElse, notOwned), want: false},
+		{name: "update non-resourcegroup owned by configsync", got: update(notResourceGroup, notResourceGroup), want: true},
+
+		{name: "delete owned by configsync", got: yeet(ownedByConfigSync), want: true},
+		{name: "delete owned by someone else", got: yeet(ownedBySomeoneElse), want: false},
+		{name: "delete not owned", got: yeet(notOwned), want: false},
+		{name: "delete non-resourcegroup owned by configsync", got: yeet(notResourceGroup), want: true},
+
+		{name: "generic owned by configsync", got: generic(ownedByConfigSync), want: true},
+		{name: "generic owned by someone else", got: generic(ownedBySomeoneElse), want: false},
+		{name: "generic not owned", got: generic(notOwned), want: false},
+		{name: "generic non-resourcegroup owned by configsync", got: generic(notResourceGroup), want: true},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+			if testCase.got != testCase.want {
+				t.Errorf("%s = %v, want %v", testCase.name, testCase.got, testCase.want)
+			}
+		})
+	}
+}