[Backport release-1.8] Fix: install dependency is invalid for runtime…

… addon when it's cluster arg is nil (#5935) * Fix: install dependency is invalid for runtime addon when it's clusters arg is nil Signed-off-by: zhaohuihui <zhaohuihui_yewu@cmss.chinamobile.com> (cherry picked from commit 142242a) * Fix: add unit test for getDependencyArgs and checkDependencyNeedInstall Signed-off-by: zhaohuihui <zhaohuihui_yewu@cmss.chinamobile.com> (cherry picked from commit c38f6e4) * Fix: Simplified the checkDependencyNeedInstall func logic Signed-off-by: zhaohuihui <zhaohuihui_yewu@cmss.chinamobile.com> (cherry picked from commit f038023) * Fix: add comments for checkDependencyNeedInstall Signed-off-by: zhaohuihui <zhaohuihui_yewu@cmss.chinamobile.com> (cherry picked from commit 1009de2) --------- Co-authored-by: zhaohuihui <zhaohuihui_yewu@cmss.chinamobile.com>
kubevela · Apr 28, 2023 · e528902 · e528902
1 parent c3b736f
commit e528902
Show file tree

Hide file tree

Showing 11 changed files with 385 additions and 68 deletions.
diff --git a/e2e/addon/addon_test.go b/e2e/addon/addon_test.go
@@ -142,11 +142,132 @@ var _ = Describe("Addon Test", func() {
 			Expect(output).To(ContainSubstring("you can try another version by command"))
 			Expect(err).NotTo(HaveOccurred())
 		})
+	})
+
+	Context("Addon registry test", func() {
+		It("List all addon registry", func() {
+			output, err := e2e.Exec("vela addon registry list")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output).To(ContainSubstring("KubeVela"))
+		})
+
+		It("Get addon registry", func() {
+			output, err := e2e.Exec("vela addon registry get KubeVela")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output).To(ContainSubstring("KubeVela"))
+		})
+
+		It("Add test addon registry", func() {
+			output, err := e2e.LongTimeExec("vela addon registry add my-repo --type=git --endpoint=https://github.com/oam-dev/catalog --path=/experimental/addons", 600*time.Second)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output).To(ContainSubstring("Successfully add an addon registry my-repo"))
+
+			Eventually(func() error {
+				output, err := e2e.LongTimeExec("vela addon registry update my-repo --type=git --endpoint=https://github.com/oam-dev/catalog --path=/addons", 300*time.Second)
+				if err != nil {
+					return err
+				}
+				if !strings.Contains(output, "Successfully update an addon registry my-repo") {
+					return fmt.Errorf("cannot update addon registry")
+				}
+				return nil
+			}, 30*time.Second, 300*time.Millisecond).Should(BeNil())
+
+			output, err = e2e.LongTimeExec("vela addon registry delete my-repo", 600*time.Second)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output).To(ContainSubstring("Successfully delete an addon registry my-repo"))
+		})
+	})
+
+	Context("Enable dependency addon test", func() {
+		It(" enable mock-dependence-rely without specified clusters when mock-dependence addon is not enabled", func() {
+			output, err := e2e.Exec("vela addon enable mock-dependence-rely")
+			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-dependence", 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["clusterLabelSelector"]).Should(Equal(map[string]interface{}{}))
+			}, 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) {
+				_, err = c.SendLine("y")
+				Expect(err).NotTo(HaveOccurred())
+			})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output).To(ContainSubstring("enabled successfully."))
+			Eventually(func(g Gomega) {
+				// check application render cluster
+				app := &v1beta1.Application{}
+				Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: "addon-mock-dependence", 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"}))
+				Expect(topologyPolicyValue["clusterLabelSelector"]).Should(BeNil())
+			}, 600*time.Second).Should(Succeed())
+			// 2. enable mock-dependence-rely addon without clusters
+			output1, err := e2e.InteractiveExec("vela addon enable mock-dependence-rely", func(c *expect.Console) {
+				_, err = c.SendLine("y")
+				Expect(err).NotTo(HaveOccurred())
+			})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(output1).To(ContainSubstring("enabled successfully."))
+			// 3. enable mock-dependence-rely addon changes the mock-dependence topology policy
+			Eventually(func(g Gomega) {
+				app := &v1beta1.Application{}
+				Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: "addon-mock-dependence", 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["clusterLabelSelector"]).Should(Equal(map[string]interface{}{}))
+				Expect(topologyPolicyValue["clusters"]).Should(BeNil())
+			}, 30*time.Second).Should(Succeed())
+		})
 
 		It("Test addon dependency with specified clusters", func() {
 			const clusterName = "k3s-default"
 			// enable addon
-			output, err := e2e.Exec("vela addon enable mock-dependence --clusters local myparam=test")
+			output, err := e2e.InteractiveExec("vela addon enable mock-dependence --clusters local myparam=test", func(c *expect.Console) {
+				_, err = c.SendLine("y")
+				Expect(err).NotTo(HaveOccurred())
+			})
 			Expect(err).NotTo(HaveOccurred())
 			Expect(output).To(ContainSubstring("enabled successfully."))
 			output1, err := e2e.Exec("vela ls -A")
@@ -185,7 +306,10 @@ var _ = Describe("Addon Test", func() {
 				Expect(topologyPolicyValue["clusters"]).Should(Equal([]interface{}{"local"}))
 			}, 600*time.Second).Should(Succeed())
 			// enable addon which rely on mock-dependence addon
-			e2e.Exec("vela addon enable mock-dependence-rely --clusters local," + clusterName)
+			e2e.InteractiveExec("vela addon enable mock-dependence-rely --clusters local,"+clusterName, func(c *expect.Console) {
+				_, err = c.SendLine("y")
+				Expect(err).NotTo(HaveOccurred())
+			})
 			// check mock-dependence application parameter
 			Eventually(func(g Gomega) {
 				sec := &v1.Secret{}
@@ -210,38 +334,4 @@ var _ = Describe("Addon Test", func() {
 		})
 	})
 
-	Context("Addon registry test", func() {
-		It("List all addon registry", func() {
-			output, err := e2e.Exec("vela addon registry list")
-			Expect(err).NotTo(HaveOccurred())
-			Expect(output).To(ContainSubstring("KubeVela"))
-		})
-
-		It("Get addon registry", func() {
-			output, err := e2e.Exec("vela addon registry get KubeVela")
-			Expect(err).NotTo(HaveOccurred())
-			Expect(output).To(ContainSubstring("KubeVela"))
-		})
-
-		It("Add test addon registry", func() {
-			output, err := e2e.LongTimeExec("vela addon registry add my-repo --type=git --endpoint=https://github.com/oam-dev/catalog --path=/experimental/addons", 600*time.Second)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(output).To(ContainSubstring("Successfully add an addon registry my-repo"))
-
-			Eventually(func() error {
-				output, err := e2e.LongTimeExec("vela addon registry update my-repo --type=git --endpoint=https://github.com/oam-dev/catalog --path=/addons", 300*time.Second)
-				if err != nil {
-					return err
-				}
-				if !strings.Contains(output, "Successfully update an addon registry my-repo") {
-					return fmt.Errorf("cannot update addon registry")
-				}
-				return nil
-			}, 30*time.Second, 300*time.Millisecond).Should(BeNil())
-
-			output, err = e2e.LongTimeExec("vela addon registry delete my-repo", 600*time.Second)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(output).To(ContainSubstring("Successfully delete an addon registry my-repo"))
-		})
-	})
 })
