Fix: fix panic if trait tries to patch an invalid workload like terraform (#5329)

Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com>

Signed-off-by: FogDong <dongtianxin.tx@alibaba-inc.com>

pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go

@@ -257,7 +257,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		}
 	case common.WorkflowStateSkipping:
 		logCtx.Info("Skip this reconcile")
-		return ctrl.Result{}, nil
+		return r.result(nil).requeue(wf.GetBackoffWaitTime()).ret()
 	}
 
 	var phase = common.ApplicationRunning

pkg/cue/definition/template.go

@@ -292,6 +292,7 @@ func NewTraitAbstractEngine(name string, pd *packages.PackageDiscover) AbstractE
 }
 
 // Complete do trait definition's rendering
+// nolint:gocyclo
 func (td *traitDef) Complete(ctx process.Context, abstractTemplate string, params interface{}) error {
 	bi := build.NewContext().NewInstance("", nil)
 	if err := bi.AddFile("-", abstractTemplate); err != nil {
@@ -360,6 +361,9 @@ func (td *traitDef) Complete(ctx process.Context, abstractTemplate string, param
 		if err != nil {
 			return errors.WithMessagef(err, "invalid patch of trait %s", td.name)
 		}
+		if base == nil {
+			return fmt.Errorf("patch trait %s into an invalid workload", td.name)
+		}
 		if err := base.Unify(p, sets.CreateUnifyOptionsForPatcher(patcher)...); err != nil {
 			return errors.WithMessagef(err, "invalid patch trait %s into workload", td.name)
 		}

pkg/cue/definition/template_test.go

@@ -1266,6 +1266,38 @@ outputs: abc :{
 	}
 }
 
+func TestTraitCompleteErrorCases(t *testing.T) {
+	cases := map[string]struct {
+		ctx       process.Context
+		traitName string
+		template  string
+		params    map[string]interface{}
+		err       string
+	}{
+		"patch trait": {
+			ctx: process.NewContext(process.ContextData{}),
+			template: `
+patch: {
+      // +patchKey=name
+      spec: template: spec: containers: [parameter]
+}
+
+parameter: {
+	name: string
+	image: string
+	command?: [...string]
+}`,
+			err: "patch trait patch trait into an invalid workload",
+		},
+	}
+	for k, v := range cases {
+		td := NewTraitAbstractEngine(k, &packages.PackageDiscover{})
+		err := td.Complete(v.ctx, v.template, v.params)
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), v.err)
+	}
+}
+
 func TestCheckHealth(t *testing.T) {
 	cases := map[string]struct {
 		tpContext  map[string]interface{}

pkg/workflow/workflow.go

@@ -149,13 +149,6 @@ func (w *workflow) ExecuteSteps(ctx monitorContext.Context, appRev *oamcore.Appl
 		return common.WorkflowStateSucceeded, nil
 	}
 
-	if cacheValue, ok := StepStatusCache.Load(cacheKey); ok {
-		// handle cache resource
-		if len(wfStatus.Steps) < cacheValue.(int) {
-			return common.WorkflowStateSkipping, nil
-		}
-	}
-
 	wfCtx, err := w.makeContext(w.app.Name)
 	if err != nil {
 		ctx.Error(err, "make context")
@@ -164,6 +157,13 @@ func (w *workflow) ExecuteSteps(ctx monitorContext.Context, appRev *oamcore.Appl
 	}
 	w.wfCtx = wfCtx
 
+	if cacheValue, ok := StepStatusCache.Load(cacheKey); ok {
+		// handle cache resource
+		if len(wfStatus.Steps) < cacheValue.(int) {
+			return common.WorkflowStateSkipping, nil
+		}
+	}
+
 	e := newEngine(ctx, wfCtx, w, wfStatus)
 
 	err = e.Run(taskRunners, w.dagMode)

references/cli/addon.go

@@ -1071,12 +1071,12 @@ func hasAddon(addons []*pkgaddon.UIData, name string) bool {
 	return false
 }
 
-func transClusters(cstr string) []string {
+func transClusters(cstr string) []interface{} {
 	if len(cstr) == 0 {
 		return nil
 	}
 	cstr = strings.TrimPrefix(strings.TrimSuffix(cstr, "}"), "{")
-	var clusterL []string
+	var clusterL []interface{}
 	clusterList := strings.Split(cstr, ",")
 	for _, v := range clusterList {
 		clusterL = append(clusterL, strings.TrimSpace(v))

references/cli/addon_test.go

@@ -192,19 +192,19 @@ func TestAddonUpgradeCmdWithErrLocalPath(t *testing.T) {
 func TestTransCluster(t *testing.T) {
 	testcase := []struct {
 		str string
-		res []string
+		res []interface{}
 	}{
 		{
 			str: "{cluster1, cluster2}",
-			res: []string{"cluster1", "cluster2"},
+			res: []interface{}{"cluster1", "cluster2"},
 		},
 		{
 			str: "{cluster1,cluster2}",
-			res: []string{"cluster1", "cluster2"},
+			res: []interface{}{"cluster1", "cluster2"},
 		},
 		{
 			str: "{cluster1,  cluster2   }",
-			res: []string{"cluster1", "cluster2"},
+			res: []interface{}{"cluster1", "cluster2"},
 		},
 	}
 	for _, s := range testcase {