PROD-972 ID-1324 Deduplicate group synchronization calls to google. #1472

Ghost-in-a-Jar · 2024-07-01T20:17:31Z

Ticket: https://broadworkbench.atlassian.net/browse/ID-1324

What:

We make a LOT of calls to the google groups api, a lot of which are duplicated and unnecessary. This PR aims to reduce the number of calls to google group apis during the group syncing

Why:

We are hitting the quota for listing group memberships every day and ended up with a prod incident over the weekend.

How:

The plan is to record the last synchronized version and current version of group, only synchronize group if group version is greater than last synchronized version.

Pseudocode:

Pull message from pubsub, check current version of group against last synchronized version
if equal:
	do nothing
if the group version is greater than last synchronized version
	sychronize the group

	update last sycnronized version to the version of the group when the synchronization started

In DB migration set initial values of current group version to 1 and last synchronized version to 0

Testing/monitoring notes:

can test manually by pushing our own messages into the queue
add unit tests in GoogleExtensionSpec
add some telemetry around no-ops and list group membership

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 (requires Broad Internal network access) and attached the result to the JIRA ticket

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

dvoet · 2024-07-01T21:13:26Z

I think you also need to update the group version somewhere in or before GoogleExtension.onGroupUpdate. This is the logic that figures out all the policies related to an auth domain that must be synced. The related policies need to have their versions bumped so that the GoogleGroupSynchronizer doesn't ignore them.

dvoet · 2024-07-01T21:29:03Z

also need to update the group version in overwritePolicyMembersInternal

add condition to last synchronized version

Add authdomain test fix google extensions tests

Ghost-in-a-Jar · 2024-07-02T15:57:09Z

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

+        case _: BasicWorkbenchGroup => verify(mockDirectoryDAO).updateSynchronizedDateAndVersion(target, samRequestContext)
+        case p: AccessPolicy =>
+          verify(mockDirectoryDAO).updateSynchronizedDateAndVersion(p.copy(members = p.members + CloudExtensions.allUsersGroupName), samRequestContext)
+      }


This started failing because updateSynchronizedDateAndVersion was being called for the policy with the addition of the all users group in the policy. Still trying to get my head around exactly why. A lot is mocked out here and this test is really only focused around group emails so I think this change is fine.

probably because the policy is set to public

Ghost-in-a-Jar · 2024-07-02T15:57:59Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala

@@ -298,6 +287,7 @@ class ResourceService(
        } else IO.unit
      policies <- listResourcePolicies(resource, samRequestContext)
      _ <- accessPolicyDAO.addResourceAuthDomain(resource, authDomains, samRequestContext)
+      _ <- policies.traverse(p => directoryDAO.updateGroupUpdatedDateAndVersionWithSession(FullyQualifiedPolicyId(resource, p.policyName), samRequestContext))


This updates all of the policies when an auth domain is added to a resource

Ghost-in-a-Jar · 2024-07-02T15:58:39Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala

