diff --git a/pkg/kubectl/cmd/expose.go b/pkg/kubectl/cmd/expose.go index f44236cc53a..8c0f7e82913 100644 --- a/pkg/kubectl/cmd/expose.go +++ b/pkg/kubectl/cmd/expose.go @@ -85,7 +85,8 @@ Examples: inline := util.GetFlagString(cmd, "overrides") if len(inline) > 0 { - util.Merge(service, inline, "Service") + service, err = util.Merge(service, inline, "Service") + checkErr(err) } // TODO: extract this flag to a central location, when such a location exists. diff --git a/pkg/kubectl/cmd/run.go b/pkg/kubectl/cmd/run.go index 09219db869d..974471b802e 100644 --- a/pkg/kubectl/cmd/run.go +++ b/pkg/kubectl/cmd/run.go @@ -74,7 +74,8 @@ Examples: inline := util.GetFlagString(cmd, "overrides") if len(inline) > 0 { - util.Merge(controller, inline, "ReplicationController") + controller, err = util.Merge(controller, inline, "ReplicationController") + checkErr(err) } // TODO: extract this flag to a central location, when such a location exists. diff --git a/pkg/kubectl/cmd/update.go b/pkg/kubectl/cmd/update.go index 86a19b051df..5b43e5260ef 100644 --- a/pkg/kubectl/cmd/update.go +++ b/pkg/kubectl/cmd/update.go @@ -116,12 +116,13 @@ func updateWithPatch(cmd *cobra.Command, args []string, f *Factory, patch string obj, err := helper.Get(namespace, name) checkErr(err) - cmdutil.Merge(obj, patch, mapping.Kind) - - data, err := helper.Codec.Encode(obj) + patchedObj, err := cmdutil.Merge(obj, patch, mapping.Kind) checkErr(err) - obj, err = helper.Update(namespace, name, true, data) + data, err := helper.Codec.Encode(patchedObj) + checkErr(err) + + _, err = helper.Update(namespace, name, true, data) checkErr(err) return name } diff --git a/pkg/kubectl/cmd/util/helpers.go b/pkg/kubectl/cmd/util/helpers.go index ca5d9d2b5ad..a2c2b485497 100644 --- a/pkg/kubectl/cmd/util/helpers.go +++ b/pkg/kubectl/cmd/util/helpers.go @@ -29,8 +29,8 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util/merge" "github.com/golang/glog" - "github.com/imdario/mergo" "github.com/spf13/cobra" ) @@ -188,36 +188,40 @@ func ReadConfigDataFromLocation(location string) ([]byte, error) { } } -func Merge(dst runtime.Object, fragment, kind string) error { +func Merge(dst runtime.Object, fragment, kind string) (runtime.Object, error) { // Ok, this is a little hairy, we'd rather not force the user to specify a kind for their JSON // So we pull it into a map, add the Kind field, and then reserialize. // We also pull the apiVersion for proper parsing var intermediate interface{} if err := json.Unmarshal([]byte(fragment), &intermediate); err != nil { - return err + return nil, err } dataMap, ok := intermediate.(map[string]interface{}) if !ok { - return fmt.Errorf("Expected a map, found something else: %s", fragment) + return nil, fmt.Errorf("Expected a map, found something else: %s", fragment) } version, found := dataMap["apiVersion"] if !found { - return fmt.Errorf("Inline JSON requires an apiVersion field") + return nil, fmt.Errorf("Inline JSON requires an apiVersion field") } versionString, ok := version.(string) if !ok { - return fmt.Errorf("apiVersion must be a string") + return nil, fmt.Errorf("apiVersion must be a string") } - codec := runtime.CodecFor(api.Scheme, versionString) - dataMap["kind"] = kind - data, err := json.Marshal(intermediate) + codec := runtime.CodecFor(api.Scheme, versionString) + // encode dst into versioned json and apply fragment directly too it + target, err := codec.Encode(dst) if err != nil { - return err + return nil, err } - src, err := codec.Decode(data) + patched, err := merge.MergeJSON(target, []byte(fragment)) if err != nil { - return err + return nil, err } - return mergo.Merge(dst, src) + out, err := codec.Decode(patched) + if err != nil { + return nil, err + } + return out, nil } diff --git a/pkg/kubectl/cmd/util/helpers_test.go b/pkg/kubectl/cmd/util/helpers_test.go index 32673373c74..751f4712dc1 100644 --- a/pkg/kubectl/cmd/util/helpers_test.go +++ b/pkg/kubectl/cmd/util/helpers_test.go @@ -30,14 +30,16 @@ func TestMerge(t *testing.T) { fragment string expected runtime.Object expectErr bool + kind string }{ { + kind: "Pod", obj: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", }, }, - fragment: "{ \"apiVersion\": \"v1beta1\" }", + fragment: `{ "apiVersion": "v1beta1" }`, expected: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", @@ -51,12 +53,13 @@ func TestMerge(t *testing.T) { }, }, { + kind: "Pod", obj: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", }, }, - fragment: "{ \"apiVersion\": \"v1beta1\", \"id\": \"baz\", \"desiredState\": { \"host\": \"bar\" } }", + fragment: `{ "apiVersion": "v1beta1", "id": "baz", "desiredState": { "host": "bar" } }`, expected: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "baz", @@ -71,12 +74,13 @@ func TestMerge(t *testing.T) { }, }, { + kind: "Pod", obj: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", }, }, - fragment: "{ \"apiVersion\": \"v1beta3\", \"spec\": { \"volumes\": [ {\"name\": \"v1\"}, {\"name\": \"v2\"} ] } }", + fragment: `{ "apiVersion": "v1beta3", "spec": { "volumes": [ {"name": "v1"}, {"name": "v2"} ] } }`, expected: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", @@ -100,24 +104,81 @@ func TestMerge(t *testing.T) { }, }, { + kind: "Pod", obj: &api.Pod{}, fragment: "invalid json", expected: &api.Pod{}, expectErr: true, }, + { + kind: "Pod", + obj: &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + }, + }, + fragment: `{ "apiVersion": "v1beta1", "id": null}`, + expected: &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "", + }, + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicy{ + Always: &api.RestartPolicyAlways{}, + }, + DNSPolicy: api.DNSClusterFirst, + }, + }, + }, + { + kind: "Service", + obj: &api.Service{ + Spec: api.ServiceSpec{ + Port: 10, + }, + }, + fragment: `{ "apiVersion": "v1beta1", "port": 0 }`, + expected: &api.Service{ + Spec: api.ServiceSpec{ + Port: 0, + Protocol: "TCP", + SessionAffinity: "None", + }, + }, + }, + { + kind: "Service", + obj: &api.Service{ + Spec: api.ServiceSpec{ + Selector: map[string]string{ + "version": "v1", + }, + }, + }, + fragment: `{ "apiVersion": "v1beta1", "selector": { "version": "v2" } }`, + expected: &api.Service{ + Spec: api.ServiceSpec{ + Protocol: "TCP", + SessionAffinity: "None", + Selector: map[string]string{ + "version": "v2", + }, + }, + }, + }, } for i, test := range tests { - err := Merge(test.obj, test.fragment, "Pod") + out, err := Merge(test.obj, test.fragment, test.kind) if !test.expectErr { if err != nil { - t.Errorf("unexpected error: %v", err) - } else if !reflect.DeepEqual(test.obj, test.expected) { - t.Errorf("\n\ntestcase[%d]\nexpected:\n%v\nsaw:\n%v", i, test.expected, test.obj) + t.Errorf("testcase[%d], unexpected error: %v", i, err) + } else if !reflect.DeepEqual(out, test.expected) { + t.Errorf("\n\ntestcase[%d]\nexpected:\n%+v\nsaw:\n%+v", i, test.expected, out) } } if test.expectErr && err == nil { - t.Errorf("unexpected non-error") + t.Errorf("testcase[%d], unexpected non-error", i) } } diff --git a/pkg/util/merge/merge.go b/pkg/util/merge/merge.go new file mode 100644 index 00000000000..8b1cbf3c1f3 --- /dev/null +++ b/pkg/util/merge/merge.go @@ -0,0 +1,57 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed 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 merge + +import ( + "encoding/json" +) + +// MergeJSON merges JSON according to RFC7386 +// (see https://tools.ietf.org/html/rfc7386) +func MergeJSON(dst, src []byte) ([]byte, error) { + var target interface{} + if err := json.Unmarshal(dst, &target); err != nil { + return nil, err + } + var patch interface{} + if err := json.Unmarshal(src, &patch); err != nil { + return nil, err + } + return json.Marshal(MergePatch(target, patch)) +} + +// MergePatch is an implementation of MergePatch described in RFC7386 that operates on +// json marshalled into empty interface{} by encoding/json.Unmarshal() +// (see https://tools.ietf.org/html/rfc7386#section-2) +func MergePatch(target, patch interface{}) interface{} { + if patchObject, isPatchObject := patch.(map[string]interface{}); isPatchObject { + targetObject := make(map[string]interface{}) + if m, isTargetObject := target.(map[string]interface{}); isTargetObject { + targetObject = m + } + for name, value := range patchObject { + if _, found := targetObject[name]; value == nil && found { + delete(targetObject, name) + } else { + targetObject[name] = MergePatch(targetObject[name], value) + } + } + return targetObject + } else { + return patch + } +} diff --git a/pkg/util/merge/merge_test.go b/pkg/util/merge/merge_test.go new file mode 100644 index 00000000000..eb3118bcabf --- /dev/null +++ b/pkg/util/merge/merge_test.go @@ -0,0 +1,68 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed 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 merge + +import ( + "testing" +) + +func TestMergeJSON(t *testing.T) { + tests := []struct { + target string + patch string + expected string + }{ + {target: `{}`, patch: `{}`, expected: `{}`}, + {target: `{"a":"b","c":{"d":"e","f":"g"}}`, patch: `{}`, expected: `{"a":"b","c":{"d":"e","f":"g"}}`}, + //update + {target: `{"a":"b","c":"d"}`, patch: `{"a":"z"}`, expected: `{"a":"z","c":"d"}`}, + //remove key + {target: `{"f":"g"}`, patch: `{"f":null}`, expected: `{}`}, + //inner update + {target: `{"c":{"d":"e","f":"g"}}`, patch: `{"c":{"f":"z"}}`, expected: `{"c":{"d":"e","f":"z"}}`}, + //inner remove + {target: `{"c":{"d":"e","f":"g"}}`, patch: `{"c":{"f":null}}`, expected: `{"c":{"d":"e"}}`}, + //complex update and remove + {target: `{"a":"b","c":{"d":"e","f":"g"}}`, patch: `{"a":"z","c":{"f":null}}`, expected: `{"a":"z","c":{"d":"e"}}`}, + // test cases from https://tools.ietf.org/html/rfc7386#appendix-A slightly adapted to correspond to go's + // encoding/json conventions + {target: `{"a":"b"}`, patch: `{"a":"c"}`, expected: `{"a":"c"}`}, + {target: `{"a":"b"}`, patch: `{"b":"c"}`, expected: `{"a":"b","b":"c"}`}, + {target: `{"a":"b"}`, patch: `{"a":null}`, expected: `{}`}, + {target: `{"a":"b","b":"c"}`, patch: `{"a":null}`, expected: `{"b":"c"}`}, + {target: `{"a":["b"]}`, patch: `{"a":"c"}`, expected: `{"a":"c"}`}, + {target: `{"a":"c"}`, patch: `{"a":["b"]}`, expected: `{"a":["b"]}`}, + {target: `{"a":{"b": "c"}}`, patch: `{"a": {"b": "d","c": null}}`, expected: `{"a":{"b":"d","c":null}}`}, + {target: `{"a":[{"b":"c"}]}`, patch: `{"a":[1]}`, expected: `{"a":[1]}`}, + {target: `["a","b"]`, patch: `["c","d"]`, expected: `["c","d"]`}, + {target: `{"a":"b"}`, patch: `["c"]`, expected: `["c"]`}, + {target: `{"a":"foo"}`, patch: `null`, expected: `null`}, + {target: `{"a":"foo"}`, patch: `"bar"`, expected: `"bar"`}, + {target: `{"e":null}`, patch: `{"a":1}`, expected: `{"a":1,"e":null}`}, + {target: `[1,2]`, patch: `{"a":"b","c":null}`, expected: `{"a":"b","c":null}`}, + {target: `{}`, patch: `{"a":{"bb":{"ccc":null}}}`, expected: `{"a":{"bb":{"ccc":null}}}`}, + } + for i, test := range tests { + out, err := MergeJSON([]byte(test.target), []byte(test.patch)) + if err != nil { + t.Errorf("case %v, unexpected error: %v", i, err) + } + if string(out) != test.expected { + t.Errorf("case %v, expected:\n%v\nsaw:\n%v\n", i, test.expected, string(out)) + } + } +}