fix: namespace-qualify cluster-scoped ClusterRoleBinding names

controllers/authorino_controller.go

@@ -151,12 +151,12 @@ func (r *AuthorinoReconciler) cleanupClusterScopedPermissions(ctx context.Contex
 	// namespaced ones are garbage collected automatically by k8s because of the owner reference
 
 	// Delete instance-specific ClusterRoleBindings
-	managerBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crName, reconcilers.AuthorinoManagerClusterRoleBindingName, reconcilers.AuthorinoManagerClusterRoleName, sa, labels)
+	managerBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crNamespacedName.Namespace, crName, reconcilers.AuthorinoManagerClusterRoleBindingName, reconcilers.AuthorinoManagerClusterRoleName, sa, labels)
 	if err := r.Client.Delete(ctx, managerBinding); err != nil && !errors.IsNotFound(err) {
 		r.Log.Error(err, "failed to delete ClusterRoleBinding", "name", managerBinding.Name)
 	}
 
-	k8sAuthBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crName, reconcilers.AuthorinoK8sAuthClusterRoleBindingName, reconcilers.AuthorinoK8sAuthClusterRoleName, sa, labels)
+	k8sAuthBinding := authorinoResources.GetAuthorinoClusterRoleBinding(crNamespacedName.Namespace, crName, reconcilers.AuthorinoK8sAuthClusterRoleBindingName, reconcilers.AuthorinoK8sAuthClusterRoleName, sa, labels)
 	if err := r.Client.Delete(ctx, k8sAuthBinding); err != nil && !errors.IsNotFound(err) {
 		r.Log.Error(err, "failed to delete ClusterRoleBinding", "name", k8sAuthBinding.Name)
 	}

controllers/authorino_controller_test.go

