From c372578e59ba92a644f422f7975504872b92860f Mon Sep 17 00:00:00 2001 From: wyike Date: Fri, 29 Jul 2022 16:42:16 +0800 Subject: [PATCH] Fix: check definition of addon whether is conflict (#4493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix checksemver Signed-off-by: 楚岳 override defs Signed-off-by: 楚岳 add tests Signed-off-by: 楚岳 * add test and fix some special cases Signed-off-by: 楚岳 fix checkdiff Signed-off-by: 楚岳 fix flags Signed-off-by: 楚岳 * fix comments Signed-off-by: 楚岳 * small fix Signed-off-by: 楚岳 --- pkg/addon/addon.go | 27 +++++++--- pkg/addon/addon_suite_test.go | 99 +++++++++++++++++++++++++++++++++++ pkg/addon/addon_test.go | 22 ++++++++ pkg/addon/utils.go | 91 +++++++++++++++++++++++++++----- pkg/addon/utils_test.go | 23 +++++++- references/cli/addon.go | 10 ++++ 6 files changed, 252 insertions(+), 20 deletions(-) diff --git a/pkg/addon/addon.go b/pkg/addon/addon.go index 139478076..7ee65829f 100644 --- a/pkg/addon/addon.go +++ b/pkg/addon/addon.go @@ -889,6 +889,7 @@ type Installer struct { cache *Cache dc *discovery.DiscoveryClient skipVersionValidate bool + overrideDefs bool } // NewAddonInstaller will create an installer for addon @@ -1069,6 +1070,16 @@ func (h *Installer) dispatchAddonResource(addon *InstallPackage) error { return errors.Wrap(err, "render addon definitions fail") } + if !h.overrideDefs { + existDefs, err := checkConflictDefs(h.ctx, h.cli, defs, app.Name) + if err != nil { + return err + } + if len(existDefs) != 0 { + return produceDefConflictError(existDefs) + } + } + schemas, err := RenderDefinitionSchema(addon) if err != nil { return errors.Wrap(err, "render addon definitions' schema fail") @@ -1286,26 +1297,30 @@ func checkSemVer(actual string, require string) (bool, error) { if len(require) == 0 { return true, nil } - smeVer := strings.TrimPrefix(actual, "v") + semVer := strings.TrimPrefix(actual, "v") l := strings.ReplaceAll(require, "v", " ") constraint, err := semver.NewConstraint(l) if err != nil { log.Logger.Errorf("fail to new constraint: %s", err.Error()) return false, err } - v, err := semver.NewVersion(smeVer) + v, err := semver.NewVersion(semVer) if err != nil { - log.Logger.Errorf("fail to new version %s: %s", smeVer, err.Error()) + log.Logger.Errorf("fail to new version %s: %s", semVer, err.Error()) return false, err } if constraint.Check(v) { return true, nil } if strings.Contains(actual, "-") && !strings.Contains(require, "-") { - smeVer := strings.TrimPrefix(actual[:strings.Index(actual, "-")], "v") - v, err := semver.NewVersion(smeVer) + semVer := strings.TrimPrefix(actual[:strings.Index(actual, "-")], "v") + if strings.Contains(require, ">=") && require[strings.Index(require, "=")+1:] == semVer { + // for case: `actual` is 1.5.0-beta.1 require is >=`1.5.0` + return false, nil + } + v, err := semver.NewVersion(semVer) if err != nil { - log.Logger.Errorf("fail to new version %s: %s", smeVer, err.Error()) + log.Logger.Errorf("fail to new version %s: %s", semVer, err.Error()) return false, err } if constraint.Check(v) { diff --git a/pkg/addon/addon_suite_test.go b/pkg/addon/addon_suite_test.go index 210ad50d4..b996ca051 100644 --- a/pkg/addon/addon_suite_test.go +++ b/pkg/addon/addon_suite_test.go @@ -26,6 +26,7 @@ import ( appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" types2 "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -396,6 +397,60 @@ var _ = Describe("test enable addon which applies the views independently", func }) }) +var _ = Describe("test override defs of addon", func() { + It("test compDef exist", func() { + ctx := context.Background() + comp := v1beta1.ComponentDefinition{TypeMeta: metav1.TypeMeta{APIVersion: v1beta1.SchemeGroupVersion.String(), Kind: v1beta1.ComponentDefinitionKind}} + Expect(yaml.Unmarshal([]byte(helmCompDefYaml), &comp)).Should(BeNil()) + Expect(k8sClient.Create(ctx, &comp)).Should(BeNil()) + + comp2 := v1beta1.ComponentDefinition{TypeMeta: metav1.TypeMeta{APIVersion: v1beta1.SchemeGroupVersion.String(), Kind: v1beta1.ComponentDefinitionKind}} + Expect(yaml.Unmarshal([]byte(kustomizeCompDefYaml), &comp2)).Should(BeNil()) + Expect(k8sClient.Create(ctx, &comp2)).Should(BeNil()) + app := v1beta1.Application{ObjectMeta: metav1.ObjectMeta{Name: "addon-fluxcd"}} + + comp3 := v1beta1.ComponentDefinition{TypeMeta: metav1.TypeMeta{APIVersion: v1beta1.SchemeGroupVersion.String(), Kind: v1beta1.ComponentDefinitionKind}} + Expect(yaml.Unmarshal([]byte(kustomizeCompDefYaml1), &comp3)).Should(BeNil()) + Expect(k8sClient.Create(ctx, &comp3)).Should(BeNil()) + + compUnstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&comp) + Expect(err).Should(BeNil()) + u := unstructured.Unstructured{Object: compUnstructured} + u.SetAPIVersion(v1beta1.SchemeGroupVersion.String()) + u.SetKind(v1beta1.ComponentDefinitionKind) + c, err := checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app.GetName()) + Expect(err).Should(BeNil()) + Expect(len(c)).Should(BeEquivalentTo(1)) + + u.SetName("rollout") + c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app.GetName()) + Expect(err).Should(BeNil()) + Expect(len(c)).Should(BeEquivalentTo(0)) + + u.SetKind("NotExistKind") + _, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app.GetName()) + Expect(err).ShouldNot(BeNil()) + + compUnstructured2, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&comp2) + Expect(err).Should(BeNil()) + u2 := &unstructured.Unstructured{Object: compUnstructured2} + u2.SetAPIVersion(v1beta1.SchemeGroupVersion.String()) + u2.SetKind(v1beta1.ComponentDefinitionKind) + c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{u2}, app.GetName()) + Expect(err).Should(BeNil()) + Expect(len(c)).Should(BeEquivalentTo(1)) + + compUnstructured3, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&comp3) + Expect(err).Should(BeNil()) + u3 := &unstructured.Unstructured{Object: compUnstructured3} + u3.SetAPIVersion(v1beta1.SchemeGroupVersion.String()) + u3.SetKind(v1beta1.ComponentDefinitionKind) + c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{u3}, app.GetName()) + Expect(err).Should(BeNil()) + Expect(len(c)).Should(BeEquivalentTo(0)) + }) +}) + const ( appYaml = `apiVersion: core.oam.dev/v1beta1 kind: Application @@ -491,5 +546,49 @@ spec: properties: image: crccheck/hello-world port: 8000 +` + helmCompDefYaml = ` +apiVersion: core.oam.dev/v1beta1 +kind: ComponentDefinition +metadata: + name: helm + namespace: vela-system + ownerReferences: + - apiVersion: core.oam.dev/v1beta1 + blockOwnerDeletion: true + controller: true + kind: Application + name: addon-fluxcd-helm + uid: 73c47933-002e-4182-a673-6da6a9dcf080 +spec: + schematic: + cue: +` + kustomizeCompDefYaml = ` +apiVersion: core.oam.dev/v1beta1 +kind: ComponentDefinition +metadata: + name: kustomize + namespace: vela-system +spec: + schematic: + cue: +` + kustomizeCompDefYaml1 = ` +apiVersion: core.oam.dev/v1beta1 +kind: ComponentDefinition +metadata: + name: kustomize-another + namespace: vela-system + ownerReferences: + - apiVersion: core.oam.dev/v1beta1 + blockOwnerDeletion: true + controller: true + kind: Application + name: addon-fluxcd + uid: 73c47933-002e-4182-a673-6da6a9dcf080 +spec: + schematic: + cue: ` ) diff --git a/pkg/addon/addon_test.go b/pkg/addon/addon_test.go index 9cec6358f..56c58db7c 100644 --- a/pkg/addon/addon_test.go +++ b/pkg/addon/addon_test.go @@ -945,6 +945,16 @@ func TestCheckSemVer(t *testing.T) { require: ">=v1.2.4-beta.3", res: false, }, + { + actual: "1.5.0-beta.2", + require: ">=1.5.0", + res: false, + }, + { + actual: "1.5.0-alpha.2", + require: ">=1.5.0", + res: false, + }, } for _, testCase := range testCases { result, err := checkSemVer(testCase.actual, testCase.require) @@ -1306,3 +1316,15 @@ func TestMergeAddonInstallArgs(t *testing.T) { } } + +func TestGenerateConflictError(t *testing.T) { + confictAddon := map[string]string{ + "helm": "definition: helm already exist and not belong to any addon \n", + "kustomize": "definition: kustomize in this addon already exist in fluxcd \n", + } + err := produceDefConflictError(confictAddon) + assert.Error(t, err) + strings.Contains(err.Error(), "in this addon already exist in fluxcd") + + assert.NoError(t, produceDefConflictError(map[string]string{})) +} diff --git a/pkg/addon/utils.go b/pkg/addon/utils.go index 23527f1d8..07af51cdb 100644 --- a/pkg/addon/utils.go +++ b/pkg/addon/utils.go @@ -27,6 +27,8 @@ import ( errors "github.com/pkg/errors" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" + errors2 "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" rest "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,6 +38,7 @@ import ( "github.com/oam-dev/kubevela/pkg/definition" "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/util" + "github.com/oam-dev/kubevela/pkg/utils/addon" "github.com/oam-dev/kubevela/pkg/utils/common" ) @@ -43,12 +46,17 @@ const ( compDefAnnotation = "addon.oam.dev/componentDefinitions" traitDefAnnotation = "addon.oam.dev/traitDefinitions" workflowStepDefAnnotation = "addon.oam.dev/workflowStepDefinitions" + policyDefAnnotation = "addon.oam.dev/policyDefinitions" defKeytemplate = "addon-%s-%s" + compMapKey = "comp" + traitMapKey = "trait" + wfStepMapKey = "wfStep" + policyMapKey = "policy" ) // parse addon's created x-defs in addon-app's annotation, this will be used to check whether app still using it while disabling. func passDefInAppAnnotation(defs []*unstructured.Unstructured, app *v1beta1.Application) error { - var comps, traits, workflowSteps []string + var comps, traits, workflowSteps, policies []string for _, def := range defs { switch def.GetObjectKind().GroupVersionKind().Kind { case v1beta1.ComponentDefinitionKind: @@ -57,6 +65,8 @@ func passDefInAppAnnotation(defs []*unstructured.Unstructured, app *v1beta1.Appl traits = append(traits, def.GetName()) case v1beta1.WorkflowStepDefinitionKind: workflowSteps = append(workflowSteps, def.GetName()) + case v1beta1.PolicyDefinitionKind: + policies = append(policies, def.GetName()) default: return fmt.Errorf("cannot handle definition types %s, name %s", def.GetObjectKind().GroupVersionKind().Kind, def.GetName()) } @@ -70,6 +80,9 @@ func passDefInAppAnnotation(defs []*unstructured.Unstructured, app *v1beta1.Appl if len(workflowSteps) != 0 { app.SetAnnotations(util.MergeMapOverrideWithDst(app.GetAnnotations(), map[string]string{workflowStepDefAnnotation: strings.Join(workflowSteps, ",")})) } + if len(policies) != 0 { + app.SetAnnotations(util.MergeMapOverrideWithDst(app.GetAnnotations(), map[string]string{policyDefAnnotation: strings.Join(policies, ",")})) + } return nil } @@ -87,7 +100,7 @@ func checkAddonHasBeenUsed(ctx context.Context, k8sClient client.Client, name st createdDefs := make(map[string]bool) for key, defNames := range addonApp.GetAnnotations() { switch key { - case compDefAnnotation, traitDefAnnotation, workflowStepDefAnnotation: + case compDefAnnotation, traitDefAnnotation, workflowStepDefAnnotation, policyDefAnnotation: merge2DefMap(key, defNames, createdDefs) } } @@ -102,25 +115,34 @@ func checkAddonHasBeenUsed(ctx context.Context, k8sClient client.Client, name st CHECKNEXT: for _, app := range apps.Items { for _, component := range app.Spec.Components { - if createdDefs[fmt.Sprintf(defKeytemplate, "comp", component.Type)] { + if createdDefs[fmt.Sprintf(defKeytemplate, compMapKey, component.Type)] { res = append(res, app) // this app has used this addon, there is no need check other components continue CHECKNEXT } for _, trait := range component.Traits { - if createdDefs[fmt.Sprintf(defKeytemplate, "trait", trait.Type)] { + if createdDefs[fmt.Sprintf(defKeytemplate, traitMapKey, trait.Type)] { res = append(res, app) continue CHECKNEXT } } } - if app.Spec.Workflow == nil || len(app.Spec.Workflow.Steps) == 0 { - return res, nil + + if app.Spec.Workflow != nil && len(app.Spec.Workflow.Steps) != 0 { + for _, s := range app.Spec.Workflow.Steps { + if createdDefs[fmt.Sprintf(defKeytemplate, wfStepMapKey, s.Type)] { + res = append(res, app) + continue CHECKNEXT + } + } } - for _, s := range app.Spec.Workflow.Steps { - if createdDefs[fmt.Sprintf(defKeytemplate, "wfStep", s.Type)] { - res = append(res, app) - continue CHECKNEXT + + if app.Spec.Policies != nil && len(app.Spec.Policies) != 0 { + for _, p := range app.Spec.Policies { + if createdDefs[fmt.Sprintf(defKeytemplate, policyMapKey, p.Type)] { + res = append(res, app) + continue CHECKNEXT + } } } } @@ -134,11 +156,13 @@ func merge2DefMap(defType string, defNames string, defMap map[string]bool) { for _, defName := range list { switch defType { case compDefAnnotation: - defMap[fmt.Sprintf(template, "comp", defName)] = true + defMap[fmt.Sprintf(template, compMapKey, defName)] = true case traitDefAnnotation: - defMap[fmt.Sprintf(template, "trait", defName)] = true + defMap[fmt.Sprintf(template, traitMapKey, defName)] = true case workflowStepDefAnnotation: - defMap[fmt.Sprintf(template, "wfStep", defName)] = true + defMap[fmt.Sprintf(template, wfStepMapKey, defName)] = true + case policyDefAnnotation: + defMap[fmt.Sprintf(template, policyMapKey, defName)] = true } } } @@ -210,6 +234,8 @@ func findLegacyAddonDefs(ctx context.Context, k8sClient client.Client, addonName defs[fmt.Sprintf(defKeytemplate, "trait", defObject.GetName())] = true case v1beta1.WorkflowStepDefinitionKind: defs[fmt.Sprintf(defKeytemplate, "wfStep", defObject.GetName())] = true + case v1beta1.PolicyDefinitionKind: + } } return nil @@ -242,6 +268,11 @@ func SkipValidateVersion(installer *Installer) { installer.skipVersionValidate = true } +// OverrideDefinitions menas override definitions within this addon if some of them already exist +func OverrideDefinitions(installer *Installer) { + installer.overrideDefs = true +} + // IsAddonDir validates an addon directory. // It checks required files like metadata.yaml and template.yaml func IsAddonDir(dirName string) (bool, error) { @@ -409,3 +440,37 @@ func generateAnnotation(meta *Meta) map[string]string { func isErrorCueRenderPathNotFound(err error, path string) bool { return err.Error() == fmt.Sprintf("var(path=%s) not exist", path) } + +func checkConflictDefs(ctx context.Context, k8sClient client.Client, defs []*unstructured.Unstructured, appName string) (map[string]string, error) { + res := map[string]string{} + for _, def := range defs { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(def), def) + if err == nil { + owner := metav1.GetControllerOf(def) + if owner == nil || owner.Kind != v1beta1.ApplicationKind { + res[def.GetName()] = fmt.Sprintf("definition: %s already exist and not belong to any addon \n", def.GetName()) + continue + } + if owner.Name != appName { + // if addon not belong to an addon or addon name is another one, we should put them in result + res[def.GetName()] = fmt.Sprintf("definition: %s in this addon already exist in %s \n", def.GetName(), addon.AppName2Addon(appName)) + } + } + if err != nil && !errors2.IsNotFound(err) { + return nil, err + } + } + return res, nil +} + +func produceDefConflictError(conflictDefs map[string]string) error { + if len(conflictDefs) == 0 { + return nil + } + var errorInfo string + for _, s := range conflictDefs { + errorInfo += s + } + errorInfo += "if you want override them, please use argument '--override-definitions' to enable \n" + return errors.New(errorInfo) +} diff --git a/pkg/addon/utils_test.go b/pkg/addon/utils_test.go index b911880b7..7c01cd928 100644 --- a/pkg/addon/utils_test.go +++ b/pkg/addon/utils_test.go @@ -108,9 +108,13 @@ var _ = Describe("Test definition check", func() { Expect(yaml.Unmarshal([]byte(testApp3Yaml), &app3)).Should(BeNil()) Expect(k8sClient.Create(ctx, &app3)).Should(BeNil()) + app4 := v1beta1.Application{} + Expect(yaml.Unmarshal([]byte(testApp4Yaml), &app4)).Should(BeNil()) + Expect(k8sClient.Create(ctx, &app4)).Should(BeNil()) + usedApps, err := checkAddonHasBeenUsed(ctx, k8sClient, "my-addon", addonApp, cfg) Expect(err).Should(BeNil()) - Expect(len(usedApps)).Should(BeEquivalentTo(3)) + Expect(len(usedApps)).Should(BeEquivalentTo(4)) }) It("check fetch lagacy addon definitions", func() { @@ -319,6 +323,7 @@ metadata: addon.oam.dev/componentDefinitions: "my-comp" addon.oam.dev/traitDefinitions: "my-trait" addon.oam.dev/workflowStepDefinitions: "my-wfstep" + addon.oam.dev/policyDefinitions: "my-policy" name: addon-myaddon namespace: vela-system spec: @@ -367,6 +372,22 @@ spec: - type: my-wfstep name: deploy ` + testApp4Yaml = ` +apiVersion: core.oam.dev/v1beta1 +kind: Application +metadata: + name: app-4 + namespace: test-ns +spec: + components: + - name: podinfo + type: webservice + + policies: + - type: my-policy + name: topology +` + registryCmYaml = ` apiVersion: v1 data: diff --git a/references/cli/addon.go b/references/cli/addon.go index bd0b0e49d..f1a583de5 100644 --- a/references/cli/addon.go +++ b/references/cli/addon.go @@ -77,6 +77,8 @@ var verboseStatus bool var skipValidate bool +var overrideDefs bool + // NewAddonCommand create `addon` command func NewAddonCommand(c common.Args, order string, ioStreams cmdutil.IOStreams) *cobra.Command { cmd := &cobra.Command{ @@ -203,6 +205,7 @@ func NewAddonEnableCommand(c common.Args, ioStream cmdutil.IOStreams) *cobra.Com cmd.Flags().StringVarP(&addonVersion, "version", "v", "", "specify the addon version to enable") cmd.Flags().StringVarP(&addonClusters, types.ClustersArg, "c", "", "specify the runtime-clusters to enable") cmd.Flags().BoolVarP(&skipValidate, "skip-version-validating", "s", false, "skip validating system version requirement") + cmd.Flags().BoolVarP(&overrideDefs, "override-definitions", "", false, "override existing definitions if conflict with those contained in this addon") return cmd } @@ -323,6 +326,7 @@ non-empty new arg } cmd.Flags().StringVarP(&addonVersion, "version", "v", "", "specify the addon version to upgrade") cmd.Flags().BoolVarP(&skipValidate, "skip-version-validating", "s", false, "skip validating system version requirement") + cmd.Flags().BoolVarP(&overrideDefs, "override-definitions", "", false, "override existing definitions if conflict with those contained in this addon") return cmd } @@ -535,6 +539,9 @@ func enableAddon(ctx context.Context, k8sClient client.Client, dc *discovery.Dis if skipValidate { opts = append(opts, pkgaddon.SkipValidateVersion) } + if overrideDefs { + opts = append(opts, pkgaddon.OverrideDefinitions) + } err = pkgaddon.EnableAddon(ctx, name, version, k8sClient, dc, apply.NewAPIApplicator(k8sClient), config, registry, args, nil, opts...) if errors.Is(err, pkgaddon.ErrNotExist) { continue @@ -570,6 +577,9 @@ func enableAddonByLocal(ctx context.Context, name string, dir string, k8sClient if skipValidate { opts = append(opts, pkgaddon.SkipValidateVersion) } + if overrideDefs { + opts = append(opts, pkgaddon.OverrideDefinitions) + } if err := pkgaddon.EnableAddonByLocalDir(ctx, name, dir, k8sClient, dc, apply.NewAPIApplicator(k8sClient), config, args, opts...); err != nil { return err }