Fix RoleBinding ownership conflict for multi-function namespaces

creydr · creydr · commit 4e871ec4fac2 · 2026-04-29T09:43:15.000+02:00
The shared deploy-function RoleBinding previously set a single Function
as the controller owner, overwriting any prior owner. Deleting that
Function would garbage-collect the RoleBinding, breaking other Functions
in the same namespace.

Use controllerutil.SetOwnerReference to append each Function as a
non-controller owner so the RoleBinding is only cleaned up when all
Functions in the namespace are deleted.
diff --git a/internal/controller/function_controller.go b/internal/controller/function_controller.go
@@ -48,8 +48,9 @@ import (
 )
 
 const (
-	deployFunctionRoleName = "func-operator-deploy-function"
-	controllerConfigName   = "func-operator-controller-config"
+	deployFunctionRoleName        = "func-operator-deploy-function"
+	deployFunctionRoleBindingName = "deploy-function-default"
+	controllerConfigName          = "func-operator-controller-config"
 
 	funcAnnotationPrefix       = "functions.knative.dev/"
 	funcAnnotationLastDeployed = funcAnnotationPrefix + "last-deployed"
diff --git a/internal/controller/function_rbac.go b/internal/controller/function_rbac.go
@@ -26,7 +26,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -109,37 +109,34 @@ func (r *FunctionReconciler) ensureDeployFunctionRole(ctx context.Context, names
 func (r *FunctionReconciler) ensureDeployFunctionRoleBinding(ctx context.Context, function *v1alpha1.Function) error {
 	logger := log.FromContext(ctx)
 
-	expectedRoleBinding := &rbacv1.RoleBinding{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "deploy-function-default",
-			Namespace: function.Namespace,
-			OwnerReferences: []metav1.OwnerReference{
-				{
-					APIVersion: v1alpha1.GroupVersion.String(),
-					Kind:       "Function",
-					Name:       function.Name,
-					UID:        function.UID,
-					Controller: ptr.To(true),
-				},
-			},
-		},
-		Subjects: []rbacv1.Subject{{
-			Kind:      "ServiceAccount",
-			Name:      "default",
-			Namespace: function.Namespace,
-		}},
-		RoleRef: rbacv1.RoleRef{
-			APIGroup: "rbac.authorization.k8s.io",
-			Kind:     "Role",
-			Name:     deployFunctionRoleName,
-		},
+	expectedSubjects := []rbacv1.Subject{{
+		Kind:      "ServiceAccount",
+		Name:      "default",
+		Namespace: function.Namespace,
+	}}
+
+	expectedRoleRef := rbacv1.RoleRef{
+		APIGroup: "rbac.authorization.k8s.io",
+		Kind:     "Role",
+		Name:     deployFunctionRoleName,
 	}
 
 	foundRoleBinding := &rbacv1.RoleBinding{}
-	err := r.Get(ctx, types.NamespacedName{Name: expectedRoleBinding.Name, Namespace: expectedRoleBinding.Namespace}, foundRoleBinding)
+	err := r.Get(ctx, types.NamespacedName{Name: deployFunctionRoleBindingName, Namespace: function.Namespace}, foundRoleBinding)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			if err := r.Create(ctx, expectedRoleBinding); err != nil {
+			rb := &rbacv1.RoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      deployFunctionRoleBindingName,
+					Namespace: function.Namespace,
+				},
+				Subjects: expectedSubjects,
+				RoleRef:  expectedRoleRef,
+			}
+			if err := controllerutil.SetOwnerReference(function, rb, r.Scheme); err != nil {
+				return fmt.Errorf("failed to set owner reference: %w", err)
+			}
+			if err := r.Create(ctx, rb); err != nil {
 				return fmt.Errorf("failed to create role binding: %w", err)
 			}
 			logger.Info("Created deploy-function role binding")
@@ -148,12 +145,30 @@ func (r *FunctionReconciler) ensureDeployFunctionRoleBinding(ctx context.Context
 		return fmt.Errorf("failed to get role binding: %w", err)
 	}
 
-	// Update if needed
-	if !equality.Semantic.DeepDerivative(expectedRoleBinding, foundRoleBinding) {
-		foundRoleBinding.Subjects = expectedRoleBinding.Subjects
-		foundRoleBinding.RoleRef = expectedRoleBinding.RoleRef
-		foundRoleBinding.OwnerReferences = expectedRoleBinding.OwnerReferences
+	needsUpdate := false
+
+	hasRef, err := controllerutil.HasOwnerReference(foundRoleBinding.OwnerReferences, function, r.Scheme)
+	if err != nil {
+		return fmt.Errorf("failed to check owner reference: %w", err)
+	}
+	if !hasRef {
+		if err := controllerutil.SetOwnerReference(function, foundRoleBinding, r.Scheme); err != nil {
+			return fmt.Errorf("failed to set owner reference: %w", err)
+		}
+		needsUpdate = true
+	}
+
+	if !equality.Semantic.DeepDerivative(expectedSubjects, foundRoleBinding.Subjects) {
+		foundRoleBinding.Subjects = expectedSubjects
+		needsUpdate = true
+	}
+
+	if !equality.Semantic.DeepDerivative(expectedRoleRef, foundRoleBinding.RoleRef) {
+		foundRoleBinding.RoleRef = expectedRoleRef
+		needsUpdate = true
+	}
 
+	if needsUpdate {
 		if err := r.Update(ctx, foundRoleBinding); err != nil {
 			return fmt.Errorf("failed to update role binding: %w", err)
 		}
diff --git a/internal/controller/function_rbac_test.go b/internal/controller/function_rbac_test.go
@@ -0,0 +1,132 @@
+package controller
+
+import (
+	functionsdevv1alpha1 "github.com/functions-dev/func-operator/api/v1alpha1"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/rand"
+	"k8s.io/client-go/tools/events"
+)
+
+var _ = Describe("Function RBAC", func() {
+	Context("ensureDeployFunctionRoleBinding", func() {
+		var reconciler *FunctionReconciler
+		var testNamespace string
+
+		BeforeEach(func() {
+			testNamespace = "rbac-test-" + rand.String(6)
+			ns := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}}
+			Expect(k8sClient.Create(ctx, ns)).To(Succeed())
+
+			reconciler = &FunctionReconciler{
+				Client:   k8sClient,
+				Scheme:   k8sClient.Scheme(),
+				Recorder: &events.FakeRecorder{},
+			}
+		})
+
+		It("should add owner references for multiple Functions without overwriting", func() {
+			functionA := &functionsdevv1alpha1.Function{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "function-a",
+					Namespace: testNamespace,
+				},
+				Spec: functionsdevv1alpha1.FunctionSpec{
+					Repository: functionsdevv1alpha1.FunctionSpecRepository{
+						URL: "https://github.com/foo/bar",
+					},
+				},
+			}
+			Expect(k8sClient.Create(ctx, functionA)).To(Succeed())
+
+			functionB := &functionsdevv1alpha1.Function{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "function-b",
+					Namespace: testNamespace,
+				},
+				Spec: functionsdevv1alpha1.FunctionSpec{
+					Repository: functionsdevv1alpha1.FunctionSpecRepository{
+						URL: "https://github.com/foo/baz",
+					},
+				},
+			}
+			Expect(k8sClient.Create(ctx, functionB)).To(Succeed())
+
+			By("Reconciling the RoleBinding for Function A")
+			Expect(reconciler.ensureDeployFunctionRoleBinding(ctx, functionA)).To(Succeed())
+
+			By("Verifying RoleBinding was created with Function A as owner")
+			rb := &rbacv1.RoleBinding{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{
+				Name:      deployFunctionRoleBindingName,
+				Namespace: testNamespace,
+			}, rb)).To(Succeed())
+			Expect(rb.OwnerReferences).To(HaveLen(1))
+			Expect(rb.OwnerReferences[0].Name).To(Equal("function-a"))
+
+			By("Reconciling the RoleBinding for Function B")
+			Expect(reconciler.ensureDeployFunctionRoleBinding(ctx, functionB)).To(Succeed())
+
+			By("Verifying RoleBinding now has both Functions as owners")
+			Expect(k8sClient.Get(ctx, types.NamespacedName{
+				Name:      deployFunctionRoleBindingName,
+				Namespace: testNamespace,
+			}, rb)).To(Succeed())
+			Expect(rb.OwnerReferences).To(HaveLen(2))
+
+			ownerNames := []string{rb.OwnerReferences[0].Name, rb.OwnerReferences[1].Name}
+			Expect(ownerNames).To(ConsistOf("function-a", "function-b"))
+
+			By("Verifying no owner is marked as controller")
+			for _, ref := range rb.OwnerReferences {
+				if ref.Controller != nil {
+					Expect(*ref.Controller).To(BeFalse())
+				}
+			}
+
+			By("Reconciling Function A again should be idempotent")
+			Expect(reconciler.ensureDeployFunctionRoleBinding(ctx, functionA)).To(Succeed())
+
+			Expect(k8sClient.Get(ctx, types.NamespacedName{
+				Name:      deployFunctionRoleBindingName,
+				Namespace: testNamespace,
+			}, rb)).To(Succeed())
+			Expect(rb.OwnerReferences).To(HaveLen(2))
+		})
+
+		It("should set the expected Subjects and RoleRef", func() {
+			function := &functionsdevv1alpha1.Function{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "function-rbac-check",
+					Namespace: testNamespace,
+				},
+				Spec: functionsdevv1alpha1.FunctionSpec{
+					Repository: functionsdevv1alpha1.FunctionSpecRepository{
+						URL: "https://github.com/foo/bar",
+					},
+				},
+			}
+			Expect(k8sClient.Create(ctx, function)).To(Succeed())
+
+			Expect(reconciler.ensureDeployFunctionRoleBinding(ctx, function)).To(Succeed())
+
+			rb := &rbacv1.RoleBinding{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{
+				Name:      deployFunctionRoleBindingName,
+				Namespace: testNamespace,
+			}, rb)).To(Succeed())
+
+			Expect(rb.Subjects).To(HaveLen(1))
+			Expect(rb.Subjects[0].Kind).To(Equal("ServiceAccount"))
+			Expect(rb.Subjects[0].Name).To(Equal("default"))
+
+			Expect(rb.RoleRef.APIGroup).To(Equal("rbac.authorization.k8s.io"))
+			Expect(rb.RoleRef.Kind).To(Equal("Role"))
+			Expect(rb.RoleRef.Name).To(Equal(deployFunctionRoleName))
+		})
+	})
+})
