diff --git a/pkg/utils/apply/apply.go b/pkg/utils/apply/apply.go index 9a7f6a80a..3852f4f2f 100644 --- a/pkg/utils/apply/apply.go +++ b/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 } } diff --git a/pkg/utils/apply/apply_test.go b/pkg/utils/apply/apply_test.go index 2c4130d06..a83e9c9f9 100644 --- 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") + } + } } }) }