chore: rollback service account name when upgrading componentdefinition (#9967)

Author: cjc7373

controllers/apps/component/component_controller.go

@@ -169,10 +169,11 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			&componentReloadSidecarTransformer{Client: r.Client},
 			// handle restore before workloads transform
 			&componentRestoreTransformer{Client: r.Client},
-			// handle the component workload
-			&componentWorkloadTransformer{Client: r.Client},
 			// handle RBAC for component workloads
+			// it should be put before workload transformer, because we modify podSpec's serviceaccount in it
 			&componentRBACTransformer{},
+			// handle the component workload
+			&componentWorkloadTransformer{Client: r.Client},
 			// handle component postProvision lifecycle action
 			&componentPostProvisionTransformer{},
 			// update component status

controllers/apps/component/transformer_component_rbac.go

@@ -20,14 +20,17 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 package component
 
 import (
+	"encoding/json"
 	"fmt"
+	"hash/fnv"
 
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -50,9 +53,11 @@ type componentRBACTransformer struct{}
 var _ graph.Transformer = &componentRBACTransformer{}
 
 const EventReasonRBACManager = "RBACManager"
+const EventReasonServiceAccountRollback = "ServiceAccountRollback"
 
 func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *graph.DAG) error {
 	transCtx, _ := ctx.(*componentTransformContext)
+	graphCli, _ := transCtx.Client.(model.GraphClient)
 	synthesizedComp := transCtx.SynthesizeComponent
 	if isCompDeleting(transCtx.ComponentOrig) {
 		return nil
@@ -83,17 +88,41 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr
 		return nil
 	}
 
-	graphCli, _ := transCtx.Client.(model.GraphClient)
-
 	var err error
+	comp := transCtx.Component
+	lastServiceAccountName := comp.Annotations[constant.ComponentLastServiceAccountNameAnnotationKey]
+	lastHash := comp.Annotations[constant.ComponentLastServiceAccountRuleHashAnnotationKey]
 	if serviceAccountName == "" {
+		rollback, err := needRollbackServiceAccount(transCtx)
+		if err != nil {
+			return err
+		}
+
 		serviceAccountName = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName)
+		if rollback {
+			transCtx.EventRecorder.Event(comp, corev1.EventTypeNormal, EventReasonServiceAccountRollback, "Change to serviceaccount has been rolled back to prevent pod restart")
+			serviceAccountName = lastServiceAccountName
+		}
 		// if no rolebinding is needed, sa will be created anyway, because other modules may reference it.
 		sa, err = createOrUpdateServiceAccount(transCtx, serviceAccountName, graphCli, dag)
 		if err != nil {
 			return err
 		}
+
+		hash, err := computeServiceAccountRuleHash(transCtx)
+		if err != nil {
+			return err
+		}
+
+		if lastServiceAccountName != serviceAccountName || lastHash != hash {
+			comp.Annotations[constant.ComponentLastServiceAccountNameAnnotationKey] = serviceAccountName
+			comp.Annotations[constant.ComponentLastServiceAccountRuleHashAnnotationKey] = hash
+			graphCli.Update(dag, transCtx.ComponentOrig, transCtx.Component)
+		}
 	}
+
+	synthesizedComp.PodSpec.ServiceAccountName = serviceAccountName
+
 	role, err := createOrUpdateRole(transCtx, graphCli, dag)
 	if err != nil {
 		return err
@@ -104,8 +133,9 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr
 		return err
 	}
 
-	objs := []client.Object{sa, role}
+	objs := []client.Object{role}
 	if sa != nil {
+		objs = append(objs, sa)
 		// serviceAccount should be created before roleBinding and role
 		for _, rb := range rbs {
 			objs = append(objs, rb)
@@ -131,6 +161,32 @@ func (t *componentRBACTransformer) rbacInstanceAssistantObjects(graphCli model.G
 	}
 }
 
+func computeServiceAccountRuleHash(transCtx *componentTransformContext) (string, error) {
+	hash := fnv.New32a()
+	data, err := json.Marshal(transCtx.SynthesizeComponent.PolicyRules)
+	if err != nil {
+		return "", err
+	}
+	hash.Write(data)
+	enabled := transCtx.SynthesizeComponent.LifecycleActions != nil
+	fmt.Fprint(hash, enabled)
+	return rand.SafeEncodeString(fmt.Sprintf("%d", hash.Sum32())), nil
+}
+
+func needRollbackServiceAccount(transCtx *componentTransformContext) (rollback bool, err error) {
+	hash, err := computeServiceAccountRuleHash(transCtx)
+	if err != nil {
+		return false, err
+	}
+
+	lastHash, ok := transCtx.Component.Annotations[constant.ComponentLastServiceAccountRuleHashAnnotationKey]
+	if !ok {
+		return false, nil
+	}
+
+	return hash == lastHash, nil
+}
+
 func labelAndAnnotationEqual(old, new metav1.Object) bool {
 	// exclude component labels, since they are different for each component
 	compLabels := constant.GetCompLabels("", "")

controllers/apps/component/transformer_component_rbac_test.go

@@ -189,7 +189,7 @@ var _ = Describe("object rbac transformer test.", func() {
 			// sa should be created
 			serviceAccount := factory.BuildServiceAccount(synthesizedComp, serviceAccountName)
 
-			dagExpected := mockDAG(graphCli, compObj)
+			dagExpected := mockDAGWithUpdate(graphCli, compObj)
 			graphCli.Create(dagExpected, serviceAccount)
 
 			Expect(dag.Equals(dagExpected, model.DefaultLess)).Should(BeTrue())
@@ -204,7 +204,7 @@ var _ = Describe("object rbac transformer test.", func() {
 				Name:     constant.RBACRoleName,
 			}, serviceAccountName)
 			serviceAccount := factory.BuildServiceAccount(synthesizedComp, serviceAccountName)
-			dagExpected := mockDAG(graphCli, compObj)
+			dagExpected := mockDAGWithUpdate(graphCli, compObj)
 			graphCli.Create(dagExpected, serviceAccount)
 			graphCli.Create(dagExpected, clusterPodRoleBinding)
 			graphCli.DependOn(dagExpected, clusterPodRoleBinding, serviceAccount)
@@ -225,7 +225,7 @@ var _ = Describe("object rbac transformer test.", func() {
 				Name:     cmpdRole.Name,
 			}, serviceAccountName)
 			serviceAccount := factory.BuildServiceAccount(synthesizedComp, serviceAccountName)
-			dagExpected := mockDAG(graphCli, compObj)
+			dagExpected := mockDAGWithUpdate(graphCli, compObj)
 			graphCli.Create(dagExpected, serviceAccount)
 			graphCli.Create(dagExpected, cmpdRoleBinding)
 			graphCli.Create(dagExpected, cmpdRole)
@@ -242,6 +242,111 @@ var _ = Describe("object rbac transformer test.", func() {
 			Expect(reflect.DeepEqual(rb.Subjects, cmpdRoleBinding.Subjects)).To(BeTrue())
 			Expect(reflect.DeepEqual(rb.RoleRef, cmpdRoleBinding.RoleRef)).To(BeTrue())
 		})
+
+		Context("rollback behavior", func() {
+			It("tests needRollbackServiceAccount", func() {
+				init(true, false)
+				ctx := transCtx.(*componentTransformContext)
+
+				By("create another cmpd")
+				anotherTpl := testapps.NewComponentDefinitionFactory(compDefName).
+					WithRandomName().
+					SetDefaultSpec().
+					Create(&testCtx).
+					GetObject()
+				hash, err := computeServiceAccountRuleHash(ctx)
+				Expect(err).ShouldNot(HaveOccurred())
+
+				// Case: No label, should return false
+				needRollback, err := needRollbackServiceAccount(ctx)
+				Expect(err).Should(BeNil())
+				Expect(needRollback).Should(BeFalse())
+
+				// Case: With same cmpd
+				ctx.Component.Annotations[constant.ComponentLastServiceAccountRuleHashAnnotationKey] = hash
+				ctx.Component.Annotations[constant.ComponentLastServiceAccountNameAnnotationKey] = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName)
+				needRollback, err = needRollbackServiceAccount(ctx)
+				Expect(err).Should(BeNil())
+				Expect(needRollback).Should(BeTrue())
+
+				// Case: Different cmpd, same spec
+				another := anotherTpl.DeepCopy()
+				ctx.SynthesizeComponent, err = component.BuildSynthesizedComponent(ctx, k8sClient, another, compObj)
+				Expect(err).Should(Succeed())
+				needRollback, err = needRollbackServiceAccount(ctx)
+				Expect(err).Should(BeNil())
+				Expect(needRollback).Should(BeTrue())
+
+				// Case: Different cmpd, different policy rules
+				another = anotherTpl.DeepCopy()
+				another.Spec.PolicyRules = []rbacv1.PolicyRule{
+					{
+						APIGroups: []string{""},
+						Resources: []string{"pods"},
+						Verbs:     []string{"get"},
+					},
+				}
+				ctx.SynthesizeComponent, err = component.BuildSynthesizedComponent(ctx, k8sClient, another, compObj)
+				Expect(err).Should(Succeed())
+				needRollback, err = needRollbackServiceAccount(ctx)
+				Expect(err).Should(BeNil())
+				Expect(needRollback).Should(BeFalse())
+
+				// Case: Different cmpd, different lifecycle action
+				another = anotherTpl.DeepCopy()
+				another.Spec.PolicyRules = nil
+				another.Spec.LifecycleActions = nil
+				ctx.SynthesizeComponent, err = component.BuildSynthesizedComponent(ctx, k8sClient, another, compObj)
+				Expect(err).Should(Succeed())
+				needRollback, err = needRollbackServiceAccount(ctx)
+				Expect(err).Should(BeNil())
+				Expect(needRollback).Should(BeFalse())
+			})
+
+			mockDAGWithUpdate := func(graphCli model.GraphClient, comp *appsv1.Component) *graph.DAG {
+				d := graph.NewDAG()
+				graphCli.Root(d, comp, comp, model.ActionUpdatePtr())
+				its := &workloads.InstanceSet{}
+				graphCli.Create(d, its)
+				return d
+			}
+
+			less := func(v1, v2 graph.Vertex) bool {
+				o1, ok1 := v1.(*model.ObjectVertex)
+				o2, ok2 := v2.(*model.ObjectVertex)
+				if !ok1 || !ok2 {
+					return false
+				}
+				if o1.String() != o2.String() {
+					return o1.String() < o2.String()
+				}
+				if !reflect.DeepEqual(o1.Obj.GetLabels(), o2.Obj.GetLabels()) {
+					return true
+				}
+				return !reflect.DeepEqual(o1.Obj.GetAnnotations(), o2.Obj.GetAnnotations())
+			}
+
+			It("adds labels for an old component", func() {
+				init(false, false)
+				// mock a running workload
+				ctx := transCtx.(*componentTransformContext)
+				ctx.RunningWorkload = &workloads.InstanceSet{}
+				expectedComp := compObj.DeepCopy()
+				Expect(transformer.Transform(transCtx, dag)).Should(BeNil())
+				// sa should be created
+				oldSAName := constant.GenerateDefaultServiceAccountName(compDefObj.Name)
+				serviceAccount := factory.BuildServiceAccount(synthesizedComp, oldSAName)
+
+				hash, err := computeServiceAccountRuleHash(ctx)
+				Expect(err).ShouldNot(HaveOccurred())
+				expectedComp.Annotations[constant.ComponentLastServiceAccountRuleHashAnnotationKey] = hash
+				expectedComp.Annotations[constant.ComponentLastServiceAccountNameAnnotationKey] = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName)
+				dagExpected := mockDAGWithUpdate(graphCli, expectedComp)
+				graphCli.Create(dagExpected, serviceAccount)
+
+				Expect(dag.Equals(dagExpected, less)).Should(BeTrue())
+			})
+		})
 	})
 })
 
