Merge pull request kubernetes#4706 from mikedanese/PATCH

add PATCH verb to apiserver
duda · Mar 10, 2015 · 7aa060b · 7aa060b
2 parents 8273fff + 46ec580
commit 7aa060b
Show file tree

Hide file tree

Showing 7 changed files with 263 additions and 14 deletions.
diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go
@@ -465,6 +465,81 @@ func runAtomicPutTest(c *client.Client) {
 	glog.Info("Atomic PUTs work.")
 }
 
+func runPatchTest(c *client.Client) {
+	name := "patchservice"
+	resource := "services"
+	var svc api.Service
+	err := c.Post().Resource(resource).Body(
+		&api.Service{
+			TypeMeta: api.TypeMeta{
+				APIVersion: latest.Version,
+			},
+			ObjectMeta: api.ObjectMeta{
+				Name: name,
+				Labels: map[string]string{
+					"name": name,
+				},
+			},
+			Spec: api.ServiceSpec{
+				Port: 12345,
+				// This is here because validation requires it.
+				Selector: map[string]string{
+					"foo": "bar",
+				},
+				Protocol:        "TCP",
+				SessionAffinity: "None",
+			},
+		},
+	).Do().Into(&svc)
+	if err != nil {
+		glog.Fatalf("Failed creating patchservice: %v", err)
+	}
+	if len(svc.Labels) != 1 {
+		glog.Fatalf("Original length does not equal one")
+	}
+
+	// add label
+	_, err = c.Patch().Resource(resource).Name(name).Body([]byte("{\"labels\":{\"foo\":\"bar\"}}")).Do().Get()
+	if err != nil {
+		glog.Fatalf("Failed updating patchservice: %v", err)
+	}
+	err = c.Get().Resource(resource).Name(name).Do().Into(&svc)
+	if err != nil {
+		glog.Fatalf("Failed getting patchservice: %v", err)
+	}
+	if len(svc.Labels) != 2 || svc.Labels["foo"] != "bar" {
+		glog.Fatalf("Failed updating patchservice, labels are: %v", svc.Labels)
+	}
+
+	// remove one label
+	_, err = c.Patch().Resource(resource).Name(name).Body([]byte("{\"labels\":{\"name\":null}}")).Do().Get()
+	if err != nil {
+		glog.Fatalf("Failed updating patchservice: %v", err)
+	}
+	err = c.Get().Resource(resource).Name(name).Do().Into(&svc)
+	if err != nil {
+		glog.Fatalf("Failed getting patchservice: %v", err)
+	}
+	if len(svc.Labels) != 1 || svc.Labels["foo"] != "bar" {
+		glog.Fatalf("Failed updating patchservice, labels are: %v", svc.Labels)
+	}
+
+	// remove all labels
+	_, err = c.Patch().Resource(resource).Name(name).Body([]byte("{\"labels\":null}")).Do().Get()
+	if err != nil {
+		glog.Fatalf("Failed updating patchservice: %v", err)
+	}
+	err = c.Get().Resource(resource).Name(name).Do().Into(&svc)
+	if err != nil {
+		glog.Fatalf("Failed getting patchservice: %v", err)
+	}
+	if svc.Labels != nil {
+		glog.Fatalf("Failed remove all labels from patchservice: %v", svc.Labels)
+	}
+
+	glog.Info("PATCHs work.")
+}
+
 func runMasterServiceTest(client *client.Client) {
 	time.Sleep(12 * time.Second)
 	var svcList api.ServiceList
@@ -665,6 +740,7 @@ func main() {
 	testFuncs := []testFunc{
 		runReplicationControllerTest,
 		runAtomicPutTest,
+		runPatchTest,
 		runServiceTest,
 		runAPIVersionsTest,
 		runMasterServiceTest,

diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go
@@ -132,6 +132,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage
 	getter, isGetter := storage.(RESTGetter)
 	deleter, isDeleter := storage.(RESTDeleter)
 	updater, isUpdater := storage.(RESTUpdater)
+	patcher, isPatcher := storage.(RESTPatcher)
 	_, isWatcher := storage.(ResourceWatcher)
 	_, isRedirector := storage.(Redirector)
 
@@ -167,6 +168,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage
 
 		actions = appendIf(actions, action{"GET", itemPath, nameParams, namer}, isGetter)
 		actions = appendIf(actions, action{"PUT", itemPath, nameParams, namer}, isUpdater)
+		actions = appendIf(actions, action{"PATCH", itemPath, nameParams, namer}, isPatcher)
 		actions = appendIf(actions, action{"DELETE", itemPath, nameParams, namer}, isDeleter)
 		actions = appendIf(actions, action{"WATCH", "watch/" + itemPath, nameParams, namer}, isWatcher)
 		actions = appendIf(actions, action{"REDIRECT", "redirect/" + itemPath, nameParams, namer}, isRedirector)
@@ -196,6 +198,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage
 
 			actions = appendIf(actions, action{"GET", itemPath, nameParams, namer}, isGetter)
 			actions = appendIf(actions, action{"PUT", itemPath, nameParams, namer}, isUpdater)
+			actions = appendIf(actions, action{"PATCH", itemPath, nameParams, namer}, isPatcher)
 			actions = appendIf(actions, action{"DELETE", itemPath, nameParams, namer}, isDeleter)
 			actions = appendIf(actions, action{"WATCH", "watch/" + itemPath, nameParams, namer}, isWatcher)
 			actions = appendIf(actions, action{"REDIRECT", "redirect/" + itemPath, nameParams, namer}, isRedirector)
@@ -229,6 +232,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage
 
 			actions = appendIf(actions, action{"GET", itemPath, nameParams, namer}, isGetter)
 			actions = appendIf(actions, action{"PUT", itemPath, nameParams, namer}, isUpdater)
+			actions = appendIf(actions, action{"PATCH", itemPath, nameParams, namer}, isPatcher)
 			actions = appendIf(actions, action{"DELETE", itemPath, nameParams, namer}, isDeleter)
 			actions = appendIf(actions, action{"WATCH", "watch/" + itemPath, nameParams, namer}, isWatcher)
 			actions = appendIf(actions, action{"REDIRECT", "redirect/" + itemPath, nameParams, namer}, isRedirector)
@@ -281,6 +285,16 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage
 				Reads(versionedObject)
 			addParams(route, action.Params)
 			ws.Route(route)
+		case "PATCH": // Partially update a resource
+			route := ws.PATCH(action.Path).To(PatchResource(patcher, ctxFn, action.Namer, mapping.Codec, a.group.Typer, resource, admit)).
+				Filter(m).
+				Doc("partially update the specified " + kind).
+				// TODO: toggle patch strategy by content type
+				// Consumes("application/merge-patch+json", "application/json-patch+json").
+				Operation("patch" + kind).
+				Reads(versionedObject)
+			addParams(route, action.Params)
+			ws.Route(route)
 		case "POST": // Create a resource.
 			route := ws.POST(action.Path).To(CreateResource(creater, ctxFn, action.Namer, mapping.Codec, a.group.Typer, resource, admit)).
 				Filter(m).

diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go
@@ -173,7 +173,8 @@ func handleInternal(storage map[string]RESTStorage, admissionControl admission.I
 type Simple struct {
 	api.TypeMeta   `json:",inline"`
 	api.ObjectMeta `json:"metadata"`
-	Other          string `json:"other,omitempty"`
+	Other          string            `json:"other,omitempty"`
+	Labels         map[string]string `json:"labels,omitempty"`
 }
 
 func (*Simple) IsAnAPIObject() {}
@@ -826,6 +827,70 @@ func TestDeleteMissing(t *testing.T) {
 	}
 }
 
+func TestPatch(t *testing.T) {
+	storage := map[string]RESTStorage{}
+	ID := "id"
+	item := &Simple{
+		ObjectMeta: api.ObjectMeta{
+			Name:      ID,
+			Namespace: "", // update should allow the client to send an empty namespace
+		},
+		Other: "bar",
+	}
+	simpleStorage := SimpleRESTStorage{item: *item}
+	storage["simple"] = &simpleStorage
+	selfLinker := &setTestSelfLinker{
+		t:           t,
+		expectedSet: "/api/version/simple/" + ID + "?namespace=default",
+		name:        ID,
+		namespace:   api.NamespaceDefault,
+	}
+	handler := handleLinker(storage, selfLinker)
+	server := httptest.NewServer(handler)
+	defer server.Close()
+
+	client := http.Client{}
+	request, err := http.NewRequest("PATCH", server.URL+"/api/version/simple/"+ID, bytes.NewReader([]byte(`{"labels":{"foo":"bar"}}`)))
+	_, err = client.Do(request)
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+
+	if simpleStorage.updated == nil || simpleStorage.updated.Labels["foo"] != "bar" {
+		t.Errorf("Unexpected update value %#v, expected %#v.", simpleStorage.updated, item)
+	}
+	if !selfLinker.called {
+		t.Errorf("Never set self link")
+	}
+}
+
+func TestPatchRequiresMatchingName(t *testing.T) {
+	storage := map[string]RESTStorage{}
+	ID := "id"
+	item := &Simple{
+		ObjectMeta: api.ObjectMeta{
+			Name:      ID,
+			Namespace: "", // update should allow the client to send an empty namespace
+		},
+		Other: "bar",
+	}
+	simpleStorage := SimpleRESTStorage{item: *item}
+	storage["simple"] = &simpleStorage
+	handler := handle(storage)
+	server := httptest.NewServer(handler)
+	defer server.Close()
+
+	client := http.Client{}
+	request, err := http.NewRequest("PATCH", server.URL+"/api/version/simple/"+ID, bytes.NewReader([]byte(`{"metadata":{"name":"idbar"}}`)))
+	response, err := client.Do(request)
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+	if response.StatusCode != http.StatusBadRequest {
+		t.Errorf("Unexpected response %#v", response)
+	}
+}
+
 func TestUpdate(t *testing.T) {
 	storage := map[string]RESTStorage{}
 	simpleStorage := SimpleRESTStorage{}

diff --git a/pkg/apiserver/interfaces.go b/pkg/apiserver/interfaces.go
@@ -77,6 +77,11 @@ type RESTUpdater interface {
 	Update(ctx api.Context, obj runtime.Object) (runtime.Object, bool, error)
 }
 
+type RESTPatcher interface {
+	RESTGetter
+	RESTUpdater
+}
+
 // RESTResult indicates the result of a REST transformation.
 type RESTResult struct {
 	// The result of this operation. May be nil if the operation has no meaningful

diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go
@@ -30,6 +30,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 
 	"github.com/emicklei/go-restful"
+	"github.com/evanphx/json-patch"
 	"github.com/golang/glog"
 )
 
@@ -187,6 +188,83 @@ func CreateResource(r RESTCreater, ctxFn ContextFunc, namer ScopeNamer, codec ru
 	}
 }
 
+// PatchResource returns a function that will handle a resource patch
+// TODO: Eventually PatchResource should just use AtomicUpdate and this routine should be a bit cleaner
+func PatchResource(r RESTPatcher, ctxFn ContextFunc, namer ScopeNamer, codec runtime.Codec, typer runtime.ObjectTyper, resource string, admit admission.Interface) restful.RouteFunction {
+	return func(req *restful.Request, res *restful.Response) {
+		w := res.ResponseWriter
+		glog.Infof("hi")
+
+		// TODO: we either want to remove timeout or document it (if we document, move timeout out of this function and declare it in api_installer)
+		timeout := parseTimeout(req.Request.URL.Query().Get("timeout"))
+
+		namespace, name, err := namer.Name(req)
+		if err != nil {
+			notFound(w, req.Request)
+			return
+		}
+
+		obj := r.New()
+		// PATCH requires same permission as UPDATE
+		err = admit.Admit(admission.NewAttributesRecord(obj, namespace, resource, "UPDATE"))
+		if err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+
+		ctx := ctxFn(req)
+		ctx = api.WithNamespace(ctx, namespace)
+
+		original, err := r.Get(ctx, name)
+		if err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+
+		originalObjJs, err := codec.Encode(original)
+		if err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+		patchJs, err := readBody(req.Request)
+		if err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+		patchedObjJs, err := jsonpatch.MergePatch(originalObjJs, patchJs)
+		if err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+
+		if err := codec.DecodeInto(patchedObjJs, obj); err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+		if err := checkName(obj, name, namespace, namer); err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+
+		result, err := finishRequest(timeout, func() (runtime.Object, error) {
+			// update should never create as previous get would fail
+			obj, _, err := r.Update(ctx, obj)
+			return obj, err
+		})
+		if err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+
+		if err := setSelfLink(result, req, namer); err != nil {
+			errorJSON(err, codec, w)
+			return
+		}
+
+		writeJSON(http.StatusOK, codec, result, w)
+	}
+}
+
 // UpdateResource returns a function that will handle a resource update
 func UpdateResource(r RESTUpdater, ctxFn ContextFunc, namer ScopeNamer, codec runtime.Codec, typer runtime.ObjectTyper, resource string, admit admission.Interface) restful.RouteFunction {
 	return func(req *restful.Request, res *restful.Response) {
@@ -216,18 +294,9 @@ func UpdateResource(r RESTUpdater, ctxFn ContextFunc, namer ScopeNamer, codec ru
 			return
 		}
 
-		// check the provided name against the request
-		if objNamespace, objName, err := namer.ObjectName(obj); err == nil {
-			if objName != name {
-				errorJSON(errors.NewBadRequest("the name of the object does not match the name on the URL"), codec, w)
-				return
-			}
-			if len(namespace) > 0 {
-				if len(objNamespace) > 0 && objNamespace != namespace {
-					errorJSON(errors.NewBadRequest("the namespace of the object does not match the namespace on the request"), codec, w)
-					return
-				}
-			}
+		if err := checkName(obj, name, namespace, namer); err != nil {
+			errorJSON(err, codec, w)
+			return
 		}
 
 		err = admit.Admit(admission.NewAttributesRecord(obj, namespace, resource, "UPDATE"))
@@ -378,6 +447,21 @@ func setSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer) err
 	return namer.SetSelfLink(obj, newURL.String())
 }
 
+// checkName checks the provided name against the request
+func checkName(obj runtime.Object, name, namespace string, namer ScopeNamer) error {
+	if objNamespace, objName, err := namer.ObjectName(obj); err == nil {
+		if objName != name {
+			return errors.NewBadRequest("the name of the object does not match the name on the URL")
+		}
+		if len(namespace) > 0 {
+			if len(objNamespace) > 0 && objNamespace != namespace {
+				return errors.NewBadRequest("the namespace of the object does not match the namespace on the request")
+			}
+		}
+	}
+	return nil
+}
+
 // setListSelfLink sets the self link of a list to the base URL, then sets the self links
 // on all child objects returned.
 func setListSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer) error {

diff --git a/pkg/client/restclient.go b/pkg/client/restclient.go
@@ -105,6 +105,11 @@ func (c *RESTClient) Put() *Request {
 	return c.Verb("PUT")
 }
 
+// Patch begins a PATCH request. Short for c.Verb("Patch").
+func (c *RESTClient) Patch() *Request {
+	return c.Verb("PATCH")
+}
+
 // Get begins a GET request. Short for c.Verb("GET").
 func (c *RESTClient) Get() *Request {
 	return c.Verb("GET")

diff --git a/test/integration/auth_test.go b/test/integration/auth_test.go
@@ -202,12 +202,12 @@ func getTestRequests() []struct {
 		{"PUT", "/api/v1beta1/pods/a" + timeoutFlag, aPod, code200},
 		{"GET", "/api/v1beta1/pods", "", code200},
 		{"GET", "/api/v1beta1/pods/a", "", code200},
+		{"PATCH", "/api/v1beta1/pods/a", "{%v}", code200},
 		{"DELETE", "/api/v1beta1/pods/a" + timeoutFlag, "", code200},
 
 		// Non-standard methods (not expected to work,
 		// but expected to pass/fail authorization prior to
 		// failing validation.
-		{"PATCH", "/api/v1beta1/pods/a", "", code405},
 		{"OPTIONS", "/api/v1beta1/pods", "", code405},
 		{"OPTIONS", "/api/v1beta1/pods/a", "", code405},
 		{"HEAD", "/api/v1beta1/pods", "", code405},