[Backend][Multi-user] Add namespace to Experiment API implementation #3243

chensun · 2020-03-09T22:59:49Z

Conduct user authorization on namespace for the following experiment APIs:

CreateExperiment
GetExperiment
ListExperiment
DeleteExperiment

/cc @IronPan @Bobgy

Image for testing: gcr.io/chesu-dev/api-server:pr3243

This change is

chensun · 2020-03-12T21:08:26Z

/retest

Bobgy · 2020-03-13T09:57:56Z

/assign @Bobgy

Bobgy · 2020-03-13T10:06:24Z

Shall we add some tests when multi user mode is on for resource_manager_test, api_converter_test? Because there will be new code paths just for that.

Bobgy

Great work!
/lgtm

Now I realize you emphasized a lot on integration tests for the complete experiment server from end to end. I think that's a good practice, but it's different from existing pattern. It seems with existing pattern, we need some tests for each unit.
/cc @IronPan
for reviewing this part.

I'm okay with your choices because I don't have much context on which would be better and why it is like current convention.

backend/src/apiserver/server/api_converter.go

backend/src/apiserver/resource/model_converter.go

Bobgy · 2020-03-13T10:03:12Z

backend/src/apiserver/resource/resource_manager.go

@@ -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.",
 	}


There is no namespace reference here, so it will always fail in multi user mode.

Shall we add a check earlier? We just skip trying to create a default namespace when in multi user mode?

Great catch! Apparently this is one place I missed unit test coverage, because this default experiment creation doesn't hit experiment_server.
To allow creating default experiment in multi-user mode, I moved the namespace requirement out of ToModelExperiment -- it is enforced in experimentServer.CreateExperiment which is sufficient to capture invalid input. Also added unit tests for CreateDefaultExperiment and ToModelExperiment

I don't fully remember your design about default experiment. If default experiment doesn't contain a namespace, it will be useless in multi user mode.

Maybe we should just throw an error message here for multi user mode like Please specify an experiment. No default experiment has been set up.?

The plan is:

for short term, we provide a SQL script to help users backfill existing experiments including the default one.

long term, we will automate the backfill in api-server using the namespace specified by a config map/environment variable.
So allowing the default experiment without namespace is okay for now.

backend/src/apiserver/server/experiment_server.go

Bobgy

Now I understand, most of your added code is in experiment_server.go, so you tested it the most. Sounds like adding minimal test for other small changes in each unit will be an additional good thing.

backend/src/apiserver/server/experiment_server.go

chensun · 2020-03-14T08:01:01Z

Great work!
/lgtm

Now I realize you emphasized a lot on integration tests for the complete experiment server from end to end. I think that's a good practice, but it's different from existing pattern. It seems with existing pattern, we need some tests for each unit.
/cc @IronPan
for reviewing this part.

I'm okay with your choices because I don't have much context on which would be better and why it is like current convention.

While there're things unit tests cannot really verify - such as the real authorization, I'm with you that unit test is critical and should not be missed -- this is also one of my top complains about our code base. So I added more unit tests following your comments -- thanks again for catching the issue where Default experiment creation would fail in multi-user mode.
Let me know if you found other pieces missing unit tests. Regarding CreateExperiment/GetExperiment/ListExperiments, we have the unit tests in experiment_server_test.go but not in resource_manager_test.go, which I think is probably okay, since experiment_server is a thin wrapper layer, it mostly passes the request to resources_manager. If we have tests in both places, they will likely look almost identical.

chensun · 2020-03-16T23:18:52Z

/assign @IronPan

IronPan · 2020-03-16T23:31:06Z

backend/src/apiserver/server/experiment_server.go

+		// Skip authorization if not multi-user mode.
+		return nil
+	}
+	namespace, err := s.resourceManager.GetNamespaceFromExperimentID(experimentID)


does it make sense to move the GetNamespaceFromExperimentID to inline here?

The difference is probably trivial, but I feel like having this wrapper in resource_manager.go is slightly better. It keeps the experiment model details within resource_manager and hides it away from experiment_server.
That said, we might refactor the code to get rid of this canAccessExperiment (just added a TODO), so this is subject to change. So I think we can just keep it as-is for now.

IronPan · 2020-03-16T23:39:42Z

backend/src/apiserver/server/api_converter.go

