fix: fix enforcing up to date ControlPlane's ClusterRole and ClusterR…

…oleBinding (#11)

Co-authored-by: Jakub Warczarek <[email protected]>

controllers/controlplane/controller_reconciler_utils.go

@@ -319,28 +319,34 @@ func (r *Reconciler) ensureClusterRole(
 	}
 
 	controlplaneContainer := k8sutils.GetPodContainerByName(&controlplane.Spec.Deployment.PodTemplateSpec.Spec, consts.ControlPlaneControllerContainerName)
-	generatedClusterRole, err := k8sresources.GenerateNewClusterRoleForControlPlane(controlplane.Name, controlplaneContainer.Image, r.DevelopmentMode)
+	generated, err := k8sresources.GenerateNewClusterRoleForControlPlane(controlplane.Name, controlplaneContainer.Image, r.DevelopmentMode)
 	if err != nil {
 		return false, nil, err
 	}
-	k8sutils.SetOwnerForObject(generatedClusterRole, controlplane)
+	k8sutils.SetOwnerForObject(generated, controlplane)
 
 	if count == 1 {
-		var updated bool
-		existingClusterRole := &clusterRoles[0]
-		updated, generatedClusterRole.ObjectMeta = k8sutils.EnsureObjectMetaIsUpdated(existingClusterRole.ObjectMeta, generatedClusterRole.ObjectMeta)
+		var (
+			updated  bool
+			existing = &clusterRoles[0]
+			old      = existing.DeepCopy()
+		)
+
+		updated, existing.ObjectMeta = k8sutils.EnsureObjectMetaIsUpdated(existing.ObjectMeta, generated.ObjectMeta)
 		if updated ||
-			!cmp.Equal(existingClusterRole.Rules, generatedClusterRole.Rules) ||
-			!cmp.Equal(existingClusterRole.AggregationRule, generatedClusterRole.AggregationRule) {
-			if err := r.Client.Patch(ctx, generatedClusterRole, client.MergeFrom(existingClusterRole)); err != nil {
-				return false, existingClusterRole, fmt.Errorf("failed patching ControlPlane's ClusterRole %s: %w", existingClusterRole.Name, err)
+			!cmp.Equal(existing.Rules, generated.Rules) ||
+			!cmp.Equal(existing.AggregationRule, generated.AggregationRule) {
+			existing.Rules = generated.Rules
+			existing.AggregationRule = generated.AggregationRule
+			if err := r.Client.Patch(ctx, existing, client.MergeFrom(old)); err != nil {
+				return false, existing, fmt.Errorf("failed patching ControlPlane's ClusterRole %s: %w", existing.Name, err)
 			}
-			return true, generatedClusterRole, nil
+			return true, existing, nil
 		}
-		return false, generatedClusterRole, nil
+		return false, existing, nil
 	}
 
-	return true, generatedClusterRole, r.Client.Create(ctx, generatedClusterRole)
+	return true, generated, r.Client.Create(ctx, generated)
 }
 
 func (r *Reconciler) ensureClusterRoleBinding(
@@ -371,37 +377,47 @@ func (r *Reconciler) ensureClusterRoleBinding(
 		return false, nil, errors.New("number of clusterRoleBindings reduced")
 	}
 
-	generatedClusterRoleBinding := k8sresources.GenerateNewClusterRoleBindingForControlPlane(controlplane.Namespace, controlplane.Name, serviceAccountName, clusterRoleName)
-	k8sutils.SetOwnerForObject(generatedClusterRoleBinding, controlplane)
+	generated := k8sresources.GenerateNewClusterRoleBindingForControlPlane(controlplane.Namespace, controlplane.Name, serviceAccountName, clusterRoleName)
+	k8sutils.SetOwnerForObject(generated, controlplane)
 
 	if count == 1 {
-		existingClusterRoleBinding := &clusterRoleBindings[0]
+		existing := &clusterRoleBindings[0]
 		// Delete and re-create ClusterRoleBinding if name of ClusterRole changed because RoleRef is immutable.
-		if !k8sresources.CompareClusterRoleName(existingClusterRoleBinding, clusterRoleName) {
+		if !k8sresources.CompareClusterRoleName(existing, clusterRoleName) {
 			log.Debug(logger, "ClusterRole name changed, delete and re-create a ClusterRoleBinding",
-				existingClusterRoleBinding,
-				"old_cluster_role", existingClusterRoleBinding.RoleRef.Name,
+				existing,
+				"old_cluster_role", existing.RoleRef.Name,
 				"new_cluster_role", clusterRoleName,
 			)
-			if err := r.Client.Delete(ctx, existingClusterRoleBinding); err != nil {
+			if err := r.Client.Delete(ctx, existing); err != nil {
 				return false, nil, err
 			}
 			return false, nil, errors.New("name of ClusterRole changed, out of date ClusterRoleBinding deleted")
 		}
-		var updated bool
-		updated, generatedClusterRoleBinding.ObjectMeta = k8sutils.EnsureObjectMetaIsUpdated(existingClusterRoleBinding.ObjectMeta, generatedClusterRoleBinding.ObjectMeta)
-		if updated ||
-			!k8sresources.ClusterRoleBindingContainsServiceAccount(existingClusterRoleBinding, controlplane.Namespace, serviceAccountName) {
-			if err := r.Client.Patch(ctx, generatedClusterRoleBinding, client.MergeFrom(existingClusterRoleBinding)); err != nil {
-				return false, existingClusterRoleBinding, fmt.Errorf("failed patching ControlPlane's ClusterRoleBinding %s: %w", existingClusterRoleBinding.Name, err)
+
+		var (
+			old                   = existing.DeepCopy()
+			updated               bool
+			updatedServiceAccount bool
+		)
+		updated, existing.ObjectMeta = k8sutils.EnsureObjectMetaIsUpdated(existing.ObjectMeta, generated.ObjectMeta)
+
+		if !k8sresources.ClusterRoleBindingContainsServiceAccount(existing, controlplane.Namespace, serviceAccountName) {
+			existing.Subjects = generated.Subjects
+			updatedServiceAccount = true
+		}
+
+		if updated || updatedServiceAccount {
+			if err := r.Client.Patch(ctx, existing, client.MergeFrom(old)); err != nil {
+				return false, existing, fmt.Errorf("failed patching ControlPlane's ClusterRoleBinding %s: %w", existing.Name, err)
 			}
-			return true, generatedClusterRoleBinding, nil
+			return true, existing, nil
 		}
-		return false, generatedClusterRoleBinding, nil
+		return false, existing, nil
 
 	}
 
-	return true, generatedClusterRoleBinding, r.Client.Create(ctx, generatedClusterRoleBinding)
+	return true, generated, r.Client.Create(ctx, generated)
 }
 
 // ensureAdminMTLSCertificateSecret ensures that a Secret is created with the certificate for mTLS communication between the

pkg/utils/test/predicates.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"slices"
 	"strings"
 	"testing"
 
@@ -13,6 +14,7 @@ import (
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/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"
@@ -191,6 +193,47 @@ func ControlPlaneHasClusterRole(t *testing.T, ctx context.Context, controlplane
 	}
 }
 
+// ControlPlanesClusterRoleHasPolicyRule is a helper function for tests that returns a function
+// that can be used to check if ControlPlane's ClusterRole contains the provided PolicyRule.
+// Should be used in conjunction with require.Eventually or assert.Eventually.
+func ControlPlanesClusterRoleHasPolicyRule(t *testing.T, ctx context.Context, controlplane *operatorv1beta1.ControlPlane, clients K8sClients, pr rbacv1.PolicyRule) func() bool {
+	return func() bool {
+		clusterRoles := MustListControlPlaneClusterRoles(t, ctx, controlplane, clients)
+		t.Logf("%d clusterroles", len(clusterRoles))
+		if len(clusterRoles) == 0 {
+			return false
+		}
+		t.Logf("got %s clusterrole, checking if it contains the requested PolicyRule", clusterRoles[0].Name)
+		return slices.ContainsFunc(clusterRoles[0].Rules, func(e rbacv1.PolicyRule) bool {
+			return slices.Equal(e.APIGroups, pr.APIGroups) &&
+				slices.Equal(e.ResourceNames, pr.ResourceNames) &&
+				slices.Equal(e.Resources, pr.Resources) &&
+				slices.Equal(e.Verbs, pr.Verbs) &&
+				slices.Equal(e.NonResourceURLs, pr.NonResourceURLs)
+		})
+	}
+}
+
+// ControlPlanesClusterRoleBindingHasSubject is a helper function for tests that returns a function
+// that can be used to check if ControlPlane's ClusterRoleBinding contains the provided Subject.
+// Should be used in conjunction with require.Eventually or assert.Eventually.
+func ControlPlanesClusterRoleBindingHasSubject(t *testing.T, ctx context.Context, controlplane *operatorv1beta1.ControlPlane, clients K8sClients, subject rbacv1.Subject) func() bool {
+	return func() bool {
+		clusterRoleBindings := MustListControlPlaneClusterRoleBindings(t, ctx, controlplane, clients)
+		t.Logf("%d clusterrolesbindings", len(clusterRoleBindings))
+		if len(clusterRoleBindings) == 0 {
+			return false
+		}
+		t.Logf("got %s clusterrolebinding, checking if it contains the requested Subject", clusterRoleBindings[0].Name)
+		return slices.ContainsFunc(clusterRoleBindings[0].Subjects, func(e rbacv1.Subject) bool {
+			return e.Kind == subject.Kind &&
+				e.APIGroup == subject.APIGroup &&
+				e.Name == subject.Name &&
+				e.Namespace == subject.Namespace
+		})
+	}
+}
+
 // ControlPlaneHasClusterRoleBinding is a helper function for tests that returns a function
 // that can be used to check if a ControlPlane has a ClusterRoleBinding.
 // Should be used in conjunction with require.Eventually or assert.Eventually.

test/integration/test_controlplane.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"slices"
 	"testing"
 	"time"
 
@@ -12,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -169,6 +171,13 @@ func TestControlPlaneEssentials(t *testing.T) {
 									{
 										Name:  consts.DataPlaneProxyContainerName,
 										Image: helpers.GetDefaultDataPlaneImage(),
+										// Speed up the test.
+										ReadinessProbe: func() *corev1.Probe {
+											p := k8sresources.GenerateDataPlaneReadinessProbe(consts.DataPlaneStatusEndpoint)
+											p.InitialDelaySeconds = 1
+											p.PeriodSeconds = 1
+											return p
+										}(),
 									},
 								},
 							},
@@ -209,6 +218,13 @@ func TestControlPlaneEssentials(t *testing.T) {
 									},
 									Name:  consts.ControlPlaneControllerContainerName,
 									Image: consts.DefaultControlPlaneImage,
+									// Speed up the test.
+									ReadinessProbe: func() *corev1.Probe {
+										p := k8sresources.GenerateControlPlaneProbe("/readyz", intstr.FromInt(10254))
+										p.InitialDelaySeconds = 1
+										p.PeriodSeconds = 1
+										return p
+									}(),
 								},
 							},
 						},
@@ -325,10 +341,39 @@ func TestControlPlaneEssentials(t *testing.T) {
 	t.Log("verifying controlplane's webhook is functional")
 	verifyControlPlaneWebhookIsFunctional(t, GetCtx(), clients)
 
+	t.Log("verifying that controlplane's ClusterRole is patched if it goes out of sync")
+	clusterRoles = testutils.MustListControlPlaneClusterRoles(t, GetCtx(), controlplane, clients)
+	require.Len(t, clusterRoles, 1, "There must be only one ControlPlane ClusterRole")
+	clusterRole := clusterRoles[0]
+	idx := slices.IndexFunc(clusterRole.Rules, func(pr rbacv1.PolicyRule) bool {
+		return pr.Resources != nil && slices.Contains(pr.Resources, "endpointslices")
+	})
+	require.GreaterOrEqual(t, idx, 0)
+	endpointSlicesRule := clusterRole.Rules[idx]
+	oldClusterRole := clusterRole.DeepCopy()
+	require.NotEmpty(t, clusterRole.Rules)
+	clusterRole.Rules = slices.Delete(clusterRole.Rules, idx, idx+1)
+	t.Logf("deleting endpointslices policyrule form %s clusterrole", clusterRole.Name)
+	require.NoError(t, clients.MgrClient.Patch(GetCtx(), &clusterRole, client.MergeFrom(oldClusterRole)))
+	t.Log("verifying that controlplane's ClusterRole is patched with the policy rule that was removed")
+	require.Eventually(t, testutils.ControlPlanesClusterRoleHasPolicyRule(t, GetCtx(), controlplane, clients, endpointSlicesRule), testutils.ControlPlaneCondDeadline, testutils.ControlPlaneCondTick)
+
+	t.Log("verifying that controlplane's ClusterRoleBinding is patched if it goes out of sync")
+	clusterRoleBindings = testutils.MustListControlPlaneClusterRoleBindings(t, GetCtx(), controlplane, clients)
+	require.Len(t, clusterRoleBindings, 1, "There must be only one ControlPlane ClusterRoleBinding")
+	clusterRoleBinding := clusterRoleBindings[0]
+	require.NotEmpty(t, clusterRoleBinding.Subjects)
+	subject := clusterRoleBinding.Subjects[0]
+	oldClusterRoleBinding := clusterRoleBinding.DeepCopy()
+	clusterRoleBinding.Subjects = slices.Delete(clusterRoleBinding.Subjects, 0, 1)
+	t.Logf("deleting %s/%s subject form %s clusterrolebinding", subject.Namespace, subject.Name, clusterRoleBinding.Name)
+	require.NoError(t, clients.MgrClient.Patch(GetCtx(), &clusterRoleBinding, client.MergeFrom(oldClusterRoleBinding)))
+	t.Log("verifying that controlplane's ClusterRoleBinding is patched with the subject that was removed")
+	require.Eventually(t, testutils.ControlPlanesClusterRoleBindingHasSubject(t, GetCtx(), controlplane, clients, subject), testutils.ControlPlaneCondDeadline, testutils.ControlPlaneCondTick)
+
 	// delete controlplane and verify that cluster wide resources removed.
 	t.Log("verifying cluster wide resources removed after controlplane deleted")
-	err = controlplaneClient.Delete(GetCtx(), controlplane.Name, metav1.DeleteOptions{})
-	require.NoError(t, err)
+	require.NoError(t, controlplaneClient.Delete(GetCtx(), controlplane.Name, metav1.DeleteOptions{}))
 	require.Eventually(t, testutils.Not(testutils.ControlPlaneHasClusterRole(t, GetCtx(), controlplane, clients)), testutils.ControlPlaneCondDeadline, testutils.ControlPlaneCondTick)
 	require.Eventually(t, testutils.Not(testutils.ControlPlaneHasClusterRoleBinding(t, GetCtx(), controlplane, clients)), testutils.ControlPlaneCondDeadline, testutils.ControlPlaneCondTick)
 	require.Eventually(t, testutils.Not(testutils.ControlPlaneHasAdmissionWebhookConfiguration(t, GetCtx(), controlplane, clients)), testutils.ControlPlaneCondDeadline, testutils.ControlPlaneCondTick)