Merge pull request #67211 from juanvallejo/jvallejo/prototype-sorter

Automatic merge from submit-queue (batch tested with PRs 68051, 68130, 67211, 68065, 68117). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Update `kubectl get` sorter to deal with server-side printing

**Release note**:
```release-note
NONE
```

### Why?

Currently, we default to non-server-side printing when sorting items in `kubectl get`. This means that instead of taking advantage of having the server tell `kubectl` how to display information, `kubectl` falls back to using hardcoded resource types to figure out how to print its output. This does not really work with resources that `kubectl` does not know about, and it goes against our goal of snipping any dependencies that `kubectl` has on the core repo.

This patch adds a sorter capable of dealing with Table objects sent by the server when using "server-side printing".

A few things left to take care of:

- ~~[ ] When printing `all` resources, this implementation does not handle sorting every single Table object, but rather _only_ the rows in each object. As a result, output will contain sorted resources of the same _kind_, but the overall list of mixed resources will _not_ itself be sorted. Example:~~

```bash
$ kubectl get all --sort-by .metadata.name
NAME            READY     STATUS    RESTARTS   AGE
# pods here will be sorted:
pod/bar         0/2       Pending   0          31m
pod/foo         1/1       Running   0          37m

NAME                        DESIRED   CURRENT   READY     AGE
# replication controllers here will be sorted as well:
replicationcontroller/baz   1         1         1         37m
replicationcontroller/buz   1         1         1         37m

# ... but the overall mixed list of rc's and pods will not be sorted
```
This occurs because each Table object received from the server contains all rows for that resource _kind_. We would need a way to build an ambiguous Table object containing all rows for all objects regardless of their type to have a fully sorted mixed-object output.

