diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go index f97cbaa2c85..653c1ff82a8 100644 --- 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 index bd47efe2227..192509abb70 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 @@ -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) { 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 index 4778fa79259..40f608c8283 100644 --- 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{ 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 index b3e230eba3c..3ec16a8386e 100644 --- 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/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. - 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()) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index e9d11001758..c295d0aa659 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/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 { - return nil, fmt.Errorf("failed to update object (json PATCH for %v) managed fields: %v", p.kind, err) - } + objToUpdate = p.fieldManager.UpdateNoErrors(currentObject, objToUpdate, managerOrUserAgent(p.options.FieldManager, p.userAgent)) } 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 { - return nil, fmt.Errorf("failed to update object (smp PATCH for %v) managed fields: %v", p.kind, err) - } + newObj = p.fieldManager.UpdateNoErrors(currentObject, newObj, managerOrUserAgent(p.options.FieldManager, p.userAgent)) } return newObj, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index c58fe9d5792..91b909ea6d1 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/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())) - if err != nil { - return nil, fmt.Errorf("failed to update object (Update for %v) managed fields: %v", scope.Kind, err) - } - return obj, nil + return scope.FieldManager.UpdateNoErrors(liveObj, newObj, managerOrUserAgent(options.FieldManager, req.UserAgent())), nil } return newObj, nil }) diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index 496daedb257..0204438070d 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/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