v2: correct and test controller RBAC

Prior to this commit the declared permissions for the RedpandaReconciler had
become out of date. This went unnoticed due to tests utilizing admin
permissions or the inflated permissions required for executing `rpk debug
bundle`.

This commit corrects the permission declaration of the RedpandaReconciler,
updates its tests to use the ClusterRole and Role generated by controller-gen,
and adds a test to statically assert the correctness of the permissions.

Author: chrisseto

operator/config/rbac/bases/operator/role.yaml

@@ -155,6 +155,7 @@ rules:
   - clusterroles
   verbs:
   - create
+  - delete
   - get
   - list
   - patch
@@ -215,6 +216,8 @@ rules:
   resources:
   - configmaps
   - pods
+  - rolebindings
+  - roles
   - secrets
   - serviceaccounts
   - services
@@ -394,6 +397,7 @@ rules:
 - apiGroups:
   - monitoring.coreos.com
   resources:
+  - podmonitors
   - servicemonitors
   verbs:
   - create

operator/internal/controller/redpanda/redpanda_controller.go

@@ -104,22 +104,20 @@ type RedpandaReconciler struct {
 
 // any resource that Redpanda helm creates and flux controller needs to reconcile them
 // +kubebuilder:rbac:groups="",namespace=default,resources=pods,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,namespace=default,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,namespace=default,resources=roles,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,namespace=default,resources=roles;rolebindings,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=batch,namespace=default,resources=jobs,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=core,namespace=default,resources=secrets,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=core,namespace=default,resources=services,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=core,namespace=default,resources=serviceaccounts,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=core,namespace=default,resources=configmaps;roles;rolebindings;secrets;services;serviceaccounts,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=apps,namespace=default,resources=statefulsets,verbs=get;list;watch;create;update;patch;delete;
 // +kubebuilder:rbac:groups=policy,namespace=default,resources=poddisruptionbudgets,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=apps,namespace=default,resources=deployments,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=cert-manager.io,namespace=default,resources=certificates,verbs=get;create;update;patch;delete;list;watch
 // +kubebuilder:rbac:groups=cert-manager.io,namespace=default,resources=issuers,verbs=get;create;update;patch;delete;list;watch
-// +kubebuilder:rbac:groups="monitoring.coreos.com",namespace=default,resources=servicemonitors,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=networking.k8s.io,namespace=default,resources=ingresses,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups="monitoring.coreos.com",namespace=default,resources=podmonitors;servicemonitors,verbs=get;list;watch;create;update;patch;delete
 
 // Console chart
 // +kubebuilder:rbac:groups=autoscaling,namespace=default,resources=horizontalpodautoscalers,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=networking.k8s.io,namespace=default,resources=ingresses,verbs=get;list;watch;create;update;patch;delete
 
 // redpanda resources
 // +kubebuilder:rbac:groups=cluster.redpanda.com,namespace=default,resources=redpandas,verbs=get;list;watch;create;update;patch;delete

operator/internal/controller/redpanda/redpanda_controller_test.go

@@ -11,11 +11,13 @@ package redpanda_test
 
 import (
 	"context"
+	_ "embed"
 	"encoding/json"
 	"fmt"
 	"math/rand"
 	"sort"
 	"strings"
+	"slices"
 	"testing"
 	"time"
 
@@ -36,6 +38,7 @@ import (
 	"github.com/stretchr/testify/suite"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -47,6 +50,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+// operatorRBAC is the ClusterRole and Role generated via controller-gen and
+// goembeded so it can be used for tests.
+//
+//go:embed role.yaml
+var operatorRBAC []byte
+
 // NB: This test setup is largely incompatible with webhooks. Though we might
 // be able to figure something freaky out.
 func TestRedpandaController(t *testing.T) {
@@ -254,7 +263,7 @@ func (s *RedpandaControllerSuite) TestManagedDecommission() {
 		"operator.redpanda.com/managed-decommission": "2999-12-31T00:00:00Z",
 	}
 
-	s.applyAndWaitFor(rp, func(o client.Object) bool {
+	s.applyAndWaitFor(func(o client.Object) bool {
 		rp := o.(*redpandav1alpha2.Redpanda)
 
 		for _, cond := range rp.Status.Conditions {
@@ -263,7 +272,7 @@ func (s *RedpandaControllerSuite) TestManagedDecommission() {
 			}
 		}
 		return false
-	})
+	}, rp)
 
 	s.waitFor(rp, func(o client.Object) bool {
 		rp := o.(*redpandav1alpha2.Redpanda)
@@ -318,15 +327,15 @@ func (s *RedpandaControllerSuite) TestClusterSettings() {
 
 		rp.Spec.ClusterSpec.Config.Cluster = &runtime.RawExtension{Raw: asJson}
 		s.applyAndWait(rp)
-		s.applyAndWaitFor(rp, func(o client.Object) bool {
+		s.applyAndWaitFor(func(o client.Object) bool {
 			rp := o.(*redpandav1alpha2.Redpanda)
 			for _, cond := range rp.Status.Conditions {
 				if cond.Type == redpandav1alpha2.ClusterConfigSynced {
 					return cond.ObservedGeneration == rp.Generation && cond.Status == metav1.ConditionTrue
 				}
 			}
 			return false
-		})
+		}, rp)
 	}
 	s.applyAndWait(&corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
@@ -426,14 +435,14 @@ func (s *RedpandaControllerSuite) SetupSuite() {
 	// rest config given to the manager.
 	s.ctx = context.Background()
 	s.env = testenv.New(t, testenv.Options{
-		Scheme: controller.UnifiedScheme,
+		Scheme: controller.V2Scheme,
 		CRDs:   crds.All(),
 		Logger: testr.New(t),
 	})
 
 	s.client = s.env.Client()
 
-	s.env.SetupManager(func(mgr ctrl.Manager) error {
+	s.env.SetupManager(s.setupRBAC(), func(mgr ctrl.Manager) error {
 		controllers := flux.NewFluxControllers(mgr, fluxclient.Options{}, fluxclient.KubeConfigOptions{})
 		for _, controller := range controllers {
 			if err := controller.SetupWithManager(s.ctx, mgr); err != nil {
@@ -462,18 +471,81 @@ func (s *RedpandaControllerSuite) SetupSuite() {
 	})
 }
 
-func (s *RedpandaControllerSuite) minimalRP(useFlux bool) *redpandav1alpha2.Redpanda {
+func (s *RedpandaControllerSuite) setupRBAC() string {
+	roles, err := kube.DecodeYAML(operatorRBAC, s.client.Scheme())
+	s.Require().NoError(err)
+
+	role := roles[1].(*rbacv1.Role)
+	clusterRole := roles[0].(*rbacv1.ClusterRole)
+
+	// Inject additional permissions required for running in testenv.
+	role.Rules = append(role.Rules, rbacv1.PolicyRule{
+		APIGroups: []string{""},
+		Resources: []string{"pods/portforward"},
+		Verbs:     []string{"*"},
+	})
+
+	name := "testenv-" + s.randString(6)
+
+	role.Name = name
+	role.Namespace = s.env.Namespace()
+	clusterRole.Name = name
+	clusterRole.Namespace = s.env.Namespace()
+
+	s.applyAndWait(roles...)
+	s.applyAndWait(
+		&corev1.ServiceAccount{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+		},
+		&rbacv1.RoleBinding{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+			Subjects: []rbacv1.Subject{
+				{Kind: "ServiceAccount", Namespace: s.env.Namespace(), Name: name},
+			},
+			RoleRef: rbacv1.RoleRef{
+				APIGroup: "rbac.authorization.k8s.io",
+				Kind:     "Role",
+				Name:     role.Name,
+			},
+		},
+		&rbacv1.ClusterRoleBinding{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+			Subjects: []rbacv1.Subject{
+				{Kind: "ServiceAccount", Namespace: s.env.Namespace(), Name: name},
+			},
+			RoleRef: rbacv1.RoleRef{
+				APIGroup: "rbac.authorization.k8s.io",
+				Kind:     "ClusterRole",
+				Name:     clusterRole.Name,
+			},
+		},
+	)
+
+	return name
+}
+
+func (s *RedpandaControllerSuite) randString(length int) string {
 	const alphabet = "abcdefghijklmnopqrstuvwxyz0123456789"
 
-	name := "rp-"
+	name := ""
 	for i := 0; i < 6; i++ {
 		//nolint:gosec // not meant to be a secure random string.
 		name += string(alphabet[rand.Intn(len(alphabet))])
 	}
 
+	return name
+}
+
+func (s *RedpandaControllerSuite) minimalRP(useFlux bool) *redpandav1alpha2.Redpanda {
 	return &redpandav1alpha2.Redpanda{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: name, // GenerateName doesn't play nice with SSA.
+			Name: "rp-" + s.randString(6), // GenerateName doesn't play nice with SSA.
 		},
 		Spec: redpandav1alpha2.RedpandaSpec{
 			ChartRef: redpandav1alpha2.ChartRef{
@@ -541,45 +613,50 @@ func (s *RedpandaControllerSuite) deleteAndWait(obj client.Object) {
 	}))
 }
 
-func (s *RedpandaControllerSuite) applyAndWait(obj client.Object) {
-	s.applyAndWaitFor(obj, func(o client.Object) bool {
+func (s *RedpandaControllerSuite) applyAndWait(objs ...client.Object) {
+	s.applyAndWaitFor(func(obj client.Object) bool {
 		switch obj := obj.(type) {
 		case *redpandav1alpha2.Redpanda:
 			ready := apimeta.IsStatusConditionTrue(obj.Status.Conditions, "Ready")
 			upToDate := obj.Generation != 0 && obj.Generation == obj.Status.ObservedGeneration
 			return upToDate && ready
 
-		case *corev1.Secret, *corev1.ConfigMap:
+		case *corev1.Secret, *corev1.ConfigMap, *corev1.ServiceAccount,
+			*rbacv1.ClusterRole, *rbacv1.Role, *rbacv1.RoleBinding, *rbacv1.ClusterRoleBinding:
 			return true
 
 		default:
 			s.T().Fatalf("unhandled object %T in applyAndWait", obj)
 			panic("unreachable")
 		}
-	})
+	}, objs...)
 }
 
-func (s *RedpandaControllerSuite) applyAndWaitFor(obj client.Object, cond func(client.Object) bool) {
-	gvk, err := s.client.GroupVersionKindFor(obj)
-	s.NoError(err)
+func (s *RedpandaControllerSuite) applyAndWaitFor(cond func(client.Object) bool, objs ...client.Object) {
+	for _, obj := range objs {
+		gvk, err := s.client.GroupVersionKindFor(obj)
+		s.NoError(err)
 
-	obj.SetManagedFields(nil)
-	obj.GetObjectKind().SetGroupVersionKind(gvk)
+		obj.SetManagedFields(nil)
+		obj.GetObjectKind().SetGroupVersionKind(gvk)
 
-	s.Require().NoError(s.client.Patch(s.ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner("tests")))
+		s.Require().NoError(s.client.Patch(s.ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner("tests")))
+	}
 
-	s.NoError(wait.PollUntilContextTimeout(s.ctx, 5*time.Second, 5*time.Minute, false, func(ctx context.Context) (done bool, err error) {
-		if err := s.client.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil {
-			return false, err
-		}
+	for _, obj := range objs {
+		s.NoError(wait.PollUntilContextTimeout(s.ctx, 5*time.Second, 5*time.Minute, false, func(ctx context.Context) (done bool, err error) {
+			if err := s.client.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil {
+				return false, err
+			}
 
-		if cond(obj) {
-			return true, nil
-		}
+			if cond(obj) {
+				return true, nil
+			}
 
-		s.T().Logf("waiting for %T %q to be ready", obj, obj.GetName())
-		return false, nil
-	}))
+			s.T().Logf("waiting for %T %q to be ready", obj, obj.GetName())
+			return false, nil
+		}))
+	}
 }
 
 func (s *RedpandaControllerSuite) waitFor(obj client.Object, cond func(client.Object) bool) {
@@ -686,3 +763,59 @@ func TestPostInstallUpgradeJobIndex(t *testing.T) {
 	// `clusterConfigfor` utilizes.
 	require.Equal(t, "bootstrap-yaml-envsubst", job.Spec.Template.Spec.InitContainers[0].Name)
 }
+
+// TestControllerRBAC asserts that the declared Roles and ClusterRoles of the
+// RedpandaReconciler line up with all the resource types it needs to manage.
+func TestControllerRBAC(t *testing.T) {
+	scheme := controller.V2Scheme
+
+	expectedVerbs := []string{"create", "delete", "get", "list", "patch", "update", "watch"}
+
+	roles, err := kube.DecodeYAML(operatorRBAC, scheme)
+	require.NoError(t, err)
+
+	role := roles[1].(*rbacv1.Role)
+	clusterRole := roles[0].(*rbacv1.ClusterRole)
+
+	for _, typ := range redpandachart.Types() {
+		gkvs, _, err := scheme.ObjectKinds(typ)
+		require.NoError(t, err)
+
+		require.Len(t, gkvs, 1)
+		gvk := gkvs[0]
+
+		rules := role.Rules
+		if !isNamespaced(typ) {
+			rules = clusterRole.Rules
+		}
+
+		group := gvk.Group
+		kind := pluralize(gvk.Kind)
+
+		idx := slices.IndexFunc(rules, func(rule rbacv1.PolicyRule) bool {
+			return slices.Contains(rule.APIGroups, group) && slices.Contains(rule.Resources, kind)
+		})
+
+		require.NotEqual(t, -1, idx, "missing rules for %s %s", gvk.Group, kind)
+		require.EqualValues(t, expectedVerbs, rules[idx].Verbs, "incorrect verbs for %s %s", gvk.Group, kind)
+	}
+}
+
+func isNamespaced(obj client.Object) bool {
+	switch obj.(type) {
+	case *corev1.Namespace, *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding:
+		return false
+	default:
+		return true
+	}
+}
+
+func pluralize(kind string) string {
+	switch kind[len(kind)-1] {
+	case 's':
+		return strings.ToLower(kind) + "es"
+
+	default:
+		return strings.ToLower(kind) + "s"
+	}
+}