- [ ] handle sorting by column-names, rather than _only_ with jsonpaths (Tracked in https://github.com/kubernetes/kubernetes/issues/68027)

cc @soltysh @kubernetes/sig-cli-maintainers @seans3 @mengqiy

hack/testdata/sorted-pods/sorted-pod1.yaml

@@ -2,9 +2,10 @@ apiVersion: v1
 kind: Pod
 metadata:
   name: sorted-pod1
+  creationTimestamp: 2018-08-30T14:10:58Z
   labels:
     name: sorted-pod3-label
 spec:
   containers:
-  - name: kubernetes-pause
+  - name: kubernetes-pause2
     image: k8s.gcr.io/pause:2.0

hack/testdata/sorted-pods/sorted-pod2.yaml

@@ -2,9 +2,10 @@ apiVersion: v1
 kind: Pod
 metadata:
   name: sorted-pod2
+  creationTimestamp: 2018-08-30T14:10:55Z
   labels:
     name: sorted-pod2-label
 spec:
   containers:
-  - name: kubernetes-pause
+  - name: kubernetes-pause1
     image: k8s.gcr.io/pause:2.0

hack/testdata/sorted-pods/sorted-pod3.yaml

@@ -2,9 +2,10 @@ apiVersion: v1
 kind: Pod
 metadata:
   name: sorted-pod3
+  creationTimestamp: 2018-08-30T14:10:53Z
   labels:
     name: sorted-pod1-label
 spec:
   containers:
-  - name: kubernetes-pause
+  - name: kubernetes-pause3
     image: k8s.gcr.io/pause:2.0

pkg/kubectl/BUILD

@@ -144,6 +144,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",

pkg/kubectl/cmd/get/BUILD

@@ -81,6 +81,7 @@ go_test(
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/api/extensions/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json:go_default_library",

pkg/kubectl/cmd/get/get.go

@@ -207,10 +207,10 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri
 
 	o.NoHeaders = cmdutil.GetFlagBool(cmd, "no-headers")
 
-	// TODO (soltysh): currently we don't support sorting and custom columns
+	// TODO (soltysh): currently we don't support custom columns
 	// with server side print. So in these cases force the old behavior.
 	outputOption := cmd.Flags().Lookup("output").Value.String()
-	if o.Sort && outputOption == "custom-columns" {
+	if outputOption == "custom-columns" {
 		o.ServerPrint = false
 	}
 
@@ -296,6 +296,96 @@ func (o *GetOptions) Validate(cmd *cobra.Command) error {
 	return nil
 }
 
+type OriginalPositioner interface {
+	OriginalPosition(int) int
+}
+
+type NopPositioner struct{}
+
+func (t *NopPositioner) OriginalPosition(ix int) int {
+	return ix
+}
+
+type RuntimeSorter struct {
+	field      string
+	decoder    runtime.Decoder
+	objects    []runtime.Object
+	positioner OriginalPositioner
+}
+
+func (r *RuntimeSorter) Sort() error {
+	if len(r.objects) <= 1 {
+		// a list is only considered "sorted" if there are 0 or 1 items in it
+		// AND (if 1 item) the item is not a Table object
+		_, isTable := r.objects[0].(*metav1beta1.Table)
+		if len(r.objects) == 0 || !isTable {
+			return nil
+		}
+	}
+
+	includesTable := false
+	includesRuntimeObjs := false
+
+	for _, obj := range r.objects {
+		switch t := obj.(type) {
+		case *metav1beta1.Table:
+			includesTable = true
+
+			if err := kubectl.NewTableSorter(t, r.field).Sort(); err != nil {
+				continue
+			}
+		default:
+			includesRuntimeObjs = true
+		}
+	}
+
+	// we use a NopPositioner when dealing with Table objects
+	// because the objects themselves are not swapped, but rather
+	// the rows in each object are swapped / sorted.
+	r.positioner = &NopPositioner{}
+
+	if includesRuntimeObjs && includesTable {
+		return fmt.Errorf("sorting is not supported on mixed Table and non-Table object lists")
+	}
+	if includesTable {
+		return nil
+	}
+
+	// if not dealing with a Table response from the server, assume
+	// all objects are runtime.Object as usual, and sort using old method.
+	var err error
+	if r.positioner, err = kubectl.SortObjects(r.decoder, r.objects, r.field); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (r *RuntimeSorter) OriginalPosition(ix int) int {
+	if r.positioner == nil {
+		return 0
+	}
+	return r.positioner.OriginalPosition(ix)
+}
+
+// allows custom decoder to be set for testing
+func (r *RuntimeSorter) WithDecoder(decoder runtime.Decoder) *RuntimeSorter {
+	r.decoder = decoder
+	return r
+}
+
+func NewRuntimeSorter(objects []runtime.Object, sortBy string) *RuntimeSorter {
+	parsedField, err := printers.RelaxedJSONPathExpression(sortBy)
+	if err != nil {
+		parsedField = sortBy
+	}
+
+	return &RuntimeSorter{
+		field:   parsedField,
+		decoder: cmdutil.InternalVersionDecoder(),
+		objects: objects,
+	}
+}
+
 // Run performs the get operation.
 // TODO: remove the need to pass these arguments, like other commands.
 func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
@@ -311,6 +401,13 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
 		fmt.Fprintf(o.IOStreams.ErrOut, "warning: --%s requested, --%s will be ignored\n", useOpenAPIPrintColumnFlagLabel, useServerPrintColumns)
 	}
 
+	chunkSize := o.ChunkSize
+	if o.Sort {
+		// TODO(juanvallejo): in the future, we could have the client use chunking
+		// to gather all results, then sort them all at the end to reduce server load.
+		chunkSize = 0
+	}
+
 	r := f.NewBuilder().
 		Unstructured().
 		NamespaceParam(o.Namespace).DefaultNamespace().AllNamespaces(o.AllNamespaces).
@@ -318,7 +415,7 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
 		LabelSelectorParam(o.LabelSelector).
 		FieldSelectorParam(o.FieldSelector).
 		ExportParam(o.Export).
-		RequestChunksOf(o.ChunkSize).
+		RequestChunksOf(chunkSize).
 		IncludeUninitialized(o.IncludeUninitialized).
 		ResourceTypeOrNameArgs(true, args...).
 		ContinueOnError().
@@ -329,12 +426,19 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
 			if o.PrintWithOpenAPICols {
 				return
 			}
-			if o.ServerPrint && o.IsHumanReadablePrinter && !o.Sort {
+			if !o.ServerPrint || !o.IsHumanReadablePrinter {
+				return
+			}
+
 			group := metav1beta1.GroupName
 			version := metav1beta1.SchemeGroupVersion.Version
 
 			tableParam := fmt.Sprintf("application/json;as=Table;v=%s;g=%s, application/json", version, group)
 			req.SetHeader("Accept", tableParam)
+
+			// if sorting, ensure we receive the full object in order to introspect its fields via jsonpath
+			if o.Sort {
+				req.Param("includeObject", "Object")
 			}
 		}).
 		Do()
@@ -378,12 +482,14 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
 	if err != nil {
 		return err
 	}
-	var sorter *kubectl.RuntimeSort
+
-	if o.Sort && len(objs) > 1 {
+	var positioner OriginalPositioner
-		// TODO: questionable
+	if o.Sort {
-		if sorter, err = kubectl.SortObjects(cmdutil.InternalVersionDecoder(), objs, sorting); err != nil {
+		sorter := NewRuntimeSorter(objs, sorting)
+		if err := sorter.Sort(); err != nil {
 			return err
 		}
+		positioner = sorter
 	}
 
 	var printer printers.ResourcePrinter
@@ -393,8 +499,8 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
 	for ix := range objs {
 		var mapping *meta.RESTMapping
 		var info *resource.Info
-		if sorter != nil {
+		if positioner != nil {
-			info = infos[sorter.OriginalPosition(ix)]
+			info = infos[positioner.OriginalPosition(ix)]
 			mapping = info.Mapping
 		} else {
 			info = infos[ix]

pkg/kubectl/cmd/get/get_test.go

@@ -19,6 +19,7 @@ package get
 import (
 	"bytes"
 	encjson "encoding/json"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -34,6 +35,7 @@ import (
 	api "k8s.io/api/core/v1"
 	apiextensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer/json"
@@ -526,6 +528,150 @@ c         0/0                 0          <unknown>
 	}
 }
 
+func sortTestData() []runtime.Object {
+	return []runtime.Object{
+		&api.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "test", ResourceVersion: "10"},
+			Spec:       apitesting.V1DeepEqualSafePodSpec(),
+		},
+		&api.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "test", ResourceVersion: "11"},
+			Spec:       apitesting.V1DeepEqualSafePodSpec(),
+		},
+		&api.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "test", ResourceVersion: "9"},
+			Spec:       apitesting.V1DeepEqualSafePodSpec(),
+		},
+	}
+}
+
+func sortTestTableData() []runtime.Object {
+	return []runtime.Object{
+		&metav1beta1.Table{
+			TypeMeta: metav1.TypeMeta{Kind: "Table"},
+			Rows: []metav1beta1.TableRow{
+				{
+					Object: runtime.RawExtension{
+						Object: &api.Pod{
+							ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "test", ResourceVersion: "10"},
+							Spec:       apitesting.V1DeepEqualSafePodSpec(),
+						},
+					},
+				},
+				{
+					Object: runtime.RawExtension{
+						Object: &api.Pod{
+							ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "test", ResourceVersion: "11"},
+							Spec:       apitesting.V1DeepEqualSafePodSpec(),
+						},
+					},
+				},
+				{
+					Object: runtime.RawExtension{
+						Object: &api.Pod{
+							ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "test", ResourceVersion: "9"},
+							Spec:       apitesting.V1DeepEqualSafePodSpec(),
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+func TestRuntimeSorter(t *testing.T) {
+	tests := []struct {
+		name        string
+		field       string
+		objs        []runtime.Object
+		op          func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error
+		expect      string
+		expectError string
+	}{
+		{
+			name:  "ensure sorter returns original position",
+			field: "metadata.name",
+			objs:  sortTestData(),
+			op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error {
+				for idx := range objs {
+					p := sorter.OriginalPosition(idx)
+					fmt.Fprintf(out, "%v,", p)
+				}
+				return nil
+			},
+			expect: "2,1,0,",
+		},
+		{
+			name:  "ensure sorter handles table object position",
+			field: "metadata.name",
+			objs:  sortTestTableData(),
+			op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error {
+				for idx := range objs {
+					p := sorter.OriginalPosition(idx)
+					fmt.Fprintf(out, "%v,", p)
+				}
+				return nil
+			},
+			expect: "0,",
+		},
+		{
+			name:  "ensure sorter sorts table objects",
+			field: "metadata.name",
+			objs:  sortTestData(),
+			op: func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error {
+				for _, o := range objs {
+					fmt.Fprintf(out, "%s,", o.(*api.Pod).Name)
+				}
+				return nil
+			},
+			expect: "a,b,c,",
+		},
+		{
+			name:        "ensure sorter rejects mixed Table + non-Table object lists",
+			field:       "metadata.name",
+			objs:        append(sortTestData(), sortTestTableData()...),
+			op:          func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { return nil },
+			expectError: "sorting is not supported on mixed Table",
+		},
+		{
+			name:        "ensure sorter errors out on invalid jsonpath",
+			field:       "metadata.unknown",
+			objs:        sortTestData(),
+			op:          func(sorter *RuntimeSorter, objs []runtime.Object, out io.Writer) error { return nil },
+			expectError: "couldn't find any field with path",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			sorter := NewRuntimeSorter(tc.objs, tc.field)
+			if err := sorter.Sort(); err != nil {
+				if len(tc.expectError) > 0 && strings.Contains(err.Error(), tc.expectError) {
+					return
+				}
+
+				if len(tc.expectError) > 0 {
+					t.Fatalf("unexpected error: expecting %s, but got %s", tc.expectError, err)
+				}
+
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			out := bytes.NewBuffer([]byte{})
+			err := tc.op(sorter, tc.objs, out)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			if tc.expect != out.String() {
+				t.Fatalf("unexpected output: expecting %s, but got %s", tc.expect, out.String())
+			}
+
+		})
+	}
+
+}
+
 func TestGetObjectsIdentifiedByFile(t *testing.T) {
 	pods, _, _ := testData()
 

pkg/kubectl/sorter.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/util/integer"
 	"k8s.io/client-go/util/jsonpath"
@@ -111,12 +112,7 @@ func SortObjects(decoder runtime.Decoder, objs []runtime.Object, fieldInput stri
 	// Note that this requires empty fields to be considered later, when sorting.
 	var fieldFoundOnce bool
 	for _, obj := range objs {
-		var values [][]reflect.Value
+		values, err := findJSONPathResults(parser, obj)
-		if unstructured, ok := obj.(*unstructured.Unstructured); ok {
-			values, err = parser.FindResults(unstructured.Object)
-		} else {
-			values, err = parser.FindResults(reflect.ValueOf(obj).Elem().Interface())
-		}
 		if err != nil {
 			return nil, err
 		}
@@ -274,20 +270,12 @@ func (r *RuntimeSort) Less(i, j int) bool {
 		panic(err)
 	}
 
-	if unstructured, ok := iObj.(*unstructured.Unstructured); ok {
+	iValues, err = findJSONPathResults(parser, iObj)
-		iValues, err = parser.FindResults(unstructured.Object)
-	} else {
-		iValues, err = parser.FindResults(reflect.ValueOf(iObj).Elem().Interface())
-	}
 	if err != nil {
 		glog.Fatalf("Failed to get i values for %#v using %s (%#v)", iObj, r.field, err)
 	}
 
-	if unstructured, ok := jObj.(*unstructured.Unstructured); ok {
+	jValues, err = findJSONPathResults(parser, jObj)
-		jValues, err = parser.FindResults(unstructured.Object)
-	} else {
-		jValues, err = parser.FindResults(reflect.ValueOf(jObj).Elem().Interface())
-	}
 	if err != nil {
 		glog.Fatalf("Failed to get j values for %#v using %s (%v)", jObj, r.field, err)
 	}
@@ -316,3 +304,77 @@ func (r *RuntimeSort) OriginalPosition(ix int) int {
 	}
 	return r.origPosition[ix]
 }
+
+type TableSorter struct {
+	field string
+	obj   *metav1beta1.Table
+}
+
+func (t *TableSorter) Len() int {
+	return len(t.obj.Rows)
+}
+
+func (t *TableSorter) Swap(i, j int) {
+	t.obj.Rows[i], t.obj.Rows[j] = t.obj.Rows[j], t.obj.Rows[i]
+}
+
+func (t *TableSorter) Less(i, j int) bool {
+	iObj := t.obj.Rows[i].Object.Object
+	jObj := t.obj.Rows[j].Object.Object
+
+	var iValues [][]reflect.Value
+	var jValues [][]reflect.Value
+	var err error
+
+	parser := jsonpath.New("sorting").AllowMissingKeys(true)
+	err = parser.Parse(t.field)
+	if err != nil {
+		glog.Fatalf("sorting error: %v\n", err)
+	}
+
+	// TODO(juanvallejo): this is expensive for very large sets.
+	// To improve runtime complexity, build an array which contains all
+	// resolved fields, and sort that instead.
+	iValues, err = findJSONPathResults(parser, iObj)
+	if err != nil {
+		glog.Fatalf("Failed to get i values for %#v using %s (%#v)", iObj, t.field, err)
+	}
+
+	jValues, err = findJSONPathResults(parser, jObj)
+	if err != nil {
+		glog.Fatalf("Failed to get j values for %#v using %s (%v)", jObj, t.field, err)
+	}
+
+	if len(iValues) == 0 || len(iValues[0]) == 0 || len(jValues) == 0 || len(jValues[0]) == 0 {
+		glog.Fatalf("couldn't find any field with path %q in the list of objects", t.field)
+	}
+
+	iField := iValues[0][0]
+	jField := jValues[0][0]
+
+	less, err := isLess(iField, jField)
+	if err != nil {
+		glog.Fatalf("Field %s in %T is an unsortable type: %s, err: %v", t.field, iObj, iField.Kind().String(), err)
+	}
+	return less
+}
+
+func (t *TableSorter) Sort() error {
+	sort.Sort(t)
+	return nil
+}
+
+func NewTableSorter(table *metav1beta1.Table, field string) *TableSorter {
+	return &TableSorter{
+		obj:   table,
+		field: field,
+	}
+}
+
+func findJSONPathResults(parser *jsonpath.JSONPath, from runtime.Object) ([][]reflect.Value, error) {
+	if unstructuredObj, ok := from.(*unstructured.Unstructured); ok {
+		return parser.FindResults(unstructuredObj.Object)
+	}
+
+	return parser.FindResults(reflect.ValueOf(from).Elem().Interface())
+}

test/cmd/get.sh

@@ -240,6 +240,11 @@ run_kubectl_sort_by_tests() {
   # Check output of sort-by
   output_message=$(kubectl get pods --sort-by="{metadata.name}")
   kube::test::if_has_string "${output_message}" "valid-pod"
+  # ensure sort-by receivers objects as Table
+  output_message=$(kubectl get pods --v=8 --sort-by="{metadata.name}" 2>&1)
+  kube::test::if_has_string "${output_message}" "as=Table"
+  # ensure sort-by requests the full object
+  kube::test::if_has_string "${output_message}" "includeObject=Object"
   ### Clean up
   # Pre-condition: valid-pod exists
   kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'valid-pod:'
@@ -273,6 +278,19 @@ run_kubectl_sort_by_tests() {
   output_message=$(kubectl get pods --sort-by="{metadata.labels.name}")
   kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod3:sorted-pod2:sorted-pod1:"
 
+  # if sorting, we should be able to use any field in our objects
+  output_message=$(kubectl get pods --sort-by="{spec.containers[0].name}")
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod2:sorted-pod1:sorted-pod3:"
+
+  # ensure sorting by creation timestamps works
+  output_message=$(kubectl get pods --sort-by="{metadata.creationTimestamp}")
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod1:sorted-pod2:sorted-pod3:"
+
+  # ensure sorting using fallback codepath still works
+  output_message=$(kubectl get pods --sort-by="{spec.containers[0].name}" --server-print=false --v=8 2>&1)
+  kube::test::if_sort_by_has_correct_order "${output_message}" "sorted-pod2:sorted-pod1:sorted-pod3:"
+  kube::test::if_has_not_string "${output_message}" "Table"
+
   ### Clean up
   # Pre-condition: valid-pod exists
   kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'sorted-pod1:sorted-pod2:sorted-pod3:'