Fix: check definition of addon whether is conflict (#4493)

* fix checksemver

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

override defs

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

add tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

* add test and fix some special cases

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix checkdiff

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix flags

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

* fix comments

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

* small fix

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

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) {

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:
 `
 )

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{}))
+}

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)
+}

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:

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
 	}