[CA-1149] Add initial pass at user disable monitor #504

s-rubenstein · 2021-02-08T19:33:30Z

Ticket:

PR checklist

I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
I've updated the RC_XXX release ticket with any manual steps required to release this change
I've updated the FISMA documentation if I've made any security-related changes, including auth, encryption, or auditing

coveralls · 2021-02-08T19:45:17Z

Coverage decreased (-0.3%) to 78.54% when pulling 8121faa on sr-user-disable-monitor into 4b981bf on develop.

dvoet

some tests too please

dvoet · 2021-02-08T19:43:49Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+        userService
+          .disableUser(message.contents.parseJson.convertTo[WorkbenchUserId], samRequestContext = SamRequestContext(Option(span)))
+      }


I think you are missing something here to message back to the actor to ack the message. Something ending in pipeTo self

dvoet · 2021-02-08T19:45:43Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+  case object StartMonitorPass
+
+  sealed abstract class DisableUserResult(ackId: String)
+  final case class ReportMessage(value: Map[WorkbenchEmail, Seq[SyncReportItem]], ackId: String) extends DisableUserResult(ackId = ackId)


I think SyncReportItem is specific to google group sync. The value here probably needs to be different and constructed at the end of case Some(message: PubSubMessage).

dvoet · 2021-02-08T19:46:55Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+
+  sealed abstract class DisableUserResult(ackId: String)
+  final case class ReportMessage(value: Map[WorkbenchEmail, Seq[SyncReportItem]], ackId: String) extends DisableUserResult(ackId = ackId)
+  final case class FailToSynchronize(t: Throwable, ackId: String) extends DisableUserResult(ackId = ackId)


FailToDisable?

gpolumbo-broad

certainly do not need to wait for my thumb, I just wanted to take a look and had a few questions.

gpolumbo-broad · 2021-02-09T14:14:31Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+    case ReceiveTimeout =>
+      throw new WorkbenchException("DisableUsersMonitorActor has received no messages for too long")
+
+    case x => logger.info(s"unhandled $x")


I would make this message a little more descriptive, maybe:

Suggested change

case x => logger.info(s"unhandled $x")

case x => logger.info(s"Received unhandleable message in DisableUsersMonitor: $x")

actually doing $x is a big no no because it writes only the exception message and loses critical information: the exception class and stack trace.

logger.info(s"Received unhandleable message in DisableUsersMonitor", x) is correct

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

