From ddf0d4b8034697a8dca23a3c8bc5620629bd691b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kevin=20Wiesm=C3=BCller?= Date: Thu, 9 Jan 2020 22:34:33 +0100 Subject: [PATCH 1/2] change Apply signature and move decoding into handlers --- .../apiserver/pkg/endpoints/handlers/BUILD | 2 + .../pkg/endpoints/handlers/fieldmanager/BUILD | 2 - .../handlers/fieldmanager/buildmanagerinfo.go | 4 +- .../handlers/fieldmanager/capmanagers.go | 4 +- .../handlers/fieldmanager/capmanagers_test.go | 2 +- .../handlers/fieldmanager/fieldmanager.go | 6 +- .../fieldmanager/fieldmanager_test.go | 117 +++++++++++++++--- .../handlers/fieldmanager/skipnonapplied.go | 4 +- .../fieldmanager/skipnonapplied_test.go | 21 +++- .../handlers/fieldmanager/stripmeta.go | 6 +- .../handlers/fieldmanager/structuredmerge.go | 17 +-- .../apiserver/pkg/endpoints/handlers/patch.go | 10 +- 12 files changed, 144 insertions(+), 51 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD index 2b1e7c591b4..47a27a3340c 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD @@ -70,6 +70,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1/validation:go_default_library", @@ -104,6 +105,7 @@ go_library( "//vendor/golang.org/x/net/websocket:go_default_library", "//vendor/k8s.io/klog:go_default_library", "//vendor/k8s.io/utils/trace:go_default_library", + "//vendor/sigs.k8s.io/yaml:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD index dad87445a77..0562f8dca15 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD @@ -17,7 +17,6 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal:go_default_library", @@ -25,7 +24,6 @@ go_library( "//vendor/k8s.io/kube-openapi/pkg/util/proto:go_default_library", "//vendor/sigs.k8s.io/structured-merge-diff/fieldpath:go_default_library", "//vendor/sigs.k8s.io/structured-merge-diff/merge:go_default_library", - "//vendor/sigs.k8s.io/yaml:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/buildmanagerinfo.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/buildmanagerinfo.go index 450adad05d1..fc471e79759 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/buildmanagerinfo.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/buildmanagerinfo.go @@ -51,12 +51,12 @@ func (f *buildManagerInfoManager) Update(liveObj, newObj runtime.Object, managed } // Apply implements Manager. -func (f *buildManagerInfoManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { +func (f *buildManagerInfoManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { manager, err := f.buildManagerInfo(manager, metav1.ManagedFieldsOperationApply) if err != nil { return nil, nil, fmt.Errorf("failed to build manager identifier: %v", err) } - return f.fieldManager.Apply(liveObj, patch, managed, manager, force) + return f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force) } func (f *buildManagerInfoManager) buildManagerInfo(prefix string, operation metav1.ManagedFieldsOperationType) (string, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers.go index 8c38461613f..70ddc571973 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers.go @@ -58,8 +58,8 @@ func (f *capManagersManager) Update(liveObj, newObj runtime.Object, managed Mana } // Apply implements Manager. -func (f *capManagersManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) { - return f.fieldManager.Apply(liveObj, patch, managed, fieldManager, force) +func (f *capManagersManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) { + return f.fieldManager.Apply(liveObj, appliedObj, managed, fieldManager, force) } // capUpdateManagers merges a number of the oldest update entries into versioned buckets, diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers_test.go index e64a2b3c492..b4f43dd90d4 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers_test.go @@ -41,7 +41,7 @@ func (*fakeManager) Update(_, newObj runtime.Object, managed fieldmanager.Manage return newObj, managed, nil } -func (*fakeManager) Apply(_ runtime.Object, _ []byte, _ fieldmanager.Managed, _ string, force bool) (runtime.Object, fieldmanager.Managed, error) { +func (*fakeManager) Apply(_, _ runtime.Object, _ fieldmanager.Managed, _ string, force bool) (runtime.Object, fieldmanager.Managed, error) { panic("not implemented") return nil, nil, 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 3f2b08984ea..95742bf9d57 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 @@ -51,7 +51,7 @@ type Manager interface { // Apply is used when server-side apply is called, as it merges the // object and updates the managed fields. - Apply(liveObj runtime.Object, patch []byte, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) + Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) } // FieldManager updates the managed fields and merge applied @@ -135,7 +135,7 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (o // Apply is used when server-side apply is called, as it merges the // object and updates the managed fields. -func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, manager string, force bool) (object runtime.Object, err error) { +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 { return nil, fmt.Errorf("couldn't get accessor: %v", err) @@ -149,7 +149,7 @@ func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, manager strin internal.RemoveObjectManagedFields(liveObj) - if object, managed, err = f.fieldManager.Apply(liveObj, patch, managed, manager, force); err != nil { + if object, managed, err = f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force); err != nil { return nil, 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 1a222a0a02a..cf4d8b9417f 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 @@ -106,7 +106,7 @@ func (f *TestFieldManager) Reset() { f.liveObj = f.emptyObj.DeepCopyObject() } -func (f *TestFieldManager) Apply(obj []byte, manager string, force bool) error { +func (f *TestFieldManager) Apply(obj runtime.Object, manager string, force bool) error { out, err := fieldmanager.NewFieldManager(f.fieldManager).Apply(f.liveObj, obj, manager, force) if err == nil { f.liveObj = out @@ -174,7 +174,8 @@ func TestUpdateApplyConflict(t *testing.T) { t.Fatalf("failed to apply object: %v", err) } - err := f.Apply([]byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "apps/v1", "kind": "Deployment", "metadata": { @@ -183,7 +184,11 @@ func TestUpdateApplyConflict(t *testing.T) { "spec": { "replicas": 101, } - }`), "fieldmanager_conflict", false) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + err := f.Apply(appliedObj, "fieldmanager_conflict", false) if err == nil || !apierrors.IsConflict(err) { t.Fatalf("Expecting to get conflicts but got %v", err) } @@ -225,20 +230,68 @@ func TestApplyStripsFields(t *testing.T) { func TestVersionCheck(t *testing.T) { f := NewTestFieldManager(schema.FromAPIVersionAndKind("apps/v1", "Deployment")) - // patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors - err := f.Apply([]byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "apps/v1", "kind": "Deployment", - }`), "fieldmanager_test", false) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + // patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors + err := f.Apply(appliedObj, "fieldmanager_test", false) if err != nil { t.Fatalf("failed to apply object: %v", err) } - // patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error - err = f.Apply([]byte(`{ + appliedObj = &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "apps/v1beta1", "kind": "Deployment", - }`), "fieldmanager_test", false) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + // patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error + err = f.Apply(appliedObj, "fieldmanager_test", false) + if err == nil { + t.Fatalf("expected an error from mismatched patch and live versions") + } + switch typ := err.(type) { + default: + t.Fatalf("expected error to be of type %T was %T", apierrors.StatusError{}, typ) + case apierrors.APIStatus: + if typ.Status().Code != http.StatusBadRequest { + t.Fatalf("expected status code to be %d but was %d", + http.StatusBadRequest, typ.Status().Code) + } + } +} +func TestVersionCheckDoesNotPanic(t *testing.T) { + f := NewTestFieldManager(schema.FromAPIVersionAndKind("apps/v1", "Deployment")) + + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ + "apiVersion": "apps/v1", + "kind": "Deployment", + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + // patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors + err := f.Apply(appliedObj, "fieldmanager_test", false) + if err != nil { + t.Fatalf("failed to apply object: %v", err) + } + + appliedObj = &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + // patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error + err = f.Apply(appliedObj, "fieldmanager_test", false) if err == nil { t.Fatalf("expected an error from mismatched patch and live versions") } @@ -256,7 +309,8 @@ func TestVersionCheck(t *testing.T) { func TestApplyDoesNotStripLabels(t *testing.T) { f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod")) - err := f.Apply([]byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "v1", "kind": "Pod", "metadata": { @@ -264,7 +318,11 @@ func TestApplyDoesNotStripLabels(t *testing.T) { "a": "b" }, } - }`), "fieldmanager_test", false) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + err := f.Apply(appliedObj, "fieldmanager_test", false) if err != nil { t.Fatalf("failed to apply object: %v", err) } @@ -305,7 +363,12 @@ func TestApplyNewObject(t *testing.T) { t.Run(test.gvk.String(), func(t *testing.T) { f := NewTestFieldManager(test.gvk) - if err := f.Apply(test.obj, "fieldmanager_test", false); err != nil { + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal(test.obj, &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + if err := f.Apply(appliedObj, "fieldmanager_test", false); err != nil { t.Fatal(err) } }) @@ -362,7 +425,11 @@ func BenchmarkNewObject(b *testing.B) { b.ReportAllocs() b.ResetTimer() for n := 0; n < b.N; n++ { - err := f.Apply(test.obj, "fieldmanager_test", false) + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal(test.obj, &appliedObj.Object); err != nil { + b.Fatalf("error decoding YAML: %v", err) + } + err := f.Apply(appliedObj, "fieldmanager_test", false) if err != nil { b.Fatal(err) } @@ -387,7 +454,12 @@ func BenchmarkRepeatedUpdate(b *testing.B) { obj.Spec.Containers[0].Image = "nginx:4.3" objs = append(objs, obj) - err := f.Apply(podBytes, "fieldmanager_apply", false) + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal(podBytes, &appliedObj.Object); err != nil { + b.Fatalf("error decoding YAML: %v", err) + } + + err := f.Apply(appliedObj, "fieldmanager_apply", false) if err != nil { b.Fatal(err) } @@ -410,7 +482,8 @@ func BenchmarkRepeatedUpdate(b *testing.B) { func TestApplyFailsWithManagedFields(t *testing.T) { f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod")) - err := f.Apply([]byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "v1", "kind": "Pod", "metadata": { @@ -420,7 +493,11 @@ func TestApplyFailsWithManagedFields(t *testing.T) { } ] } - }`), "fieldmanager_test", false) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + err := f.Apply(appliedObj, "fieldmanager_test", false) if err == nil { t.Fatalf("successfully applied with set managed fields") @@ -430,7 +507,8 @@ func TestApplyFailsWithManagedFields(t *testing.T) { func TestApplySuccessWithNoManagedFields(t *testing.T) { f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod")) - err := f.Apply([]byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "v1", "kind": "Pod", "metadata": { @@ -438,7 +516,10 @@ func TestApplySuccessWithNoManagedFields(t *testing.T) { "a": "b" }, } - }`), "fieldmanager_test", false) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + err := f.Apply(appliedObj, "fieldmanager_test", false) if err != nil { t.Fatalf("failed to apply object: %v", err) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied.go index 4230a61d644..913615a6613 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied.go @@ -51,7 +51,7 @@ func (f *skipNonAppliedManager) Update(liveObj, newObj runtime.Object, managed M } // Apply implements Manager. -func (f *skipNonAppliedManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) { +func (f *skipNonAppliedManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) { if len(managed.Fields()) == 0 { emptyObj, err := f.objectCreater.New(f.gvk) if err != nil { @@ -62,5 +62,5 @@ func (f *skipNonAppliedManager) Apply(liveObj runtime.Object, patch []byte, mana return nil, nil, fmt.Errorf("failed to create manager for existing fields: %v", err) } } - return f.fieldManager.Apply(liveObj, patch, managed, fieldManager, force) + return f.fieldManager.Apply(liveObj, appliedObj, managed, fieldManager, force) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied_test.go index e8d8842984b..2f8d4fdcbcc 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" + "sigs.k8s.io/yaml" ) type fakeObjectCreater struct { @@ -49,7 +50,8 @@ func TestNoUpdateBeforeFirstApply(t *testing.T) { schema.GroupVersionKind{}, ) - if err := f.Apply([]byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "v1", "kind": "Pod", "metadata": { @@ -62,7 +64,11 @@ func TestNoUpdateBeforeFirstApply(t *testing.T) { "image": "nginx:latest" }] } - }`), "fieldmanager_test_apply", false); err != nil { + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } + + if err := f.Apply(appliedObj, "fieldmanager_test_apply", false); err != nil { t.Fatalf("failed to update object: %v", err) } @@ -96,7 +102,8 @@ func TestUpdateBeforeFirstApply(t *testing.T) { t.Fatalf("managedFields were tracked on update only: %v", m) } - appliedBytes := []byte(`{ + appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal([]byte(`{ "apiVersion": "v1", "kind": "Pod", "metadata": { @@ -109,9 +116,11 @@ func TestUpdateBeforeFirstApply(t *testing.T) { "image": "nginx:latest" }] } - }`) + }`), &appliedObj.Object); err != nil { + t.Fatalf("error decoding YAML: %v", err) + } - err := f.Apply(appliedBytes, "fieldmanager_test_apply", false) + err := f.Apply(appliedObj, "fieldmanager_test_apply", false) apiStatus, _ := err.(apierrors.APIStatus) if err == nil || !apierrors.IsConflict(err) || len(apiStatus.Status().Details.Causes) != 1 { t.Fatalf("Expecting to get one conflict but got %v", err) @@ -125,7 +134,7 @@ func TestUpdateBeforeFirstApply(t *testing.T) { t.Fatalf("Expecting conflict message to contain %q but got %q: %v", e, a, err) } - if err := f.Apply(appliedBytes, "fieldmanager_test_apply", true); err != nil { + if err := f.Apply(appliedObj, "fieldmanager_test_apply", true); err != nil { t.Fatalf("failed to update object: %v", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go index 64457a0e2fe..3e02fcc7dbd 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go @@ -64,13 +64,13 @@ func (f *stripMetaManager) Update(liveObj, newObj runtime.Object, managed Manage } // Apply implements Manager. -func (f *stripMetaManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { - newObj, managed, err := f.fieldManager.Apply(liveObj, patch, managed, manager, force) +func (f *stripMetaManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { + appliedObj, managed, err := f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force) if err != nil { return nil, nil, err } f.stripFields(managed.Fields(), manager) - return newObj, managed, nil + return appliedObj, managed, nil } // stripFields removes a predefined set of paths found in typed from managed 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 93fcc2dad11..03557532aa4 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 @@ -21,8 +21,8 @@ import ( "time" "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" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal" @@ -30,7 +30,6 @@ import ( openapiproto "k8s.io/kube-openapi/pkg/util/proto" "sigs.k8s.io/structured-merge-diff/fieldpath" "sigs.k8s.io/structured-merge-diff/merge" - "sigs.k8s.io/yaml" ) type structuredMergeManager struct { @@ -136,22 +135,18 @@ func (f *structuredMergeManager) Update(liveObj, newObj runtime.Object, managed } // Apply implements Manager. -func (f *structuredMergeManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { - patchObj := &unstructured.Unstructured{Object: map[string]interface{}{}} - if err := yaml.Unmarshal(patch, &patchObj.Object); err != nil { - return nil, nil, errors.NewBadRequest(fmt.Sprintf("error decoding YAML: %v", err)) - } - +func (f *structuredMergeManager) Apply(liveObj, patchObj runtime.Object, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { // Check that the patch object has the same version as the live object - if patchObj.GetAPIVersion() != f.groupVersion.String() { + if patchObj.GetObjectKind().GroupVersionKind().GroupVersion() != f.groupVersion { return nil, nil, errors.NewBadRequest( fmt.Sprintf("Incorrect version specified in apply patch. "+ "Specified patch version: %s, expected: %s", - patchObj.GetAPIVersion(), f.groupVersion.String())) + patchObj.GetObjectKind().GroupVersionKind().GroupVersion(), f.groupVersion)) } - if patchObj.GetManagedFields() != nil { + patchObjMeta, err := meta.Accessor(patchObj) + if patchObjMeta.GetManagedFields() != nil { return nil, nil, errors.NewBadRequest(fmt.Sprintf("metadata.managedFields must be nil")) } 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 4893578f1c3..8e28c8afbcc 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metainternalversionscheme "k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -48,6 +49,7 @@ import ( "k8s.io/apiserver/pkg/util/dryrun" utilfeature "k8s.io/apiserver/pkg/util/feature" utiltrace "k8s.io/utils/trace" + "sigs.k8s.io/yaml" ) const ( @@ -433,7 +435,13 @@ func (p *applyPatcher) applyPatchToCurrentObject(obj runtime.Object) (runtime.Ob if p.fieldManager == nil { panic("FieldManager must be installed to run apply") } - return p.fieldManager.Apply(obj, p.patch, p.options.FieldManager, force) + + patchObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := yaml.Unmarshal(p.patch, &patchObj.Object); err != nil { + return nil, errors.NewBadRequest(fmt.Sprintf("error decoding YAML: %v", err)) + } + + return p.fieldManager.Apply(obj, patchObj, p.options.FieldManager, force) } func (p *applyPatcher) createNewObject() (runtime.Object, error) { From a2526286498762de187db23f13da5d747ddd1c90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kevin=20Wiesm=C3=BCller?= Date: Fri, 10 Jan 2020 00:58:52 +0100 Subject: [PATCH 2/2] fix nits --- .../pkg/endpoints/handlers/fieldmanager/stripmeta.go | 4 ++-- .../pkg/endpoints/handlers/fieldmanager/structuredmerge.go | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go index 3e02fcc7dbd..cf949e5d746 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/stripmeta.go @@ -65,12 +65,12 @@ func (f *stripMetaManager) Update(liveObj, newObj runtime.Object, managed Manage // Apply implements Manager. func (f *stripMetaManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { - appliedObj, managed, err := f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force) + newObj, managed, err := f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force) if err != nil { return nil, nil, err } f.stripFields(managed.Fields(), manager) - return appliedObj, managed, nil + return newObj, managed, nil } // stripFields removes a predefined set of paths found in typed from managed 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 03557532aa4..067e286a2e2 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 @@ -137,15 +137,18 @@ func (f *structuredMergeManager) Update(liveObj, newObj runtime.Object, managed // Apply implements Manager. func (f *structuredMergeManager) Apply(liveObj, patchObj runtime.Object, managed Managed, manager string, force bool) (runtime.Object, Managed, error) { // Check that the patch object has the same version as the live object - if patchObj.GetObjectKind().GroupVersionKind().GroupVersion() != f.groupVersion { + if patchVersion := patchObj.GetObjectKind().GroupVersionKind().GroupVersion(); patchVersion != f.groupVersion { return nil, nil, errors.NewBadRequest( fmt.Sprintf("Incorrect version specified in apply patch. "+ "Specified patch version: %s, expected: %s", - patchObj.GetObjectKind().GroupVersionKind().GroupVersion(), f.groupVersion)) + patchVersion, f.groupVersion)) } patchObjMeta, err := meta.Accessor(patchObj) + if err != nil { + return nil, nil, fmt.Errorf("couldn't get accessor: %v", err) + } if patchObjMeta.GetManagedFields() != nil { return nil, nil, errors.NewBadRequest(fmt.Sprintf("metadata.managedFields must be nil")) }