@@ -120,7 +120,7 @@ var _ = Describe("Authorino controller", func() {
 			var bindingNsdName types.NamespacedName
 			if authorinoInstance.Spec.ClusterWide {
 				binding = &k8srbac.ClusterRoleBinding{}
-				bindingNsdName = types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName}
+				bindingNsdName = types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName}
 			} else {
 				binding = &k8srbac.RoleBinding{}
 				bindingNsdName = namespacedName(testAuthorinoNamespace, authorinoInstance.Name+"-authorino")
@@ -132,7 +132,7 @@ var _ = Describe("Authorino controller", func() {
 
 			// Authorino Auth ClusterRoleBinding
 			k8sAuthBinding := &k8srbac.ClusterRoleBinding{}
-			k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
+			k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
 
 			Eventually(func(ctx context.Context) error {
 				return k8sClient.Get(ctx, k8sAuthBindingNsdName, k8sAuthBinding)
@@ -378,7 +378,7 @@ var _ = Describe("Authorino controller", func() {
 
 			// Authorino ClusterRoleBinding
 			binding := &k8srbac.ClusterRoleBinding{}
-			bindingNsdName := types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName}
+			bindingNsdName := types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName}
 
 			Eventually(func(g Gomega, ctx context.Context) {
 				g.Expect(k8sClient.Get(ctx, bindingNsdName, binding)).ToNot(HaveOccurred())
@@ -389,7 +389,7 @@ var _ = Describe("Authorino controller", func() {
 
 			// Authorino Auth ClusterRoleBinding
 			k8sAuthBinding := &k8srbac.ClusterRoleBinding{}
-			k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
+			k8sAuthBindingNsdName := types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
 
 			Eventually(func(g Gomega, ctx context.Context) {
 				g.Expect(k8sClient.Get(ctx, k8sAuthBindingNsdName, k8sAuthBinding)).ToNot(HaveOccurred())
@@ -417,7 +417,7 @@ var _ = Describe("Authorino controller", func() {
 			Expect(k8sClient.Create(ctx, authorinoInstance)).Should(Succeed())
 
 			// Update binding name for the new instance
-			bindingNsdName = types.NamespacedName{Name: authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName}
+			bindingNsdName = types.NamespacedName{Name: authorinoInstance.Namespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoManagerClusterRoleBindingName}
 
 			// manager cluster role binding should get service account added
 			Eventually(func(g Gomega, ctx context.Context) {
@@ -430,6 +430,173 @@ var _ = Describe("Authorino controller", func() {
 		})
 	})
 
+	// Regression coverage for
+	// https://github.com/Kuadrant/authorino-operator/issues/348: two cluster-wide
+	// Authorino instances that share the same CR name in different namespaces must
+	// each own a distinct, namespace-qualified k8s-auth ClusterRoleBinding and must
+	// not evict each other's ServiceAccount.
+	Context("Same-named cluster-wide instances across namespaces", func() {
+		const sharedName = "authorino"
+		const nsA = "authorino-ns-a"
+		const nsB = "authorino-ns-b"
+
+		newClusterWideInstance := func(name, namespace string) *api.Authorino {
+			return &api.Authorino{
+				TypeMeta: v1.TypeMeta{
+					APIVersion: api.GroupVersion.String(),
+					Kind:       "Authorino",
+				},
+				ObjectMeta: v1.ObjectMeta{
+					Name:      name,
+					Namespace: namespace,
+				},
+				Spec: api.AuthorinoSpec{
+					Image:       "example.com/authorino:latest",
+					Replicas:    pointer.Int32(1),
+					ClusterWide: true,
+					Listener:    api.Listener{Tls: api.Tls{Enabled: pointer.Bool(false)}},
+					OIDCServer:  api.OIDCServer{Tls: api.Tls{Enabled: pointer.Bool(false)}},
+				},
+			}
+		}
+
+		k8sAuthBindingName := func(namespace, name string) types.NamespacedName {
+			return types.NamespacedName{Name: namespace + "." + name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
+		}
+
+		BeforeEach(func(ctx context.Context) {
+			for _, ns := range []string{nsA, nsB} {
+				namespace := &k8score.Namespace{ObjectMeta: v1.ObjectMeta{Name: ns}}
+				if err := k8sClient.Create(ctx, namespace); err != nil && !apierrors.IsAlreadyExists(err) {
+					Expect(err).ToNot(HaveOccurred())
+				}
+			}
+		})
+
+		It("Should give each instance its own binding without evicting the other", func(ctx context.Context) {
+			instanceA := newClusterWideInstance(sharedName, nsA)
+			instanceB := newClusterWideInstance(sharedName, nsB)
+			Expect(k8sClient.Create(ctx, instanceA)).To(Succeed())
+			Expect(k8sClient.Create(ctx, instanceB)).To(Succeed())
+
+			subjectA := authorinoResources.GetSubjectForRoleBinding(authorinoResources.GetAuthorinoServiceAccount(nsA, sharedName, nil))
+			subjectB := authorinoResources.GetSubjectForRoleBinding(authorinoResources.GetAuthorinoServiceAccount(nsB, sharedName, nil))
+
+			// Each instance provisions its own namespace-qualified binding pointing
+			// at its own ServiceAccount.
+			Eventually(func(g Gomega, ctx context.Context) {
+				bindingA := &k8srbac.ClusterRoleBinding{}
+				g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsA, sharedName), bindingA)).To(Succeed())
+				g.Expect(bindingA.Subjects).To(ContainElement(subjectA))
+
+				bindingB := &k8srbac.ClusterRoleBinding{}
+				g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsB, sharedName), bindingB)).To(Succeed())
+				g.Expect(bindingB.Subjects).To(ContainElement(subjectB))
+			}).WithContext(ctx).Should(Succeed())
+
+			// Force instance A to reconcile by mutating a label, which propagates
+			// from the CR to its ClusterRoleBinding.
+			const propagatedLabel = "test.authorino.kuadrant.io/propagated"
+			Eventually(func(g Gomega, ctx context.Context) {
+				g.Expect(k8sClient.Get(ctx, namespacedName(nsA, sharedName), instanceA)).To(Succeed())
+				if instanceA.Labels == nil {
+					instanceA.Labels = map[string]string{}
+				}
+				instanceA.Labels[propagatedLabel] = "yes"
+				g.Expect(k8sClient.Update(ctx, instanceA)).To(Succeed())
+			}).WithContext(ctx).Should(Succeed())
+
+			// Wait until binding A reflects the change, confirming instance A has
+			// reconciled its permissions.
+			Eventually(func(g Gomega, ctx context.Context) {
+				bindingA := &k8srbac.ClusterRoleBinding{}
+				g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsA, sharedName), bindingA)).To(Succeed())
+				g.Expect(bindingA.Labels).To(HaveKeyWithValue(propagatedLabel, "yes"))
+			}).WithContext(ctx).Should(Succeed())
+
+			// Instance B's binding must still contain only its own ServiceAccount
+			// (no cross-instance eviction).
+			Consistently(func(g Gomega, ctx context.Context) {
+				bindingB := &k8srbac.ClusterRoleBinding{}
+				g.Expect(k8sClient.Get(ctx, k8sAuthBindingName(nsB, sharedName), bindingB)).To(Succeed())
+				g.Expect(bindingB.Subjects).To(ConsistOf(subjectB))
+			}, "5s", "1s").WithContext(ctx).Should(Succeed())
+		})
+	})
+
+	Context("Legacy ClusterRoleBinding migration", func() {
+		It("Should remove the pre-namespace-qualified ClusterRoleBinding after reconcile", func(ctx context.Context) {
+			authorinoInstance := newFullAuthorinoInstance()
+			authorinoInstance.Spec.ClusterWide = true
+
+			// Simulate a binding created by an older operator version, whose name
+			// omits the namespace, owned by this instance's ServiceAccount.
+			sa := authorinoResources.GetAuthorinoServiceAccount(testAuthorinoNamespace, authorinoInstance.Name, nil)
+			legacyName := authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName
+			legacyBinding := &k8srbac.ClusterRoleBinding{
+				ObjectMeta: v1.ObjectMeta{Name: legacyName},
+				RoleRef: k8srbac.RoleRef{
+					APIGroup: k8srbac.GroupName,
+					Kind:     "ClusterRole",
+					Name:     reconcilers.AuthorinoK8sAuthClusterRoleName,
+				},
+				Subjects: []k8srbac.Subject{authorinoResources.GetSubjectForRoleBinding(sa)},
+			}
+			Expect(k8sClient.Create(ctx, legacyBinding)).To(Succeed())
+
+			Expect(k8sClient.Create(ctx, authorinoInstance)).To(Succeed())
+
+			// The namespace-qualified binding is provisioned.
+			newBindingName := types.NamespacedName{Name: testAuthorinoNamespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
+			Eventually(func(g Gomega, ctx context.Context) {
+				g.Expect(k8sClient.Get(ctx, newBindingName, &k8srbac.ClusterRoleBinding{})).To(Succeed())
+			}).WithContext(ctx).Should(Succeed())
+
+			// The legacy binding is cleaned up.
+			Eventually(func(g Gomega, ctx context.Context) {
+				err := k8sClient.Get(ctx, types.NamespacedName{Name: legacyName}, &k8srbac.ClusterRoleBinding{})
+				g.Expect(apierrors.IsNotFound(err)).To(BeTrue())
+			}).WithContext(ctx).Should(Succeed())
+		})
+
+		It("Should not remove a legacy ClusterRoleBinding owned by an instance in another namespace", func(ctx context.Context) {
+			authorinoInstance := newFullAuthorinoInstance()
+			authorinoInstance.Spec.ClusterWide = true
+
+			// A legacy binding whose name collides with this instance's (same CR
+			// name) but which belongs to an instance in a different namespace:
+			// its subject references a ServiceAccount in that other namespace.
+			foreignSA := authorinoResources.GetAuthorinoServiceAccount("other-namespace", authorinoInstance.Name, nil)
+			legacyName := authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName
+			foreignBinding := &k8srbac.ClusterRoleBinding{
+				ObjectMeta: v1.ObjectMeta{Name: legacyName},
+				RoleRef: k8srbac.RoleRef{
+					APIGroup: k8srbac.GroupName,
+					Kind:     "ClusterRole",
+					Name:     reconcilers.AuthorinoK8sAuthClusterRoleName,
+				},
+				Subjects: []k8srbac.Subject{authorinoResources.GetSubjectForRoleBinding(foreignSA)},
+			}
+			Expect(k8sClient.Create(ctx, foreignBinding)).To(Succeed())
+			DeferCleanup(func(ctx context.Context) {
+				_ = k8sClient.Delete(ctx, foreignBinding)
+			})
+
+			Expect(k8sClient.Create(ctx, authorinoInstance)).To(Succeed())
+
+			// Once this instance's own namespace-qualified binding exists, the
+			// cleanup has run; the foreign legacy binding must still be present.
+			newBindingName := types.NamespacedName{Name: testAuthorinoNamespace + "." + authorinoInstance.Name + "-" + reconcilers.AuthorinoK8sAuthClusterRoleBindingName}
+			Eventually(func(g Gomega, ctx context.Context) {
+				g.Expect(k8sClient.Get(ctx, newBindingName, &k8srbac.ClusterRoleBinding{})).To(Succeed())
+			}).WithContext(ctx).Should(Succeed())
+
+			Consistently(func(g Gomega, ctx context.Context) {
+				g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: legacyName}, &k8srbac.ClusterRoleBinding{})).To(Succeed())
+			}, "2s", "500ms").WithContext(ctx).Should(Succeed())
+		})
+	})
+
 	Context("Authorino with part TLS enabled", func() {
 		var authorinoInstance *api.Authorino
 

pkg/reconcilers/authorino_reconciler.go

@@ -12,7 +12,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	k8smeta "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -143,9 +142,70 @@ func (r *AuthorinoReconciler) ReconcileAuthorinoPermissions(ctx context.Context,
 		return err
 	}
 
+	// Best-effort migration: remove ClusterRoleBindings created with the legacy
+	// name scheme (<crName>-<suffix>). Those names omit the namespace, so
+	// instances sharing a CR name in different namespaces collided on a single
+	// cluster-scoped binding. This runs after the namespace-qualified bindings
+	// are reconciled, so this instance's own permissions are never dropped
+	// mid-reconcile. Note that when several instances shared a single legacy
+	// binding, deleting it here can briefly remove another instance's permissions
+	// until that instance reconciles and provisions its own namespace-qualified
+	// binding.
+	r.cleanupLegacyClusterRoleBindings(ctx, authorinoInstance)
+
 	return nil
 }
 
+// cleanupLegacyClusterRoleBindings deletes the pre-namespace-qualified
+// ClusterRoleBindings (named <crName>-<suffix>) for this instance, if present.
+// It is best-effort: a missing binding is not an error, and other failures are
+// logged rather than surfaced so they don't block reconciliation.
+//
+// The legacy name is <crName>-<suffix>, and crName is fully controlled by
+// whoever creates the Authorino CR. Deleting purely by name would let a tenant
+// name a CR so that the operator deletes a legacy binding owned by an instance
+// in another namespace. To avoid that, each binding is read and only deleted
+// once it is confirmed to belong to this instance: it must reference this
+// instance's ServiceAccount as a subject. Bindings that cannot be confirmed are
+// left in place.
+func (r *AuthorinoReconciler) cleanupLegacyClusterRoleBindings(ctx context.Context, authorinoInstance *api.Authorino) {
+	logger, _ := logr.FromContext(ctx)
+
+	sa := authorinoResources.GetAuthorinoServiceAccount(authorinoInstance.Namespace, authorinoInstance.Name, authorinoInstance.Labels)
+	ownSubject := authorinoResources.GetSubjectForRoleBinding(sa)
+
+	for _, suffix := range []string{AuthorinoManagerClusterRoleBindingName, AuthorinoK8sAuthClusterRoleBindingName} {
+		legacyName := fmt.Sprintf("%s-%s", authorinoInstance.Name, suffix)
+
+		binding := &k8srbac.ClusterRoleBinding{}
+		if err := r.Client.Get(ctx, client.ObjectKey{Name: legacyName}, binding); err != nil {
+			if !errors.IsNotFound(err) {
+				logger.Error(err, "failed to get legacy ClusterRoleBinding", "name", legacyName)
+			}
+			continue
+		}
+
+		if !authorinoResources.SubjectIncluded(binding.Subjects, ownSubject) {
+			// The binding does not reference this instance's ServiceAccount, so
+			// it was created for a different Authorino instance (e.g. one sharing
+			// the CR name in another namespace). Leave it untouched.
+			logger.Info("skipping legacy ClusterRoleBinding not owned by this instance", "name", legacyName)
+			continue
+		}
+
+		// Delete only the exact object that was validated: the resourceVersion and
+		// UID preconditions make the delete fail rather than remove a binding that
+		// changed between the Get and the Delete.
+		preconditions := client.Preconditions{
+			UID:             &binding.UID,
+			ResourceVersion: &binding.ResourceVersion,
+		}
+		if err := r.Client.Delete(ctx, binding, preconditions); err != nil && !errors.IsNotFound(err) {
+			logger.Error(err, "failed to delete legacy ClusterRoleBinding", "name", legacyName)
+		}
+	}
+}
+
 func (r *AuthorinoReconciler) reconcileManagerClusterRoleBinding(ctx context.Context, authorinoInstance *api.Authorino) error {
 	clusterRoleKey := client.ObjectKey{Name: AuthorinoManagerClusterRoleName}
 	if err := r.checkClusterRoleExists(ctx, clusterRoleKey, authorinoInstance); err != nil {
@@ -156,13 +216,13 @@ func (r *AuthorinoReconciler) reconcileManagerClusterRoleBinding(ctx context.Con
 
 	// if cluster scoped, ensure service account is in the binding
 	if authorinoInstance.Spec.ClusterWide {
-		binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels)
+		binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Namespace, authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels)
 		return r.reconcileClusterRoleBinding(ctx, binding, authorinoInstance)
 	}
 
 	// local namespace scope
 	// if switching from cluster-wide to namespaced, delete the ClusterRoleBinding
-	binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels)
+	binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Namespace, authorinoInstance.Name, AuthorinoManagerClusterRoleBindingName, AuthorinoManagerClusterRoleName, sa, authorinoInstance.Labels)
 	TagObjectToDelete(binding)
 	r.reconcileClusterRoleBinding(ctx, binding, authorinoInstance)
 
@@ -194,7 +254,7 @@ func (r *AuthorinoReconciler) reconcileManagerAuthClusterRoleBinding(ctx context
 
 	sa := authorinoResources.GetAuthorinoServiceAccount(authorinoInstance.Namespace, authorinoInstance.Name, authorinoInstance.Labels)
 
-	binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Name, AuthorinoK8sAuthClusterRoleBindingName, AuthorinoK8sAuthClusterRoleName, sa, authorinoInstance.Labels)
+	binding := authorinoResources.GetAuthorinoClusterRoleBinding(authorinoInstance.Namespace, authorinoInstance.Name, AuthorinoK8sAuthClusterRoleBindingName, AuthorinoK8sAuthClusterRoleName, sa, authorinoInstance.Labels)
 	return r.reconcileClusterRoleBinding(ctx, binding, authorinoInstance)
 }
 
@@ -517,31 +577,3 @@ func (r *AuthorinoReconciler) ReconcileAuthorinoServiceAccount(ctx context.Conte
 
 	return nil
 }
-
-// remove SA from list of subjects of the clusterrolebinding
-func (r *AuthorinoReconciler) UnboundAuthorinoServiceAccountFromClusterRole(ctx context.Context, roleBindingName string, sa *k8score.ServiceAccount) {
-	// TODO: should return error for error handling
-	logger, _ := logr.FromContext(ctx)
-	roleBinding := &k8srbac.ClusterRoleBinding{}
-	if err := r.Client.Get(ctx, types.NamespacedName{Name: roleBindingName}, roleBinding); err == nil {
-		staleSubject := authorinoResources.GetSubjectForRoleBinding(sa)
-		var subjects []k8srbac.Subject
-		for _, subject := range roleBinding.Subjects {
-			if subject.Kind != staleSubject.Kind || subject.Name != staleSubject.Name || subject.Namespace != staleSubject.Namespace {
-				subjects = append(subjects, subject)
-			}
-		}
-
-		if len(subjects) == 0 {
-			if err = r.DeleteResource(ctx, roleBinding); err != nil {
-				logger.Error(err, "failed to delete authorino role binding", "roleBinding", roleBinding, "subject", staleSubject)
-			}
-		} else {
-			// FIXME: This is subject to race condition. The list of subjects may be outdated under concurrent updates
-			roleBinding.Subjects = subjects
-			if err = r.Client.Update(ctx, roleBinding); err != nil {
-				logger.Error(err, "failed to cleanup subject from authorino role binding", "roleBinding", roleBinding, "subject", staleSubject)
-			}
-		}
-	}
-}