[IA-4295] [IA-4327] Add Sam client to Leonardo, populate workspace ID #4757

rtitle · 2024-08-20T14:28:22Z

https://broadworkbench.atlassian.net/browse/IA-4327
https://broadworkbench.atlassian.net/browse/IA-4295

Summary of changes

This PR does a few things:

Populates workspaceId going forward for v1 runtimes, disks, and apps
Adds the sam-client dependency to Leonardo
Adds SamApiClientProvider and SamService classes for interacting with Sam using the generated client. Deprecated [Http]SamDAO and [Leo|Sam]AuthProvider.
Ports functionality from PetClusterServiceAccountProvider over to SamService as a POC (read: logic for retrieving pets, proxy groups).

Note: access control logic is not touched as part of this PR -- that will be a separate pull request.

Testing these changes

What to test

Unit tests written for new functionality
Automation tests
BEE manual testing: create GCP and Azure runtimes/disks/apps of each type, verify workspaceId is always populated
Test in a BEE: create GCP and Azure runtimes/disks/apps of each type, verify workspaceId is always populated
Test in RWB: verify runtimes, disks, and apps have workspaceId populated
Verify logs and metrics for the above

Who tested and where

This change is covered by automated tests
- NB: Rerun automation tests on this PR by commenting jenkins retest or jenkins multi-test.
I validated this change
Primary reviewer validated this change
I validated this change in the dev environment

rtitle · 2024-08-20T15:24:20Z

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/diskRoutesModels.scala

@@ -42,7 +42,8 @@ final case class ListPersistentDiskResponse(id: DiskId,
                                            size: DiskSize,
                                            diskType: DiskType,
                                            blockSize: BlockSize,
-                                            labels: LabelMap
+                                            labels: LabelMap,
+                                            workspaceId: Option[WorkspaceId]


Added optional workspaceId to the v1 getDisk and listDisks API responses.

(It was already there for runtimes and apps)

rtitle · 2024-08-20T15:25:22Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/auth/SamAuthProvider.scala

+   * @param ev trace id
+   * @return optional workspace ID
+   */
+  override def lookupWorkspaceParentForGoogleProject(userInfo: UserInfo, googleProject: GoogleProject)(implicit


Here is where it looks up the workspaceId from Sam. This code is shared for runtimes, disks, and apps.

Gotcha, so for GCP, Rawls creates the workspace and then calls Sam to create the corresponding resource? So we are more or less guaranteed that the Sam resource has been created by the time Leo makes this call?

Yup that's correct, there should always be a workspace Sam resource for any Terra workspace in use today. Even so I tried to make this code handle errors and emit logs/metrics, just in case something goes wrong -- we don't want this code to cause runtime creation failures.

rtitle · 2024-08-20T15:26:32Z

...c/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala

              )
            )
            _ <- authProvider
              .notifyResourceCreated(samResource, userInfo.userEmail, googleProject)
              .handleErrorWith { t =>
                log.error(t)(
                  s"[${ctx.traceId}] Failed to notify the AuthProvider for creation of persistent disk ${disk.projectNameString}"
-                ) >> F.raiseError(t)
+                ) >> F.raiseError[Unit](t)


I don't know if it's just me, but my Intellij flags this as a compile error -- adding the Unit type param fixed
it. (Made similar changes in a couple other places.)

Oh interesting, my IntelliJ never yelled at me for that, but it can't hurt, thanks for fixing :)

rtitle · 2024-08-20T15:39:11Z

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

-                        lastUsedApp,
-                        petSA,
-                        nodepool.id,
-                        req.workspaceId,


Note v1 apps actually have workspaceId as an optional field in the request json. However not all clients are sending it (seems like AoU "allowed" apps are populating it, but not Galaxy). Also Leo does not validate this field, so theoretically a caller could provide any workspace ID at app creation time. For those reasons I'm switching it to the workspace ID retrieved from Sam from the google project (which is validated).

codecov · 2024-08-20T15:46:32Z

Codecov Report

Attention: Patch coverage is 82.48175% with 24 lines in your changes missing coverage. Please review.

Project coverage is 74.22%. Comparing base (54c8a19) to head (f14f67a).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
...kbench/leonardo/dao/sam/SamApiClientProvider.scala	0.00%	9 Missing ⚠️
...ch/leonardo/http/BaselineDependenciesBuilder.scala	0.00%	4 Missing ⚠️
...te/dsde/workbench/leonardo/util/BucketHelper.scala	55.55%	4 Missing ⚠️
.../workbench/leonardo/dao/sam/SamServiceInterp.scala	96.00%	2 Missing ⚠️
...h/leonardo/http/service/RuntimeServiceInterp.scala	86.66%	2 Missing ⚠️
...rkbench/leonardo/auth/CloudAuthTokenProvider.scala	0.00%	1 Missing ⚠️
...rkbench/leonardo/http/GcpDependenciesBuilder.scala	85.71%	1 Missing ⚠️
...ench/leonardo/http/service/DiskServiceInterp.scala	83.33%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4757      +/-   ##
===========================================
+ Coverage    74.17%   74.22%   +0.05%     
===========================================
  Files          161      164       +3     
  Lines        14941    14981      +40     
  Branches      1226     1243      +17     
