[GAWB-2942] Leo pets per project: Make ServiceAccountProvider interface and update Leo code to use it #89

rtitle · 2017-12-07T15:14:05Z

https://broadinstitute.atlassian.net/browse/GAWB-2927

Adds a ServiceAccountProvider interface based on this Tech Doc.

Updates Leo code to use this interface to obtain service accounts. NOTE: this PR does not change current Leo behavior; switching to a pets-per-project implementation will be done in a follow-up (much smaller) PR.

A note on naming. I used the following terms (in code and in comments):

leoServiceAccount == the service account Leo executes gcloud commands as
clusterServiceAccount == the service account passed to the 'gcloud dataproc create cluster --serviceAccount' command
notebookServiceAccount == the service account whose key we localize onto the cluster

If people have other suggestions re naming, please let me know.

Includes a database migration to track both types of SAs in the database.

Also beefs up swat tests slightly to check notebook credentials from multiple sources (oauth2, fiss, hadoop).

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

I've documented my API changes in Swagger

In all cases:

Get two thumbsworth of review and PO signoff if necessary
Verify all tests go green
Squash and merge; you can delete your branch after this
Test this change deployed correctly and works on dev environment after deployment

rtitle · 2017-12-07T15:34:36Z

jenkins retest

rtitle · 2017-12-07T15:47:07Z

jenkins retest

rtitle · 2017-12-07T19:33:32Z

.../dsde/workbench/leonardo/liquibase/changesets/20171205_cluster-split-up-service-accounts.xml

+    </changeSet>
+
+    <changeSet logicalFilePath="leonardo" author="rtitle" id="cluster_migrate_service_accounts">
+        <sql dbms="mysql">UPDATE CLUSTER SET notebookServiceAccount = googleServiceAccount</sql>


All existing service accounts in the DB are really notebook service accounts.

rtitle · 2017-12-07T19:38:16Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/model/ServiceAccountProvider.scala

+    *
+    * @return service account email
+    */
+  def getLeoServiceAccount: WorkbenchEmail


Is it OK for this to not return a Future? I'm expecting this to be a constant value, and not an asynchronous operation. It's easier to work with in Leo code if it's not a future.

Seems reasonable.

Actually, shouldn't this one be specified in config, so configurers can pass in the json keyblob?

Ah that's a good point. Leo config currently has a path to the serviceAccountPemFile. Should we remove this interface method and expect people running Leo to just configure the Leo SA+key via config?

Or maybe this interface method can still exist but have a default implementation that reads the Leo SA+key from config?

rtitle · 2017-12-07T19:39:55Z

Should be ready for review. Unit+swat tests should be passing (can't actually check since the github hooks aren't appearing).

rtitle · 2017-12-07T19:42:50Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/monitor/ClusterMonitorActor.scala

@@ -283,4 +280,18 @@ class ClusterMonitorActor(val cluster: Cluster,
        case Right(_) => throw ClusterNotReadyException(cluster.googleProject, cluster.clusterName)
      }
  }
+
+  private def removeCredentialsFromMetadata: Future[Unit] = {


Here's that stub method Hussein

rtitle · 2017-12-07T19:49:18Z

jenkins retest

rtitle · 2017-12-07T20:15:05Z

jenkins retest

rtitle · 2017-12-07T20:46:29Z

jenkins retest

rtitle · 2017-12-08T15:57:31Z

jenkins retest

helgridly

minor comments and questions but 👍

helgridly · 2017-12-07T23:54:40Z

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

@@ -81,11 +85,11 @@ object Boot extends App with LazyLogging {
      }
  }