diff --git a/e2e/addon/mock/testdata/mock-dependence-rely2/README.md b/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
diff --git a/e2e/addon/mock/testdata/mock-dependence-rely2/metadata.yaml b/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
diff --git a/e2e/addon/mock/testdata/mock-dependence-rely2/parameter.cue b/e2e/addon/mock/testdata/mock-dependence-rely2/parameter.cue
@@ -0,0 +1,4 @@
+parameter: {
+	// +usage=Custom parameter description
+	myparam: *"mynsrely" | string
+}
diff --git a/e2e/addon/mock/testdata/mock-dependence-rely2/template.cue b/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: []
+	}
+}
diff --git a/e2e/addon/mock/testdata/mock-dependence2/README.md b/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
diff --git a/e2e/addon/mock/testdata/mock-dependence2/metadata.yaml b/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
diff --git a/e2e/addon/mock/testdata/mock-dependence2/parameter.cue b/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]
+}
diff --git a/e2e/addon/mock/testdata/mock-dependence2/template.cue b/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: {}
+					}
+				}
+			},
+		]
+	}
+}
diff --git a/pkg/addon/addon.go b/pkg/addon/addon.go
@@ -1015,18 +1015,11 @@ func (h *Installer) installDependency(addon *InstallPackage) error {
 			continue
 		}
 		depHandler := *h
-		// get dependency addon original parameters
-		depArgs, depArgsErr := GetAddonLegacyParameters(h.ctx, h.cli, dep.Name)
+		// reset dependency addon clusters parameter
+		depArgs, depArgsErr := getDependencyArgs(h.ctx, h.cli, dep.Name, depClusters)
 		if depArgsErr != nil {
-			if !apierrors.IsNotFound(depArgsErr) {
-				return depArgsErr
-			}
-		}
-		if depArgs == nil {
-			depArgs = map[string]interface{}{}
+			return depArgsErr
 		}
-		// reset the cluster arg
-		depArgs[types.ClustersArg] = depClusters
 
 		depHandler.args = depArgs
 
@@ -1082,34 +1075,69 @@ 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 {
-		// 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)
+		// dependent addon is not exist
+		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
 			}
+			break
 		}
-
-		// 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
-			}
+	}
+	// nil clusters indicates that the dependent addon is installed on all clusters
+	if topologyPolicyValue["clusters"] == nil {
+		return false, nil, nil
+	}
+	// nil addonClusters indicates the addon will be installed,
+	// thus we should set the dependent addon's clusters arg to be nil so that it is installed on all clusters
+	if addonClusters == nil {
+		return true, nil, nil
+	}
+	// Determine whether the dependent addon's existing clusters can cover the new addon's clusters
+	var needInstallAddonDep = false
+	var depClusters []string
+	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
 }
 
+// getDependencyArgs resets the dependency clusters arg according needed install depClusters
+func getDependencyArgs(ctx context.Context, k8sClient client.Client, depName string, depClusters []string) (map[string]interface{}, error) {
+	depArgs, depArgsErr := GetAddonLegacyParameters(ctx, k8sClient, depName)
+	if depArgsErr != nil && !apierrors.IsNotFound(depArgsErr) {
+		return nil, depArgsErr
+	}
+	// reset the cluster arg
+	if depClusters == nil {
+		// delete clusters args, when render addon, it will use clusterLabelSelector then render addon to all clusters
+		if depArgs != nil && depArgs[types.ClustersArg] != nil {
+			delete(depArgs, types.ClustersArg)
+		}
+	} else {
+		if depArgs == nil {
+			depArgs = map[string]interface{}{}
+		}
+		depArgs[types.ClustersArg] = depClusters
+	}
+	return depArgs, nil
+}
+
 // checkDependency checks if addon's dependency
 func (h *Installer) checkDependency(addon *InstallPackage) ([]string, error) {
 	var app v1beta1.Application