===========================================
+ Hits         11082    11120      +38     
- Misses        3859     3861       +2

Files with missing lines	Coverage Δ
...sde/workbench/leonardo/http/diskRoutesModels.scala	`0.00% <ø> (ø)`
...ench/leonardo/auth/GcpCloudAuthTokenProvider.scala	`0.00% <ø> (ø)`
...dsde/workbench/leonardo/auth/SamAuthProvider.scala	`58.77% <ø> (+0.44%)`	⬆️
...titute/dsde/workbench/leonardo/config/Config.scala	`97.76% <ø> (-0.03%)`	⬇️
...itute/dsde/workbench/leonardo/dao/HttpSamDAO.scala	`18.78% <ø> (ø)`
...institute/dsde/workbench/leonardo/dao/SamDAO.scala	`0.00% <ø> (ø)`
...dsde/workbench/leonardo/dao/sam/SamException.scala	`100.00% <100.00%> (ø)`
...e/dsde/workbench/leonardo/dao/sam/SamService.scala	`100.00% <100.00%> (ø)`
...e/workbench/leonardo/db/DiskServiceDbQueries.scala	`100.00% <ø> (ø)`
...bench/leonardo/http/AzureDependenciesBuilder.scala	`97.36% <100.00%> (ø)`
... and 12 more

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54c8a19...f14f67a. Read the comment docs.

LizBaldo

LGTM, my main comment is around switching to the Sam client instead of the DAO if possible.

LizBaldo · 2024-08-21T14:25:46Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/auth/SamAuthProvider.scala

+   * @param ev trace id
+   * @return optional workspace ID
+   */
+  override def lookupWorkspaceParentForGoogleProject(userInfo: UserInfo, googleProject: GoogleProject)(implicit


Gotcha, so for GCP, Rawls creates the workspace and then calls Sam to create the corresponding resource? So we are more or less guaranteed that the Sam resource has been created by the time Leo makes this call?

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/auth/SamAuthProvider.scala

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/HttpSamDAO.scala

LizBaldo · 2024-08-21T14:35:59Z

...c/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala

              )
            )
            _ <- authProvider
              .notifyResourceCreated(samResource, userInfo.userEmail, googleProject)
              .handleErrorWith { t =>
                log.error(t)(
                  s"[${ctx.traceId}] Failed to notify the AuthProvider for creation of persistent disk ${disk.projectNameString}"
-                ) >> F.raiseError(t)
+                ) >> F.raiseError[Unit](t)


Oh interesting, my IntelliJ never yelled at me for that, but it can't hurt, thanks for fixing :)

rtitle · 2024-08-23T19:17:46Z

Updated the description -- added the generated Sam client to Leo and started using it for certain things (though not access control decisions yet). Unit tests are all passing, will do more manual testing as a next step.

...src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamApiClientProvider.scala

project/Dependencies.scala

...src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamApiClientProvider.scala

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamException.scala

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamServiceInterp.scala

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

rtitle · 2024-08-27T19:48:53Z

@LizBaldo no rush but I applied PR feedback, think it's ready for another look. Thanks!

LizBaldo

Looks great, so much cleaner. Thanks for addressing all of my comments!

LizBaldo · 2024-08-27T19:56:48Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamException.scala

