Merge pull request #106195 from alexzielenski/ssa-dont-mutate-liveobj

Don't clear managedFields on live object

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go

@@ -56,10 +56,18 @@ type Manager interface {
 	// Update is used when the object has already been merged (non-apply
 	// use-case), and simply updates the managed fields in the output
 	// object.
+	//  * `liveObj` is not mutated by this function
+	//  * `newObj` may be mutated by this function
+	// Returns the new object with managedFields removed, and the object's new
+	// proposed managedFields separately.
 	Update(liveObj, newObj runtime.Object, managed Managed, manager string) (runtime.Object, Managed, error)
 
 	// Apply is used when server-side apply is called, as it merges the
 	// object and updates the managed fields.
+	//  * `liveObj` is not mutated by this function
+	//  * `newObj` may be mutated by this function
+	// Returns the new object with managedFields removed, and the object's new
+	// proposed managedFields separately.
 	Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error)
 }
 
@@ -173,7 +181,6 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (o
 		return newObj, nil
 	}
 
-	internal.RemoveObjectManagedFields(liveObj)
 	internal.RemoveObjectManagedFields(newObj)
 
 	if object, managed, err = f.fieldManager.Update(liveObj, newObj, managed, manager); err != nil {
@@ -242,8 +249,6 @@ func (f *FieldManager) Apply(liveObj, appliedObj runtime.Object, manager string,
 		return nil, fmt.Errorf("failed to decode managed fields: %v", err)
 	}
 
-	internal.RemoveObjectManagedFields(liveObj)
-
 	object, managed, err = f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force)
 	if err != nil {
 		if conflicts, ok := err.(merge.Conflicts); ok {

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go

@@ -780,14 +780,14 @@ func TestNoOpChanges(t *testing.T) {
 	}`), &obj.Object); err != nil {
 		t.Fatalf("error decoding YAML: %v", err)
 	}
-	if err := f.Apply(obj, "fieldmanager_test_apply", false); err != nil {
+	if err := f.Apply(obj.DeepCopyObject(), "fieldmanager_test_apply", false); err != nil {
 		t.Fatalf("failed to apply object: %v", err)
 	}
 	before := f.liveObj.DeepCopyObject()
 	// Wait to make sure the timestamp is different
 	time.Sleep(time.Second)
 	// Applying with a different fieldmanager will create an entry..
-	if err := f.Apply(obj, "fieldmanager_test_apply_other", false); err != nil {
+	if err := f.Apply(obj.DeepCopyObject(), "fieldmanager_test_apply_other", false); err != nil {
 		t.Fatalf("failed to update object: %v", err)
 	}
 	if reflect.DeepEqual(before, f.liveObj) {
@@ -796,7 +796,7 @@ func TestNoOpChanges(t *testing.T) {
 	before = f.liveObj.DeepCopyObject()
 	// Wait to make sure the timestamp is different
 	time.Sleep(time.Second)
-	if err := f.Update(obj, "fieldmanager_test_update"); err != nil {
+	if err := f.Update(obj.DeepCopyObject(), "fieldmanager_test_update"); err != nil {
 		t.Fatalf("failed to update object: %v", err)
 	}
 	if !reflect.DeepEqual(before, f.liveObj) {
@@ -805,7 +805,7 @@ func TestNoOpChanges(t *testing.T) {
 	before = f.liveObj.DeepCopyObject()
 	// Wait to make sure the timestamp is different
 	time.Sleep(time.Second)
-	if err := f.Apply(obj, "fieldmanager_test_apply", true); err != nil {
+	if err := f.Apply(obj.DeepCopyObject(), "fieldmanager_test_apply", true); err != nil {
 		t.Fatalf("failed to re-apply object: %v", err)
 	}
 	if !reflect.DeepEqual(before, f.liveObj) {
@@ -1291,3 +1291,258 @@ func TestUpdateViaSubresources(t *testing.T) {
 		t.Fatalf("Expected new managed fields to have one entry. Got:\n%#v", newManagedFields)
 	}
 }
+
+// Ensures that a no-op Apply does not mutate managed fields
+func TestApplyDoesNotChangeManagedFields(t *testing.T) {
+	originalManagedFields := []metav1.ManagedFieldsEntry{}
+	f := NewDefaultTestFieldManager(
+		schema.FromAPIVersionAndKind("apps/v1", "Deployment"))
+	newObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{},
+	}
+	appliedObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{},
+	}
+
+	// Convert YAML string inputs to unstructured instances
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+			"labels": {"app": "nginx"}
+		},
+		"spec": {
+			"selector": {
+				"matchLabels": {
+					"app": "nginx"
+				}
+			},
+			"template": {
+				"metadata": {
+					"labels": {
+						"app": "nginx"
+					}
+				},
+				"spec": {
+					"containers": [{
+						"name":  "nginx",
+						"image": "nginx:latest"
+					}]
+				}
+			}
+		}
+	}`), &newObj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+		},
+		"spec": {
+			"replicas": 101,
+		}
+	}`), &appliedObj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+
+	// Agent A applies initial configuration
+	if err := f.Apply(newObj.DeepCopyObject(), "fieldmanager_z", false); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	// Agent B applies additive configuration
+	if err := f.Apply(appliedObj, "fieldmanager_b", false); err != nil {
+		t.Fatalf("failed to apply object %v", err)
+	}
+
+	// Next, agent A applies the initial configuration again, but we expect
+	// a no-op to managed fields.
+	//
+	// The following update is expected not to change the liveObj, save off
+	//	the fields
+	for _, field := range f.ManagedFields() {
+		originalManagedFields = append(originalManagedFields, *field.DeepCopy())
+	}
+
+	// Make sure timestamp change would be caught
+	time.Sleep(2 * time.Second)
+
+	if err := f.Apply(newObj, "fieldmanager_z", false); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	// ensure that the live object is unchanged
+	if !reflect.DeepEqual(originalManagedFields, f.ManagedFields()) {
+		originalYAML, _ := yaml.Marshal(originalManagedFields)
+		current, _ := yaml.Marshal(f.ManagedFields())
+
+		// should have been a no-op w.r.t. managed fields
+		t.Fatalf("managed fields changed: ORIGINAL\n%v\nCURRENT\n%v",
+			string(originalYAML), string(current))
+	}
+}
+
+// Ensures that a no-op Update does not mutate managed fields
+func TestUpdateDoesNotChangeManagedFields(t *testing.T) {
+	originalManagedFields := []metav1.ManagedFieldsEntry{}
+	f := NewDefaultTestFieldManager(
+		schema.FromAPIVersionAndKind("apps/v1", "Deployment"))
+	newObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{},
+	}
+
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+			"labels": {"app": "nginx"}
+		},
+		"spec": {
+			"selector": {
+				"matchLabels": {
+					"app": "nginx"
+				}
+			},
+			"template": {
+				"metadata": {
+					"labels": {
+						"app": "nginx"
+					}
+				},
+				"spec": {
+					"containers": [{
+						"name":  "nginx",
+						"image": "nginx:latest"
+					}]
+				}
+			}
+		}
+	}`), &newObj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+
+	// Agent A updates with initial configuration
+	if err := f.Update(newObj.DeepCopyObject(), "fieldmanager_z"); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	for _, field := range f.ManagedFields() {
+		originalManagedFields = append(originalManagedFields, *field.DeepCopy())
+	}
+
+	// Make sure timestamp change would be caught
+	time.Sleep(2 * time.Second)
+
+	// If the same exact configuration is updated once again, the
+	// managed fields are not expected to change
+	//
+	// However, a change in field ownership WOULD be a semantic change which
+	// should cause managed fields to change.
+	if err := f.Update(newObj, "fieldmanager_z"); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	// ensure that the live object is unchanged
+	if !reflect.DeepEqual(originalManagedFields, f.ManagedFields()) {
+		originalYAML, _ := yaml.Marshal(originalManagedFields)
+		current, _ := yaml.Marshal(f.ManagedFields())
+
+		// should have been a no-op w.r.t. managed fields
+		t.Fatalf("managed fields changed: ORIGINAL\n%v\nCURRENT\n%v",
+			string(originalYAML), string(current))
+	}
+}
+
+// This test makes sure that the liveObject during a patch does not mutate
+// its managed fields.
+func TestLiveObjectManagedFieldsNotRemoved(t *testing.T) {
+	f := NewDefaultTestFieldManager(
+		schema.FromAPIVersionAndKind("apps/v1", "Deployment"))
+	newObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{},
+	}
+	appliedObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{},
+	}
+	// Convert YAML string inputs to unstructured instances
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+			"labels": {"app": "nginx"}
+		},
+		"spec": {
+			"selector": {
+				"matchLabels": {
+					"app": "nginx"
+				}
+			},
+			"template": {
+				"metadata": {
+					"labels": {
+						"app": "nginx"
+					}
+				},
+				"spec": {
+					"containers": [{
+						"name":  "nginx",
+						"image": "nginx:latest"
+					}]
+				}
+			}
+		}
+	}`), &newObj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "apps/v1",
+		"kind": "Deployment",
+		"metadata": {
+			"name": "deployment",
+		},
+		"spec": {
+			"replicas": 101,
+		}
+	}`), &appliedObj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+
+	// Agent A applies initial configuration
+	if err := f.Apply(newObj.DeepCopyObject(), "fieldmanager_z", false); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	originalLiveObj := f.liveObj
+
+	accessor, err := meta.Accessor(originalLiveObj)
+	if err != nil {
+		panic(fmt.Errorf("couldn't get accessor: %v", err))
+	}
+
+	// Managed fields should not be stripped
+	if len(accessor.GetManagedFields()) == 0 {
+		t.Fatalf("empty managed fields of object which expected nonzero fields")
+	}
+
+	// Agent A applies the exact same configuration
+	if err := f.Apply(appliedObj.DeepCopyObject(), "fieldmanager_z", false); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	accessor, err = meta.Accessor(originalLiveObj)
+	if err != nil {
+		panic(fmt.Errorf("couldn't get accessor: %v", err))
+	}
+
+	// Managed fields should not be stripped
+	if len(accessor.GetManagedFields()) == 0 {
+		t.Fatalf("empty managed fields of object which expected nonzero fields")
+	}
+}

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater.go

