Merge pull request from GHSA-r847-6w6h-r8g4

* Fix list filters * Fix sort keys * split into 2 maps * move columns to models * move columns to models
flyteorg · Aug 31, 2023 · b3177ef · b3177ef
1 parent 6c97bb9
commit b3177ef
Show file tree

Hide file tree

Showing 51 changed files with 500 additions and 379 deletions.
diff --git a/pkg/clusterresource/impl/admin_service_data_provider.go b/pkg/clusterresource/impl/admin_service_data_provider.go
@@ -4,9 +4,12 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/flyteorg/flyteadmin/pkg/clusterresource/interfaces"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/service"
+
+	"github.com/flyteorg/flyteadmin/pkg/clusterresource/interfaces"
+	"github.com/flyteorg/flyteadmin/pkg/common"
+	"github.com/flyteorg/flyteadmin/pkg/repositories/models"
 )
 
 // Implementation of an interfaces.FlyteAdminDataProvider which fetches data using a flyteadmin service client
@@ -32,6 +35,13 @@ func (p serviceAdminProvider) GetClusterResourceAttributes(ctx context.Context,
 
 var activeProjectsFilter = fmt.Sprintf("ne(state,%d)", admin.Project_ARCHIVED)
 
+var descCreatedAtSortParam = admin.Sort{
+	Direction: admin.Sort_DESCENDING,
+	Key:       "created_at",
+}
+
+var descCreatedAtSortDBParam, _ = common.NewSortParameter(&descCreatedAtSortParam, models.ProjectColumns)
+
 func (p serviceAdminProvider) GetProjects(ctx context.Context) (*admin.Projects, error) {
 	projects := make([]*admin.Project, 0)
 	listReq := &admin.ProjectListRequest{

diff --git a/pkg/clusterresource/impl/shared.go b/pkg/clusterresource/impl/shared.go
@@ -1,19 +1,10 @@
 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"
 )
 
 func NewMissingEntityError(entity string) error {
 	return errors.NewFlyteAdminErrorf(codes.NotFound, "Failed to find [%s]", entity)
 }
-
-var descCreatedAtSortParam = admin.Sort{
-	Direction: admin.Sort_DESCENDING,
-	Key:       "created_at",
-}
-
-var descCreatedAtSortDBParam, _ = common.NewSortParameter(descCreatedAtSortParam)
diff --git a/pkg/common/sorting.go b/pkg/common/sorting.go
@@ -2,6 +2,7 @@ package common
 
 import (
 	"fmt"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/flyteorg/flyteadmin/pkg/errors"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
@@ -23,13 +24,22 @@ 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 sort == nil {
+		return nil, nil
+	}
+
+	key := sort.Key
+	if !allowed.Has(key) {
+		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort key '%s'", key)
+	}
+
 	var gormOrderExpression string
 	switch sort.Direction {
 	case admin.Sort_DESCENDING:
-		gormOrderExpression = fmt.Sprintf(gormDescending, sort.Key)
+		gormOrderExpression = fmt.Sprintf(gormDescending, key)
 	case admin.Sort_ASCENDING:
-		gormOrderExpression = fmt.Sprintf(gormAscending, sort.Key)
+		gormOrderExpression = fmt.Sprintf(gormAscending, key)
 	default:
 		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid sort order specified: %v", sort)
 	}

diff --git a/pkg/common/sorting_test.go b/pkg/common/sorting_test.go
@@ -3,24 +3,53 @@ package common
 import (
 	"testing"
 
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/stretchr/testify/assert"
 )
 
+func TestSortParameter_Nil(t *testing.T) {
+	sortParameter, err := NewSortParameter(nil, nil)
+
+	assert.NoError(t, err)
+	assert.Nil(t, sortParameter)
+}
+
+func TestSortParameter_InvalidSortKey(t *testing.T) {
+	_, err := NewSortParameter(&admin.Sort{
+		Direction: admin.Sort_ASCENDING,
+		Key:       "wrong",
+	}, sets.NewString("name"))
+
+	assert.EqualError(t, err, "invalid sort key 'wrong'")
+}
+
+func TestSortParameter_InvalidSortDirection(t *testing.T) {
+	_, err := NewSortParameter(&admin.Sort{
+		Direction: 2,
+		Key:       "name",
+	}, sets.NewString("name"))
+
+	assert.EqualError(t, err, `invalid sort order specified: key:"name" direction:2 `)
+}
+
 func TestSortParameter_Ascending(t *testing.T) {
-	sortParameter, err := NewSortParameter(admin.Sort{
+	sortParameter, err := NewSortParameter(&admin.Sort{
 		Direction: admin.Sort_ASCENDING,
 		Key:       "name",
-	})
-	assert.Nil(t, err)
+	}, sets.NewString("name"))
+
+	assert.NoError(t, err)
 	assert.Equal(t, "name asc", sortParameter.GetGormOrderExpr())
 }
 
 func TestSortParameter_Descending(t *testing.T) {
-	sortParameter, err := NewSortParameter(admin.Sort{
+	sortParameter, err := NewSortParameter(&admin.Sort{
 		Direction: admin.Sort_DESCENDING,
 		Key:       "project",
-	})
-	assert.Nil(t, err)
+	}, sets.NewString("project"))
+
+	assert.NoError(t, err)
 	assert.Equal(t, "project desc", sortParameter.GetGormOrderExpr())
 }
diff --git a/pkg/manager/impl/description_entity_manager.go b/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"
 	repoInterfaces "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"
-	"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,13 +65,12 @@ 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
-		}
+
+	sortParameter, err := common.NewSortParameter(request.SortBy, models.DescriptionEntityColumns)
+	if err != nil {
+		return nil, err
 	}
+
 	offset, err := validation.ValidateToken(request.Token)
 	if err != nil {
 		return nil, errors.NewFlyteAdminErrorf(codes.InvalidArgument,

diff --git a/pkg/manager/impl/execution_manager.go b/pkg/manager/impl/execution_manager.go
@@ -6,34 +6,32 @@ 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"
@@ -42,13 +40,7 @@ import (
 	"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 +1426,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
-		}
+
+	sortParameter, err := common.NewSortParameter(request.SortBy, models.ExecutionColumns)
+	if err != nil {
+		return nil, err
 	}
 
 	offset, err := validation.ValidateToken(request.Token)

diff --git a/pkg/manager/impl/execution_manager_test.go b/pkg/manager/impl/execution_manager_test.go
@@ -3,64 +3,54 @@ package impl
 import (
 	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
-
-	"github.com/flyteorg/flyteadmin/plugins"
-
-	"google.golang.org/grpc/status"
-
-	"google.golang.org/protobuf/types/known/timestamppb"
+	"time"
 
 	"github.com/benbjohnson/clock"
-	"github.com/flyteorg/flyteadmin/pkg/common"
-	commonTestUtils "github.com/flyteorg/flyteadmin/pkg/common/testutils"
-	flyteAdminErrors "github.com/flyteorg/flyteadmin/pkg/errors"
-	"github.com/flyteorg/flyteadmin/pkg/manager/impl/executions"
-	"github.com/flyteorg/flyteadmin/pkg/manager/impl/shared"
-	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/admin"
+	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/core"
 	"github.com/flyteorg/flyteidl/gen/pb-go/flyteidl/event"
+	mockScope "github.com/flyteorg/flytestdlib/promutils"
+	"github.com/flyteorg/flytestdlib/storage"
 	"github.com/gogo/protobuf/jsonpb"
+	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes"
+	"github.com/golang/protobuf/ptypes/wrappers"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"google.golang.org/grpc/codes"
-
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/types/known/timestamppb"
 	"k8s.io/apimachinery/pkg/api/resource"
-
-	eventWriterMocks "github.com/flyteorg/flyteadmin/pkg/async/events/mocks"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/flyteorg/flyteadmin/auth"
-
-	commonMocks "github.com/flyteorg/flyteadmin/pkg/common/mocks"
-
-	"github.com/flyteorg/flytestdlib/storage"
-
-	"time"
-
-	"fmt"
-
+	eventWriterMocks "github.com/flyteorg/flyteadmin/pkg/async/events/mocks"
 	notificationMocks "github.com/flyteorg/flyteadmin/pkg/async/notifications/mocks"
+	"github.com/flyteorg/flyteadmin/pkg/common"
+	commonMocks "github.com/flyteorg/flyteadmin/pkg/common/mocks"
+	commonTestUtils "github.com/flyteorg/flyteadmin/pkg/common/testutils"
 	dataMocks "github.com/flyteorg/flyteadmin/pkg/data/mocks"
+	flyteAdminErrors "github.com/flyteorg/flyteadmin/pkg/errors"
+	"github.com/flyteorg/flyteadmin/pkg/manager/impl/executions"
+	"github.com/flyteorg/flyteadmin/pkg/manager/impl/shared"
 	"github.com/flyteorg/flyteadmin/pkg/manager/impl/testutils"
+	managerInterfaces "github.com/flyteorg/flyteadmin/pkg/manager/interfaces"
+	managerMocks "github.com/flyteorg/flyteadmin/pkg/manager/mocks"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/interfaces"
 	repositoryMocks "github.com/flyteorg/flyteadmin/pkg/repositories/mocks"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/models"
 	"github.com/flyteorg/flyteadmin/pkg/repositories/transformers"
+	"github.com/flyteorg/flyteadmin/pkg/runtime"
 	runtimeInterfaces "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces"
 	runtimeIFaceMocks "github.com/flyteorg/flyteadmin/pkg/runtime/interfaces/mocks"
 	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"
+	"github.com/flyteorg/flyteadmin/plugins"
 )
 
 var spec = testutils.GetExecutionRequest().Spec
@@ -2979,7 +2969,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, "execution_domain asc", input.SortParameter.GetGormOrderExpr())
 		assert.Equal(t, 2, input.Offset)
 		assert.EqualValues(t, map[common.Entity]bool{
 			common.Execution: true,
@@ -3027,7 +3017,7 @@ func TestListExecutions(t *testing.T) {
 		Limit: limit,
 		SortBy: &admin.Sort{
 			Direction: admin.Sort_ASCENDING,
-			Key:       "domain",
+			Key:       "execution_domain",
 		},
 		Token: "2",
 	})
@@ -3965,7 +3955,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, "execution_domain asc", input.SortParameter.GetGormOrderExpr())
 		assert.Equal(t, 2, input.Offset)
 		return interfaces.ExecutionCollectionOutput{
 			Executions: []models.Execution{
@@ -4010,7 +4000,7 @@ func TestListExecutions_LegacyModel(t *testing.T) {
 		Limit: limit,
 		SortBy: &admin.Sort{
 			Direction: admin.Sort_ASCENDING,
-			Key:       "domain",
+			Key:       "execution_domain",
 		},
 		Token: "2",
 	})