Skip to content

Commit a8ea87f

Browse files
cjc7373cjc7373
committed
add tests
1 parent 8e2364b commit a8ea87f

File tree

4 files changed

+103
-52
lines changed

4 files changed

+103
-52
lines changed

‎controllers/apps/component/component_controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,10 +164,10 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
164164
&componentReloadSidecarTransformer{Client: r.Client},
165165
// handle restore before workloads transform
166166
&componentRestoreTransformer{Client: r.Client},
167+
// handle RBAC for component workloads, it should be put before workload transformer
168+
&componentRBACTransformer{},
167169
// handle the component workload
168170
&componentWorkloadTransformer{Client: r.Client},
169-
// handle RBAC for component workloads
170-
&componentRBACTransformer{},
171171
// handle component postProvision lifecycle action
172172
&componentPostProvisionTransformer{},
173173
// update component status

‎controllers/apps/component/transformer_component_rbac.go‎

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ type componentRBACTransformer struct{}
5353
var _ graph.Transformer = &componentRBACTransformer{}
5454

5555
const EventReasonRBACManager = "RBACManager"
56+
const EventReasonServiceAccountRollback = "ServiceAccountRollback"
5657

5758
func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *graph.DAG) error {
5859
transCtx, _ := ctx.(*componentTransformContext)
@@ -89,35 +90,15 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr
8990

9091
graphCli, _ := transCtx.Client.(model.GraphClient)
9192

92-
needRollbackServiceAccount := func() (bool, error) {
93-
lastName, ok := transCtx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey]
94-
if !ok {
95-
return false, nil
96-
}
97-
98-
lastCmpdName := strings.Join(strings.Split(lastName, "-")[1:], "-")
99-
lastCmpd, err := component.GetCompDefByName(transCtx.Context, transCtx.Client, lastCmpdName)
100-
if err != nil {
101-
return false, err
102-
}
103-
104-
curLifecycleActionEnabled := synthesizedComp.LifecycleActions != nil
105-
lastLifecycleActionEnabled := lastCmpd.Spec.LifecycleActions != nil
106-
if equality.Semantic.DeepEqual(synthesizedComp.PolicyRules, lastCmpd.Spec.PolicyRules) &&
107-
curLifecycleActionEnabled == lastLifecycleActionEnabled {
108-
return true, nil
109-
}
110-
return false, nil
111-
}
112-
11393
var err error
11494
if serviceAccountName == "" {
11595
serviceAccountName = constant.GenerateDefaultServiceAccountName(synthesizedComp.CompDefName)
116-
rollback, err := needRollbackServiceAccount()
96+
rollback, err := needRollbackServiceAccount(transCtx)
11797
if err != nil {
11898
return err
11999
}
120100
if rollback {
101+
transCtx.EventRecorder.Event(transCtx.Component, corev1.EventTypeNormal, EventReasonServiceAccountRollback, "Change to serviceaccount has rolled back to prevent pod restart")
121102
// don't change anything, just return
122103
return nil
123104
}
@@ -158,6 +139,31 @@ func (t *componentRBACTransformer) Transform(ctx graph.TransformContext, dag *gr
158139
return nil
159140
}
160141

142+
func needRollbackServiceAccount(transCtx *componentTransformContext) (bool, error) {
143+
lastName, ok := transCtx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey]
144+
if !ok {
145+
return false, nil
146+
}
147+
148+
lastCmpdName := strings.Join(strings.Split(lastName, "-")[1:], "-")
149+
if lastCmpdName == transCtx.CompDef.Name {
150+
return false, nil
151+
}
152+
153+
lastCmpd, err := component.GetCompDefByName(transCtx.Context, transCtx.Client, lastCmpdName)
154+
if err != nil {
155+
return false, err
156+
}
157+
158+
curLifecycleActionEnabled := transCtx.SynthesizeComponent.LifecycleActions != nil
159+
lastLifecycleActionEnabled := lastCmpd.Spec.LifecycleActions != nil
160+
if equality.Semantic.DeepEqual(transCtx.SynthesizeComponent.PolicyRules, lastCmpd.Spec.PolicyRules) &&
161+
curLifecycleActionEnabled == lastLifecycleActionEnabled {
162+
return true, nil
163+
}
164+
return false, nil
165+
}
166+
161167
func (t *componentRBACTransformer) rbacInstanceAssistantObjects(graphCli model.GraphClient, dag *graph.DAG, objs []client.Object) {
162168
itsList := graphCli.FindAll(dag, &workloads.InstanceSet{})
163169
for _, itsObj := range itsList {

‎controllers/apps/component/transformer_component_rbac_test.go‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,79 @@ var _ = Describe("object rbac transformer test.", func() {
243243
Expect(reflect.DeepEqual(rb.RoleRef, cmpdRoleBinding.RoleRef)).To(BeTrue())
244244
})
245245
})
246+
247+
Context("tests serviceaccount rollback", func() {
248+
It("tests needRollbackServiceAccount basic cases", func() {
249+
init(true, false)
250+
ctx := transCtx.(*componentTransformContext)
251+
252+
By("create another cmpd")
253+
anotherCompDef := testapps.NewComponentDefinitionFactory(compDefName).
254+
WithRandomName().
255+
SetDefaultSpec().
256+
Create(&testCtx).
257+
GetObject()
258+
259+
// Case 1: No label, should return false
260+
needRollback, err := needRollbackServiceAccount(ctx)
261+
Expect(err).Should(BeNil())
262+
Expect(needRollback).Should(BeFalse())
263+
264+
// Case 2: With label but component definitions are the same
265+
ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-" + compDefObj.Name
266+
needRollback, err = needRollbackServiceAccount(ctx)
267+
Expect(err).Should(BeNil())
268+
Expect(needRollback).Should(BeFalse())
269+
270+
// Case 3: Different cmpd, same spec
271+
ctx.Component.Labels[constant.ComponentLastServiceAccountNameLabelKey] = "kb-" + anotherCompDef.Name
272+
needRollback, err = needRollbackServiceAccount(ctx)
273+
Expect(err).Should(BeNil())
274+
Expect(needRollback).Should(BeTrue())
275+
276+
// Case 4: Different cmpd, different policy rules
277+
Expect(testapps.ChangeObj(&testCtx, anotherCompDef, func(cmpd *appsv1.ComponentDefinition) {
278+
cmpd.Spec.PolicyRules = []rbacv1.PolicyRule{
279+
{
280+
APIGroups: []string{""},
281+
Resources: []string{"pods"},
282+
Verbs: []string{"get"},
283+
},
284+
}
285+
})).Should(Succeed())
286+
needRollback, err = needRollbackServiceAccount(ctx)
287+
Expect(err).Should(BeNil())
288+
Expect(needRollback).Should(BeFalse())
289+
290+
// Case 5: Different cmpd, different lifecycle action
291+
Expect(testapps.ChangeObj(&testCtx, anotherCompDef, func(cmpd *appsv1.ComponentDefinition) {
292+
cmpd.Spec.PolicyRules = nil
293+
cmpd.Spec.LifecycleActions = nil
294+
})).Should(Succeed())
295+
needRollback, err = needRollbackServiceAccount(ctx)
296+
Expect(err).Should(BeNil())
297+
Expect(needRollback).Should(BeFalse())
298+
})
299+
300+
It("tests needRollbackServiceAccount error cases", func() {
301+
init(false, false)
302+
ctx := transCtx.(*componentTransformContext)
303+
304+
// Case 1: Invalid label format
305+
ctx.Component.Labels = map[string]string{
306+
constant.ComponentLastServiceAccountNameLabelKey: "invalid-format",
307+
}
308+
_, err := needRollbackServiceAccount(ctx)
309+
Expect(err).Should(HaveOccurred())
310+
311+
// Case 2: Component definition not found
312+
ctx.Component.Labels = map[string]string{
313+
constant.ComponentLastServiceAccountNameLabelKey: "kb-non-existent",
314+
}
315+
_, err = needRollbackServiceAccount(ctx)
316+
Expect(err).Should(HaveOccurred())
317+
})
318+
})
246319
})
247320

