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 5bd2a221177..b1037f1eef2 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" @@ -45,11 +46,14 @@ 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{} const EventReasonRBACManager = "RBACManager" +const EventReasonServiceAccountRollback = "ServiceAccountRollback" func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *graph.DAG) error { transCtx, _ := ctx.(*componentTransformContext) @@ -77,6 +81,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") @@ -88,11 +93,22 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr var err error if serviceAccountName == "" { serviceAccountName = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName) + 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 + } // 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 { @@ -123,6 +139,32 @@ 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 + } + + // this logic is in line with GenerateDefaultServiceAccountName + 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, client.IgnoreNotFound(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..6fbb6299166 100644 --- a/controllers/apps/component/transformer_component_rbac_test.go +++ b/controllers/apps/component/transformer_component_rbac_test.go @@ -243,6 +243,66 @@ var _ = Describe("object rbac transformer test.", func() { Expect(reflect.DeepEqual(rb.RoleRef, cmpdRoleBinding.RoleRef)).To(BeTrue()) }) }) + + Context("tests serviceaccount rollback", func() { + It("tests needRollbackServiceAccount", func() { + init(true, false) + ctx := transCtx.(*componentTransformContext) + + By("create another cmpd") + anotherCompDef := testapps.NewComponentDefinitionFactory(compDefName). + WithRandomName(). + SetDefaultSpec(). + Create(&testCtx). + GetObject() + + // Case: No label, should return false + needRollback, err := needRollbackServiceAccount(ctx) + Expect(err).Should(BeNil()) + Expect(needRollback).Should(BeFalse()) + + // 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: 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: 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: 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()) + }) + }) }) func mockDAG(graphCli model.GraphClient, comp *appsv1.Component) *graph.DAG { diff --git a/pkg/constant/labels.go b/pkg/constant/labels.go index e1532f2b09e..be6da5ad561 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" + 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 ac916c0c19a..353064b4d04 100644 --- a/pkg/controller/component/synthesize_component.go +++ b/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 ( @@ -107,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), @@ -146,9 +144,6 @@ func BuildSynthesizedComponent(ctx context.Context, cli client.Reader, // override componentService overrideComponentServices(synthesizeComp, comp) - // build serviceAccountName - buildServiceAccountName(synthesizeComp) - // build runtimeClassName buildRuntimeClassName(synthesizeComp, comp) @@ -510,19 +505,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 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