From 162c615753932a1a72180b9c5038ab4e5ae905c6 Mon Sep 17 00:00:00 2001 From: Tomas Aschan <1550920+tomasaschan@users.noreply.github.com> Date: Fri, 27 Sep 2024 21:57:36 +0200 Subject: [PATCH] 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 --- .../testresourcegroup/resourcegroup.go | 2 + .../controllers/root/root_controller.go | 37 +++++++ .../controllers/root/root_controller_test.go | 103 ++++++++++++++++++ 3 files changed, 142 insertions(+) diff --git a/e2e/nomostest/testresourcegroup/resourcegroup.go b/e2e/nomostest/testresourcegroup/resourcegroup.go index 5ef7e78ce8..7ada95cbe9 100644 --- a/e2e/nomostest/testresourcegroup/resourcegroup.go +++ b/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 diff --git a/pkg/resourcegroup/controllers/root/root_controller.go b/pkg/resourcegroup/controllers/root/root_controller.go index 3cead3bb1f..6b8de5d7e2 100644 --- a/pkg/resourcegroup/controllers/root/root_controller.go +++ b/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 +} diff --git a/pkg/resourcegroup/controllers/root/root_controller_test.go b/pkg/resourcegroup/controllers/root/root_controller_test.go index 5f974dc83d..d6ece13e90 100644 --- a/pkg/resourcegroup/controllers/root/root_controller_test.go +++ b/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) + } + }) + } +}