From ed2cf6ef2cc4f4d358eee038d2d87387614bbf45 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 21 May 2020 13:23:30 -0700 Subject: [PATCH 1/2] FieldManager: Reset if we receive nil or a list with one empty item --- .../handlers/fieldmanager/fieldmanager.go | 39 +++++++-- .../fieldmanager/fieldmanager_test.go | 83 +++++++++++++++++++ .../fieldmanager/internal/managedfields.go | 12 +-- .../integration/apiserver/apply/apply_test.go | 74 +++++++++++++++++ 4 files changed, 189 insertions(+), 19 deletions(-) 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 192509abb70..56054fc6eb0 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,6 +18,7 @@ package fieldmanager import ( "fmt" + "reflect" "time" "k8s.io/apimachinery/pkg/api/meta" @@ -111,21 +112,25 @@ 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 { - // If we also can't decode the liveObject, then - // just restart managedFields from scratch. + if managed, err = internal.DecodeObjectManagedFields(liveAccessor.GetManagedFields()); err != nil { managed = internal.NewEmptyManaged() } } @@ -163,17 +168,33 @@ func (f *FieldManager) UpdateNoErrors(liveObj, newObj runtime.Object, manager st 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 index 629f3fe0c68..0a8b1aa4d36 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 @@ -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 index 40f608c8283..9e4fa96f887 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 @@ -78,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) } diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index 0204438070d..da1b11133bd 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -1578,6 +1578,80 @@ func TestErrorsDontFailPatch(t *testing.T) { } +// TestClearManagedFieldsWithUpdateEmptyList verifies it's possible to clear the managedFields by sending an empty list. +func TestClearManagedFieldsWithUpdateEmptyList(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + _, client, closeFn := setup(t) + defer closeFn() + + _, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Resource("configmaps"). + Name("test-cm"). + 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 using Apply patch: %v", err) + } + + _, err = client.CoreV1().RESTClient().Put(). + Namespace("default"). + Resource("configmaps"). + Name("test-cm"). + Body([]byte(`{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm", + "namespace": "default", + "managedFields": [], + "labels": { + "test-label": "test" + } + }, + "data": { + "key": "value" + } + }`)).Do(context.TODO()).Get() + if err != nil { + t.Fatalf("Failed to patch object: %v", err) + } + + object, err := client.CoreV1().RESTClient().Get().Namespace("default").Resource("configmaps").Name("test-cm").Do(context.TODO()).Get() + if err != nil { + t.Fatalf("Failed to retrieve object: %v", err) + } + + accessor, err := meta.Accessor(object) + if err != nil { + t.Fatalf("Failed to get meta accessor: %v", err) + } + + if managedFields := accessor.GetManagedFields(); len(managedFields) != 0 { + t.Fatalf("Failed to clear managedFields, got: %v", managedFields) + } + + if labels := accessor.GetLabels(); len(labels) < 1 { + t.Fatalf("Expected other fields to stay untouched, got: %v", object) + } +} + var podBytes = []byte(` apiVersion: v1 kind: Pod From 3f10709e4ce7e14fa9efc019ade3e99bb0a84b8a Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 30 Apr 2020 13:38:23 -0700 Subject: [PATCH 2/2] Fix fieldType being dropped by older go-clients --- .../pkg/apis/meta/v1/validation/validation.go | 2 +- .../meta/v1/validation/validation_test.go | 10 ++--- .../fieldmanager/internal/managedfields.go | 11 ++++- .../internal/managedfields_test.go | 45 +++++++++++++++++++ 4 files changed, 61 insertions(+), 7 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 index 2743793dde2..fcd491f4c07 100644 --- 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 index 30d6289f8ee..aa71a600ae2 100644 --- 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/fieldmanager/internal/managedfields.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go index 9e4fa96f887..c5434b101e0 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 @@ -107,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/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go index 4b92ea6ec33..68a6fa5a2b7 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go +++ b/staging/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) {