Fix: enforce TraitDefinition conflictsWith at admission (#7303)

* fix: enforce TraitDefinition conflictsWith at admission Validate attached traits in the Application webhook using GetCapabilityDefinition so namespaced and revisioned TraitDefinitions are resolved correctly. Keep the check outside ValidateComponents so it still runs under sharding. Signed-off-by: Junhuan Zheng <3373484735@qq.com> * fix: address cubic review on trait conflict validation Fail closed on definition lookup errors, compare API groups exactly for *.group rules, and simplify conflict test object setup. Signed-off-by: hwan <3373484735@qq.com> * test: reuse baseObjects for crd conflict case Avoid duplicating the TraitDefinition fixture list in the crd-name conflict test case. Signed-off-by: hwan <3373484735@qq.com> * fix: convert conflict tests to Ginkgo and log lookup failures Address maintainer feedback by rewriting trait conflict tests in Ginkgo and logging TraitDefinition resolution errors when failing closed. Signed-off-by: hwan <3373484735@qq.com> * style: gofmt the trait conflict test var block gofmt aligns the variable declarations in the ValidateTraitConflicts test block so the repo's reviewable check passes. Signed-off-by: Junhuan Zheng <3373484735@qq.com> --------- Signed-off-by: Junhuan Zheng <3373484735@qq.com> Signed-off-by: hwan <3373484735@qq.com>
2026-08-19 04:26:39 +00:00 · 2026-08-13 11:34:20 +01:00
parent 334e41060f
commit dc5674c12b
2 changed files with 339 additions and 0 deletions
@@ -20,12 +20,14 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 	"time"

 	"github.com/kubevela/pkg/controller/sharding"
 	"github.com/kubevela/pkg/util/singleton"
 	authv1 "k8s.io/api/authorization/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/klog/v2"
@@ -36,6 +38,7 @@ import (
 	"github.com/oam-dev/kubevela/pkg/appfile"
 	"github.com/oam-dev/kubevela/pkg/features"
 	"github.com/oam-dev/kubevela/pkg/oam"
+	oamutil "github.com/oam-dev/kubevela/pkg/oam/util"
 )

 // ValidateWorkflow validates the Application workflow
@@ -452,6 +455,121 @@ func (h *ValidatingHandler) ValidateAnnotations(_ context.Context, app *v1beta1.
 	return annotationsErrs
 }

+// ValidateTraitConflicts validates TraitDefinition.spec.conflictsWith for traits
+// attached to the same component. Kept outside ValidateComponents so it still
+// runs when sharding skips component schematic validation.
+func (h *ValidatingHandler) ValidateTraitConflicts(ctx context.Context, app *v1beta1.Application) field.ErrorList {
+	var errs field.ErrorList
+	defCtx := oamutil.SetNamespaceInCtx(ctx, app.Namespace)
+
+	// Cache resolved TraitDefinitions across components so each trait type is fetched once.
+	defCache := make(map[string]*v1beta1.TraitDefinition)
+	getTraitDefinition := func(traitType string) (*v1beta1.TraitDefinition, error) {
+		if def, ok := defCache[traitType]; ok {
+			return def, nil
+		}
+		def := &v1beta1.TraitDefinition{}
+		if err := oamutil.GetCapabilityDefinition(defCtx, h.Client, def, traitType, app.GetAnnotations()); err != nil {
+			if errors.IsNotFound(err) {
+				// Existence is validated elsewhere; missing defs are skipped here.
+				defCache[traitType] = nil
+				return nil, nil
+			}
+			return nil, err
+		}
+		defCache[traitType] = def
+		return def, nil
+	}
+
+	for compIdx, comp := range app.Spec.Components {
+		if len(comp.Traits) < 2 {
+			continue
+		}
+
+		type attachedTrait struct {
+			traitIdx int
+			def      *v1beta1.TraitDefinition
+		}
+		var attached []attachedTrait
+		for i, trait := range comp.Traits {
+			def, err := getTraitDefinition(trait.Type)
+			if err != nil {
+				// Fail closed so unresolved definitions cannot bypass conflict checks, but
+				// log so operators can distinguish transient API/cache failures from policy rejects.
+				klog.Errorf("Failed to resolve TraitDefinition %q for conflict validation: %v", trait.Type, err)
+				errs = append(errs, field.InternalError(
+					field.NewPath("spec", "components").Index(compIdx).Child("traits").Index(i).Child("type"),
+					err))
+				continue
+			}
+			if def != nil {
+				attached = append(attached, attachedTrait{traitIdx: i, def: def})
+			}
+		}
+
+		for i := 0; i < len(attached); i++ {
+			for j := i + 1; j < len(attached); j++ {
+				first, second := attached[i], attached[j]
+				// Matching is unidirectional: either side declaring the other is enough.
+				if traitConflictsWith(first.def, second.def) || traitConflictsWith(second.def, first.def) {
+					errs = append(errs, field.Invalid(
+						field.NewPath("spec", "components").Index(compIdx).Child("traits").Index(second.traitIdx).Child("type"),
+						second.def.Name,
+						fmt.Sprintf("trait %q conflicts with trait %q on component %q", first.def.Name, second.def.Name, comp.Name)))
+				}
+			}
+		}
+	}
+
+	return errs
+}
+
+// traitConflictsWith reports whether def's conflictsWith rules match target.
+func traitConflictsWith(def, target *v1beta1.TraitDefinition) bool {
+	for _, rule := range def.Spec.ConflictsWith {
+		if traitConflictRuleMatches(rule, target) {
+			return true
+		}
+	}
+	return false
+}
+
+// traitConflictRuleMatches checks a single conflictsWith rule against a TraitDefinition.
+// Supported rule forms (see TraitDefinitionSpec.ConflictsWith docs):
+//   - "*"                    matches any trait
+//   - "<definition-name>"    matches the trait definition's name
+//   - "<crd-name>"           matches the trait's referenced CRD name (definitionRef.name)
+//   - "*.<group>"            matches any CRD in the given API group
+//   - "labelSelector:<expr>" matches the trait definition's labels against a label selector
+func traitConflictRuleMatches(rule string, target *v1beta1.TraitDefinition) bool {
+	switch {
+	case rule == "*":
+		return true
+	case strings.HasPrefix(rule, "labelSelector:"):
+		selector, err := labels.Parse(strings.TrimPrefix(rule, "labelSelector:"))
+		if err != nil {
+			return false
+		}
+		return selector.Matches(labels.Set(target.GetLabels()))
+	case rule == target.Name:
+		return true
+	case target.Spec.Reference.Name != "" && rule == target.Spec.Reference.Name:
+		return true
+	case strings.HasPrefix(rule, "*."):
+		group := strings.TrimPrefix(rule, "*.")
+		refName := target.Spec.Reference.Name
+		if refName == "" {
+			return false
+		}
+		if dot := strings.Index(refName, "."); dot >= 0 {
+			return refName[dot+1:] == group
+		}
+		return false
+	default:
+		return false
+	}
+}
+
 // ValidateCreate validates the Application on creation
 func (h *ValidatingHandler) ValidateCreate(ctx context.Context, app *v1beta1.Application, req admission.Request) field.ErrorList {
 	var errs field.ErrorList
@@ -460,6 +578,7 @@ func (h *ValidatingHandler) ValidateCreate(ctx context.Context, app *v1beta1.App
 	errs = append(errs, h.ValidateDefinitionPermissions(ctx, app, req)...)
 	errs = append(errs, h.ValidateWorkflow(ctx, app)...)
 	errs = append(errs, h.ValidateComponents(ctx, app)...)
+	errs = append(errs, h.ValidateTraitConflicts(ctx, app)...)
 	return errs
 }

@@ -0,0 +1,220 @@
+/*
+Copyright 2026 The KubeVela Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package application
+
+import (
+	"context"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/common"
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
+	"github.com/oam-dev/kubevela/pkg/oam"
+)
+
+var _ = Describe("Trait conflict validation", func() {
+	Describe("traitConflictRuleMatches", func() {
+		cueTrait := &v1beta1.TraitDefinition{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "scaler",
+				Labels: map[string]string{"team": "platform"},
+			},
+		}
+		crdTrait := &v1beta1.TraitDefinition{
+			ObjectMeta: metav1.ObjectMeta{Name: "service"},
+			Spec: v1beta1.TraitDefinitionSpec{
+				Reference: common.DefinitionReference{Name: "services.k8s.io"},
+			},
+		}
+		nestedGroupTrait := &v1beta1.TraitDefinition{
+			ObjectMeta: metav1.ObjectMeta{Name: "ingress"},
+			Spec: v1beta1.TraitDefinitionSpec{
+				Reference: common.DefinitionReference{Name: "ingresses.networking.k8s.io"},
+			},
+		}
+
+		DescribeTable("rule matching",
+			func(rule string, target *v1beta1.TraitDefinition, want bool) {
+				Expect(traitConflictRuleMatches(rule, target)).To(Equal(want))
+			},
+			Entry("wildcard", "*", cueTrait, true),
+			Entry("definition name match", "scaler", cueTrait, true),
+			Entry("definition name miss", "ingress", cueTrait, false),
+			Entry("crd name match", "services.k8s.io", crdTrait, true),
+			Entry("crd name ignored for empty reference", "services.k8s.io", cueTrait, false),
+			Entry("group wildcard match", "*.k8s.io", crdTrait, true),
+			Entry("group wildcard does not match nested group suffix", "*.k8s.io", nestedGroupTrait, false),
+			Entry("group wildcard miss", "*.networking.k8s.io", crdTrait, false),
+			Entry("group wildcard ignored for empty reference", "*.k8s.io", cueTrait, false),
+			Entry("label selector match", "labelSelector:team=platform", cueTrait, true),
+			Entry("label selector miss", "labelSelector:team=edge", cueTrait, false),
+			Entry("invalid label selector", "labelSelector:@@@", cueTrait, false),
+		)
+	})
+
+	Describe("ValidateTraitConflicts", func() {
+		var (
+			scheme        *runtime.Scheme
+			conflictA     *v1beta1.TraitDefinition
+			conflictB     *v1beta1.TraitDefinition
+			scaler        *v1beta1.TraitDefinition
+			service       *v1beta1.TraitDefinition
+			ingress       *v1beta1.TraitDefinition
+			gateway       *v1beta1.TraitDefinition
+			labelConflict *v1beta1.TraitDefinition
+			nsConflictA   *v1beta1.TraitDefinition
+			baseObjects   []runtime.Object
+		)
+
+		BeforeEach(func() {
+			scheme = runtime.NewScheme()
+			Expect(v1beta1.AddToScheme(scheme)).To(Succeed())
+
+			conflictA = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "conflict-a", Namespace: oam.SystemDefinitionNamespace},
+				Spec:       v1beta1.TraitDefinitionSpec{ConflictsWith: []string{"conflict-b"}},
+			}
+			conflictB = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "conflict-b", Namespace: oam.SystemDefinitionNamespace},
+				Spec:       v1beta1.TraitDefinitionSpec{},
+			}
+			scaler = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "scaler", Namespace: oam.SystemDefinitionNamespace},
+				Spec:       v1beta1.TraitDefinitionSpec{},
+			}
+			service = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "service", Namespace: oam.SystemDefinitionNamespace},
+				Spec: v1beta1.TraitDefinitionSpec{
+					Reference: common.DefinitionReference{Name: "services.k8s.io"},
+				},
+			}
+			ingress = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "ingress",
+					Namespace: oam.SystemDefinitionNamespace,
+					Labels:    map[string]string{"feature": "expose"},
+				},
+				Spec: v1beta1.TraitDefinitionSpec{
+					Reference:     common.DefinitionReference{Name: "ingresses.networking.k8s.io"},
+					ConflictsWith: []string{"*.networking.k8s.io"},
+				},
+			}
+			gateway = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "gateway", Namespace: oam.SystemDefinitionNamespace},
+				Spec: v1beta1.TraitDefinitionSpec{
+					Reference: common.DefinitionReference{Name: "gateways.networking.k8s.io"},
+				},
+			}
+			labelConflict = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "label-conflict", Namespace: oam.SystemDefinitionNamespace},
+				Spec:       v1beta1.TraitDefinitionSpec{ConflictsWith: []string{"labelSelector:feature=expose"}},
+			}
+			nsConflictA = &v1beta1.TraitDefinition{
+				ObjectMeta: metav1.ObjectMeta{Name: "conflict-a", Namespace: "default"},
+				Spec:       v1beta1.TraitDefinitionSpec{ConflictsWith: []string{"scaler"}},
+			}
+
+			baseObjects = []runtime.Object{
+				conflictA.DeepCopy(), conflictB.DeepCopy(), scaler.DeepCopy(),
+				service.DeepCopy(), ingress.DeepCopy(), gateway.DeepCopy(), labelConflict.DeepCopy(),
+			}
+		})
+
+		validateWith := func(objects []runtime.Object, traits []common.ApplicationTrait) int {
+			handler := &ValidatingHandler{
+				Client: fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objects...).Build(),
+			}
+			app := &v1beta1.Application{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-app", Namespace: "default"},
+				Spec: v1beta1.ApplicationSpec{
+					Components: []common.ApplicationComponent{{
+						Name:   "web",
+						Type:   "webservice",
+						Traits: traits,
+					}},
+				},
+			}
+			return len(handler.ValidateTraitConflicts(context.Background(), app))
+		}
+
+		It("rejects unidirectional definition-name conflict", func() {
+			Expect(validateWith(baseObjects, []common.ApplicationTrait{
+				{Type: "conflict-a"},
+				{Type: "conflict-b"},
+			})).To(Equal(1))
+		})
+
+		It("allows non-conflicting traits", func() {
+			Expect(validateWith(baseObjects, []common.ApplicationTrait{
+				{Type: "conflict-a"},
+				{Type: "scaler"},
+			})).To(Equal(0))
+		})
+
+		It("allows a single trait", func() {
+			Expect(validateWith(baseObjects, []common.ApplicationTrait{
+				{Type: "conflict-a"},
+			})).To(Equal(0))
+		})
+
+		It("rejects crd name conflict", func() {
+			objects := make([]runtime.Object, 0, len(baseObjects))
+			for _, o := range baseObjects {
+				if td, ok := o.(*v1beta1.TraitDefinition); ok && td.Name == "conflict-a" {
+					td = td.DeepCopy()
+					td.Spec.ConflictsWith = []string{"services.k8s.io"}
+					objects = append(objects, td)
+					continue
+				}
+				objects = append(objects, o.DeepCopyObject())
+			}
+			Expect(validateWith(objects, []common.ApplicationTrait{
+				{Type: "conflict-a"},
+				{Type: "service"},
+			})).To(Equal(1))
+		})
+
+		It("rejects group wildcard conflict", func() {
+			Expect(validateWith(baseObjects, []common.ApplicationTrait{
+				{Type: "ingress"},
+				{Type: "gateway"},
+			})).To(Equal(1))
+		})
+
+		It("rejects labelSelector conflict", func() {
+			Expect(validateWith(baseObjects, []common.ApplicationTrait{
+				{Type: "label-conflict"},
+				{Type: "ingress"},
+			})).To(Equal(1))
+		})
+
+		It("prefers namespaced TraitDefinition over system definition", func() {
+			objects := append([]runtime.Object{}, baseObjects...)
+			objects = append(objects, nsConflictA.DeepCopy())
+			// System conflict-a only conflicts with conflict-b. The namespaced
+			// override conflicts with scaler and must win the lookup.
+			Expect(validateWith(objects, []common.ApplicationTrait{
+				{Type: "conflict-a"},
+				{Type: "scaler"},
+			})).To(Equal(1))
+		})
+	})
+})