Validate sort order in admin.ResourceListRequest

Signed-off-by: Iaroslav Ciupin <[email protected]>

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/cloudevents/sdk-go/v2 v2.8.0
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/evanphx/json-patch v4.12.0+incompatible
-	github.com/flyteorg/flyteidl v1.5.14
+	github.com/flyteorg/flyteidl v1.5.17-0.20230822102414-1c76702b5f6a
 	github.com/flyteorg/flyteplugins v1.0.67
 	github.com/flyteorg/flytepropeller v1.1.98
 	github.com/flyteorg/flytestdlib v1.0.22

go.sum

@@ -295,6 +295,8 @@ github.com/felixge/httpsnoop v1.0.1 h1:lvB5Jl89CsZtGIWuTcDM1E/vkVs49/Ml7JJe07l8S
 github.com/felixge/httpsnoop v1.0.1/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
 github.com/flyteorg/flyteidl v1.5.14 h1:+3ewipoOp82fPyIVgvvrMq1lorl5Kz3Lh6sh/a9+loI=
 github.com/flyteorg/flyteidl v1.5.14/go.mod h1:EtE/muM2lHHgBabjYcxqe9TWeJSL0kXwbI0RgVwI4Og=
+github.com/flyteorg/flyteidl v1.5.17-0.20230822102414-1c76702b5f6a h1:eqQ7g71w8NG5fnTq1QiyPNoIaryNnU2wW/2DtATFHa0=
+github.com/flyteorg/flyteidl v1.5.17-0.20230822102414-1c76702b5f6a/go.mod h1:EtE/muM2lHHgBabjYcxqe9TWeJSL0kXwbI0RgVwI4Og=
 github.com/flyteorg/flyteplugins v1.0.67 h1:d2FXpwxQwX/k4YdmhuusykOemHb/cUTPEob4WBmdpjE=
 github.com/flyteorg/flyteplugins v1.0.67/go.mod h1:HHt4nKDKVwrZPKDsj99dNtDSIJL378xNotYMA3a/TFA=
 github.com/flyteorg/flytepropeller v1.1.98 h1:Zk2ENYB9VZRT5tFUIFjm+aCkr0TU2EuyJ5gh52fpLoA=
@@ -760,6 +762,7 @@ github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXi
 github.com/google/martian/v3 v3.0.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0=
 github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0=
 github.com/google/martian/v3 v3.3.2 h1:IqNFLAmvJOgVlpdEBiQbDc2EwKW77amAycfTuWKdfvw=
+github.com/google/martian/v3 v3.3.2/go.mod h1:oBOf6HBosgwRXnUGWUB05QECsc6uvmMiJ3+6W4l/CUk=
 github.com/google/pprof v0.0.0-20181206194817-3ea8567a2e57/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
 github.com/google/pprof v0.0.0-20191218002539-d4f498aebedc/go.mod h1:ZgVRPoUq/hfqzAqh7sHMqb3I9Rq5C59dIz2SbBwJ4eM=
@@ -785,6 +788,7 @@ github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
 github.com/googleapis/gax-go/v2 v2.7.1 h1:gF4c0zjUP2H/s/hEGyLA3I0fA2ZWjzYiONAD6cvPr8A=
 github.com/googleapis/gax-go/v2 v2.7.1/go.mod h1:4orTrqY6hXxxaUL4LHIPl6lGo8vAE38/qKbhSAKP6QI=
+github.com/googleapis/go-type-adapters v1.0.0/go.mod h1:zHW75FOG2aur7gAO2B+MLby+cLsWGBF62rFAi7WjWO4=
 github.com/googleapis/google-cloud-go-testing v0.0.0-20200911160855-bcd43fbb19e8/go.mod h1:dvDLG8qkwmyD9a/MJJN3XJcT3xFxOKAvTZGvuZmac9g=
 github.com/gopherjs/gopherjs v0.0.0-20181004151105-1babbf986f6f/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=

pkg/clusterresource/impl/db_admin_data_provider.go

