Merge pull request #63216 from liggitt/collapse-patch-convertor

Automatic merge from submit-queue (batch tested with PRs 59735, 63216). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use single convertor in patch handler the request scope convertors are used to convert the object for output after the patch handler is finished with it (and were used internally by the patch handler already). they are required to be correct for the type being handled. ```release-note NONE ```

Merge pull request #63216 from liggitt/collapse-patch-convertor
b87a392b · Kubernetes Submit Queue · GitHub · 6b64c07b · 2c1a6899 · b87a392b
Unverified Commit b87a392b authored Apr 26, 2018 by Kubernetes Submit Queue Committed by GitHub Apr 26, 2018
5 changed files
--- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go
+++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go
@@ -236,7 +236,7 @@ func (r *crdHandler) serveResource(w http.ResponseWriter, req *http.Request, req
 	case "update":
 		return handlers.UpdateResource(storage, requestScope, r.admission)
 	case "patch":
-		return handlers.PatchResource(storage, requestScope, r.admission, unstructured.UnstructuredObjectConverter{}, supportedTypes)
+		return handlers.PatchResource(storage, requestScope, r.admission, supportedTypes)
 	case "delete":
 		allowsOptions := true
 		return handlers.DeleteResource(storage, allowsOptions, requestScope, r.admission)
@@ -259,7 +259,7 @@ func (r *crdHandler) serveStatus(w http.ResponseWriter, req *http.Request, reque
 	case "update":
 		return handlers.UpdateResource(storage, requestScope, r.admission)
 	case "patch":
-		return handlers.PatchResource(storage, requestScope, r.admission, unstructured.UnstructuredObjectConverter{}, supportedTypes)
+		return handlers.PatchResource(storage, requestScope, r.admission, supportedTypes)
 	default:
 		http.Error(w, fmt.Sprintf("unhandled verb %q", requestInfo.Verb), http.StatusMethodNotAllowed)
 		return nil
@@ -276,7 +276,7 @@ func (r *crdHandler) serveScale(w http.ResponseWriter, req *http.Request, reques
 	case "update":
 		return handlers.UpdateResource(storage, requestScope, r.admission)
 	case "patch":
-		return handlers.PatchResource(storage, requestScope, r.admission, unstructured.UnstructuredObjectConverter{}, supportedTypes)
+		return handlers.PatchResource(storage, requestScope, r.admission, supportedTypes)
 	default:
 		http.Error(w, fmt.Sprintf("unhandled verb %q", requestInfo.Verb), http.StatusMethodNotAllowed)
 		return nil

--- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go
@@ -245,6 +245,7 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission.

 		Creater:         scheme,
 		Convertor:       scheme,
+		UnsafeConvertor: runtime.UnsafeObjectConvertor(scheme),
 		Defaulter:       scheme,
 		Typer:           scheme,
 		Linker:          selfLinker,
@@ -3170,6 +3171,7 @@ func TestParentResourceIsRequired(t *testing.T) {
 		Root:            "/" + prefix,
 		Creater:         scheme,
 		Convertor:       scheme,
+		UnsafeConvertor: runtime.UnsafeObjectConvertor(scheme),
 		Defaulter:       scheme,
 		Typer:           scheme,
 		Linker:          selfLinker,
@@ -3197,12 +3199,13 @@ func TestParentResourceIsRequired(t *testing.T) {
 			"simple":     &SimpleRESTStorage{},
 			"simple/sub": storage,
 		},
-		Root:      "/" + prefix,
-		Creater:   scheme,
-		Convertor: scheme,
-		Defaulter: scheme,
-		Typer:     scheme,
-		Linker:    selfLinker,
+		Root:            "/" + prefix,
+		Creater:         scheme,
+		Convertor:       scheme,
+		UnsafeConvertor: runtime.UnsafeObjectConvertor(scheme),
+		Defaulter:       scheme,
+		Typer:           scheme,
+		Linker:          selfLinker,

 		Admit: admissionControl,

@@ -3822,11 +3825,12 @@ func TestXGSubresource(t *testing.T) {
 	group := APIGroupVersion{
 		Storage: storage,

-		Creater:   scheme,
-		Convertor: scheme,
-		Defaulter: scheme,
-		Typer:     scheme,
-		Linker:    selfLinker,
+		Creater:         scheme,
+		Convertor:       scheme,
+		UnsafeConvertor: runtime.UnsafeObjectConvertor(scheme),
+		Defaulter:       scheme,
+		Typer:           scheme,
+		Linker:          selfLinker,

 		ParameterCodec: parameterCodec,


--- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go
@@ -42,7 +42,7 @@ import (
 )

 // PatchResource returns a function that will handle a resource patch.
-func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface, converter runtime.ObjectConvertor, patchTypes []string) http.HandlerFunc {
+func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface, patchTypes []string) http.HandlerFunc {
 	return func(w http.ResponseWriter, req *http.Request) {
 		// For performance tracking purposes.
 		trace := utiltrace.New("Patch " + req.URL.Path)
@@ -77,14 +77,7 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface
 		ctx := req.Context()
 		ctx = request.WithNamespace(ctx, namespace)

-		// TODO: this is NOT using the scope's convertor [sic]. Figure
-		// out if this is intentional or not. Perhaps it matters on
-		// subresources? Rename this parameter if this is purposful and
-		// delete it (using scope.Convertor instead) otherwise.
-		//
-		// Already some tests set this converter but apparently not the
-		// scope's unsafeConvertor.
-		schemaReferenceObj, err := converter.ConvertToVersion(r.New(), scope.Kind.GroupVersion())
+		schemaReferenceObj, err := scope.UnsafeConvertor.ConvertToVersion(r.New(), scope.Kind.GroupVersion())
 		if err != nil {
 			scope.err(err, w, req)
 			return

--- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go
@@ -419,7 +419,7 @@ func (tc *patchTestCase) Run(t *testing.T) {

 		if tc.expectedTries > 0 {
 			if tc.expectedTries != testPatcher.numUpdates {
-				t.Errorf("%s: expected %d tries, got %d", tc.expectedTries, testPatcher.numUpdates)
+				t.Errorf("%s: expected %d tries, got %d", tc.name, tc.expectedTries, testPatcher.numUpdates)
 			}
 		}


--- a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go
+++ b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go
@@ -629,7 +629,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 				string(types.MergePatchType),
 				string(types.StrategicMergePatchType),
 			}
-			handler := metrics.InstrumentRouteFunc(action.Verb, resource, subresource, requestScope, restfulPatchResource(patcher, reqScope, admit, a.group.Convertor, supportedTypes))
+			handler := metrics.InstrumentRouteFunc(action.Verb, resource, subresource, requestScope, restfulPatchResource(patcher, reqScope, admit, supportedTypes))
 			route := ws.PATCH(action.Path).To(handler).
 				Doc(doc).
 				Param(ws.QueryParameter("pretty", "If 'true', then the output is pretty printed.")).
@@ -1005,9 +1005,9 @@ func restfulUpdateResource(r rest.Updater, scope handlers.RequestScope, admit ad
 	}
 }

-func restfulPatchResource(r rest.Patcher, scope handlers.RequestScope, admit admission.Interface, converter runtime.ObjectConvertor, supportedTypes []string) restful.RouteFunction {
+func restfulPatchResource(r rest.Patcher, scope handlers.RequestScope, admit admission.Interface, supportedTypes []string) restful.RouteFunction {
 	return func(req *restful.Request, res *restful.Response) {
-		handlers.PatchResource(r, scope, admit, converter, supportedTypes)(res.ResponseWriter, req.Request)
+		handlers.PatchResource(r, scope, admit, supportedTypes)(res.ResponseWriter, req.Request)
 	}
 }