[IA-5053] Add retries to SamService, flesh out more authz methods #4779

rtitle · 2024-09-05T14:27:23Z

https://broadworkbench.atlassian.net/browse/IA-5053

Summary of changes

Draft while I self-review and test. This PR fleshes out more authz logic in SamService/SamServiceInterp. It does NOT actually update Leo to use SamService for authorization yet (I think we will need to do some more testing and potential historical data migrations before that can happen).

What

Added pattern for retries in SamServiceInterp
Added access control methods (checkAuthz, create/deleteResource, etc)
Improved logging
Added unit tests

Why

I'm trying to prepare for when we can cut over to SamService for real. Next step will be understanding/migrating historical data to the desired state. Docs 1 and 2 will help with this.

Testing these changes

What to test

Unit tests
Automation tests

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

…nts (not used yet)

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

rtitle · 2024-09-05T14:37:59Z

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

@@ -13,45 +19,45 @@ trait SamService[F[_]] {

  /**
   * Gets a user's pet GCP service account, using the user's token.
-   * @param userInfo the user info containing an access token
+   * @param bearerToken the user's access token


I decided to have these methods take a token string instead of a UserInfo object. It was too tempting to use the userInfo.email field for logging/etc, but that field should not be relied upon. We should instead call SamService.getUserEmail when we need to resolve the email from a token.

rtitle · 2024-09-05T14:39:19Z

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

      }
+      _ <- logger.info(ctx.loggingCtx)(


Added some log statements to these methods.

Note: this class looks a lot like WSM SamService, but in Scala. :)

rtitle · 2024-09-05T14:40:56Z

...src/test/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamServiceInterpSpec.scala

 import org.scalatest.BeforeAndAfterAll
 import org.scalatest.funspec.AnyFunSpecLike
 import org.scalatestplus.mockito.MockitoSugar

 import java.util.UUID
+import scala.jdk.CollectionConverters._

 class SamServiceInterpSpec extends AnyFunSpecLike with LeonardoTestSuite with BeforeAndAfterAll with MockitoSugar {


Lot of tests here but I think it has pretty complete coverage of SamServiceInterp. It mocks everything so runs very fast. I tried to organize test cases a bit with ScalaTest FunSpec.

rtitle · 2024-09-05T14:54:53Z

@LizBaldo this might be relevant to your auth domain work. I'm trying to get SamService in Leo "ready for prime time". This PR doesn't actually start using it yet for authz, but improves some of the code.

rtitle · 2024-09-05T15:04:45Z

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

+  private val defaultSamRetryConfig =
+    RetryConfig(addJitter(1 seconds, 1 seconds), _ * 2, 5, isRetryable)
+
+  /**


Borrowed this logic from TCL SamRetry: https://github.com/DataBiosphere/terra-common-lib/blob/develop/src/main/java/bio/terra/common/sam/SamRetry.java#L58

mlilynolting · 2024-09-05T15:28:40Z

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

+          )
+      }
+
+      // All Leo resources have a creator role


Is this true for apps with the workspace-shared access policy? Leo's config has an "owner" role there. I'm assuming also that when you say "leo resource" you're referring only to v1 stuff? I'm not sure how WSM handles these roles.

Also, are there cases where the Leonardo SA is creating resources?

So I was trying to assume "desired state" here, where we migrate away from WSM-owned resources, and use dedicated resource types for Leo. That was a recommendation in this doc.

Hm you're right that kubernetes-app-shared has owner and user roles. And kubernetes-app has creator and manager.

Maybe I'll revisit the method signature here… I was trying to see if createResource could be generic to all types of resources Leo supports, or if we need a createRuntimeResource, createDiskResource, createKubernetesAppResource, etc.

Also, are there cases where the Leonardo SA is creating resources?

I don't think so -- all Sam resources should be created by the user. It's also a recommendation to not create Sam resources async. So I was intentionally giving the createResource method a userToken parameter.

That makes sense. And I love the idea of a standardized Leo-managed resource with predictable roles. Maybe creating a resource gives you the Creator role but depending on the resource, the context, and the user, they could get other roles as well? This might just require a bit more design to nail down an approach.

We could start with this method and add other methods for special cases if we need to.

@mlilynolting what do you think of this change? Instead of assuming a creator role, I updated SamService.createResource to just take the policies directly. We can maybe add specialized methods (getRuntimePolicies, getAppPolicies, etc) later on.

I think that's a solid change that reflects the Sam contract really faithfully - Leo calls Sam with a new policy it wants to create for this resource. The policy name doesn't need to be unique, right?

Right, the policy name can be any string -- I think it just needs to be unique per resource (e.g. you can't have 2 foo policies on a resource). But the Sam request should fail if uniqueness is violated.

http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/mocks.scala

mlilynolting · 2024-09-05T18:10:07Z

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

@@ -211,7 +217,7 @@ class SamServiceInterp[F[_]](apiClientProvider: SamApiClientProvider[F],
                              samResourceId: SamResourceId,
                              projectParent: Option[GoogleProject],
                              workspaceParent: Option[WorkspaceId],
-                              creator: Option[WorkbenchEmail]
+                              policies: Map[String, SamPolicyData]


Does it make sense to type the key here as SamPolicyName?

I went with String because I don't love how SamPolicyName is an enum in Leo. It's not really an enumeration that maps to anything in Sam, this is just an arbitrary name for the policy. (Often role names are reused for policy names, see WSM for example).

SamPolicyName used all over the place in the old auth code (and conflated with roles somewhat) so we can't remove/change it yet.

That's a great reason not to do it, lol. Could you add a comment suggesting that we might want to move to a dedicated policyName type?

codecov · 2024-09-05T18:50:53Z

Codecov Report

Attention: Patch coverage is 97.97980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.36%. Comparing base (0f69d85) to head (8c7690e).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
...kbench/leonardo/dao/sam/SamApiClientProvider.scala	0.00%	1 Missing ⚠️
...ute/dsde/workbench/leonardo/dao/sam/SamRetry.scala	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4779      +/-   ##
===========================================
+ Coverage    74.20%   74.36%   +0.16%     
===========================================
  Files          164      165       +1     
  Lines        14981    15060      +79     
  Branches      1243     1197      -46     
===========================================
+ Hits         11117    11200      +83     
+ Misses        3864     3860       -4

Files with missing lines	Coverage Δ
...dsde/workbench/leonardo/dao/sam/SamException.scala	`100.00% <100.00%> (ø)`
...e/dsde/workbench/leonardo/dao/sam/SamService.scala	`100.00% <100.00%> (ø)`
.../workbench/leonardo/dao/sam/SamServiceInterp.scala	`98.29% <100.00%> (+2.29%)`	⬆️
...ench/leonardo/http/service/DiskServiceInterp.scala	`90.90% <100.00%> (ø)`
...ch/leonardo/http/service/LeoAppServiceInterp.scala	`86.96% <100.00%> (ø)`
...h/leonardo/http/service/RuntimeServiceInterp.scala	`87.90% <100.00%> (ø)`
...kbench/leonardo/dao/sam/SamApiClientProvider.scala	`0.00% <0.00%> (ø)`
...ute/dsde/workbench/leonardo/dao/sam/SamRetry.scala	`90.00% <90.00%> (ø)`

... and 1 file 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 0f69d85...8c7690e. Read the comment docs.

mlilynolting · 2024-09-05T19:10:46Z

This all looks great to me. Are you planning anything else before you un-draft the PR?

rtitle · 2024-09-05T19:47:30Z

Thanks! I'm just tweaking some of the retry behavior, but not planning anything else substantial. I'll plan to un-draft it soon.

rtitle · 2024-09-06T12:51:17Z

@mlilynolting @LizBaldo I tested this on a BEE and things look good. Moved out of draft, requesting reviews -- thanks.

mlilynolting · 2024-09-06T14:23:53Z

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

+      .adaptError { case e: ApiException =>
+        SamException.create("Error listing resources from Sam", e, ctx.traceId)
+      }
+  } yield resources.asScala.toList.map(_.getResourceId)


"has access to" might be a bit limiting. I don't know that there is a use case for listing just the resource IDs here - there might be a specific access right that we want to check, like read. I don't think we necessarily need to change that in this PR, since this method isn't actually used yet. Just a headsup that this might need to change for Leo's use case

Yeah that's a good point - we might want to additionally check specific actions here. I agree this can be changed in a subsequent PR.

So y ou are saying adding a scope to this function that could be a role or an action? E.g. list all resources the user has read access to? That might be tricky since different resources can have completely different actions no?

I think ideally the current use cases would be covered by adding a list of actions. We don't want to check roles if we can avoid it (we might have to if we have to use parent role as a standin for child action). This method is only called in the context of a single resource type so that will scope us to only the actions for that type.

+1 to @mlilynolting -- I'm imagining adding a new parameter actions: List[SamAction] to this method, which would further constrain the results.

LizBaldo · 2024-09-06T15:12:26Z

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

+      .adaptError { case e: ApiException =>
+        SamException.create("Error listing resources from Sam", e, ctx.traceId)
+      }
+  } yield resources.asScala.toList.map(_.getResourceId)


So y ou are saying adding a scope to this function that could be a role or an action? E.g. list all resources the user has read access to? That might be tricky since different resources can have completely different actions no?

rtitle added 4 commits September 4, 2024 10:14

IA-5053: add retries to SamServiceInterp, flesh out more authz endpoi…

4817dd3

…nts (not used yet)

more unit tests

c5e79b0

Add a SamRetrySpec

04a94a0

Add some comments

407c71a

rtitle commented Sep 5, 2024

View reviewed changes

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/sam/SamRetry.scala Outdated Show resolved Hide resolved

rtitle commented Sep 5, 2024

View reviewed changes

Use SamException

b20be0f

unused imports

ebca307

rtitle commented Sep 5, 2024

View reviewed changes

mlilynolting reviewed Sep 5, 2024

View reviewed changes

http/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/mocks.scala Outdated Show resolved Hide resolved

rtitle added 2 commits September 5, 2024 12:43

Fix unit test

816b915

Refactor createResource signature

a74f5bf

mlilynolting reviewed Sep 5, 2024

View reviewed changes

s/checkAuthz/checkAuthorized

54c1097

Don't use wb-libs tracedRetryF

1bc0d45

Add _some_ logging back to SamRetry

299d88b

rtitle added 2 commits September 5, 2024 15:47

Merge branch 'develop' into rt-ia-5053

4b71c91

Improve comment

8c7690e

rtitle marked this pull request as ready for review September 6, 2024 12:50

rtitle requested review from LizBaldo and mlilynolting September 6, 2024 12:50

mlilynolting approved these changes Sep 6, 2024

View reviewed changes

rtitle changed the title ~~[DRAFT] [IA-5053] Add retries to SamService, flesh out more authz methods~~ [IA-5053] Add retries to SamService, flesh out more authz methods Sep 6, 2024

mlilynolting reviewed Sep 6, 2024

View reviewed changes

LizBaldo approved these changes Sep 6, 2024

View reviewed changes

rtitle merged commit 87b81f7 into develop Sep 6, 2024
23 of 24 checks passed

rtitle deleted the rt-ia-5053 branch September 6, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IA-5053] Add retries to SamService, flesh out more authz methods #4779

[IA-5053] Add retries to SamService, flesh out more authz methods #4779

rtitle commented Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024

rtitle Sep 5, 2024 •

edited

Loading

rtitle commented Sep 5, 2024

rtitle Sep 5, 2024

mlilynolting Sep 5, 2024

mlilynolting Sep 5, 2024

rtitle Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024

mlilynolting Sep 5, 2024

mlilynolting Sep 5, 2024

rtitle Sep 5, 2024

mlilynolting Sep 5, 2024

rtitle Sep 5, 2024 •

edited

Loading

mlilynolting Sep 5, 2024

rtitle Sep 5, 2024 •

edited

Loading

mlilynolting Sep 5, 2024

rtitle Sep 5, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

mlilynolting commented Sep 5, 2024

rtitle commented Sep 5, 2024

rtitle commented Sep 6, 2024

mlilynolting Sep 6, 2024

rtitle Sep 6, 2024 •

edited

Loading

LizBaldo Sep 6, 2024

mlilynolting Sep 6, 2024

rtitle Sep 6, 2024 •

edited

Loading

LizBaldo Sep 6, 2024

[IA-5053] Add retries to SamService, flesh out more authz methods #4779

[IA-5053] Add retries to SamService, flesh out more authz methods #4779

Conversation

rtitle commented Sep 5, 2024 • edited Loading

Summary of changes

What

Why

Testing these changes

What to test

Who tested and where

rtitle Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

rtitle commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Sep 5, 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

rtitle Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

mlilynolting commented Sep 5, 2024

rtitle commented Sep 5, 2024

rtitle commented Sep 6, 2024

Choose a reason for hiding this comment

rtitle Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024 •

edited

Loading

rtitle Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

rtitle Sep 6, 2024 •

edited

Loading

rtitle Sep 6, 2024 •

edited

Loading