From d5bd17cda0c134e5ef5c03c3eac79a9ce4e18003 Mon Sep 17 00:00:00 2001 From: Aaron Prindle Date: Thu, 7 Mar 2019 16:52:26 -0800 Subject: [PATCH] Added version check between patch and live object in server side apply What is the problem being solved? https://github.com/kubernetes/kubernetes/pull/75135 Currently version compatibility is not being checked in server side apply between the patch object and the live object. This is causing a merge that will error to be run and the apiserver returns a 500 error. The request should fail if the apiVersion provided in the object is different from the apiVersion in the url, but it should fail before trying to merge, and be a 4xx error. Probably a bad request error. Why is this the best approach? The approach of serializing the patch byte array and then checking for version equality with the already serialized live object is the simplest and most straightforward solution. --- .../pkg/endpoints/handlers/fieldmanager/BUILD | 4 ++ .../handlers/fieldmanager/fieldmanager.go | 20 +++++++-- .../fieldmanager/fieldmanager_test.go | 41 +++++++++++++++++-- 3 files changed, 59 insertions(+), 6 deletions(-) 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 1c2892a40e..10379edd55 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD @@ -7,14 +7,17 @@ go_library( importpath = "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager", visibility = ["//visibility:public"], deps = [ + "//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", "//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", ], ) @@ -41,6 +44,7 @@ go_test( embed = [":go_default_library"], deps = [ "//staging/src/k8s.io/api/core/v1:go_default_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/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", 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 7879dacb47..212bcadfe2 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 @@ -20,14 +20,17 @@ import ( "fmt" "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" 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" ) // FieldManager updates the managed fields and merge applied @@ -149,9 +152,20 @@ func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, fieldManager if err != nil { return nil, fmt.Errorf("failed to decode managed fields: %v", err) } - // We can assume that patchObj is already on the proper version: - // it shouldn't have to be converted so that it's not defaulted. - // TODO (jennybuckley): Explicitly checkt that patchObj is in the proper version. + // Check that the patch object has the same version as the live object + patchObj := &unstructured.Unstructured{Object: map[string]interface{}{}} + + if err := yaml.Unmarshal(patch, &patchObj.Object); err != nil { + return nil, fmt.Errorf("error decoding YAML: %v", err) + } + if patchObj.GetAPIVersion() != f.groupVersion.String() { + return nil, + errors.NewBadRequest( + fmt.Sprintf("Incorrect version specified in apply patch. "+ + "Specified patch version: %s, expected: %s", + patchObj.GetAPIVersion(), f.groupVersion.String())) + } + liveObjVersioned, err := f.toVersioned(liveObj) if err != nil { return nil, fmt.Errorf("failed to convert live object to proper version: %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 f512c98b76..776dfd0a1e 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 @@ -18,9 +18,11 @@ package fieldmanager_test import ( "errors" + "net/http" "testing" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -72,7 +74,7 @@ func TestApplyStripsFields(t *testing.T) { obj := &corev1.Pod{} newObj, err := f.Apply(obj, []byte(`{ - "apiVersion": "v1", + "apiVersion": "apps/v1", "kind": "Deployment", "metadata": { "name": "b", @@ -85,7 +87,7 @@ func TestApplyStripsFields(t *testing.T) { "managedFields": [{ "manager": "apply", "operation": "Apply", - "apiVersion": "v1", + "apiVersion": "apps/v1", "fields": { "f:metadata": { "f:labels": { @@ -111,13 +113,46 @@ func TestApplyStripsFields(t *testing.T) { } } +func TestVersionCheck(t *testing.T) { + f := NewTestFieldManager(t) + + obj := &corev1.Pod{} + + // patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors + _, err := f.Apply(obj, []byte(`{ + "apiVersion": "apps/v1", + "kind": "Deployment", + }`), "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(obj, []byte(`{ + "apiVersion": "apps/v2", + "kind": "Deployment", + }`), "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 TestApplyDoesNotStripLabels(t *testing.T) { f := NewTestFieldManager(t) obj := &corev1.Pod{} newObj, err := f.Apply(obj, []byte(`{ - "apiVersion": "v1", + "apiVersion": "apps/v1", "kind": "Pod", "metadata": { "labels": {