+			Name:        experiment.Name,
+			Description: experiment.Description,
+			CreatedAt:   &timestamp.Timestamp{Seconds: experiment.CreatedAtInSec},
+			ResourceReferences: []*api.ResourceReference{


Can we dedup the common code and only add RR field in multi-user mode?

backend/src/apiserver/server/experiment_server.go

IronPan · 2020-03-17T06:20:55Z

/lgtm

Bobgy

/lgtm
Looks great @chensun!

Bobgy · 2020-03-17T10:06:27Z

backend/src/apiserver/server/experiment_server.go

@@ -122,7 +133,7 @@ func (s *ExperimentServer) canAccessExperiment(ctx context.Context, experimentID
 		return util.Wrap(err, "Failed to authorize with the experiment ID.")
 	}
 	if len(namespace) == 0 {
-		return util.NewInternalServerError(errors.New("There is no namespace found"), "There is no namespace found")
+		return util.NewInternalServerError(errors.New("Empty namespace"), "The experiment doesn't have a valid namespace.")


nit: not a big issue, would it make sense to allow access to experiment with namespace="" blindly?
because these namespaces are coming from before multi user mode was enabled.

I think you can decide whatever makes sense to you.

Initially, I allow listing experiments with empty namespace, but reject accessing experiment with empty namespace. The idea was to allow user see a list, but when they click on it, they get an error message.
Now that we disallowed empty namespace during listing, the experiments with empty namespace is almost invisible. So I'm leaning towards keeping it consistent for both list and get.
Eventually, this shouldn't happen once backfill is done (either via a SQL script or by automated backfill from api-server).

Bobgy · 2020-03-17T10:07:36Z

backend/src/apiserver/server/experiment_server_test.go

-		ResourceReferences: resourceReferences,
-	}}
-	assert.Equal(t, expectedExperiment, result.Experiments)
+	tests := []struct {


Great work! Thanks for improving unit tests!

chensun · 2020-03-17T17:31:36Z

/retest

chensun · 2020-03-17T18:38:27Z

/retest

IronPan · 2020-03-17T21:30:31Z

/lgtm
/approve

k8s-ci-robot · 2020-03-17T21:30:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~backend/OWNERS~~ [IronPan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ubeflow#3243) * Experiment API implementation * Address review comments and add additional unit tests * Update build file * fix unit tests * Reject empty namespace in multi user mode * Address review comments

Experiment API implementation

b70e7ff

k8s-ci-robot requested review from Bobgy and IronPan March 9, 2020 22:59

k8s-ci-robot added the size/L label Mar 9, 2020

chensun mentioned this pull request Mar 12, 2020

[UI] Pass namespace to experiment api calls #3252

Merged

k8s-ci-robot assigned Bobgy Mar 13, 2020

Bobgy reviewed Mar 13, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Mar 13, 2020

Bobgy reviewed Mar 13, 2020

View reviewed changes

backend/src/apiserver/server/experiment_server.go Outdated Show resolved Hide resolved

Bobgy reviewed Mar 13, 2020

View reviewed changes

backend/src/apiserver/server/experiment_server.go Show resolved Hide resolved

backend/src/apiserver/server/experiment_server.go Outdated Show resolved Hide resolved

This was referenced Mar 13, 2020

[SDK][Multi-user] Add namespace to experiment SDK calls #3272

Merged

[Multi user] Backend changes for experiment - filter experiment by namespace #3273

Closed

chensun changed the title ~~[Backend][Multiuser] Add namespace to Experiment API implementation~~ [Backend][Multi-user] Add namespace to Experiment API implementation Mar 13, 2020

Address review comments and add additional unit tests

b8a8c86

k8s-ci-robot added size/XL and removed lgtm size/L labels Mar 14, 2020

chensun added 3 commits March 14, 2020 01:07

Update build file

0ff252f

fix unit tests

0b4b38d

Reject empty namespace in multi user mode

d06f4d6

k8s-ci-robot assigned IronPan Mar 16, 2020

IronPan reviewed Mar 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Mar 17, 2020

Address review comments

0dcd2a1

k8s-ci-robot removed the lgtm label Mar 17, 2020

Bobgy reviewed Mar 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Mar 17, 2020

k8s-ci-robot added the approved label Mar 17, 2020

k8s-ci-robot merged commit 809f97a into kubeflow:master Mar 17, 2020

chensun mentioned this pull request Mar 23, 2020

[Backend][Multi-user] Add authorization check on ListExperiment #3341

Merged

chensun deleted the experiment-api-impl branch April 3, 2020 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backend][Multi-user] Add namespace to Experiment API implementation #3243

[Backend][Multi-user] Add namespace to Experiment API implementation #3243

chensun commented Mar 9, 2020 •

edited

Loading

chensun commented Mar 12, 2020

Bobgy commented Mar 13, 2020

Bobgy commented Mar 13, 2020 •

edited

Loading

Bobgy left a comment

Bobgy Mar 13, 2020

chensun Mar 13, 2020

Bobgy Mar 17, 2020

chensun Mar 17, 2020

Bobgy left a comment

chensun commented Mar 14, 2020

chensun commented Mar 16, 2020

IronPan Mar 16, 2020

chensun Mar 17, 2020

IronPan Mar 16, 2020

chensun Mar 17, 2020

IronPan commented Mar 17, 2020

Bobgy left a comment

Bobgy Mar 17, 2020

chensun Mar 17, 2020

Bobgy Mar 17, 2020

chensun commented Mar 17, 2020

chensun commented Mar 17, 2020

IronPan commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

[Backend][Multi-user] Add namespace to Experiment API implementation #3243

[Backend][Multi-user] Add namespace to Experiment API implementation #3243

Conversation

chensun commented Mar 9, 2020 • edited Loading

chensun commented Mar 12, 2020

Bobgy commented Mar 13, 2020

Bobgy commented Mar 13, 2020 • edited Loading

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy left a comment

Choose a reason for hiding this comment

chensun commented Mar 14, 2020

chensun commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IronPan commented Mar 17, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun commented Mar 17, 2020

chensun commented Mar 17, 2020

IronPan commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

chensun commented Mar 9, 2020 •

edited

Loading

Bobgy commented Mar 13, 2020 •

edited

Loading