List output with differing types should be more resilient

kubectl get can output a series of objects as a List in versioned
form, but not all API objects are available in the same schema.
Make the act of converting a []runtime.Object to api.List more
robust and add a test to verify its behavior in Get.

Makes it easier for client code to output unified objects.

hack/test-cmd.sh

@@ -162,6 +162,7 @@ for version in "${kube_api_versions[@]}"; do
   # Command
   kubectl create "${kube_flags[@]}" -f examples/limitrange/valid-pod.json
   # Post-condition: valid-pod POD is running
+  kubectl get "${kube_flags[@]}" pods -o json
   kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'valid-pod:'
   kube::test::get_object_assert 'pod valid-pod' "{{$id_field}}" 'valid-pod'
   kube::test::get_object_assert 'pod/valid-pod' "{{$id_field}}" 'valid-pod'

pkg/api/meta/restmapper.go

@@ -251,3 +251,42 @@ func (m *DefaultRESTMapper) AliasesForResource(alias string) ([]string, bool) {
 	}
 	return nil, false
 }
+
+// MultiRESTMapper is a wrapper for multiple RESTMappers.
+type MultiRESTMapper []RESTMapper
+
+// VersionAndKindForResource provides the Version and Kind  mappings for the
+// REST resources. This implementation supports multiple REST schemas and return
+// the first match.
+func (m MultiRESTMapper) VersionAndKindForResource(resource string) (defaultVersion, kind string, err error) {
+	for _, t := range m {
+		defaultVersion, kind, err = t.VersionAndKindForResource(resource)
+		if err == nil {
+			return
+		}
+	}
+	return
+}
+
+// RESTMapping provides the REST mapping for the resource based on the resource
+// kind and version. This implementation supports multiple REST schemas and
+// return the first match.
+func (m MultiRESTMapper) RESTMapping(kind string, versions ...string) (mapping *RESTMapping, err error) {
+	for _, t := range m {
+		mapping, err = t.RESTMapping(kind, versions...)
+		if err == nil {
+			return
+		}
+	}
+	return
+}
+
+// AliasesForResource finds the first alias response for the provided mappers.
+func (m MultiRESTMapper) AliasesForResource(alias string) (aliases []string, ok bool) {
+	for _, t := range m {
+		if aliases, ok = t.AliasesForResource(alias); ok {
+			return
+		}
+	}
+	return nil, false
+}

pkg/conversion/encode.go

