Denote if a printer is generic.

This fixes kubernetes#38779. This allows us to avoid case in which printers.GetStandardPrinter returns nil for both printer and err removing any potential panics that may arise throughout kubectl commands. Please see kubernetes#38779 and kubernetes#38112 for complete context. Add comment explaining adding handlers to printers.HumanReadablePrinter also remove an unnecessary conversion of printers.HumanReadablePrinter to printers.ResourcePrinter.
kubermatic · Jun 6, 2017 · 8912a6e · 8912a6e
1 parent 4205fd7
commit 8912a6e
Show file tree

Hide file tree

Showing 24 changed files with 182 additions and 96 deletions.
diff --git a/pkg/kubectl/cmd/apply_test.go b/pkg/kubectl/cmd/apply_test.go
@@ -456,8 +456,7 @@ func TestApplyObjectOutput(t *testing.T) {
 	}
 
 	f, tf, _, _ := cmdtesting.NewAPIFactory()
-	tf.CommandPrinter = &printers.YAMLPrinter{}
-	tf.GenericPrinter = true
+	tf.Printer = &printers.YAMLPrinter{}
 	tf.UnstructuredClient = &fake.RESTClient{
 		APIRegistry:          api.Registry,
 		NegotiatedSerializer: unstructuredSerializer,

diff --git a/pkg/kubectl/cmd/cmd_test.go b/pkg/kubectl/cmd/cmd_test.go
@@ -80,8 +80,9 @@ func defaultClientConfigForVersion(version *schema.GroupVersion) *restclient.Con
 }
 
 type testPrinter struct {
-	Objects []runtime.Object
-	Err     error
+	Objects        []runtime.Object
+	Err            error
+	GenericPrinter bool
 }
 
 func (t *testPrinter) PrintObj(obj runtime.Object, out io.Writer) error {
@@ -99,6 +100,10 @@ func (t *testPrinter) AfterPrint(output io.Writer, res string) error {
 	return nil
 }
 
+func (t *testPrinter) IsGeneric() bool {
+	return t.GenericPrinter
+}
+
 type testDescriber struct {
 	Name, Namespace string
 	Settings        printers.DescriberSettings

diff --git a/pkg/kubectl/cmd/config/view.go b/pkg/kubectl/cmd/config/view.go
@@ -81,7 +81,7 @@ func NewCmdConfigView(out, errOut io.Writer, ConfigAccess clientcmd.ConfigAccess
 				cmd.Flags().Set("output", defaultOutputFormat)
 			}
 
-			printer, _, err := cmdutil.PrinterForCommand(cmd, meta.NewDefaultRESTMapper(nil, nil), latest.Scheme, []runtime.Decoder{latest.Codec})
+			printer, err := cmdutil.PrinterForCommand(cmd, meta.NewDefaultRESTMapper(nil, nil), latest.Scheme, nil, []runtime.Decoder{latest.Codec}, printers.PrintOptions{})
 			cmdutil.CheckErr(err)
 			printer = printers.NewVersionedPrinter(printer, latest.Scheme, latest.ExternalVersion)
 

diff --git a/pkg/kubectl/cmd/convert.go b/pkg/kubectl/cmd/convert.go
@@ -164,7 +164,7 @@ func (o *ConvertOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.C
 		cmd.Flags().Set("output", outputFormat)
 	}
 	o.encoder = f.JSONEncoder()
-	o.printer, _, err = f.PrinterForCommand(cmd)
+	o.printer, err = f.PrinterForCommand(cmd, printers.PrintOptions{})
 	if err != nil {
 		return err
 	}

diff --git a/pkg/kubectl/cmd/create_clusterrole_test.go b/pkg/kubectl/cmd/create_clusterrole_test.go
@@ -46,6 +46,10 @@ func (t *testClusterRolePrinter) HandledResources() []string {
 	return []string{}
 }
 
+func (t *testClusterRolePrinter) IsGeneric() bool {
+	return true
+}
+
 func TestCreateClusterRole(t *testing.T) {
 	clusterRoleName := "my-cluster-role"
 

diff --git a/pkg/kubectl/cmd/create_role_test.go b/pkg/kubectl/cmd/create_role_test.go
@@ -46,6 +46,10 @@ func (t *testRolePrinter) HandledResources() []string {
 	return []string{}
 }
 
+func (t *testRolePrinter) IsGeneric() bool {
+	return true
+}
+
 func TestCreateRole(t *testing.T) {
 	roleName := "my-role"
 

diff --git a/pkg/kubectl/cmd/get.go b/pkg/kubectl/cmd/get.go
@@ -302,7 +302,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 		return err
 	}
 
-	printer, generic, err := f.PrinterForCommand(cmd)
+	printer, err := f.PrinterForCommand(cmd, printers.PrintOptions{})
 	if err != nil {
 		return err
 	}
@@ -313,7 +313,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 		r.IgnoreErrors(kapierrors.IsNotFound)
 	}
 
-	if generic {
+	if printer.IsGeneric() {
 		// we flattened the data from the builder, so we have individual items, but now we'd like to either:
 		// 1. if there is more than one item, combine them all into a single list
 		// 2. if there is a single item and that item is a list, leave it as its specific list
@@ -436,7 +436,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 			mapping = infos[ix].Mapping
 			original = infos[ix].Object
 		}
-		if printer == nil || lastMapping == nil || mapping == nil || mapping.Resource != lastMapping.Resource {
+		if shouldGetNewPrinterForMapping(printer, lastMapping, mapping) {
 			if printer != nil {
 				w.Flush()
 			}
@@ -513,3 +513,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 	cmdutil.PrintFilterCount(errOut, len(objs), filteredResourceCount, len(allErrs), filterOpts, options.IgnoreNotFound)
 	return utilerrors.NewAggregate(allErrs)
 }
+
+func shouldGetNewPrinterForMapping(printer printers.ResourcePrinter, lastMapping, mapping *meta.RESTMapping) bool {
+	return printer == nil || lastMapping == nil || mapping == nil || mapping.Resource != lastMapping.Resource
+}
diff --git a/pkg/kubectl/cmd/get_test.go b/pkg/kubectl/cmd/get_test.go
@@ -220,15 +220,16 @@ func TestGetObjectsFiltered(t *testing.T) {
 	second := &pods.Items[1]
 
 	testCases := []struct {
-		args   []string
-		resp   runtime.Object
-		flags  map[string]string
-		expect []runtime.Object
+		args           []string
+		resp           runtime.Object
+		flags          map[string]string
+		expect         []runtime.Object
+		genericPrinter bool
 	}{
-		{args: []string{"pods", "foo"}, resp: first, expect: []runtime.Object{first}},
-		{args: []string{"pods", "foo"}, flags: map[string]string{"show-all": "false"}, resp: first, expect: []runtime.Object{first}},
+		{args: []string{"pods", "foo"}, resp: first, expect: []runtime.Object{first}, genericPrinter: true},
+		{args: []string{"pods", "foo"}, flags: map[string]string{"show-all": "false"}, resp: first, expect: []runtime.Object{first}, genericPrinter: true},
 		{args: []string{"pods"}, flags: map[string]string{"show-all": "true"}, resp: pods, expect: []runtime.Object{first, second}},
-		{args: []string{"pods/foo"}, resp: first, expect: []runtime.Object{first}},
+		{args: []string{"pods/foo"}, resp: first, expect: []runtime.Object{first}, genericPrinter: true},
 		{args: []string{"pods"}, flags: map[string]string{"output": "yaml"}, resp: pods, expect: []runtime.Object{second}},
 		{args: []string{}, flags: map[string]string{"filename": "../../../examples/storage/cassandra/cassandra-controller.yaml"}, resp: pods, expect: []runtime.Object{first, second}},
 
@@ -240,7 +241,7 @@ func TestGetObjectsFiltered(t *testing.T) {
 	for i, test := range testCases {
 		t.Logf("%d", i)
 		f, tf, codec, _ := cmdtesting.NewAPIFactory()
-		tf.Printer = &testPrinter{}
+		tf.Printer = &testPrinter{GenericPrinter: test.genericPrinter}
 		tf.UnstructuredClient = &fake.RESTClient{
 			APIRegistry:          api.Registry,
 			NegotiatedSerializer: unstructuredSerializer,
@@ -281,7 +282,7 @@ func TestGetObjectIgnoreNotFound(t *testing.T) {
 	}
 
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
-	tf.Printer = &testPrinter{}
+	tf.Printer = &testPrinter{GenericPrinter: true}
 	tf.UnstructuredClient = &fake.RESTClient{
 		APIRegistry:          api.Registry,
 		NegotiatedSerializer: unstructuredSerializer,
@@ -393,7 +394,7 @@ func TestGetObjectsIdentifiedByFile(t *testing.T) {
 	pods, _, _ := testData()
 
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
-	tf.Printer = &testPrinter{}
+	tf.Printer = &testPrinter{GenericPrinter: true}
 	tf.UnstructuredClient = &fake.RESTClient{
 		APIRegistry:          api.Registry,
 		NegotiatedSerializer: unstructuredSerializer,
@@ -561,8 +562,7 @@ func TestGetMultipleTypeObjectsAsList(t *testing.T) {
 	pods, svc, _ := testData()
 
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
-	tf.CommandPrinter = &testPrinter{}
-	tf.GenericPrinter = true
+	tf.Printer = &testPrinter{GenericPrinter: true}
 	tf.UnstructuredClient = &fake.RESTClient{
 		APIRegistry:          api.Registry,
 		NegotiatedSerializer: unstructuredSerializer,
@@ -589,7 +589,7 @@ func TestGetMultipleTypeObjectsAsList(t *testing.T) {
 	cmd.Flags().Set("output", "json")
 	cmd.Run(cmd, []string{"pods,services"})
 
-	actual := tf.CommandPrinter.(*testPrinter).Objects
+	actual := tf.Printer.(*testPrinter).Objects
 	fn := func(obj runtime.Object) unstructured.Unstructured {
 		data, err := runtime.Encode(api.Codecs.LegacyCodec(schema.GroupVersion{Version: "v1"}), obj)
 		if err != nil {
@@ -716,7 +716,7 @@ func TestGetByFormatForcesFlag(t *testing.T) {
 	pods, _, _ := testData()
 
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
-	tf.Printer = &testPrinter{}
+	tf.Printer = &testPrinter{GenericPrinter: true}
 	tf.UnstructuredClient = &fake.RESTClient{
 		APIRegistry:          api.Registry,
 		NegotiatedSerializer: unstructuredSerializer,

diff --git a/pkg/kubectl/cmd/patch_test.go b/pkg/kubectl/cmd/patch_test.go
@@ -161,8 +161,7 @@ func TestPatchObjectFromFileOutput(t *testing.T) {
 	svcCopy.Labels["post-patch"] = "post-patch-value"
 
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
-	tf.CommandPrinter = &printers.YAMLPrinter{}
-	tf.GenericPrinter = true
+	tf.Printer = &printers.YAMLPrinter{}
 	tf.UnstructuredClient = &fake.RESTClient{
 		APIRegistry:          api.Registry,
 		NegotiatedSerializer: unstructuredSerializer,

diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go
@@ -221,13 +221,11 @@ type TestFactory struct {
 	UnstructuredClient kubectl.RESTClient
 	Describer          printers.Describer
 	Printer            printers.ResourcePrinter
-	CommandPrinter     printers.ResourcePrinter
 	Validator          validation.Schema
 	Namespace          string
 	ClientConfig       *restclient.Config
 	Err                error
 	Command            string
-	GenericPrinter     bool
 	TmpDir             string
 
 	ClientForMappingFunc             func(mapping *meta.RESTMapping) (resource.RESTClient, error)
@@ -338,8 +336,8 @@ func (f *FakeFactory) Describer(*meta.RESTMapping) (printers.Describer, error) {
 	return f.tf.Describer, f.tf.Err
 }
 
-func (f *FakeFactory) PrinterForCommand(cmd *cobra.Command) (printers.ResourcePrinter, bool, error) {
-	return f.tf.CommandPrinter, f.tf.GenericPrinter, f.tf.Err
+func (f *FakeFactory) PrinterForCommand(cmd *cobra.Command, options printers.PrintOptions) (printers.ResourcePrinter, error) {
+	return f.tf.Printer, f.tf.Err
 }
 
 func (f *FakeFactory) Printer(mapping *meta.RESTMapping, options printers.PrintOptions) (printers.ResourcePrinter, error) {
@@ -619,8 +617,8 @@ func (f *fakeAPIFactory) UnstructuredClientForMapping(m *meta.RESTMapping) (reso
 	return f.tf.UnstructuredClient, f.tf.Err
 }
 
-func (f *fakeAPIFactory) PrinterForCommand(cmd *cobra.Command) (printers.ResourcePrinter, bool, error) {
-	return f.tf.CommandPrinter, f.tf.GenericPrinter, f.tf.Err
+func (f *fakeAPIFactory) PrinterForCommand(cmd *cobra.Command, options printers.PrintOptions) (printers.ResourcePrinter, error) {
+	return f.tf.Printer, f.tf.Err
 }
 
 func (f *fakeAPIFactory) Describer(*meta.RESTMapping) (printers.Describer, error) {

diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go
@@ -230,10 +230,10 @@ type ObjectMappingFactory interface {
 // Generally they depend upon client mapper functions
 type BuilderFactory interface {
 	// PrinterForCommand returns the default printer for the command. It requires that certain options
-	// are declared on the command (see AddPrinterFlags). Returns a printer, true if the printer is
-	// generic (is not internal), or an error if a printer could not be found.
+	// are declared on the command (see AddPrinterFlags). Returns a printer, or an error if a printer
+	// could not be found.
 	// TODO: Break the dependency on cmd here.
-	PrinterForCommand(cmd *cobra.Command) (printers.ResourcePrinter, bool, error)
+	PrinterForCommand(cmd *cobra.Command, options printers.PrintOptions) (printers.ResourcePrinter, error)
 	// PrinterForMapping returns a printer suitable for displaying the provided resource type.
 	// Requires that printer flags have been added to cmd (see AddPrinterFlags).
 	PrinterForMapping(cmd *cobra.Command, mapping *meta.RESTMapping, withNamespace bool) (printers.ResourcePrinter, error)

diff --git a/pkg/kubectl/cmd/util/factory_builder.go b/pkg/kubectl/cmd/util/factory_builder.go
@@ -31,6 +31,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/plugins"
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 	"k8s.io/kubernetes/pkg/printers"
+	printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
 )
 
 type ring2Factory struct {
@@ -47,24 +48,41 @@ func NewBuilderFactory(clientAccessFactory ClientAccessFactory, objectMappingFac
 	return f
 }
 
-func (f *ring2Factory) PrinterForCommand(cmd *cobra.Command) (printers.ResourcePrinter, bool, error) {
+func (f *ring2Factory) PrinterForCommand(cmd *cobra.Command, options printers.PrintOptions) (printers.ResourcePrinter, error) {
 	mapper, typer, err := f.objectMappingFactory.UnstructuredObject()
 	if err != nil {
-		return nil, false, err
+		return nil, err
 	}
 	// TODO: used by the custom column implementation and the name implementation, break this dependency
 	decoders := []runtime.Decoder{f.clientAccessFactory.Decoder(true), unstructured.UnstructuredJSONScheme}
-	return PrinterForCommand(cmd, mapper, typer, decoders)
+	encoder := f.clientAccessFactory.JSONEncoder()
+	return PrinterForCommand(cmd, mapper, typer, encoder, decoders, options)
 }
 
 func (f *ring2Factory) PrinterForMapping(cmd *cobra.Command, mapping *meta.RESTMapping, withNamespace bool) (printers.ResourcePrinter, error) {
-	printer, generic, err := f.PrinterForCommand(cmd)
+	// Some callers do not have "label-columns" so we can't use the GetFlagStringSlice() helper
+	columnLabel, err := cmd.Flags().GetStringSlice("label-columns")
+	if err != nil {
+		columnLabel = []string{}
+	}
+
+	options := printers.PrintOptions{
+		NoHeaders:          GetFlagBool(cmd, "no-headers"),
+		WithNamespace:      withNamespace,
+		Wide:               GetWideFlag(cmd),
+		ShowAll:            GetFlagBool(cmd, "show-all"),
+		ShowLabels:         GetFlagBool(cmd, "show-labels"),
+		AbsoluteTimestamps: isWatch(cmd),
+		ColumnLabels:       columnLabel,
+	}
+
+	printer, err := f.PrinterForCommand(cmd, options)
 	if err != nil {
 		return nil, err
 	}
 
 	// Make sure we output versioned data for generic printers
-	if generic {
+	if printer.IsGeneric() {
 		if mapping == nil {
 			return nil, fmt.Errorf("no serialization format found")
 		}
@@ -74,25 +92,17 @@ func (f *ring2Factory) PrinterForMapping(cmd *cobra.Command, mapping *meta.RESTM
 		}
 
 		printer = printers.NewVersionedPrinter(printer, mapping.ObjectConvertor, version, mapping.GroupVersionKind.GroupVersion())
+
 	} else {
-		// Some callers do not have "label-columns" so we can't use the GetFlagStringSlice() helper
-		columnLabel, err := cmd.Flags().GetStringSlice("label-columns")
-		if err != nil {
-			columnLabel = []string{}
-		}
-		printer, err = f.clientAccessFactory.Printer(mapping, printers.PrintOptions{
-			NoHeaders:          GetFlagBool(cmd, "no-headers"),
-			WithNamespace:      withNamespace,
-			Wide:               GetWideFlag(cmd),
-			ShowAll:            GetFlagBool(cmd, "show-all"),
-			ShowLabels:         GetFlagBool(cmd, "show-labels"),
-			AbsoluteTimestamps: isWatch(cmd),
-			ColumnLabels:       columnLabel,
-		})
-		if err != nil {
-			return nil, err
+		// We add handlers to the printer in case it is printers.HumanReadablePrinter.
+		// printers.AddHandlers expects concrete type of printers.HumanReadablePrinter
+		// as its parameter because of this we have to do a type check on printer and
+		// extract out concrete HumanReadablePrinter from it. We are then able to attach
+		// handlers on it.
+		if humanReadablePrinter, ok := printer.(*printers.HumanReadablePrinter); ok {
+			printersinternal.AddHandlers(humanReadablePrinter)
+			printer = humanReadablePrinter
 		}
-		printer = maybeWrapSortingPrinter(cmd, printer)
 	}
 
 	return printer, nil

diff --git a/pkg/kubectl/cmd/util/printing.go b/pkg/kubectl/cmd/util/printing.go
@@ -107,7 +107,7 @@ func ValidateOutputArgs(cmd *cobra.Command) error {
 
 // PrinterForCommand returns the default printer for this command.
 // Requires that printer flags have been added to cmd (see AddPrinterFlags).
-func PrinterForCommand(cmd *cobra.Command, mapper meta.RESTMapper, typer runtime.ObjectTyper, decoders []runtime.Decoder) (printers.ResourcePrinter, bool, error) {
+func PrinterForCommand(cmd *cobra.Command, mapper meta.RESTMapper, typer runtime.ObjectTyper, encoder runtime.Encoder, decoders []runtime.Decoder, options printers.PrintOptions) (printers.ResourcePrinter, error) {
 	outputFormat := GetFlagString(cmd, "output")
 
 	// templates are logically optional for specifying a format.
@@ -133,27 +133,27 @@ func PrinterForCommand(cmd *cobra.Command, mapper meta.RESTMapper, typer runtime
 	if cmd.Flags().Lookup("allow-missing-template-keys") != nil {
 		allowMissingTemplateKeys = GetFlagBool(cmd, "allow-missing-template-keys")
 	}
-	printer, generic, err := printers.GetStandardPrinter(
+	printer, err := printers.GetStandardPrinter(
 		outputFormat, templateFile, GetFlagBool(cmd, "no-headers"), allowMissingTemplateKeys,
-		mapper, typer, decoders,
+		mapper, typer, encoder, decoders, options,
 	)
 	if err != nil {
-		return nil, generic, err
+		return nil, err
 	}
 
-	return maybeWrapSortingPrinter(cmd, printer), generic, nil
+	return maybeWrapSortingPrinter(cmd, printer), nil
 }
 
 // PrintResourceInfoForCommand receives a *cobra.Command and a *resource.Info and
 // attempts to print an info object based on the specified output format. If the
 // object passed is non-generic, it attempts to print the object using a HumanReadablePrinter.
 // Requires that printer flags have been added to cmd (see AddPrinterFlags).
 func PrintResourceInfoForCommand(cmd *cobra.Command, info *resource.Info, f Factory, out io.Writer) error {
-	printer, generic, err := f.PrinterForCommand(cmd)
+	printer, err := f.PrinterForCommand(cmd, printers.PrintOptions{})
 	if err != nil {
 		return err
 	}
-	if !generic || printer == nil {
+	if !printer.IsGeneric() {
 		printer, err = f.PrinterForMapping(cmd, nil, false)
 		if err != nil {
 			return err

diff --git a/pkg/kubectl/sorting_printer.go b/pkg/kubectl/sorting_printer.go
@@ -60,10 +60,14 @@ func (s *SortingPrinter) PrintObj(obj runtime.Object, out io.Writer) error {
 }
 
 // TODO: implement HandledResources()
-func (p *SortingPrinter) HandledResources() []string {
+func (s *SortingPrinter) HandledResources() []string {
 	return []string{}
 }
 
+func (s *SortingPrinter) IsGeneric() bool {
+	return s.Delegate.IsGeneric()
+}
+
 func (s *SortingPrinter) sortObj(obj runtime.Object) error {
 	objs, err := meta.ExtractList(obj)
 	if err != nil {