Reworked RBAC machinery for APIServiceExports (#451)

* Reworked RBAC machinery for APIServiceExports

Added new APIServiceExportRBAC controller for managing APIServiceExport RBACs.

Additionally:
* kube-binder ClusterRoleBinding creation/update was moved to /bind handler
* Redundant boundschemas and boundschemas/status rules in
  `kube-binder-<ClusterBinding.Namespace>` ClusterRole were removed, because
  they are already present in `kube-binder` ClusterRole.
* e2e: adjusted tests for the new APIServiceExport RBAC machinery
* cleanup manager
* serviceexportrbac reconciler: don't set Controller=True in owner refs
* serviceexportrbac reconciler unit test: add missing t.Helper()
* code cleaning
* added docs into developers/architecture.md

On-behalf-of: @SAP robert.vasek@sap.com
Signed-off-by: Robert Vasek <robert.vasek@clyso.com>

Author: gman0

backend/controllers/clusterbinding/clusterbinding_controller.go

@@ -21,13 +21,9 @@ import (
 	"fmt"
 	"reflect"
 
-	v1 "k8s.io/api/core/v1"
-	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/cluster"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -64,84 +60,6 @@ func NewClusterBindingReconciler(
 		manager: mgr,
 		opts:    opts,
 		scope:   scope,
-		reconciler: reconciler{
-			scope: scope,
-			listServiceExports: func(ctx context.Context, cache cache.Cache, ns string) ([]*kubebindv1alpha2.APIServiceExport, error) {
-				var list kubebindv1alpha2.APIServiceExportList
-				if err := cache.List(ctx, &list, client.InNamespace(ns)); err != nil {
-					return nil, err
-				}
-				var exports []*kubebindv1alpha2.APIServiceExport
-				for i := range list.Items {
-					exports = append(exports, &list.Items[i])
-				}
-				return exports, nil
-			},
-			getBoundSchema: func(ctx context.Context, cache cache.Cache, namespace, name string) (*kubebindv1alpha2.BoundSchema, error) {
-				result := &kubebindv1alpha2.BoundSchema{}
-				err := cache.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, result)
-				if err != nil {
-					return nil, fmt.Errorf("failed to get BoundSchema %q: %w", name, err)
-				}
-				return result, nil
-			},
-			getClusterRole: func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRole, error) {
-				var role rbacv1.ClusterRole
-				key := types.NamespacedName{Name: name}
-				if err := cache.Get(ctx, key, &role); err != nil {
-					return nil, err
-				}
-				return &role, nil
-			},
-			createClusterRole: func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRole) error {
-				return client.Create(ctx, binding)
-			},
-			updateClusterRole: func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRole) error {
-				return client.Update(ctx, binding)
-			},
-			getClusterRoleBinding: func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRoleBinding, error) {
-				var binding rbacv1.ClusterRoleBinding
-				key := types.NamespacedName{Name: name}
-				if err := cache.Get(ctx, key, &binding); err != nil {
-					return nil, err
-				}
-				return &binding, nil
-			},
-			createClusterRoleBinding: func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRoleBinding) error {
-				return client.Create(ctx, binding)
-			},
-			updateClusterRoleBinding: func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRoleBinding) error {
-				return client.Update(ctx, binding)
-			},
-			deleteClusterRoleBinding: func(ctx context.Context, client client.Client, name string) error {
-				binding := &rbacv1.ClusterRoleBinding{
-					ObjectMeta: metav1.ObjectMeta{Name: name},
-				}
-				return client.Delete(ctx, binding)
-			},
-			getNamespace: func(ctx context.Context, cache cache.Cache, name string) (*v1.Namespace, error) {
-				var ns v1.Namespace
-				key := types.NamespacedName{Name: name}
-				if err := cache.Get(ctx, key, &ns); err != nil {
-					return nil, err
-				}
-				return &ns, nil
-			},
-			createRoleBinding: func(ctx context.Context, client client.Client, binding *rbacv1.RoleBinding) error {
-				return client.Create(ctx, binding)
-			},
-			updateRoleBinding: func(ctx context.Context, client client.Client, binding *rbacv1.RoleBinding) error {
-				return client.Update(ctx, binding)
-			},
-			getRoleBinding: func(ctx context.Context, cache cache.Cache, ns, name string) (*rbacv1.RoleBinding, error) {
-				var binding rbacv1.RoleBinding
-				key := types.NamespacedName{Namespace: ns, Name: name}
-				if err := cache.Get(ctx, key, &binding); err != nil {
-					return nil, err
-				}
-				return &binding, nil
-			},
-		},
 	}
 
 	return r, nil
