Merge pull request #64 from pusher/fix-cgto-child-watch

Fix bug where multiple controllers attempt to reconcile CGTOs

Author: JoelSpeed

pkg/controller/gittrackobject/gittrackobject_controller.go

@@ -136,6 +136,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 				},
 				RestMapper: restMapper,
 			},
+			utils.NewOwnersOwnerInNamespacePredicate(mgr.GetClient()),
 		)
 		if err != nil {
 			msg := fmt.Sprintf("unable to watch channel: %v", err)

pkg/controller/gittrackobject/gittrackobject_controller_test.go

@@ -391,6 +391,14 @@ var (
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "example",
 				Namespace: "default",
+				OwnerReferences: []metav1.OwnerReference{
+					{
+						APIVersion: "faros.pusher.com/v1alpha1",
+						Kind:       "GitTrack",
+						UID:        gitTrack.UID,
+						Name:       gitTrack.Name,
+					},
+				},
 			},
 			Spec: farosv1alpha1.GitTrackObjectSpec{
 				Name: "deployment-example",
@@ -650,6 +658,10 @@ var (
 		var ns *v1.Namespace
 		var gt *farosv1alpha1.GitTrack
 		BeforeEach(func() {
+			CreateClusterInstance([]byte(exampleClusterRoleBinding))
+			Eventually(requests, timeout).Should(Receive(Equal(expectedClusterRequest)))
+			Eventually(requests, timeout).Should(Receive(Equal(expectedClusterRequest)))
+
 			ns = &v1.Namespace{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "not-default",
@@ -669,34 +681,32 @@ var (
 			}
 			Expect(c.Create(context.TODO(), gt)).NotTo(HaveOccurred())
 
-			clusterInstance = &farosv1alpha1.ClusterGitTrackObject{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "example",
-					OwnerReferences: []metav1.OwnerReference{
-						{
-							APIVersion: "faros.pusher.com/v1alpha1",
-							Kind:       "GitTrack",
-							UID:        gt.UID,
-							Name:       gt.Name,
-						},
-					},
-				},
-				Spec: farosv1alpha1.GitTrackObjectSpec{
-					Name: "clusterrolebinding-example",
-					Kind: "ClusterRoleBinding",
-					Data: []byte(exampleClusterRoleBinding),
+			key := types.NamespacedName{Name: "example"}
+			Expect(c.Get(context.TODO(), key, clusterInstance)).NotTo(HaveOccurred())
+			clusterInstance.SetOwnerReferences([]metav1.OwnerReference{
+				{
+					APIVersion: "faros.pusher.com/v1alpha1",
+					Kind:       "GitTrack",
+					Name:       gt.Name,
+					UID:        gt.UID,
 				},
-			}
-			Expect(c.Create(context.TODO(), clusterInstance)).NotTo(HaveOccurred())
+			})
+			Expect(c.Update(context.TODO(), clusterInstance)).NotTo(HaveOccurred())
 		})
 
-		AfterEach(func() {
-			Expect(c.Delete(context.TODO(), gt)).NotTo(HaveOccurred())
-			Expect(c.Delete(context.TODO(), clusterInstance)).NotTo(HaveOccurred())
+		It("should not reconcile it", func() {
+			Consistently(requests).ShouldNot(Receive())
 		})
 
-		It("should not reconcile it", func() {
-			Eventually(requests, timeout).ShouldNot(Receive())
+		It("should not reconcile when the child is modified", func() {
+			crb := &rbacv1.ClusterRoleBinding{}
+			key := types.NamespacedName{Name: "example"}
+			Expect(c.Get(context.TODO(), key, crb)).NotTo(HaveOccurred())
+
+			crb.SetLabels(map[string]string{})
+			Expect(c.Update(context.TODO(), crb)).NotTo(HaveOccurred())
+
+			Consistently(requests).ShouldNot(Receive())
 		})
 	}
 

pkg/utils/predicate.go

@@ -25,6 +25,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/event"
 )
 
+const (
+	farosGroupVersion = "faros.pusher.com/v1alpha1"
+)
+
 // OwnerInNamespacePredicate filters events to check the owner of the event
 // object is in the controller's namespace
 type OwnerInNamespacePredicate struct {
@@ -66,7 +70,7 @@ func (p OwnerInNamespacePredicate) ownerInNamespace(ownerRefs []metav1.OwnerRefe
 		return false
 	}
 	for _, ref := range ownerRefs {
-		if ref.Kind == "GitTrack" && ref.APIVersion == "faros.pusher.com/v1alpha1" {
+		if ref.Kind == "GitTrack" && ref.APIVersion == farosGroupVersion {
 			for _, gt := range gtList.Items {
 				if ref.UID == gt.UID {
 					return true
@@ -83,3 +87,78 @@ func NewOwnerInNamespacePredicate(client client.Client) OwnerInNamespacePredicat
 		client: client,
 	}
 }
+
+// OwnersOwnerInNamespacePredicate filters events to check the owners owner of
+// the event object is in the controller's namespace
+type OwnersOwnerInNamespacePredicate struct {
+	client                    client.Client
+	ownerInNamespacePredicate OwnerInNamespacePredicate
+}
+
+// Create returns true if the event object owners owner is in the same namespace
+func (p OwnersOwnerInNamespacePredicate) Create(e event.CreateEvent) bool {
+	return p.ownersOwnerInNamespace(e.Meta.GetOwnerReferences())
+}
+
+// Update returns true if the event object owners owner is in the same namespace
+func (p OwnersOwnerInNamespacePredicate) Update(e event.UpdateEvent) bool {
+	return p.ownersOwnerInNamespace(e.MetaNew.GetOwnerReferences())
+}
+
+// Delete returns true if the event object owners owner is in the same namespace
+func (p OwnersOwnerInNamespacePredicate) Delete(e event.DeleteEvent) bool {
+	return p.ownersOwnerInNamespace(e.Meta.GetOwnerReferences())
+}
+
+// Generic returns true if the event object owners owner is in the same namespace
+func (p OwnersOwnerInNamespacePredicate) Generic(e event.GenericEvent) bool {
+	return p.ownersOwnerInNamespace(e.Meta.GetOwnerReferences())
+}
+
+// ownersOwnerInNamespace returns true if the the GitTrackObject's GitTrack
+// owner of the event object is in the namespace  managed by the controller
+//
+// This works on the premise that listing objects from the client will only
+// return those in its cache.
+// When it is restricted to a namespace this should only be the GitTracks
+// in the namespace the controller is managing.
+func (p OwnersOwnerInNamespacePredicate) ownersOwnerInNamespace(ownerRefs []metav1.OwnerReference) bool {
+	cgtoList := &farosv1alpha1.ClusterGitTrackObjectList{}
+	err := p.client.List(context.TODO(), &client.ListOptions{}, cgtoList)
+	if err != nil {
+		// We can't list CGTOs so fail closed and ignore the requests
+		return false
+	}
+	gtoList := &farosv1alpha1.GitTrackObjectList{}
+	err = p.client.List(context.TODO(), &client.ListOptions{}, gtoList)
+	if err != nil {
+		// We can't list GTOs so fail closed and ignore the requests
+		return false
+	}
+
+	for _, ref := range ownerRefs {
+		if ref.Kind == "GitTrackObject" && ref.APIVersion == farosGroupVersion {
+			for _, gto := range gtoList.Items {
+				if ref.UID == gto.UID {
+					return p.ownerInNamespacePredicate.ownerInNamespace(gto.GetOwnerReferences())
+				}
+			}
+		}
+		if ref.Kind == "ClusterGitTrackObject" && ref.APIVersion == farosGroupVersion {
+			for _, cgto := range cgtoList.Items {
+				if ref.UID == cgto.UID {
+					return p.ownerInNamespacePredicate.ownerInNamespace(cgto.GetOwnerReferences())
+				}
+			}
+		}
+	}
+	return false
+}
+
+// NewOwnersOwnerInNamespacePredicate constructs a new OwnersOwnerInNamespacePredicate
+func NewOwnersOwnerInNamespacePredicate(client client.Client) OwnersOwnerInNamespacePredicate {
+	return OwnersOwnerInNamespacePredicate{
+		client: client,
+		ownerInNamespacePredicate: NewOwnerInNamespacePredicate(client),
+	}
+}