From c756b1c6579f7caed88ea57498a12598f2f37ec2 Mon Sep 17 00:00:00 2001 From: nic-chen <33000667+nic-chen@users.noreply.github.com> Date: Fri, 11 Dec 2020 16:08:53 +0800 Subject: [PATCH] fix: PATCH method bug (#1005) * fix: PATCH method bug * test: use sub path patch in e2e test * fix: lint * fix: naming stype * fix: according to review * fix: style --- api/go.mod | 1 + api/go.sum | 4 + api/internal/handler/ssl/ssl.go | 42 ++--- api/internal/utils/json_patch.go | 75 +++++++++ api/internal/utils/json_patch_test.go | 211 ++++++++++++++++++++++++++ api/test/e2e/ssl_test.go | 56 ++++++- 6 files changed, 358 insertions(+), 31 deletions(-) create mode 100644 api/internal/utils/json_patch.go create mode 100644 api/internal/utils/json_patch_test.go diff --git a/api/go.mod b/api/go.mod index 3c483718f7..b67cd955b1 100644 --- a/api/go.mod +++ b/api/go.mod @@ -11,6 +11,7 @@ require ( github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf // indirect github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f // indirect github.com/dgrijalva/jwt-go v3.2.0+incompatible + github.com/evanphx/json-patch/v5 v5.1.0 github.com/gin-contrib/pprof v1.3.0 github.com/gin-contrib/sessions v0.0.3 github.com/gin-contrib/static v0.0.0-20200916080430-d45d9a37d28e diff --git a/api/go.sum b/api/go.sum index 80c6f95448..67988630eb 100644 --- a/api/go.sum +++ b/api/go.sum @@ -22,6 +22,9 @@ github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZm github.com/elazarl/go-bindata-assetfs v1.0.0/go.mod h1:v+YaWX3bdea5J/mo8dSETolEo7R71Vk1u8bnjau5yw4= github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= +github.com/evanphx/json-patch v4.9.0+incompatible h1:kLcOMZeuLAJvL2BPWLMIj5oaZQobrkAqrL+WFZwQses= +github.com/evanphx/json-patch/v5 v5.1.0 h1:B0aXl1o/1cP8NbviYiBMkcHBtUjIJ1/Ccg6b+SwCLQg= +github.com/evanphx/json-patch/v5 v5.1.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/gin-contrib/pprof v1.3.0 h1:G9eK6HnbkSqDZBYbzG4wrjCsA4e+cvYAHUZw6W+W9K0= github.com/gin-contrib/pprof v1.3.0/go.mod h1:waMjT1H9b179t3CxuG1cV3DHpga6ybizwfBaM5OXaB0= github.com/gin-contrib/sessions v0.0.3 h1:PoBXki+44XdJdlgDqDrY5nDVe3Wk7wDV/UCOuLP6fBI= @@ -77,6 +80,7 @@ github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+ github.com/gorilla/sessions v1.1.1/go.mod h1:8KCfur6+4Mqcc6S0FEfKuN15Vl5MgXW92AE8ovaJD0w= github.com/gorilla/sessions v1.1.3 h1:uXoZdcdA5XdXF3QzuSlheVRUvjl+1rKY7zBXL68L9RU= github.com/gorilla/sessions v1.1.3/go.mod h1:8KCfur6+4Mqcc6S0FEfKuN15Vl5MgXW92AE8ovaJD0w= +github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= github.com/json-iterator/go v1.1.7/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4= github.com/json-iterator/go v1.1.9/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4= github.com/json-iterator/go v1.1.10/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4= diff --git a/api/internal/handler/ssl/ssl.go b/api/internal/handler/ssl/ssl.go index b49ecea203..19009954f8 100644 --- a/api/internal/handler/ssl/ssl.go +++ b/api/internal/handler/ssl/ssl.go @@ -27,7 +27,6 @@ import ( "reflect" "strings" - "github.com/api7/go-jsonpatch" "github.com/gin-gonic/gin" "github.com/shiningrush/droplet" "github.com/shiningrush/droplet/data" @@ -38,6 +37,7 @@ import ( "github.com/apisix/manager-api/internal/core/entity" "github.com/apisix/manager-api/internal/core/store" "github.com/apisix/manager-api/internal/handler" + "github.com/apisix/manager-api/internal/utils" "github.com/apisix/manager-api/internal/utils/consts" ) @@ -62,13 +62,14 @@ func (h *Handler) ApplyRoute(r *gin.Engine) { wrapper.InputType(reflect.TypeOf(UpdateInput{})))) r.PUT("/apisix/admin/ssl/:id", wgin.Wraps(h.Update, wrapper.InputType(reflect.TypeOf(UpdateInput{})))) - r.PATCH("/apisix/admin/ssl/:id", wgin.Wraps(h.Patch, - wrapper.InputType(reflect.TypeOf(UpdateInput{})))) r.DELETE("/apisix/admin/ssl/:ids", wgin.Wraps(h.BatchDelete, wrapper.InputType(reflect.TypeOf(BatchDelete{})))) r.POST("/apisix/admin/check_ssl_cert", wgin.Wraps(h.Validate, wrapper.InputType(reflect.TypeOf(entity.SSL{})))) + r.PATCH("/apisix/admin/ssl/:id", consts.ErrorWrapper(Patch)) + r.PATCH("/apisix/admin/ssl/:id/*path", consts.ErrorWrapper(Patch)) + r.POST("/apisix/admin/check_ssl_exists", consts.ErrorWrapper(Exist)) } @@ -216,40 +217,29 @@ func (h *Handler) Update(c droplet.Context) (interface{}, error) { return nil, nil } -func (h *Handler) Patch(c droplet.Context) (interface{}, error) { - input := c.Input().(*UpdateInput) - arr := strings.Split(input.ID, "/") - var subPath string - if len(arr) > 1 { - input.ID = arr[0] - subPath = arr[1] - } +func Patch(c *gin.Context) (interface{}, error) { + reqBody, _ := c.GetRawData() + ID := c.Param("id") + subPath := c.Param("path") - stored, err := h.sslStore.Get(input.ID) + sslStore := store.GetStore(store.HubKeySsl) + stored, err := sslStore.Get(ID) if err != nil { return handler.SpecCodeResponse(err), err } - var patch jsonpatch.Patch - if subPath != "" { - patch = jsonpatch.Patch{ - Operations: []jsonpatch.PatchOperation{ - {Op: jsonpatch.Replace, Path: subPath, Value: c.Input()}, - }, - } - } else { - patch, err = jsonpatch.MakePatch(stored, input.SSL) - if err != nil { - return handler.SpecCodeResponse(err), err - } + res, err := utils.MergePatch(stored, subPath, reqBody) + if err != nil { + return handler.SpecCodeResponse(err), err } - err = patch.Apply(&stored) + var ssl entity.SSL + err = json.Unmarshal(res, &ssl) if err != nil { return handler.SpecCodeResponse(err), err } - if err := h.sslStore.Update(c.Context(), &stored, false); err != nil { + if err := sslStore.Update(c, &ssl, false); err != nil { return handler.SpecCodeResponse(err), err } diff --git a/api/internal/utils/json_patch.go b/api/internal/utils/json_patch.go new file mode 100644 index 0000000000..d6bcd1d285 --- /dev/null +++ b/api/internal/utils/json_patch.go @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package utils + +import ( + "encoding/json" + jsonpatch "github.com/evanphx/json-patch/v5" +) + +func MergeJson(doc, patch []byte) ([]byte, error) { + out, err := jsonpatch.MergePatch(doc, patch) + + if err != nil { + return nil, err + } + return out, nil +} + +func PatchJson(doc []byte, path, val string) ([]byte, error) { + patch := []byte(`[ { "op": "replace", "path": "` + path + `", "value": ` + val + `}]`) + obj, err := jsonpatch.DecodePatch(patch) + if err != nil { + return nil, err + } + + out, err := obj.Apply(doc) + + if err != nil { + // try to add if field not exist + patch = []byte(`[ { "op": "add", "path": "` + path + `", "value": ` + val + `}]`) + obj, err = jsonpatch.DecodePatch(patch) + if err != nil { + return nil, err + } + out, err = obj.Apply(doc) + if err != nil { + return nil, err + } + } + + return out, nil +} + +func MergePatch(obj interface{}, subPath string, reqBody []byte) ([]byte, error) { + var res []byte + jsonBytes, err := json.Marshal(obj) + if err != nil { + return res, err + } + + if subPath != "" { + res, err = PatchJson(jsonBytes, subPath, string(reqBody)) + } else { + res, err = MergeJson(jsonBytes, reqBody) + } + + if err != nil { + return res, err + } + return res, nil +} diff --git a/api/internal/utils/json_patch_test.go b/api/internal/utils/json_patch_test.go new file mode 100644 index 0000000000..8dd25404e1 --- /dev/null +++ b/api/internal/utils/json_patch_test.go @@ -0,0 +1,211 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package utils + +import ( + "bytes" + "encoding/json" + "reflect" + "testing" +) + +func compareJSON(a, b string) bool { + var objA, objB interface{} + json.Unmarshal([]byte(a), &objA) + json.Unmarshal([]byte(b), &objB) + + return reflect.DeepEqual(objA, objB) +} + +func formatJSON(j string) string { + buf := new(bytes.Buffer) + + json.Indent(buf, []byte(j), "", " ") + + return buf.String() +} + +func TestMergeJson(t *testing.T) { + cases := []struct { + doc, patch, result, desc string + }{ + { + desc: "simple merge", + doc: `{ + "id": "1", + "status": 1, + "key": "fake key", + "cert": "fake cert", + "create_time": 1, + "update_time": 2 + }`, + patch: `{ + "id": "1", + "status": 0, + "key": "fake key1", + "cert": "fake cert1" + }`, + result: `{ + "id": "1", + "status": 0, + "key": "fake key1", + "cert": "fake cert1", + "create_time": 1, + "update_time": 2 + }`, + }, + { + desc: `array merge`, + doc: `{ + "uri": "/index.html", + "upstream": { + "type": "roundrobin", + "nodes": [{ + "host": "39.97.63.215", + "port": 80, + "weight" : 1 + }] + } + }`, + patch: `{ + "upstream": { + "nodes": [{ + "host": "39.97.63.216", + "port": 80, + "weight" : 1 + },{ + "host": "39.97.63.217", + "port": 80, + "weight" : 1 + }] + } + }`, + result: `{ + "uri": "/index.html", + "upstream": { + "type": "roundrobin", + "nodes": [{ + "host": "39.97.63.216", + "port": 80, + "weight" : 1 + },{ + "host": "39.97.63.217", + "port": 80, + "weight" : 1 + }] + } + }`, + }, + } + for _, c := range cases { + out, err := MergeJson([]byte(c.doc), []byte(c.patch)) + + if err != nil { + t.Errorf("Unable to merge patch: %s", err) + } + + if !compareJSON(string(out), c.result) { + t.Errorf("Merge failed. Expected:\n%s\n\nActual:\n%s", + formatJSON(c.result), formatJSON(string(out))) + } + } +} + +func TestPatchJson(t *testing.T) { + cases := []struct { + doc, path, value, result, desc string + }{ + { + desc: "patch array", + doc: `{ + "uri": "/index.html", + "upstream": { + "type": "roundrobin", + "nodes": [{ + "host": "39.97.63.215", + "port": 80, + "weight" : 1 + }] + } + }`, + path: `/upstream/nodes`, + value: `[{ + "host": "39.97.63.216", + "port": 80, + "weight" : 1 + },{ + "host": "39.97.63.217", + "port": 80, + "weight" : 1 + }]`, + result: `{ + "uri": "/index.html", + "upstream": { + "type": "roundrobin", + "nodes": [{ + "host": "39.97.63.216", + "port": 80, + "weight" : 1 + },{ + "host": "39.97.63.217", + "port": 80, + "weight" : 1 + }] + } + }`, + }, + { + desc: "patch field that non existent", + doc: `{ + "uri": "/index.html", + "upstream": { + "type": "roundrobin", + "nodes": [{ + "host": "39.97.63.215", + "port": 80, + "weight" : 1 + }] + } + }`, + path: `/upstream/labels`, + value: `{"app": "test"}`, + result: `{ + "uri": "/index.html", + "upstream": { + "type": "roundrobin", + "nodes": [{ + "host": "39.97.63.215", + "port": 80, + "weight" : 1 + }], + "labels": {"app": "test"} + } + }`, + }, + } + for _, c := range cases { + out, err := PatchJson([]byte(c.doc), c.path, c.value) + if err != nil { + t.Errorf("Unable to patch: %s", err) + } + + if !compareJSON(string(out), c.result) { + t.Errorf("Patch failed. Expected:\n%s\n\nActual:\n%s", + formatJSON(c.result), formatJSON(string(out))) + } + } +} diff --git a/api/test/e2e/ssl_test.go b/api/test/e2e/ssl_test.go index 85a332e36b..5290fa3288 100644 --- a/api/test/e2e/ssl_test.go +++ b/api/test/e2e/ssl_test.go @@ -112,10 +112,13 @@ func TestSSL_Basic(t *testing.T) { Sleep: sleepTime, }, { - caseDesc: "delete ssl", - Object: ManagerApiExpect(t), - Method: http.MethodDelete, - Path: "/apisix/admin/ssl/1", + caseDesc: "disable SSL", + Object: ManagerApiExpect(t), + Method: http.MethodPatch, + Path: "/apisix/admin/ssl/1", + Body: `{ + "status": 0 + }`, Headers: map[string]string{"Authorization": token}, ExpectStatus: http.StatusOK, }, @@ -125,6 +128,50 @@ func TestSSL_Basic(t *testing.T) { testCaseCheck(tc) } + // try again after disable SSL, make a HTTPS request + // If use the test framework, errors will cause failure, so we need to make a separate https request for testing. + time.Sleep(sleepTime) + _, err = http.Get("https://www.test2.com:9443") + assert.NotNil(t, err) + assert.EqualError(t, err, "Get https://www.test2.com:9443: remote error: tls: internal error") + + // enable SSL again + tests = []HttpTestCase{ + { + caseDesc: "enable SSL", + Object: ManagerApiExpect(t), + Method: http.MethodPatch, + Path: "/apisix/admin/ssl/1/status", + Body: `1`, + Headers: map[string]string{"Authorization": token}, + ExpectStatus: http.StatusOK, + }, + { + caseDesc: "hit the route using HTTPS, make sure enable successful", + Object: APISIXHTTPSExpect(t), + Method: http.MethodGet, + Path: "/hello_", + Headers: map[string]string{"Host": "www.test2.com"}, + ExpectStatus: http.StatusOK, + ExpectBody: "hello world\n", + Sleep: sleepTime, + }, + } + for _, tc := range tests { + testCaseCheck(tc) + } + + // delete SSL + delSSL := HttpTestCase{ + caseDesc: "delete SSL", + Object: ManagerApiExpect(t), + Method: http.MethodDelete, + Path: "/apisix/admin/ssl/1", + Headers: map[string]string{"Authorization": token}, + ExpectStatus: http.StatusOK, + } + testCaseCheck(delSSL) + // try again after deleting SSL, make a HTTPS request // If use the test framework, errors will cause failure, so we need to make a separate https request for testing. time.Sleep(sleepTime) @@ -142,5 +189,4 @@ func TestSSL_Basic(t *testing.T) { ExpectStatus: http.StatusOK, } testCaseCheck(delRoute) - }