[PF-1681] Add discoverer policy when creating SAM workspace resource #755

melissachang · 2022-08-08T23:47:02Z

This PR:

Changes WSM to understand SAM discoverer role
When a workspace is created, adds empty discoverer policy. (Since policies can only be added when a SAM resource is created.)

Future PRs will modify ListWorkspaces/GetWorkspace to add a request permission for returning discoverer workspaces. For now, ListWorkspaces does not return a workspace if caller is only discoverer.

Manually tested:

Created workspace. Can see empty discoverer policy with getRoles.
Called grantRole to add second user as discoverer
Second user calls GetWorkspace. GetWorkspace returns 403 as expected
At first, when second user called ListWorkspaces, discoverer workspace was returned, which was wrong. I made the change to SamService.java. Now when second user calls ListWorkspace, discoverer workspace is not returned.

WorkspaceApiControllerTest.java uses mock bio.terra.workspace.service.iam.SamService. I wanted my tests to use real bio.terra.workspace.service.iam.SamService, so I created WorkspaceApiControllerConnectedTest.java.

zloery · 2022-08-09T15:21:15Z

service/src/main/java/bio/terra/workspace/service/workspace/flight/GcpCloudSyncStep.java

@@ -71,7 +72,11 @@ public StepResult doStep(FlightContext flightContext)
      newBindings.addAll(currentPolicy.getBindings());
      // Add appropriate project-level roles for each WSM IAM role.
      workspaceRoleGroupsMap.forEach(
-          (role, email) -> newBindings.add(bindingForRole(role, email, gcpProjectId)));
+          (wsmRole, email) -> {
+            if (CUSTOM_GCP_PROJECT_IAM_ROLES.containsKey(wsmRole)) {


nit: now that CUSTOM_GCP_PROJECT_IAM_ROLES is imported, also update line 110?

zloery · 2022-08-09T16:20:02Z

...rra/workspace/app/configuration/external/controller/WorkspaceApiControllerConnectedTest.java

+
+  @Test
+  public void getWorkspace_doesNotReturnWorkspaceWithOnlyDiscovererRole() throws Exception {
+    ApiCreatedWorkspace workspace = createWorkspace();


since these are talking to real Sam, these tests will leave workspaces. could you delete them from these tests as well?

Done, thanks. I always forget when to have tests delete workspaces, so I added a section to README, PTAL.

ddietterich

The documentation is incorrect and we don't need to sync the discoverer role

ddietterich · 2022-08-10T14:31:28Z

README.md

@@ -316,3 +316,14 @@ The integration tests live in the `integration` project. Consult the integration

 In the early days of the project, there were JUnit-based integration tests. We are in
 process of migrating them to Test Runner.
+
+#### Cleaning up workspaces in tests


I believe this is incorrect. Please double check my understanding @zloery

Janitor just deletes projects, it does not do anything to clean up Sam, so we should never skip in-test cleanup.
Connected tests also use RBS and Janitor. It doesn't have anything to do with WSM being ephemeral, since Janitor is just deleting projects.

We can use Janitor in two ways:

the tools RBS instance registers every project it hands out with Janitor to be cleaned up after some time. Since all our connected tests talk to this instance, the GCP projects get cleaned up eventually

After PF-886, you also can register a workspace with Janitor. After some time, Janitor will call WSM to delete the workspaces, which will clean up all of the WSM DB + Sam workspace + GCP project. The CLI tests use this when they run against real WSM environments, like dev.

We can't use the second pattern for WSM connected tests because the instance is ephemeral (so there's nothing for Janitor to call later), but we can and do use the first pattern to make sure the GCP projects get cleaned up. the Sam workspaces will still be around if the test doesn't clean them up, though

Updated:

https://github.com/DataBiosphere/terra-workspace-manager/blob/17d64b6d6cf4b891977230672b6f4cc9cd592105/README.md#cleaning-up-workspaces-in-tests

This has @zloery 's stamp of approval.

ddietterich · 2022-08-10T14:36:00Z

service/src/main/java/bio/terra/workspace/service/iam/SamService.java

          if (highestRole.isPresent()) {
+            // TODO(PF-1875): Support requesting discoverer workspaces in ListWorkspaces.


Why not just filter in ListWorkspaces, so you don't have to remember to come back to this code?
You need the filtering there anyway.

In the next PR, I updated both here and ListWorkspaces.

ddietterich · 2022-08-10T14:53:14Z

service/src/main/java/bio/terra/workspace/service/workspace/flight/SyncSamGroupsStep.java

@@ -47,6 +47,9 @@ public StepResult doStep(FlightContext flightContext)
    workspaceRoleGroupMap.put(
        WsmIamRole.READER,
        samService.syncWorkspacePolicy(workspaceUuid, WsmIamRole.READER, userRequest));
+    workspaceRoleGroupMap.put(


I don't think we need to sync the discoverer role. That will create Google groups for discoverers. We only need that when we are going to apply that group to a cloud resource ACL; like granting a READER actual read access to a GCS bucket.

I think we only ever enforce discoverer by checking with Sam - never the cloud.

Done, good point.

zloery · 2022-08-10T17:31:05Z

README.md

+
+#### Cleaning up workspaces in tests
+
+For integration tests, don't need to delete workspaces because it's taken care of


TestRunner isn't integrated with Janitor so it isn't doing this cleanup (though it could if we were testing against live environments and we built that integration). The cleanup is currently part of the tests (though since it's included in the WorkspaceAllocateTestScriptBase base class, developers don't need to think about it while writing tests)

melissachang added 5 commits August 8, 2022 10:37

[PF-1681] Add discoverer policy when creating SAM workspace

12d1ea7

Tests, don't return discoverer workspace for ListWorkspaces

571d23f

fix CreateCustomGcpRolesStep

e6a3a1b

fix more tests

8268fa9

go back to original CUSTOM_GCP_PROJECT_IAM_ROLES

8de451d

melissachang requested review from zloery and ddietterich August 8, 2022 23:47

zloery approved these changes Aug 9, 2022

View reviewed changes

melissachang added 2 commits August 9, 2022 11:20

review fixes

f64dab8

minor edit

faac7d0

ddietterich suggested changes Aug 10, 2022

View reviewed changes

don't sync SAM group

99f9a65

zloery reviewed Aug 10, 2022

View reviewed changes

melissachang added 2 commits August 10, 2022 11:33

update "Cleaning up workspaces in tests"

285fc2b

add note about WorkspaceAllocateTestScriptBase

17d64b6

ddietterich approved these changes Aug 10, 2022

View reviewed changes

melissachang merged commit 3cd7fc0 into main Aug 10, 2022

melissachang deleted the mc/discoverer branch August 10, 2022 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PF-1681] Add discoverer policy when creating SAM workspace resource #755

[PF-1681] Add discoverer policy when creating SAM workspace resource #755

melissachang commented Aug 8, 2022

zloery Aug 9, 2022

melissachang Aug 9, 2022

zloery Aug 9, 2022

melissachang Aug 9, 2022

ddietterich left a comment

ddietterich Aug 10, 2022

zloery Aug 10, 2022

melissachang Aug 10, 2022

ddietterich Aug 10, 2022

melissachang Aug 10, 2022

ddietterich Aug 10, 2022

melissachang Aug 10, 2022

zloery Aug 10, 2022

		if (highestRole.isPresent()) {
		// TODO(PF-1875): Support requesting discoverer workspaces in ListWorkspaces.


		#### Cleaning up workspaces in tests

		For integration tests, don't need to delete workspaces because it's taken care of

[PF-1681] Add discoverer policy when creating SAM workspace resource #755

[PF-1681] Add discoverer policy when creating SAM workspace resource #755

Conversation

melissachang commented Aug 8, 2022

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

ddietterich 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

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