From c1805696ce7395bf099dbedb9d6d2c16f782a2eb Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Mon, 27 Jan 2025 11:55:09 -0800 Subject: [PATCH] SQLite backed cache: Support sorting mgmt clusters on value in a specific condition (#447) * Replace primary/secondary sort fields with an array of sort directives. * Allow more than 2 sort-params in a search query. * Add a virtual 'status.ready' field to clusters. * Rename status.ready -> status.connected * Set virtual field 'spec.internal' <- spec.displayName == 'local' * Need to declare all virtual fields to index. * Ready clusters have condition[type==Ready && status=True] * Update the README to reflect generalized sorting. * Bump lasso to get revised sort directives. * Review-driven changes, mostly comments and drop unneeded code. * Add unit tests to verify sort-order stringification. * Ignore empty-string sort components. * Fix a rebase mishap. * Drop unneeded commented-out code. * Clusters have a 'spec.internal' field, no need to synthesize one. * Added a note on square-brackets for label references. This should be added to the README for filter queries in the PR for 46333. * Bump to latest sqlcache-free lasso --- README.md | 22 +- go.mod | 2 +- go.sum | 4 +- pkg/resources/virtual/clusters/clusters.go | 38 +++ .../virtual/clusters/clusters_test.go | 297 ++++++++++++++++++ pkg/resources/virtual/virtual.go | 3 + pkg/resources/virtual/virtual_test.go | 172 ++++++++++ .../partition/listprocessor/processor.go | 91 +++--- .../partition/listprocessor/processor_test.go | 87 ++++- pkg/stores/partition/store_test.go | 4 +- pkg/stores/queryhelper/safesplit.go | 16 + .../sqlpartition/listprocessor/processor.go | 33 +- .../listprocessor/processor_test.go | 38 ++- pkg/stores/sqlproxy/proxy_store.go | 1 + 14 files changed, 709 insertions(+), 99 deletions(-) create mode 100644 pkg/resources/virtual/clusters/clusters.go create mode 100644 pkg/resources/virtual/clusters/clusters_test.go create mode 100644 pkg/stores/queryhelper/safesplit.go diff --git a/README.md b/README.md index c034e4c3b..d626f561b 100644 --- a/README.md +++ b/README.md @@ -187,9 +187,9 @@ The list can be negated to exclude results: #### `sort` -Results can be sorted lexicographically by primary and secondary columns. +Results can be sorted lexicographically by any number of columns given in descending order of importance. -Sorting by only a primary column, for example name: +Sorting by only a single column, for example name: ``` /v1/{type}?sort=metadata.name @@ -201,30 +201,38 @@ Reverse sorting by name: /v1/{type}?sort=-metadata.name ``` -The secondary sort criteria is comma separated. +Multiple sort criteria are comma separated. -Example, sorting by name and creation time in ascending order: +Example, sorting first by name and then by creation time in ascending order: ``` /v1/{type}?sort=metadata.name,metadata.creationTimestamp ``` -Reverse sort by name, normal sort by creation time: +Reverse sort by name, then normal sort by creation time: ``` /v1/{type}?sort=-metadata.name,metadata.creationTimestamp ``` -Normal sort by name, reverse sort by creation time: +Normal sort by namespace, then by name, reverse sort by creation time: ``` -/v1/{type}?sort=metadata.name,-metadata.creationTimestamp +/v1/{type}?sort=metadata.namespace,metadata.name,-metadata.creationTimestamp ``` **If SQLite caching is enabled** (`server.Options.SQLCache=true`), sorting is only supported for the set of attributes supported by filtering (see above). +Sorting by labels (also requires SQLite caching) can use complex label names. +This query sorts by app name within their architectures, with the architectures +listed in reverse lexicographic order. Note that complex label names need to be +surrounded by square brackets (which themselves need to be percent-escaped for some web queries) + +``` +/v1/nodes?sort=-metadata.labels[kubernetes.io/arch],metadata.name +``` #### `page`, `pagesize`, and `revision` diff --git a/go.mod b/go.mod index 36c7be589..c5d151852 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/rancher/apiserver v0.0.0-20241009200134-5a4ecca7b988 github.com/rancher/dynamiclistener v0.6.1-rc.2 github.com/rancher/kubernetes-provider-detector v0.1.5 - github.com/rancher/lasso v0.0.0-20241202185148-04649f379358 + github.com/rancher/lasso v0.0.0-20250123080302-9325fed68518 github.com/rancher/norman v0.0.0-20241001183610-78a520c160ab github.com/rancher/remotedialer v0.3.2 github.com/rancher/wrangler/v3 v3.0.1-rc.2 diff --git a/go.sum b/go.sum index 3b37583b9..cc668967f 100644 --- a/go.sum +++ b/go.sum @@ -230,8 +230,8 @@ github.com/rancher/dynamiclistener v0.6.1-rc.2 h1:PTKNKcYXZjc/lo40EivRcXuEbCXwjp github.com/rancher/dynamiclistener v0.6.1-rc.2/go.mod h1:0KhUMHy3VcGMGavTY3i1/Mr8rVM02wFqNlUzjc+Cplg= github.com/rancher/kubernetes-provider-detector v0.1.5 h1:hWRAsWuJOemzGjz/XrbTlM7QmfO4OedvFE3QwXiH60I= github.com/rancher/kubernetes-provider-detector v0.1.5/go.mod h1:ypuJS7kP7rUiAn330xG46mj+Nhvym05GM8NqMVekpH0= -github.com/rancher/lasso v0.0.0-20241202185148-04649f379358 h1:pJwgJXPt4fi0ysXsJcl28rvxhx/Z/9SNCDwFOEyeGu0= -github.com/rancher/lasso v0.0.0-20241202185148-04649f379358/go.mod h1:IxgTBO55lziYhTEETyVKiT8/B5Rg92qYiRmcIIYoPgI= +github.com/rancher/lasso v0.0.0-20250123080302-9325fed68518 h1:aElNUJg6kxTvs/e4yfMEkk0Dgzo/lyHl85xDqP+FC+A= +github.com/rancher/lasso v0.0.0-20250123080302-9325fed68518/go.mod h1:stR7zYyew1IOnKYV5vFx1kXX5/pUoKeo5K5c78qAdV8= github.com/rancher/norman v0.0.0-20241001183610-78a520c160ab h1:ihK6See3y/JilqZlc0CG7NXPN+ue5nY9U7xUZUA8M7I= github.com/rancher/norman v0.0.0-20241001183610-78a520c160ab/go.mod h1:qX/OG/4wY27xSAcSdRilUBxBumV6Ey2CWpAeaKnBQDs= github.com/rancher/remotedialer v0.3.2 h1:kstZbRwPS5gPWpGg8VjEHT2poHtArs+Fc317YM8JCzU= diff --git a/pkg/resources/virtual/clusters/clusters.go b/pkg/resources/virtual/clusters/clusters.go new file mode 100644 index 000000000..40e2794a3 --- /dev/null +++ b/pkg/resources/virtual/clusters/clusters.go @@ -0,0 +1,38 @@ +package clusters + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// TransformManagedClusters does special-case handling on s: +// creates a new virtual `status.connected` boolean field that looks for `type = "Ready"` in any +// of the status.conditions records. + +func TransformManagedCluster(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + conditions, ok, err := unstructured.NestedFieldNoCopy(obj.Object, "status", "conditions") + if err != nil { + return obj, err + } + if !ok { + return obj, fmt.Errorf("failed to find status.conditions block in cluster %s", obj.GetName()) + } + connectedStatus := false + conditionsAsArray, ok := conditions.([]interface{}) + if !ok { + return obj, fmt.Errorf("failed to parse status.conditions as array") + } + for _, condition := range conditionsAsArray { + conditionMap, ok := condition.(map[string]interface{}) + if !ok { + return obj, fmt.Errorf("failed to parse a condition as a map") + } + if conditionMap["type"] == "Ready" && conditionMap["status"] == "True" { + connectedStatus = true + break + } + } + err = unstructured.SetNestedField(obj.Object, connectedStatus, "status", "connected") + return obj, err +} diff --git a/pkg/resources/virtual/clusters/clusters_test.go b/pkg/resources/virtual/clusters/clusters_test.go new file mode 100644 index 000000000..3b6a89b45 --- /dev/null +++ b/pkg/resources/virtual/clusters/clusters_test.go @@ -0,0 +1,297 @@ +package clusters + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func TestTransformManagedCluster(t *testing.T) { + tests := []struct { + name string + input *unstructured.Unstructured + wantOutput *unstructured.Unstructured + wantError bool + }{ + { + name: "a non-ready cluster", + input: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "id": 1, + "type": "management.cattle.io.cluster", + "metadata": map[string]interface{}{ + "name": "c-m-boris", + }, + "spec": map[string]interface{}{ + "displayName": "boris", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "SystemProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:17Z", + "status": "False", + "transitioning": false, + "type": "Ready", + }, + }, + }, + }, + }, + wantOutput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "id": 1, + "type": "management.cattle.io.cluster", + "metadata": map[string]interface{}{ + "name": "c-m-boris", + }, + "spec": map[string]interface{}{ + "displayName": "boris", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "SystemProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:17Z", + "status": "False", + "transitioning": false, + "type": "Ready", + }, + }, + "connected": false, + }, + }, + }, + }, + { + name: "the local cluster", + input: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "id": 2, + "type": "management.cattle.io.cluster", + "metadata": map[string]interface{}{ + "name": "local", + }, + "spec": map[string]interface{}{ + "displayName": "local", + "internal": true, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "", + "status": "True", + "transitioning": false, + "type": "Ready", + }, + }, + }, + }, + }, + wantOutput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "id": 2, + "type": "management.cattle.io.cluster", + "metadata": map[string]interface{}{ + "name": "local", + }, + "spec": map[string]interface{}{ + "displayName": "local", + "internal": true, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "", + "status": "True", + "transitioning": false, + "type": "Ready", + }, + }, + "connected": true, + }, + }, + }, + }, + { + name: "a ready non-local cluster", + input: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "id": 3, + "type": "management.cattle.io.cluster", + "metadata": map[string]interface{}{ + "name": "c-m-natasha", + }, + "spec": map[string]interface{}{ + "displayName": "c-m-natasha", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "Ready", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "", + "status": "True", + "transitioning": false, + "type": "Ready", + }, + }, + }, + }, + }, + wantOutput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "id": 3, + "type": "management.cattle.io.cluster", + "metadata": map[string]interface{}{ + "name": "c-m-natasha", + }, + "spec": map[string]interface{}{ + "displayName": "c-m-natasha", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "Ready", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + + "error": false, + "lastUpdateTime": "2025-01-10T22:41:37Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "", + "status": "True", + "transitioning": false, + "type": "Ready", + }, + }, + "connected": true, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := TransformManagedCluster(tt.input) + if tt.wantError { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.wantOutput, got) + } + if (err != nil) != tt.wantError { + t.Errorf("TransformManagedCluster() error = %v, wantErr %v", err, tt.wantError) + return + } + if !reflect.DeepEqual(got, tt.wantOutput) { + t.Errorf("TransformManagedCluster() got = %v, want %v", got, tt.wantOutput) + } + }) + } +} diff --git a/pkg/resources/virtual/virtual.go b/pkg/resources/virtual/virtual.go index 270b786b6..105260c93 100644 --- a/pkg/resources/virtual/virtual.go +++ b/pkg/resources/virtual/virtual.go @@ -5,6 +5,7 @@ package virtual import ( "fmt" + "github.com/rancher/steve/pkg/resources/virtual/clusters" "github.com/rancher/steve/pkg/resources/virtual/common" "github.com/rancher/steve/pkg/resources/virtual/events" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -31,6 +32,8 @@ func (t *TransformBuilder) GetTransformFunc(gvk schema.GroupVersionKind) cache.T converters := make([]func(*unstructured.Unstructured) (*unstructured.Unstructured, error), 0) if gvk.Kind == "Event" && gvk.Group == "" && gvk.Version == "v1" { converters = append(converters, events.TransformEventObject) + } else if gvk.Kind == "Cluster" && gvk.Group == "management.cattle.io" && gvk.Version == "v3" { + converters = append(converters, clusters.TransformManagedCluster) } converters = append(converters, t.defaultFields.TransformCommon) diff --git a/pkg/resources/virtual/virtual_test.go b/pkg/resources/virtual/virtual_test.go index f9f05d27a..1163987a5 100644 --- a/pkg/resources/virtual/virtual_test.go +++ b/pkg/resources/virtual/virtual_test.go @@ -1,6 +1,7 @@ package virtual_test import ( + "fmt" "github.com/rancher/steve/pkg/resources/virtual" "k8s.io/apimachinery/pkg/runtime/schema" "strings" @@ -176,6 +177,174 @@ func TestTransformChain(t *testing.T) { }, }, }, + { + name: "a non-ready cluster", + input: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "management.cattle.io/v3", + "kind": "Cluster", + "id": 1, + "metadata": map[string]interface{}{ + "name": "c-m-boris", + }, + "spec": map[string]interface{}{ + "displayName": "boris", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "SystemProjectCreated", + }, + }, + }, + }, + }, + wantOutput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "management.cattle.io/v3", + "kind": "Cluster", + "id": "c-m-boris", + "_id": 1, + "metadata": map[string]interface{}{ + "name": "c-m-boris", + "relationships": []any(nil), + }, + "spec": map[string]interface{}{ + "displayName": "boris", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "DefaultProjectCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "SystemProjectCreated", + }, + }, + "connected": false, + }, + }, + }, + }, + { + name: "a ready cluster", + input: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "management.cattle.io/v3", + "kind": "Cluster", + "id": 2, + "metadata": map[string]interface{}{ + "name": "c-m-natasha", + }, + "spec": map[string]interface{}{ + "displayName": "natasha", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "Ready", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "SystemProjectCreated", + }, + }, + }, + }, + }, + wantOutput: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "management.cattle.io/v3", + "kind": "Cluster", + "id": "c-m-natasha", + "_id": 2, + "metadata": map[string]interface{}{ + "name": "c-m-natasha", + "relationships": []any(nil), + }, + "spec": map[string]interface{}{ + "displayName": "natasha", + "internal": false, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "BackingNamespaceCreated", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "Ready", + }, + map[string]interface{}{ + "error": false, + "lastUpdateTime": "2025-01-10T22:52:16Z", + "status": "True", + "transitioning": false, + "type": "SystemProjectCreated", + }, + }, + "connected": true, + }, + }, + }, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -190,6 +359,9 @@ func TestTransformChain(t *testing.T) { apiVersion := raw.GetAPIVersion() parts := strings.Split(apiVersion, "/") gvk := schema.GroupVersionKind{Group: parts[0], Version: parts[1], Kind: raw.GetKind()} + if test.name == "a non-ready cluster" { + fmt.Printf("Stop here") + } output, err := tb.GetTransformFunc(gvk)(test.input) require.Equal(t, test.wantOutput, output) if test.wantError { diff --git a/pkg/stores/partition/listprocessor/processor.go b/pkg/stores/partition/listprocessor/processor.go index 5a80ebc4d..9b39d0846 100644 --- a/pkg/stores/partition/listprocessor/processor.go +++ b/pkg/stores/partition/listprocessor/processor.go @@ -2,12 +2,14 @@ package listprocessor import ( + "fmt" "regexp" "sort" "strconv" "strings" "github.com/rancher/apiserver/pkg/types" + "github.com/rancher/steve/pkg/stores/queryhelper" "github.com/rancher/wrangler/v3/pkg/data" "github.com/rancher/wrangler/v3/pkg/data/convert" corecontrollers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" @@ -99,27 +101,29 @@ const ( // The subfield is internally represented as a slice, e.g. [metadata, name]. // The order is represented by prefixing the sort key by '-', e.g. sort=-metadata.name. type Sort struct { - primaryField []string - secondaryField []string - primaryOrder SortOrder - secondaryOrder SortOrder + Fields [][]string + Orders []SortOrder } // String returns the sort parameters as a query string. func (s Sort) String() string { - field := "" - if s.primaryOrder == DESC { - field = "-" + field - } - field += strings.Join(s.primaryField, ".") - if len(s.secondaryField) > 0 { - field += "," - if s.secondaryOrder == DESC { - field += "-" + nonIdentifierField := regexp.MustCompile(`[^a-zA-Z0-9_]`) + fields := make([]string, len(s.Fields)) + for i, field := range s.Fields { + lastIndex := len(field) - 1 + newField := strings.Join(field[0:lastIndex], ".") + if nonIdentifierField.MatchString(field[lastIndex]) { + // label keys may contain non-identifier characters `/`, `.` and `-` + newField += fmt.Sprintf("[%s]", field[lastIndex]) + } else { + newField += fmt.Sprintf(".%s", field[lastIndex]) + } + if s.Orders[i] == DESC { + newField = fmt.Sprintf("-%s", newField) } - field += strings.Join(s.secondaryField, ".") + fields[i] = newField } - return field + return strings.Join(fields, ",") } // Pagination represents how to return paginated results. @@ -189,24 +193,24 @@ func ParseQuery(apiOp *types.APIRequest) *ListOptions { sortOpts := Sort{} sortKeys := q.Get(sortParam) if sortKeys != "" { - sortParts := strings.SplitN(sortKeys, ",", 2) - primaryField := sortParts[0] - if primaryField != "" && primaryField[0] == '-' { - sortOpts.primaryOrder = DESC - primaryField = primaryField[1:] - } - if primaryField != "" { - sortOpts.primaryField = strings.Split(primaryField, ".") - } - if len(sortParts) > 1 { - secondaryField := sortParts[1] - if secondaryField != "" && secondaryField[0] == '-' { - sortOpts.secondaryOrder = DESC - secondaryField = secondaryField[1:] + sortParts := strings.Split(sortKeys, ",") + for _, field := range sortParts { + sortOrder := ASC + if len(field) > 0 { + if field[0] == '-' { + sortOrder = DESC + field = field[1:] + } } - if secondaryField != "" { - sortOpts.secondaryField = strings.Split(secondaryField, ".") + if len(field) == 0 { + // Old semantics: if we have a sort query like + // sort=,metadata.someOtherField + // we *DON'T sort + // Fixed: skip empty-strings. + continue } + sortOpts.Fields = append(sortOpts.Fields, queryhelper.SafeSplit(field)) + sortOpts.Orders = append(sortOpts.Orders, sortOrder) } } opts.Sort = sortOpts @@ -350,24 +354,23 @@ func matchesAll(obj map[string]interface{}, filters []OrFilter) bool { // SortList sorts the slice by the provided sort criteria. func SortList(list []unstructured.Unstructured, s Sort) []unstructured.Unstructured { - if len(s.primaryField) == 0 { + if len(s.Fields) == 0 { return list } sort.Slice(list, func(i, j int) bool { - leftPrime := convert.ToString(data.GetValueN(list[i].Object, s.primaryField...)) - rightPrime := convert.ToString(data.GetValueN(list[j].Object, s.primaryField...)) - if leftPrime == rightPrime && len(s.secondaryField) > 0 { - leftSecond := convert.ToString(data.GetValueN(list[i].Object, s.secondaryField...)) - rightSecond := convert.ToString(data.GetValueN(list[j].Object, s.secondaryField...)) - if s.secondaryOrder == ASC { - return leftSecond < rightSecond + leftNode := list[i].Object + rightNode := list[j].Object + for i, field := range s.Fields { + leftValue := convert.ToString(data.GetValueN(leftNode, field...)) + rightValue := convert.ToString(data.GetValueN(rightNode, field...)) + if leftValue != rightValue { + if s.Orders[i] == ASC { + return leftValue < rightValue + } + return rightValue < leftValue } - return rightSecond < leftSecond - } - if s.primaryOrder == ASC { - return leftPrime < rightPrime } - return rightPrime < leftPrime + return false }) return list } diff --git a/pkg/stores/partition/listprocessor/processor_test.go b/pkg/stores/partition/listprocessor/processor_test.go index d5e4fe5a2..173690db7 100644 --- a/pkg/stores/partition/listprocessor/processor_test.go +++ b/pkg/stores/partition/listprocessor/processor_test.go @@ -1787,7 +1787,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"metadata", "name"}, + Fields: [][]string{{"metadata", "name"}}, + Orders: []SortOrder{ASC}, }, want: []unstructured.Unstructured{ { @@ -1863,8 +1864,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"metadata", "name"}, - primaryOrder: DESC, + Fields: [][]string{{"metadata", "name"}}, + Orders: []SortOrder{DESC}, }, want: []unstructured.Unstructured{ { @@ -1940,7 +1941,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"data", "productType"}, + Fields: [][]string{{"data", "productType"}}, + Orders: []SortOrder{ASC}, }, want: []unstructured.Unstructured{ { @@ -2090,8 +2092,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"data", "color"}, - secondaryField: []string{"metadata", "name"}, + Fields: [][]string{{"data", "color"}, {"metadata", "name"}}, + Orders: []SortOrder{ASC, ASC}, }, want: []unstructured.Unstructured{ { @@ -2130,7 +2132,7 @@ func TestSortList(t *testing.T) { }, }, { - name: "primary sort ascending, secondary sort descending", + name: "1st sort ascending, 2nd sort descending", objects: []unstructured.Unstructured{ { Object: map[string]interface{}{ @@ -2167,9 +2169,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"data", "color"}, - secondaryField: []string{"metadata", "name"}, - secondaryOrder: DESC, + Fields: [][]string{{"data", "color"}, {"metadata", "name"}}, + Orders: []SortOrder{ASC, DESC}, }, want: []unstructured.Unstructured{ { @@ -2245,9 +2246,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"data", "color"}, - primaryOrder: DESC, - secondaryField: []string{"metadata", "name"}, + Fields: [][]string{{"data", "color"}, {"metadata", "name"}}, + Orders: []SortOrder{DESC, ASC}, }, want: []unstructured.Unstructured{ { @@ -2323,10 +2323,8 @@ func TestSortList(t *testing.T) { }, }, sort: Sort{ - primaryField: []string{"data", "color"}, - primaryOrder: DESC, - secondaryField: []string{"metadata", "name"}, - secondaryOrder: DESC, + Fields: [][]string{{"data", "color"}, {"metadata", "name"}}, + Orders: []SortOrder{DESC, DESC}, }, want: []unstructured.Unstructured{ { @@ -2373,6 +2371,61 @@ func TestSortList(t *testing.T) { } } +func TestSortString(t *testing.T) { + tests := []struct { + name string + sort Sort + want string + }{ + { + name: "empty sort list", + sort: Sort{}, + want: "", + }, + { + name: "single sort asc", + sort: Sort{ + Fields: [][]string{{"field1a", "field1b"}}, + Orders: []SortOrder{ASC}, + }, + want: "field1a.field1b", + }, + { + name: "single sort desc", + sort: Sort{ + Fields: [][]string{{"field2a", "field2b"}}, + Orders: []SortOrder{DESC}, + }, + want: "-field2a.field2b", + }, + { + name: "multiple sort with complex name, asc", + sort: Sort{ + Fields: [][]string{{"field3a", "field3b"}, {"metadata", "labels", "slash/warning"}}, + Orders: []SortOrder{ASC, DESC}, + }, + want: "field3a.field3b,-metadata.labels[slash/warning]", + }, + { + name: "multiple sort with complex name, asc", + sort: Sort{ + Fields: [][]string{{"metadata", "labels", "cows/dogs"}, + {"metadata", "labels", "hyphens-are-special"}, + {"metadata", "labels", "dots.are.special"}}, + Orders: []SortOrder{DESC, ASC, DESC}, + }, + want: "-metadata.labels[cows/dogs],metadata.labels[hyphens-are-special],-metadata.labels[dots.are.special]", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.sort.String() + assert.Equal(t, test.want, got) + }) + } + +} + func TestPaginateList(t *testing.T) { objects := []unstructured.Unstructured{ { diff --git a/pkg/stores/partition/store_test.go b/pkg/stores/partition/store_test.go index 41b464dfb..44c98b070 100644 --- a/pkg/stores/partition/store_test.go +++ b/pkg/stores/partition/store_test.go @@ -523,7 +523,7 @@ func TestList(t *testing.T) { }, }, { - name: "sorting with missing primary sort is unsorted", + name: "sorting with missing primary sort continues", apiOps: []*types.APIRequest{ newRequest("sort=,metadata.name", "user1"), }, @@ -553,8 +553,8 @@ func TestList(t *testing.T) { Count: 3, Objects: []types.APIObject{ newApple("fuji").toObj(), - newApple("honeycrisp").toObj(), newApple("granny-smith").toObj(), + newApple("honeycrisp").toObj(), }, }, }, diff --git a/pkg/stores/queryhelper/safesplit.go b/pkg/stores/queryhelper/safesplit.go new file mode 100644 index 000000000..0edfb0bfc --- /dev/null +++ b/pkg/stores/queryhelper/safesplit.go @@ -0,0 +1,16 @@ +package queryhelper + +import "strings" + +// SafeSplit breaks a regular "a.b.c" path to the string slice ["a", "b", "c"] +// but if the final part is in square brackets, like "metadata.labels[cattleprod.io/moo]", +// it returns ["metadata", "labels", "cattleprod.io/moo"] +func SafeSplit(fieldPath string) []string { + squareBracketLocation := strings.Index(fieldPath, "[") + if squareBracketLocation == -1 { + return strings.Split(fieldPath, ".") + } + s := strings.Split(fieldPath[0:squareBracketLocation], ".") + s = append(s, fieldPath[squareBracketLocation+1:len(fieldPath)-1]) + return s +} diff --git a/pkg/stores/sqlpartition/listprocessor/processor.go b/pkg/stores/sqlpartition/listprocessor/processor.go index 24a8af4ca..71f3e12e8 100644 --- a/pkg/stores/sqlpartition/listprocessor/processor.go +++ b/pkg/stores/sqlpartition/listprocessor/processor.go @@ -12,6 +12,7 @@ import ( "github.com/rancher/apiserver/pkg/types" "github.com/rancher/steve/pkg/sqlcache/informer" "github.com/rancher/steve/pkg/sqlcache/partition" + "github.com/rancher/steve/pkg/stores/queryhelper" "github.com/rancher/wrangler/v3/pkg/schemas/validation" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -88,27 +89,19 @@ func ParseQuery(apiOp *types.APIRequest, namespaceCache Cache) (informer.ListOpt sortOpts := informer.Sort{} sortKeys := q.Get(sortParam) if sortKeys != "" { - sortParts := strings.SplitN(sortKeys, ",", 2) - primaryField := sortParts[0] - if primaryField != "" { - if primaryField[0] == '-' { - sortOpts.Orders = append(sortOpts.Orders, informer.DESC) - primaryField = primaryField[1:] - } else { - sortOpts.Orders = append(sortOpts.Orders, informer.ASC) - } - sortOpts.Fields = append(sortOpts.Fields, strings.Split(primaryField, ".")) - } - if len(sortParts) > 1 { - secondaryField := sortParts[1] - if secondaryField != "" { - if secondaryField[0] == '-' { - sortOpts.Orders = append(sortOpts.Orders, informer.DESC) - secondaryField = secondaryField[1:] - } else { - sortOpts.Orders = append(sortOpts.Orders, informer.ASC) + sortParts := strings.Split(sortKeys, ",") + for _, sortPart := range sortParts { + field := sortPart + if len(field) > 0 { + sortOrder := informer.ASC + if field[0] == '-' { + sortOrder = informer.DESC + field = field[1:] + } + if len(field) > 0 { + sortOpts.Fields = append(sortOpts.Fields, queryhelper.SafeSplit(field)) + sortOpts.Orders = append(sortOpts.Orders, sortOrder) } - sortOpts.Fields = append(sortOpts.Fields, strings.Split(secondaryField, ".")) } } } diff --git a/pkg/stores/sqlpartition/listprocessor/processor_test.go b/pkg/stores/sqlpartition/listprocessor/processor_test.go index 3a1b470d0..04fd90faa 100644 --- a/pkg/stores/sqlpartition/listprocessor/processor_test.go +++ b/pkg/stores/sqlpartition/listprocessor/processor_test.go @@ -342,7 +342,7 @@ func TestParseQuery(t *testing.T) { }, }) tests = append(tests, testCase{ - description: "ParseQuery() with no errors returned should returned no errors. If one sort param is given, primary field" + + description: "ParseQuery() with no errors returned should returned no errors. It should sort on the one given" + " sort option should be set", req: &types.APIRequest{ Request: &http.Request{ @@ -353,11 +353,8 @@ func TestParseQuery(t *testing.T) { ChunkSize: defaultLimit, Sort: informer.Sort{ Fields: [][]string{ - {"metadata", "name"}, - }, - Orders: []informer.SortOrder{ - informer.ASC, - }, + {"metadata", "name"}}, + Orders: []informer.SortOrder{informer.ASC}, }, Filters: make([]informer.OrFilter, 0), Pagination: informer.Pagination{ @@ -420,6 +417,32 @@ func TestParseQuery(t *testing.T) { return nil }, }) + + tests = append(tests, testCase{ + description: "sorting can parse bracketed field names correctly", + req: &types.APIRequest{ + Request: &http.Request{ + URL: &url.URL{RawQuery: "sort=-metadata.labels[beef.cattle.io/snort],metadata.labels.steer,metadata.labels[bossie.cattle.io/moo],spec.something"}, + }, + }, + expectedLO: informer.ListOptions{ + ChunkSize: defaultLimit, + Sort: informer.Sort{ + Fields: [][]string{{"metadata", "labels", "beef.cattle.io/snort"}, + {"metadata", "labels", "steer"}, + {"metadata", "labels", "bossie.cattle.io/moo"}, + {"spec", "something"}}, + Orders: []informer.SortOrder{informer.DESC, informer.ASC, informer.ASC, informer.ASC}, + }, + Filters: make([]informer.OrFilter, 0), + Pagination: informer.Pagination{ + Page: 1, + }, + }, + setupNSCache: func() Cache { + return nil + }, + }) tests = append(tests, testCase{ description: "ParseQuery() with no errors returned should returned no errors. If continue params is given, resume" + " should be set with assigned value.", @@ -522,6 +545,9 @@ func TestParseQuery(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { test.nsc = test.setupNSCache() + if test.description == "sorting can parse bracketed field names correctly" { + fmt.Printf("stop here") + } lo, err := ParseQuery(test.req, test.nsc) if test.errExpected { assert.NotNil(t, err) diff --git a/pkg/stores/sqlproxy/proxy_store.go b/pkg/stores/sqlproxy/proxy_store.go index 878405a0a..7d6cdb4f4 100644 --- a/pkg/stores/sqlproxy/proxy_store.go +++ b/pkg/stores/sqlproxy/proxy_store.go @@ -127,6 +127,7 @@ var ( {"metadata", "labels[provider.cattle.io]"}, {"spec", "internal"}, {"spec", "displayName"}, + {"status", "connected"}, {"status", "provider"}, }, gvkKey("management.cattle.io", "v3", "Node"): {