@@ -252,3 +357,11 @@ func mockDAG(graphCli model.GraphClient, comp *appsv1.Component) *graph.DAG {
 	graphCli.Create(d, its)
 	return d
 }
+
+func mockDAGWithUpdate(graphCli model.GraphClient, comp *appsv1.Component) *graph.DAG {
+	d := graph.NewDAG()
+	graphCli.Root(d, comp, comp, model.ActionUpdatePtr())
+	its := &workloads.InstanceSet{}
+	graphCli.Create(d, its)
+	return d
+}

pkg/constant/annotations.go

@@ -51,6 +51,9 @@ const (
 	NodeSelectorOnceAnnotationKey = "workloads.kubeblocks.io/node-selector-once"
 
 	PVCNamePrefixAnnotationKey = "apps.kubeblocks.io/pvc-name-prefix"
+
+	ComponentLastServiceAccountNameAnnotationKey     = "component.kubeblocks.io/last-service-account-name"
+	ComponentLastServiceAccountRuleHashAnnotationKey = "component.kubeblocks.io/last-service-account-rule-hash"
 )
 
 const (

pkg/controller/component/synthesize_component.go

@@ -37,7 +37,6 @@ import (
 	"github.com/apecloud/kubeblocks/pkg/controller/scheduling"
 	intctrlutil "github.com/apecloud/kubeblocks/pkg/controllerutil"
 	"github.com/apecloud/kubeblocks/pkg/generics"
-	viper "github.com/apecloud/kubeblocks/pkg/viperx"
 )
 
 var (
@@ -108,7 +107,6 @@ func BuildSynthesizedComponent(ctx context.Context, cli client.Reader,
 		Replicas:                         comp.Spec.Replicas,
 		Resources:                        comp.Spec.Resources,
 		TLSConfig:                        comp.Spec.TLSConfig,
-		ServiceAccountName:               comp.Spec.ServiceAccountName,
 		Instances:                        comp.Spec.Instances,
 		Ordinals:                         comp.Spec.Ordinals,
 		FlatInstanceOrdinal:              comp.Spec.FlatInstanceOrdinal,
@@ -149,9 +147,6 @@ func BuildSynthesizedComponent(ctx context.Context, cli client.Reader,
 	// override componentService
 	overrideComponentServices(synthesizeComp, comp)
 
-	// build serviceAccountName
-	buildServiceAccountName(synthesizeComp)
-
 	// build runtimeClassName
 	buildRuntimeClassName(synthesizeComp, comp)
 
@@ -513,19 +508,6 @@ func synthesizeFileTemplate(comp *appsv1.Component, tpl appsv1.ComponentFileTemp
 	return stpl
 }
 
-// buildServiceAccountName builds serviceAccountName for component and podSpec.
-func buildServiceAccountName(synthesizeComp *SynthesizedComponent) {
-	if synthesizeComp.ServiceAccountName != "" {
-		synthesizeComp.PodSpec.ServiceAccountName = synthesizeComp.ServiceAccountName
-		return
-	}
-	if !viper.GetBool(constant.EnableRBACManager) {
-		return
-	}
-	synthesizeComp.ServiceAccountName = constant.GenerateDefaultServiceAccountName(synthesizeComp.CompDefName)
-	synthesizeComp.PodSpec.ServiceAccountName = synthesizeComp.ServiceAccountName
-}
-
 func buildRuntimeClassName(synthesizeComp *SynthesizedComponent, comp *appsv1.Component) {
 	if comp.Spec.RuntimeClassName == nil {
 		return

pkg/controller/component/type.go

@@ -49,7 +49,6 @@ type SynthesizedComponent struct {
 	FileTemplates                    []SynthesizedFileTemplate
 	LogConfigs                       []kbappsv1.LogConfig                   `json:"logConfigs,omitempty"`
 	TLSConfig                        *kbappsv1.TLSConfig                    `json:"tlsConfig"`
-	ServiceAccountName               string                                 `json:"serviceAccountName,omitempty"`
 	ServiceReferences                map[string]*kbappsv1.ServiceDescriptor `json:"serviceReferences,omitempty"`
 	Labels                           map[string]string                      `json:"labels,omitempty"`
 	StaticLabels                     map[string]string                      // labels defined by the component definition