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>

.github/workflows/apiserver-test.yaml

@@ -104,7 +104,7 @@ jobs:
           name: codecov-umbrella
 
   apiserver-e2e-tests:
-    runs-on: aliyun
+    runs-on: aliyun-legacy
     needs: [ detect-noop,set-k8s-matrix ]
     if: needs.detect-noop.outputs.noop != 'true'
     strategy:

.github/workflows/e2e-multicluster-test.yml

@@ -52,7 +52,7 @@ jobs:
 
 
   e2e-multi-cluster-tests:
-    runs-on: aliyun
+    runs-on: aliyun-legacy
     needs: [ detect-noop,set-k8s-matrix ]
     if: needs.detect-noop.outputs.noop != 'true'
     strategy:

.github/workflows/e2e-rollout-test.yml

@@ -51,7 +51,7 @@ jobs:
           fi
 
   e2e-rollout-tests:
-    runs-on: aliyun
+    runs-on: aliyun-legacy
     needs: [ detect-noop,set-k8s-matrix ]
     if: needs.detect-noop.outputs.noop != 'true'
     strategy:

.github/workflows/e2e-test.yml

@@ -51,7 +51,7 @@ jobs:
           fi
 
   e2e-tests:
-    runs-on: aliyun
+    runs-on: aliyun-legacy
     needs: [ detect-noop,set-k8s-matrix ]
     if: needs.detect-noop.outputs.noop != 'true'
     strategy:

.github/workflows/go.yml

@@ -97,7 +97,7 @@ jobs:
           version: ${{ env.GOLANGCI_VERSION }}
 
   check-diff:
-    runs-on: aliyun
+    runs-on: aliyun-legacy
     needs: detect-noop
     if: needs.detect-noop.outputs.noop != 'true'
 

.github/workflows/timed-task.yml

@@ -4,7 +4,7 @@ on:
     - cron: '* * * * *'
 jobs:
   clean-image:
-    runs-on: aliyun
+    runs-on: aliyun-legacy
     steps:
       - name: Cleanup image
         run: docker image prune -f

pkg/addon/helper_test.go

@@ -86,7 +86,6 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() {
 				Expect(res).To(HaveLen(1))
 				Expect(res[0].Name).To(Equal("velaux"))
 				Expect(res[0].InstallPackage).ToNot(BeNil())
-				Expect(res[0].APISchema).ToNot(BeNil())
 			})
 			It("should return one valid result, matching one registry", func() {
 				res, err := FindWholeAddonPackagesFromRegistry(context.Background(), k8sClient, []string{"velaux"}, []string{"KubeVela"})
@@ -94,7 +93,6 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() {
 				Expect(res).To(HaveLen(1))
 				Expect(res[0].Name).To(Equal("velaux"))
 				Expect(res[0].InstallPackage).ToNot(BeNil())
-				Expect(res[0].APISchema).ToNot(BeNil())
 			})
 		})
 
@@ -113,10 +111,8 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() {
 				Expect(res).To(HaveLen(2))
 				Expect(res[0].Name).To(Equal("velaux"))
 				Expect(res[0].InstallPackage).ToNot(BeNil())
-				Expect(res[0].APISchema).ToNot(BeNil())
 				Expect(res[1].Name).To(Equal("traefik"))
 				Expect(res[1].InstallPackage).ToNot(BeNil())
-				Expect(res[1].APISchema).ToNot(BeNil())
 			})
 		})
 
@@ -127,7 +123,6 @@ var _ = Describe("test FindWholeAddonPackagesFromRegistry", func() {
 				Expect(res).To(HaveLen(1))
 				Expect(res[0].Name).To(Equal("velaux"))
 				Expect(res[0].InstallPackage).ToNot(BeNil())
-				Expect(res[0].APISchema).ToNot(BeNil())
 			})
 		})
 	})

pkg/apiserver/domain/service/application.go

@@ -1550,6 +1550,13 @@ func (c *applicationServiceImpl) DryRunAppOrRevision(ctx context.Context, appMod
 		}
 	case "REVISION":
 		app, _, err = c.getAppModelFromRevision(ctx, appModel.Name, dryRunReq.Version)
+		originalApp := &v1beta1.Application{}
+		if err := c.KubeClient.Get(ctx, types.NamespacedName{
+			Name:      app.Name,
+			Namespace: app.Namespace,
+		}, originalApp); err == nil {
+			app.ResourceVersion = originalApp.ResourceVersion
+		}
 		if err != nil {
 			return nil, err
 		}

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
@@ -299,6 +299,11 @@ func (r *Reconciler) gcResourceTrackers(logCtx monitorContext.Context, handler *
 	}))
 	defer subCtx.Commit("finish gc resourceTrackers")
 
