[Backport release-1.6] Fix: bug when addon dependent an addon in other registry (#5115)

* fix several bugs of addon

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit eadabe6517)

* fix golint error

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 2ba81880bf)

* fix error and add tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 43f6925566)

* fix comments and fix apiserver test

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 96a6a3f4a3)

* fix typo

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit cafa5adb46)

* fix tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 3599b01aeb)

* small fix

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 30cafbc3e9)

* small fix

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 083413479e)

* add parameter in apiserver and test

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
(cherry picked from commit 9159749477)

Co-authored-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

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

@@ -0,0 +1,8 @@
+name: mock-dep-addon
+version: v1.0.0
+description: Vela test addon named mock-dep-addon
+icon: https://www.test.com/icon
+url: https://www.test.com
+
+dependencies:
+  - name: mock-be-dep-addon

e2e/addon/mock/testrepo/helm-repo/index.yaml

@@ -60,4 +60,13 @@ entries:
       urls:
         - http://127.0.0.1:9098/helm/bar-v2.0.0.tgz
       version: v2.0.0
+  mock-be-dep-addon:
+    - created: "2022-10-29T09:11:16.865230605Z"
+      description: Vela test addon named mock-be-dep-addon
+      home:  https://www.test.com/icon
+      icon: https://www.test.com
+      name: mock-be-dep-addon
+      urls:
+        - http://127.0.0.1:9098/helm/mock-be-dep-addon-v1.0.0.tgz
+      version: v1.0.0
 generated: "2022-06-15T13:17:04.733573+08:00"

e2e/addon/mock/vela_addon_mock_server.go

@@ -149,6 +149,12 @@ var helmHandler http.HandlerFunc = func(rw http.ResponseWriter, req *http.Reques
 			_, _ = rw.Write([]byte(err.Error()))
 		}
 		rw.Write(file)
+	case strings.Contains(req.URL.Path, "mock-be-dep-addon-v1.0.0.tgz"):
+		file, err := os.ReadFile("./e2e/addon/mock/testrepo/helm-repo/mock-be-dep-addon-v1.0.0.tgz")
+		if err != nil {
+			_, _ = rw.Write([]byte(err.Error()))
+		}
+		rw.Write(file)
 	}
 
 }

pkg/addon/addon.go

@@ -932,10 +932,12 @@ type Installer struct {
 
 	dryRun     bool
 	dryRunBuff *bytes.Buffer
+
+	registries []Registry
 }
 
 // NewAddonInstaller will create an installer for addon
