Merge pull request #91690 from apelisse/ignore-failures

fieldManager: Ignore and log all errors when updating managedFields

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()))
+				obj = scope.FieldManager.UpdateNoErrors(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)
-				}
 			}
 			if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && mutatingAdmission.Handles(admission.Create) {
 				if err := mutatingAdmission.Admit(ctx, admissionAttributes, scope); err != nil {

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

@@ -18,12 +18,14 @@ package fieldmanager
 
 import (
 	"fmt"
+	"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/v2"
 	openapiproto "k8s.io/kube-openapi/pkg/util/proto"
 	"sigs.k8s.io/structured-merge-diff/v3/fieldpath"
 )
@@ -37,6 +39,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.
@@ -120,7 +124,9 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (o
 		// 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 we also can't decode the liveObject, then
+			// just restart managedFields from scratch.
+			managed = internal.NewEmptyManaged()
 		}
 	}
 
@@ -138,6 +144,25 @@ 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
+}
+
 // 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) {

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{

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/v2"
 	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.
+		return nil, nil, fmt.Errorf("failed to convert new object (%v) to smd typed: %v", newObjVersioned.GetObjectKind().GroupVersionKind(), err)
-		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
 	}
 	liveObjTyped, err := f.typeConverter.ObjectToTyped(liveObjVersioned)
 	if err != nil {
-		// Return newObj and just by-pass fields update. This really shouldn't happen.
+		return nil, nil, fmt.Errorf("failed to convert live object (%v) to smd typed: %v", liveObjVersioned.GetObjectKind().GroupVersionKind(), err)
-		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
 	}
 	apiVersion := fieldpath.APIVersion(f.groupVersion.String())
 

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

@@ -323,9 +323,7 @@ func (p *jsonPatcher) applyPatchToCurrentObject(currentObject runtime.Object) (r
 	}
 
 	if p.fieldManager != nil {
-		if objToUpdate, err = p.fieldManager.Update(currentObject, objToUpdate, managerOrUserAgent(p.options.FieldManager, p.userAgent)); err != nil {
+		objToUpdate = p.fieldManager.UpdateNoErrors(currentObject, objToUpdate, managerOrUserAgent(p.options.FieldManager, p.userAgent))
-			return nil, fmt.Errorf("failed to update object (json PATCH for %v) managed fields: %v", p.kind, err)
-		}
 	}
 	return objToUpdate, nil
 }
@@ -408,9 +406,7 @@ func (p *smpPatcher) applyPatchToCurrentObject(currentObject runtime.Object) (ru
 	}
 
 	if p.fieldManager != nil {
-		if newObj, err = p.fieldManager.Update(currentObject, newObj, managerOrUserAgent(p.options.FieldManager, p.userAgent)); err != nil {
+		newObj = p.fieldManager.UpdateNoErrors(currentObject, newObj, managerOrUserAgent(p.options.FieldManager, p.userAgent))
-			return nil, fmt.Errorf("failed to update object (smp PATCH for %v) managed fields: %v", p.kind, err)
-		}
 	}
 	return newObj, nil
 }

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