-  private def constructAuthProvider(authProviderClass: String, authConfig: Config): LeoAuthProvider = {
-    Class.forName(authProviderClass)
+  private def constructProvider[T](className: String, config: Config): T = {


helgridly · 2017-12-07T23:59:31Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/model/ServiceAccountProvider.scala

+    *
+    * @return service account email
+    */
+  def getLeoServiceAccount: WorkbenchEmail


Seems reasonable.

helgridly · 2017-12-08T00:08:51Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/model/ServiceAccountProvider.scala

+  *
+  * @param config any necessary configuration information.
+  */
+abstract class ServiceAccountProvider(config: Config) {


tedious question: the auth provider is called LeoAuthProvider. should this be called LeoServiceAccountProvider?

(we seem to be inconsistent in general whether we should call classes X or LeoX or LeonardoX)

Yeah we're inconsistent in naming. :( I'm not a huge fan of LeoServiceAccountProvider though because it's misleading. It's providing service accounts, not Leo service accounts. LeoAuthProvider is a better because it's providing authorization specific to Leo operations.

helgridly · 2017-12-08T00:13:58Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/model/ServiceAccountProvider.scala

+    *
+    * @return service account email
+    */
+  def getLeoServiceAccount: WorkbenchEmail


Actually, shouldn't this one be specified in config, so configurers can pass in the json keyblob?

helgridly · 2017-12-08T16:18:16Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/monitor/ClusterMonitorActor.scala

@@ -283,4 +280,18 @@ class ClusterMonitorActor(val cluster: Cluster,
        case Right(_) => throw ClusterNotReadyException(cluster.googleProject, cluster.clusterName)
      }
  }
+
+  private def removeCredentialsFromMetadata: Future[Unit] = {


helgridly · 2017-12-08T16:20:24Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/service/LeonardoService.scala

+      _ <- OptionT.liftF(googleIamDAO.removeServiceAccountKey(dataprocConfig.leoGoogleProject, serviceAccountEmail, key))
+    } yield ()
+
+    tea.value.void


suck it, baby shoes

I don't get this reference but I appreciate the comment!

This is a better short story than "for sale, baby shoes, never worn"

helgridly · 2017-12-08T16:25:49Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/service/LeonardoService.scala

+    val tea = for {
+      key <- OptionT(dbRef.inTransaction { _.clusterQuery.getServiceAccountKeyId(googleProject, clusterName) })
+      serviceAccountEmail <- OptionT.fromOption[Future](serviceAccountOpt)
+      _ <- OptionT.liftF(googleIamDAO.removeServiceAccountKey(dataprocConfig.leoGoogleProject, serviceAccountEmail, key))


this might return a Googly exception and nix the " internalDeleteCluster must march on regardless of failures" condition

though on further thought i'm conflicted about this. we don't monitor the result of the internalDeleteCluster future in some cases... do we need delete-retries?

(possibly a problem for another PR)

That's a good catch. I'm not sure what the right thing to do is. I see the value in having delete march on regardless of intermediate failures. But I also don't like swallowing errors if there is a problem.

Note the call to googleIamDAO.removeServiceAccountKey itself retries on errors (and handles 404s): https://github.com/broadinstitute/workbench-libs/blob/develop/google/src/main/scala/org/broadinstitute/dsde/workbench/google/HttpGoogleIamDAO.scala#L168

Is that a reasonable compromise? Or should I jam a recover on this?

I'd say we jam a recover on it. We're going to swallow the error anyway; might as well not bail on cleaning up everything else too.

I did this in the internalDeleteCluster method

helgridly · 2017-12-08T16:32:23Z

automation/src/test/scala/org/broadinstitute/dsde/workbench/leonardo/LeonardoModelCopy.scala

+object ServiceAccountInfo {
+  // TODO: something less hacky, please!
+  // If we're going to use Jackson we should use it the right way, with annotations in our model.
+  // Otherwise we should rip out LeonardoModelCopy + ClusterKluge and just use Leo model objects + spray json (my prefrence).


+1 publish leo-model and use it in swat. both leo-swat and our jupyter-docker are both in need of some serious love once we're AoU-ready.

Sounds good

helgridly · 2017-12-08T16:52:13Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/GoogleDataprocDAO.scala

@@ -104,7 +105,7 @@ class GoogleDataprocDAO(protected val dataprocConfig: DataprocConfig,
    new GoogleCredential.Builder()
      .setTransport(httpTransport)
      .setJsonFactory(jsonFactory)
-      .setServiceAccountId(dataprocConfig.serviceAccountEmail.value)
+      .setServiceAccountId(serviceAccountProvider.getLeoServiceAccount.value)


if this is the only place where we use serviceAccountProvider can we just pass in the leo SA in the ctor instead

helgridly · 2017-12-08T16:55:12Z

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/db/ClusterComponent.scala

@@ -35,7 +27,20 @@ case class ClusterRecord(id: Long,
                         destroyedDate: Timestamp,
                         jupyterExtensionUri: Option[String],
                         initBucket: String,
-                         serviceAccountKeyId: Option[String])
+                         machineConfig: MachineConfigRecord,
+                         serviceAccountInfo: ServiceAccountInfoRecord)


do we need these in the Record classes? i can see why we'd need them in the Cluster class, since we're json-ifying that, but here too?

I added them here because slick doesn't like the ClusterRecord case class with >22 members.

22 is rapidly becoming my least favorite number.

…e service account is used.

…he pem file

… flakey

…ce and update Leo code to use it (#89) * Initial refactoring. Main compiles, tests don't. * Tests compile, don't pass * Add DB migration, tests pass now * Added Sam ServiceAccountProvider implementation, clean up some code * Oops, fix name * Beef up pet swat a little bit * Rebase from develop * Minor fixup * post rebase fix * Fix LeonardoModelCopy + ClusterKluge * Add stub method to remove creds from instance metadata, if an override service account is used. * Fix label swat test * s/overrideServiceAccount/notebookServiceAccount/g * Fix comment * Fix substring check * Dummy commit to test github * PR feedback: made ServiceAccountProvider return both the Leo SA and the pem file * Sleep a little more * Jam a recover on removeServiceAccountKey so the delete doesn't fail * Fix swat test flakiness, fix TODOs * retrieving service account keys from Google is SUPER inconsistent and flakey

rtitle force-pushed the rt-gawb-2942 branch from ca91ece to a75680f Compare December 7, 2017 15:24

rtitle commented Dec 7, 2017

View reviewed changes

rtitle changed the title ~~[NOTYET] [GAWB-2942] Leo pets per project: Make ServiceAccountProvider interface and update Leo code to use it~~ [GAWB-2942] Leo pets per project: Make ServiceAccountProvider interface and update Leo code to use it Dec 7, 2017

rtitle requested a review from helgridly December 7, 2017 19:39

rtitle commented Dec 7, 2017

View reviewed changes

helgridly approved these changes Dec 8, 2017

View reviewed changes

rtitle force-pushed the rt-gawb-2942 branch from f760ab1 to d59c275 Compare December 8, 2017 20:06

rtitle added 15 commits December 11, 2017 13:22

Initial refactoring. Main compiles, tests don't.

8a36b6c

Tests compile, don't pass

99463f8

Add DB migration, tests pass now

d821d00

Added Sam ServiceAccountProvider implementation, clean up some code

383f056

Oops, fix name

9ccc7c1

Beef up pet swat a little bit

d5db83c

Rebase from develop

da95bf3

Minor fixup

aea3622

post rebase fix

28eff38

Fix LeonardoModelCopy + ClusterKluge

826eba3

Add stub method to remove creds from instance metadata, if an overrid…

2560f8a

…e service account is used.

Fix label swat test

82b9ac0

s/overrideServiceAccount/notebookServiceAccount/g

5e58293

Fix comment

28ed197

Fix substring check

b89aecb

rtitle added 6 commits December 11, 2017 13:22

Dummy commit to test github

a87500a

PR feedback: made ServiceAccountProvider return both the Leo SA and t…

913f9d8

…he pem file

Sleep a little more

ee060f9

Jam a recover on removeServiceAccountKey so the delete doesn't fail

f6a2573

Fix swat test flakiness, fix TODOs

5de8cb9

retrieving service account keys from Google is SUPER inconsistent and…

538395c

… flakey

rtitle force-pushed the rt-gawb-2942 branch from 2a7deb3 to 538395c Compare December 11, 2017 18:22

rtitle merged commit b2ec3ec into develop Dec 11, 2017

rtitle deleted the rt-gawb-2942 branch December 11, 2017 23:32

[GAWB-2942] Leo pets per project: Make ServiceAccountProvider interface and update Leo code to use it #89

[GAWB-2942] Leo pets per project: Make ServiceAccountProvider interface and update Leo code to use it #89

Conversation

rtitle commented Dec 7, 2017 • edited Loading

rtitle commented Dec 7, 2017

rtitle commented Dec 7, 2017

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 commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Dec 7, 2017

rtitle commented Dec 7, 2017

rtitle commented Dec 7, 2017

rtitle commented Dec 8, 2017

helgridly left a 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

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

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 Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Dec 7, 2017 •

edited

Loading

rtitle Dec 8, 2017 •

edited

Loading