Merge pull request kubernetes#25178 from openshift-cherrypick-robot/c…

…herry-pick-25153-to-release-4.5 [release-4.5] Bug 1849340: 4.6: UPSTREAM: 91748: FieldManager: Reset if we receive nil or a list with one Origin-commit: 002a51f51065024cddd18d373bf7f80aa30a36de
openshift · Aug 6, 2020 · 681fb1a · 681fb1a
2 parents da5ec26 + cef05d4
commit 681fb1a
Show file tree

Hide file tree

Showing 11 changed files with 441 additions and 53 deletions.
diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go
@@ -178,7 +178,7 @@ func ValidateManagedFields(fieldsList []metav1.ManagedFieldsEntry, fldPath *fiel
 		default:
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("operation"), fields.Operation, "must be `Apply` or `Update`"))
 		}
-		if fields.FieldsType != "FieldsV1" {
+		if len(fields.FieldsType) > 0 && fields.FieldsType != "FieldsV1" {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("fieldsType"), fields.FieldsType, "must be `FieldsV1`"))
 		}
 	}

diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation_test.go
@@ -242,12 +242,8 @@ func TestValidateFieldManagerInvalid(t *testing.T) {
 	}
 }
 
-func TestValidateMangedFieldsInvalid(t *testing.T) {
+func TestValidateManagedFieldsInvalid(t *testing.T) {
 	tests := []metav1.ManagedFieldsEntry{
-		{
-			Operation: metav1.ManagedFieldsOperationUpdate,
-			// FieldsType is missing
-		},
 		{
 			Operation:  metav1.ManagedFieldsOperationUpdate,
 			FieldsType: "RandomVersion",
@@ -274,6 +270,10 @@ func TestValidateMangedFieldsInvalid(t *testing.T) {
 
 func TestValidateMangedFieldsValid(t *testing.T) {
 	tests := []metav1.ManagedFieldsEntry{
+		{
+			Operation: metav1.ManagedFieldsOperationUpdate,
+			// FieldsType is missing
+		},
 		{
 			Operation:  metav1.ManagedFieldsOperationUpdate,
 			FieldsType: "FieldsV1",

diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go
@@ -156,10 +156,7 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int
 				if err != nil {
 					return nil, fmt.Errorf("failed to create new object (Create for %v): %v", scope.Kind, err)
 				}
-				obj, err = scope.FieldManager.Update(liveObj, obj, managerOrUserAgent(options.FieldManager, req.UserAgent()))
-				if err != nil {
-					return nil, fmt.Errorf("failed to update object (Create for %v) managed fields: %v", scope.Kind, err)
-				}
+				obj = scope.FieldManager.UpdateNoErrors(liveObj, obj, managerOrUserAgent(options.FieldManager, req.UserAgent()))
 			}
 			if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && mutatingAdmission.Handles(admission.Create) {
 				if err := mutatingAdmission.Admit(ctx, admissionAttributes, scope); err != nil {

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
@@ -18,12 +18,15 @@ package fieldmanager
 
 import (
 	"fmt"
+	"reflect"
+	"time"
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal"
+	"k8s.io/klog"
 	openapiproto "k8s.io/kube-openapi/pkg/util/proto"
 	"sigs.k8s.io/structured-merge-diff/v3/fieldpath"
 )
@@ -37,6 +40,8 @@ const DefaultMaxUpdateManagers int = 10
 // starts being tracked from the object's creation, instead of from the first time the object is applied to.
 const DefaultTrackOnCreateProbability float32 = 1
 
+var atMostEverySecond = internal.NewAtMostEvery(time.Second)
+
 // Managed groups a fieldpath.ManagedFields together with the timestamps associated with each operation.
 type Managed interface {
 	// Fields gets the fieldpath.ManagedFields.
@@ -107,20 +112,26 @@ func newDefaultFieldManager(f Manager, objectCreater runtime.ObjectCreater, kind
 func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (object runtime.Object, err error) {
 	// If the object doesn't have metadata, we should just return without trying to
 	// set the managedFields at all, so creates/updates/patches will work normally.
-	if _, err = meta.Accessor(newObj); err != nil {
+	newAccessor, err := meta.Accessor(newObj)
+	if err != nil {
 		return newObj, nil
 	}
 
 	// First try to decode the managed fields provided in the update,
 	// This is necessary to allow directly updating managed fields.
 	var managed Managed
-	if managed, err = internal.DecodeObjectManagedFields(newObj); err != nil || len(managed.Fields()) == 0 {
+	if isResetManagedFields(newAccessor.GetManagedFields()) {
+		managed = internal.NewEmptyManaged()
+	} else if managed, err = internal.DecodeObjectManagedFields(newAccessor.GetManagedFields()); err != nil || len(managed.Fields()) == 0 {
+		liveAccessor, err := meta.Accessor(liveObj)
+		if err != nil {
+			return newObj, nil
+		}
 		// If the managed field is empty or we failed to decode it,
 		// let's try the live object. This is to prevent clients who
 		// don't understand managedFields from deleting it accidentally.
-		managed, err = internal.DecodeObjectManagedFields(liveObj)
-		if err != nil {
-			return nil, fmt.Errorf("failed to decode managed fields: %v", err)
+		if managed, err = internal.DecodeObjectManagedFields(liveAccessor.GetManagedFields()); err != nil {
+			managed = internal.NewEmptyManaged()
 		}
 	}
 
@@ -138,17 +149,52 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (o
 	return object, nil
 }
 
+// UpdateNoErrors is the same as Update, but it will not return
+// errors. If an error happens, the object is returned with
+// managedFields cleared.
+func (f *FieldManager) UpdateNoErrors(liveObj, newObj runtime.Object, manager string) runtime.Object {
+	obj, err := f.Update(liveObj, newObj, manager)
+	if err != nil {
+		atMostEverySecond.Do(func() {
+			klog.Errorf("[SHOULD NOT HAPPEN] failed to update managedFields for %v: %v",
+				newObj.GetObjectKind().GroupVersionKind(),
+				err)
+		})
+		// Explicitly remove managedFields on failure, so that
+		// we can't have garbage in it.
+		internal.RemoveObjectManagedFields(newObj)
+		return newObj
+	}
+	return obj
+}
+
+// Returns true if the managedFields indicate that the user is trying to
+// reset the managedFields, i.e. if the list is non-nil but empty, or if
+// the list has one empty item.
+func isResetManagedFields(managedFields []metav1.ManagedFieldsEntry) bool {
+	if len(managedFields) == 0 {
+		return managedFields != nil
+	}
+
+	if len(managedFields) == 1 {
+		return reflect.DeepEqual(managedFields[0], metav1.ManagedFieldsEntry{})
+	}
+
+	return false
+}
+
 // Apply is used when server-side apply is called, as it merges the
 // object and updates the managed fields.
 func (f *FieldManager) Apply(liveObj, appliedObj runtime.Object, manager string, force bool) (object runtime.Object, err error) {
 	// If the object doesn't have metadata, apply isn't allowed.
-	if _, err = meta.Accessor(liveObj); err != nil {
+	accessor, err := meta.Accessor(liveObj)
+	if err != nil {
 		return nil, fmt.Errorf("couldn't get accessor: %v", err)
 	}
 
 	// Decode the managed fields in the live object, since it isn't allowed in the patch.
-	var managed Managed
-	if managed, err = internal.DecodeObjectManagedFields(liveObj); err != nil {
+	managed, err := internal.DecodeObjectManagedFields(accessor.GetManagedFields())
+	if err != nil {
 		return nil, fmt.Errorf("failed to decode managed fields: %v", err)
 	}
 

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
@@ -783,3 +783,86 @@ func TestNoOpChanges(t *testing.T) {
 		t.Fatalf("No-op apply has changed the object:\n%v\n---\n%v", before, f.liveObj)
 	}
 }
+
+// Tests that one can reset the managedFields by sending either an empty
+// list
+func TestResetManagedFieldsEmptyList(t *testing.T) {
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod"))
+
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"labels": {
+				"a": "b"
+			},
+		}
+	}`), &obj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+	if err := f.Apply(obj, "fieldmanager_test_apply", false); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"managedFields": [],
+			"labels": {
+				"a": "b"
+			},
+		}
+	}`), &obj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+	if err := f.Update(obj, "update_manager"); err != nil {
+		t.Fatalf("failed to update with empty manager: %v", err)
+	}
+
+	if len(f.ManagedFields()) != 0 {
+		t.Fatalf("failed to reset managedFields: %v", f.ManagedFields())
+	}
+}
+
+// Tests that one can reset the managedFields by sending either a list with one empty item.
+func TestResetManagedFieldsEmptyItem(t *testing.T) {
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod"))
+
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"labels": {
+				"a": "b"
+			},
+		}
+	}`), &obj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+	if err := f.Apply(obj, "fieldmanager_test_apply", false); err != nil {
+		t.Fatalf("failed to apply object: %v", err)
+	}
+
+	if err := yaml.Unmarshal([]byte(`{
+		"apiVersion": "v1",
+		"kind": "Pod",
+		"metadata": {
+			"managedFields": [{}],
+			"labels": {
+				"a": "b"
+			},
+		}
+	}`), &obj.Object); err != nil {
+		t.Fatalf("error decoding YAML: %v", err)
+	}
+	if err := f.Update(obj, "update_manager"); err != nil {
+		t.Fatalf("failed to update with empty manager: %v", err)
+	}
+
+	if len(f.ManagedFields()) != 0 {
+		t.Fatalf("failed to reset managedFields: %v", f.ManagedFields())
+	}
+}
diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go
@@ -53,6 +53,11 @@ func (m *managedStruct) Times() map[string]*metav1.Time {
 	return m.times
 }
 
+// NewEmptyManaged creates an empty ManagedInterface.
+func NewEmptyManaged() ManagedInterface {
+	return NewManaged(fieldpath.ManagedFields{}, map[string]*metav1.Time{})
+}
+
 // NewManaged creates a ManagedInterface from a fieldpath.ManagedFields and the timestamps associated with each operation.
 func NewManaged(f fieldpath.ManagedFields, t map[string]*metav1.Time) ManagedInterface {
 	return &managedStruct{
@@ -73,16 +78,8 @@ func RemoveObjectManagedFields(obj runtime.Object) {
 }
 
 // DecodeObjectManagedFields extracts and converts the objects ManagedFields into a fieldpath.ManagedFields.
-func DecodeObjectManagedFields(from runtime.Object) (ManagedInterface, error) {
-	if from == nil {
-		return &managedStruct{}, nil
-	}
-	accessor, err := meta.Accessor(from)
-	if err != nil {
-		panic(fmt.Sprintf("couldn't get accessor: %v", err))
-	}
-
-	managed, err := decodeManagedFields(accessor.GetManagedFields())
+func DecodeObjectManagedFields(from []metav1.ManagedFieldsEntry) (ManagedInterface, error) {
+	managed, err := decodeManagedFields(from)
 	if err != nil {
 		return nil, fmt.Errorf("failed to convert managed fields from API: %v", err)
 	}
@@ -110,7 +107,16 @@ func EncodeObjectManagedFields(obj runtime.Object, managed ManagedInterface) err
 func decodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (managed managedStruct, err error) {
 	managed.fields = make(fieldpath.ManagedFields, len(encodedManagedFields))
 	managed.times = make(map[string]*metav1.Time, len(encodedManagedFields))
-	for _, encodedVersionedSet := range encodedManagedFields {
+
+	for i, encodedVersionedSet := range encodedManagedFields {
+		switch encodedVersionedSet.FieldsType {
+		case "FieldsV1":
+			// Valid case.
+		case "":
+			return managedStruct{}, fmt.Errorf("missing fieldsType in managed fields entry %d", i)
+		default:
+			return managedStruct{}, fmt.Errorf("invalid fieldsType %q in managed fields entry %d", encodedVersionedSet.FieldsType, i)
+		}
 		manager, err := BuildManagerIdentifier(&encodedVersionedSet)
 		if err != nil {
 			return managedStruct{}, fmt.Errorf("error decoding manager from %v: %v", encodedVersionedSet, err)

diff --git a/...g/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go b/...g/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go
@@ -27,6 +27,51 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
+// TestHasFieldsType makes sure that we fail if we don't have a
+// FieldsType set properly.
+func TestHasFieldsType(t *testing.T) {
+	var unmarshaled []metav1.ManagedFieldsEntry
+	if err := yaml.Unmarshal([]byte(`- apiVersion: v1
+  fieldsType: FieldsV1
+  fieldsV1:
+    f:field: {}
+  manager: foo
+  operation: Apply
+`), &unmarshaled); err != nil {
+		t.Fatalf("did not expect yaml unmarshalling error but got: %v", err)
+	}
+	if _, err := decodeManagedFields(unmarshaled); err != nil {
+		t.Fatalf("did not expect decoding error but got: %v", err)
+	}
+
+	// Invalid fieldsType V2.
+	if err := yaml.Unmarshal([]byte(`- apiVersion: v1
+  fieldsType: FieldsV2
+  fieldsV1:
+    f:field: {}
+  manager: foo
+  operation: Apply
+`), &unmarshaled); err != nil {
+		t.Fatalf("did not expect yaml unmarshalling error but got: %v", err)
+	}
+	if _, err := decodeManagedFields(unmarshaled); err == nil {
+		t.Fatal("Expect decoding error but got none")
+	}
+
+	// Missing fieldsType.
+	if err := yaml.Unmarshal([]byte(`- apiVersion: v1
+  fieldsV1:
+    f:field: {}
+  manager: foo
+  operation: Apply
+`), &unmarshaled); err != nil {
+		t.Fatalf("did not expect yaml unmarshalling error but got: %v", err)
+	}
+	if _, err := decodeManagedFields(unmarshaled); err == nil {
+		t.Fatal("Expect decoding error but got none")
+	}
+}
+
 // TestRoundTripManagedFields will roundtrip ManagedFields from the wire format
 // (api format) to the format used by sigs.k8s.io/structured-merge-diff and back
 func TestRoundTripManagedFields(t *testing.T) {

diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go
@@ -18,14 +18,12 @@ package fieldmanager
 
 import (
 	"fmt"
-	"time"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal"
-	"k8s.io/klog"
 	openapiproto "k8s.io/kube-openapi/pkg/util/proto"
 	"sigs.k8s.io/structured-merge-diff/v3/fieldpath"
 	"sigs.k8s.io/structured-merge-diff/v3/merge"
@@ -41,7 +39,6 @@ type structuredMergeManager struct {
 }
 
 var _ Manager = &structuredMergeManager{}
-var atMostEverySecond = internal.NewAtMostEvery(time.Second)
 
 // NewStructuredMergeManager creates a new Manager that merges apply requests
 // and update managed fields for other types of requests.
@@ -98,19 +95,11 @@ func (f *structuredMergeManager) Update(liveObj, newObj runtime.Object, managed
 	}
 	newObjTyped, err := f.typeConverter.ObjectToTyped(newObjVersioned)
 	if err != nil {
-		// Return newObj and just by-pass fields update. This really shouldn't happen.
-		atMostEverySecond.Do(func() {
-			klog.Errorf("[SHOULD NOT HAPPEN] failed to create typed new object of type %v: %v", newObjVersioned.GetObjectKind().GroupVersionKind(), err)
-		})
-		return newObj, managed, nil
+		return nil, nil, fmt.Errorf("failed to convert new object (%v) to smd typed: %v", newObjVersioned.GetObjectKind().GroupVersionKind(), err)
 	}
 	liveObjTyped, err := f.typeConverter.ObjectToTyped(liveObjVersioned)
 	if err != nil {
-		// Return newObj and just by-pass fields update. This really shouldn't happen.
-		atMostEverySecond.Do(func() {
-			klog.Errorf("[SHOULD NOT HAPPEN] failed to create typed live object of type %v: %v", liveObjVersioned.GetObjectKind().GroupVersionKind(), err)
-		})
-		return newObj, managed, nil
+		return nil, nil, fmt.Errorf("failed to convert live object (%v) to smd typed: %v", liveObjVersioned.GetObjectKind().GroupVersionKind(), err)
 	}
 	apiVersion := fieldpath.APIVersion(f.groupVersion.String())