From 59d5f0e6d4ec3865888da7e628b627a14ee64678 Mon Sep 17 00:00:00 2001 From: KubeSphere CI Bot <47586280+ks-ci-bot@users.noreply.github.com> Date: Mon, 13 Jan 2025 16:43:25 +0800 Subject: [PATCH] fix serviceaccount controller remove unnecessary retries (#2188) Signed-off-by: hongming Co-authored-by: hongming --- .../namespace/namespace_controller.go | 18 +++++++++ .../serviceaccount_controller.go | 37 ++++++++++--------- .../serviceaccount_controller_test.go | 12 ++---- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/pkg/controller/namespace/namespace_controller.go b/pkg/controller/namespace/namespace_controller.go index ad2549fda..9ac2251b2 100644 --- a/pkg/controller/namespace/namespace_controller.go +++ b/pkg/controller/namespace/namespace_controller.go @@ -20,6 +20,7 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/klog/v2" iamv1beta1 "kubesphere.io/api/iam/v1beta1" + tenantv1alpha1 "kubesphere.io/api/tenant/v1beta1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -200,5 +201,22 @@ func (r *Reconciler) cleanUp(ctx context.Context, namespace *corev1.Namespace) e if err := r.DeleteAllOf(ctx, roleBinding, client.InNamespace(namespace.Name)); err != nil { return errors.Wrapf(err, "failed to delete role bindings") } + updated := namespace.DeepCopy() + modified := false + newOwnerReferences := make([]metav1.OwnerReference, 0, len(updated.OwnerReferences)) + for _, owner := range updated.OwnerReferences { + if owner.Kind != tenantv1alpha1.ResourceKindWorkspace { + newOwnerReferences = append(newOwnerReferences, owner) + } else { + modified = true + } + } + + if modified { + updated.OwnerReferences = newOwnerReferences + if err := r.Patch(ctx, updated, client.MergeFrom(namespace)); err != nil { + return errors.Wrapf(err, "failed to cleanup ownerReferences for namespace %s", namespace.Name) + } + } return nil } diff --git a/pkg/controller/serviceaccount/serviceaccount_controller.go b/pkg/controller/serviceaccount/serviceaccount_controller.go index 02a378da2..747868245 100644 --- a/pkg/controller/serviceaccount/serviceaccount_controller.go +++ b/pkg/controller/serviceaccount/serviceaccount_controller.go @@ -10,18 +10,18 @@ import ( "fmt" "github.com/go-logr/logr" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + iamv1beta1 "kubesphere.io/api/iam/v1beta1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - iamv1beta1 "kubesphere.io/api/iam/v1beta1" - kscontroller "kubesphere.io/kubesphere/pkg/controller" rbacutils "kubesphere.io/kubesphere/pkg/utils/rbac" ) @@ -58,8 +58,7 @@ func (r *Reconciler) SetupWithManager(mgr *kscontroller.Manager) error { // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := r.logger.WithValues("serivceaccount", req.NamespacedName) - // ctx := context.Background() + logger := r.logger.WithValues("serviceaccount", req.NamespacedName) sa := &corev1.ServiceAccount{} if err := r.Get(ctx, req.NamespacedName, sa); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) @@ -78,12 +77,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu func (r *Reconciler) getReferenceRole(ctx context.Context, roleName, namespace string) (*rbacv1.Role, error) { refRole := &rbacv1.Role{} refRoleName := rbacutils.RelatedK8sResourceName(roleName) - err := r.Client.Get(ctx, types.NamespacedName{Name: refRoleName, Namespace: namespace}, refRole) - if err != nil { + if err := r.Get(ctx, types.NamespacedName{Name: refRoleName, Namespace: namespace}, refRole); err != nil { return nil, err } - if v := refRole.Labels[iamv1beta1.RoleReferenceLabel]; v != roleName { - return nil, errors.NewNotFound(rbacv1.Resource("roles"), refRoleName) + if refRole.Labels[iamv1beta1.RoleReferenceLabel] != roleName { + return nil, apierrors.NewNotFound(rbacv1.Resource("roles"), refRoleName) } return refRole, nil } @@ -93,16 +91,21 @@ func (r *Reconciler) CreateOrUpdateRoleBinding(ctx context.Context, logger logr. if roleName == "" { return nil } - var role *rbacv1.Role + role, err := r.getReferenceRole(ctx, roleName, sa.Namespace) if err != nil { - logger.Error(err, "cannot get reference role", "roleName", roleName) - return err + if apierrors.IsNotFound(err) { + logger.V(4).Info("related role not found", "namespace", sa.Namespace, "role", roleName) + return nil + } + return errors.Wrapf(err, "cannot get reference role %s/%s", sa.Namespace, roleName) } // Delete existing rolebindings. saRoleBinding := &rbacv1.RoleBinding{} - _ = r.Client.DeleteAllOf(ctx, saRoleBinding, client.InNamespace(sa.Namespace), client.MatchingLabels{iamv1beta1.ServiceAccountReferenceLabel: sa.Name}) + if err = r.DeleteAllOf(ctx, saRoleBinding, client.InNamespace(sa.Namespace), client.MatchingLabels{iamv1beta1.ServiceAccountReferenceLabel: sa.Name}); err != nil { + return errors.Wrapf(err, "failed to delete RoleBindings for %s/%s", sa.Namespace, sa.Name) + } saRoleBinding = &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ @@ -125,14 +128,12 @@ func (r *Reconciler) CreateOrUpdateRoleBinding(ctx context.Context, logger logr. } if err := controllerutil.SetControllerReference(sa, saRoleBinding, r.Scheme()); err != nil { - logger.Error(err, "set controller reference failed") - return err + return errors.Wrapf(err, "failed to set controller reference for RoleBinding %s/%s", sa.Namespace, saRoleBinding.Name) } logger.V(4).Info("create ServiceAccount rolebinding", "ServiceAccount", sa.Name) - if err := r.Client.Create(ctx, saRoleBinding); err != nil { - logger.Error(err, "create rolebinding failed") - return err + if err := r.Create(ctx, saRoleBinding); err != nil { + return errors.Wrapf(err, "failed to create RoleBinding %s/%s", sa.Namespace, saRoleBinding.Name) } return nil } diff --git a/pkg/controller/serviceaccount/serviceaccount_controller_test.go b/pkg/controller/serviceaccount/serviceaccount_controller_test.go index ddb21e1e2..5a24fa996 100644 --- a/pkg/controller/serviceaccount/serviceaccount_controller_test.go +++ b/pkg/controller/serviceaccount/serviceaccount_controller_test.go @@ -13,17 +13,15 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" + iamv1beta1 "kubesphere.io/api/iam/v1beta1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - iamv1beta1 "kubesphere.io/api/iam/v1beta1" - "kubesphere.io/kubesphere/pkg/utils/k8sutil" ) @@ -78,7 +76,6 @@ var _ = Describe("ServiceAccount", func() { ctx := context.Background() reconciler := &Reconciler{ - //nolint:staticcheck Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(), logger: ctrl.Log.WithName("controllers").WithName("serviceaccount"), recorder: record.NewFakeRecorder(5), @@ -93,16 +90,15 @@ var _ = Describe("ServiceAccount", func() { By("Expecting to bind role successfully") rolebindings := &rbacv1.RoleBindingList{} Expect(func() bool { - reconciler.List(ctx, rolebindings, client.InNamespace(sa.Namespace), client.MatchingLabels{iamv1beta1.ServiceAccountReferenceLabel: sa.Name}) + _ = reconciler.List(ctx, rolebindings, client.InNamespace(sa.Namespace), client.MatchingLabels{iamv1beta1.ServiceAccountReferenceLabel: sa.Name}) return len(rolebindings.Items) == 1 && k8sutil.IsControlledBy(rolebindings.Items[0].OwnerReferences, "ServiceAccount", saName) }()).Should(BeTrue()) }) - It("Should report NotFound error when role doesn't exist", func() { + It("Should not report NotFound error when role doesn't exist", func() { ctx := context.Background() reconciler := &Reconciler{ - //nolint:staticcheck Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(), logger: ctrl.Log.WithName("controllers").WithName("serviceaccount"), recorder: record.NewFakeRecorder(5), @@ -110,7 +106,7 @@ var _ = Describe("ServiceAccount", func() { Expect(reconciler.Create(ctx, sa)).Should(Succeed()) _, err := reconciler.Reconcile(ctx, req) - Expect(apierrors.IsNotFound(err)).To(BeTrue()) + Expect(err).Should(Succeed()) }) }) })