@@ -3,13 +3,14 @@ package impl
 import (
 	"context"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+
 	"github.com/flyteorg/flyteadmin/pkg/clusterresource/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/common"
 	managerInterfaces "github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
 	repositoryInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/transformers"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 )
 
 // Implementation of an interfaces.FlyteAdminDataProvider which fetches data directly from the provided database connection.
@@ -52,8 +53,8 @@ func (p dbAdminProvider) GetProjects(ctx context.Context) (*admin.Projects, erro
 		return nil, err
 	}
 	projectModels, err := p.db.ProjectRepo().List(ctx, repositoryInterfaces.ListResourceInput{
-		SortParameter: descCreatedAtSortDBParam,
-		InlineFilters: []common.InlineFilter{filter},
+		SortParameters: descCreatedAtSortDBParam,
+		InlineFilters:  []common.InlineFilter{filter},
 	})
 	if err != nil {
 		return nil, err

pkg/clusterresource/impl/db_admin_data_provider_test.go

@@ -5,17 +5,18 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+	"github.com/stretchr/testify/assert"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	"github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/manager/mocks"
 	repoInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	repoMocks "github.com/flyteorg/flyteadmin/pkg/repositories/mocks"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/models"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	configMocks "github.com/flyteorg/flyteadmin/pkg/runtime/mocks"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/stretchr/testify/assert"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 var errFoo = errors.New("foo")
@@ -104,7 +105,7 @@ func TestGetProjects(t *testing.T) {
 		mockRepo.(*repoMocks.MockRepository).ProjectRepoIface = &repoMocks.MockProjectRepo{
 			ListProjectsFunction: func(ctx context.Context, input repoInterfaces.ListResourceInput) ([]models.Project, error) {
 				assert.Len(t, input.InlineFilters, 1)
-				assert.Equal(t, input.SortParameter.GetGormOrderExpr(), "created_at desc")
+				assert.Equal(t, input.SortParameters.GetGormOrderExpr(), "created_at desc")
 				return []models.Project{
 					{
 						Identifier: "flytesnacks",

pkg/clusterresource/impl/shared.go

@@ -1,10 +1,11 @@
 package impl
 
 import (
-	"github.com/flyteorg/flyteadmin/pkg/common"
-	"github.com/flyteorg/flyteadmin/pkg/errors"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"google.golang.org/grpc/codes"
+
+	"github.com/flyteorg/flyteadmin/pkg/common"
+	"github.com/flyteorg/flyteadmin/pkg/errors"
 )
 
 func NewMissingEntityError(entity string) error {

pkg/common/sorting.go

@@ -3,9 +3,12 @@ package common
 import (
 	"fmt"
 
-	"github.com/flyteorg/flyteadmin/pkg/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"google.golang.org/grpc/codes"
+
+	"github.com/flyteorg/flyteadmin/pkg/errors"
 )
 
 const gormDescending = "%s desc"
@@ -23,7 +26,11 @@ func (s *sortParamImpl) GetGormOrderExpr() string {
 	return s.gormOrderExpression
 }
 
-func NewSortParameter(sort admin.Sort) (SortParameter, error) {
+func NewSortParameter(sort *admin.Sort, allowed sets.String) ([]SortParameter, error) {
+	if !allowed.Has(sort.Key) {
+		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort_key: %s", sort.Key)
+	}
+
 	var gormOrderExpression string
 	switch sort.Direction {
 	case admin.Sort_DESCENDING:
@@ -33,7 +40,27 @@ func NewSortParameter(sort admin.Sort) (SortParameter, error) {
 	default:
 		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort order specified: %v", sort)
 	}
-	return &sortParamImpl{
-		gormOrderExpression: gormOrderExpression,
-	}, nil
+
+	return []SortParameter{&sortParamImpl{gormOrderExpression: gormOrderExpression}}, nil
+}
+
+func NewSortParameters(request *admin.ResourceListRequest, allowed sets.String) ([]SortParameter, error) {
+	if len(request.SortKeys) > 0 && request.SortBy != nil {
+		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "cannot specify both sort_keys and sort_by")
+	}
+
+	if request.SortBy != nil {
+		request.SortKeys = append(request.SortKeys, request.SortBy)
+	}
+
+	sortParams := make([]SortParameter, 0, len(request.SortKeys))
+	for _, sortKey := range request.SortKeys {
+		params, err := NewSortParameter(sortKey, allowed)
+		if err != nil {
+			return sortParams, err
+		}
+		sortParams = append(sortParams, params...)
+	}
+
+	return sortParams, nil
 }

pkg/manager/impl/description_entity_manager.go

@@ -4,22 +4,22 @@ import (
 	"context"
 	"strconv"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
+	"github.com/flyteorg/flytestdlib/contextutils"
+	"github.com/flyteorg/flytestdlib/logger"
+	"github.com/flyteorg/flytestdlib/promutils"
+	"google.golang.org/grpc/codes"
 
 	"github.com/flyteorg/flyteadmin/pkg/common"
-
 	"github.com/flyteorg/flyteadmin/pkg/errors"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/util"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/validation"
 	"github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
+	"github.com/flyteorg/flyteadmin/pkg/repositories/gormimpl"
 	repoInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/transformers"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/flyteorg/flytestdlib/contextutils"
-	"github.com/flyteorg/flytestdlib/logger"
-	"github.com/flyteorg/flytestdlib/promutils"
-	"google.golang.org/grpc/codes"
 )
 
 type DescriptionEntityMetrics struct {
@@ -65,23 +65,21 @@ func (d *DescriptionEntityManager) ListDescriptionEntity(ctx context.Context, re
 		logger.Error(ctx, "failed to get database filter")
 		return nil, err
 	}
-	var sortParameter common.SortParameter
-	if request.SortBy != nil {
-		sortParameter, err = common.NewSortParameter(*request.SortBy)
-		if err != nil {
-			return nil, err
-		}
+	sortParameters, err := common.NewSortParameter(request.SortBy, gormimpl.DescriptionEntityColumns)
+	if err != nil {
+		return nil, err
 	}
+
 	offset, err := validation.ValidateToken(request.Token)
 	if err != nil {
 		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument,
 			"invalid pagination token %s for ListWorkflows", request.Token)
 	}
 	listDescriptionEntitiesInput := repoInterfaces.ListResourceInput{
-		Limit:         int(request.Limit),
-		Offset:        offset,
-		InlineFilters: filters,
-		SortParameter: sortParameter,
+		Limit:          int(request.Limit),
+		Offset:         offset,
+		InlineFilters:  filters,
+		SortParameters: sortParameters,
 	}
 	output, err := d.db.DescriptionEntityRepo().List(ctx, listDescriptionEntitiesInput)
 	if err != nil {

pkg/manager/impl/execution_manager.go

@@ -6,49 +6,42 @@ import (
 	"strconv"
 	"time"
 
-	"github.com/flyteorg/flytestdlib/promutils/labeled"
-
-	"github.com/flyteorg/flyteadmin/plugins"
-
+	"github.com/benbjohnson/clock"
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
 	"github.com/flyteorg/flyteplugins/go/tasks/pluginmachinery/flytek8s"
-
-	"github.com/flyteorg/flyteadmin/auth"
-
-	"github.com/flyteorg/flyteadmin/pkg/manager/impl/resources"
-
-	dataInterfaces "github.com/flyteorg/flyteadmin/pkg/data/interfaces"
 	"github.com/flyteorg/flytestdlib/contextutils"
+	"github.com/flyteorg/flytestdlib/logger"
 	"github.com/flyteorg/flytestdlib/promutils"
+	"github.com/flyteorg/flytestdlib/promutils/labeled"
+	"github.com/flyteorg/flytestdlib/storage"
+	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes"
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/prometheus/client_golang/prometheus"
+	"google.golang.org/grpc/codes"
 
-	"github.com/flyteorg/flyteadmin/pkg/common"
-
-	"github.com/flyteorg/flytestdlib/logger"
-	"github.com/flyteorg/flytestdlib/storage"
-
+	"github.com/flyteorg/flyteadmin/auth"
 	cloudeventInterfaces "github.com/flyteorg/flyteadmin/pkg/async/cloudevent/interfaces"
 	eventWriter "github.com/flyteorg/flyteadmin/pkg/async/events/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/async/notifications"
 	notificationInterfaces "github.com/flyteorg/flyteadmin/pkg/async/notifications/interfaces"
+	"github.com/flyteorg/flyteadmin/pkg/common"
+	dataInterfaces "github.com/flyteorg/flyteadmin/pkg/data/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/errors"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/executions"
+	"github.com/flyteorg/flyteadmin/pkg/manager/impl/resources"
+	"github.com/flyteorg/flyteadmin/pkg/manager/impl/shared"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/util"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/validation"
 	"github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
+	"github.com/flyteorg/flyteadmin/pkg/repositories/gormimpl"
 	repositoryInterfaces "github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/models"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/transformers"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	workflowengineInterfaces "github.com/flyteorg/flyteadmin/pkg/workflowengine/interfaces"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
-	"google.golang.org/grpc/codes"
-
-	"github.com/benbjohnson/clock"
-	"github.com/flyteorg/flyteadmin/pkg/manager/impl/shared"
-	"github.com/golang/protobuf/proto"
+	"github.com/flyteorg/flyteadmin/plugins"
 )
 
 const childContainerQueueKey = "child_queue"
@@ -1434,12 +1427,10 @@ func (m *ExecutionManager) ListExecutions(
 	if err != nil {
 		return nil, err
 	}
-	var sortParameter common.SortParameter
-	if request.SortBy != nil {
-		sortParameter, err = common.NewSortParameter(*request.SortBy)
-		if err != nil {
-			return nil, err
-		}
+
+	sortParameters, err := common.NewSortParameters(&request, gormimpl.ExecutionColumns)
+	if err != nil {
+		return nil, err
 	}
 
 	offset, err := validation.ValidateToken(request.Token)
@@ -1461,7 +1452,7 @@ func (m *ExecutionManager) ListExecutions(
 		Limit:             int(request.Limit),
 		Offset:            offset,
 		InlineFilters:     filters,
-		SortParameter:     sortParameter,
+		SortParameters:    sortParameters,
 		JoinTableEntities: joinTableEntities,
 	}
 	output, err := m.db.ExecutionRepo().List(ctx, listExecutionsInput)

pkg/manager/impl/execution_manager_test.go

@@ -13,6 +13,13 @@ import (
 	"google.golang.org/protobuf/types/known/timestamppb"
 
 	"github.com/benbjohnson/clock"
+	"github.com/flyteorg/flyteidl/clients/go/coreutils"
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/event"
+	"github.com/gogo/protobuf/jsonpb"
+	"github.com/golang/protobuf/ptypes"
+	"github.com/stretchr/testify/mock"
+	"google.golang.org/grpc/codes"
+
 	"github.com/flyteorg/flyteadmin/pkg/common"
 	commonTestUtils "github.com/flyteorg/flyteadmin/pkg/common/testutils"
 	flyteAdminErrors "github.com/flyteorg/flyteadmin/pkg/errors"
@@ -21,18 +28,13 @@ import (
 	managerInterfaces "github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
 	managerMocks "github.com/flyteorg/flyteadmin/pkg/manager/mocks"
 	"github.com/flyteorg/flyteadmin/pkg/runtime"
-	"github.com/flyteorg/flyteidl/clients/go/coreutils"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/event"
-	"github.com/gogo/protobuf/jsonpb"
-	"github.com/golang/protobuf/ptypes"
-	"github.com/stretchr/testify/mock"
-	"google.golang.org/grpc/codes"
 
 	"k8s.io/apimachinery/pkg/api/resource"
 
-	eventWriterMocks "github.com/flyteorg/flyteadmin/pkg/async/events/mocks"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	eventWriterMocks "github.com/flyteorg/flyteadmin/pkg/async/events/mocks"
+
 	"github.com/flyteorg/flyteadmin/auth"
 
 	commonMocks "github.com/flyteorg/flyteadmin/pkg/common/mocks"
@@ -43,6 +45,13 @@ import (
 
 	"fmt"
 
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
+	mockScope "github.com/flyteorg/flytestdlib/promutils"
+	"github.com/golang/protobuf/proto"
+	"github.com/golang/protobuf/ptypes/wrappers"
+	"github.com/stretchr/testify/assert"
+
 	notificationMocks "github.com/flyteorg/flyteadmin/pkg/async/notifications/mocks"
 	dataMocks "github.com/flyteorg/flyteadmin/pkg/data/mocks"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/testutils"
@@ -55,12 +64,6 @@ import (
 	runtimeMocks "github.com/flyteorg/flyteadmin/pkg/runtime/mocks"
 	workflowengineInterfaces "github.com/flyteorg/flyteadmin/pkg/workflowengine/interfaces"
 	workflowengineMocks "github.com/flyteorg/flyteadmin/pkg/workflowengine/mocks"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
-	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
-	mockScope "github.com/flyteorg/flytestdlib/promutils"
-	"github.com/golang/protobuf/proto"
-	"github.com/golang/protobuf/ptypes/wrappers"
-	"github.com/stretchr/testify/assert"
 )
 
 var spec = testutils.GetExecutionRequest().Spec
@@ -2979,7 +2982,7 @@ func TestListExecutions(t *testing.T) {
 		assert.True(t, domainFilter, "Missing domain equality filter")
 		assert.False(t, nameFilter, "Included name equality filter")
 		assert.Equal(t, limit, input.Limit)
-		assert.Equal(t, "domain asc", input.SortParameter.GetGormOrderExpr())
+		assert.Equal(t, "domain asc", input.SortParameters.GetGormOrderExpr())
 		assert.Equal(t, 2, input.Offset)
 		assert.EqualValues(t, map[common.Entity]bool{
 			common.Execution: true,
@@ -3965,7 +3968,7 @@ func TestListExecutions_LegacyModel(t *testing.T) {
 		assert.True(t, domainFilter, "Missing domain equality filter")
 		assert.False(t, nameFilter, "Included name equality filter")
 		assert.Equal(t, limit, input.Limit)
-		assert.Equal(t, "domain asc", input.SortParameter.GetGormOrderExpr())
+		assert.Equal(t, "domain asc", input.SortParameters.GetGormOrderExpr())
 		assert.Equal(t, 2, input.Offset)
 		return interfaces.ExecutionCollectionOutput{
 			Executions: []models.Execution{