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

(cherry picked from commit 552764d48f)

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

.github/workflows/e2e-test.yml

@@ -18,7 +18,7 @@ permissions:
 
 env:
   # Common versions
-  GO_VERSION: '1.19'
+  GO_VERSION: '1.22'
 
 jobs:
 
@@ -78,7 +78,8 @@ jobs:
       - name: Get Ginkgo
         run: |
           go install github.com/onsi/ginkgo/v2/ginkgo@v2.10.0
-          go get github.com/onsi/gomega/...
+          go get github.com/onsi/gomega@v1.34.2
+          go mod tidy
 
       - name: Load image
         run: |

.github/workflows/license.yml

@@ -9,7 +9,6 @@ on:
     branches:
       - master
       - release-*
-      -
 permissions:
   contents: read
 

Dockerfile.e2e

@@ -1,6 +1,6 @@
 ARG BASE_IMAGE
 # Build the manager binary
-FROM --platform=${BUILDPLATFORM:-linux/amd64} golang:1.19-alpine@sha256:2381c1e5f8350a901597d633b2e517775eeac7a6682be39225a93b22cfd0f8bb as builder
+FROM --platform=${BUILDPLATFORM:-linux/amd64} golang:1.22.7-alpine@sha256:48eab5e3505d8c8b42a06fe5f1cf4c346c167cc6a89e772f31cb9e5c301dcf60 as builder
 
 WORKDIR /workspace
 # Copy the Go Modules manifests

charts/vela-core/README.md

@@ -186,6 +186,21 @@ if [ $fluxcd ]; then
 fi
 ```
 
+Make sure all existing KubeVela resources deleted before uninstallation:
+```shell
+kubectl delete applicationrevisions.core.oam.dev --all
+kubectl delete applications.core.oam.dev --all
+kubectl delete componentdefinitions.core.oam.dev --all
+kubectl delete definitionrevisions.core.oam.dev --all
+kubectl delete policies.core.oam.dev --all
+kubectl delete policydefinitions.core.oam.dev --all
+kubectl delete resourcetrackers.core.oam.dev --all
+kubectl delete traitdefinitions.core.oam.dev --all
+kubectl delete workflows.core.oam.dev --all
+kubectl delete workflowstepdefinitions.core.oam.dev --all
+kubectl delete workloaddefinitions.core.oam.dev --all
+```
+
 To uninstall the KubeVela helm release:
 
 ```shell

charts/vela-core/templates/defwithtemplate/startup-probe.yaml

@@ -65,7 +65,7 @@ spec:
         	// +usage=Instructions for assessing container startup status by probing a TCP socket. Either this attribute or the exec attribute or the tcpSocket attribute or the httpGet attribute MUST be specified. This attribute is mutually exclusive with the exec attribute and the httpGet attribute and the gRPC attribute.
         	tcpSocket?: {
         		// +usage=Number or name of the port to access on the container.
-        		port: string
+        		port: int
         		// +usage=Host name to connect to, defaults to the pod IP.
         		host?: string
         	}
@@ -144,7 +144,7 @@ spec:
         					grpc: parameter.grpc
         				}
         				if parameter.tcpSocket != _|_ {
-        					tcpSocket: parameter.grtcpSocketpc
+        					tcpSocket: parameter.tcpSocket
         				}
         			}}
         		}]

charts/vela-core/templates/defwithtemplate/vela-cli.yaml

@@ -46,11 +46,11 @@ spec:
         				}
         			}
         		}
-        		if parameter.storage != _|_ && parameter.storage.hostPath != _|_ for v in parameter.storage.hostPath {
-        			{
-        				name: "hostpath-" + v.name
-        				path: v.path
-        			}
+        	},
+        	if parameter.storage != _|_ && parameter.storage.hostPath != _|_ for v in parameter.storage.hostPath {
+        		{
+        			name: "hostpath-" + v.name
+        			path: v.path
         		}
         	},
         ]

