From 28e7279336a50acedbd7ef6eb4eb8c97ff318d55 Mon Sep 17 00:00:00 2001 From: Harold Cheng Date: Mon, 7 Jul 2025 16:51:04 +0800 Subject: [PATCH 1/4] chore: rollback service account name when upgrading componentdefinition --- .../component/transformer_component_meta.go | 1 + .../component/transformer_component_rbac.go | 2 ++ pkg/constant/labels.go | 15 +++++----- pkg/controller/component/suite_test.go | 1 + .../component/synthesize_component.go | 28 +++++++++++++++++-- .../component/synthesize_component_test.go | 27 ++++++++++++++++++ 6 files changed, 64 insertions(+), 10 deletions(-) diff --git a/controllers/apps/component/transformer_component_meta.go b/controllers/apps/component/transformer_component_meta.go index 9ae06222038..dfafcd110d9 100644 --- a/controllers/apps/component/transformer_component_meta.go +++ b/controllers/apps/component/transformer_component_meta.go @@ -42,6 +42,7 @@ func (t *componentMetaTransformer) Transform(ctx graph.TransformContext, dag *gr } compDef := comp.Labels[constant.ComponentDefinitionLabelKey] if compDef != comp.Spec.CompDef { + comp.Labels[constant.LastComponentDefinitionLabelKey] = compDef comp.Labels[constant.ComponentDefinitionLabelKey] = comp.Spec.CompDef } controllerutil.AddFinalizer(comp, constant.DBComponentFinalizerName) diff --git a/controllers/apps/component/transformer_component_rbac.go b/controllers/apps/component/transformer_component_rbac.go index 5bd2a221177..9754cf5af49 100644 --- a/controllers/apps/component/transformer_component_rbac.go +++ b/controllers/apps/component/transformer_component_rbac.go @@ -45,6 +45,8 @@ import ( ) // componentRBACTransformer puts the RBAC objects at the beginning of the DAG +// Note: rbac objects created in this transformer are not necessarily used in workload objects, +// as when updating componentdefition, old serviceaccount may be retained to prevent pod restart. type componentRBACTransformer struct{} var _ graph.Transformer = &componentRBACTransformer{} diff --git a/pkg/constant/labels.go b/pkg/constant/labels.go index e1532f2b09e..b11b38a961d 100644 --- a/pkg/constant/labels.go +++ b/pkg/constant/labels.go @@ -33,13 +33,14 @@ const ( // labels defined by KubeBlocks const ( - ClusterDefLabelKey = "clusterdefinition.kubeblocks.io/name" - ShardingDefLabelKey = "shardingdefinition.kubeblocks.io/name" - ComponentDefinitionLabelKey = "componentdefinition.kubeblocks.io/name" - ComponentVersionLabelKey = "componentversion.kubeblocks.io/name" - SidecarDefLabelKey = "sidecardefinition.kubeblocks.io/name" - ServiceDescriptorNameLabelKey = "servicedescriptor.kubeblocks.io/name" - AddonNameLabelKey = "extensions.kubeblocks.io/addon-name" + ClusterDefLabelKey = "clusterdefinition.kubeblocks.io/name" + ShardingDefLabelKey = "shardingdefinition.kubeblocks.io/name" + LastComponentDefinitionLabelKey = "componentdefinition.kubeblocks.io/last=name" + ComponentDefinitionLabelKey = "componentdefinition.kubeblocks.io/name" + ComponentVersionLabelKey = "componentversion.kubeblocks.io/name" + SidecarDefLabelKey = "sidecardefinition.kubeblocks.io/name" + ServiceDescriptorNameLabelKey = "servicedescriptor.kubeblocks.io/name" + AddonNameLabelKey = "extensions.kubeblocks.io/addon-name" KBAppShardingNameLabelKey = "apps.kubeblocks.io/sharding-name" KBAppShardTemplateLabelKey = "apps.kubeblocks.io/shard-template" diff --git a/pkg/controller/component/suite_test.go b/pkg/controller/component/suite_test.go index 9d0376053cb..fec4e8e8d4a 100644 --- a/pkg/controller/component/suite_test.go +++ b/pkg/controller/component/suite_test.go @@ -83,6 +83,7 @@ var _ = BeforeSuite(func() { o.TimeEncoder = zapcore.ISO8601TimeEncoder })) } + viper.SetDefault(constant.EnableRBACManager, true) ctx, cancel = context.WithCancel(context.TODO()) logger = logf.FromContext(ctx).WithValues() diff --git a/pkg/controller/component/synthesize_component.go b/pkg/controller/component/synthesize_component.go index ac916c0c19a..0ca886349b2 100644 --- a/pkg/controller/component/synthesize_component.go +++ b/pkg/controller/component/synthesize_component.go @@ -26,6 +26,7 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" @@ -147,7 +148,15 @@ func BuildSynthesizedComponent(ctx context.Context, cli client.Reader, overrideComponentServices(synthesizeComp, comp) // build serviceAccountName - buildServiceAccountName(synthesizeComp) + var lastCompDef *appsv1.ComponentDefinition + if lastCmpdName := comp.Labels[constant.LastComponentDefinitionLabelKey]; lastCmpdName != "" { + lastCompDef, err = GetCompDefByName(ctx, cli, lastCmpdName) + if err != nil { + // FIXME: should we ignore not found error? + return nil, err + } + } + buildServiceAccountName(synthesizeComp, lastCompDef) // build runtimeClassName buildRuntimeClassName(synthesizeComp, comp) @@ -511,7 +520,7 @@ func synthesizeFileTemplate(comp *appsv1.Component, tpl appsv1.ComponentFileTemp } // buildServiceAccountName builds serviceAccountName for component and podSpec. -func buildServiceAccountName(synthesizeComp *SynthesizedComponent) { +func buildServiceAccountName(synthesizeComp *SynthesizedComponent, lastCompDef *appsv1.ComponentDefinition) { if synthesizeComp.ServiceAccountName != "" { synthesizeComp.PodSpec.ServiceAccountName = synthesizeComp.ServiceAccountName return @@ -519,7 +528,20 @@ func buildServiceAccountName(synthesizeComp *SynthesizedComponent) { if !viper.GetBool(constant.EnableRBACManager) { return } - synthesizeComp.ServiceAccountName = constant.GenerateDefaultServiceAccountName(synthesizeComp.CompDefName) + + saName := constant.GenerateDefaultServiceAccountName(synthesizeComp.CompDefName) + // HACK: roll back serviceaccount name if current and last compDef's roles' contents are not changed. + // The comparison should align with componentRBACTransformer. + if lastCompDef != nil { + curLifecycleActionEnabled := synthesizeComp.LifecycleActions != nil + lastLifecycleActionEnabled := lastCompDef.Spec.LifecycleActions != nil + if equality.Semantic.DeepEqual(synthesizeComp.PolicyRules, lastCompDef.Spec.PolicyRules) && + curLifecycleActionEnabled == lastLifecycleActionEnabled { + saName = constant.GenerateDefaultServiceAccountName(lastCompDef.Name) + } + } + + synthesizeComp.ServiceAccountName = saName synthesizeComp.PodSpec.ServiceAccountName = synthesizeComp.ServiceAccountName } diff --git a/pkg/controller/component/synthesize_component_test.go b/pkg/controller/component/synthesize_component_test.go index 4e0b0b924c4..a587472dd52 100644 --- a/pkg/controller/component/synthesize_component_test.go +++ b/pkg/controller/component/synthesize_component_test.go @@ -506,5 +506,32 @@ var _ = Describe("synthesized component", func() { Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-b", int32(1))) Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-c", int32(5))) }) + + It("roll back serviceaccount change", func() { + lastCompDef := compDef.DeepCopy() + lastCompDef.Name = "last-comp-def" + comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name + reader.objs = []client.Object{comp1, lastCompDef} + + synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) + Expect(err).Should(BeNil()) + Expect(synthesizedComp).ShouldNot(BeNil()) + Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(lastCompDef.Name))) + }) + + It("does not roll back serviceaccount change when roles change", func() { + lastCompDef := compDef.DeepCopy() + lastCompDef.Name = "last-comp-def" + lastCompDef.Spec.LifecycleActions = &appsv1.ComponentLifecycleActions{ + RoleProbe: &appsv1.Probe{}, + } + comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name + reader.objs = []client.Object{comp1, lastCompDef} + + synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) + Expect(err).Should(BeNil()) + Expect(synthesizedComp).ShouldNot(BeNil()) + Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(compDef.Name))) + }) }) }) From 8e2364bbc879ec13c59d4beb42c697f9a9ed62e1 Mon Sep 17 00:00:00 2001 From: Harold Cheng Date: Wed, 3 Sep 2025 14:22:28 +0800 Subject: [PATCH 2/4] wip --- .../component/transformer_component_meta.go | 1 - .../component/transformer_component_rbac.go | 33 ++++++++++++ pkg/constant/labels.go | 16 +++--- .../component/synthesize_component.go | 40 -------------- .../component/synthesize_component_test.go | 53 ++++++++++--------- pkg/controller/component/type.go | 1 - 6 files changed, 68 insertions(+), 76 deletions(-) diff --git a/controllers/apps/component/transformer_component_meta.go b/controllers/apps/component/transformer_component_meta.go index dfafcd110d9..9ae06222038 100644 --- a/controllers/apps/component/transformer_component_meta.go +++ b/controllers/apps/component/transformer_component_meta.go @@ -42,7 +42,6 @@ func (t *componentMetaTransformer) Transform(ctx graph.TransformContext, dag *gr } compDef := comp.Labels[constant.ComponentDefinitionLabelKey] if compDef != comp.Spec.CompDef { - comp.Labels[constant.LastComponentDefinitionLabelKey] = compDef comp.Labels[constant.ComponentDefinitionLabelKey] = comp.Spec.CompDef } controllerutil.AddFinalizer(comp, constant.DBComponentFinalizerName) diff --git a/controllers/apps/component/transformer_component_rbac.go b/controllers/apps/component/transformer_component_rbac.go index 9754cf5af49..a706c59cdd3 100644 --- a/controllers/apps/component/transformer_component_rbac.go +++ b/controllers/apps/component/transformer_component_rbac.go @@ -21,6 +21,7 @@ package component import ( "fmt" + "strings" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -79,6 +80,7 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr } return err } + synthesizedComp.PodSpec.ServiceAccountName = serviceAccountName } if !viper.GetBool(constant.EnableRBACManager) { transCtx.EventRecorder.Event(transCtx.Component, corev1.EventTypeNormal, EventReasonRBACManager, "RBAC manager is disabled") @@ -87,14 +89,45 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr graphCli, _ := transCtx.Client.(model.GraphClient) + needRollbackServiceAccount := func() (bool, error) { + lastName, ok := transCtx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] + if !ok { + return false, nil + } + + lastCmpdName := strings.Join(strings.Split(lastName, "-")[1:], "-") + lastCmpd, err := component.GetCompDefByName(transCtx.Context, transCtx.Client, lastCmpdName) + if err != nil { + return false, err + } + + curLifecycleActionEnabled := synthesizedComp.LifecycleActions != nil + lastLifecycleActionEnabled := lastCmpd.Spec.LifecycleActions != nil + if equality.Semantic.DeepEqual(synthesizedComp.PolicyRules, lastCmpd.Spec.PolicyRules) && + curLifecycleActionEnabled == lastLifecycleActionEnabled { + return true, nil + } + return false, nil + } + var err error if serviceAccountName == "" { serviceAccountName = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName) + rollback, err := needRollbackServiceAccount() + if err != nil { + return err + } + if rollback { + // don't change anything, just return + return nil + } // 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 } + synthesizedComp.PodSpec.ServiceAccountName = serviceAccountName + transCtx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = serviceAccountName } role, err := createOrUpdateRole(transCtx, graphCli, dag) if err != nil { diff --git a/pkg/constant/labels.go b/pkg/constant/labels.go index b11b38a961d..be6da5ad561 100644 --- a/pkg/constant/labels.go +++ b/pkg/constant/labels.go @@ -33,14 +33,14 @@ const ( // labels defined by KubeBlocks const ( - ClusterDefLabelKey = "clusterdefinition.kubeblocks.io/name" - ShardingDefLabelKey = "shardingdefinition.kubeblocks.io/name" - LastComponentDefinitionLabelKey = "componentdefinition.kubeblocks.io/last=name" - ComponentDefinitionLabelKey = "componentdefinition.kubeblocks.io/name" - ComponentVersionLabelKey = "componentversion.kubeblocks.io/name" - SidecarDefLabelKey = "sidecardefinition.kubeblocks.io/name" - ServiceDescriptorNameLabelKey = "servicedescriptor.kubeblocks.io/name" - AddonNameLabelKey = "extensions.kubeblocks.io/addon-name" + ClusterDefLabelKey = "clusterdefinition.kubeblocks.io/name" + ShardingDefLabelKey = "shardingdefinition.kubeblocks.io/name" + ComponentDefinitionLabelKey = "componentdefinition.kubeblocks.io/name" + ComponentVersionLabelKey = "componentversion.kubeblocks.io/name" + SidecarDefLabelKey = "sidecardefinition.kubeblocks.io/name" + ServiceDescriptorNameLabelKey = "servicedescriptor.kubeblocks.io/name" + AddonNameLabelKey = "extensions.kubeblocks.io/addon-name" + ComponentLastServiceAccountNameLabelKey = "component.kubeblocks.io/last-service-account-name" KBAppShardingNameLabelKey = "apps.kubeblocks.io/sharding-name" KBAppShardTemplateLabelKey = "apps.kubeblocks.io/shard-template" diff --git a/pkg/controller/component/synthesize_component.go b/pkg/controller/component/synthesize_component.go index 0ca886349b2..353064b4d04 100644 --- a/pkg/controller/component/synthesize_component.go +++ b/pkg/controller/component/synthesize_component.go @@ -26,7 +26,6 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" @@ -38,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 +106,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, FlatInstanceOrdinal: comp.Spec.FlatInstanceOrdinal, InstanceImages: make(map[string]map[string]string), @@ -147,17 +144,6 @@ func BuildSynthesizedComponent(ctx context.Context, cli client.Reader, // override componentService overrideComponentServices(synthesizeComp, comp) - // build serviceAccountName - var lastCompDef *appsv1.ComponentDefinition - if lastCmpdName := comp.Labels[constant.LastComponentDefinitionLabelKey]; lastCmpdName != "" { - lastCompDef, err = GetCompDefByName(ctx, cli, lastCmpdName) - if err != nil { - // FIXME: should we ignore not found error? - return nil, err - } - } - buildServiceAccountName(synthesizeComp, lastCompDef) - // build runtimeClassName buildRuntimeClassName(synthesizeComp, comp) @@ -519,32 +505,6 @@ func synthesizeFileTemplate(comp *appsv1.Component, tpl appsv1.ComponentFileTemp return stpl } -// buildServiceAccountName builds serviceAccountName for component and podSpec. -func buildServiceAccountName(synthesizeComp *SynthesizedComponent, lastCompDef *appsv1.ComponentDefinition) { - if synthesizeComp.ServiceAccountName != "" { - synthesizeComp.PodSpec.ServiceAccountName = synthesizeComp.ServiceAccountName - return - } - if !viper.GetBool(constant.EnableRBACManager) { - return - } - - saName := constant.GenerateDefaultServiceAccountName(synthesizeComp.CompDefName) - // HACK: roll back serviceaccount name if current and last compDef's roles' contents are not changed. - // The comparison should align with componentRBACTransformer. - if lastCompDef != nil { - curLifecycleActionEnabled := synthesizeComp.LifecycleActions != nil - lastLifecycleActionEnabled := lastCompDef.Spec.LifecycleActions != nil - if equality.Semantic.DeepEqual(synthesizeComp.PolicyRules, lastCompDef.Spec.PolicyRules) && - curLifecycleActionEnabled == lastLifecycleActionEnabled { - saName = constant.GenerateDefaultServiceAccountName(lastCompDef.Name) - } - } - - synthesizeComp.ServiceAccountName = saName - synthesizeComp.PodSpec.ServiceAccountName = synthesizeComp.ServiceAccountName -} - func buildRuntimeClassName(synthesizeComp *SynthesizedComponent, comp *appsv1.Component) { if comp.Spec.RuntimeClassName == nil { return diff --git a/pkg/controller/component/synthesize_component_test.go b/pkg/controller/component/synthesize_component_test.go index a587472dd52..be31404845f 100644 --- a/pkg/controller/component/synthesize_component_test.go +++ b/pkg/controller/component/synthesize_component_test.go @@ -507,31 +507,32 @@ var _ = Describe("synthesized component", func() { Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-c", int32(5))) }) - It("roll back serviceaccount change", func() { - lastCompDef := compDef.DeepCopy() - lastCompDef.Name = "last-comp-def" - comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name - reader.objs = []client.Object{comp1, lastCompDef} - - synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) - Expect(err).Should(BeNil()) - Expect(synthesizedComp).ShouldNot(BeNil()) - Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(lastCompDef.Name))) - }) - - It("does not roll back serviceaccount change when roles change", func() { - lastCompDef := compDef.DeepCopy() - lastCompDef.Name = "last-comp-def" - lastCompDef.Spec.LifecycleActions = &appsv1.ComponentLifecycleActions{ - RoleProbe: &appsv1.Probe{}, - } - comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name - reader.objs = []client.Object{comp1, lastCompDef} - - synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) - Expect(err).Should(BeNil()) - Expect(synthesizedComp).ShouldNot(BeNil()) - Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(compDef.Name))) - }) + // FIXME! + // It("roll back serviceaccount change", func() { + // lastCompDef := compDef.DeepCopy() + // lastCompDef.Name = "last-comp-def" + // comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name + // reader.objs = []client.Object{comp1, lastCompDef} + + // synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) + // Expect(err).Should(BeNil()) + // Expect(synthesizedComp).ShouldNot(BeNil()) + // Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(lastCompDef.Name))) + // }) + + // It("does not roll back serviceaccount change when roles change", func() { + // lastCompDef := compDef.DeepCopy() + // lastCompDef.Name = "last-comp-def" + // lastCompDef.Spec.LifecycleActions = &appsv1.ComponentLifecycleActions{ + // RoleProbe: &appsv1.Probe{}, + // } + // comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name + // reader.objs = []client.Object{comp1, lastCompDef} + + // synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) + // Expect(err).Should(BeNil()) + // Expect(synthesizedComp).ShouldNot(BeNil()) + // Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(compDef.Name))) + // }) }) }) diff --git a/pkg/controller/component/type.go b/pkg/controller/component/type.go index e1c848df546..17d574c053b 100644 --- a/pkg/controller/component/type.go +++ b/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 From f9a98f18e202e896ef2ee7ef98ca002de6c4d6d0 Mon Sep 17 00:00:00 2001 From: Harold Cheng Date: Wed, 3 Sep 2025 15:16:25 +0800 Subject: [PATCH 3/4] add tests --- .../apps/component/component_controller.go | 4 +- .../component/transformer_component_rbac.go | 50 +++++++------ .../transformer_component_rbac_test.go | 73 +++++++++++++++++++ pkg/controller/component/suite_test.go | 1 - .../component/synthesize_component_test.go | 28 ------- 5 files changed, 103 insertions(+), 53 deletions(-) diff --git a/controllers/apps/component/component_controller.go b/controllers/apps/component/component_controller.go index c50114b06e7..12939692e8e 100644 --- a/controllers/apps/component/component_controller.go +++ b/controllers/apps/component/component_controller.go @@ -164,10 +164,10 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( &componentReloadSidecarTransformer{Client: r.Client}, // handle restore before workloads transform &componentRestoreTransformer{Client: r.Client}, + // handle RBAC for component workloads, it should be put before workload transformer + &componentRBACTransformer{}, // handle the component workload &componentWorkloadTransformer{Client: r.Client}, - // handle RBAC for component workloads - &componentRBACTransformer{}, // handle component postProvision lifecycle action &componentPostProvisionTransformer{}, // update component status diff --git a/controllers/apps/component/transformer_component_rbac.go b/controllers/apps/component/transformer_component_rbac.go index a706c59cdd3..1de6b7ac515 100644 --- a/controllers/apps/component/transformer_component_rbac.go +++ b/controllers/apps/component/transformer_component_rbac.go @@ -53,6 +53,7 @@ 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) @@ -89,35 +90,15 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr graphCli, _ := transCtx.Client.(model.GraphClient) - needRollbackServiceAccount := func() (bool, error) { - lastName, ok := transCtx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] - if !ok { - return false, nil - } - - lastCmpdName := strings.Join(strings.Split(lastName, "-")[1:], "-") - lastCmpd, err := component.GetCompDefByName(transCtx.Context, transCtx.Client, lastCmpdName) - if err != nil { - return false, err - } - - curLifecycleActionEnabled := synthesizedComp.LifecycleActions != nil - lastLifecycleActionEnabled := lastCmpd.Spec.LifecycleActions != nil - if equality.Semantic.DeepEqual(synthesizedComp.PolicyRules, lastCmpd.Spec.PolicyRules) && - curLifecycleActionEnabled == lastLifecycleActionEnabled { - return true, nil - } - return false, nil - } - var err error if serviceAccountName == "" { serviceAccountName = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName) - rollback, err := needRollbackServiceAccount() + rollback, err := needRollbackServiceAccount(transCtx) if err != nil { return err } if rollback { + transCtx.EventRecorder.Event(transCtx.Component, corev1.EventTypeNormal, EventReasonServiceAccountRollback, "Change to serviceaccount has rolled back to prevent pod restart") // don't change anything, just return return nil } @@ -158,6 +139,31 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr return nil } +func needRollbackServiceAccount(transCtx *componentTransformContext) (bool, error) { + lastName, ok := transCtx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] + if !ok { + return false, nil + } + + lastCmpdName := strings.Join(strings.Split(lastName, "-")[1:], "-") + if lastCmpdName == transCtx.CompDef.Name { + return false, nil + } + + lastCmpd, err := component.GetCompDefByName(transCtx.Context, transCtx.Client, lastCmpdName) + if err != nil { + return false, err + } + + curLifecycleActionEnabled := transCtx.SynthesizeComponent.LifecycleActions != nil + lastLifecycleActionEnabled := lastCmpd.Spec.LifecycleActions != nil + if equality.Semantic.DeepEqual(transCtx.SynthesizeComponent.PolicyRules, lastCmpd.Spec.PolicyRules) && + curLifecycleActionEnabled == lastLifecycleActionEnabled { + return true, nil + } + return false, nil +} + func (t *componentRBACTransformer) rbacInstanceAssistantObjects(graphCli model.GraphClient, dag *graph.DAG, objs []client.Object) { itsList := graphCli.FindAll(dag, &workloads.InstanceSet{}) for _, itsObj := range itsList { diff --git a/controllers/apps/component/transformer_component_rbac_test.go b/controllers/apps/component/transformer_component_rbac_test.go index ad214b33f9c..471fc4626d4 100644 --- a/controllers/apps/component/transformer_component_rbac_test.go +++ b/controllers/apps/component/transformer_component_rbac_test.go @@ -243,6 +243,79 @@ var _ = Describe("object rbac transformer test.", func() { Expect(reflect.DeepEqual(rb.RoleRef, cmpdRoleBinding.RoleRef)).To(BeTrue()) }) }) + + Context("tests serviceaccount rollback", func() { + It("tests needRollbackServiceAccount basic cases", func() { + init(true, false) + ctx := transCtx.(*componentTransformContext) + + By("create another cmpd") + anotherCompDef := testapps.NewComponentDefinitionFactory(compDefName). + WithRandomName(). + SetDefaultSpec(). + Create(&testCtx). + GetObject() + + // Case 1: No label, should return false + needRollback, err := needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeFalse()) + + // Case 2: With label but component definitions are the same + ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-" + compDefObj.Name + needRollback, err = needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeFalse()) + + // Case 3: Different cmpd, same spec + ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-" + anotherCompDef.Name + needRollback, err = needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeTrue()) + + // Case 4: Different cmpd, different policy rules + Expect(testapps.ChangeObj(&testCtx, anotherCompDef, func(cmpd *appsv1.ComponentDefinition) { + cmpd.Spec.PolicyRules = []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get"}, + }, + } + })).Should(Succeed()) + needRollback, err = needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeFalse()) + + // Case 5: Different cmpd, different lifecycle action + Expect(testapps.ChangeObj(&testCtx, anotherCompDef, func(cmpd *appsv1.ComponentDefinition) { + cmpd.Spec.PolicyRules = nil + cmpd.Spec.LifecycleActions = nil + })).Should(Succeed()) + needRollback, err = needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeFalse()) + }) + + It("tests needRollbackServiceAccount error cases", func() { + init(false, false) + ctx := transCtx.(*componentTransformContext) + + // Case 1: Invalid label format + ctx.Component.Labels = map[string]string{ + constant.ComponentLastServiceAccountNameLabelKey: "invalid-format", + } + _, err := needRollbackServiceAccount(ctx) + Expect(err).Should(HaveOccurred()) + + // Case 2: Component definition not found + ctx.Component.Labels = map[string]string{ + constant.ComponentLastServiceAccountNameLabelKey: "kb-non-existent", + } + _, err = needRollbackServiceAccount(ctx) + Expect(err).Should(HaveOccurred()) + }) + }) }) func mockDAG(graphCli model.GraphClient, comp *appsv1.Component) *graph.DAG { diff --git a/pkg/controller/component/suite_test.go b/pkg/controller/component/suite_test.go index fec4e8e8d4a..9d0376053cb 100644 --- a/pkg/controller/component/suite_test.go +++ b/pkg/controller/component/suite_test.go @@ -83,7 +83,6 @@ var _ = BeforeSuite(func() { o.TimeEncoder = zapcore.ISO8601TimeEncoder })) } - viper.SetDefault(constant.EnableRBACManager, true) ctx, cancel = context.WithCancel(context.TODO()) logger = logf.FromContext(ctx).WithValues() diff --git a/pkg/controller/component/synthesize_component_test.go b/pkg/controller/component/synthesize_component_test.go index be31404845f..4e0b0b924c4 100644 --- a/pkg/controller/component/synthesize_component_test.go +++ b/pkg/controller/component/synthesize_component_test.go @@ -506,33 +506,5 @@ var _ = Describe("synthesized component", func() { Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-b", int32(1))) Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-c", int32(5))) }) - - // FIXME! - // It("roll back serviceaccount change", func() { - // lastCompDef := compDef.DeepCopy() - // lastCompDef.Name = "last-comp-def" - // comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name - // reader.objs = []client.Object{comp1, lastCompDef} - - // synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) - // Expect(err).Should(BeNil()) - // Expect(synthesizedComp).ShouldNot(BeNil()) - // Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(lastCompDef.Name))) - // }) - - // It("does not roll back serviceaccount change when roles change", func() { - // lastCompDef := compDef.DeepCopy() - // lastCompDef.Name = "last-comp-def" - // lastCompDef.Spec.LifecycleActions = &appsv1.ComponentLifecycleActions{ - // RoleProbe: &appsv1.Probe{}, - // } - // comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name - // reader.objs = []client.Object{comp1, lastCompDef} - - // synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1) - // Expect(err).Should(BeNil()) - // Expect(synthesizedComp).ShouldNot(BeNil()) - // Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(compDef.Name))) - // }) }) }) From 96045af3454fb571469b165143cd6cd89ce0bc6a Mon Sep 17 00:00:00 2001 From: Harold Cheng Date: Thu, 4 Sep 2025 14:54:16 +0800 Subject: [PATCH 4/4] ignore cmpd not found --- .../component/transformer_component_rbac.go | 3 +- .../transformer_component_rbac_test.go | 37 ++++++------------- 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/controllers/apps/component/transformer_component_rbac.go b/controllers/apps/component/transformer_component_rbac.go index 1de6b7ac515..b1037f1eef2 100644 --- a/controllers/apps/component/transformer_component_rbac.go +++ b/controllers/apps/component/transformer_component_rbac.go @@ -145,6 +145,7 @@ func needRollbackServiceAccount(transCtx *componentTransformContext) (bool, erro return false, nil } + // this logic is in line with GenerateDefaultServiceAccountName lastCmpdName := strings.Join(strings.Split(lastName, "-")[1:], "-") if lastCmpdName == transCtx.CompDef.Name { return false, nil @@ -152,7 +153,7 @@ func needRollbackServiceAccount(transCtx *componentTransformContext) (bool, erro lastCmpd, err := component.GetCompDefByName(transCtx.Context, transCtx.Client, lastCmpdName) if err != nil { - return false, err + return false, client.IgnoreNotFound(err) } curLifecycleActionEnabled := transCtx.SynthesizeComponent.LifecycleActions != nil diff --git a/controllers/apps/component/transformer_component_rbac_test.go b/controllers/apps/component/transformer_component_rbac_test.go index 471fc4626d4..6fbb6299166 100644 --- a/controllers/apps/component/transformer_component_rbac_test.go +++ b/controllers/apps/component/transformer_component_rbac_test.go @@ -245,7 +245,7 @@ var _ = Describe("object rbac transformer test.", func() { }) Context("tests serviceaccount rollback", func() { - It("tests needRollbackServiceAccount basic cases", func() { + It("tests needRollbackServiceAccount", func() { init(true, false) ctx := transCtx.(*componentTransformContext) @@ -256,24 +256,30 @@ var _ = Describe("object rbac transformer test.", func() { Create(&testCtx). GetObject() - // Case 1: No label, should return false + // Case: No label, should return false needRollback, err := needRollbackServiceAccount(ctx) Expect(err).Should(BeNil()) Expect(needRollback).Should(BeFalse()) - // Case 2: With label but component definitions are the same + // Case: With label but component definitions are the same ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-" + compDefObj.Name needRollback, err = needRollbackServiceAccount(ctx) Expect(err).Should(BeNil()) Expect(needRollback).Should(BeFalse()) - // Case 3: Different cmpd, same spec + // Case: with a non-exist cmpd + ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-non-existent" + needRollback, err = needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeFalse()) + + // Case: Different cmpd, same spec ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-" + anotherCompDef.Name needRollback, err = needRollbackServiceAccount(ctx) Expect(err).Should(BeNil()) Expect(needRollback).Should(BeTrue()) - // Case 4: Different cmpd, different policy rules + // Case: Different cmpd, different policy rules Expect(testapps.ChangeObj(&testCtx, anotherCompDef, func(cmpd *appsv1.ComponentDefinition) { cmpd.Spec.PolicyRules = []rbacv1.PolicyRule{ { @@ -287,7 +293,7 @@ var _ = Describe("object rbac transformer test.", func() { Expect(err).Should(BeNil()) Expect(needRollback).Should(BeFalse()) - // Case 5: Different cmpd, different lifecycle action + // Case: Different cmpd, different lifecycle action Expect(testapps.ChangeObj(&testCtx, anotherCompDef, func(cmpd *appsv1.ComponentDefinition) { cmpd.Spec.PolicyRules = nil cmpd.Spec.LifecycleActions = nil @@ -296,25 +302,6 @@ var _ = Describe("object rbac transformer test.", func() { Expect(err).Should(BeNil()) Expect(needRollback).Should(BeFalse()) }) - - It("tests needRollbackServiceAccount error cases", func() { - init(false, false) - ctx := transCtx.(*componentTransformContext) - - // Case 1: Invalid label format - ctx.Component.Labels = map[string]string{ - constant.ComponentLastServiceAccountNameLabelKey: "invalid-format", - } - _, err := needRollbackServiceAccount(ctx) - Expect(err).Should(HaveOccurred()) - - // Case 2: Component definition not found - ctx.Component.Labels = map[string]string{ - constant.ComponentLastServiceAccountNameLabelKey: "kb-non-existent", - } - _, err = needRollbackServiceAccount(ctx) - Expect(err).Should(HaveOccurred()) - }) }) })