-func NewAddonInstaller(ctx context.Context, cli client.Client, discoveryClient *discovery.DiscoveryClient, apply apply.Applicator, config *rest.Config, r *Registry, args map[string]interface{}, cache *Cache, opts ...InstallOption) Installer {
+func NewAddonInstaller(ctx context.Context, cli client.Client, discoveryClient *discovery.DiscoveryClient, apply apply.Applicator, config *rest.Config, r *Registry, args map[string]interface{}, cache *Cache, registries []Registry, opts ...InstallOption) Installer {
 	i := Installer{
 		ctx:        ctx,
 		config:     config,
@@ -946,6 +948,7 @@ func NewAddonInstaller(ctx context.Context, cli client.Client, discoveryClient *
 		cache:      cache,
 		dc:         discoveryClient,
 		dryRunBuff: &bytes.Buffer{},
+		registries: registries,
 	}
 	for _, opt := range opts {
 		opt(&i)
@@ -1042,16 +1045,41 @@ func (h *Installer) installDependency(addon *InstallPackage) error {
 		if h.dryRun {
 			continue
 		}
-		// always install addon's latest version
-		depAddon, err := h.loadInstallPackage(dep.Name, dep.Version)
-		if err != nil {
-			return err
-		}
 		depHandler := *h
 		depHandler.args = nil
-		if err = depHandler.enableAddon(depAddon); err != nil {
-			return errors.Wrap(err, "fail to dispatch dependent addon resource")
+		var depAddon *InstallPackage
+		// try to install the dependent addon from the same registry with the current addon
+		depAddon, err = h.loadInstallPackage(dep.Name, dep.Version)
+		if err == nil {
+			if err = depHandler.enableAddon(depAddon); err != nil {
+				return errors.Wrap(err, "fail to dispatch dependent addon resource")
+			}
+			return nil
 		}
+		if !errors.Is(err, ErrNotExist) {
+			return err
+		}
+		for _, registry := range h.registries {
+			// try to install dependent addon from other registries
+			depHandler.r = &Registry{
+				Name: registry.Name, Helm: registry.Helm, OSS: registry.OSS, Git: registry.Git, Gitee: registry.Gitee, Gitlab: registry.Gitlab,
+			}
+			depAddon, err = depHandler.loadInstallPackage(dep.Name, dep.Version)
+			if err == nil {
+				break
+			}
+			if errors.Is(err, ErrNotExist) {
+				continue
+			}
+			return err
+		}
+		if err == nil {
+			if err = depHandler.enableAddon(depAddon); err != nil {
+				return errors.Wrap(err, "fail to dispatch dependent addon resource")
+			}
+			return nil
+		}
+		return fmt.Errorf("dependency addon: %s with version: %s cannot be found from all registries", dep.Name, dep.Version)
 	}
 	if h.dryRun && len(dependencies) > 0 {
 		klog.Warningf("dry run addon won't install dependencies, please make sure your system has already installed these addons: %v", strings.Join(dependencies, ", "))

pkg/addon/addon_suite_test.go

@@ -355,7 +355,7 @@ var _ = Describe("func addon update ", func() {
 		}, time.Millisecond*500, 30*time.Second).Should(BeNil())
 
 		pkg := &InstallPackage{Meta: Meta{Name: "test-update", Version: "1.3.0"}}
-		h := NewAddonInstaller(context.Background(), k8sClient, nil, nil, nil, &Registry{Name: "test"}, nil, nil)
+		h := NewAddonInstaller(context.Background(), k8sClient, nil, nil, nil, &Registry{Name: "test"}, nil, nil, nil)
 		h.addon = pkg
 		Expect(h.dispatchAddonResource(pkg)).Should(BeNil())
 
@@ -418,7 +418,7 @@ var _ = Describe("test dry-run addon from local dir", func() {
 		pkg, err := GetInstallPackageFromReader(r, &meta, UIData)
 		Expect(err).Should(BeNil())
 
-		h := NewAddonInstaller(ctx, k8sClient, dc, apply.NewAPIApplicator(k8sClient), cfg, &Registry{Name: LocalAddonRegistryName}, map[string]interface{}{"example": "test-dry-run"}, nil, DryRunAddon)
+		h := NewAddonInstaller(ctx, k8sClient, dc, apply.NewAPIApplicator(k8sClient), cfg, &Registry{Name: LocalAddonRegistryName}, map[string]interface{}{"example": "test-dry-run"}, nil, nil, DryRunAddon)
 
 		err = h.enableAddon(pkg)
 		Expect(err).Should(BeNil())

pkg/addon/helper.go

@@ -55,8 +55,8 @@ const (
 )
 
 // EnableAddon will enable addon with dependency check, source is where addon from.
-func EnableAddon(ctx context.Context, name string, version string, cli client.Client, discoveryClient *discovery.DiscoveryClient, apply apply.Applicator, config *rest.Config, r Registry, args map[string]interface{}, cache *Cache, opts ...InstallOption) error {
-	h := NewAddonInstaller(ctx, cli, discoveryClient, apply, config, &r, args, cache, opts...)
+func EnableAddon(ctx context.Context, name string, version string, cli client.Client, discoveryClient *discovery.DiscoveryClient, apply apply.Applicator, config *rest.Config, r Registry, args map[string]interface{}, cache *Cache, registries []Registry, opts ...InstallOption) error {
+	h := NewAddonInstaller(ctx, cli, discoveryClient, apply, config, &r, args, cache, registries, opts...)
 	pkg, err := h.loadInstallPackage(name, version)
 	if err != nil {
 		return err
@@ -113,7 +113,7 @@ func EnableAddonByLocalDir(ctx context.Context, name string, dir string, cli cli
 	if err != nil {
 		return err
 	}
-	h := NewAddonInstaller(ctx, cli, dc, applicator, config, &Registry{Name: LocalAddonRegistryName}, args, nil, opts...)
+	h := NewAddonInstaller(ctx, cli, dc, applicator, config, &Registry{Name: LocalAddonRegistryName}, args, nil, nil, opts...)
 	needEnableAddonNames, err := h.checkDependency(pkg)
 	if err != nil {
 		return err

pkg/addon/utils.go

@@ -187,7 +187,7 @@ func findLegacyAddonDefs(ctx context.Context, k8sClient client.Client, addonName
 		if registry.Name == registryName {
 			var uiData *UIData
 			if !IsVersionRegistry(registry) {
-				installer := NewAddonInstaller(ctx, k8sClient, nil, nil, config, &registries[i], nil, nil)
+				installer := NewAddonInstaller(ctx, k8sClient, nil, nil, config, &registries[i], nil, nil, nil)
 				metas, err := installer.getAddonMeta()
 				if err != nil {
 					return err
@@ -502,3 +502,11 @@ func checkBondComponentExist(u unstructured.Unstructured, app v1beta1.Applicatio
 	}
 	return false
 }
+
+// FilterDependencyRegistries will return all registries besides the target registry itself
+func FilterDependencyRegistries(i int, registries []Registry) []Registry {
+	if i < len(registries) {
+		return append(registries[:i], registries[i+1:]...)
+	}
+	return registries
+}

pkg/addon/utils_test.go

@@ -329,6 +329,44 @@ func TestCheckObjectBindingComponent(t *testing.T) {
 	}
 }
 
+func TestFilterDependencyRegistries(t *testing.T) {
+	testCases := []struct {
+		registries []Registry
+		index      int
+		res        []Registry
+	}{
+		{
+			registries: []Registry{{Name: "r1"}, {Name: "r2"}, {Name: "r3"}},
+			index:      0,
+			res:        []Registry{{Name: "r2"}, {Name: "r3"}},
+		},
+		{
+			registries: []Registry{{Name: "r1"}, {Name: "r2"}, {Name: "r3"}},
+			index:      1,
+			res:        []Registry{{Name: "r1"}, {Name: "r3"}},
+		},
+		{
+			registries: []Registry{{Name: "r1"}, {Name: "r2"}, {Name: "r3"}},
+			index:      2,
+			res:        []Registry{{Name: "r1"}, {Name: "r2"}},
+		},
+		{
+			registries: []Registry{{Name: "r1"}, {Name: "r2"}, {Name: "r3"}},
+			index:      3,
+			res:        []Registry{{Name: "r1"}, {Name: "r2"}, {Name: "r3"}},
+		},
+		{
+			registries: []Registry{},
+			index:      0,
+			res:        []Registry{},
+		},
+	}
+	for _, testCase := range testCases {
+		res := FilterDependencyRegistries(testCase.index, testCase.registries)
+		assert.Equal(t, res, testCase.res)
+	}
+}
+
 const (
 	compDefYaml = `
 apiVersion: core.oam.dev/v1beta1

pkg/apiserver/domain/service/addon.go

@@ -400,8 +400,22 @@ func (u *addonServiceImpl) EnableAddon(ctx context.Context, name string, args ap
 	if err != nil {
 		return err
 	}
-	for _, r := range registries {
-		err = pkgaddon.EnableAddon(ctx, name, args.Version, u.kubeClient, u.discoveryClient, u.apply, u.config, r, args.Args, u.addonRegistryCache)
+	if len(args.RegistryName) != 0 {
+		foundRegistry := false
+		for _, registry := range registries {
+			if registry.Name == args.RegistryName {
+				foundRegistry = true
+			}
+		}
+		if !foundRegistry {
+			return bcode.ErrAddonRegistryNotExist.SetMessage(fmt.Sprintf("specified registry %s not exist", args.RegistryName))
+		}
+	}
+	for i, r := range registries {
+		if len(args.RegistryName) != 0 && args.RegistryName != r.Name {
+			continue
+		}
+		err = pkgaddon.EnableAddon(ctx, name, args.Version, u.kubeClient, u.discoveryClient, u.apply, u.config, r, args.Args, u.addonRegistryCache, pkgaddon.FilterDependencyRegistries(i, registries))
 		if err == nil {
 			return nil
 		}
@@ -471,8 +485,8 @@ func (u *addonServiceImpl) UpdateAddon(ctx context.Context, name string, args ap
 		return err
 	}
 
-	for _, r := range registries {
-		err = pkgaddon.EnableAddon(ctx, name, args.Version, u.kubeClient, u.discoveryClient, u.apply, u.config, r, args.Args, u.addonRegistryCache)
+	for i, r := range registries {
+		err = pkgaddon.EnableAddon(ctx, name, args.Version, u.kubeClient, u.discoveryClient, u.apply, u.config, r, args.Args, u.addonRegistryCache, pkgaddon.FilterDependencyRegistries(i, registries))
 		if err == nil {
 			return nil
 		}

pkg/apiserver/interfaces/api/dto/v1/types.go

@@ -129,6 +129,8 @@ type EnableAddonRequest struct {
 	Clusters []string `json:"clusters,omitempty"`
 	// Version specify the version of addon to enable
 	Version string `json:"version,omitempty"`
+	// RegistryName specify the registry name
+	RegistryName string `json:"registryName,omitempty"`
 }
 
 // ListAddonResponse defines the format for addon list response

pkg/apiserver/utils/bcode/005_addon.go

@@ -73,6 +73,9 @@ var (
 
 	// ErrCloudShellNotInit means the cloudshell CR not created
 	ErrCloudShellNotInit = NewBcode(400, 50021, "Closing the console window and retry")
+
+	// ErrRegistryNotExist means the specified registry not exist
+	ErrRegistryNotExist = NewBcode(400, 50022, "The specified not exist")
 )
 
 // isGithubRateLimit check if error is github rate limit

references/cli/addon.go

@@ -148,6 +148,8 @@ func NewAddonEnableCommand(c common.Args, ioStream cmdutil.IOStreams) *cobra.Com
 	vela addon enable <your-local-addon-path>
   Enable addon with specified args (the args should be defined in addon's parameters):
 	vela addon enable <addon-name> <my-parameter-of-addon>=<my-value>
+  Enable addon with specified registry:
+    vela addon enable <registryName>/<addonName>
 `,
 		RunE: func(cmd *cobra.Command, args []string) error {
 
@@ -543,10 +545,27 @@ func enableAddon(ctx context.Context, k8sClient client.Client, dc *discovery.Dis
 	if err != nil {
 		return err
 	}
-
-	for _, registry := range registries {
+	registryName, addonName, err := splitSpecifyRegistry(name)
+	if err != nil {
+		return err
+	}
+	if len(registryName) != 0 {
+		foundRegistry := false
+		for _, registry := range registries {
+			if registry.Name == registryName {
+				foundRegistry = true
+			}
+		}
+		if !foundRegistry {
+			return fmt.Errorf("specified registry %s not exist", registryName)
+		}
+	}
+	for i, registry := range registries {
 		opts := addonOptions()
-		err = pkgaddon.EnableAddon(ctx, name, version, k8sClient, dc, apply.NewAPIApplicator(k8sClient), config, registry, args, nil, opts...)
+		if len(registryName) != 0 && registryName != registry.Name {
+			continue
+		}
+		err = pkgaddon.EnableAddon(ctx, addonName, version, k8sClient, dc, apply.NewAPIApplicator(k8sClient), config, registry, args, nil, pkgaddon.FilterDependencyRegistries(i, registries), opts...)
 		if errors.Is(err, pkgaddon.ErrNotExist) {
 			continue
 		}
@@ -558,21 +577,24 @@ func enableAddon(ctx context.Context, k8sClient client.Client, dc *discovery.Dis
 			}
 			input := NewUserInput()
 			if input.AskBool(unMatchErr.Error(), &UserInputOptions{AssumeYes: false}) {
-				err = pkgaddon.EnableAddon(ctx, name, availableVersion, k8sClient, dc, apply.NewAPIApplicator(k8sClient), config, registry, args, nil)
+				err = pkgaddon.EnableAddon(ctx, addonName, availableVersion, k8sClient, dc, apply.NewAPIApplicator(k8sClient), config, registry, args, nil, pkgaddon.FilterDependencyRegistries(i, registries))
 				return err
 			}
 			// The user does not agree to use the version provided by us
-			return fmt.Errorf("you can try another version by command: \"vela addon enable %s --version <version> \" ", name)
+			return fmt.Errorf("you can try another version by command: \"vela addon enable %s --version <version> \" ", addonName)
 		}
 		if err != nil {
 			return err
 		}
-		if err = waitApplicationRunning(k8sClient, name); err != nil {
+		if err = waitApplicationRunning(k8sClient, addonName); err != nil {
 			return err
 		}
 		return nil
 	}
-	return fmt.Errorf("addon: %s not found in registries", name)
+	if len(registryName) != 0 {
+		return fmt.Errorf("addon: %s not found in registry %s", addonName, registryName)
+	}
+	return fmt.Errorf("addon: %s not found in all candidate registries", addonName)
 }
 
 func addonOptions() []pkgaddon.InstallOption {
@@ -1136,3 +1158,15 @@ func NewAddonPackageCommand(c common.Args) *cobra.Command {
 	}
 	return cmd
 }
+
+func splitSpecifyRegistry(name string) (string, string, error) {
+	res := strings.Split(name, "/")
+	switch len(res) {
+	case 2:
+		return res[0], res[1], nil
+	case 1:
+		return "", res[0], nil
+	default:
+		return "", "", fmt.Errorf("invalid addon name, you should specify name only  <addonName>  or with registry as prefix <registryName>/<addonName>")
+	}
+}

references/cli/addon_test.go

@@ -443,3 +443,37 @@ func TestNewAddonCreateCommand(t *testing.T) {
 	_ = os.RemoveAll("test-addon")
 
 }
+
+func TestCheckSpecifyRegistry(t *testing.T) {
+	testCases := []struct {
+		name      string
+		registry  string
+		addonName string
+		hasError  bool
+	}{
+		{
+			name:      "fluxcd",
+			registry:  "",
+			addonName: "fluxcd",
+			hasError:  false,
+		},
+		{
+			name:      "kubevela/fluxcd",
+			registry:  "kubevela",
+			addonName: "fluxcd",
+			hasError:  false,
+		},
+		{
+			name:      "test/kubevela/fluxcd",
+			registry:  "",
+			addonName: "",
+			hasError:  true,
+		},
+	}
+	for _, testCase := range testCases {
+		r, n, err := splitSpecifyRegistry(testCase.name)
+		assert.Equal(t, err != nil, testCase.hasError)
+		assert.Equal(t, r, testCase.registry)
+		assert.Equal(t, n, testCase.addonName)
+	}
+}

test/e2e-apiserver-test/addon_test.go

@@ -232,4 +232,35 @@ var _ = Describe("Test addon rest api", func() {
 			}, 30*time.Second, 300*time.Millisecond).Should(Succeed())
 		})
 	})
+
+	Describe("Test addon dependency addon in other registry", func() {
+		It("Test Operation of enable addon from other registry", func() {
+			req := apisv1.EnableAddonRequest{}
+			res := post("/addons/mock-dep-addon/enable", req)
+			defer res.Body.Close()
+			var addon apisv1.AddonStatusResponse
+			Expect(decodeResponseBody(res, &addon)).Should(Succeed())
+			Expect(addon.Name).Should(BeEquivalentTo("mock-dep-addon"))
+
+			Eventually(func(g Gomega) {
+				status := get("/addons/mock-dep-addon/status")
+				var newaddonStatus apisv1.AddonStatusResponse
+				g.Expect(decodeResponseBody(status, &newaddonStatus)).Should(Succeed())
+				g.Expect(newaddonStatus.Name).Should(BeEquivalentTo("mock-dep-addon"))
+				g.Expect(newaddonStatus.InstalledVersion).Should(BeEquivalentTo("v1.0.0"))
+				g.Expect(newaddonStatus.Phase).Should(BeEquivalentTo(apisv1.AddonPhaseEnabled))
+			}, 30*time.Second, 300*time.Millisecond).Should(Succeed())
+		})
+	})
+
+	Describe("Test enable an addon with specified registry", func() {
+		It("Test with a not exist registry", func() {
+			req := apisv1.EnableAddonRequest{
+				RegistryName: "not-exist",
+			}
+			res := post("/addons/test-addon/enable", req)
+			defer res.Body.Close()
+			Expect(res.StatusCode).Should(BeEquivalentTo(400))
+		})
+	})
 })