@@ -153,10 +71,6 @@ func NewClusterBindingReconciler(
 //+kubebuilder:rbac:groups=kube-bind.io,resources=apiserviceexports,verbs=get;list;watch
 //+kubebuilder:rbac:groups=kube-bind.io,resources=collections,verbs=get;list;watch
 //+kubebuilder:rbac:groups=kube-bind.io,resources=apiserviceexporttemplates,verbs=get;list;watch
-//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles,verbs=get;list;watch;create;update;patch;delete
-//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=get;list;watch;create;update;patch;delete
-//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
-//+kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
@@ -210,9 +124,6 @@ func (r *ClusterBindingReconciler) Reconcile(ctx context.Context, req mcreconcil
 func (r *ClusterBindingReconciler) SetupWithManager(mgr mcmanager.Manager) error {
 	return mcbuilder.ControllerManagedBy(mgr).
 		For(&kubebindv1alpha2.ClusterBinding{}).
-		Owns(&rbacv1.ClusterRole{}).
-		Owns(&rbacv1.ClusterRoleBinding{}).
-		Owns(&rbacv1.RoleBinding{}).
 		Watches(
 			&kubebindv1alpha2.APIServiceExport{},
 			mapAPIResourceSchema,

backend/controllers/clusterbinding/clusterbinding_reconcile.go

@@ -18,63 +18,25 @@ package clusterbinding
 
 import (
 	"context"
-	"fmt"
-	"maps"
-	"reflect"
-	"slices"
 	"time"
 
-	corev1 "k8s.io/api/core/v1"
-	rbacv1 "k8s.io/api/rbac/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
-	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	kuberesources "github.com/kube-bind/kube-bind/backend/kubernetes/resources"
 	kubebindv1alpha2 "github.com/kube-bind/kube-bind/sdk/apis/kubebind/v1alpha2"
 	conditionsapi "github.com/kube-bind/kube-bind/sdk/apis/third_party/conditions/apis/conditions/v1alpha1"
 	"github.com/kube-bind/kube-bind/sdk/apis/third_party/conditions/util/conditions"
 )
 
-type reconciler struct {
-	scope kubebindv1alpha2.InformerScope
-
-	listServiceExports func(ctx context.Context, cache cache.Cache, ns string) ([]*kubebindv1alpha2.APIServiceExport, error)
-	getBoundSchema     func(ctx context.Context, cache cache.Cache, namespace, name string) (*kubebindv1alpha2.BoundSchema, error)
-	getClusterRole     func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRole, error)
-	createClusterRole  func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRole) error
-	updateClusterRole  func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRole) error
-
-	getClusterRoleBinding    func(ctx context.Context, cache cache.Cache, name string) (*rbacv1.ClusterRoleBinding, error)
-	createClusterRoleBinding func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRoleBinding) error
-	updateClusterRoleBinding func(ctx context.Context, client client.Client, binding *rbacv1.ClusterRoleBinding) error
-	deleteClusterRoleBinding func(ctx context.Context, client client.Client, name string) error
-
-	getRoleBinding    func(ctx context.Context, cache cache.Cache, ns, name string) (*rbacv1.RoleBinding, error)
-	createRoleBinding func(ctx context.Context, client client.Client, binding *rbacv1.RoleBinding) error
-	updateRoleBinding func(ctx context.Context, client client.Client, binding *rbacv1.RoleBinding) error
-
-	getNamespace func(ctx context.Context, cache cache.Cache, name string) (*corev1.Namespace, error)
-}
+type reconciler struct{}
 
 func (r *reconciler) reconcile(ctx context.Context, client client.Client, cache cache.Cache, clusterBinding *kubebindv1alpha2.ClusterBinding) error {
 	var errs []error
 
 	if err := r.ensureClusterBindingConditions(ctx, clusterBinding); err != nil {
 		errs = append(errs, err)
 	}
-	if err := r.ensureRBACRoleBinding(ctx, client, cache, clusterBinding); err != nil {
-		errs = append(errs, err)
-	}
-	if err := r.ensureRBACClusterRole(ctx, client, cache, clusterBinding); err != nil {
-		errs = append(errs, err)
-	}
-	if err := r.ensureRBACClusterRoleBinding(ctx, client, cache, clusterBinding); err != nil {
-		errs = append(errs, err)
-	}
 
 	conditions.SetSummary(clusterBinding)
 
@@ -120,187 +82,3 @@ func (r *reconciler) ensureClusterBindingConditions(_ context.Context, clusterBi
 
 	return nil
 }
-
-func (r *reconciler) ensureRBACClusterRole(ctx context.Context, client client.Client, cache cache.Cache, clusterBinding *kubebindv1alpha2.ClusterBinding) error {
-	name := "kube-binder-" + clusterBinding.Namespace
-	role, err := r.getClusterRole(ctx, cache, name)
-	if err != nil && !errors.IsNotFound(err) {
-		return fmt.Errorf("failed to get ClusterRole %s: %w", name, err)
-	}
-
-	ns, err := r.getNamespace(ctx, cache, clusterBinding.Namespace)
-	if err != nil {
-		return fmt.Errorf("failed to get Namespace %s: %w", clusterBinding.Namespace, err)
-	}
-
-	exports, err := r.listServiceExports(ctx, cache, clusterBinding.Namespace)
-	if err != nil {
-		return fmt.Errorf("failed to list APIServiceExports: %w", err)
-	}
-	expected := &rbacv1.ClusterRole{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: name,
-			OwnerReferences: []metav1.OwnerReference{
-				{
-					APIVersion: "v1",
-					Kind:       "Namespace",
-					Name:       clusterBinding.Namespace,
-					Controller: ptr.To(true),
-					UID:        ns.UID,
-				},
-			},
-		},
-		Rules: []rbacv1.PolicyRule{
-			// Always need to be able to get/list/watch the BoundSchemas
-			// to be able to figure out what to bind.
-			{
-				APIGroups: []string{kubebindv1alpha2.GroupName},
-				Resources: []string{"boundschemas"},
-				Verbs:     []string{"get", "list", "watch"},
-			},
-			{
-				APIGroups: []string{kubebindv1alpha2.GroupName},
-				Resources: []string{"boundschemas/status"},
-				Verbs:     []string{"get", "update", "patch"},
-			},
-		}}
-	for _, export := range exports {
-		// Collect unique GroupResources and sort for stable rule ordering.
-		grSet := map[string]kubebindv1alpha2.GroupResource{}
-		for _, res := range export.Spec.Resources {
-			key := res.ResourceGroupName()
-			grSet[key] = kubebindv1alpha2.GroupResource{Group: res.Group, Resource: res.Resource}
-		}
-		keys := slices.Collect(maps.Keys(grSet))
-		slices.Sort(keys)
-		for _, k := range keys {
-			// k is already normalized (e.g., "pods.core" for empty group).
-			schema, err := r.getBoundSchema(ctx, cache, clusterBinding.Namespace, k)
-			if err != nil {
-				return fmt.Errorf("failed to get BoundSchema %q: %w", k, err)
-			}
-			expected.Rules = append(expected.Rules,
-				rbacv1.PolicyRule{
-					APIGroups: []string{schema.Spec.Group},
-					Resources: []string{schema.Spec.Names.Plural},
-					Verbs:     []string{"get", "list", "watch", "create", "update", "patch", "delete"},
-				},
-			)
-		}
-	}
-
-	if role == nil {
-		if err := r.createClusterRole(ctx, client, expected); err != nil {
-			return fmt.Errorf("failed to create ClusterRole %s: %w", expected.Name, err)
-		}
-	} else if !reflect.DeepEqual(role.Rules, expected.Rules) {
-		role = role.DeepCopy()
-		role.Rules = expected.Rules
-		if err := r.updateClusterRole(ctx, client, role); err != nil {
-			return fmt.Errorf("failed to create ClusterRole %s: %w", role.Name, err)
-		}
-	}
-
-	return nil
-}
-
-func (r *reconciler) ensureRBACClusterRoleBinding(ctx context.Context, client client.Client, cache cache.Cache, clusterBinding *kubebindv1alpha2.ClusterBinding) error {
-	name := "kube-binder-" + clusterBinding.Namespace
-	binding, err := r.getClusterRoleBinding(ctx, cache, name)
-	if err != nil && !errors.IsNotFound(err) {
-		return fmt.Errorf("failed to get ClusterRoleBinding %s: %w", name, err)
-	}
-	if r.scope != kubebindv1alpha2.ClusterScope {
-		if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) {
-			return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err)
-		}
-	}
-
-	ns, err := r.getNamespace(ctx, cache, clusterBinding.Namespace)
-	if err != nil {
-		return fmt.Errorf("failed to get Namespace %s: %w", clusterBinding.Namespace, err)
-	}
-
-	expected := &rbacv1.ClusterRoleBinding{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: name,
-			OwnerReferences: []metav1.OwnerReference{
-				{
-					APIVersion: "v1",
-					Kind:       "Namespace",
-					Name:       clusterBinding.Namespace,
-					Controller: ptr.To(true),
-					UID:        ns.UID,
-				},
-			},
-		},
-		Subjects: []rbacv1.Subject{
-			{
-				Kind:      "ServiceAccount",
-				Namespace: clusterBinding.Namespace,
-				Name:      kuberesources.ServiceAccountName,
-			},
-		},
-		RoleRef: rbacv1.RoleRef{
-			Kind:     "ClusterRole",
-			Name:     name,
-			APIGroup: "rbac.authorization.k8s.io",
-		},
-	}
-
-	if binding == nil {
-		if err := r.createClusterRoleBinding(ctx, client, expected); err != nil {
-			return fmt.Errorf("failed to create ClusterRoleBinding %s: %w", expected.Name, err)
-		}
-	} else if !reflect.DeepEqual(binding.Subjects, expected.Subjects) {
-		binding = binding.DeepCopy()
-		binding.Subjects = expected.Subjects
-		// roleRef is immutable
-		if err := r.updateClusterRoleBinding(ctx, client, binding); err != nil {
-			return fmt.Errorf("failed to create ClusterRoleBinding %s: %w", expected.Namespace, err)
-		}
-	}
-
-	return nil
-}
-
-func (r *reconciler) ensureRBACRoleBinding(ctx context.Context, client client.Client, cache cache.Cache, clusterBinding *kubebindv1alpha2.ClusterBinding) error {
-	binding, err := r.getRoleBinding(ctx, cache, clusterBinding.Namespace, "kube-binder")
-	if err != nil && !errors.IsNotFound(err) {
-		return fmt.Errorf("failed to get RoleBinding \"kube-binder\": %w", err)
-	}
-
-	expected := &rbacv1.RoleBinding{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: kuberesources.ServiceAccountName,
-		},
-		Subjects: []rbacv1.Subject{
-			{
-				Kind:      "ServiceAccount",
-				Name:      kuberesources.ServiceAccountName,
-				Namespace: clusterBinding.Namespace,
-			},
-		},
-		RoleRef: rbacv1.RoleRef{
-			APIGroup: "rbac.authorization.k8s.io",
-			Kind:     "ClusterRole",
-			Name:     "kube-binder",
-		},
-	}
-
-	if binding == nil {
-		expected.Namespace = clusterBinding.Namespace
-		if err := r.createRoleBinding(ctx, client, expected); err != nil {
-			return fmt.Errorf("failed to create RoleBinding %s: %w", expected.Name, err)
-		}
-	} else if !reflect.DeepEqual(binding.Subjects, expected.Subjects) {
-		binding = binding.DeepCopy()
-		binding.Subjects = expected.Subjects
-		// roleRef is immutable
-		if err := r.updateRoleBinding(ctx, client, binding); err != nil {
-			return fmt.Errorf("failed to create RoleBinding %s: %w", expected.Namespace, err)
-		}
-	}
-
-	return nil
-}