Fix: Enhance shared resource handling to avoid last-applied-configuration pollution (#6998) (#7001)

(cherry picked from commit 552764d48f)

Signed-off-by: Brian Kane <briankane1@gmail.com>
Co-authored-by: Ayush Kumar <65535504+roguepikachu@users.noreply.github.com>

hack/debug-webhook-setup.sh

@@ -199,6 +199,18 @@ webhooks:
   admissionReviewVersions: ["v1", "v1beta1"]
   sideEffects: None
   failurePolicy: Fail
+- name: applications.core.oam.dev
+  clientConfig:
+    url: https://${HOST_IP}:9445/validating-core-oam-dev-v1beta1-applications
+    caBundle: ${CA_BUNDLE}
+  rules:
+  - apiGroups: ["core.oam.dev"]
+    apiVersions: ["v1beta1"]
+    resources: ["applications"]
+    operations: ["CREATE", "UPDATE"]
+  admissionReviewVersions: ["v1", "v1beta1"]
+  sideEffects: None
+  failurePolicy: Fail
 EOF
 
     kubectl apply -f /tmp/webhook-config.yaml

pkg/definition/ast/transformer.go

@@ -21,7 +21,6 @@ import (
 	"strings"
 
 	"cuelang.org/go/cue/ast"
-	"cuelang.org/go/cue/parser"
 	"cuelang.org/go/cue/token"
 )
 
@@ -111,14 +110,15 @@ func unmarshalField[T ast.Node](field *ast.Field, key string, validator func(T)
 		}
 
 		unquoted := strings.TrimSpace(TrimCueRawString(basicLit.Value))
-		expr, err := parser.ParseExpr("-", WrapCueStruct(unquoted))
-		if err != nil {
-			return fmt.Errorf("unexpected error re-parsing validated %s string: %w", key, err)
+
+		structLit, hasImports, hasPackage, parseErr := ParseCueContent(unquoted)
+		if parseErr != nil {
+			return fmt.Errorf("unexpected error re-parsing validated %s string: %w", key, parseErr)
 		}
 
-		structLit, ok := expr.(*ast.StructLit)
-		if !ok {
-			return fmt.Errorf("expected struct after validation in field %s", key)
+		if hasImports || hasPackage {
+			// Keep as string literal to preserve imports/package
+			return nil
 		}
 
 		statusField.Value = structLit

pkg/definition/ast/transformer_test.go

@@ -22,6 +22,7 @@ import (
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/format"
 	"cuelang.org/go/cue/parser"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -176,6 +177,50 @@ func TestMarshalAndUnmarshalMetadata(t *testing.T) {
 	        `,
 			expectContains: "$local",
 		},
+		{
+			name: "status details with import statement should work",
+			input: `
+				attributes: {
+					status: {
+						details: #"""
+							import "strconv"
+							replicas: strconv.Atoi(context.output.status.replicas)
+						"""#
+					}
+				}
+			`,
+			expectContains: "import \"strconv\"",
+		},
+		{
+			name: "status details with package declaration",
+			input: `
+				attributes: {
+					status: {
+						details: #"""
+							package status
+							
+							ready: true
+							phase: "Running"
+						"""#
+					}
+				}
+			`,
+			expectContains: "package status",
+		},
+		{
+			name: "status details with import cannot bypass validation",
+			input: `
+				attributes: {
+					status: {
+						details: #"""
+							import "strings"
+							data: { nested: "structure" }
+						"""#
+					}
+				}
+			`,
+			expectMarshalErr: "unsupported expression type",
+		},
 	}
 
 	for _, tt := range tests {
@@ -379,6 +424,21 @@ func TestMarshalAndUnmarshalHealthPolicy(t *testing.T) {
 			`,
 			expectContains: "isHealth",
 		},
+		{
+			name: "healthPolicy with package declaration",
+			input: `
+				attributes: {
+					status: {
+						healthPolicy: #"""
+							package health
+							
+							isHealth: context.output.status.phase == "Running"
+						"""#
+					}
+				}
+			`,
+			expectContains: "package health",
+		},
 	}
 
 	for _, tt := range tests {
@@ -610,6 +670,96 @@ func TestMarshalAndUnmarshalCustomStatus(t *testing.T) {
 			`,
 			expectContains: "message",
 		},
+		{
+			name: "customStatus with import statement should work",
+			input: `
+				attributes: {
+					status: {
+						customStatus: #"""
+							import "strings"
+							message: strings.Join(["hello", "world"], ",")
+						"""#
+					}
+				}
+			`,
+			expectContains: "import \"strings\"",
+		},
+		{
+			name: "customStatus with multiple imports",
+			input: `
+				attributes: {
+					status: {
+						customStatus: #"""
+							import "strings"
+							import "strconv"
+							count: strconv.Atoi("42")
+							message: strings.Join(["Count", strconv.FormatInt(count, 10)], ": ")
+						"""#
+					}
+				}
+			`,
+			expectContains: "import \"strconv\"",
+		},
+		{
+			name: "customStatus with import alias",
+			input: `
+				attributes: {
+					status: {
+						customStatus: #"""
+							import str "strings"
+							message: str.ToUpper(str.Join(["hello", "world"], " "))
+						"""#
+					}
+				}
+			`,
+			expectContains: "import str \"strings\"",
+		},
+		{
+			name: "customStatus with package declaration",
+			input: `
+				attributes: {
+					status: {
+						customStatus: #"""
+							package mytest
+							
+							message: "Package test"
+						"""#
+					}
+				}
+			`,
+			expectContains: "package mytest",
+		},
+		{
+			name: "customStatus with package and imports",
+			input: `
+				attributes: {
+					status: {
+						customStatus: #"""
+							package mytest
+							
+							import "strings"
+							
+							message: strings.ToUpper("hello world")
+						"""#
+					}
+				}
+			`,
+			expectContains: "package mytest",
+		},
+		{
+			name: "customStatus with import still requires message field",
+			input: `
+				attributes: {
+					status: {
+						customStatus: #"""
+							import "strings"
+							someOtherField: "value"
+						"""#
+					}
+				}
+			`,
+			expectMarshalErr: "customStatus must contain a 'message' field",
+		},
 	}
 
 	for _, tt := range tests {
@@ -951,6 +1101,57 @@ func TestCustomStatusEdgeCases(t *testing.T) {
 	}
 }
 
+func TestMixedFieldsWithAndWithoutImports(t *testing.T) {
+	input := `
+		attributes: {
+			status: {
+				healthPolicy: #"""
+					isHealth: context.output.status.phase == "Running"
+				"""#
+				customStatus: #"""
+					import "strings"
+					message: strings.ToLower(context.output.status.phase)
+				"""#
+			}
+		}
+	`
+
+	file, err := parser.ParseFile("-", input)
+	require.NoError(t, err)
+
+	var rootField *ast.Field
+	for _, decl := range file.Decls {
+		if f, ok := decl.(*ast.Field); ok {
+			rootField = f
+			break
+		}
+	}
+	require.NotNil(t, rootField)
+
+	// Encode (struct -> string)
+	err = EncodeMetadata(rootField)
+	require.NoError(t, err)
+
+	// Decode (string -> struct/string based on imports)
+	err = DecodeMetadata(rootField)
+	require.NoError(t, err)
+
+	// Check healthPolicy (no imports) - should be decoded to struct
+	healthField, ok := GetFieldByPath(rootField, "attributes.status.healthPolicy")
+	require.True(t, ok)
+	_, isStruct := healthField.Value.(*ast.StructLit)
+	assert.True(t, isStruct, "healthPolicy without imports should be decoded to struct")
+
+	// Check customStatus (has imports) - should remain as string
+	customField, ok := GetFieldByPath(rootField, "attributes.status.customStatus")
+	require.True(t, ok)
+	basicLit, isString := customField.Value.(*ast.BasicLit)
+	assert.True(t, isString, "customStatus with imports should remain as string")
+	if isString {
+		assert.Contains(t, basicLit.Value, "import \"strings\"")
+	}
+}
+
 func TestBackwardCompatibility(t *testing.T) {
 	tests := []struct {
 		name  string

pkg/definition/ast/utils.go

@@ -152,18 +152,15 @@ func ValidateCueStringLiteral[T ast.Node](lit *ast.BasicLit, validator func(T) e
 		return nil
 	}
 
-	wrapped := WrapCueStruct(raw)
-
-	expr, err := parser.ParseExpr("-", wrapped)
+	structLit, _, _, err := ParseCueContent(raw)
 	if err != nil {
 		return fmt.Errorf("invalid cue content in string literal: %w", err)
 	}
 
-	node, ok := expr.(T)
+	node, ok := ast.Node(structLit).(T)
 	if !ok {
 		return fmt.Errorf("parsed expression is not of expected type %T", *new(T))
 	}
-
 	return validator(node)
 }
 
@@ -197,6 +194,36 @@ func WrapCueStruct(s string) string {
 	return fmt.Sprintf("{\n%s\n}", s)
 }
 
+// ParseCueContent parses CUE content and extracts struct fields, skipping imports/packages
+func ParseCueContent(content string) (*ast.StructLit, bool, bool, error) {
+	if strings.TrimSpace(content) == "" {
+		return &ast.StructLit{Elts: []ast.Decl{}}, false, false, nil
+	}
+
+	file, err := parser.ParseFile("-", content)
+	if err != nil {
+		return nil, false, false, err
+	}
+
+	hasImports := len(file.Imports) > 0
+	hasPackage := file.PackageName() != ""
+
+	structLit := &ast.StructLit{
+		Elts: []ast.Decl{},
+	}
+
+	for _, decl := range file.Decls {
+		switch decl.(type) {
+		case *ast.ImportDecl, *ast.Package:
+			// Skip imports and package declarations
+		default:
+			structLit.Elts = append(structLit.Elts, decl)
+		}
+	}
+
+	return structLit, hasImports, hasPackage, nil
+}
+
 // FindAndValidateField searches for a field at the top level or within top-level if statements
 func FindAndValidateField(sl *ast.StructLit, fieldName string, validator fieldValidator) (found bool, err error) {
 	// First check top-level fields

pkg/utils/apply/apply.go

@@ -18,7 +18,6 @@ package apply
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"reflect"
 	"strconv"
@@ -207,6 +206,20 @@ func (a *APIApplicator) Apply(ctx context.Context, desired client.Object, ao ...
 		return nil
 	}
 
+	// Short-circuit for shared resources: only patch the shared-by annotation
+	// This avoids the three-way merge which could pollute last-applied-configuration
+	if applyAct.isShared {
+		loggingApply("patching shared resource annotation only", desired, applyAct.quiet)
+		sharedBy := desired.GetAnnotations()[oam.AnnotationAppSharedBy]
+		patch := []byte(fmt.Sprintf(`{"metadata":{"annotations":{"%s":"%s"}}}`, oam.AnnotationAppSharedBy, sharedBy))
+		var patchOpts []client.PatchOption
+		if applyAct.dryRun {
+			patchOpts = append(patchOpts, client.DryRunAll)
+		}
+		return errors.Wrapf(a.c.Patch(ctx, existing, client.RawPatch(types.MergePatchType, patch), patchOpts...),
+			"cannot patch shared resource annotation")
+	}
+
 	strategy := applyAct.updateStrategy
 	if strategy.Op == "" {
 		if utilfeature.DefaultMutableFeatureGate.Enabled(features.ApplyResourceByReplace) && isUpdatableResource(desired) {
@@ -478,39 +491,39 @@ func DisableUpdateAnnotation() ApplyOption {
 // SharedByApp let the resource be sharable
 func SharedByApp(app *v1beta1.Application) ApplyOption {
 	return func(act *applyAction, existing, desired client.Object) error {
-		// calculate the shared-by annotation
-		// if resource exists, add the current application into the resource shared-by field
+		// Calculate the shared-by annotation value
 		var sharedBy string
 		if existing != nil && existing.GetAnnotations() != nil {
 			sharedBy = existing.GetAnnotations()[oam.AnnotationAppSharedBy]
 		}
 		sharedBy = AddSharer(sharedBy, app)
+
+		// Always add the shared-by annotation to desired (for create case)
 		util.AddAnnotations(desired, map[string]string{oam.AnnotationAppSharedBy: sharedBy})
+
 		if existing == nil {
 			return nil
 		}
 
-		// resource exists and controlled by current application
+		// Resource exists - check if controlled by current application
 		appKey, controlledBy := GetAppKey(app), GetControlledBy(existing)
 		if controlledBy == "" || appKey == controlledBy {
+			// Owner app - use normal three-way merge flow
 			return nil
 		}
 
-		// resource exists but not controlled by current application
+		// Resource exists but not controlled by current application
 		if existing.GetAnnotations() == nil || existing.GetAnnotations()[oam.AnnotationAppSharedBy] == "" {
-			// if the application that controls the resource does not allow sharing, return error
+			// Owner doesn't allow sharing
 			return fmt.Errorf("application is controlled by %s but is not sharable", controlledBy)
 		}
-		// the application that controls the resource allows sharing, then only mutate the shared-by annotation
+
+		// Non-owner sharer: set flags for short-circuit in Apply()
+		// The short-circuit will only patch the shared-by annotation, avoiding
+		// any manipulation of the resource spec or last-applied-configuration
 		act.isShared = true
-		bs, err := json.Marshal(existing)
-		if err != nil {
-			return err
-		}
-		if err = json.Unmarshal(bs, desired); err != nil {
-			return err
-		}
-		util.AddAnnotations(desired, map[string]string{oam.AnnotationAppSharedBy: sharedBy})
+		act.updateAnnotation = false
+
 		return nil
 	}
 }

pkg/utils/apply/apply_test.go

@@ -445,10 +445,11 @@ func TestSharedByApp(t *testing.T) {
 	app := &v1beta1.Application{ObjectMeta: metav1.ObjectMeta{Name: "app"}}
 	ao := SharedByApp(app)
 	testCases := map[string]struct {
-		existing client.Object
-		desired  client.Object
-		output   client.Object
-		hasError bool
+		existing       client.Object
+		desired        client.Object
+		output         client.Object
+		hasError       bool
+		expectIsShared bool
 	}{
 		"create new resource": {
 			existing: nil,
@@ -492,17 +493,17 @@ func TestSharedByApp(t *testing.T) {
 				"kind": "ConfigMap",
 				"data": "y",
 			}},
+			// Non-owner sharer: desired only gets the shared-by annotation added
+			// The actual resource content is NOT modified - the short-circuit in Apply()
+			// will patch only the annotation on the server
 			output: &unstructured.Unstructured{Object: map[string]interface{}{
 				"kind": "ConfigMap",
 				"metadata": map[string]interface{}{
-					"labels": map[string]interface{}{
-						oam.LabelAppName:      "example",
-						oam.LabelAppNamespace: "default",
-					},
 					"annotations": map[string]interface{}{oam.AnnotationAppSharedBy: "x/y,default/app"},
 				},
-				"data": "x",
+				"data": "y",
 			}},
+			expectIsShared: true,
 		},
 		"add sharer to existing sharing resource owned by self": {
 			existing: &unstructured.Unstructured{Object: map[string]interface{}{
@@ -554,16 +555,102 @@ func TestSharedByApp(t *testing.T) {
 			}},
 			hasError: true,
 		},
+		"non-owner sharer sets short-circuit flags": {
+			existing: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Pod",
+				"metadata": map[string]interface{}{
+					"name":            "test-pod",
+					"resourceVersion": "12345",
+					"labels": map[string]interface{}{
+						oam.LabelAppName:      "app1",
+						oam.LabelAppNamespace: "default",
+					},
+					"annotations": map[string]interface{}{
+						oam.AnnotationAppSharedBy: "default/app1",
+					},
+				},
+			}},
+			desired: &unstructured.Unstructured{Object: map[string]interface{}{
+				"kind": "Pod",
+				"metadata": map[string]interface{}{
+					"name": "test-pod",
+				},
+			}},
+			// For non-owner sharers, desired only gets the shared-by annotation added
+			// The actual patching happens in Apply() via short-circuit
+			output: &unstructured.Unstructured{Object: map[string]interface{}{
+				"kind": "Pod",
+				"metadata": map[string]interface{}{
+					"name": "test-pod",
+					"annotations": map[string]interface{}{
+						oam.AnnotationAppSharedBy: "default/app1,default/app",
+					},
+				},
+			}},
+			// These flags are checked in the test loop
+			expectIsShared: true,
+		},
+		"non-owner sharer works without last-applied-configuration": {
+			existing: &unstructured.Unstructured{Object: map[string]interface{}{
+				"kind": "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name": "test-cm",
+					"labels": map[string]interface{}{
+						oam.LabelAppName:      "app1",
+						oam.LabelAppNamespace: "default",
+					},
+					"annotations": map[string]interface{}{
+						oam.AnnotationAppSharedBy: "default/app1",
+					},
+				},
+				"data": map[string]interface{}{
+					"key": "value",
+				},
+			}},
+			desired: &unstructured.Unstructured{Object: map[string]interface{}{
+				"kind": "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name": "test-cm",
+				},
+			}},
+			// For non-owner sharers, desired only gets the shared-by annotation added
+			output: &unstructured.Unstructured{Object: map[string]interface{}{
+				"kind": "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name": "test-cm",
+					"annotations": map[string]interface{}{
+						oam.AnnotationAppSharedBy: "default/app1,default/app",
+					},
+				},
+			}},
+			expectIsShared: true,
+		},
 	}
 	for name, tc := range testCases {
 		t.Run(name, func(t *testing.T) {
 			r := require.New(t)
-			err := ao(&applyAction{}, tc.existing, tc.desired)
+			act := &applyAction{}
+			err := ao(act, tc.existing, tc.desired)
 			if tc.hasError {
 				r.Error(err)
 			} else {
 				r.NoError(err)
 				r.Equal(tc.output, tc.desired)
+
+				// Verify short-circuit flags for non-owner sharers
+				if tc.expectIsShared {
+					r.True(act.isShared, "isShared should be true for non-owner sharers")
+					r.False(act.updateAnnotation, "updateAnnotation should be false for non-owner sharers")
+				}
+
+				// Legacy check: When a resource is shared by another app, updateAnnotation should be false
+				if tc.existing != nil && tc.existing.GetAnnotations() != nil && tc.existing.GetAnnotations()[oam.AnnotationAppSharedBy] != "" {
+					existingController := GetControlledBy(tc.existing)
+					if existingController != "" && existingController != GetAppKey(app) {
+						r.False(act.updateAnnotation, "updateAnnotation should be false when sharing resource controlled by another app")
+					}
+				}
 			}
 		})
 	}

pkg/webhook/core.oam.dev/v1beta1/application/validation.go

@@ -25,6 +25,7 @@ import (
 	"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/util/validation/field"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/klog/v2"
@@ -114,7 +115,7 @@ func (h *ValidatingHandler) ValidateComponents(ctx context.Context, app *v1beta1
 
 // checkDefinitionPermission checks if user has permission to access a definition in either system namespace or app namespace
 func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req admission.Request, resource, definitionType, appNamespace string) (bool, error) {
-	// Check permission in system namespace (vela-system) first since most definitions are there
+	// Check permission in vela-system namespace first since most definitions are there
 	// This optimizes for the common case and reduces API calls
 	systemNsSar := &authv1.SubjectAccessReview{
 		Spec: authv1.SubjectAccessReviewSpec{
@@ -136,8 +137,19 @@ func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req a
 	}
 
 	if systemNsSar.Status.Allowed {
-		// User has permission in system namespace - no need to check app namespace
-		return true, nil
+		// User has permission in system namespace
+		// Verify the definition actually exists in vela-system
+		if exists, err := h.definitionExistsInNamespace(ctx, resource, definitionType, oam.SystemDefinitionNamespace); err != nil {
+			klog.V(4).Infof("Failed to check if %s %q exists in vela-system: %v", resource, definitionType, err)
+			// On error checking existence, deny access for safety
+			return false, nil
+		} else if !exists {
+			klog.V(4).Infof("%s %q does not exist in vela-system, checking app namespace", resource, definitionType)
+			// Definition doesn't exist in vela-system, fall through to check app namespace
+		} else {
+			// Definition exists in vela-system and user has permission
+			return true, nil
+		}
 	}
 
 	// If not in system namespace and app namespace is different, check app namespace
@@ -163,6 +175,17 @@ func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req a
 
 		if appNsSar.Status.Allowed {
 			// User has permission in app namespace
+			// But we need to verify the definition actually exists in the app namespace
+			// to prevent users with wildcard permissions from using definitions that only exist in vela-system
+			if exists, err := h.definitionExistsInNamespace(ctx, resource, definitionType, appNamespace); err != nil {
+				klog.V(4).Infof("Failed to check if %s %q exists in namespace %q: %v", resource, definitionType, appNamespace, err)
+				// On error checking existence, deny access for safety
+				return false, nil
+			} else if !exists {
+				klog.V(4).Infof("%s %q does not exist in namespace %q, denying access", resource, definitionType, appNamespace)
+				return false, nil
+			}
+			// Definition exists and user has permission
 			return true, nil
 		}
 	}
@@ -171,6 +194,38 @@ func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req a
 	return false, nil
 }
 
+// definitionExistsInNamespace checks if a definition actually exists in the specified namespace
+func (h *ValidatingHandler) definitionExistsInNamespace(ctx context.Context, resource, name, namespace string) (bool, error) {
+	// Determine the object type based on the resource
+	var obj client.Object
+	switch resource {
+	case "componentdefinitions":
+		obj = &v1beta1.ComponentDefinition{}
+	case "traitdefinitions":
+		obj = &v1beta1.TraitDefinition{}
+	case "policydefinitions":
+		obj = &v1beta1.PolicyDefinition{}
+	case "workflowstepdefinitions":
+		obj = &v1beta1.WorkflowStepDefinition{}
+	default:
+		return false, fmt.Errorf("unknown resource type: %s", resource)
+	}
+
+	// Try to get the definition from the namespace
+	key := client.ObjectKey{Name: name, Namespace: namespace}
+	if err := h.Client.Get(ctx, key, obj); err != nil {
+		if !errors.IsNotFound(err) {
+			// Handle other errors than not found
+			return false, err
+		}
+		// Definition not found
+		return false, nil
+	}
+
+	// Definition exists
+	return true, nil
+}
+
 // workflowStepLocation represents the location of a workflow step
 type workflowStepLocation struct {
 	StepIndex    int

pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go

@@ -26,6 +26,7 @@ import (
 	admissionv1 "k8s.io/api/admission/v1"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	authv1 "k8s.io/api/authorization/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
@@ -55,6 +56,7 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 		app                 *v1beta1.Application
 		userInfo            authenticationv1.UserInfo
 		allowedDefinitions  map[string]bool // resource/namespace/name -> allowed
+		existingDefinitions map[string]bool // namespace/name -> exists
 		expectedErrorCount  int
 		expectedErrorFields []string
 		expectedErrorMsgs   []string
@@ -101,6 +103,12 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"policydefinitions/vela-system/topology":      true,
 				"workflowstepdefinitions/vela-system/deploy":  true,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/webservice": true,
+				"vela-system/scaler":     true,
+				"vela-system/topology":   true,
+				"vela-system/deploy":     true,
+			},
 			expectedErrorCount: 0,
 		},
 		{
@@ -166,6 +174,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"traitdefinitions/vela-system/gateway":        false,
 				"traitdefinitions/test-ns/gateway":            false,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/webservice": true,
+			},
 			expectedErrorCount:  2,
 			expectedErrorFields: []string{"spec.components[0].traits[1].type", "spec.components[0].traits[0].type"},
 			expectedErrorMsgs:   []string{"cannot get TraitDefinition \"gateway\"", "cannot get TraitDefinition \"scaler\""},
@@ -206,6 +217,10 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"policydefinitions/vela-system/override": false,
 				"policydefinitions/test-ns/override":     false,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/topology": true,
+				"test-ns/topology":     true,
+			},
 			expectedErrorCount:  1,
 			expectedErrorFields: []string{"spec.policies[1].type"},
 			expectedErrorMsgs:   []string{"cannot get PolicyDefinition \"override\""},
@@ -251,6 +266,10 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"componentdefinitions/vela-system/webservice": true,
 				"componentdefinitions/test-ns/webservice":     true,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/webservice": true,
+				"test-ns/webservice":     true,
+			},
 			expectedErrorCount: 0,
 		},
 		{
@@ -325,6 +344,12 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"workflowstepdefinitions/vela-system/notification": false,
 				"workflowstepdefinitions/test-ns/notification":     false,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/webservice": true,
+				"vela-system/ingress":    true,
+				"vela-system/topology":   true,
+				"vela-system/deploy":     true,
+			},
 			expectedErrorCount: 4,
 			expectedErrorFields: []string{
 				"spec.components[0].traits[0].type",
@@ -418,7 +443,11 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"componentdefinitions/vela-system/custom-comp": false,
 				"componentdefinitions/test-ns/custom-comp":     true, // Allowed in app namespace
 			},
-			expectedErrorCount: 0, // Should pass as user has permission in app namespace
+			existingDefinitions: map[string]bool{
+				// Definition exists in app namespace
+				"test-ns/custom-comp": true,
+			},
+			expectedErrorCount: 0, // Should pass as user has permission in their namespace
 		},
 		{
 			name: "user has permission in system namespace but not app namespace",
@@ -444,6 +473,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"componentdefinitions/vela-system/webservice": true, // Allowed in system namespace
 				"componentdefinitions/test-ns/webservice":     false,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/webservice": true,
+			},
 			expectedErrorCount: 0, // Should pass as user has permission in system namespace
 		},
 		{
@@ -487,6 +519,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 				"workflowstepdefinitions/vela-system/notification": false,
 				"workflowstepdefinitions/test-ns/notification":     false,
 			},
+			existingDefinitions: map[string]bool{
+				"vela-system/deploy": true,
+			},
 			expectedErrorCount:  2,
 			expectedErrorFields: []string{"spec.workflow.steps[0].subSteps[0].type", "spec.workflow.steps[0].subSteps[1].type"},
 			expectedErrorMsgs:   []string{"cannot get WorkflowStepDefinition \"suspend\"", "cannot get WorkflowStepDefinition \"notification\""},
@@ -561,6 +596,108 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 			expectedErrorCount:  1,
 			expectedErrorFields: []string{"spec.components[0].type"},
 		},
+		{
+			name: "namespace admin cannot use vela-system definitions without explicit access",
+			app: &v1beta1.Application{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-app",
+					Namespace: "test",
+				},
+				Spec: v1beta1.ApplicationSpec{
+					Components: []common.ApplicationComponent{
+						{
+							Name: "hello",
+							Type: "hello-cm", // This definition exists only in vela-system
+						},
+					},
+				},
+			},
+			userInfo: authenticationv1.UserInfo{
+				Username: "system:serviceaccount:test:app-writer",
+				Groups:   []string{"system:serviceaccounts", "system:serviceaccounts:test"},
+			},
+			allowedDefinitions: map[string]bool{
+				// User has wildcard permissions in test namespace
+				"componentdefinitions/test/hello-cm": true,
+				// But no explicit access to vela-system
+				"componentdefinitions/vela-system/hello-cm": false,
+			},
+			existingDefinitions: map[string]bool{
+				// Definition exists in vela-system but not in test namespace
+				"vela-system/hello-cm": true,
+				"test/hello-cm":        false,
+			},
+			expectedErrorCount:  1,
+			expectedErrorFields: []string{"spec.components[0].type"},
+			expectedErrorMsgs:   []string{"cannot get ComponentDefinition \"hello-cm\""},
+		},
+		{
+			name: "user has vela-system permission but definition does not exist there",
+			app: &v1beta1.Application{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-app",
+					Namespace: "test",
+				},
+				Spec: v1beta1.ApplicationSpec{
+					Components: []common.ApplicationComponent{
+						{
+							Name: "phantom",
+							Type: "phantom-def", // User has permission but doesn't exist
+						},
+					},
+				},
+			},
+			userInfo: authenticationv1.UserInfo{
+				Username: "phantom-user",
+				Groups:   []string{"phantom-group"},
+			},
+			allowedDefinitions: map[string]bool{
+				// User has explicit permission to phantom-def in vela-system
+				"componentdefinitions/vela-system/phantom-def": true,
+				// And also in test namespace
+				"componentdefinitions/test/phantom-def": true,
+			},
+			existingDefinitions: map[string]bool{
+				// But definition doesn't exist in either namespace
+				"vela-system/phantom-def": false,
+				"test/phantom-def":        false,
+			},
+			expectedErrorCount:  1,
+			expectedErrorFields: []string{"spec.components[0].type"},
+			expectedErrorMsgs:   []string{"cannot get ComponentDefinition \"phantom-def\""},
+		},
+		{
+			name: "user has vela-system permission but definition only exists in app namespace",
+			app: &v1beta1.Application{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-app",
+					Namespace: "test",
+				},
+				Spec: v1beta1.ApplicationSpec{
+					Components: []common.ApplicationComponent{
+						{
+							Name: "local-only",
+							Type: "local-only-def", // Exists only in app namespace
+						},
+					},
+				},
+			},
+			userInfo: authenticationv1.UserInfo{
+				Username: "mixed-user",
+				Groups:   []string{"mixed-group"},
+			},
+			allowedDefinitions: map[string]bool{
+				// User has permission in both namespaces
+				"componentdefinitions/vela-system/local-only-def": true,
+				"componentdefinitions/test/local-only-def":        true,
+			},
+			existingDefinitions: map[string]bool{
+				// Definition only exists in app namespace
+				"vela-system/local-only-def": false,
+				"test/local-only-def":        true,
+			},
+			expectedErrorCount: 0, // Should succeed using test namespace version
+		},
 	}
 
 	for _, tc := range testCases {
@@ -571,8 +708,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
 			_ = authv1.AddToScheme(scheme)
 
 			fakeClient := &mockSARClient{
-				Client:             fake.NewClientBuilder().WithScheme(scheme).Build(),
-				allowedDefinitions: tc.allowedDefinitions,
+				Client:              fake.NewClientBuilder().WithScheme(scheme).Build(),
+				allowedDefinitions:  tc.allowedDefinitions,
+				existingDefinitions: tc.existingDefinitions,
 			}
 
 			handler := &ValidatingHandler{
@@ -696,6 +834,10 @@ func TestValidateDefinitionPermissions_AuthenticationDisabled(t *testing.T) {
 			"componentdefinitions/vela-system/webservice": true,
 			"componentdefinitions/test-ns/webservice":     true,
 		},
+		existingDefinitions: map[string]bool{
+			"vela-system/webservice": true,
+			"test-ns/webservice":     true,
+		},
 	}
 
 	handler := &ValidatingHandler{
@@ -865,7 +1007,8 @@ func TestGetWorkflowStepFieldPath(t *testing.T) {
 // mockSARClient is a mock client that simulates SubjectAccessReview responses
 type mockSARClient struct {
 	client.Client
-	allowedDefinitions map[string]bool
+	allowedDefinitions  map[string]bool
+	existingDefinitions map[string]bool // namespace/name -> exists
 }
 
 func (m *mockSARClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
@@ -893,3 +1036,20 @@ func (m *mockSARClient) Create(ctx context.Context, obj client.Object, opts ...c
 	}
 	return m.Client.Create(ctx, obj, opts...)
 }
+
+func (m *mockSARClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	// Handle definition existence checks
+	switch obj.(type) {
+	case *v1beta1.ComponentDefinition, *v1beta1.TraitDefinition, *v1beta1.PolicyDefinition, *v1beta1.WorkflowStepDefinition:
+		defKey := fmt.Sprintf("%s/%s", key.Namespace, key.Name)
+		if m.existingDefinitions != nil {
+			if exists, ok := m.existingDefinitions[defKey]; ok && exists {
+				// Definition exists - return success
+				return nil
+			}
+		}
+		// Definition not found
+		return errors.NewNotFound(v1beta1.SchemeGroupVersion.WithResource("componentdefinitions").GroupResource(), key.Name)
+	}
+	return m.Client.Get(ctx, key, obj, opts...)
+}