@@ -53,7 +53,7 @@ func (s *Scheme) EncodeToVersion(obj interface{}, destVersion string) (data []by
 	obj = maybeCopy(obj)
 	v, _ := EnforcePtr(obj) // maybeCopy guarantees a pointer
 	if _, registered := s.typeToVersion[v.Type()]; !registered {
-		return nil, fmt.Errorf("type %v is not registered and it will be impossible to Decode it, therefore Encode will refuse to encode it.", v.Type())
+		return nil, fmt.Errorf("type %v is not registered for %q and it will be impossible to Decode it, therefore Encode will refuse to encode it.", v.Type(), destVersion)
 	}
 
 	objVersion, objKind, err := s.ObjectVersionAndKind(obj)

pkg/kubectl/cmd/cmd_test.go

@@ -49,23 +49,32 @@ type externalType struct {
 	Name string `json:"name"`
 }
 
-func (*internalType) IsAnAPIObject() {}
-func (*externalType) IsAnAPIObject() {}
+type ExternalType2 struct {
+	Kind       string `json:"kind"`
+	APIVersion string `json:"apiVersion"`
+
+	Name string `json:"name"`
+}
+
+func (*internalType) IsAnAPIObject()  {}
+func (*externalType) IsAnAPIObject()  {}
+func (*ExternalType2) IsAnAPIObject() {}
 
 func newExternalScheme() (*runtime.Scheme, meta.RESTMapper, runtime.Codec) {
 	scheme := runtime.NewScheme()
 	scheme.AddKnownTypeWithName("", "Type", &internalType{})
 	scheme.AddKnownTypeWithName("unlikelyversion", "Type", &externalType{})
+	scheme.AddKnownTypeWithName("v1beta1", "Type", &ExternalType2{})
 
 	codec := runtime.CodecFor(scheme, "unlikelyversion")
-	mapper := meta.NewDefaultRESTMapper([]string{"unlikelyversion"}, func(version string) (*meta.VersionInterfaces, bool) {
+	mapper := meta.NewDefaultRESTMapper([]string{"unlikelyversion", "v1beta1"}, func(version string) (*meta.VersionInterfaces, bool) {
 		return &meta.VersionInterfaces{
-			Codec:            codec,
+			Codec:            runtime.CodecFor(scheme, version),
 			ObjectConvertor:  scheme,
 			MetadataAccessor: meta.NewAccessor(),
-		}, (version == "unlikelyversion")
+		}, (version == "v1beta1" || version == "unlikelyversion")
 	})
-	for _, version := range []string{"unlikelyversion"} {
+	for _, version := range []string{"unlikelyversion", "v1beta1"} {
 		for kind := range scheme.KnownTypes(version) {
 			mixedCase := false
 			scope := meta.RESTScopeNamespace
@@ -142,6 +151,20 @@ func NewTestFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) {
 	}, t, codec
 }
 
+func NewMixedFactory(apiClient resource.RESTClient) (*cmdutil.Factory, *testFactory, runtime.Codec) {
+	f, t, c := NewTestFactory()
+	f.Object = func() (meta.RESTMapper, runtime.ObjectTyper) {
+		return meta.MultiRESTMapper{t.Mapper, latest.RESTMapper}, runtime.MultiObjectTyper{t.Typer, api.Scheme}
+	}
+	f.RESTClient = func(m *meta.RESTMapping) (resource.RESTClient, error) {
+		if m.ObjectConvertor == api.Scheme {
+			return apiClient, t.Err
+		}
+		return t.Client, t.Err
+	}
+	return f, t, c
+}
+
 func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec) {
 	t := &testFactory{
 		Validator: validation.NullSchema{},

pkg/kubectl/cmd/get.go

@@ -20,8 +20,6 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl"
 	cmdutil "github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/util"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/resource"
@@ -162,29 +160,21 @@ func RunGet(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []string
 		}
 		defaultVersion := clientConfig.Version
 
-		// the outermost object will be converted to the output-version
-		version := cmdutil.OutputVersion(cmd, defaultVersion)
-
+		singular := false
 		r := b.Flatten().Do()
-		obj, err := r.Object()
+		infos, err := r.IntoSingular(&singular).Infos()
 		if err != nil {
 			return err
 		}
 
-		// try conversion to all the possible versions
-		// TODO: simplify by adding a ResourceBuilder mode
-		versions := []string{version, latest.Version}
-		infos, _ := r.Infos()
-		for _, info := range infos {
-			versions = append(versions, info.Mapping.APIVersion)
+		// the outermost object will be converted to the output-version, but inner
+		// objects can use their mappings
+		version := cmdutil.OutputVersion(cmd, defaultVersion)
+		obj, err := resource.AsVersionedObject(infos, !singular, version)
+		if err != nil {
+			return err
 		}
 
-		// TODO: add a new ResourceBuilder mode for Object() that attempts to ensure the objects
-		// are in the appropriate version if one exists (and if not, use the best effort).
-		// TODO: ensure api-version is set with the default preferred api version by the client
-		// builder on initialization
-		printer := kubectl.NewVersionedPrinter(printer, api.Scheme, versions...)
-
 		return printer.PrintObj(obj, out)
 	}
 

pkg/kubectl/cmd/get_test.go

@@ -18,6 +18,7 @@ package cmd
 
 import (
 	"bytes"
+	encjson "encoding/json"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -138,6 +139,84 @@ func TestGetUnknownSchemaObject(t *testing.T) {
 	}
 }
 
+// Verifies that schemas that are not in the master tree of Kubernetes can be retrieved via Get.
+// Because api.List is part of the Kube API, resource.Builder has to perform a conversion on
+// api.Scheme, which may not have access to all objects, and not all objects are at the same
+// internal versioning scheme. This test verifies that two isolated schemes (Test, and api.Scheme)
+// can be conjoined into a single output object.
+func TestGetUnknownSchemaObjectListGeneric(t *testing.T) {
+	testCases := map[string]struct {
+		output string
+		list   string
+		obj1   string
+		obj2   string
+	}{
+		"handles specific version": {
+			output: "v1beta3",
+			list:   "v1beta3",
+			obj1:   "unlikelyversion",
+			obj2:   "v1beta3",
+		},
+		"handles second specific version": {
+			output: "unlikelyversion",
+			list:   "v1beta3",
+			obj1:   "unlikelyversion", // doesn't have v1beta3
+			obj2:   "v1beta1",         // version of the API response
+		},
+		"handles common version": {
+			output: "v1beta1",
+			list:   "v1beta1",
+			obj1:   "unlikelyversion", // because test scheme defaults to unlikelyversion
+			obj2:   "v1beta1",
+		},
+	}
+	for k, test := range testCases {
+		apiCodec := runtime.CodecFor(api.Scheme, "v1beta1")
+		regularClient := &client.FakeRESTClient{
+			Codec: apiCodec,
+			Client: client.HTTPClientFunc(func(req *http.Request) (*http.Response, error) {
+				return &http.Response{StatusCode: 200, Body: objBody(apiCodec, &api.ReplicationController{ObjectMeta: api.ObjectMeta{Name: "foo"}})}, nil
+			}),
+		}
+
+		f, tf, codec := NewMixedFactory(regularClient)
+		tf.Printer = &testPrinter{}
+		tf.Client = &client.FakeRESTClient{
+			Codec: codec,
+			Client: client.HTTPClientFunc(func(req *http.Request) (*http.Response, error) {
+				return &http.Response{StatusCode: 200, Body: objBody(codec, &internalType{Name: "foo"})}, nil
+			}),
+		}
+		tf.Namespace = "test"
+		tf.ClientConfig = &client.Config{Version: latest.Version}
+		buf := bytes.NewBuffer([]byte{})
+		cmd := NewCmdGet(f, buf)
+		cmd.SetOutput(buf)
+		cmd.Flags().Set("output", "json")
+		cmd.Flags().Set("output-version", test.output)
+		err := RunGet(f, buf, cmd, []string{"type/foo", "replicationcontrollers/foo"})
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", k, err)
+			continue
+		}
+		out := make(map[string]interface{})
+		if err := encjson.Unmarshal(buf.Bytes(), &out); err != nil {
+			t.Errorf("%s: unexpected error: %v\n%s", k, err, buf.String())
+			continue
+		}
+		if out["apiVersion"] != test.list {
+			t.Errorf("%s: unexpected list: %#v", k, out)
+		}
+		arr := out["items"].([]interface{})
+		if arr[0].(map[string]interface{})["apiVersion"] != test.obj1 {
+			t.Errorf("%s: unexpected list: %#v", k, out)
+		}
+		if arr[1].(map[string]interface{})["apiVersion"] != test.obj2 {
+			t.Errorf("%s: unexpected list: %#v", k, out)
+		}
+	}
+}
+
 // Verifies that schemas that are not in the master tree of Kubernetes can be retrieved via Get.
 func TestGetSchemaObject(t *testing.T) {
 	f, tf, _ := NewTestFactory()

pkg/kubectl/resource/result.go

@@ -21,6 +21,7 @@ import (
 	"reflect"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
@@ -200,3 +201,67 @@ func (r *Result) Watch(resourceVersion string) (watch.Interface, error) {
 	}
 	return w.Watch(resourceVersion)
 }
+
+// AsVersionedObject converts a list of infos into a single object - either a List containing
+// 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 string) (runtime.Object, error) {
+	objects := []runtime.Object{}
+	for _, info := range infos {
+		if info.Object == nil {
+			continue
+		}
+
+		// objects that are not part of api.Scheme must be converted to JSON
+		// TODO: convert to map[string]interface{}, attach to runtime.Unknown?
+		if len(version) > 0 {
+			if _, _, err := api.Scheme.ObjectVersionAndKind(info.Object); runtime.IsNotRegisteredError(err) {
+				// TODO: ideally this would encode to version, but we don't expose multiple codecs here.
+				data, err := info.Mapping.Codec.Encode(info.Object)
+				if err != nil {
+					return nil, err
+				}
+				objects = append(objects, &runtime.Unknown{RawJSON: data})
+				continue
+			}
+		}
+
+		converted, err := tryConvert(info.Mapping.ObjectConvertor, info.Object, version, info.Mapping.APIVersion)
+		if err != nil {
+			return nil, err
+		}
+		objects = append(objects, converted)
+	}
+
+	var object runtime.Object
+	if len(objects) == 1 && !forceList {
+		object = objects[0]
+	} else {
+		object = &api.List{Items: objects}
+		converted, err := tryConvert(api.Scheme, object, version, latest.Version)
+		if err != nil {
+			return nil, err
+		}
+		object = converted
+	}
+	return object, nil
+}
+
+// tryConvert attempts to convert the given object to the provided versions in order. This function assumes
+// the object is in internal version.
+func tryConvert(convertor runtime.ObjectConvertor, object runtime.Object, versions ...string) (runtime.Object, error) {
+	var last error
+	for _, version := range versions {
+		if len(version) == 0 {
+			return object, nil
+		}
+		obj, err := convertor.ConvertToVersion(object, version)
+		if err != nil {
+			last = err
+			continue
+		}
+		return obj, nil
+	}
+	return nil, last
+}

pkg/runtime/helper.go

@@ -149,3 +149,25 @@ func FieldPtr(v reflect.Value, fieldName string, dest interface{}) error {
 	}
 	return fmt.Errorf("couldn't assign/convert %v to %v", field.Type(), v.Type())
 }
+
+// MultiObjectTyper returns the types of objects across multiple schemes in order.
+type MultiObjectTyper []ObjectTyper
+
+func (m MultiObjectTyper) DataVersionAndKind(data []byte) (version, kind string, err error) {
+	for _, t := range m {
+		version, kind, err = t.DataVersionAndKind(data)
+		if err == nil {
+			return
+		}
+	}
+	return
+}
+func (m MultiObjectTyper) ObjectVersionAndKind(obj Object) (version, kind string, err error) {
+	for _, t := range m {
+		version, kind, err = t.ObjectVersionAndKind(obj)
+		if err == nil {
+			return
+		}
+	}
+	return
+}

pkg/runtime/scheme.go

@@ -18,9 +18,10 @@ package runtime
 
 import (
 	"fmt"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
 	"net/url"
 	"reflect"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
 )
 
 // Scheme defines methods for serializing and deserializing API objects. It
@@ -147,8 +148,9 @@ func (self *Scheme) rawExtensionToEmbeddedObject(in *RawExtension, out *Embedded
 }
 
 // runtimeObjectToRawExtensionArray takes a list of objects and encodes them as RawExtension in the output version
-// defined by the conversion.Scope. If objects must be encoded to different schema versions you should set them as
-// runtime.Unknown in the internal version instead.
+// defined by the conversion.Scope. If objects must be encoded to different schema versions than the default, you
+// should encode them yourself with runtime.Unknown, or convert the object prior to invoking conversion. Objects
+// outside of the current scheme must be added as runtime.Unknown.
 func (self *Scheme) runtimeObjectToRawExtensionArray(in *[]Object, out *[]RawExtension, s conversion.Scope) error {
 	src := *in
 	dest := make([]RawExtension, len(src))
@@ -160,7 +162,12 @@ func (self *Scheme) runtimeObjectToRawExtensionArray(in *[]Object, out *[]RawExt
 		case *Unknown:
 			dest[i].RawJSON = t.RawJSON
 		default:
-			data, err := scheme.EncodeToVersion(src[i], outVersion)
+			version := outVersion
+			// if the object exists
+			if inVersion, _, err := scheme.ObjectVersionAndKind(src[i]); err == nil && len(inVersion) != 0 {
+				version = inVersion
+			}
+			data, err := scheme.EncodeToVersion(src[i], version)
 			if err != nil {
 				return err
 			}