From 03223aa786dc43a9347afff2dbdfdc13f3394a1d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 24 Nov 2022 09:46:54 +0800 Subject: [PATCH] [Backport release-1.6] Fix: bug when addon dependent an addon in other registry (#5115) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix several bugs of addon Signed-off-by: 楚岳 (cherry picked from commit eadabe6517e82f73526bce146ab669e6f919cca5) * fix golint error Signed-off-by: 楚岳 (cherry picked from commit 2ba81880bf7e19c5dd3350305bab30baef4c4c57) * fix error and add tests Signed-off-by: 楚岳 (cherry picked from commit 43f69255660e853d7cd78c3cfb2871eadcd190a9) * fix comments and fix apiserver test Signed-off-by: 楚岳 (cherry picked from commit 96a6a3f4a34d2379ab8ddc9537956e00c8c937d3) * fix typo Signed-off-by: 楚岳 (cherry picked from commit cafa5adb4652c533aa69653f30474c1baab4bdc1) * fix tests Signed-off-by: 楚岳 (cherry picked from commit 3599b01aeba17d344e40932d7965bf82b532c50b) * small fix Signed-off-by: 楚岳 (cherry picked from commit 30cafbc3e9cf7eb28f659c17ff67e9e326df7e4c) * small fix Signed-off-by: 楚岳 (cherry picked from commit 083413479eebabb351938526acfedd102e83b7be) * add parameter in apiserver and test Signed-off-by: 楚岳 (cherry picked from commit 915974947725fd2b3db27a17f3e7a9ba194c351d) Co-authored-by: 楚岳 --- .../testdata/mock-dep-addon/metadata.yaml | 8 +++ e2e/addon/mock/testrepo/helm-repo/index.yaml | 9 ++++ .../helm-repo/mock-be-dep-addon-v1.0.0.tgz | Bin 0 -> 245 bytes e2e/addon/mock/vela_addon_mock_server.go | 6 +++ pkg/addon/addon.go | 44 +++++++++++++--- pkg/addon/addon_suite_test.go | 4 +- pkg/addon/helper.go | 6 +-- pkg/addon/utils.go | 10 +++- pkg/addon/utils_test.go | 38 ++++++++++++++ pkg/apiserver/domain/service/addon.go | 22 ++++++-- pkg/apiserver/interfaces/api/dto/v1/types.go | 2 + pkg/apiserver/utils/bcode/005_addon.go | 3 ++ references/cli/addon.go | 48 +++++++++++++++--- references/cli/addon_test.go | 34 +++++++++++++ test/e2e-apiserver-test/addon_test.go | 31 +++++++++++ 15 files changed, 240 insertions(+), 25 deletions(-) create mode 100644 e2e/addon/mock/testdata/mock-dep-addon/metadata.yaml create mode 100644 e2e/addon/mock/testrepo/helm-repo/mock-be-dep-addon-v1.0.0.tgz diff --git a/e2e/addon/mock/testdata/mock-dep-addon/metadata.yaml b/e2e/addon/mock/testdata/mock-dep-addon/metadata.yaml new file mode 100644 index 000000000..d8def0db7 --- /dev/null +++ b/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 diff --git a/e2e/addon/mock/testrepo/helm-repo/index.yaml b/e2e/addon/mock/testrepo/helm-repo/index.yaml index f10666a8c..c488b9876 100644 --- a/e2e/addon/mock/testrepo/helm-repo/index.yaml +++ b/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" \ No newline at end of file diff --git a/e2e/addon/mock/testrepo/helm-repo/mock-be-dep-addon-v1.0.0.tgz b/e2e/addon/mock/testrepo/helm-repo/mock-be-dep-addon-v1.0.0.tgz new file mode 100644 index 0000000000000000000000000000000000000000..2c09938fbc265b1dd7bdbe72bb8c83573bd67714 GIT binary patch literal 245 zcmV 0 { klog.Warningf("dry run addon won't install dependencies, please make sure your system has already installed these addons: %v", strings.Join(dependencies, ", ")) diff --git a/pkg/addon/addon_suite_test.go b/pkg/addon/addon_suite_test.go index f223cd04d..33fe8e1ee 100644 --- a/pkg/addon/addon_suite_test.go +++ b/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()) diff --git a/pkg/addon/helper.go b/pkg/addon/helper.go index e93dc122e..ae60bce54 100644 --- a/pkg/addon/helper.go +++ b/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 diff --git a/pkg/addon/utils.go b/pkg/addon/utils.go index 1fe5d43b7..2851688bc 100644 --- a/pkg/addon/utils.go +++ b/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, ®istries[i], nil, nil) + installer := NewAddonInstaller(ctx, k8sClient, nil, nil, config, ®istries[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 +} diff --git a/pkg/addon/utils_test.go b/pkg/addon/utils_test.go index 128ef3143..fa156e5d9 100644 --- a/pkg/addon/utils_test.go +++ b/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 diff --git a/pkg/apiserver/domain/service/addon.go b/pkg/apiserver/domain/service/addon.go index b5354d6c0..48e91fc15 100644 --- a/pkg/apiserver/domain/service/addon.go +++ b/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 } diff --git a/pkg/apiserver/interfaces/api/dto/v1/types.go b/pkg/apiserver/interfaces/api/dto/v1/types.go index 5ddac6c11..6eb7a18f2 100644 --- a/pkg/apiserver/interfaces/api/dto/v1/types.go +++ b/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 diff --git a/pkg/apiserver/utils/bcode/005_addon.go b/pkg/apiserver/utils/bcode/005_addon.go index 420c92ccb..9e8039d38 100644 --- a/pkg/apiserver/utils/bcode/005_addon.go +++ b/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 diff --git a/references/cli/addon.go b/references/cli/addon.go index c364af331..3beb3d249 100644 --- a/references/cli/addon.go +++ b/references/cli/addon.go @@ -148,6 +148,8 @@ func NewAddonEnableCommand(c common.Args, ioStream cmdutil.IOStreams) *cobra.Com vela addon enable Enable addon with specified args (the args should be defined in addon's parameters): vela addon enable = + Enable addon with specified registry: + vela addon enable / `, 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 \" ", name) + return fmt.Errorf("you can try another version by command: \"vela addon enable %s --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 or with registry as prefix /") + } +} diff --git a/references/cli/addon_test.go b/references/cli/addon_test.go index 461fdf308..441355b4b 100644 --- a/references/cli/addon_test.go +++ b/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) + } +} diff --git a/test/e2e-apiserver-test/addon_test.go b/test/e2e-apiserver-test/addon_test.go index 2636e381b..116732328 100644 --- a/test/e2e-apiserver-test/addon_test.go +++ b/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)) + }) + }) })