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

Signed-off-by: Brian Kane <briankane1@gmail.com>
2026-02-14 18:10:21 +00:00 · 2025-11-26 16:38:22 +05:30
parent 9b558e38cd
commit 552764d48f
2 changed files with 125 additions and 25 deletions
--- a/pkg/utils/apply/apply.go
+++ b/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
 	}
 }
--- a/pkg/utils/apply/apply_test.go
+++ b/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")
+					}
+				}
 			}
 		})
 	}