@@ -21,6 +21,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal"
 	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
 )
 
@@ -80,7 +81,8 @@ func (f *managedFieldsUpdater) Apply(liveObj, appliedObj runtime.Object, managed
 		managed.Times()[fieldManager] = &metav1.Time{Time: time.Now().UTC()}
 	}
 	if object == nil {
-		object = liveObj
+		object = liveObj.DeepCopyObject()
+		internal.RemoveObjectManagedFields(object)
 	}
 	return object, managed, nil
 }

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater_test.go

@@ -21,8 +21,11 @@ import (
 	"testing"
 	"time"
 
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal"
 	"sigs.k8s.io/yaml"
 )
 
@@ -64,3 +67,79 @@ func TestNoManagedFieldsUpdateDoesntUpdateTime(t *testing.T) {
 		t.Errorf("ManagedFields changed:\nBefore:\n%v\nAfter:\n%v", managed, f.ManagedFields())
 	}
 }
+
+type NoopManager struct{}
+
+func (NoopManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) {
+	return nil, managed, nil
+}
+
+func (NoopManager) Update(liveObj, newObj runtime.Object, managed Managed, manager string) (runtime.Object, Managed, error) {
+	return nil, nil, nil
+}
+
+// Ensures that if ManagedFieldsUpdater gets a nil value from its nested manager
+// chain (meaning the operation was a no-op), then the ManagedFieldsUpdater
+// itself will return a copy of the input live object, with its managed fields
+// removed
+func TestNilNewObjectReplacedWithDeepCopyExcludingManagedFields(t *testing.T) {
+	// Initialize our "live object" with some managed fields
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"name": "pod",
+			"labels": {"app": "nginx"},
+			"managedFields": [
+				{
+					"apiVersion": "v1",
+					"fieldsType": "FieldsV1",
+					"fieldsV1": {
+						"f:metadata": {
+							"f:labels": {
+								"f:app": {}
+							}
+						}
+					},
+					"manager": "fieldmanager_test",
+					"operation": "Apply",
+					"time": "2021-11-11T18:41:17Z"
+				}
+			]
+		}
+	}`), &obj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+
+	accessor, err := meta.Accessor(obj)
+	if err != nil {
+		t.Fatalf("couldn't get accessor: %v", err)
+	}
+
+	// Decode the managed fields in the live object, since it isn't allowed in the patch.
+	managed, err := DecodeManagedFields(accessor.GetManagedFields())
+	if err != nil {
+		t.Fatalf("failed to decode managed fields: %v", err)
+	}
+
+	updater := NewManagedFieldsUpdater(NoopManager{})
+
+	newObject, _, err := updater.Apply(obj, obj.DeepCopyObject(), managed, "some_manager", false)
+	if err != nil {
+		t.Fatalf("failed to apply configuration %v", err)
+	}
+
+	if newObject == obj {
+		t.Fatalf("returned newObject must not be the same instance as the passed in liveObj")
+	}
+
+	// Rip off managed fields of live, and check that it is deeply
+	// equal to newObject
+	liveWithoutManaged := obj.DeepCopyObject()
+	internal.RemoveObjectManagedFields(liveWithoutManaged)
+
+	if !reflect.DeepEqual(liveWithoutManaged, newObject) {
+		t.Fatalf("returned newObject must be deeply equal to the input live object, without managed fields")
+	}
+}