From 4e871ec4fac28f8df5424c489195b05ea614343f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20St=C3=A4bler?= Date: Wed, 29 Apr 2026 09:36:58 +0200 Subject: [PATCH] Fix RoleBinding ownership conflict for multi-function namespaces 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. --- internal/controller/function_controller.go | 5 +- internal/controller/function_rbac.go | 79 +++++++----- internal/controller/function_rbac_test.go | 132 +++++++++++++++++++++ 3 files changed, 182 insertions(+), 34 deletions(-) create mode 100644 internal/controller/function_rbac_test.go diff --git a/internal/controller/function_controller.go b/internal/controller/function_controller.go index deb0860..475b3ba 100644 --- 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 index 6c7fb12..21ab01b 100644 --- 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 new file mode 100644 index 0000000..d482dc2 --- /dev/null +++ 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)) + }) + }) +})