From 00f85d5a6caa416caf78fdbb1559237652b0c393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Fri, 15 Mar 2024 15:46:51 +0100 Subject: [PATCH] fix: fix enforcing up to date ControlPlane's ClusterRole and ClusterRoleBinding (#11) Co-authored-by: Jakub Warczarek --- .../controller_reconciler_utils.go | 72 +++++++++++-------- pkg/utils/test/predicates.go | 43 +++++++++++ test/integration/test_controlplane.go | 49 ++++++++++++- 3 files changed, 134 insertions(+), 30 deletions(-) diff --git a/controllers/controlplane/controller_reconciler_utils.go b/controllers/controlplane/controller_reconciler_utils.go index be8c300c6..50784de5d 100644 --- a/controllers/controlplane/controller_reconciler_utils.go +++ b/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 diff --git a/pkg/utils/test/predicates.go b/pkg/utils/test/predicates.go index 4d38de776..9b47a626c 100644 --- a/pkg/utils/test/predicates.go +++ b/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. diff --git a/test/integration/test_controlplane.go b/test/integration/test_controlplane.go index edc846943..f79fce0cb 100644 --- a/test/integration/test_controlplane.go +++ b/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)