diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go index efbd3df3553..dde120d1502 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go +++ b/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 { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go index a635625c5b0..aaf86cec0ac 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go +++ b/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") + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater.go index 9bee82a854f..83f92b8ccec 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater.go +++ b/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 } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater_test.go index 3c1b39b8104..b54bbaed782 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/managedfieldsupdater_test.go +++ b/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") + } +}