[TOAZ-126] Add Sam API for pet managed identities #737

rtitle · 2022-05-24T21:02:24Z

JIRA: https://broadworkbench.atlassian.net/browse/TOAZ-126

Design doc: https://docs.google.com/document/d/1cyrRTibpvMbu0DtAjDsXeQp6-U2RftO66hHNb3nw_h4/edit#heading=h.focnnsveczp4

Demo recording: https://drive.google.com/file/d/1HiiAxQDVteaznwlRQLuFrXfm9PkSJDyU/view?usp=drive_web

Friends with: https://github.com/broadinstitute/firecloud-develop/pull/2991 (but the PRs are independent)

Overview:

Adds AzureRoutes and AzureService classes to Sam
- Contains single POST /api/azure/v1/user/petManagedIdentity API which gets or creates a pet-UAMI
Adds CRL dependency to Sam
Adds SAM_PET_MANAGED_IDENTITY SQL table and associated queries
Updates StandardUserInfoDirectives to handle requests from pet UAMIs and switch back to the owning user
Add unit + connected tests for the above

PR checklist

I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
I've filled out the Security Risk Assessment and attached the result to the JIRA ticket

rtitle · 2022-06-17T20:29:37Z

jenkins retest

rtitle · 2022-06-17T20:30:08Z

render-test-config.sh

+#!/bin/bash
+
+# TODO: Add Janitor secrets here
+# https://broadworkbench.atlassian.net/browse/ID-96


I fleshed out the https://broadworkbench.atlassian.net/browse/ID-96 description a bit more

rtitle · 2022-06-17T20:35:41Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