+	statusUpdater := r.updateStatus
+	if isPatch {
+		statusUpdater = r.patchStatus
+	}
+
 	var options []resourcekeeper.GCOption
 	if !gcOutdated {
 		options = append(options, resourcekeeper.DisableMarkStageGCOption{}, resourcekeeper.DisableGCComponentRevisionOption{}, resourcekeeper.DisableLegacyGCOption{})
@@ -306,8 +311,10 @@ func (r *Reconciler) gcResourceTrackers(logCtx monitorContext.Context, handler *
 	finished, waiting, err := handler.resourceKeeper.GarbageCollect(logCtx, options...)
 	if err != nil {
 		logCtx.Error(err, "Failed to gc resourcetrackers")
-		r.Recorder.Event(handler.app, event.Warning(velatypes.ReasonFailedGC, err))
-		return r.endWithNegativeCondition(logCtx, handler.app, condition.ReconcileError(err), phase)
+		cond := condition.Deleting()
+		cond.Message = fmt.Sprintf("error encountered during garbage collection: %s", err.Error())
+		handler.app.Status.SetConditions(cond)
+		return r.result(statusUpdater(logCtx, handler.app, phase)).ret()
 	}
 	if !finished {
 		logCtx.Info("GarbageCollecting resourcetrackers unfinished")
@@ -316,13 +323,13 @@ func (r *Reconciler) gcResourceTrackers(logCtx monitorContext.Context, handler *
 			cond.Message = fmt.Sprintf("Waiting for %s to delete. (At least %d resources are deleting.)", waiting[0].DisplayName(), len(waiting))
 		}
 		handler.app.Status.SetConditions(cond)
-		return r.result(r.patchStatus(logCtx, handler.app, phase)).requeue(baseGCBackoffWaitTime).ret()
+		return r.result(statusUpdater(logCtx, handler.app, phase)).requeue(baseGCBackoffWaitTime).ret()
 	}
 	logCtx.Info("GarbageCollected resourcetrackers")
 	if !isPatch {
-		return r.result(r.updateStatus(logCtx, handler.app, common.ApplicationRunningWorkflow)).ret()
+		phase = common.ApplicationRunningWorkflow
 	}
-	return r.result(r.patchStatus(logCtx, handler.app, phase)).ret()
+	return r.result(statusUpdater(logCtx, handler.app, phase)).ret()
 }
 
 type reconcileResult struct {

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_suite_test.go

@@ -169,7 +169,7 @@ var _ = Describe("Addon status or info", func() {
 				Expect(ds.DeleteRegistry(context.Background(), "KubeVela")).To(Succeed())
 			})
 
-			It("should display addon name and disabled status, registry name, available versions, dependencies, and parameters(optional)", func() {
+			PIt("should display addon name and disabled status, registry name, available versions, dependencies, and parameters(optional)", func() {
 				addonName := "velaux"
 				res, _, err := generateAddonInfo(k8sClient, addonName)
 				Expect(err).Should(BeNil())

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 {

test/e2e-multicluster-test/multicluster_test.go

@@ -41,6 +41,7 @@ import (
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/common"
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha1"
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
+	kubevelatypes "github.com/oam-dev/kubevela/apis/types"
 	"github.com/oam-dev/kubevela/pkg/multicluster"
 	"github.com/oam-dev/kubevela/pkg/oam"
 	"github.com/oam-dev/kubevela/pkg/utils"
@@ -696,5 +697,95 @@ var _ = Describe("Test multicluster scenario", func() {
 				g.Expect(k8sClient.Get(workerCtx, client.ObjectKey{Namespace: testNamespace, Name: "data-worker"}, &appsv1.Deployment{})).Should(Succeed())
 			}, 20*time.Second).Should(Succeed())
 		})
+
+		It("Test application with failed gc and restart workflow", func() {
+			By("duplicate cluster")
+			secret := &corev1.Secret{}
+			const secretName = "disconnection-test"
+			Expect(k8sClient.Get(hubCtx, types.NamespacedName{Namespace: kubevelatypes.DefaultKubeVelaNS, Name: WorkerClusterName}, secret)).Should(Succeed())
+			secret.SetName(secretName)
+			secret.SetResourceVersion("")
+			Expect(k8sClient.Create(hubCtx, secret)).Should(Succeed())
+			defer func() {
+				_ = k8sClient.Delete(hubCtx, secret)
+			}()
+
+			By("create cluster normally")
+			bs, err := os.ReadFile("./testdata/app/app-disconnection-test.yaml")
+			Expect(err).Should(Succeed())
+			app := &v1beta1.Application{}
+			Expect(yaml.Unmarshal(bs, app)).Should(Succeed())
+			app.SetNamespace(namespace)
+			Expect(k8sClient.Create(hubCtx, app)).Should(Succeed())
+			key := client.ObjectKeyFromObject(app)
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())
+				g.Expect(app.Status.Phase).Should(Equal(common.ApplicationRunning))
+			}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
+
+			By("disconnect cluster")
+			Expect(k8sClient.Get(hubCtx, types.NamespacedName{Namespace: kubevelatypes.DefaultKubeVelaNS, Name: secretName}, secret)).Should(Succeed())
+			secret.Data["tls.crt"] = []byte("-")
+			Expect(k8sClient.Update(hubCtx, secret)).Should(Succeed())
+
+			By("update application")
+			Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())
+			app.Spec.Policies = nil
+			Expect(k8sClient.Update(hubCtx, app)).Should(Succeed())
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())
+				g.Expect(app.Status.ObservedGeneration).Should(Equal(app.Generation))
+				g.Expect(app.Status.Phase).Should(Equal(common.ApplicationRunning))
+				rts := &v1beta1.ResourceTrackerList{}
+				g.Expect(k8sClient.List(hubCtx, rts, client.MatchingLabels{oam.LabelAppName: key.Name, oam.LabelAppNamespace: key.Namespace})).Should(Succeed())
+				cnt := 0
+				for _, item := range rts.Items {
+					if item.Spec.Type == v1beta1.ResourceTrackerTypeVersioned {
+						cnt++
+					}
+				}
+				g.Expect(cnt).Should(Equal(2))
+			}).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
+
+			By("try update application again")
+			Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())
+			if app.Annotations == nil {
+				app.Annotations = map[string]string{}
+			}
+			app.Annotations[oam.AnnotationPublishVersion] = "test"
+			Expect(k8sClient.Update(hubCtx, app)).Should(Succeed())
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())
+				g.Expect(app.Status.LatestRevision).ShouldNot(BeNil())
+				g.Expect(app.Status.LatestRevision.Revision).Should(Equal(int64(3)))
+				g.Expect(app.Status.ObservedGeneration).Should(Equal(app.Generation))
+				g.Expect(app.Status.Phase).Should(Equal(common.ApplicationRunning))
+			}).WithTimeout(1 * time.Minute).WithPolling(2 * time.Second).Should(Succeed())
+
+			By("clear disconnection cluster secret")
+			Expect(k8sClient.Get(hubCtx, types.NamespacedName{Namespace: kubevelatypes.DefaultKubeVelaNS, Name: secretName}, secret)).Should(Succeed())
+			Expect(k8sClient.Delete(hubCtx, secret)).Should(Succeed())
+
+			By("update application again")
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())
+				app.Annotations[oam.AnnotationPublishVersion] = "test2"
+				g.Expect(k8sClient.Update(hubCtx, app)).Should(Succeed())
+			}).WithTimeout(10 * time.Second).WithPolling(2 * time.Second).Should(Succeed())
+
+			By("wait gc application completed")
+			Eventually(func(g Gomega) {
+				rts := &v1beta1.ResourceTrackerList{}
+				g.Expect(k8sClient.List(hubCtx, rts, client.MatchingLabels{oam.LabelAppName: key.Name, oam.LabelAppNamespace: key.Namespace})).Should(Succeed())
+				cnt := 0
+				for _, item := range rts.Items {
+					if item.Spec.Type == v1beta1.ResourceTrackerTypeVersioned {
+						cnt++
+					}
+				}
+				g.Expect(cnt).Should(Equal(1))
+			}).WithTimeout(3 * time.Minute).WithPolling(10 * time.Second).Should(Succeed())
+		})
+
 	})
 })

test/e2e-multicluster-test/testdata/app/app-disconnection-test.yaml

@@ -0,0 +1,17 @@
+apiVersion: core.oam.dev/v1beta1
+kind: Application
+metadata:
+  name: app-disconnection-test
+spec:
+  components:
+    - type: k8s-objects
+      name: app-dis-cm
+      properties:
+        objects:
+          - apiVersion: v1
+            kind: ConfigMap
+  policies:
+    - type: topology
+      name: disconnection-test
+      properties:
+        clusters: ["disconnection-test"]