dvoet · 2021-02-10T15:43:53Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+object DisableUsersMonitor {
+  case object StartMonitorPass
+
+  sealed abstract class DisableUserResult(ackId: String)


is there benefit to this having ackId? The common pattern is for this to be a sealed trait and used only as a marker so that when the compiler sees a match on DisableUserResult it can determine that the cases are comprehensive or not. But actually embedding the common field ackId does not buy us anything as we never treat these objects generically.

dvoet · 2021-02-10T15:48:40Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+    case ReceiveTimeout =>
+      throw new WorkbenchException("DisableUsersMonitorActor has received no messages for too long")
+
+    case x => logger.info(s"unhandled $x")


actually doing $x is a big no no because it writes only the exception message and loses critical information: the exception class and stack trace.

logger.info(s"Received unhandleable message in DisableUsersMonitor", x) is correct

dvoet · 2021-02-10T16:26:12Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

+        case userNotFound: WorkbenchExceptionWithErrorReport if userNotFound.errorReport.statusCode.contains(StatusCodes.NotFound) =>
+          // this can happen if a user is deleted before the disable message is handled
+          // acknowledge it so we don't have to handle it again
+          acknowledgeMessage(ackId).map(_ => StartMonitorPass) pipeTo self
+          logger.info(s"user to disable not found: ${userNotFound.errorReport}")


Is this actually true? It looks like disabling a user that does not exist just returns None from userService.disableUser

Yeah, I noticed that. I think maybe there's no special casing here.

dvoet · 2021-02-10T17:09:27Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitorSpec.scala

+    // try to use `Retrying[Future[T]]`, which gets weird when we're using mockito together with it.
+    // Hence adding ascribing [Unit] explicitly here so that `eventually` will use `Retrying[Unit]`
+    eventually[Unit] {
+      assertResult(1) { mockGooglePubSubDAO.acks.size() }


if you move this to after the call the verify you can remove the () below and [Unit] above and get rid of the comment

dvoet · 2021-02-10T17:09:46Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitorSpec.scala

+    // `eventually` now requires an implicit `Retrying` instance. When the statement inside returns future, it'll
+
+    // try to use `Retrying[Future[T]]`, which gets weird when we're using mockito together with it.
+    // Hence adding ascribing [Unit] explicitly here so that `eventually` will use `Retrying[Unit]`


copy pasta error

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala

s-rubenstein · 2021-02-11T15:22:18Z

Jenkins retest

s-rubenstein · 2021-02-11T15:46:00Z

Jenkins retest

marctalbott

Looks good! I suggested one more test, but not sure if the test is viable/useful so consider it nonblocking

marctalbott · 2021-02-11T16:06:01Z

src/test/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitorSpec.scala

@@ -0,0 +1,92 @@
+package org.broadinstitute.dsde.workbench.sam.google


Would it be possible/valuable to add a test that goes through the FailToDisable path? It doesn't seem like that path will really happen since disableUser always seems to return a successful Future, but I'm not positive

I don't think it will ever go through that path, since either disableUser will either return a successful future or a none. I could add a test which throws an exception just in case we add some exceptions to the endpoint? But that feels future-proof-y, and not necessarily correct for current behavior.

s-rubenstein · 2021-02-11T18:58:33Z

Jenkins retest

s-rubenstein · 2021-02-12T19:25:18Z

Jenkins retest

marctalbott · 2021-02-12T19:30:00Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/config/GoogleServicesConfig.scala

@@ -31,6 +31,11 @@ final case class GoogleServicesConfig(
    groupSyncTopic: String,
    groupSyncSubscription: String,
    groupSyncWorkerCount: Int,


Should there be a disableUsersPubSubProject value here too?

So, it wasn't used. I did a bit of searching though, and the only usage of the groupSyncPubSubProject was in setting up the googlePubSubDao, so maybe we should do a refactor of that in general.

marctalbott · 2021-02-12T19:33:28Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/config/GoogleServicesConfig.scala

+    disableUsersPollJitter: FiniteDuration,
+    disableUsersTopic: String,
+    disableUsersSubscription: String,
+    disableUsersWorkerCount: Int,


I also wouldn't mind seeing this and the config for groupSync broken into separate PubSubConfig classes like we have with the GoogleKeyCacheConfig, especially if we'd need these same values for any future pubsub topics we might add to Sam

Yeah, will do some abstraction into a separate config.

s-rubenstein · 2021-02-12T21:00:57Z

Jenkins retest

s-rubenstein · 2021-02-16T15:26:39Z

Jenkins retest

…sableUsersMonitor.scala Co-authored-by: dvoet <dvoet@users.noreply.github.com>

s-rubenstein · 2021-02-16T20:57:37Z

Jenkins retest

s-rubenstein · 2021-02-16T21:18:43Z

Jenkins retest

s-rubenstein · 2021-02-16T21:41:05Z

Jenkins retest

s-rubenstein · 2021-02-17T15:29:33Z

Jenkins retest

s-rubenstein · 2021-02-17T16:15:37Z

Jenkins retest

s-rubenstein · 2021-02-17T18:27:41Z

Jenkins retest

s-rubenstein · 2021-02-17T18:51:50Z

Jenkins retest

s-rubenstein · 2021-02-17T19:24:44Z

Jenkins retest

s-rubenstein · 2021-02-17T20:23:45Z

Jenkins retest

…heck

s-rubenstein · 2021-02-22T19:55:39Z

Jenkins retest

[CA-1190] Change pubSubDAO to be one DAO for each topic

s-rubenstein · 2021-02-24T02:42:48Z

Jenkins retest

s-rubenstein · 2021-02-24T20:15:03Z

Jenkins retest

s-rubenstein · 2021-02-24T20:44:52Z

Jenkins retest

s-rubenstein · 2021-02-25T17:51:00Z

Jenkins retest

s-rubenstein · 2021-02-25T18:31:00Z

jenkins retest

s-rubenstein · 2021-02-25T19:33:34Z

Jenkins retest

s-rubenstein · 2021-02-26T04:10:54Z

Jenkins retest

s-rubenstein · 2021-02-26T15:42:11Z

Jenkins retest

s-rubenstein · 2021-02-26T18:57:27Z

Jenkins retest

s-rubenstein · 2021-03-01T14:53:22Z

Jenkins retest

s-rubenstein · 2021-03-02T15:12:33Z

Jenkins retest

s-rubenstein · 2021-03-02T22:24:22Z

Jenkins retest

s-rubenstein · 2021-03-02T23:00:29Z

Jenkins retest

s-rubenstein · 2021-03-04T19:30:18Z

Jenkins retest

s-rubenstein requested review from dvoet and marctalbott February 8, 2021 19:33

dvoet requested changes Feb 8, 2021

View reviewed changes

gpolumbo-broad reviewed Feb 9, 2021

View reviewed changes

dvoet requested changes Feb 10, 2021

View reviewed changes

dvoet approved these changes Feb 11, 2021

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/DisableUsersMonitor.scala Outdated Show resolved Hide resolved

marctalbott approved these changes Feb 11, 2021

View reviewed changes

s-rubenstein force-pushed the sr-user-disable-monitor branch from abdd3cf to d795234 Compare February 11, 2021 19:34

marctalbott reviewed Feb 12, 2021

View reviewed changes

s-rubenstein and others added 7 commits February 16, 2021 11:39

Add initial pass at user disable monitor

6a6418d

PR Feedback and ide cleanup

11d3a1a

Add tests and change sam model variable internally

5c3da16

Fix tests

cf866f8

PR Comments

9b6a684

Update src/main/scala/org/broadinstitute/dsde/workbench/sam/google/Di…

252cf0b

…sableUsersMonitor.scala Co-authored-by: dvoet <dvoet@users.noreply.github.com>

Fix tests and config values

bd7f4b1

s-rubenstein force-pushed the sr-user-disable-monitor branch from f5b5d37 to bd7f4b1 Compare February 16, 2021 16:39

s-rubenstein added 3 commits February 18, 2021 13:32

Change pubSubDAO to be one DAO for each topic

9db2d77

Add notification config, intermediate google var, and update status c…

b9a4496

…heck

Add specific config, and fix tests

add0ce2

Merge pull request #507 from broadinstitute/sr-pubsub-dao-change

175f3a8

[CA-1190] Change pubSubDAO to be one DAO for each topic

s-rubenstein added 3 commits March 4, 2021 11:31

Revert sam model change

8a3ca26

Fix tests

2fcd444

Keep fixing tests

8121faa

marctalbott approved these changes Mar 4, 2021

View reviewed changes

s-rubenstein merged commit 3b24828 into develop Mar 4, 2021

s-rubenstein deleted the sr-user-disable-monitor branch March 4, 2021 20:36

	case x => logger.info(s"unhandled $x")
	case x => logger.info(s"Received unhandleable message in DisableUsersMonitor: $x")

		@@ -0,0 +1,92 @@
		package org.broadinstitute.dsde.workbench.sam.google

[CA-1149] Add initial pass at user disable monitor #504

[CA-1149] Add initial pass at user disable monitor #504

Conversation

s-rubenstein commented Feb 8, 2021 • edited Loading

coveralls commented Feb 8, 2021 • edited Loading

dvoet 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

gpolumbo-broad 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

s-rubenstein commented Feb 11, 2021

s-rubenstein commented Feb 11, 2021

marctalbott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-rubenstein commented Feb 11, 2021

s-rubenstein commented Feb 12, 2021

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

s-rubenstein commented Feb 12, 2021

s-rubenstein commented Feb 16, 2021

s-rubenstein commented Feb 16, 2021

s-rubenstein commented Feb 16, 2021

s-rubenstein commented Feb 16, 2021

s-rubenstein commented Feb 17, 2021

s-rubenstein commented Feb 17, 2021

s-rubenstein commented Feb 17, 2021

s-rubenstein commented Feb 17, 2021

s-rubenstein commented Feb 17, 2021

s-rubenstein commented Feb 17, 2021

s-rubenstein commented Feb 22, 2021

s-rubenstein commented Feb 24, 2021

s-rubenstein commented Feb 24, 2021

s-rubenstein commented Feb 24, 2021

s-rubenstein commented Feb 25, 2021

s-rubenstein commented Feb 25, 2021

s-rubenstein commented Feb 25, 2021

s-rubenstein commented Feb 26, 2021

s-rubenstein commented Feb 26, 2021

s-rubenstein commented Feb 26, 2021

s-rubenstein commented Mar 1, 2021

s-rubenstein commented Mar 2, 2021

s-rubenstein commented Mar 2, 2021

s-rubenstein commented Mar 2, 2021

s-rubenstein commented Mar 4, 2021

s-rubenstein commented Feb 8, 2021 •

edited

Loading

coveralls commented Feb 8, 2021 •

edited

Loading