@@ -131,11 +131,7 @@ func UpdateResource(r rest.Updater, scope *RequestScope, admit admission.Interfa
 		if scope.FieldManager != nil {
 			transformers = append(transformers, func(_ context.Context, newObj, liveObj runtime.Object) (runtime.Object, error) {
 				if shouldUpdateManagedFields {
-					obj, err := scope.FieldManager.Update(liveObj, newObj, managerOrUserAgent(options.FieldManager, req.UserAgent()))
+					return scope.FieldManager.UpdateNoErrors(liveObj, newObj, managerOrUserAgent(options.FieldManager, req.UserAgent())), nil
-					if err != nil {
-						return nil, fmt.Errorf("failed to update object (Update for %v) managed fields: %v", scope.Kind, err)
-					}
-					return obj, nil
 				}
 				return newObj, nil
 			})

test/integration/apiserver/apply/apply_test.go

@@ -1422,6 +1422,162 @@ func TestClearManagedFieldsWithUpdate(t *testing.T) {
 	}
 }
 
+// TestErrorsDontFail
+func TestErrorsDontFail(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)()
+
+	_, client, closeFn := setup(t)
+	defer closeFn()
+
+	// Tries to create with a managed fields that has an empty `fieldsType`.
+	_, err := client.CoreV1().RESTClient().Post().
+		Namespace("default").
+		Resource("configmaps").
+		Param("fieldManager", "apply_test").
+		Body([]byte(`{
+			"apiVersion": "v1",
+			"kind": "ConfigMap",
+			"metadata": {
+				"name": "test-cm",
+				"namespace": "default",
+				"managedFields": [{
+					"manager": "apply_test",
+					"operation": "Apply",
+					"apiVersion": "v1",
+					"time": "2019-07-08T09:31:18Z",
+					"fieldsType": "",
+					"fieldsV1": {}
+				}],
+				"labels": {
+					"test-label": "test"
+				}
+			},
+			"data": {
+				"key": "value"
+			}
+		}`)).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object with empty fieldsType: %v", err)
+	}
+}
+
+func TestErrorsDontFailUpdate(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)()
+
+	_, client, closeFn := setup(t)
+	defer closeFn()
+
+	_, err := client.CoreV1().RESTClient().Post().
+		Namespace("default").
+		Resource("configmaps").
+		Param("fieldManager", "apply_test").
+		Body([]byte(`{
+			"apiVersion": "v1",
+			"kind": "ConfigMap",
+			"metadata": {
+				"name": "test-cm",
+				"namespace": "default",
+				"labels": {
+					"test-label": "test"
+				}
+			},
+			"data": {
+				"key": "value"
+			}
+		}`)).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object: %v", err)
+	}
+
+	_, err = client.CoreV1().RESTClient().Put().
+		Namespace("default").
+		Resource("configmaps").
+		Name("test-cm").
+		Param("fieldManager", "apply_test").
+		Body([]byte(`{
+			"apiVersion": "v1",
+			"kind": "ConfigMap",
+			"metadata": {
+				"name": "test-cm",
+				"namespace": "default",
+				"managedFields": [{
+					"manager": "apply_test",
+					"operation": "Apply",
+					"apiVersion": "v1",
+					"time": "2019-07-08T09:31:18Z",
+					"fieldsType": "",
+					"fieldsV1": {}
+				}],
+				"labels": {
+					"test-label": "test"
+				}
+			},
+			"data": {
+				"key": "value"
+			}
+		}`)).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to update object with empty fieldsType: %v", err)
+	}
+}
+
+func TestErrorsDontFailPatch(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)()
+
+	_, client, closeFn := setup(t)
+	defer closeFn()
+
+	_, err := client.CoreV1().RESTClient().Post().
+		Namespace("default").
+		Resource("configmaps").
+		Param("fieldManager", "apply_test").
+		Body([]byte(`{
+			"apiVersion": "v1",
+			"kind": "ConfigMap",
+			"metadata": {
+				"name": "test-cm",
+				"namespace": "default",
+				"labels": {
+					"test-label": "test"
+				}
+			},
+			"data": {
+				"key": "value"
+			}
+		}`)).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to create object: %v", err)
+	}
+
+	_, err = client.CoreV1().RESTClient().Patch(types.JSONPatchType).
+		Namespace("default").
+		Resource("configmaps").
+		Name("test-cm").
+		Param("fieldManager", "apply_test").
+		Body([]byte(`[{"op": "replace", "path": "/metadata/managedFields", "value": [{
+			"manager": "apply_test",
+			"operation": "Apply",
+			"apiVersion": "v1",
+			"time": "2019-07-08T09:31:18Z",
+			"fieldsType": "",
+			"fieldsV1": {}
+		}]}]`)).
+		Do(context.TODO()).
+		Get()
+	if err != nil {
+		t.Fatalf("Failed to patch object with empty FieldsType: %v", err)
+	}
+
+}
+
 var podBytes = []byte(`
 apiVersion: v1
 kind: Pod