[IA-5061] Update Leo to set a parent for notebook-cluster and persistent-disk resources #4781

rtitle · 2024-09-06T18:30:18Z

JIRA ticket: https://broadworkbench.atlassian.net/browse/IA-5061

Dependencies

Depends on Sam PR: broadinstitute/sam#1535.

Summary of changes

Phase 2 of Simplify Leo Access Control epic. Sets a parent (google-project for V1, workspace for V2) for all new notebook-cluster and persistent-disk resources in Sam.

It's a big change set but there is more deleted code than added.

What

Moves all Sam resource creation/deletion to new Sam client
Sets the appropriate parent resource for all resource types
Does not touch access control decision logic; this PR is just concerned with resource creation/deletion
Does not migrate historical records

Why

Using Sam hierarchical resources will let us model resource inheritance in Sam (including auth domain enforcement), simplifying Leonardo.

Testing these changes

What to test

Test plan: https://broadworkbench.atlassian.net/wiki/spaces/IA/pages/3362848772/Leonardo+Access+Control+-+Desired+State#BEE-Test-Plan

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

codecov · 2024-09-09T15:41:47Z

Codecov Report

Attention: Patch coverage is 84.55285% with 19 lines in your changes missing coverage. Please review.

Project coverage is 74.79%. Comparing base (6ef8bcf) to head (736f279).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
...dinstitute/dsde/workbench/leonardo/samModels.scala	0.00%	10 Missing ⚠️
...h/leonardo/http/service/RuntimeServiceInterp.scala	85.71%	3 Missing ⚠️
...kbench/leonardo/monitor/AutoDeleteAppMonitor.scala	40.00%	3 Missing ⚠️
...nch/leonardo/monitor/NonLeoMessageSubscriber.scala	0.00%	2 Missing ⚠️
...itute/dsde/workbench/leonardo/dao/HttpSamDAO.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4781      +/-   ##
===========================================
+ Coverage    74.36%   74.79%   +0.42%     
===========================================
  Files          165      165              
  Lines        15060    14946     -114     
  Branches      1197     1178      -19     
===========================================
- Hits         11200    11179      -21     
+ Misses        3860     3767      -93

Files with missing lines	Coverage Δ
...dsde/workbench/leonardo/auth/SamAuthProvider.scala	`57.72% <ø> (-1.06%)`	⬇️
...institute/dsde/workbench/leonardo/dao/SamDAO.scala	`0.00% <ø> (ø)`
...e/dsde/workbench/leonardo/dao/sam/SamService.scala	`100.00% <ø> (ø)`
.../workbench/leonardo/dao/sam/SamServiceInterp.scala	`98.43% <100.00%> (+0.14%)`	⬆️
...rkbench/leonardo/http/AppDependenciesBuilder.scala	`97.88% <100.00%> (ø)`
...bench/leonardo/http/AzureDependenciesBuilder.scala	`97.29% <ø> (-0.08%)`	⬇️
...rkbench/leonardo/http/GcpDependenciesBuilder.scala	`73.44% <100.00%> (+0.30%)`	⬆️
...ench/leonardo/http/service/DiskServiceInterp.scala	`91.75% <100.00%> (+0.84%)`	⬆️
...ch/leonardo/http/service/DiskV2ServiceInterp.scala	`92.45% <ø> (ø)`
...ch/leonardo/http/service/LeoAppServiceInterp.scala	`87.37% <100.00%> (+0.41%)`	⬆️
... and 11 more

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 6ef8bcf...736f279. Read the comment docs.

rtitle · 2024-09-10T14:24:06Z

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

@@ -315,45 +315,6 @@ class SamAuthProvider[F[_]: OpenTelemetryMetrics](
    } yield roles.nonEmpty
  }

