Builder should allow moving between unstructured / versioned easily

Demonstrate its use with get.go by removing the need to call decode with
a hardcoded scheme. Add tests to get_test.go to restore proving that
unstructured conversion is possible.

pkg/kubectl/cmd/resource/get.go

@@ -232,13 +232,8 @@ func (options *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []str
 		return options.watch(f, cmd, args)
 	}
 
-	mapper, typer, err := f.UnstructuredObject()
-	if err != nil {
-		return err
-	}
-
 	r := f.NewBuilder().
-		Unstructured(f.UnstructuredClientForMapping, mapper, typer).
+		Unstructured().
 		NamespaceParam(options.Namespace).DefaultNamespace().AllNamespaces(options.AllNamespaces).
 		FilenameParam(options.ExplicitNamespace, &options.FilenameOptions).
 		LabelSelectorParam(options.LabelSelector).
@@ -315,13 +310,15 @@ func (options *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []str
 	for ix := range objs {
 		var mapping *meta.RESTMapping
 		var original runtime.Object
-
+		var info *resource.Info
 		if sorter != nil {
-			mapping = infos[sorter.OriginalPosition(ix)].Mapping
+			info = infos[sorter.OriginalPosition(ix)]
-			original = infos[sorter.OriginalPosition(ix)].Object
+			mapping = info.Mapping
+			original = info.Object
 		} else {
-			mapping = infos[ix].Mapping
+			info = infos[ix]
-			original = infos[ix].Object
+			mapping = info.Mapping
+			original = info.Object
 		}
 		if shouldGetNewPrinterForMapping(printer, lastMapping, mapping) {
 			if printer != nil {
@@ -360,11 +357,10 @@ func (options *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []str
 			lastMapping = mapping
 		}
 
-		// try to convert before apply filter func
+		typedObj := info.AsInternal()
-		decodedObj, _ := kubectl.DecodeUnknownObject(original)
 
 		// filter objects if filter has been defined for current object
-		if isFiltered, err := filterFuncs.Filter(decodedObj, filterOpts); isFiltered {
+		if isFiltered, err := filterFuncs.Filter(typedObj, filterOpts); isFiltered {
 			if err == nil {
 				filteredResourceCount++
 				continue
@@ -394,7 +390,7 @@ func (options *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []str
 				resourcePrinter.EnsurePrintWithKind(resourceName)
 			}
 
-			if err := printer.PrintObj(decodedObj, w); err != nil {
+			if err := printer.PrintObj(typedObj, w); err != nil {
 				if !errs.Has(err.Error()) {
 					errs.Insert(err.Error())
 					allErrs = append(allErrs, err)
@@ -402,7 +398,7 @@ func (options *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []str
 			}
 			continue
 		}
-		objToPrint := decodedObj
+		objToPrint := typedObj
 		if printer.IsGeneric() {
 			// use raw object as recieved from the builder when using generic
 			// printer instead of decodedObj
@@ -445,18 +441,13 @@ func (options *GetOptions) raw(f cmdutil.Factory) error {
 // watch starts a client-side watch of one or more resources.
 // TODO: remove the need for arguments here.
 func (options *GetOptions) watch(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
-	mapper, typer, err := f.UnstructuredObject()
-	if err != nil {
-		return err
-	}
-
 	// TODO: this could be better factored
 	// include uninitialized objects when watching on a single object
 	// unless explicitly set --include-uninitialized=false
 	includeUninitialized := cmdutil.ShouldIncludeUninitialized(cmd, len(args) == 2)
 
 	r := f.NewBuilder().
-		Unstructured(f.UnstructuredClientForMapping, mapper, typer).
+		Unstructured().
 		NamespaceParam(options.Namespace).DefaultNamespace().AllNamespaces(options.AllNamespaces).
 		FilenameParam(options.ExplicitNamespace, &options.FilenameOptions).
 		LabelSelectorParam(options.LabelSelector).
@@ -468,8 +459,7 @@ func (options *GetOptions) watch(f cmdutil.Factory, cmd *cobra.Command, args []s
 		SingleResourceType().
 		Latest().
 		Do()
-	err = r.Err()
+	if err := r.Err(); err != nil {
-	if err != nil {
 		return err
 	}
 	infos, err := r.Infos()

pkg/kubectl/cmd/resource/get_test.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer/json"
 	"k8s.io/apimachinery/pkg/runtime/serializer/streaming"
+	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/dynamic"
 	restclient "k8s.io/client-go/rest"
@@ -208,15 +209,49 @@ func testComponentStatusData() *api.ComponentStatusList {
 // Verifies that schemas that are not in the master tree of Kubernetes can be retrieved via Get.
 func TestGetUnknownSchemaObject(t *testing.T) {
 	f, tf, _, _ := cmdtesting.NewAPIFactory()
+	tf.WithCustomScheme()
 	_, _, codec, _ := cmdtesting.NewTestFactory()
 	tf.Printer = &testPrinter{}
 	tf.OpenAPISchemaFunc = openapitesting.CreateOpenAPISchemaFunc(openapiSchemaPath)
+
+	obj := &cmdtesting.ExternalType{
+		Kind:       "Type",
+		APIVersion: "apitest/unlikelyversion",
+		Name:       "foo",
+	}
+
 	tf.UnstructuredClient = &fake.RESTClient{
 		NegotiatedSerializer: unstructuredSerializer,
-		Resp:                 &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, cmdtesting.NewInternalType("", "", "foo"))},
+		Resp: &http.Response{
+			StatusCode: 200, Header: defaultHeader(),
+			Body: objBody(codec, obj),
+		},
 	}
 	tf.Namespace = "test"
 	tf.ClientConfig = defaultClientConfig()
+
+	mapper, _, err := f.UnstructuredObject()
+	if err != nil {
+		t.Fatal(err)
+	}
+	m, err := mapper.RESTMapping(schema.GroupKind{Group: "apitest", Kind: "Type"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	convertedObj, err := m.ConvertToVersion(&unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"kind":       "Type",
+			"apiVersion": "apitest/unlikelyversion",
+			"name":       "foo",
+		},
+	}, schema.GroupVersion{Group: "apitest", Version: "unlikelyversion"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !reflect.DeepEqual(convertedObj, obj) {
+		t.Fatalf("unexpected conversion of unstructured object to structured: %s", diff.ObjectReflectDiff(convertedObj, obj))
+	}
+
 	buf := bytes.NewBuffer([]byte{})
 	errBuf := bytes.NewBuffer([]byte{})
 
@@ -229,6 +264,7 @@ func TestGetUnknownSchemaObject(t *testing.T) {
 	if len(actual) != len(expected) {
 		t.Fatalf("expected: %#v, but actual: %#v", expected, actual)
 	}
+	t.Logf("actual: %#v", actual[0])
 	for i, obj := range actual {
 		expectedJSON := runtime.EncodeOrDie(codec, expected[i])
 		expectedMap := map[string]interface{}{}
@@ -236,7 +272,7 @@ func TestGetUnknownSchemaObject(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		actualJSON := runtime.EncodeOrDie(scheme.Codecs.LegacyCodec(), obj)
+		actualJSON := runtime.EncodeOrDie(codec, obj)
 		actualMap := map[string]interface{}{}
 		if err := encjson.Unmarshal([]byte(actualJSON), &actualMap); err != nil {
 			t.Fatal(err)
@@ -381,30 +417,32 @@ func TestGetObjectsFiltered(t *testing.T) {
 	}
 
 	for i, test := range testCases {
-		t.Logf("%d", i)
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
-		f, tf, codec, _ := cmdtesting.NewAPIFactory()
+			f, tf, codec, _ := cmdtesting.NewAPIFactory()
-		tf.Printer = &testPrinter{GenericPrinter: test.genericPrinter}
+			tf.WithLegacyScheme()
-		tf.UnstructuredClient = &fake.RESTClient{
+			tf.Printer = &testPrinter{GenericPrinter: test.genericPrinter}
-			GroupVersion:         schema.GroupVersion{Version: "v1"},
+			tf.UnstructuredClient = &fake.RESTClient{
-			NegotiatedSerializer: unstructuredSerializer,
+				GroupVersion:         schema.GroupVersion{Version: "v1"},
-			Resp:                 &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, test.resp)},
+				NegotiatedSerializer: unstructuredSerializer,
-		}
+				Resp:                 &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, test.resp)},
-		tf.Namespace = "test"
+			}
-		buf := bytes.NewBuffer([]byte{})
+			tf.Namespace = "test"
-		errBuf := bytes.NewBuffer([]byte{})
+			buf := bytes.NewBuffer([]byte{})
+			errBuf := bytes.NewBuffer([]byte{})
 
-		cmd := NewCmdGet(f, buf, errBuf)
+			cmd := NewCmdGet(f, buf, errBuf)
-		cmd.SetOutput(buf)
+			cmd.SetOutput(buf)
-		for k, v := range test.flags {
+			for k, v := range test.flags {
-			cmd.Flags().Lookup(k).Value.Set(v)
+				cmd.Flags().Lookup(k).Value.Set(v)
-		}
+			}
-		cmd.Run(cmd, test.args)
+			cmd.Run(cmd, test.args)
 
-		verifyObjects(t, test.expect, tf.Printer.(*testPrinter).Objects)
+			verifyObjects(t, test.expect, tf.Printer.(*testPrinter).Objects)
 
-		if len(buf.String()) == 0 {
+			if len(buf.String()) == 0 {
-			t.Errorf("%d: unexpected empty output", i)
+				t.Errorf("%d: unexpected empty output", i)
-		}
+			}
+		})
 	}
 }
 

pkg/kubectl/cmd/testing/fake.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
@@ -286,7 +287,7 @@ func (f *FakeFactory) Object() (meta.RESTMapper, runtime.ObjectTyper) {
 
 func (f *FakeFactory) UnstructuredObject() (meta.RESTMapper, runtime.ObjectTyper, error) {
 	groupResources := testDynamicResources()
-	mapper := discovery.NewRESTMapper(groupResources, meta.InterfacesForUnstructured)
+	mapper := discovery.NewRESTMapper(groupResources, meta.InterfacesForUnstructuredConversion(legacyscheme.Registry.InterfacesFor))
 	typer := discovery.NewUnstructuredObjectTyper(groupResources)
 
 	fakeDs := &fakeCachedDiscoveryClient{}
@@ -518,7 +519,23 @@ func (f *FakeFactory) PrinterForMapping(cmd *cobra.Command, isLocal bool, output
 
 func (f *FakeFactory) NewBuilder() *resource.Builder {
 	mapper, typer := f.Object()
-	return resource.NewBuilder(mapper, f.CategoryExpander(), typer, resource.ClientMapperFunc(f.ClientForMapping), f.Decoder(true))
+	unstructuredMapper, unstructuredTyper, _ := f.UnstructuredObject()
+
+	return resource.NewBuilder(
+		&resource.Mapper{
+			RESTMapper:   mapper,
+			ObjectTyper:  typer,
+			ClientMapper: resource.ClientMapperFunc(f.ClientForMapping),
+			Decoder:      f.Decoder(true),
+		},
+		&resource.Mapper{
+			RESTMapper:   unstructuredMapper,
+			ObjectTyper:  unstructuredTyper,
+			ClientMapper: resource.ClientMapperFunc(f.UnstructuredClientForMapping),
+			Decoder:      unstructured.UnstructuredJSONScheme,
+		},
+		f.CategoryExpander(),
+	)
 }
 
 func (f *FakeFactory) DefaultResourceFilterOptions(cmd *cobra.Command, withNamespace bool) *printers.PrintOptions {
@@ -593,7 +610,23 @@ func (f *fakeAPIFactory) Object() (meta.RESTMapper, runtime.ObjectTyper) {
 
 func (f *fakeAPIFactory) UnstructuredObject() (meta.RESTMapper, runtime.ObjectTyper, error) {
 	groupResources := testDynamicResources()
-	mapper := discovery.NewRESTMapper(groupResources, meta.InterfacesForUnstructured)
+	mapper := discovery.NewRESTMapper(
+		groupResources,
+		meta.InterfacesForUnstructuredConversion(func(version schema.GroupVersion) (*meta.VersionInterfaces, error) {
+			switch version {
+			// provide typed objects for these two versions
+			case ValidVersionGV, UnlikelyGV:
+				return &meta.VersionInterfaces{
+					ObjectConvertor:  f.tf.Typer.(*runtime.Scheme),
+					MetadataAccessor: meta.NewAccessor(),
+				}, nil
+			// otherwise fall back to the legacy scheme
+			default:
+				return legacyscheme.Registry.InterfacesFor(version)
+			}
+		}),
+	)
+
 	typer := discovery.NewUnstructuredObjectTyper(groupResources)
 	fakeDs := &fakeCachedDiscoveryClient{}
 	expander, err := cmdutil.NewShortcutExpander(mapper, fakeDs)
@@ -832,7 +865,23 @@ func (f *fakeAPIFactory) PrinterForMapping(cmd *cobra.Command, isLocal bool, out
 
 func (f *fakeAPIFactory) NewBuilder() *resource.Builder {
 	mapper, typer := f.Object()
-	return resource.NewBuilder(mapper, f.CategoryExpander(), typer, resource.ClientMapperFunc(f.ClientForMapping), f.Decoder(true))
+	unstructuredMapper, unstructuredTyper, _ := f.UnstructuredObject()
+
+	return resource.NewBuilder(
+		&resource.Mapper{
+			RESTMapper:   mapper,
+			ObjectTyper:  typer,
+			ClientMapper: resource.ClientMapperFunc(f.ClientForMapping),
+			Decoder:      f.Decoder(true),
+		},
+		&resource.Mapper{
+			RESTMapper:   unstructuredMapper,
+			ObjectTyper:  unstructuredTyper,
+			ClientMapper: resource.ClientMapperFunc(f.UnstructuredClientForMapping),
+			Decoder:      unstructured.UnstructuredJSONScheme,
+		},
+		f.CategoryExpander(),
+	)
 }
 
 func (f *fakeAPIFactory) SuggestedPodTemplateResources() []schema.GroupResource {
@@ -857,6 +906,17 @@ func NewAPIFactory() (cmdutil.Factory, *TestFactory, runtime.Codec, runtime.Nego
 	}, t, testapi.Default.Codec(), testapi.Default.NegotiatedSerializer()
 }
 
+func (f *TestFactory) WithCustomScheme() *TestFactory {
+	scheme, _, _ := newExternalScheme()
+	f.Typer = scheme
+	return f
+}
+
+func (f *TestFactory) WithLegacyScheme() *TestFactory {
+	f.Typer = legacyscheme.Scheme
+	return f
+}
+
 func testDynamicResources() []*discovery.APIGroupResources {
 	return []*discovery.APIGroupResources{
 		{
@@ -875,7 +935,6 @@ func testDynamicResources() []*discovery.APIGroupResources {
 					{Name: "nodes", Namespaced: false, Kind: "Node"},
 					{Name: "secrets", Namespaced: true, Kind: "Secret"},
 					{Name: "configmaps", Namespaced: true, Kind: "ConfigMap"},
-					{Name: "type", Namespaced: false, Kind: "Type"},
 					{Name: "namespacedtype", Namespaced: true, Kind: "NamespacedType"},
 				},
 			},
@@ -943,5 +1002,21 @@ func testDynamicResources() []*discovery.APIGroupResources {
 				},
 			},
 		},
+		{
+			Group: metav1.APIGroup{
+				Name: "apitest",
+				Versions: []metav1.GroupVersionForDiscovery{
+					{GroupVersion: "apitest/unlikelyversion", Version: "unlikelyversion"},
+				},
+				PreferredVersion: metav1.GroupVersionForDiscovery{
+					GroupVersion: "apitest/unlikelyversion",
+					Version:      "unlikelyversion"},
+			},
+			VersionedResources: map[string][]metav1.APIResource{
+				"unlikelyversion": {
+					{Name: "types", SingularName: "type", Namespaced: false, Kind: "Type"},
+				},
+			},
+		},
 	}
 }

pkg/kubectl/resource/visitor.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -73,15 +74,22 @@ type ResourceMapping interface {
 // Info contains temporary info to execute a REST call, or show the results
 // of an already completed REST call.
 type Info struct {
-	Client    RESTClient
+	Client RESTClient
-	Mapping   *meta.RESTMapping
+	// Mapping may be nil if the object has no available metadata, but is still parseable
+	// from disk.
+	Mapping *meta.RESTMapping
+	// Namespace will be set if the object is namespaced and has a specified value.
 	Namespace string
 	Name      string
 
 	// Optional, Source is the filename or URL to template file (.json or .yaml),
 	// or stdin to use to handle the resource
 	Source string
-	// Optional, this is the most recent value returned by the server if available
+	// Optional, this is the most recent value returned by the server if available. It will
+	// typically be in unstructured or internal forms, depending on how the Builder was
+	// defined. If retrieved from the server, the Builder expects the mapping client to
+	// decide the final form. Use the AsVersioned, AsUnstructured, and AsInternal helpers
+	// to alter the object versions.
 	Object runtime.Object
 	// Optional, this is the most recent resource version the server knows about for
 	// this type of resource. It may not match the resource version of the object,
@@ -161,6 +169,61 @@ func (i *Info) ResourceMapping() *meta.RESTMapping {
 	return i.Mapping
 }
 
+// Internal attempts to convert the provided object to an internal type or returns an error.
+func (i *Info) Internal() (runtime.Object, error) {
+	return i.Mapping.ConvertToVersion(i.Object, i.Mapping.GroupVersionKind.GroupKind().WithVersion(runtime.APIVersionInternal).GroupVersion())
+}
+
+// AsInternal returns the object in internal form if possible, or i.Object if it cannot be
+// converted.
+func (i *Info) AsInternal() runtime.Object {
+	if obj, err := i.Internal(); err == nil {
+		return obj
+	}
+	return i.Object
+}
+
+// Versioned returns the object as a Go type in the mapping's version or returns an error.
+func (i *Info) Versioned() (runtime.Object, error) {
+	return i.Mapping.ConvertToVersion(i.Object, i.Mapping.GroupVersionKind.GroupVersion())
+}
+
+// AsVersioned returns the object as a Go object in the external form if possible (matching the
+// group version kind of the mapping, or i.Object if it cannot be converted.
+func (i *Info) AsVersioned() runtime.Object {
+	if obj, err := i.Versioned(); err == nil {
+		return obj
+	}
+	return i.Object
+}
+
+// Unstructured returns the current object in unstructured form (as a runtime.Unstructured)
+func (i *Info) Unstructured() (runtime.Unstructured, error) {
+	switch t := i.Object.(type) {
+	case runtime.Unstructured:
+		return t, nil
+	case *runtime.Unknown:
+		gvk := i.Mapping.GroupVersionKind
+		out, _, err := unstructured.UnstructuredJSONScheme.Decode(t.Raw, &gvk, nil)
+		return out.(runtime.Unstructured), err
+	default:
+		out := &unstructured.Unstructured{}
+		if err := i.Mapping.Convert(i.Object, out, nil); err != nil {
+			return nil, err
+		}
+		return out, nil
+	}
+}
+
+// AsUnstructured returns the object as a Go object in external form as a runtime.Unstructured
+// (map of JSON equivalent values) or as i.Object if it cannot be converted.
+func (i *Info) AsUnstructured() runtime.Object {
+	if out, err := i.Unstructured(); err == nil {
+		return out
+	}
+	return i.Object
+}
+
 // VisitorList implements Visit for the sub visitors it contains. The first error
 // returned from a child Visitor will terminate iteration.
 type VisitorList []Visitor

pkg/kubectl/resource_filter.go

@@ -19,7 +19,6 @@ package kubectl
 import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/printers"
 )
@@ -57,10 +56,6 @@ func filterPods(obj runtime.Object, options printers.PrintOptions) bool {
 
 // Filter loops through a collection of FilterFuncs until it finds one that can filter the given resource
 func (f Filters) Filter(obj runtime.Object, opts *printers.PrintOptions) (bool, error) {
-	// check if the object is unstructured. If so, let's attempt to convert it to a type we can understand
-	// before apply filter func.
-	obj, _ = DecodeUnknownObject(obj)
-
 	for _, filter := range f {
 		if ok := filter(obj, *opts); ok {
 			return true, nil
@@ -68,19 +63,3 @@ func (f Filters) Filter(obj runtime.Object, opts *printers.PrintOptions) (bool,
 	}
 	return false, nil
 }
-
-// check if the object is unstructured. If so, let's attempt to convert it to a type we can understand.
-func DecodeUnknownObject(obj runtime.Object) (runtime.Object, error) {
-	var err error
-
-	switch obj.(type) {
-	case runtime.Unstructured, *runtime.Unknown:
-		if objBytes, err := runtime.Encode(legacyscheme.Codecs.LegacyCodec(), obj); err == nil {
-			if decodedObj, err := runtime.Decode(legacyscheme.Codecs.UniversalDecoder(), objBytes); err == nil {
-				obj = decodedObj
-			}
-		}
-	}
-
-	return obj, err
-}