From 64f56764d5ba7d06cf60e2e5013dcf5e9a453a3c Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 13 Nov 2017 23:00:18 -0500 Subject: [PATCH] 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 | 38 +++++-------- pkg/kubectl/cmd/resource/get_test.go | 84 +++++++++++++++++++-------- pkg/kubectl/cmd/testing/fake.go | 85 ++++++++++++++++++++++++++-- pkg/kubectl/resource/visitor.go | 69 +++++++++++++++++++++- pkg/kubectl/resource_filter.go | 21 ------- 5 files changed, 221 insertions(+), 76 deletions(-) diff --git a/pkg/kubectl/cmd/resource/get.go b/pkg/kubectl/cmd/resource/get.go index ba2eea82318..1dd910218f0 100644 --- a/pkg/kubectl/cmd/resource/get.go +++ b/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 - original = infos[sorter.OriginalPosition(ix)].Object + info = infos[sorter.OriginalPosition(ix)] + mapping = info.Mapping + original = info.Object } else { - mapping = infos[ix].Mapping - original = infos[ix].Object + info = infos[ix] + 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 - decodedObj, _ := kubectl.DecodeUnknownObject(original) + typedObj := info.AsInternal() // 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 != nil { + if err := r.Err(); err != nil { return err } infos, err := r.Infos() diff --git a/pkg/kubectl/cmd/resource/get_test.go b/pkg/kubectl/cmd/resource/get_test.go index 02ecb2ae3b9..086d75cb610 100644 --- a/pkg/kubectl/cmd/resource/get_test.go +++ b/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) - f, tf, codec, _ := cmdtesting.NewAPIFactory() - tf.Printer = &testPrinter{GenericPrinter: test.genericPrinter} - tf.UnstructuredClient = &fake.RESTClient{ - GroupVersion: schema.GroupVersion{Version: "v1"}, - NegotiatedSerializer: unstructuredSerializer, - Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, test.resp)}, - } - tf.Namespace = "test" - buf := bytes.NewBuffer([]byte{}) - errBuf := bytes.NewBuffer([]byte{}) + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + f, tf, codec, _ := cmdtesting.NewAPIFactory() + tf.WithLegacyScheme() + tf.Printer = &testPrinter{GenericPrinter: test.genericPrinter} + tf.UnstructuredClient = &fake.RESTClient{ + GroupVersion: schema.GroupVersion{Version: "v1"}, + NegotiatedSerializer: unstructuredSerializer, + Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, test.resp)}, + } + tf.Namespace = "test" + buf := bytes.NewBuffer([]byte{}) + errBuf := bytes.NewBuffer([]byte{}) - cmd := NewCmdGet(f, buf, errBuf) - cmd.SetOutput(buf) - for k, v := range test.flags { - cmd.Flags().Lookup(k).Value.Set(v) - } - cmd.Run(cmd, test.args) + cmd := NewCmdGet(f, buf, errBuf) + cmd.SetOutput(buf) + for k, v := range test.flags { + cmd.Flags().Lookup(k).Value.Set(v) + } + 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 { - t.Errorf("%d: unexpected empty output", i) - } + if len(buf.String()) == 0 { + t.Errorf("%d: unexpected empty output", i) + } + }) } } diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index 633b447872f..bfb134b2c2b 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/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"}, + }, + }, + }, } } diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index 97faa3f25af..554fa4a6611 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/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 - Mapping *meta.RESTMapping + Client RESTClient + // 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 diff --git a/pkg/kubectl/resource_filter.go b/pkg/kubectl/resource_filter.go index a77cbf10126..b873748f688 100644 --- a/pkg/kubectl/resource_filter.go +++ b/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 -}