Fix: Simplified the checkDependencyNeedInstall func logic

Signed-off-by: zhaohuihui <zhaohuihui_yewu@cmss.chinamobile.com>
(cherry picked from commit f038023a98)

e2e/addon/addon_test.go

@@ -198,6 +198,24 @@ var _ = Describe("Addon Test", func() {
 			}, 30*time.Second).Should(Succeed())
 		})
 
+		It("enable mock-dependence-rely with specified clusters when mock-dependence addon is not enabled ", func() {
+			output, err := e2e.Exec("vela addon enable mock-dependence-rely2 --clusters local")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output).To(ContainSubstring("enabled successfully."))
+			Eventually(func(g Gomega) {
+				app := &v1beta1.Application{}
+				Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: "addon-mock-dependence2", Namespace: "vela-system"}, app)).Should(Succeed())
+				topologyPolicyValue := map[string]interface{}{}
+				for _, policy := range app.Spec.Policies {
+					if policy.Type == "topology" {
+						Expect(json.Unmarshal(policy.Properties.Raw, &topologyPolicyValue)).Should(Succeed())
+						break
+					}
+				}
+				Expect(topologyPolicyValue["clusters"]).Should(Equal([]interface{}{"local"}))
+			}, 30*time.Second).Should(Succeed())
+		})
+
 		It("enable mock-dependence-rely without specified clusters when mock-dependence addon was enabled with specified clusters", func() {
 			// 1. enable mock-dependence addon with local clusters
 			output, err := e2e.InteractiveExec("vela addon enable mock-dependence --clusters local myparam=test", func(c *expect.Console) {

e2e/addon/mock/testdata/mock-dependence-rely2/README.md

@@ -0,0 +1,3 @@
+# mock-dependence-rely2
+
+This is an addon template. Check how to build your own addon: https://kubevela.net/docs/platform-engineers/addon/intro

e2e/addon/mock/testdata/mock-dependence-rely2/metadata.yaml

@@ -0,0 +1,10 @@
+description: An addon for testing addon dependency with specified clusters.
+icon: ""
+invisible: false
+name: mock-dependence-rely2
+tags:
+- my-tag
+version: 1.0.0
+dependencies:
+  # install controller by helm.
+  - name: mock-dependence2

e2e/addon/mock/testdata/mock-dependence-rely2/parameter.cue

@@ -0,0 +1,4 @@
+parameter: {
+	// +usage=Custom parameter description
+	myparam: *"mynsrely" | string
+}

e2e/addon/mock/testdata/mock-dependence-rely2/template.cue

@@ -0,0 +1,9 @@
+package main
+output: {
+	apiVersion: "core.oam.dev/v1beta1"
+	kind:       "Application"
+	spec: {
+		components: []
+		policies: []
+	}
+}

e2e/addon/mock/testdata/mock-dependence2/README.md

@@ -0,0 +1,3 @@
+# mock-dependence2
+
+This is an addon template. Check how to build your own addon: https://kubevela.net/docs/platform-engineers/addon/intro

e2e/addon/mock/testdata/mock-dependence2/metadata.yaml

@@ -0,0 +1,7 @@
+description: An addon for testing addon dependency with specified clusters.
+icon: ""
+invisible: false
+name: mock-dependence2
+tags:
+- my-tag
+version: 1.0.0

e2e/addon/mock/testdata/mock-dependence2/parameter.cue

@@ -0,0 +1,6 @@
+parameter: {
+	// +usage=Custom parameter description
+	myparam: *"myns" | string
+        //+usage=Deploy to specified clusters. Leave empty to deploy to all clusters.
+	clusters?: [...string]
+}

e2e/addon/mock/testdata/mock-dependence2/template.cue

@@ -0,0 +1,24 @@
+package main
+_targetNamespace: parameter.myparam
+output: {
+	apiVersion: "core.oam.dev/v1beta1"
+	kind:       "Application"
+	spec: {
+		components: [],
+		policies: [
+			{
+				type: "topology"
+				name: "deploy-mock-dependency-ns"
+				properties: {
+					namespace: _targetNamespace
+					if parameter.clusters != _|_ {
+						clusters: parameter.clusters
+					}
+					if parameter.clusters == _|_ {
+						clusterLabelSelector: {}
+					}
+				}
+			},
+		]
+	}
+}

pkg/addon/addon.go

@@ -1075,46 +1075,40 @@ func (h *Installer) installDependency(addon *InstallPackage) error {
 // checkDependencyNeedInstall checks whether dependency addon needs to be installed on other clusters
 func checkDependencyNeedInstall(ctx context.Context, k8sClient client.Client, depName string, addonClusters []string) (bool, []string, error) {
 	depApp, err := FetchAddonRelatedApp(ctx, k8sClient, depName)
-	var needInstallAddonDep = false
-	var depClusters []string
 	if err != nil {
 		if !apierrors.IsNotFound(err) {
-			return needInstallAddonDep, depClusters, err
+			return false, nil, err
 		}
 		// depApp is not exist
-		needInstallAddonDep = true
-		depClusters = addonClusters
-	} else {
-		// if addon clusters is nil, meaning to deploy to all clusters
-		if addonClusters == nil {
-			hasTopologyPolicy := false
-			topologyPolicyValue := map[string]interface{}{}
-			for _, policy := range depApp.Spec.Policies {
-				if policy.Type == "topology" {
-					hasTopologyPolicy = true
-					unmarshalErr := json.Unmarshal(policy.Properties.Raw, &topologyPolicyValue)
-					if unmarshalErr != nil {
-						return true, nil, unmarshalErr
-					}
-					break
-				}
+		return true, addonClusters, nil
+	}
+	topologyPolicyValue := map[string]interface{}{}
+	for _, policy := range depApp.Spec.Policies {
+		if policy.Type == "topology" {
+			unmarshalErr := json.Unmarshal(policy.Properties.Raw, &topologyPolicyValue)
+			if unmarshalErr != nil {
+				return false, nil, unmarshalErr
 			}
-			need := hasTopologyPolicy && topologyPolicyValue["clusterLabelSelector"] == nil
-			return need, nil, nil
+			break
 		}
-		// get the runtime clusters of current dependency addon
-		for _, r := range depApp.Status.AppliedResources {
-			if r.Cluster != "" && !stringslices.Contains(depClusters, r.Cluster) {
-				depClusters = append(depClusters, r.Cluster)
-			}
-		}
-
-		// determine if there are no dependencies on the cluster to be installed
-		for _, addonCluster := range addonClusters {
-			if !stringslices.Contains(depClusters, addonCluster) {
-				depClusters = append(depClusters, addonCluster)
-				needInstallAddonDep = true
-			}
+	}
+	if topologyPolicyValue["clusters"] == nil {
+		return false, nil, nil
+	}
+	if addonClusters == nil {
+		return true, nil, nil
+	}
+	var needInstallAddonDep = false
+	var depClusters []string
+	// 判断原有的clusters是否能cover住addonClusters
+	originClusters := topologyPolicyValue["clusters"].([]interface{})
+	for _, r := range originClusters {
+		depClusters = append(depClusters, r.(string))
+	}
+	for _, addonCluster := range addonClusters {
+		if !stringslices.Contains(depClusters, addonCluster) {
+			depClusters = append(depClusters, addonCluster)
+			needInstallAddonDep = true
 		}
 	}
 	return needInstallAddonDep, depClusters, nil

pkg/addon/addon_suite_test.go

@@ -175,7 +175,7 @@ var _ = Describe("Addon test", func() {
 	})
 
 	It("checkDependencyNeedInstall func test", func() {
-		// case1: dependency addon not enable
+		// case1: dependency addon not exist, adonClusters is not nil
 		depAddonName := "legacy-addon"
 		addonClusters := []string{"cluster1", "cluster2"}
 		needInstallAddonDep, depClusters, err := checkDependencyNeedInstall(ctx, k8sClient, depAddonName, addonClusters)
@@ -183,7 +183,13 @@ var _ = Describe("Addon test", func() {
 		Expect(depClusters).Should(Equal(addonClusters))
 		Expect(err).Should(BeNil())
 
-		// case2: dependency addon enable
+		// case1.1: dependency addon not exist, adonClusters is nil
+		needInstallAddonDep1, depClusters1, err := checkDependencyNeedInstall(ctx, k8sClient, depAddonName, nil)
+		Expect(needInstallAddonDep1).Should(BeTrue())
+		Expect(depClusters1).Should(BeNil())
+		Expect(err).Should(BeNil())
+
+		// case2: dependency addon exist, no topology policy, addonClusters is not nil
 		app = v1beta1.Application{}
 		Expect(yaml.Unmarshal([]byte(legacyAppYaml), &app)).Should(BeNil())
 		app.SetNamespace(testns)
@@ -191,17 +197,29 @@ var _ = Describe("Addon test", func() {
 		Eventually(func(g Gomega) {
 			needInstallAddonDep, depClusters, err := checkDependencyNeedInstall(ctx, k8sClient, depAddonName, addonClusters)
 			Expect(err).Should(BeNil())
-			Expect(needInstallAddonDep).Should(BeTrue())
-			Expect(depClusters).Should(Equal(addonClusters))
+			Expect(needInstallAddonDep).Should(BeFalse())
+			Expect(depClusters).Should(BeNil())
 		}, 30*time.Second).Should(Succeed())
 
-		// case3: addonClusters is nil
+		// case3: clusters is nil (no topology policy), addonClusters is nil
 		needInstallAddonDep2, depClusters2, err := checkDependencyNeedInstall(ctx, k8sClient, depAddonName, nil)
 		Expect(needInstallAddonDep2).Should(BeFalse())
 		Expect(depClusters2).Should(BeNil())
 		Expect(err).Should(BeNil())
 
-		// case4: addonClusters is nil, and app has topology policy
+		// case4: clusters is nil, addonClusters is nil
+		app = v1beta1.Application{}
+		Expect(yaml.Unmarshal([]byte(legacy3AppYaml), &app)).Should(BeNil())
+		app.SetNamespace(testns)
+		Expect(k8sClient.Create(ctx, &app)).Should(BeNil())
+		Eventually(func(g Gomega) {
+			needInstallAddonDep, depClusters, err := checkDependencyNeedInstall(ctx, k8sClient, "legacy-addon3", nil)
+			Expect(err).Should(BeNil())
+			Expect(needInstallAddonDep).Should(BeFalse())
+			Expect(depClusters).Should(BeNil())
+		}, 60*time.Second).Should(Succeed())
+
+		// case5: clusters is not nil, addonClusters is nil,
 		app = v1beta1.Application{}
 		Expect(yaml.Unmarshal([]byte(legacy2AppYaml), &app)).Should(BeNil())
 		app.SetNamespace(testns)
@@ -213,16 +231,12 @@ var _ = Describe("Addon test", func() {
 			Expect(depClusters).Should(BeNil())
 		}, 60*time.Second).Should(Succeed())
 
-		// case5: addonClusters is nil, and app has topology policy, and clusterLabelSelector is not nil
-		app = v1beta1.Application{}
-		Expect(yaml.Unmarshal([]byte(legacy3AppYaml), &app)).Should(BeNil())
-		app.SetNamespace(testns)
-		Expect(k8sClient.Create(ctx, &app)).Should(BeNil())
+		// case6: clusters is [local], addonClusters is ["cluster1", "cluster2"]
 		Eventually(func(g Gomega) {
-			needInstallAddonDep, depClusters, err := checkDependencyNeedInstall(ctx, k8sClient, "legacy-addon3", nil)
+			needInstallAddonDep, depClusters, err := checkDependencyNeedInstall(ctx, k8sClient, "legacy-addon2", addonClusters)
 			Expect(err).Should(BeNil())
-			Expect(needInstallAddonDep).Should(BeFalse())
-			Expect(depClusters).Should(BeNil())
+			Expect(needInstallAddonDep).Should(BeTrue())
+			Expect(depClusters).Should(Equal(append([]string{"local"}, addonClusters...)))
 		}, 60*time.Second).Should(Succeed())
 	})