From 58c7cb4abb97688dd63ea1b141a0bc0c22a241a2 Mon Sep 17 00:00:00 2001
From: Sam Dowell <sdowell@google.com>
Date: Wed, 1 Nov 2023 17:55:31 -0700
Subject: [PATCH] fix: map RepoSync RoleBinding to RepoSync namespace (#989)

The RoleBinding created for RepoSyncs is actually in the namespace of
the RepoSync, rather than the controller namespace. This change fixes
the watch configuration and mapping logic to correctly map the
RoleBinding.
---
 .../controllers/reposync_controller.go        | 20 +++++++++++--------
 .../controllers/reposync_controller_test.go   | 14 ++++---------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/pkg/reconcilermanager/controllers/reposync_controller.go b/pkg/reconcilermanager/controllers/reposync_controller.go
index 24c228dc91..6236673d79 100644
--- a/pkg/reconcilermanager/controllers/reposync_controller.go
+++ b/pkg/reconcilermanager/controllers/reposync_controller.go
@@ -512,7 +512,9 @@ func (r *RepoSyncReconciler) SetupWithManager(mgr controllerruntime.Manager, wat
 		Watches(&source.Kind{Type: withNamespace(&corev1.ServiceAccount{}, configsync.ControllerNamespace)},
 			handler.EnqueueRequestsFromMapFunc(r.mapObjectToRepoSync),
 			builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})).
-		Watches(&source.Kind{Type: withNamespace(&rbacv1.RoleBinding{}, configsync.ControllerNamespace)},
+		// Watch RoleBindings in all namespaces, because RoleBindings are created
+		// in the namespace of the RepoSync. Only maps to existing RepoSyncs.
+		Watches(&source.Kind{Type: &rbacv1.RoleBinding{}},
 			handler.EnqueueRequestsFromMapFunc(r.mapObjectToRepoSync),
 			builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}))
 
@@ -759,12 +761,6 @@ func (r *RepoSyncReconciler) mapConfigMapToRepoSyncs(obj client.Object) []reconc
 func (r *RepoSyncReconciler) mapObjectToRepoSync(obj client.Object) []reconcile.Request {
 	objRef := client.ObjectKeyFromObject(obj)
 
-	// Ignore changes from other namespaces because all the generated resources
-	// exist in the config-management-system namespace.
-	if objRef.Namespace != configsync.ControllerNamespace {
-		return nil
-	}
-
 	// Ignore changes from resources without the ns-reconciler prefix or configsync.gke.io:ns-reconciler
 	// because all the generated resources have the prefix.
 	nsRoleBindingName := RepoSyncPermissionsName()
@@ -778,6 +774,14 @@ func (r *RepoSyncReconciler) mapObjectToRepoSync(obj client.Object) []reconcile.
 		return nil
 	}
 
+	if objRef.Namespace != configsync.ControllerNamespace {
+		switch obj.(type) {
+		case *corev1.ServiceAccount, *appsv1.Deployment:
+			// All Deployments and ServiceAccounts are in config-management-system
+			return nil
+		}
+	}
+
 	allRepoSyncs := &v1beta1.RepoSyncList{}
 	if err := r.client.List(context.Background(), allRepoSyncs); err != nil {
 		klog.Errorf("failed to list all RepoSyncs for %s (%s): %v",
@@ -794,7 +798,7 @@ func (r *RepoSyncReconciler) mapObjectToRepoSync(obj client.Object) []reconcile.
 		reconcilerName := core.NsReconcilerName(rs.GetNamespace(), rs.GetName())
 		switch obj.(type) {
 		case *rbacv1.RoleBinding:
-			if objRef.Name == nsRoleBindingName {
+			if objRef.Name == nsRoleBindingName && objRef.Namespace == rs.Namespace {
 				requests = append(requests, reconcile.Request{
 					NamespacedName: client.ObjectKeyFromObject(&rs),
 				})
diff --git a/pkg/reconcilermanager/controllers/reposync_controller_test.go b/pkg/reconcilermanager/controllers/reposync_controller_test.go
index f6ee846565..ef17f7d983 100644
--- a/pkg/reconcilermanager/controllers/reposync_controller_test.go
+++ b/pkg/reconcilermanager/controllers/reposync_controller_test.go
@@ -2821,13 +2821,13 @@ func TestMapObjectToRepoSync(t *testing.T) {
 			want:   nil,
 		},
 		{
-			name:   fmt.Sprintf("A rolebinding from the %s namespace, different from %s", nsReconcilerKey.Namespace, rsRoleBindingName),
-			object: fake.RoleBindingObject(core.Name("any"), core.Namespace(nsReconcilerKey.Namespace)),
+			name:   fmt.Sprintf("A rolebinding from the %s namespace, different from %s", rs1.Namespace, rsRoleBindingName),
+			object: fake.RoleBindingObject(core.Name("any"), core.Namespace(rs1.Namespace)),
 			want:   nil,
 		},
 		{
-			name:   fmt.Sprintf("A rolebinding from the %s namespace, same as %s", nsReconcilerKey.Namespace, rsRoleBindingName),
-			object: fake.RoleBindingObject(core.Name(rsRoleBindingName), core.Namespace(nsReconcilerKey.Namespace)),
+			name:   fmt.Sprintf("A rolebinding from the %s namespace, same as %s", rs1.Namespace, rsRoleBindingName),
+			object: fake.RoleBindingObject(core.Name(rsRoleBindingName), core.Namespace(rs1.Namespace)),
 			want: []reconcile.Request{
 				{
 					NamespacedName: types.NamespacedName{
@@ -2835,12 +2835,6 @@ func TestMapObjectToRepoSync(t *testing.T) {
 						Namespace: "ns1",
 					},
 				},
-				{
-					NamespacedName: types.NamespacedName{
-						Name:      "rs2",
-						Namespace: "ns2",
-					},
-				},
 			},
 		},
 	}