248321
func mockDAG(graphCli model.GraphClient, comp *appsv1.Component) *graph.DAG {

‎pkg/controller/component/synthesize_component_test.go‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -506,33 +506,5 @@ var _ = Describe("synthesized component", func() {
506506
Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-b", int32(1)))
507507
Expect(synthesizedComp.CompDef2CompCnt).Should(HaveKeyWithValue("test-compdef-c", int32(5)))
508508
})
509-
510-
// FIXME!
511-
// It("roll back serviceaccount change", func() {
512-
// lastCompDef := compDef.DeepCopy()
513-
// lastCompDef.Name = "last-comp-def"
514-
// comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name
515-
// reader.objs = []client.Object{comp1, lastCompDef}
516-
517-
// synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1)
518-
// Expect(err).Should(BeNil())
519-
// Expect(synthesizedComp).ShouldNot(BeNil())
520-
// Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(lastCompDef.Name)))
521-
// })
522-
523-
// It("does not roll back serviceaccount change when roles change", func() {
524-
// lastCompDef := compDef.DeepCopy()
525-
// lastCompDef.Name = "last-comp-def"
526-
// lastCompDef.Spec.LifecycleActions = &appsv1.ComponentLifecycleActions{
527-
// RoleProbe: &appsv1.Probe{},
528-
// }
529-
// comp1.Labels[constant.LastComponentDefinitionLabelKey] = lastCompDef.Name
530-
// reader.objs = []client.Object{comp1, lastCompDef}
531-
532-
// synthesizedComp, err := BuildSynthesizedComponent(ctx, reader, compDef, comp1)
533-
// Expect(err).Should(BeNil())
534-
// Expect(synthesizedComp).ShouldNot(BeNil())
535-
// Expect(synthesizedComp.ServiceAccountName).Should(Equal(constant.GenerateDefaultServiceAccountName(compDef.Name)))
536-
// })
537509
})
538510
})

0 commit comments

Comments
 (0)