+   */
+  private def extractMessage(messagePrefix: String, apiException: ApiException): String = {
+    // The generated ApiException class unfortunately formats getMessage(), and includes
+    // the entire response body. We want to extract the actual message from that.


Nice catch 😨

…one yet

rtitle · 2024-08-28T17:24:24Z

Did some more smoke testing on a BEE (different runtime/app operations, cloud environments screen, jupyter/rstudio/galaxy). It looks good. Will merge this and keep an eye on things

rtitle changed the title ~~Populate parent workspace ID for v1 runtimes, disks, and apps~~ Populate workspace ID for v1 runtimes, disks, and apps Aug 20, 2024

rtitle changed the title ~~Populate workspace ID for v1 runtimes, disks, and apps~~ [IA-4327] Populate workspace ID for v1 runtimes, disks, and apps Aug 20, 2024

rtitle force-pushed the rt-ia-4327 branch from efaa83e to ffd65da Compare August 20, 2024 14:35

rtitle commented Aug 20, 2024

View reviewed changes

rtitle requested a review from LizBaldo August 20, 2024 15:51

LizBaldo reviewed Aug 21, 2024

View reviewed changes

jdcanas self-requested a review August 22, 2024 15:41

rtitle marked this pull request as ready for review August 23, 2024 19:10

rtitle requested a review from mlilynolting August 23, 2024 19:17

rtitle force-pushed the rt-ia-4327 branch from c12bc40 to 36a001a Compare August 27, 2024 14:58

rtitle changed the title ~~[IA-4327] Populate workspace ID for v1 runtimes, disks, and apps~~ [IA-4327] Add Sam client to Leonardo, populate workspace ID Aug 27, 2024

jdcanas reviewed Aug 27, 2024

View reviewed changes

...src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamApiClientProvider.scala Show resolved Hide resolved

jdcanas approved these changes Aug 27, 2024

View reviewed changes

rtitle force-pushed the rt-ia-4327 branch from e4a2518 to 74fdad8 Compare August 27, 2024 16:08

rtitle commented Aug 27, 2024

View reviewed changes

project/Dependencies.scala Outdated Show resolved Hide resolved

LizBaldo reviewed Aug 27, 2024

View reviewed changes

rtitle changed the title ~~[IA-4327] Add Sam client to Leonardo, populate workspace ID~~ [IA-4295] [IA-4327] Add Sam client to Leonardo, populate workspace ID Aug 27, 2024

LizBaldo approved these changes Aug 27, 2024

View reviewed changes

rtitle added 6 commits August 28, 2024 10:59

Populate parent workspace ID for v1 runtimes, disks, and apps

0e4da85

Metrics fix

2c3ce90

Fix comment

3a3818e

Add workspaceId to the v1 disk response. Fix some unit tests

4a3b45e

First pass, add Sam client to Leonardo

b1718c4

Remove ServiceAccountProvider, build out SamService. Unit tests not d…

9f52db3

…one yet

rtitle added 12 commits August 28, 2024 10:59

Tests compile, probably don't pass yet

ccaedfd

Fix unit tests

3c25a46

All unit tests fixed

3309870

Support getPetServiceAccountToken

20f0f33

Bump sam client

9af2aa4

Bugfix

ffab85b

Bump sam client again

47b4371

Update sam client one more time

8696716

PR feedback

261edce

Fix logic in SamExceptionFactory to extract message

dfb6a69

Remove todo

3b4f24d

Update sam version to non-SNAP

f14f67a

rtitle force-pushed the rt-ia-4327 branch from 3d50718 to f14f67a Compare August 28, 2024 14:59

rtitle merged commit 0f2a5a0 into develop Aug 28, 2024
23 of 24 checks passed

rtitle deleted the rt-ia-4327 branch August 28, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IA-4295] [IA-4327] Add Sam client to Leonardo, populate workspace ID #4757

[IA-4295] [IA-4327] Add Sam client to Leonardo, populate workspace ID #4757

rtitle commented Aug 20, 2024 •

edited

Loading

rtitle Aug 20, 2024

rtitle Aug 20, 2024 •

edited

Loading

LizBaldo Aug 21, 2024

rtitle Aug 21, 2024

rtitle Aug 20, 2024 •

edited

Loading

LizBaldo Aug 21, 2024

rtitle Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

LizBaldo left a comment

LizBaldo Aug 21, 2024

LizBaldo Aug 21, 2024

rtitle commented Aug 23, 2024

rtitle commented Aug 27, 2024 •

edited

Loading

LizBaldo left a comment

LizBaldo Aug 27, 2024

rtitle commented Aug 28, 2024

[IA-4295] [IA-4327] Add Sam client to Leonardo, populate workspace ID #4757

[IA-4295] [IA-4327] Add Sam client to Leonardo, populate workspace ID #4757

Conversation

rtitle commented Aug 20, 2024 • edited Loading

Summary of changes

Testing these changes

What to test

Who tested and where

rtitle Aug 20, 2024

Choose a reason for hiding this comment

rtitle Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

LizBaldo Aug 21, 2024

Choose a reason for hiding this comment

rtitle Aug 21, 2024

Choose a reason for hiding this comment

rtitle Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

LizBaldo Aug 21, 2024

Choose a reason for hiding this comment

rtitle Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

LizBaldo left a comment

Choose a reason for hiding this comment

LizBaldo Aug 21, 2024

Choose a reason for hiding this comment

LizBaldo Aug 21, 2024

Choose a reason for hiding this comment

rtitle commented Aug 23, 2024

rtitle commented Aug 27, 2024 • edited Loading

LizBaldo left a comment

Choose a reason for hiding this comment

LizBaldo Aug 27, 2024

Choose a reason for hiding this comment

rtitle commented Aug 28, 2024

rtitle commented Aug 20, 2024 •

edited

Loading

rtitle Aug 20, 2024 •

edited

Loading

rtitle Aug 20, 2024 •

edited

Loading

rtitle Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

rtitle commented Aug 27, 2024 •

edited

Loading