@@ -336,18 +337,22 @@ object Boot extends IOApp with LazyLogging {
    val managedGroupService =
      new ManagedGroupService(resourceService, policyEvaluatorService, resourceTypeMap, accessPolicyDAO, directoryDAO, cloudExtensionsInitializer.cloudExtensions, config.emailDomain)
    val samApplication = SamApplication(userService, resourceService, statusService, tosService)
-
+    val azureRoutes = config.azureServicesConfig.map { config =>


Note the azureServicesConfig is optional. We won't add the AzureRoutes if it's not present.

This is NOT using the existing CloudExtensionRoutes pattern in Sam. CloudExtensionRoutes doesn't really fit this use case -- Sam code is structured to only have 1 cloud extension, and it mostly deals with Google group stuff which is not relevant on Azure.

Sounds like a renaming/refactoring opportunity to make sure other people don't get confused in the future.

https://broadworkbench.atlassian.net/browse/ID-109

Its disappointing that it's structured this way, but I think it makes sense to do it like this for the sake of expediency.

rtitle · 2022-06-17T20:37:02Z

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

+          content:
+            application/json:
+              schema:
+                type: string


Currently this just returns the managed identity objectId as a String. This fits the Leo/WSM use case. I debated returning more information in a JSON object but keeping it simple for now.

rtitle · 2022-06-17T20:38:28Z

src/test/resources/reference.conf

@@ -116,6 +116,12 @@ testStuff = {
      reuseIds = false
    }
  }
+  # Test MRG in which to create managed identities


We commonly use this MRG for testing in WSM, Leo, etc

rtitle · 2022-06-17T20:40:01Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureServiceSpec.scala

+  "AzureService" should "create a pet managed identity" taggedAs ConnectedTest in {
+    val azureServicesConfig = appConfig.azureServicesConfig
+
+    assume(azureServicesConfig.isDefined, "-- skipping Azure test")


Note these tests only run if render-test-config.sh was run. This doesn't happen in CI/CD, so these tests are skipped. Part of https://broadworkbench.atlassian.net/browse/ID-96 should be enabling these tests in CI/CD.

rtitle · 2022-06-17T20:40:57Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/MockCrlService.scala

+
+import scala.jdk.CollectionConverters._
+
+object MockCrlService extends MockitoSugar {


Hopefully this mockito usage isn't too horrible. 😂 Azure SDKs are mock-able, but create a lot of intermediate classes which need to be mocked. (FWIW WSM tests Azure SDK the same way with mocks.)

rtitle · 2022-06-17T20:43:49Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala

+    * 4. Get the managed app "plan" id
+    * 4. Validate the plan id matches the configured value
+    */
+  private def validateManagedResourceGroup(request: GetOrCreatePetManagedIdentityRequest): IO[Unit] = {


This is a separate check than user access control -- it essentially validates the MRG was created from the Terra managed app.

rtitle · 2022-06-17T20:45:56Z

@tlangs I cleaned this up and added tests -- I think it's working well. I moved out of draft mode and requested review. Feel free to assign others too as needed.

I tried to annotate the PR a bit above. Happy to answer any further questions. And I punted on Janitor integration as we discussed -- added some context to https://broadworkbench.atlassian.net/browse/ID-96 as a follow-on task.

Thanks!

(And I know it's Friday 4:45pm before a long weekend -- I don't expect you to see this until next week 😄 )

rtitle · 2022-06-21T23:31:29Z

jenkins retest

rtitle · 2022-06-22T12:27:01Z

jenkins retest

rtitle · 2022-06-22T14:23:17Z

seems like a spurious rawls failure, jenkins retest

rtitle · 2022-06-22T15:50:09Z

jenkins retest

render-test-config.sh

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

gpolumbo-broad · 2022-06-24T15:44:09Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

@@ -336,18 +337,22 @@ object Boot extends IOApp with LazyLogging {
    val managedGroupService =
      new ManagedGroupService(resourceService, policyEvaluatorService, resourceTypeMap, accessPolicyDAO, directoryDAO, cloudExtensionsInitializer.cloudExtensions, config.emailDomain)
    val samApplication = SamApplication(userService, resourceService, statusService, tosService)
-
+    val azureRoutes = config.azureServicesConfig.map { config =>


Sounds like a renaming/refactoring opportunity to make sure other people don't get confused in the future.

https://broadworkbench.atlassian.net/browse/ID-109

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamRoutes.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala

tlangs · 2022-06-24T17:05:15Z

...broadinstitute/dsde/sam/liquibase/changesets/20220524_pet_user_assigned_managed_identity.xml

+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.4.xsd">
+
+    <changeSet logicalFilePath="dummy" author="rtitle" id="pet_user_assigned_managed_identity">
+        <createTable tableName="SAM_PET_MANAGED_IDENTITY">


Do we want to make this named SAM_AZURE_MANAGED_IDENTITY_PET for clarity?

I followed the pattern of the pre-existing SAM_PET_SERVICE_ACCOUNT table.

We could throw "azure" in there: SAM_PET_AZURE_MANAGED_IDENTITY. wdyt?

Ehhh, I'll leave it. MANAGED_IDENTITY and SERVICE_ACCOUNT are Microsoft & Google concepts respectively, so it just requires that knowledge.

tlangs · 2022-06-24T17:07:53Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

@@ -336,18 +337,22 @@ object Boot extends IOApp with LazyLogging {
    val managedGroupService =
      new ManagedGroupService(resourceService, policyEvaluatorService, resourceTypeMap, accessPolicyDAO, directoryDAO, cloudExtensionsInitializer.cloudExtensions, config.emailDomain)
    val samApplication = SamApplication(userService, resourceService, statusService, tosService)
-
+    val azureRoutes = config.azureServicesConfig.map { config =>


Its disappointing that it's structured this way, but I think it makes sense to do it like this for the sake of expediency.

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/StandardSamUserDirectives.scala

src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala

rtitle · 2022-06-27T15:14:15Z

Thanks for the reviews @gpolumbo-broad and @tlangs!

Some good comments above -- will do another pass and ping you when ready for another look.

…ration

…enapi-generated client

sonarcloud · 2022-07-05T18:13:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

rtitle · 2022-07-05T18:19:23Z

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

@@ -73,6 +73,11 @@ paths:
          required: true
          schema:
            type: string
+      requestBody:


These changes are to work around an OpenAPI codegen bug which surfaced in Workspace Manager: OpenAPITools/openapi-generator#11833

Basically if PUT/POST requests don't have a request body and thus have a null content-type, the generated client throws a NPE. It used to default to application/json but this changed when we upgraded openapi in this PR.

To work around I simply set the content-type for each affected PUT/POST method in swagger. This doesn't affect the actual backend routes -- it's just a client fix.

An alternative fix would be to patch the codegen logic in the mustache code, but I don't know all the implications of that, and this change seems reasonable enough.

Let me know if you have other thoughts..

I'm ok with this.

rtitle · 2022-07-05T18:24:47Z

jenkins retest

rtitle · 2022-07-05T18:38:53Z

Hey @tlangs @gpolumbo-broad, sorry for the delay -- I think I addressed all comments, can you take another look at this? Thanks.

rtitle force-pushed the rt-uami branch from 59b88a5 to 8550d9c Compare May 25, 2022 15:40

rtitle force-pushed the rt-uami branch 4 times, most recently from 4962144 to 4e6e802 Compare June 15, 2022 16:14

rtitle force-pushed the rt-uami branch from 4e6e802 to b093e0b Compare June 17, 2022 14:03

rtitle commented Jun 17, 2022

View reviewed changes

rtitle marked this pull request as ready for review June 17, 2022 20:44

rtitle requested a review from tlangs June 17, 2022 20:44

rtitle changed the title ~~[draft] [TOAZ-126] Add Sam API for pet managed identities~~ [TOAZ-126] Add Sam API for pet managed identities Jun 17, 2022

rtitle force-pushed the rt-uami branch from 6dbd965 to 38ec17f Compare June 22, 2022 12:17

gpolumbo-broad suggested changes Jun 24, 2022

View reviewed changes

tlangs suggested changes Jun 24, 2022

View reviewed changes

rtitle mentioned this pull request Jun 27, 2022

IA-3289 Add Azure module broadinstitute/workbench-libs#1005

Merged

9 tasks

rtitle added 3 commits July 1, 2022 13:07

[TOAZ-126] Add Sam API for pet managed identities

11623b8

Fix swagger and response code

4966820

Clean up, improve test coverage, add stub w/ ticket for Janitor integ…

f3d3d3c

…ration

rtitle added 4 commits July 1, 2022 13:07

More cleanup, fix validation routine

fb3472f

Typo

1075924

Fix content-type of azure routes

8e795c7

Validate UAMI against regex

f902f03

rtitle force-pushed the rt-uami branch from 38ec17f to e86d08c Compare July 1, 2022 17:12

Set content-type on addUserToPolicyV2 in swagger to prevent NPE in op…

2c68af6

…enapi-generated client

rtitle force-pushed the rt-uami branch from e86d08c to 2c68af6 Compare July 1, 2022 17:27

PR feedback

9d89aa8

rtitle commented Jul 5, 2022

View reviewed changes

rtitle requested review from tlangs and gpolumbo-broad July 5, 2022 18:39

tlangs approved these changes Jul 6, 2022

View reviewed changes

rtitle requested a review from zykonda July 6, 2022 17:25

rtitle merged commit 3081228 into develop Jul 6, 2022

rtitle deleted the rt-uami branch July 6, 2022 20:54

zykonda approved these changes Jul 6, 2022

View reviewed changes

rtitle mentioned this pull request Jul 26, 2022

[TOAZ-177] Add route to administratively get a pet MI for a given user #769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TOAZ-126] Add Sam API for pet managed identities #737

[TOAZ-126] Add Sam API for pet managed identities #737

rtitle commented May 24, 2022 •

edited

Loading

rtitle commented Jun 17, 2022

rtitle Jun 17, 2022

rtitle Jun 17, 2022

gpolumbo-broad Jun 24, 2022

tlangs Jun 24, 2022

rtitle Jun 17, 2022

rtitle Jun 17, 2022

rtitle Jun 17, 2022 •

edited

Loading

rtitle Jun 17, 2022 •

edited

Loading

rtitle Jun 17, 2022 •

edited

Loading

rtitle commented Jun 17, 2022 •

edited

Loading

rtitle commented Jun 21, 2022

rtitle commented Jun 22, 2022

rtitle commented Jun 22, 2022

rtitle commented Jun 22, 2022

gpolumbo-broad Jun 24, 2022

tlangs Jun 24, 2022

rtitle Jun 27, 2022 •

edited

Loading

tlangs Jul 6, 2022

tlangs Jun 24, 2022

rtitle commented Jun 27, 2022

sonarcloud bot commented Jul 5, 2022

rtitle Jul 5, 2022 •

edited

Loading

tlangs Jul 6, 2022

rtitle commented Jul 5, 2022

rtitle commented Jul 5, 2022


		import scala.jdk.CollectionConverters._

		object MockCrlService extends MockitoSugar {

[TOAZ-126] Add Sam API for pet managed identities #737

[TOAZ-126] Add Sam API for pet managed identities #737

Conversation

rtitle commented May 24, 2022 • edited Loading

rtitle commented Jun 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

rtitle Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

rtitle Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

rtitle commented Jun 17, 2022 • edited Loading

rtitle commented Jun 21, 2022

rtitle commented Jun 22, 2022

rtitle commented Jun 22, 2022

rtitle commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Jun 27, 2022

sonarcloud bot commented Jul 5, 2022

rtitle Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Jul 5, 2022

rtitle commented Jul 5, 2022

rtitle commented May 24, 2022 •

edited

Loading

rtitle Jun 17, 2022 •

edited

Loading

rtitle Jun 17, 2022 •

edited

Loading

rtitle Jun 17, 2022 •

edited

Loading

rtitle commented Jun 17, 2022 •

edited

Loading

rtitle Jun 27, 2022 •

edited

Loading

rtitle Jul 5, 2022 •

edited

Loading