diff --git a/backend/src/apiserver/resource/BUILD.bazel b/backend/src/apiserver/resource/BUILD.bazel index 7bcce3a9a48..9274c460470 100644 --- a/backend/src/apiserver/resource/BUILD.bazel +++ b/backend/src/apiserver/resource/BUILD.bazel @@ -52,6 +52,7 @@ go_test( "//backend/src/crd/pkg/apis/scheduledworkflow/v1beta1:go_default_library", "@com_github_argoproj_argo//pkg/apis/workflow/v1alpha1:go_default_library", "@com_github_ghodss_yaml//:go_default_library", + "@com_github_google_go_cmp//cmp:go_default_library", "@com_github_pkg_errors//:go_default_library", "@com_github_spf13_viper//:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", diff --git a/backend/src/apiserver/resource/model_converter.go b/backend/src/apiserver/resource/model_converter.go index 630c2bc21f2..90f5ffc43e0 100644 --- a/backend/src/apiserver/resource/model_converter.go +++ b/backend/src/apiserver/resource/model_converter.go @@ -22,8 +22,27 @@ import ( "github.com/kubeflow/pipelines/backend/src/apiserver/common" "github.com/kubeflow/pipelines/backend/src/apiserver/model" "github.com/kubeflow/pipelines/backend/src/common/util" + "github.com/pkg/errors" ) +func (r *ResourceManager) ToModelExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) { + namespace := "" + resourceReferences := apiExperiment.GetResourceReferences() + if resourceReferences != nil { + if len(resourceReferences) != 1 || + resourceReferences[0].Key.Type != api.ResourceType_NAMESPACE || + resourceReferences[0].Relationship != api.Relationship_OWNER { + return nil, util.NewInternalServerError(errors.New("Invalid resource references for experiment"), "Unable to convert to model experiment.") + } + namespace = resourceReferences[0].Key.Id + } + return &model.Experiment{ + Name: apiExperiment.Name, + Description: apiExperiment.Description, + Namespace: namespace, + }, nil +} + func (r *ResourceManager) ToModelRunMetric(metric *api.RunMetric, runUUID string) *model.RunMetric { return &model.RunMetric{ RunUUID: runUUID, diff --git a/backend/src/apiserver/resource/model_converter_test.go b/backend/src/apiserver/resource/model_converter_test.go index 12f3ef0fd77..2824a8dec90 100644 --- a/backend/src/apiserver/resource/model_converter_test.go +++ b/backend/src/apiserver/resource/model_converter_test.go @@ -15,10 +15,12 @@ package resource import ( + "strings" "testing" "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1" "github.com/golang/protobuf/ptypes/timestamp" + "github.com/google/go-cmp/cmp" api "github.com/kubeflow/pipelines/backend/api/go_client" "github.com/kubeflow/pipelines/backend/src/apiserver/common" "github.com/kubeflow/pipelines/backend/src/apiserver/model" @@ -33,6 +35,93 @@ func initResourceManager() (*FakeClientManager, *ResourceManager) { return store, NewResourceManager(store) } +func TestToModelExperiment(t *testing.T) { + store, manager := initResourceManager() + defer store.Close() + + tests := []struct { + name string + experiment *api.Experiment + wantError bool + errorMessage string + expectedModelExperiment *model.Experiment + }{ + { + "No resource references", + &api.Experiment{ + Name: "exp1", + Description: "This is an experiment", + }, + false, + "", + &model.Experiment{ + Name: "exp1", + Description: "This is an experiment", + Namespace: "", + }, + }, + { + "Valid resource references", + &api.Experiment{ + Name: "exp1", + Description: "This is an experiment", + ResourceReferences: []*api.ResourceReference{ + &api.ResourceReference{ + Key: &api.ResourceKey{ + Type: api.ResourceType_NAMESPACE, + Id: "ns1", + }, + Relationship: api.Relationship_OWNER, + }, + }, + }, + false, + "", + &model.Experiment{ + Name: "exp1", + Description: "This is an experiment", + Namespace: "ns1", + }, + }, + { + "Invalid resource references", + &api.Experiment{ + Name: "exp1", + Description: "This is an experiment", + ResourceReferences: []*api.ResourceReference{ + &api.ResourceReference{ + Key: &api.ResourceKey{ + Type: api.ResourceType_EXPERIMENT, + Id: "invalid", + }, + Relationship: api.Relationship_OWNER, + }, + }, + }, + true, + "Invalid resource references for experiment", + nil, + }, + } + + for _, tc := range tests { + modelExperiment, err := manager.ToModelExperiment(tc.experiment) + if tc.wantError { + if err == nil { + t.Errorf("TestToModelExperiment(%v) expect error but got nil", tc.name) + } else if !strings.Contains(err.Error(), tc.errorMessage) { + t.Errorf("TestToModelExperiment(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err) + } + } else { + if err != nil { + t.Errorf("TestToModelExperiment(%v) expect no error but got %v", tc.name, err) + } else if !cmp.Equal(tc.expectedModelExperiment, modelExperiment) { + t.Errorf("TestToModelExperiment(%v) expect (%+v) but got (%+v)", tc.name, tc.expectedModelExperiment, modelExperiment) + } + } + } +} + func TestToModelRunMetric(t *testing.T) { store, manager := initResourceManager() defer store.Close() diff --git a/backend/src/apiserver/resource/resource_manager.go b/backend/src/apiserver/resource/resource_manager.go index a3ed4101326..51390913662 100644 --- a/backend/src/apiserver/resource/resource_manager.go +++ b/backend/src/apiserver/resource/resource_manager.go @@ -108,7 +108,11 @@ func (r *ResourceManager) GetTime() util.TimeInterface { return r.time } -func (r *ResourceManager) CreateExperiment(experiment *model.Experiment) (*model.Experiment, error) { +func (r *ResourceManager) CreateExperiment(apiExperiment *api.Experiment) (*model.Experiment, error) { + experiment, err := r.ToModelExperiment(apiExperiment) + if err != nil { + return nil, util.Wrap(err, "Failed to convert experiment model") + } return r.experimentStore.CreateExperiment(experiment) } @@ -116,9 +120,9 @@ func (r *ResourceManager) GetExperiment(experimentId string) (*model.Experiment, return r.experimentStore.GetExperiment(experimentId) } -func (r *ResourceManager) ListExperiments(opts *list.Options) ( +func (r *ResourceManager) ListExperiments(filterContext *common.FilterContext, opts *list.Options) ( experiments []*model.Experiment, total_size int, nextPageToken string, err error) { - return r.experimentStore.ListExperiments(opts) + return r.experimentStore.ListExperiments(filterContext, opts) } func (r *ResourceManager) DeleteExperiment(experimentID string) error { @@ -817,7 +821,7 @@ func (r *ResourceManager) CreateDefaultExperiment() (string, error) { } // Create default experiment - defaultExperiment := &model.Experiment{ + defaultExperiment := &api.Experiment{ Name: "Default", Description: "All runs created without specifying an experiment will be grouped here.", } @@ -1017,6 +1021,14 @@ func (r *ResourceManager) IsRequestAuthorized(userIdentity string, namespace str return r.kfamClient.IsAuthorized(userIdentity, namespace) } +func (r *ResourceManager) GetNamespaceFromExperimentID(experimentID string) (string, error) { + experiment, err := r.GetExperiment(experimentID) + if err != nil { + return "", util.Wrap(err, "Failed to get namespace from experiment ID.") + } + return experiment.Namespace, nil +} + func (r *ResourceManager) GetNamespaceFromRunID(runId string) (string, error) { runDetail, err := r.GetRun(runId) if err != nil { diff --git a/backend/src/apiserver/resource/resource_manager_test.go b/backend/src/apiserver/resource/resource_manager_test.go index 3fcdeb0a4ec..3b24684dc28 100644 --- a/backend/src/apiserver/resource/resource_manager_test.go +++ b/backend/src/apiserver/resource/resource_manager_test.go @@ -88,8 +88,8 @@ func initWithExperiment(t *testing.T) (*FakeClientManager, *ResourceManager, *mo initEnvVars() store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) manager := NewResourceManager(store) - experiment := &model.Experiment{Name: "e1"} - experiment, err := manager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "e1"} + experiment, err := manager.CreateExperiment(apiExperiment) assert.Nil(t, err) return store, manager, experiment } @@ -98,8 +98,8 @@ func initWithExperimentAndPipeline(t *testing.T) (*FakeClientManager, *ResourceM initEnvVars() store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) manager := NewResourceManager(store) - experiment := &model.Experiment{Name: "e1"} - experiment, err := manager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "e1"} + experiment, err := manager.CreateExperiment(apiExperiment) assert.Nil(t, err) pipeline, err := manager.CreatePipeline("p1", "", []byte(testWorkflow.ToStringForStore())) assert.Nil(t, err) @@ -307,8 +307,8 @@ func TestGetPipelineTemplate_PipelineFileNotFound(t *testing.T) { func TestCreateRun_ThroughPipelineID(t *testing.T) { store, manager, p := initWithPipeline(t) defer store.Close() - experiment := &model.Experiment{Name: "e1"} - experiment, err := manager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "e1"} + experiment, err := manager.CreateExperiment(apiExperiment) assert.Nil(t, err) apiRun := &api.Run{ Name: "run1", @@ -898,8 +898,8 @@ func TestCreateJob_ThroughWorkflowSpec(t *testing.T) { func TestCreateJob_ThroughPipelineID(t *testing.T) { store, manager, pipeline := initWithPipeline(t) defer store.Close() - experiment := &model.Experiment{Name: "e1"} - experiment, err := manager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "e1"} + experiment, err := manager.CreateExperiment(apiExperiment) job := &api.Job{ Name: "j1", Enabled: true, @@ -2290,3 +2290,46 @@ func TestDeletePipelineVersion_FileError(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, version) } + +func TestCreateDefaultExperiment(t *testing.T) { + store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + defer store.Close() + manager := NewResourceManager(store) + + experimentID, err := manager.CreateDefaultExperiment() + assert.Nil(t, err) + experiment, err := manager.GetExperiment(experimentID) + assert.Nil(t, err) + + expectedExperiment := &model.Experiment{ + UUID: DefaultFakeUUID, + CreatedAtInSec: 1, + Name: "Default", + Description: "All runs created without specifying an experiment will be grouped here.", + Namespace: "", + } + assert.Equal(t, expectedExperiment, experiment) +} + +func TestCreateDefaultExperiment_MultiUser(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + + store := NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + defer store.Close() + manager := NewResourceManager(store) + + experimentID, err := manager.CreateDefaultExperiment() + assert.Nil(t, err) + experiment, err := manager.GetExperiment(experimentID) + assert.Nil(t, err) + + expectedExperiment := &model.Experiment{ + UUID: DefaultFakeUUID, + CreatedAtInSec: 1, + Name: "Default", + Description: "All runs created without specifying an experiment will be grouped here.", + Namespace: "", + } + assert.Equal(t, expectedExperiment, experiment) +} diff --git a/backend/src/apiserver/server/api_converter.go b/backend/src/apiserver/server/api_converter.go index 451013ea050..c10288c2d35 100644 --- a/backend/src/apiserver/server/api_converter.go +++ b/backend/src/apiserver/server/api_converter.go @@ -26,11 +26,24 @@ import ( ) func ToApiExperiment(experiment *model.Experiment) *api.Experiment { + resourceReferences := []*api.ResourceReference(nil) + if common.IsMultiUserMode() { + resourceReferences = []*api.ResourceReference{ + &api.ResourceReference{ + Key: &api.ResourceKey{ + Type: api.ResourceType_NAMESPACE, + Id: experiment.Namespace, + }, + Relationship: api.Relationship_OWNER, + }, + } + } return &api.Experiment{ - Id: experiment.UUID, - Name: experiment.Name, - Description: experiment.Description, - CreatedAt: ×tamp.Timestamp{Seconds: experiment.CreatedAtInSec}, + Id: experiment.UUID, + Name: experiment.Name, + Description: experiment.Description, + CreatedAt: ×tamp.Timestamp{Seconds: experiment.CreatedAtInSec}, + ResourceReferences: resourceReferences, } } @@ -42,13 +55,6 @@ func ToApiExperiments(experiments []*model.Experiment) []*api.Experiment { return apiExperiments } -func ToModelExperiment(experiment *api.Experiment) *model.Experiment { - return &model.Experiment{ - Name: experiment.Name, - Description: experiment.Description, - } -} - func ToApiPipeline(pipeline *model.Pipeline) *api.Pipeline { params, err := toApiParameters(pipeline.Parameters) if err != nil { diff --git a/backend/src/apiserver/server/experiment_server.go b/backend/src/apiserver/server/experiment_server.go index 61ac9bfd49a..4ff394d804c 100644 --- a/backend/src/apiserver/server/experiment_server.go +++ b/backend/src/apiserver/server/experiment_server.go @@ -5,9 +5,11 @@ import ( "github.com/golang/protobuf/ptypes/empty" api "github.com/kubeflow/pipelines/backend/api/go_client" + "github.com/kubeflow/pipelines/backend/src/apiserver/common" "github.com/kubeflow/pipelines/backend/src/apiserver/model" "github.com/kubeflow/pipelines/backend/src/apiserver/resource" "github.com/kubeflow/pipelines/backend/src/common/util" + "github.com/pkg/errors" ) type ExperimentServer struct { @@ -20,7 +22,13 @@ func (s *ExperimentServer) CreateExperiment(ctx context.Context, request *api.Cr if err != nil { return nil, util.Wrap(err, "Validate experiment request failed.") } - newExperiment, err := s.resourceManager.CreateExperiment(ToModelExperiment(request.Experiment)) + + err = CanAccessNamespaceInResourceReferences(s.resourceManager, ctx, request.Experiment.ResourceReferences) + if err != nil { + return nil, util.Wrap(err, "Failed to authorize the request.") + } + + newExperiment, err := s.resourceManager.CreateExperiment(request.Experiment) if err != nil { return nil, util.Wrap(err, "Create experiment failed.") } @@ -29,6 +37,11 @@ func (s *ExperimentServer) CreateExperiment(ctx context.Context, request *api.Cr func (s *ExperimentServer) GetExperiment(ctx context.Context, request *api.GetExperimentRequest) ( *api.Experiment, error) { + err := s.canAccessExperiment(ctx, request.Id) + if err != nil { + return nil, util.Wrap(err, "Failed to authorize the request.") + } + experiment, err := s.resourceManager.GetExperiment(request.Id) if err != nil { return nil, util.Wrap(err, "Get experiment failed.") @@ -44,7 +57,27 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List return nil, util.Wrap(err, "Failed to create list options") } - experiments, total_size, nextPageToken, err := s.resourceManager.ListExperiments(opts) + filterContext, err := ValidateFilter(request.ResourceReferenceKey) + if err != nil { + return nil, util.Wrap(err, "Validating filter failed.") + } + + if common.IsMultiUserMode() { + refKey := filterContext.ReferenceKey + if refKey == nil || refKey.Type != common.Namespace { + return nil, util.NewInvalidInputError("Invalid resource references for experiment. ListExperiment requires filtering by namespace.") + } + if len(refKey.ID) == 0 { + return nil, util.NewInvalidInputError("Invalid resource references for experiment. Namespace is empty.") + } + } else { + // In single user mode, apply filter with empty namespace for backward compatibile. + filterContext = &common.FilterContext{ + ReferenceKey: &common.ReferenceKey{Type: common.Namespace, ID: ""}, + } + } + + experiments, total_size, nextPageToken, err := s.resourceManager.ListExperiments(filterContext, opts) if err != nil { return nil, util.Wrap(err, "List experiments failed.") } @@ -56,7 +89,12 @@ func (s *ExperimentServer) ListExperiment(ctx context.Context, request *api.List } func (s *ExperimentServer) DeleteExperiment(ctx context.Context, request *api.DeleteExperimentRequest) (*empty.Empty, error) { - err := s.resourceManager.DeleteExperiment(request.Id) + err := s.canAccessExperiment(ctx, request.Id) + if err != nil { + return nil, util.Wrap(err, "Failed to authorize the request.") + } + + err = s.resourceManager.DeleteExperiment(request.Id) if err != nil { return nil, err } @@ -67,6 +105,41 @@ func ValidateCreateExperimentRequest(request *api.CreateExperimentRequest) error if request.Experiment == nil || request.Experiment.Name == "" { return util.NewInvalidInputError("Experiment name is empty. Please specify a valid experiment name.") } + + if common.IsMultiUserMode() { + resourceReferences := request.Experiment.GetResourceReferences() + if len(resourceReferences) != 1 || + resourceReferences[0].Key.Type != api.ResourceType_NAMESPACE || + resourceReferences[0].Relationship != api.Relationship_OWNER { + return util.NewInvalidInputError( + "Invalid resource references for experiment. Expect one namespace type with owner relationship. Got: %v", resourceReferences) + } + namespace := common.GetNamespaceFromAPIResourceReferences(request.Experiment.ResourceReferences) + if len(namespace) == 0 { + return util.NewInvalidInputError("Invalid resource references for experiment. Namespace is empty.") + } + } + return nil +} + +// TODO(chensun): consider refactoring the code to get rid of double-query of experiment. +func (s *ExperimentServer) canAccessExperiment(ctx context.Context, experimentID string) error { + if !common.IsMultiUserMode() { + // Skip authorization if not multi-user mode. + return nil + } + namespace, err := s.resourceManager.GetNamespaceFromExperimentID(experimentID) + if err != nil { + return util.Wrap(err, "Failed to authorize with the experiment ID.") + } + if len(namespace) == 0 { + return util.NewInternalServerError(errors.New("Empty namespace"), "The experiment doesn't have a valid namespace.") + } + + err = isAuthorized(s.resourceManager, ctx, namespace) + if err != nil { + return util.Wrap(err, "Failed to authorize with API resource references") + } return nil } diff --git a/backend/src/apiserver/server/experiment_server_test.go b/backend/src/apiserver/server/experiment_server_test.go index 28cb34221d8..e9b6e33d8b9 100644 --- a/backend/src/apiserver/server/experiment_server_test.go +++ b/backend/src/apiserver/server/experiment_server_test.go @@ -1,13 +1,19 @@ package server import ( + "context" + "strings" "testing" "github.com/golang/protobuf/ptypes/timestamp" + "github.com/google/go-cmp/cmp" api "github.com/kubeflow/pipelines/backend/api/go_client" + "github.com/kubeflow/pipelines/backend/src/apiserver/common" "github.com/kubeflow/pipelines/backend/src/apiserver/resource" "github.com/kubeflow/pipelines/backend/src/common/util" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" + "google.golang.org/grpc/metadata" ) func TestCreateExperiment(t *testing.T) { @@ -38,6 +44,39 @@ func TestCreateExperiment_Failed(t *testing.T) { assert.Contains(t, err.Error(), "Create experiment failed.") } +func TestCreateExperiment_Multiuser(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) + ctx := metadata.NewIncomingContext(context.Background(), md) + + clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + resourceManager := resource.NewResourceManager(clientManager) + server := ExperimentServer{resourceManager: resourceManager} + resourceReferences := []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + } + experiment := &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: resourceReferences, + } + + result, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment}) + assert.Nil(t, err) + expectedExperiment := &api.Experiment{ + Id: resource.DefaultFakeUUID, + Name: "exp1", + Description: "first experiment", + CreatedAt: ×tamp.Timestamp{Seconds: 1}, + ResourceReferences: resourceReferences, + } + assert.Equal(t, expectedExperiment, result) +} + func TestGetExperiment(t *testing.T) { clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) resourceManager := resource.NewResourceManager(clientManager) @@ -70,6 +109,40 @@ func TestGetExperiment_Failed(t *testing.T) { assert.Contains(t, err.Error(), "Get experiment failed.") } +func TestGetExperiment_Multiuser(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) + ctx := metadata.NewIncomingContext(context.Background(), md) + + clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + resourceManager := resource.NewResourceManager(clientManager) + server := ExperimentServer{resourceManager: resourceManager} + resourceReferences := []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + } + experiment := &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: resourceReferences, + } + + createResult, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment}) + assert.Nil(t, err) + result, err := server.GetExperiment(ctx, &api.GetExperimentRequest{Id: createResult.Id}) + expectedExperiment := &api.Experiment{ + Id: createResult.Id, + Name: "exp1", + Description: "first experiment", + CreatedAt: ×tamp.Timestamp{Seconds: 1}, + ResourceReferences: resourceReferences, + } + assert.Equal(t, expectedExperiment, result) +} + func TestListExperiment(t *testing.T) { clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) resourceManager := resource.NewResourceManager(clientManager) @@ -102,9 +175,250 @@ func TestListExperiment_Failed(t *testing.T) { assert.Contains(t, err.Error(), "List experiments failed.") } -func TestValidateCreateExperimentRequest_EmptyName(t *testing.T) { - experiment := &api.Experiment{Description: "first experiment"} - err := ValidateCreateExperimentRequest(&api.CreateExperimentRequest{Experiment: experiment}) - assert.NotNil(t, err) - assert.Contains(t, err.Error(), "name is empty") +func TestListExperiment_Multiuser(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) + ctx := metadata.NewIncomingContext(context.Background(), md) + + clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) + resourceManager := resource.NewResourceManager(clientManager) + server := ExperimentServer{resourceManager: resourceManager} + resourceReferences := []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + } + experiment := &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: resourceReferences, + } + + createResult, err := server.CreateExperiment(ctx, &api.CreateExperimentRequest{Experiment: experiment}) + assert.Nil(t, err) + + tests := []struct { + name string + request *api.ListExperimentsRequest + wantError bool + errorMessage string + expectedExperiments []*api.Experiment + }{ + { + "Valid", + &api.ListExperimentsRequest{ + ResourceReferenceKey: &api.ResourceKey{ + Type: api.ResourceType_NAMESPACE, + Id: "ns1", + }, + }, + false, + "", + []*api.Experiment{{ + Id: createResult.Id, + Name: "exp1", + Description: "first experiment", + CreatedAt: ×tamp.Timestamp{Seconds: 1}, + ResourceReferences: resourceReferences, + }}, + }, + { + "Valid but empty result", + &api.ListExperimentsRequest{ + ResourceReferenceKey: &api.ResourceKey{ + Type: api.ResourceType_NAMESPACE, + Id: "ns2", + }, + }, + false, + "", + []*api.Experiment{}, + }, + { + "Missing resource reference key", + &api.ListExperimentsRequest{}, + true, + "Invalid resource references for experiment.", + nil, + }, + { + "Invalid resource reference key type", + &api.ListExperimentsRequest{ + ResourceReferenceKey: &api.ResourceKey{ + Type: api.ResourceType_EXPERIMENT, + Id: "fake_id", + }, + }, + true, + "Invalid resource references for experiment.", + nil, + }, + { + "Empty namespace", + &api.ListExperimentsRequest{ + ResourceReferenceKey: &api.ResourceKey{ + Type: api.ResourceType_NAMESPACE, + Id: "", + }, + }, + true, + "Invalid resource references for experiment. Namespace is empty.", + nil, + }, + } + + for _, tc := range tests { + response, err := server.ListExperiment(ctx, tc.request) + if tc.wantError { + if err == nil { + t.Errorf("TestListExperiment_Multiuser(%v) expect error but got nil", tc.name) + } else if !strings.Contains(err.Error(), tc.errorMessage) { + t.Errorf("TestListExperiment_Multiusert(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err) + } + } else { + if err != nil { + t.Errorf("TestListExperiment_Multiuser(%v) expect no error but got %v", tc.name, err) + } else if !cmp.Equal(tc.expectedExperiments, response.Experiments) { + t.Errorf("TestListExperiment_Multiuser(%v) expect (%+v) but got (%+v)", tc.name, tc.expectedExperiments, response.Experiments) + } + } + } +} + +func TestValidateCreateExperimentRequest(t *testing.T) { + tests := []struct { + name string + experiment *api.Experiment + wantError bool + errorMessage string + }{ + { + "Valid", + &api.Experiment{Name: "exp1", Description: "first experiment"}, + false, + "", + }, + { + "Empty name", + &api.Experiment{Description: "first experiment"}, + true, + "name is empty", + }, + } + + for _, tc := range tests { + err := ValidateCreateExperimentRequest(&api.CreateExperimentRequest{Experiment: tc.experiment}) + if !tc.wantError && err != nil { + t.Errorf("TestValidateCreateExperimentRequest(%v) expect no error but got %v", tc.name, err) + } + if tc.wantError { + if err == nil { + t.Errorf("TestValidateCreateExperimentRequest(%v) expect error but got nil", tc.name) + } else if !strings.Contains(err.Error(), tc.errorMessage) { + t.Errorf("TestValidateCreateExperimentRequest(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err) + } + } + } +} + +func TestValidateCreateExperimentRequest_Multiuser(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + tests := []struct { + name string + experiment *api.Experiment + wantError bool + errorMessage string + }{ + { + "Valid", + &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + }, + }, + false, + "", + }, + { + "Missing namespace", + &api.Experiment{ + Name: "exp1", + Description: "first experiment", + }, + true, + "Invalid resource references for experiment.", + }, + { + "Empty namespace", + &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: ""}, + Relationship: api.Relationship_OWNER, + }, + }, + }, + true, + "Invalid resource references for experiment. Namespace is empty.", + }, + { + "Multiple namespace", + &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns2"}, + Relationship: api.Relationship_OWNER, + }, + }, + }, + true, + "Invalid resource references for experiment.", + }, + { + "Invalid resource type", + &api.Experiment{ + Name: "exp1", + Description: "first experiment", + ResourceReferences: []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_EXPERIMENT, Id: "exp2"}, + Relationship: api.Relationship_OWNER, + }, + }, + }, + true, + "Invalid resource references for experiment.", + }, + } + + for _, tc := range tests { + err := ValidateCreateExperimentRequest(&api.CreateExperimentRequest{Experiment: tc.experiment}) + if !tc.wantError && err != nil { + t.Errorf("TestValidateCreateExperimentRequest(%v) expect no error but got %v", tc.name, err) + } + if tc.wantError { + if err == nil { + t.Errorf("TestValidateCreateExperimentRequest(%v) expect error but got nil", tc.name) + } else if !strings.Contains(err.Error(), tc.errorMessage) { + t.Errorf("TestValidateCreateExperimentRequest(%v) expect error containing: %v, but got: %v", tc.name, tc.errorMessage, err) + } + } + } } diff --git a/backend/src/apiserver/server/run_server_test.go b/backend/src/apiserver/server/run_server_test.go index a01644674b7..cff6d0def02 100644 --- a/backend/src/apiserver/server/run_server_test.go +++ b/backend/src/apiserver/server/run_server_test.go @@ -377,10 +377,13 @@ func TestReportRunMetrics_PartialFailures(t *testing.T) { } func TestCanAccessRun_Unauthorized(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + clients, manager, experiment := initWithExperiment_KFAM_Unauthorized(t) defer clients.Close() runServer := RunServer{resourceManager: manager} - viper.Set(common.MultiUserMode, "true") + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) ctx := metadata.NewIncomingContext(context.Background(), md) @@ -410,10 +413,13 @@ func TestCanAccessRun_Unauthorized(t *testing.T) { } func TestCanAccessRun_Authorized(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + clients, manager, experiment := initWithExperiment(t) defer clients.Close() runServer := RunServer{resourceManager: manager} - viper.Set(common.MultiUserMode, "true") + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) ctx := metadata.NewIncomingContext(context.Background(), md) diff --git a/backend/src/apiserver/server/test_util.go b/backend/src/apiserver/server/test_util.go index 779de5830de..f3bd28f6e71 100644 --- a/backend/src/apiserver/server/test_util.go +++ b/backend/src/apiserver/server/test_util.go @@ -82,8 +82,19 @@ func initWithExperiment(t *testing.T) (*resource.FakeClientManager, *resource.Re initEnvVars() clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) resourceManager := resource.NewResourceManager(clientManager) - experiment := &model.Experiment{Name: "123"} - experiment, err := resourceManager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "123"} + if common.IsMultiUserMode() { + apiExperiment = &api.Experiment{ + Name: "123", + ResourceReferences: []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + }, + } + } + experiment, err := resourceManager.CreateExperiment(apiExperiment) assert.Nil(t, err) return clientManager, resourceManager, experiment } @@ -93,8 +104,19 @@ func initWithExperiment_KFAM_Unauthorized(t *testing.T) (*resource.FakeClientMan clientManager := resource.NewFakeClientManagerOrFatal(util.NewFakeTimeForEpoch()) clientManager.KfamClientFake = client.NewFakeKFAMClientUnauthorized() resourceManager := resource.NewResourceManager(clientManager) - experiment := &model.Experiment{Name: "123"} - experiment, err := resourceManager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "123"} + if common.IsMultiUserMode() { + apiExperiment = &api.Experiment{ + Name: "123", + ResourceReferences: []*api.ResourceReference{ + { + Key: &api.ResourceKey{Type: api.ResourceType_NAMESPACE, Id: "ns1"}, + Relationship: api.Relationship_OWNER, + }, + }, + } + } + experiment, err := resourceManager.CreateExperiment(apiExperiment) assert.Nil(t, err) return clientManager, resourceManager, experiment } @@ -105,8 +127,8 @@ func initWithExperimentAndPipelineVersion(t *testing.T) (*resource.FakeClientMan resourceManager := resource.NewResourceManager(clientManager) // Create an experiment. - experiment := &model.Experiment{Name: "123"} - experiment, err := resourceManager.CreateExperiment(experiment) + apiExperiment := &api.Experiment{Name: "123"} + experiment, err := resourceManager.CreateExperiment(apiExperiment) assert.Nil(t, err) // Create a pipeline and then a pipeline version. diff --git a/backend/src/apiserver/server/util_test.go b/backend/src/apiserver/server/util_test.go index cb9515d3a91..957eed14884 100644 --- a/backend/src/apiserver/server/util_test.go +++ b/backend/src/apiserver/server/util_test.go @@ -346,9 +346,12 @@ func TestGetUserIdentity(t *testing.T) { } func TestCanAccessNamespaceInResourceReferencesUnauthorized(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + clients, manager, _ := initWithExperiment_KFAM_Unauthorized(t) defer clients.Close() - viper.Set(common.MultiUserMode, "true") + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) ctx := metadata.NewIncomingContext(context.Background(), md) references := []*api.ResourceReference{ @@ -363,9 +366,12 @@ func TestCanAccessNamespaceInResourceReferencesUnauthorized(t *testing.T) { } func TestCanAccessNamespaceInResourceReferences_Authorized(t *testing.T) { + viper.Set(common.MultiUserMode, "true") + defer viper.Set(common.MultiUserMode, "false") + clients, manager, _ := initWithExperiment(t) defer clients.Close() - viper.Set(common.MultiUserMode, "true") + md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"}) ctx := metadata.NewIncomingContext(context.Background(), md) references := []*api.ResourceReference{ diff --git a/backend/src/apiserver/storage/experiment_store.go b/backend/src/apiserver/storage/experiment_store.go index d62998f9c22..cd70ebe930e 100644 --- a/backend/src/apiserver/storage/experiment_store.go +++ b/backend/src/apiserver/storage/experiment_store.go @@ -14,7 +14,7 @@ import ( ) type ExperimentStoreInterface interface { - ListExperiments(opts *list.Options) ([]*model.Experiment, int, string, error) + ListExperiments(filterContext *common.FilterContext, opts *list.Options) ([]*model.Experiment, int, string, error) GetExperiment(uuid string) (*model.Experiment, error) CreateExperiment(*model.Experiment) (*model.Experiment, error) DeleteExperiment(uuid string) error @@ -30,13 +30,18 @@ type ExperimentStore struct { // Runs two SQL queries in a transaction to return a list of matching experiments, as well as their // total_size. The total_size does not reflect the page size. -func (s *ExperimentStore) ListExperiments(opts *list.Options) ([]*model.Experiment, int, string, error) { +func (s *ExperimentStore) ListExperiments(filterContext *common.FilterContext, opts *list.Options) ([]*model.Experiment, int, string, error) { errorF := func(err error) ([]*model.Experiment, int, string, error) { return nil, 0, "", util.NewInternalServerError(err, "Failed to list experiments: %v", err) } // SQL for getting the filtered and paginated rows - sqlBuilder := opts.AddFilterToSelect(sq.Select("*").From("experiments")) + sqlBuilder := sq.Select("*").From("experiments") + if filterContext.ReferenceKey != nil && filterContext.ReferenceKey.Type == common.Namespace { + sqlBuilder = sqlBuilder.Where(sq.Eq{"Namespace": filterContext.ReferenceKey.ID}) + } + sqlBuilder = opts.AddFilterToSelect(sqlBuilder) + rowsSql, rowsArgs, err := opts.AddPaginationToSelect(sqlBuilder).ToSql() if err != nil { return errorF(err) @@ -44,7 +49,11 @@ func (s *ExperimentStore) ListExperiments(opts *list.Options) ([]*model.Experime // SQL for getting total size. This matches the query to get all the rows above, in order // to do the same filter, but counts instead of scanning the rows. - sizeSql, sizeArgs, err := opts.AddFilterToSelect(sq.Select("count(*)").From("experiments")).ToSql() + sqlBuilder = sq.Select("count(*)").From("experiments") + if filterContext.ReferenceKey != nil && filterContext.ReferenceKey.Type == common.Namespace { + sqlBuilder = sqlBuilder.Where(sq.Eq{"Namespace": filterContext.ReferenceKey.ID}) + } + sizeSql, sizeArgs, err := opts.AddFilterToSelect(sqlBuilder).ToSql() if err != nil { return errorF(err) } diff --git a/backend/src/apiserver/storage/experiment_store_test.go b/backend/src/apiserver/storage/experiment_store_test.go index bbbac976a2e..8148a05bac3 100644 --- a/backend/src/apiserver/storage/experiment_store_test.go +++ b/backend/src/apiserver/storage/experiment_store_test.go @@ -6,6 +6,7 @@ import ( "fmt" api "github.com/kubeflow/pipelines/backend/api/go_client" + "github.com/kubeflow/pipelines/backend/src/apiserver/common" "github.com/kubeflow/pipelines/backend/src/apiserver/list" "github.com/kubeflow/pipelines/backend/src/apiserver/model" "github.com/kubeflow/pipelines/backend/src/common/util" @@ -60,7 +61,7 @@ func TestListExperiments_Pagination(t *testing.T) { opts, err := list.NewOptions(&model.Experiment{}, 2, "name", nil) assert.Nil(t, err) - experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts) assert.Nil(t, err) assert.NotEmpty(t, nextPageToken) @@ -84,7 +85,7 @@ func TestListExperiments_Pagination(t *testing.T) { opts, err = list.NewOptionsFromToken(nextPageToken, 2) assert.Nil(t, err) - experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(&common.FilterContext{}, opts) assert.Nil(t, err) assert.Empty(t, nextPageToken) assert.Equal(t, 4, total_size) @@ -119,7 +120,7 @@ func TestListExperiments_Pagination_Descend(t *testing.T) { opts, err := list.NewOptions(&model.Experiment{}, 2, "name desc", nil) assert.Nil(t, err) - experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts) assert.Nil(t, err) assert.NotEmpty(t, nextPageToken) @@ -143,7 +144,7 @@ func TestListExperiments_Pagination_Descend(t *testing.T) { opts, err = list.NewOptionsFromToken(nextPageToken, 2) assert.Nil(t, err) - experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(&common.FilterContext{}, opts) assert.Nil(t, err) assert.Empty(t, nextPageToken) assert.Equal(t, 4, total_size) @@ -166,7 +167,7 @@ func TestListExperiments_Pagination_LessThanPageSize(t *testing.T) { opts, err := list.NewOptions(&model.Experiment{}, 2, "", nil) assert.Nil(t, err) - experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts) assert.Nil(t, err) assert.Equal(t, "", nextPageToken) assert.Equal(t, 1, total_size) @@ -181,7 +182,7 @@ func TestListExperimentsError(t *testing.T) { opts, err := list.NewOptions(&model.Experiment{}, 2, "", nil) assert.Nil(t, err) - _, _, _, err = experimentStore.ListExperiments(opts) + _, _, _, err = experimentStore.ListExperiments(&common.FilterContext{}, opts) assert.Equal(t, codes.Internal, err.(*util.UserError).ExternalStatusCode()) } @@ -368,7 +369,7 @@ func TestListExperiments_Filtering(t *testing.T) { opts, err := list.NewOptions(&model.Experiment{}, 2, "id", filterProto) assert.Nil(t, err) - experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err := experimentStore.ListExperiments(&common.FilterContext{}, opts) expected := []*model.Experiment{ &model.Experiment{ @@ -394,7 +395,7 @@ func TestListExperiments_Filtering(t *testing.T) { opts, err = list.NewOptionsFromToken(nextPageToken, 2) assert.Nil(t, err) - experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(opts) + experiments, total_size, nextPageToken, err = experimentStore.ListExperiments(&common.FilterContext{}, opts) expected = []*model.Experiment{ &model.Experiment{