go.mod

@@ -36,8 +36,8 @@ require (
 	github.com/hashicorp/hcl/v2 v2.18.0
 	github.com/hinshun/vt10x v0.0.0-20180616224451-1954e6464174
 	github.com/imdario/mergo v0.3.16
-	github.com/kubevela/pkg v1.9.0
-	github.com/kubevela/workflow v0.6.0
+	github.com/kubevela/pkg v1.9.2
+	github.com/kubevela/workflow v0.6.1
 	github.com/kyokomi/emoji v2.2.4+incompatible
 	github.com/magiconair/properties v1.8.7
 	github.com/mattn/go-runewidth v0.0.15

go.sum

@@ -601,10 +601,10 @@ github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
-github.com/kubevela/pkg v1.9.0 h1:k3DE8yC86K4s+hY04dUyxDSd6JKeluvmgQYvtsouDDg=
-github.com/kubevela/pkg v1.9.0/go.mod h1:u/MGuFXVSECxvIWdTKS4AQs1H+USfAMQgi30BUrOb04=
-github.com/kubevela/workflow v0.6.0 h1:fYXviOYD5zqHs3J61tNbM4HZ85EcZlPm7Fyz8Q5o9Fk=
-github.com/kubevela/workflow v0.6.0/go.mod h1:sjLcYqKHKeCQ+w77gijoNILwIShJKnCU+e3q7ETtZGI=
+github.com/kubevela/pkg v1.9.2 h1:K6pGoJikf6l8vlfehewmb36hyToX0KpUaQP4NVET/S8=
+github.com/kubevela/pkg v1.9.2/go.mod h1:u/MGuFXVSECxvIWdTKS4AQs1H+USfAMQgi30BUrOb04=
+github.com/kubevela/workflow v0.6.1 h1:x6ro6dTzi5Rrh0pnnqRbNMlhvhP5Rzsls44gP1C3qzw=
+github.com/kubevela/workflow v0.6.1/go.mod h1:sjLcYqKHKeCQ+w77gijoNILwIShJKnCU+e3q7ETtZGI=
 github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348/go.mod h1:B69LEHPfb2qLo0BaaOLcbitczOKLWTsrBG9LczfCD4k=
 github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=
 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=

pkg/utils/apply/apply.go

@@ -18,7 +18,6 @@ package apply
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"reflect"
 
@@ -206,6 +205,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) {
@@ -466,39 +479,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")
+					}
+				}
 			}
 		})
 	}

vela-templates/definitions/internal/trait/startup-probe.cue

@@ -56,7 +56,7 @@ template: {
 		// +usage=Instructions for assessing container startup status by probing a TCP socket. Either this attribute or the exec attribute or the tcpSocket attribute or the httpGet attribute MUST be specified. This attribute is mutually exclusive with the exec attribute and the httpGet attribute and the gRPC attribute.
 		tcpSocket?: {
 			// +usage=Number or name of the port to access on the container.
-			port: string
+			port: int
 			// +usage=Host name to connect to, defaults to the pod IP.
 			host?: string
 		}
@@ -135,7 +135,7 @@ template: {
 						grpc: parameter.grpc
 					}
 					if parameter.tcpSocket != _|_ {
-						tcpSocket: parameter.grtcpSocketpc
+						tcpSocket: parameter.tcpSocket
 					}
 				}}
 			}]

vela-templates/definitions/internal/workflowstep/vela-cli.cue

@@ -41,11 +41,11 @@ template: {
 					}
 				}
 			}
-			if parameter.storage != _|_ && parameter.storage.hostPath != _|_ for v in parameter.storage.hostPath {
-				{
-					name: "hostpath-" + v.name
-					path: v.path
-				}
+		},
+		if parameter.storage != _|_ && parameter.storage.hostPath != _|_ for v in parameter.storage.hostPath {
+			{
+				name: "hostpath-" + v.name
+				path: v.path
 			}
 		},
 	]