From d13420f7512522ec99bb9e498868090a41873cc2 Mon Sep 17 00:00:00 2001 From: Josh Samuels Date: Wed, 14 Aug 2019 23:08:45 -0700 Subject: [PATCH 1/3] Apply will fail with managed fields + tests --- .../handlers/fieldmanager/fieldmanager.go | 6 +- .../fieldmanager/fieldmanager_test.go | 73 +++++++++++++++++++ 2 files changed, 77 insertions(+), 2 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 d6d21d450d1..239ae161940 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 @@ -149,9 +149,11 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r // Apply is used when server-side apply is called, as it merges the // object and update the managed fields. func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, fieldManager string, force bool) (runtime.Object, error) { - // If the object doesn't have metadata, apply isn't allowed. - if _, err := meta.Accessor(liveObj); err != nil { + // If the object doesn't have metadata or managed fields is not empty, apply isn't allowed. + if objMeta, err := meta.Accessor(liveObj); err != nil { return nil, fmt.Errorf("couldn't get accessor: %v", err) + } else if objMeta.GetManagedFields() != nil && len(objMeta.GetManagedFields()) != 0 { + return nil, fmt.Errorf("apply is not allowed with managed fields set but was: %v", objMeta.GetManagedFields()) } managed, err := internal.DecodeObjectManagedFields(liveObj) 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 bb347bee825..6698792e354 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 @@ -25,6 +25,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -439,3 +440,75 @@ func BenchmarkRepeatedUpdate(b *testing.B) { } } } + +func TestApplyFailsWithManagedFields(t *testing.T) { + f := NewTestFieldManager() + + obj := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + ManagedFields: []metav1.ManagedFieldsEntry{ + { + Manager: "test", + }, + }, + }, + } + + _, err := f.Apply(obj, []byte(`{ + "apiVersion": "apps/v1", + "kind": "Pod", + "metadata": { + "labels": { + "a": "b" + }, + } + }`), "fieldmanager_test", false) + + if err == nil { + t.Fatalf("successfully applied with set managed fields") + } +} + +func TestApplySuccessWithEmptyManagedFields(t *testing.T) { + f := NewTestFieldManager() + + obj := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + ManagedFields: []metav1.ManagedFieldsEntry{}, + }, + } + + _, err := f.Apply(obj, []byte(`{ + "apiVersion": "apps/v1", + "kind": "Pod", + "metadata": { + "labels": { + "a": "b" + }, + } + }`), "fieldmanager_test", false) + + if err != nil { + t.Fatalf("failed to apply object: %v", err) + } +} + +func TestApplySuccessWithNilManagedFields(t *testing.T) { + f := NewTestFieldManager() + + obj := &corev1.Pod{} + + _, err := f.Apply(obj, []byte(`{ + "apiVersion": "apps/v1", + "kind": "Pod", + "metadata": { + "labels": { + "a": "b" + }, + } + }`), "fieldmanager_test", false) + + if err != nil { + t.Fatalf("failed to apply object: %v", err) + } +} From d61b833161df5b0a47f3c90b6673af36002f844f Mon Sep 17 00:00:00 2001 From: Josh Samuels Date: Tue, 27 Aug 2019 23:19:56 -0400 Subject: [PATCH 2/3] Moved managed fields validation to server-side apply --- .../handlers/fieldmanager/fieldmanager.go | 11 ++-- .../fieldmanager/fieldmanager_test.go | 51 +++---------------- 2 files changed, 15 insertions(+), 47 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 239ae161940..e385a4f5417 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 @@ -149,11 +149,9 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r // Apply is used when server-side apply is called, as it merges the // object and update the managed fields. func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, fieldManager string, force bool) (runtime.Object, error) { - // If the object doesn't have metadata or managed fields is not empty, apply isn't allowed. - if objMeta, err := meta.Accessor(liveObj); err != nil { + // If the object doesn't have metadata, apply isn't allowed. + if _, err := meta.Accessor(liveObj); err != nil { return nil, fmt.Errorf("couldn't get accessor: %v", err) - } else if objMeta.GetManagedFields() != nil && len(objMeta.GetManagedFields()) != 0 { - return nil, fmt.Errorf("apply is not allowed with managed fields set but was: %v", objMeta.GetManagedFields()) } managed, err := internal.DecodeObjectManagedFields(liveObj) @@ -166,6 +164,11 @@ func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, fieldManager if err := yaml.Unmarshal(patch, &patchObj.Object); err != nil { return nil, fmt.Errorf("error decoding YAML: %v", err) } + + if patchObj.GetManagedFields() != nil { + return nil, fmt.Errorf("managed fields must be nil but was %v", patchObj.GetManagedFields()) + } + if patchObj.GetAPIVersion() != f.groupVersion.String() { return nil, errors.NewBadRequest( 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 6698792e354..cdabd502c61 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 @@ -25,7 +25,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -444,23 +443,15 @@ func BenchmarkRepeatedUpdate(b *testing.B) { func TestApplyFailsWithManagedFields(t *testing.T) { f := NewTestFieldManager() - obj := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - ManagedFields: []metav1.ManagedFieldsEntry{ - { - Manager: "test", - }, - }, - }, - } - - _, err := f.Apply(obj, []byte(`{ + _, err := f.Apply(&corev1.Pod{}, []byte(`{ "apiVersion": "apps/v1", "kind": "Pod", "metadata": { - "labels": { - "a": "b" - }, + "managedFields": [ + { + "manager": "test", + } + ] } }`), "fieldmanager_test", false) @@ -469,36 +460,10 @@ func TestApplyFailsWithManagedFields(t *testing.T) { } } -func TestApplySuccessWithEmptyManagedFields(t *testing.T) { +func TestApplySuccessWithNoManagedFields(t *testing.T) { f := NewTestFieldManager() - obj := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - ManagedFields: []metav1.ManagedFieldsEntry{}, - }, - } - - _, err := f.Apply(obj, []byte(`{ - "apiVersion": "apps/v1", - "kind": "Pod", - "metadata": { - "labels": { - "a": "b" - }, - } - }`), "fieldmanager_test", false) - - if err != nil { - t.Fatalf("failed to apply object: %v", err) - } -} - -func TestApplySuccessWithNilManagedFields(t *testing.T) { - f := NewTestFieldManager() - - obj := &corev1.Pod{} - - _, err := f.Apply(obj, []byte(`{ + _, err := f.Apply(&corev1.Pod{}, []byte(`{ "apiVersion": "apps/v1", "kind": "Pod", "metadata": { From 2efc617a3c111e3c3cb189ddd21c0396152bc671 Mon Sep 17 00:00:00 2001 From: Josh Samuels Date: Fri, 30 Aug 2019 00:28:13 -0400 Subject: [PATCH 3/3] Updated stripFieldsTest to be run with update instead of apply --- .../fieldmanager/fieldmanager_test.go | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) 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 cdabd502c61..35e736ad11e 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 @@ -22,9 +22,12 @@ import ( "net/http" "testing" + "time" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -76,37 +79,36 @@ func TestApplyStripsFields(t *testing.T) { obj := &corev1.Pod{} - newObj, err := f.Apply(obj, []byte(`{ - "apiVersion": "apps/v1", - "kind": "Deployment", - "metadata": { - "name": "b", - "namespace": "b", - "creationTimestamp": "2016-05-19T09:59:00Z", - "selfLink": "b", - "uid": "b", - "clusterName": "b", - "generation": 0, - "managedFields": [{ - "manager": "apply", - "operation": "Apply", - "apiVersion": "apps/v1", - "fields": { - "f:metadata": { - "f:labels": { - "f:test-label": {} - } - } - } - }], - "resourceVersion": "b" - } - }`), "fieldmanager_test", false) + newObj := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "b", + Namespace: "b", + CreationTimestamp: metav1.NewTime(time.Now()), + SelfLink: "b", + UID: "b", + ClusterName: "b", + Generation: 0, + ManagedFields: []metav1.ManagedFieldsEntry{ + { + Manager: "update", + Operation: metav1.ManagedFieldsOperationApply, + APIVersion: "apps/v1", + }, + }, + ResourceVersion: "b", + }, + } + + updatedObj, err := f.Update(obj, newObj, "fieldmanager_test") if err != nil { t.Fatalf("failed to apply object: %v", err) } - accessor, err := meta.Accessor(newObj) + accessor, err := meta.Accessor(updatedObj) if err != nil { t.Fatalf("couldn't get accessor: %v", err) }