-  override def notifyResourceCreated[R](


Removed old resource creation/deletion methods.

rtitle · 2024-09-10T14:24:36Z

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

+   * @return access token for the user's arbitrary pet service account, or SamException if
+   *         the pet could not be retrieved.
+   */
+  def getArbitraryPetServiceAccountToken(userEmail: WorkbenchEmail)(implicit ev: Ask[F, AppContext]): F[String]


I missed this earlier when implementing SamService - this is needed for Azure functionality.

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

rtitle · 2024-09-10T14:28:56Z

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

@@ -23,12 +22,10 @@ import org.typelevel.log4cats.StructuredLogger

 import scala.concurrent.ExecutionContext

-class DiskV2ServiceInterp[F[_]: Parallel](config: PersistentDiskConfig,


These fields were unused.

rtitle · 2024-09-11T15:40:50Z

I'm in the process of testing on a BEE, but requesting reviews in the meantime (@mlilynolting @LizBaldo ). Thanks!

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

mlilynolting · 2024-09-11T16:00:15Z

This looks great; I'd like to ideally replace SamRole with something specific to each resource type, since the roles are unique per resource and can mean different things (a bit silly maybe since almost everything is 'creator', but...)

rtitle · 2024-09-11T17:39:50Z

FWIW, documented test plan here: https://broadworkbench.atlassian.net/wiki/spaces/IA/pages/3362848772/Leonardo+Access+Control+-+Desired+State#BEE-Test-Plan

rtitle · 2024-09-11T19:57:26Z

I'd like to ideally replace SamRole with something specific to each resource type, since the roles are unique per resource and can mean different things (a bit silly maybe since almost everything is 'creator', but...)

Yeah it would be nice to have some more nesting of resources, actions, and roles… I can explore a little bit.

rtitle · 2024-09-12T14:17:37Z

@mlilynolting @LizBaldo I applied PR feedback so far (refactored role enums, and fixed app policy logic). It seems to be working as intended for GCP and Azure now in my BEE. Requesting re-reviews.

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/samModels.scala

mlilynolting

lfg

LizBaldo

LGTM, it is super clear, I just have a few minor comments.

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/samModels.scala

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

...ain/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala

...est/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala

rtitle · 2024-09-13T19:47:04Z

Applied PR feedback and re-tested, including auth domains as described here: https://broadworkbench.atlassian.net/wiki/spaces/IA/pages/3362848772/Leonardo+Access+Control+-+Desired+State

I think everything is working well. Going to do some final review/validation, then plan to merge this.

rtitle mentioned this pull request Sep 6, 2024

[IA-5060] Inherit manager roles to notebook-cluster and persistent-disk broadinstitute/sam#1535

Merged

2 tasks

rtitle changed the title ~~[DRAFT] First pass: move Sam resource creation/deletion to new client~~ [IA-5061] Update Leo to set a parent for notebook-cluster and persistent-disk resources Sep 10, 2024

rtitle marked this pull request as ready for review September 10, 2024 14:19

rtitle commented Sep 10, 2024

View reviewed changes

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

rtitle commented Sep 10, 2024

View reviewed changes

rtitle added 3 commits September 10, 2024 14:01

First pass: move Sam resource creation/deletion to new client

3dc2525

Tests compile, no idea if they pass

36e62e1

Fix unit tests

362ceac

rtitle force-pushed the rt-fix-leo-access-control-phase2 branch from a31cc22 to 362ceac Compare September 10, 2024 18:01

rtitle requested review from LizBaldo and mlilynolting September 11, 2024 15:40

mlilynolting reviewed Sep 11, 2024

View reviewed changes

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala Outdated Show resolved Hide resolved

rtitle added 2 commits September 11, 2024 20:28

Fix app roles; introduce resource-specific role enums

f999c28

Add unit tests for sam policy maps

dd8cb3b

rtitle requested a review from mlilynolting September 12, 2024 14:17

rtitle commented Sep 12, 2024

View reviewed changes

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/samModels.scala Outdated Show resolved Hide resolved

rtitle commented Sep 12, 2024

View reviewed changes

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/samModels.scala Show resolved Hide resolved

rtitle added 2 commits September 12, 2024 10:32

dummy commit

e1bd9e9

Create SamResourceAction, SamResourceRole base types

56d0a85

mlilynolting approved these changes Sep 12, 2024

View reviewed changes

LizBaldo approved these changes Sep 12, 2024

View reviewed changes

PR feedback

736f279

rtitle merged commit 4ad7058 into develop Sep 13, 2024
23 checks passed

rtitle deleted the rt-fix-leo-access-control-phase2 branch September 13, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IA-5061] Update Leo to set a parent for notebook-cluster and persistent-disk resources #4781

[IA-5061] Update Leo to set a parent for notebook-cluster and persistent-disk resources #4781

rtitle commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

rtitle Sep 10, 2024

rtitle Sep 10, 2024

rtitle Sep 10, 2024

rtitle commented Sep 11, 2024

mlilynolting commented Sep 11, 2024

rtitle commented Sep 11, 2024

rtitle commented Sep 11, 2024 •

edited

Loading

rtitle commented Sep 12, 2024

mlilynolting left a comment

LizBaldo left a comment

rtitle commented Sep 13, 2024

		@@ -23,12 +22,10 @@ import org.typelevel.log4cats.StructuredLogger

		import scala.concurrent.ExecutionContext

		class DiskV2ServiceInterp[F[_]: Parallel](config: PersistentDiskConfig,

[IA-5061] Update Leo to set a parent for notebook-cluster and persistent-disk resources #4781

[IA-5061] Update Leo to set a parent for notebook-cluster and persistent-disk resources #4781

Conversation

rtitle commented Sep 6, 2024 • edited Loading

Dependencies

Summary of changes

What

Why

Testing these changes

What to test

Who tested and where

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

rtitle Sep 10, 2024

Choose a reason for hiding this comment

rtitle Sep 10, 2024

Choose a reason for hiding this comment

rtitle Sep 10, 2024

Choose a reason for hiding this comment

rtitle commented Sep 11, 2024

mlilynolting commented Sep 11, 2024

rtitle commented Sep 11, 2024

rtitle commented Sep 11, 2024 • edited Loading

rtitle commented Sep 12, 2024

mlilynolting left a comment

Choose a reason for hiding this comment

LizBaldo left a comment

Choose a reason for hiding this comment

rtitle commented Sep 13, 2024

rtitle commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

rtitle commented Sep 11, 2024 •

edited

Loading