diff --git a/controllers/apps/component/component_controller.go b/controllers/apps/component/component_controller.go index 971d0cca229..3153847aa05 100644 --- a/controllers/apps/component/component_controller.go +++ b/controllers/apps/component/component_controller.go @@ -168,10 +168,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 diff --git a/controllers/apps/component/transformer_component_rbac.go b/controllers/apps/component/transformer_component_rbac.go index 9d02ac27a0b..75c8123bcd3 100644 --- a/controllers/apps/component/transformer_component_rbac.go +++ b/controllers/apps/component/transformer_component_rbac.go @@ -20,7 +20,9 @@ along with this program. If not, see . package component import ( + "encoding/json" "fmt" + "hash/fnv" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -28,6 +30,7 @@ import ( "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 @@ -119,6 +148,32 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr return nil } +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("", "") diff --git a/controllers/apps/component/transformer_component_rbac_test.go b/controllers/apps/component/transformer_component_rbac_test.go index ad214b33f9c..aaacb57304f 100644 --- a/controllers/apps/component/transformer_component_rbac_test.go +++ b/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 +} diff --git a/pkg/constant/annotations.go b/pkg/constant/annotations.go index f86d2d6a5ad..9011a42ad45 100644 --- a/pkg/constant/annotations.go +++ b/pkg/constant/annotations.go @@ -49,6 +49,9 @@ const ( // NodeSelectorOnceAnnotationKey adds nodeSelector in podSpec for one pod exactly once NodeSelectorOnceAnnotationKey = "workloads.kubeblocks.io/node-selector-once" + + ComponentLastServiceAccountNameAnnotationKey = "component.kubeblocks.io/last-service-account-name" + ComponentLastServiceAccountRuleHashAnnotationKey = "component.kubeblocks.io/last-service-account-rule-hash" ) const ( diff --git a/pkg/controller/component/synthesize_component.go b/pkg/controller/component/synthesize_component.go index f727d625cba..b0a10a4ab2f 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, OfflineInstances: comp.Spec.OfflineInstances, DisableExporter: comp.Spec.DisableExporter, @@ -142,9 +140,6 @@ func BuildSynthesizedComponent(ctx context.Context, cli client.Reader, // override componentService overrideComponentServices(synthesizeComp, comp) - // build serviceAccountName - buildServiceAccountName(synthesizeComp) - // build runtimeClassName buildRuntimeClassName(synthesizeComp, comp) @@ -480,19 +475,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 3221b6dcf9a..8f6799655a3 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