@@ -45,7 +45,9 @@ trait DirectoryDAO {

  def isGroupMember(groupId: WorkbenchGroupIdentity, member: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Boolean]

-  def updateSynchronizedDate(groupId: WorkbenchGroupIdentity, samRequestContext: SamRequestContext): IO[Unit]
+  def updateSynchronizedDateAndVersion(group: WorkbenchGroup, samRequestContext: SamRequestContext): IO[Unit]


We want to pass in the group here so that we can use the version of the group that is held in memory when the sync started.

Ghost-in-a-Jar · 2024-07-02T15:59:32Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala

-  def updateSynchronizedDate(groupId: WorkbenchGroupIdentity, samRequestContext: SamRequestContext): IO[Unit]
+  def updateSynchronizedDateAndVersion(group: WorkbenchGroup, samRequestContext: SamRequestContext): IO[Unit]
+
+  def updateGroupUpdatedDateAndVersionWithSession(groupId: WorkbenchGroupIdentity, samRequestContext: SamRequestContext): IO[Unit]


This is needed to update the policies in resource service and it just creates a write db session and then calls updateGroupUpdatedDateAndVersion

updateGroupUpdatedDateAndVersion is also only on PostgresGroupDAO which ResourceService doesnt have access to

Ghost-in-a-Jar · 2024-07-02T16:03:38Z

src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala

+    members: Set[WorkbenchSubject],
+    email: WorkbenchEmail,
+    version: Int = 1,
+    lastSynchronizedVersion: Option[Int] = None


Defaulting these values (including in the other models) was to avoid having to change BasicWorkbenchGroup in a ton of places. With the proper tests in place I think this is ok.

dvoet

1 comment but don't hold this up based on it

dvoet · 2024-07-02T16:23:16Z

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

+          if (group.version > group.lastSynchronizedVersion.getOrElse(0)) {
+            IO.unit
+          } else {
+            IO.raiseError(new GroupAlreadySynchronized)


I don't really like handling this with an exception since this is not really an exceptional situation, we just want an early escape.

dvoet · 2024-07-02T16:28:02Z

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

+        case _: BasicWorkbenchGroup => verify(mockDirectoryDAO).updateSynchronizedDateAndVersion(target, samRequestContext)
+        case p: AccessPolicy =>
+          verify(mockDirectoryDAO).updateSynchronizedDateAndVersion(p.copy(members = p.members + CloudExtensions.allUsersGroupName), samRequestContext)
+      }


probably because the policy is set to public

samanehsan

Thanks @Ghost-in-a-Jar! 🎉

sonarqubecloud · 2024-07-02T18:19:19Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
10.1% Duplication on New Code

See analysis details on SonarCloud

PROD-972 ID-1324 Deduplicate group synchronization calls to google.

4e82d9b

dvoet reviewed Jul 1, 2024

View reviewed changes

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala Outdated Show resolved Hide resolved

Ghost-in-a-Jar added 3 commits July 2, 2024 09:36

add group version update to overwritePolicyMembersInternal

1d2a7f3

add condition to last synchronized version

Change Integer -> Int

b3e38be

Add authdomain test fix google extensions tests

Merge branch 'develop' into ID-1324-dedupe-group-sync

8c7293e

Ghost-in-a-Jar marked this pull request as ready for review July 2, 2024 15:53

Ghost-in-a-Jar commented Jul 2, 2024

View reviewed changes

fix tests

74060b9

dvoet approved these changes Jul 2, 2024

View reviewed changes

samanehsan approved these changes Jul 2, 2024

View reviewed changes

Ghost-in-a-Jar added 2 commits July 2, 2024 13:53

increment policy group version when policy is made public

8dd00cd

Merge branch 'develop' into ID-1324-dedupe-group-sync

0c4c496

Ghost-in-a-Jar merged commit 6e240d2 into develop Jul 2, 2024
24 of 28 checks passed

Ghost-in-a-Jar deleted the ID-1324-dedupe-group-sync branch July 2, 2024 19:27

Ghost-in-a-Jar mentioned this pull request Jul 3, 2024

PROD-972 ID-1324 Swallow sync deduplication errors in endpoint. #1477

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROD-972 ID-1324 Deduplicate group synchronization calls to google. #1472

PROD-972 ID-1324 Deduplicate group synchronization calls to google. #1472

Ghost-in-a-Jar commented Jul 1, 2024

dvoet commented Jul 1, 2024

dvoet commented Jul 1, 2024

Ghost-in-a-Jar Jul 2, 2024

dvoet Jul 2, 2024

Ghost-in-a-Jar Jul 2, 2024

Ghost-in-a-Jar Jul 2, 2024

Ghost-in-a-Jar Jul 2, 2024 •

edited

Loading

Ghost-in-a-Jar Jul 2, 2024 •

edited

Loading

Ghost-in-a-Jar Jul 2, 2024 •

edited

Loading

dvoet left a comment

dvoet Jul 2, 2024

dvoet Jul 2, 2024

samanehsan left a comment

sonarqubecloud bot commented Jul 2, 2024

PROD-972 ID-1324 Deduplicate group synchronization calls to google. #1472

PROD-972 ID-1324 Deduplicate group synchronization calls to google. #1472

Conversation

Ghost-in-a-Jar commented Jul 1, 2024

dvoet commented Jul 1, 2024

dvoet commented Jul 1, 2024

Ghost-in-a-Jar Jul 2, 2024

Choose a reason for hiding this comment

dvoet Jul 2, 2024

Choose a reason for hiding this comment

Ghost-in-a-Jar Jul 2, 2024

Choose a reason for hiding this comment

Ghost-in-a-Jar Jul 2, 2024

Choose a reason for hiding this comment

Ghost-in-a-Jar Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Ghost-in-a-Jar Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Ghost-in-a-Jar Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

dvoet left a comment

Choose a reason for hiding this comment

dvoet Jul 2, 2024

Choose a reason for hiding this comment

dvoet Jul 2, 2024

Choose a reason for hiding this comment

samanehsan left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 2, 2024

Quality Gate passed

Ghost-in-a-Jar Jul 2, 2024 •

edited

Loading

Ghost-in-a-Jar Jul 2, 2024 •

edited

Loading

Ghost-in-a-Jar Jul 2, 2024 •

edited

Loading