Use versioned object when recording changes using --record

kubernetes · Nov 4, 2016 · 86e6fd6 · 86e6fd6
1 parent a05e46f
commit 86e6fd6
Show file tree

Hide file tree

Showing 12 changed files with 132 additions and 32 deletions.
diff --git a/pkg/kubectl/cmd/apply.go b/pkg/kubectl/cmd/apply.go
@@ -254,7 +254,7 @@ func RunApply(f cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *App
 			}
 
 			if cmdutil.ShouldRecord(cmd, info) {
-				patch, err := cmdutil.ChangeResourcePatch(info, f.Command())
+				patch, err := cmdutil.ChangeResourcePatch(info, typer, encoder, f.Command())
 				if err != nil {
 					return err
 				}

diff --git a/pkg/kubectl/cmd/convert.go b/pkg/kubectl/cmd/convert.go
@@ -93,6 +93,7 @@ type ConvertOptions struct {
 	local   bool
 
 	encoder runtime.Encoder
+	typer   runtime.ObjectTyper
 	out     io.Writer
 	printer kubectl.ResourcePrinter
 
@@ -111,13 +112,14 @@ func (o *ConvertOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.C
 
 	// build the builder
 	mapper, typer := f.Object()
+	o.typer = typer
 	clientMapper := resource.ClientMapperFunc(f.ClientForMapping)
 
 	if o.local {
 		fmt.Fprintln(os.Stderr, "running in local mode...")
-		o.builder = resource.NewBuilder(mapper, typer, resource.DisabledClientForMapping{ClientMapper: clientMapper}, f.Decoder(true))
+		o.builder = resource.NewBuilder(mapper, o.typer, resource.DisabledClientForMapping{ClientMapper: clientMapper}, f.Decoder(true))
 	} else {
-		o.builder = resource.NewBuilder(mapper, typer, clientMapper, f.Decoder(true))
+		o.builder = resource.NewBuilder(mapper, o.typer, clientMapper, f.Decoder(true))
 		schema, err := f.Validator(cmdutil.GetFlagBool(cmd, "validate"), cmdutil.GetFlagString(cmd, "schema-cache-dir"))
 		if err != nil {
 			return err
@@ -168,7 +170,7 @@ func (o *ConvertOptions) RunConvert() error {
 		}
 
 		infos := []*resource.Info{info}
-		objects, err := resource.AsVersionedObject(infos, false, o.outputVersion, o.encoder)
+		objects, err := resource.AsVersionedObject(infos, false, o.outputVersion, o.typer, o.encoder)
 		if err != nil {
 			return err
 		}

diff --git a/pkg/kubectl/cmd/edit.go b/pkg/kubectl/cmd/edit.go
@@ -127,7 +127,7 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args
 		return err
 	}
 
-	mapper, resourceMapper, r, cmdNamespace, err := getMapperAndResult(f, args, options, editMode)
+	mapper, resourceMapper, typer, r, cmdNamespace, err := getMapperAndResult(f, args, options, editMode)
 	if err != nil {
 		return err
 	}
@@ -162,7 +162,7 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args
 
 	containsError := false
 	for {
-		originalObj, err := resource.AsVersionedObject(infos, false, defaultVersion, encoder)
+		originalObj, err := resource.AsVersionedObject(infos, false, defaultVersion, typer, encoder)
 		if err != nil {
 			return err
 		}
@@ -340,10 +340,10 @@ func getPrinter(cmd *cobra.Command) (*editPrinterOptions, error) {
 	}
 }
 
-func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.FilenameOptions, editMode EditMode) (meta.RESTMapper, *resource.Mapper, *resource.Result, string, error) {
+func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.FilenameOptions, editMode EditMode) (meta.RESTMapper, *resource.Mapper, runtime.ObjectTyper, *resource.Result, string, error) {
 	cmdNamespace, enforceNamespace, err := f.DefaultNamespace()
 	if err != nil {
-		return nil, nil, nil, "", err
+		return nil, nil, nil, nil, "", err
 	}
 	var mapper meta.RESTMapper
 	var typer runtime.ObjectTyper
@@ -353,10 +353,10 @@ func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.File
 	case EditBeforeCreateMode:
 		mapper, typer, err = f.UnstructuredObject()
 	default:
-		return nil, nil, nil, "", fmt.Errorf("Not supported edit mode %q", editMode)
+		return nil, nil, nil, nil, "", fmt.Errorf("Not supported edit mode %q", editMode)
 	}
 	if err != nil {
-		return nil, nil, nil, "", err
+		return nil, nil, nil, nil, "", err
 	}
 	resourceMapper := &resource.Mapper{
 		ObjectTyper:  typer,
@@ -379,7 +379,7 @@ func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.File
 	case EditBeforeCreateMode:
 		b = resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.UnstructuredClientForMapping), runtime.UnstructuredJSONScheme)
 	default:
-		return nil, nil, nil, "", fmt.Errorf("Not supported edit mode %q", editMode)
+		return nil, nil, nil, nil, "", fmt.Errorf("Not supported edit mode %q", editMode)
 	}
 	r := b.NamespaceParam(cmdNamespace).DefaultNamespace().
 		FilenameParam(enforceNamespace, options).
@@ -388,9 +388,9 @@ func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.File
 		Do()
 	err = r.Err()
 	if err != nil {
-		return nil, nil, nil, "", err
+		return nil, nil, nil, nil, "", err
 	}
-	return mapper, resourceMapper, r, cmdNamespace, err
+	return mapper, resourceMapper, typer, r, cmdNamespace, err
 }
 
 func visitToPatch(originalObj runtime.Object, updates *resource.Info, mapper meta.RESTMapper, resourceMapper *resource.Mapper, encoder runtime.Encoder, out, errOut io.Writer, defaultVersion unversioned.GroupVersion, results *editResults, file string) error {

diff --git a/pkg/kubectl/cmd/get.go b/pkg/kubectl/cmd/get.go
@@ -39,7 +39,8 @@ import (
 type GetOptions struct {
 	resource.FilenameOptions
 
-	Raw string
+	Typer runtime.ObjectTyper
+	Raw   string
 }
 
 var (
@@ -84,7 +85,6 @@ var (
 // retrieves one or more resources from a server.
 func NewCmdGet(f cmdutil.Factory, out io.Writer, errOut io.Writer) *cobra.Command {
 	options := &GetOptions{}
-
 	// retrieve a list of handled resources from printer as valid args
 	validArgs, argAliases := []string{}, []string{}
 	p, err := f.Printer(nil, kubectl.PrintOptions{
@@ -150,6 +150,9 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 	allNamespaces := cmdutil.GetFlagBool(cmd, "all-namespaces")
 	showKind := cmdutil.GetFlagBool(cmd, "show-kind")
 	mapper, typer := f.Object()
+	if options.Typer != nil {
+		typer = options.Typer
+	}
 	printAll := false
 	filterFuncs := f.DefaultResourceFilterFunc()
 	filterOpts := f.DefaultResourceFilterOptions(cmd, allNamespaces)
@@ -332,7 +335,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 			res = infos[0].ResourceMapping().Resource
 		}
 
-		obj, err := resource.AsVersionedObject(infos, !singular, version, f.JSONEncoder())
+		obj, err := resource.AsVersionedObject(infos, !singular, version, typer, f.JSONEncoder())
 		if err != nil {
 			return err
 		}

diff --git a/pkg/kubectl/cmd/get_test.go b/pkg/kubectl/cmd/get_test.go
@@ -210,7 +210,7 @@ func TestGetUnknownSchemaObjectListGeneric(t *testing.T) {
 		cmd.Flags().Set("output", "json")
 
 		cmd.Flags().Set("output-version", test.outputVersion)
-		err := RunGet(f, buf, errBuf, cmd, []string{"type/foo", "replicationcontrollers/foo"}, &GetOptions{})
+		err := RunGet(f, buf, errBuf, cmd, []string{"type/foo", "replicationcontrollers/foo"}, &GetOptions{Typer: api.Scheme})
 		if err != nil {
 			t.Errorf("%s: unexpected error: %v", k, err)
 			continue
@@ -225,10 +225,10 @@ func TestGetUnknownSchemaObjectListGeneric(t *testing.T) {
 		}
 		arr := out["items"].([]interface{})
 		if arr[0].(map[string]interface{})["apiVersion"] != test.testtypeVersion {
-			t.Errorf("%s: unexpected list: %#v", k, out)
+			t.Errorf("%s: unexpected list: %#v (expected: %#v)", k, arr[0].(map[string]interface{})["apiVersion"], test.testtypeVersion)
 		}
 		if arr[1].(map[string]interface{})["apiVersion"] != test.rcVersion {
-			t.Errorf("%s: unexpected list: %#v", k, out)
+			t.Errorf("%s: unexpected list: %#v (expected: %#v)", k, arr[1].(map[string]interface{})["apiVersion"], test.rcVersion)
 		}
 	}
 }

diff --git a/pkg/kubectl/cmd/patch.go b/pkg/kubectl/cmd/patch.go
@@ -177,7 +177,7 @@ func RunPatch(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 				// don't bother checking for failures of this replace, because a failure to indicate the hint doesn't fail the command
 				// also, don't force the replacement.  If the replacement fails on a resourceVersion conflict, then it means this
 				// record hint is likely to be invalid anyway, so avoid the bad hint
-				patch, err := cmdutil.ChangeResourcePatch(info, f.Command())
+				patch, err := cmdutil.ChangeResourcePatch(info, typer, f.JSONEncoder(), f.Command())
 				if err == nil {
 					helper.Patch(info.Namespace, info.Name, api.StrategicMergePatchType, patch)
 				}

diff --git a/pkg/kubectl/cmd/scale.go b/pkg/kubectl/cmd/scale.go
@@ -164,7 +164,7 @@ func RunScale(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 			return err
 		}
 		if cmdutil.ShouldRecord(cmd, info) {
-			patchBytes, err := cmdutil.ChangeResourcePatch(info, f.Command())
+			patchBytes, err := cmdutil.ChangeResourcePatch(info, typer, f.JSONEncoder(), f.Command())
 			if err != nil {
 				return err
 			}

diff --git a/pkg/kubectl/cmd/set/set_image.go b/pkg/kubectl/cmd/set/set_image.go
@@ -212,7 +212,7 @@ func (o *ImageOptions) Run() error {
 
 		// record this change (for rollout history)
 		if o.Record || cmdutil.ContainsChangeCause(info) {
-			if patch, err := cmdutil.ChangeResourcePatch(info, o.ChangeCause); err == nil {
+			if patch, err := cmdutil.ChangeResourcePatch(info, o.Typer, o.Encoder, o.ChangeCause); err == nil {
 				if obj, err = resource.NewHelper(info.Client, info.Mapping).Patch(info.Namespace, info.Name, api.StrategicMergePatchType, patch); err != nil {
 					fmt.Fprintf(o.Err, "WARNING: changes to %s/%s can't be recorded: %v\n", info.Mapping.Resource, info.Name, err)
 				}

diff --git a/pkg/kubectl/cmd/util/BUILD b/pkg/kubectl/cmd/util/BUILD
@@ -96,6 +96,7 @@ go_test(
         "//pkg/kubectl/resource:go_default_library",
         "//pkg/labels:go_default_library",
         "//pkg/runtime:go_default_library",
+        "//pkg/runtime/serializer:go_default_library",
         "//pkg/util/exec:go_default_library",
         "//pkg/util/flag:go_default_library",
         "//pkg/util/validation/field:go_default_library",

diff --git a/pkg/kubectl/cmd/util/helpers.go b/pkg/kubectl/cmd/util/helpers.go
@@ -18,7 +18,6 @@ package util
 
 import (
 	"bytes"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -522,19 +521,24 @@ func RecordChangeCause(obj runtime.Object, changeCause string) error {
 
 // ChangeResourcePatch creates a strategic merge patch between the origin input resource info
 // and the annotated with change-cause input resource info.
-func ChangeResourcePatch(info *resource.Info, changeCause string) ([]byte, error) {
-	oldData, err := json.Marshal(info.Object)
+func ChangeResourcePatch(info *resource.Info, typer runtime.ObjectTyper, encoder runtime.Encoder, changeCause string) ([]byte, error) {
+	defaultVersion := info.ResourceMapping().GroupVersionKind.GroupVersion()
+	object, err := resource.AsVersionedObject([]*resource.Info{info}, false, defaultVersion, typer, encoder)
 	if err != nil {
 		return nil, err
 	}
-	if err := RecordChangeCause(info.Object, changeCause); err != nil {
+	oldData, err := runtime.Encode(encoder, object)
+	if err != nil {
+		return nil, err
+	}
+	if err := RecordChangeCause(object, changeCause); err != nil {
 		return nil, err
 	}
-	newData, err := json.Marshal(info.Object)
+	newData, err := runtime.Encode(encoder, object)
 	if err != nil {
 		return nil, err
 	}
-	return strategicpatch.CreateTwoWayMergePatch(oldData, newData, info.Object)
+	return strategicpatch.CreateTwoWayMergePatch(oldData, newData, object)
 }
 
 // containsChangeCause checks if input resource info contains change-cause annotation.

diff --git a/pkg/kubectl/cmd/util/helpers_test.go b/pkg/kubectl/cmd/util/helpers_test.go
@@ -35,7 +35,9 @@ import (
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/apimachinery/registered"
 	"k8s.io/kubernetes/pkg/apis/extensions"
+	"k8s.io/kubernetes/pkg/kubectl/resource"
 	"k8s.io/kubernetes/pkg/runtime"
+	"k8s.io/kubernetes/pkg/runtime/serializer"
 	uexec "k8s.io/kubernetes/pkg/util/exec"
 	"k8s.io/kubernetes/pkg/util/validation/field"
 )
@@ -329,6 +331,94 @@ func TestDumpReaderToFile(t *testing.T) {
 	}
 }
 
+type TestType struct {
+	unversioned.TypeMeta
+	api.ObjectMeta
+	Data string
+}
+
+func (t *TestType) GetObjectKind() unversioned.ObjectKind {
+	return &t.TypeMeta
+}
+
+type ExternalTestType struct {
+	unversioned.TypeMeta `json:",inline"`
+	v1.ObjectMeta        `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
+	Data                 string `json:"notData,omitempty" protobuf:"bytes,2,opt,name=data"`
+}
+
+func (t *ExternalTestType) GetObjectKind() unversioned.ObjectKind {
+	return &t.TypeMeta
+}
+
+func TestChangeResourcePatch(t *testing.T) {
+	// TestType is an internal type without JSON tags
+	internalGV := unversioned.GroupVersion{Group: "test", Version: runtime.APIVersionInternal}
+	externalGV := unversioned.GroupVersion{Group: "test", Version: "v1"}
+
+	s := runtime.NewScheme()
+	s.AddKnownTypes(internalGV, &TestType{})
+	s.AddKnownTypes(externalGV, &ExternalTestType{})
+	s.AddKnownTypeWithName(externalGV.WithKind("TestType"), &ExternalTestType{})
+	s.AddKnownTypeWithName(internalGV.WithKind("TestType"), &TestType{})
+	// TODO: Use AddToScheme(s) here instead of explicitely calling the conversion function.
+	// api.AddToScheme(s)
+	s.AddConversionFuncs(api.Convert_unversioned_Time_To_unversioned_Time)
+	testEncoder := serializer.NewCodecFactory(s).LegacyCodec(externalGV)
+
+	tests := []struct {
+		input     runtime.Object
+		encoder   runtime.Encoder
+		convertor runtime.ObjectConvertor
+		typer     runtime.ObjectTyper
+		expected  string
+		version   string
+	}{
+		{
+			input: &api.Pod{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+			},
+			version:   "v1",
+			convertor: api.Scheme,
+			typer:     api.Scheme,
+			encoder:   api.Codecs.LegacyCodec(registered.EnabledVersions()...),
+			expected:  "{\"metadata\":{\"annotations\":{\"kubernetes.io/change-cause\":\"changed by foo\"}}}",
+		},
+		{
+			input: &TestType{
+				ObjectMeta: api.ObjectMeta{
+					Name: "foo",
+				},
+				Data: "bar",
+			},
+			version:   "test/v1",
+			convertor: s,
+			typer:     s,
+			encoder:   testEncoder,
+			expected:  "{\"metadata\":{\"annotations\":{\"kubernetes.io/change-cause\":\"changed by foo\"}}}",
+		},
+	}
+	for i, test := range tests {
+		accessor, _ := meta.TypeAccessor(test.input)
+		info := resource.Info{
+			Object: test.input,
+			Mapping: &meta.RESTMapping{
+				ObjectConvertor:  test.convertor,
+				GroupVersionKind: unversioned.FromAPIVersionAndKind(test.version, accessor.GetKind()),
+			},
+		}
+		result, err := ChangeResourcePatch(&info, test.typer, test.encoder, "changed by foo")
+		if err != nil {
+			t.Errorf("[%d] unexpected error: %v", i, err)
+		}
+		if string(result) != test.expected {
+			t.Errorf("[%d] expected %q got %q", i, test.expected, string(result))
+		}
+	}
+}
+
 func TestMaybeConvert(t *testing.T) {
 	tests := []struct {
 		input    runtime.Object

diff --git a/pkg/kubectl/resource/result.go b/pkg/kubectl/resource/result.go
@@ -211,8 +211,8 @@ func (r *Result) Watch(resourceVersion string) (watch.Interface, error) {
 // the objects as children, or if only a single Object is present, as that object. The provided
 // version will be preferred as the conversion target, but the Object's mapping version will be
 // used if that version is not present.
-func AsVersionedObject(infos []*Info, forceList bool, version unversioned.GroupVersion, encoder runtime.Encoder) (runtime.Object, error) {
-	objects, err := AsVersionedObjects(infos, version, encoder)
+func AsVersionedObject(infos []*Info, forceList bool, version unversioned.GroupVersion, typer runtime.ObjectTyper, encoder runtime.Encoder) (runtime.Object, error) {
+	objects, err := AsVersionedObjects(infos, version, typer, encoder)
 	if err != nil {
 		return nil, err
 	}
@@ -234,7 +234,7 @@ func AsVersionedObject(infos []*Info, forceList bool, version unversioned.GroupV
 // AsVersionedObjects converts a list of infos into versioned objects. The provided
 // version will be preferred as the conversion target, but the Object's mapping version will be
 // used if that version is not present.
-func AsVersionedObjects(infos []*Info, version unversioned.GroupVersion, encoder runtime.Encoder) ([]runtime.Object, error) {
+func AsVersionedObjects(infos []*Info, version unversioned.GroupVersion, typer runtime.ObjectTyper, encoder runtime.Encoder) ([]runtime.Object, error) {
 	objects := []runtime.Object{}
 	for _, info := range infos {
 		if info.Object == nil {
@@ -251,7 +251,7 @@ func AsVersionedObjects(infos []*Info, version unversioned.GroupVersion, encoder
 		// objects that are not part of api.Scheme must be converted to JSON
 		// TODO: convert to map[string]interface{}, attach to runtime.Unknown?
 		if !version.Empty() {
-			if _, _, err := api.Scheme.ObjectKinds(info.Object); runtime.IsNotRegisteredError(err) {
+			if _, _, err := typer.ObjectKinds(info.Object); runtime.IsNotRegisteredError(err) {
 				// TODO: ideally this would encode to version, but we don't expose multiple codecs here.
 				data, err := runtime.Encode(encoder, info.Object)
 				if err != nil {