[WOR-1456] Add requester pays setting #3032

trholdridge · 2024-09-16T19:34:23Z

Ticket: https://broadworkbench.atlassian.net/browse/WOR-1456

Adds requester pays as a workspace setting.

Workspace settings previously didn't play nice with requester pays workspaces, so this PR also fixes that behavior. Workspace settings requests now always pass a user project (the workspace's google project), which works when requester pays is off as well as on.

PR checklist

Include the JIRA issue number in the PR description and title
Make sure Swagger is updated if API changes
- ...and Orchestration's Swagger too!
If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
Get two thumbsworth of PR review
Verify all tests go green, including CI tests
Squash commits and merge to develop (branches are automatically deleted after merging)
Inform other teams of any substantial changes via Slack and/or email

trholdridge · 2024-09-17T14:36:59Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/HttpGoogleServicesDAO.scala

+      .setBucketLifecycle(
+        GcsBucketName(bucketName),
+        lifecycle,
+        bucketTargetOptions = List(BucketTargetOption.userProject(userProject.value))


This passes the project so that, when requester pays is on, the workspace settings requests are accepted. I added the same line to all three settings in the DAO. I could break it out into a separate function but I thought it was straightforward enough to use multiple times. Alternate opinions are welcome.

trholdridge · 2024-09-17T14:39:16Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceSettingService.scala

@@ -94,6 +96,7 @@ class WorkspaceSettingService(protected val ctx: RawlsRequestContext,
                )
              case _ => None
            }
+          case GcpBucketRequesterPaysSetting(GcpBucketRequesterPaysConfig(_)) => None


Requester pays configs are valid if all the types are correct.

trholdridge · 2024-09-17T14:44:58Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceSettingService.scala

+
+        case GcpBucketRequesterPaysSetting(GcpBucketRequesterPaysConfig(enabled)) =>
+          for {
+            _ <- gcsDAO.setRequesterPays(workspace.bucketName, enabled, workspace.googleProjectId)
            _ <- workspaceSettingRepository.markWorkspaceSettingApplied(workspace.workspaceIdAsUUID,
                                                                        setting.settingType


On the note of code duplication though, some of this is getting more clunky now that there are three settings. Again, feedback welcome if there are spots which could use some abstraction.

One option would be to extract the case statement here into a separate WorkspaceSetting => Future[Unit] function which would perform the cloud call to apply the setting. Then you'd call that function from a single for-comp which would first apply the setting in the cloud and then mark the setting applied in the db. Something like this:

def doTheThing(workspaceSetting: WorkspaceSetting): Future[Unit] = workspaceSetting match { // call the appropriate gcsDAO method } (for { _ <- doTheThing(setting) _ <- workspaceSettingRepository.markWorkspaceSettingApplied(workspace.workspaceIdAsUUID, setting.settingType ) } yield None).recoverWith { case e => logger.error( s"Failed to apply settings. [workspaceId=${workspace.workspaceIdAsUUID},settingType=${setting.settingType}]", e ) workspaceSettingRepository .removePendingSetting(workspace.workspaceIdAsUUID, setting.settingType) .map(_ => Some((setting.settingType, ErrorReport(StatusCodes.InternalServerError, e.getMessage)))) }

cahrens · 2024-09-17T16:36:05Z

...rc/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceSettingServiceUnitTests.scala

@@ -206,7 +205,8 @@ class WorkspaceSettingServiceUnitTests extends AnyFlatSpec with MockitoTestUtils
    ).thenReturn(Future.successful(true))

    val gcsDAO = mock[GoogleServicesDAO]
-    when(gcsDAO.setBucketLifecycle(workspace.bucketName, List())).thenReturn(Future.successful())
+    when(gcsDAO.setBucketLifecycle(ArgumentMatchers.eq(workspace.bucketName), ArgumentMatchers.eq(List()), any()))


You could check here that the right google project ID is being sent.

cahrens · 2024-09-17T16:36:15Z

...rc/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceSettingServiceUnitTests.scala

+    when(
+      gcsDAO.setBucketLifecycle(ArgumentMatchers.eq(workspace.bucketName),
+                                ArgumentMatchers.eq(List(newSettingGoogleRule)),
+                                any()


Same note here.

cahrens · 2024-09-17T16:36:38Z

...rc/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceSettingServiceUnitTests.scala

+    when(
+      gcsDAO.setBucketLifecycle(ArgumentMatchers.eq(workspace.bucketName),
+                                ArgumentMatchers.eq(List(newSettingGoogleRule)),
+                                any()


cahrens · 2024-09-17T16:39:14Z

...rc/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceSettingServiceUnitTests.scala

+                                ArgumentMatchers.eq(List(newSettingGoogleRule)),
+                                any()
+      )
+    ).thenReturn(Future.failed(new Exception("failed to apply settings")))

    val service =


There is no unit test coverage of calling the gcsDAO for soft delete or requester pays settings?

cahrens · 2024-09-24T12:11:37Z

core/src/main/resources/swagger/api-docs.yaml

@@ -5980,6 +5982,14 @@ components:
        retentionDurationInSeconds:
          type: integer
          description: The length of time to retain soft-deleted objects for, in seconds
+    WorkspaceSettingGcpBucketRequesterPaysConfig:


Remember that you will need to update orchestration swagger also.

trholdridge commented Sep 17, 2024

View reviewed changes

trholdridge marked this pull request as ready for review September 17, 2024 14:55

trholdridge requested review from a team, cahrens and marctalbott and removed request for a team September 17, 2024 14:55

cahrens reviewed Sep 17, 2024

View reviewed changes

marctalbott approved these changes Sep 20, 2024

View reviewed changes

trholdridge added 4 commits September 20, 2024 13:51

Add requester pays setting

05f2662

Imports

35a77a6

Revise tests and refactor applySetting

526d32a

fix test name

c2d0e99

trholdridge force-pushed the WOR-1456 branch from 6c9997f to c2d0e99 Compare September 20, 2024 17:51

trholdridge requested review from blakery and removed request for blakery September 23, 2024 17:12

cahrens reviewed Sep 24, 2024

View reviewed changes

cahrens approved these changes Sep 24, 2024

View reviewed changes

trholdridge merged commit 5a80e2c into develop Sep 24, 2024
29 checks passed

trholdridge deleted the WOR-1456 branch September 24, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WOR-1456] Add requester pays setting #3032

[WOR-1456] Add requester pays setting #3032

trholdridge commented Sep 16, 2024 •

edited

Loading

trholdridge Sep 17, 2024 •

edited

Loading

trholdridge Sep 17, 2024

trholdridge Sep 17, 2024

marctalbott Sep 18, 2024

cahrens Sep 17, 2024

cahrens Sep 17, 2024

cahrens Sep 17, 2024

cahrens Sep 17, 2024

cahrens Sep 24, 2024

[WOR-1456] Add requester pays setting #3032

[WOR-1456] Add requester pays setting #3032

Conversation

trholdridge commented Sep 16, 2024 • edited Loading

trholdridge Sep 17, 2024 • edited Loading

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

trholdridge commented Sep 16, 2024 •

edited

Loading

trholdridge Sep 17, 2024 •

edited

Loading