Fix --override and --patch flags in kubectl

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.

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.

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)
+	patchedObj, err := cmdutil.Merge(obj, patch, mapping.Kind)
+	checkErr(err)
 
-	data, err := helper.Codec.Encode(obj)
+	data, err := helper.Codec.Encode(patchedObj)
 	checkErr(err)
 
-	obj, err = helper.Update(namespace, name, true, data)
+	_, err = helper.Update(namespace, name, true, data)
 	checkErr(err)
 	return name
 }

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
+	}
+	out, err := codec.Decode(patched)
+	if err != nil {
+		return nil, err
 	}
-	return mergo.Merge(dst, src)
+	return out, nil
 }

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)
 		}
 	}
 

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
+	}
+}

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))
+		}
+	}
+}