From 20e44da9b72bc8a20d6303ff951856390aacfbd2 Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 26 Oct 2023 14:14:10 -0400 Subject: [PATCH 1/3] ID-896 Isolate Misbehaving Query --- src/main/resources/sam.conf | 13 ++++++++++++- .../broadinstitute/dsde/workbench/sam/Boot.scala | 10 +++++++--- .../dsde/workbench/sam/config/AppConfig.scala | 7 ++++++- .../dsde/workbench/sam/config/DatabaseConfig.scala | 2 +- .../sam/dataAccess/PostgresAccessPolicyDAO.scala | 8 ++++++-- .../dsde/workbench/sam/util/DatabaseSupport.scala | 7 +++++++ 6 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/main/resources/sam.conf b/src/main/resources/sam.conf index 7fd6b23d7..3da09f16b 100644 --- a/src/main/resources/sam.conf +++ b/src/main/resources/sam.conf @@ -130,7 +130,7 @@ db { poolMaxSize = 8 poolConnectionTimeoutMillis = 5000 driver = "org.postgresql.Driver" - url = ${?POSTGRES_READ_URL} + url = ${?POSTGRES_WRITE_URL} user = ${?POSTGRES_USERNAME} password = ${?POSTGRES_PASSWORD} } @@ -162,6 +162,17 @@ db { user = ${?POSTGRES_USERNAME} password = ${?POSTGRES_PASSWORD} } + + sam_bad_query { + poolName = "sam_background" + poolInitialSize = 5 + poolMaxSize = 5 + poolConnectionTimeoutMillis = 5000 + driver = "org.postgresql.Driver" + url = ${?POSTGRES_READ_URL} + user = ${?POSTGRES_USERNAME} + password = ${?POSTGRES_PASSWORD} + } } admin { diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala index 54bc5abd1..796df6c0b 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala @@ -109,7 +109,8 @@ object Boot extends IOApp with LazyLogging { (foregroundDirectoryDAO, foregroundAccessPolicyDAO, postgresDistributedLockDAO, azureManagedResourceGroupDAO, lastQuotaErrorDAO) <- createDAOs( appConfig, appConfig.samDatabaseConfig.samWrite, - appConfig.samDatabaseConfig.samRead + appConfig.samDatabaseConfig.samRead, + appConfig.samDatabaseConfig.samBadQuery ) // This special set of objects are for operations that happen in the background, i.e. not in the immediate service @@ -117,6 +118,7 @@ object Boot extends IOApp with LazyLogging { (backgroundDirectoryDAO, backgroundAccessPolicyDAO, _, _, _) <- createDAOs( appConfig, appConfig.samDatabaseConfig.samBackground, + appConfig.samDatabaseConfig.samBackground, appConfig.samDatabaseConfig.samBackground ) @@ -201,14 +203,16 @@ object Boot extends IOApp with LazyLogging { private def createDAOs( appConfig: AppConfig, writeDbConfig: DatabaseConfig, - readDbConfig: DatabaseConfig + readDbConfig: DatabaseConfig, + badQueryDbConfig: DatabaseConfig ): cats.effect.Resource[IO, (PostgresDirectoryDAO, AccessPolicyDAO, PostgresDistributedLockDAO[IO], AzureManagedResourceGroupDAO, LastQuotaErrorDAO)] = for { writeDbRef <- DbReference.resource(appConfig.liquibaseConfig, writeDbConfig) readDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readDbConfig) + badQueryDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readDbConfig) directoryDAO = new PostgresDirectoryDAO(writeDbRef, readDbRef) - accessPolicyDAO = new PostgresAccessPolicyDAO(writeDbRef, readDbRef) + accessPolicyDAO = new PostgresAccessPolicyDAO(writeDbRef, readDbRef, Some(badQueryDbRef)) postgresDistributedLockDAO = new PostgresDistributedLockDAO[IO](writeDbRef, readDbRef, appConfig.distributedLockConfig) azureManagedResourceGroupDAO = new PostgresAzureManagedResourceGroupDAO(writeDbRef, readDbRef) lastQuotaErrorDAO = new PostgresLastQuotaErrorDAO(writeDbRef, readDbRef) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala index 31528a53e..2d737721c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala @@ -145,7 +145,12 @@ object AppConfig { } implicit val samDatabaseConfigReader: ValueReader[SamDatabaseConfig] = ValueReader.relative { config => - SamDatabaseConfig(config.as[DatabaseConfig]("sam_read"), config.as[DatabaseConfig]("sam_write"), config.as[DatabaseConfig]("sam_background")) + SamDatabaseConfig( + config.as[DatabaseConfig]("sam_read"), + config.as[DatabaseConfig]("sam_write"), + config.as[DatabaseConfig]("sam_background"), + config.as[DatabaseConfig]("sam_bad_query") + ) } final case class AdminConfig(superAdminsGroup: WorkbenchEmail, allowedEmailDomains: Set[String], serviceAccountAdmins: Set[WorkbenchEmail]) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala index cb0674ff6..97a4675f9 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala @@ -1,6 +1,6 @@ package org.broadinstitute.dsde.workbench.sam.config -final case class SamDatabaseConfig(samRead: DatabaseConfig, samWrite: DatabaseConfig, samBackground: DatabaseConfig) +final case class SamDatabaseConfig(samRead: DatabaseConfig, samWrite: DatabaseConfig, samBackground: DatabaseConfig, samBadQuery: DatabaseConfig) final case class DatabaseConfig( dbName: Symbol, diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index d19eb522e..cb14f2f99 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -23,7 +23,11 @@ import scala.collection.concurrent.TrieMap import scala.util.{Failure, Try} import cats.effect.Temporal -class PostgresAccessPolicyDAO(protected val writeDbRef: DbReference, protected val readDbRef: DbReference)(implicit timer: Temporal[IO]) +class PostgresAccessPolicyDAO( + protected val writeDbRef: DbReference, + protected val readDbRef: DbReference, + protected override val badQueryDbRef: Option[DbReference] = None +)(implicit timer: Temporal[IO]) extends AccessPolicyDAO with DatabaseSupport with PostgresGroupDAO @@ -1353,7 +1357,7 @@ class PostgresAccessPolicyDAO(protected val writeDbRef: DbReference, protected v userId: WorkbenchUserId, samRequestContext: SamRequestContext ): IO[Iterable[ResourceIdWithRolesAndActions]] = - readOnlyTransaction("listUserResourcesWithRolesAndActions", samRequestContext) { implicit session => + readOnlyTransactionBadQuery("listUserResourcesWithRolesAndActions", samRequestContext) { implicit session => class ListUserResourcesQuery extends UserResourcesQuery(resourceTypeName, None, userId) { val policyRole = EffectivePolicyRoleTable.syntax("policyRole") val resourceRole = ResourceRoleTable.syntax("resourceRole") diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala index bc8135d5b..2ae4f4292 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala @@ -13,12 +13,19 @@ import cats.effect.Temporal trait DatabaseSupport { protected val writeDbRef: DbReference protected val readDbRef: DbReference + protected val badQueryDbRef: Option[DbReference] = None protected def readOnlyTransaction[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = { val databaseIO = IO(readDbRef.readOnly(databaseFunction)) readDbRef.runDatabaseIO(dbQueryName, samRequestContext, databaseIO) } + protected def readOnlyTransactionBadQuery[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = { + val dbRef = badQueryDbRef.getOrElse(readDbRef) + val databaseIO = IO(dbRef.readOnly(databaseFunction)) + readDbRef.runDatabaseIO(dbQueryName, samRequestContext, databaseIO) + } + /** Run a database transaction with isolation level set to serializable. See https://www.postgresql.org/docs/9.6/transaction-iso.html. Use this when you need * to be sure there are no concurrent changes that affect the outcome of databaseFunction. When a transaction fails due to a serialization failure it may be * retried. From 2f05ba7912e14d5a0c273ab6a1f4095900eeb9aa Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 26 Oct 2023 14:19:10 -0400 Subject: [PATCH 2/3] rename and fix bug --- .../org/broadinstitute/dsde/workbench/sam/Boot.scala | 8 ++++---- .../dsde/workbench/sam/config/DatabaseConfig.scala | 2 +- .../sam/dataAccess/PostgresAccessPolicyDAO.scala | 4 ++-- .../dsde/workbench/sam/util/DatabaseSupport.scala | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala index 796df6c0b..4607ba14a 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala @@ -110,7 +110,7 @@ object Boot extends IOApp with LazyLogging { appConfig, appConfig.samDatabaseConfig.samWrite, appConfig.samDatabaseConfig.samRead, - appConfig.samDatabaseConfig.samBadQuery + appConfig.samDatabaseConfig.samReadReplica ) // This special set of objects are for operations that happen in the background, i.e. not in the immediate service @@ -204,15 +204,15 @@ object Boot extends IOApp with LazyLogging { appConfig: AppConfig, writeDbConfig: DatabaseConfig, readDbConfig: DatabaseConfig, - badQueryDbConfig: DatabaseConfig + readReplicaDbConfig: DatabaseConfig ): cats.effect.Resource[IO, (PostgresDirectoryDAO, AccessPolicyDAO, PostgresDistributedLockDAO[IO], AzureManagedResourceGroupDAO, LastQuotaErrorDAO)] = for { writeDbRef <- DbReference.resource(appConfig.liquibaseConfig, writeDbConfig) readDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readDbConfig) - badQueryDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readDbConfig) + readReplicaDbRef <- DbReference.resource(appConfig.liquibaseConfig.copy(initWithLiquibase = false), readReplicaDbConfig) directoryDAO = new PostgresDirectoryDAO(writeDbRef, readDbRef) - accessPolicyDAO = new PostgresAccessPolicyDAO(writeDbRef, readDbRef, Some(badQueryDbRef)) + accessPolicyDAO = new PostgresAccessPolicyDAO(writeDbRef, readDbRef, Some(readReplicaDbRef)) postgresDistributedLockDAO = new PostgresDistributedLockDAO[IO](writeDbRef, readDbRef, appConfig.distributedLockConfig) azureManagedResourceGroupDAO = new PostgresAzureManagedResourceGroupDAO(writeDbRef, readDbRef) lastQuotaErrorDAO = new PostgresLastQuotaErrorDAO(writeDbRef, readDbRef) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala index 97a4675f9..efa8bf623 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/DatabaseConfig.scala @@ -1,6 +1,6 @@ package org.broadinstitute.dsde.workbench.sam.config -final case class SamDatabaseConfig(samRead: DatabaseConfig, samWrite: DatabaseConfig, samBackground: DatabaseConfig, samBadQuery: DatabaseConfig) +final case class SamDatabaseConfig(samRead: DatabaseConfig, samWrite: DatabaseConfig, samBackground: DatabaseConfig, samReadReplica: DatabaseConfig) final case class DatabaseConfig( dbName: Symbol, diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index cb14f2f99..5732b7a36 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -26,7 +26,7 @@ import cats.effect.Temporal class PostgresAccessPolicyDAO( protected val writeDbRef: DbReference, protected val readDbRef: DbReference, - protected override val badQueryDbRef: Option[DbReference] = None + protected override val readReplicaDbRef: Option[DbReference] = None )(implicit timer: Temporal[IO]) extends AccessPolicyDAO with DatabaseSupport @@ -1357,7 +1357,7 @@ class PostgresAccessPolicyDAO( userId: WorkbenchUserId, samRequestContext: SamRequestContext ): IO[Iterable[ResourceIdWithRolesAndActions]] = - readOnlyTransactionBadQuery("listUserResourcesWithRolesAndActions", samRequestContext) { implicit session => + readOnlyTransactionReadReplica("listUserResourcesWithRolesAndActions", samRequestContext) { implicit session => class ListUserResourcesQuery extends UserResourcesQuery(resourceTypeName, None, userId) { val policyRole = EffectivePolicyRoleTable.syntax("policyRole") val resourceRole = ResourceRoleTable.syntax("resourceRole") diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala index 2ae4f4292..e7183e27f 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/util/DatabaseSupport.scala @@ -13,15 +13,15 @@ import cats.effect.Temporal trait DatabaseSupport { protected val writeDbRef: DbReference protected val readDbRef: DbReference - protected val badQueryDbRef: Option[DbReference] = None + protected val readReplicaDbRef: Option[DbReference] = None protected def readOnlyTransaction[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = { val databaseIO = IO(readDbRef.readOnly(databaseFunction)) readDbRef.runDatabaseIO(dbQueryName, samRequestContext, databaseIO) } - protected def readOnlyTransactionBadQuery[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = { - val dbRef = badQueryDbRef.getOrElse(readDbRef) + protected def readOnlyTransactionReadReplica[A](dbQueryName: String, samRequestContext: SamRequestContext)(databaseFunction: DBSession => A): IO[A] = { + val dbRef = readReplicaDbRef.getOrElse(readDbRef) val databaseIO = IO(dbRef.readOnly(databaseFunction)) readDbRef.runDatabaseIO(dbQueryName, samRequestContext, databaseIO) } From dac699448cf30cc61137640a8554859eca26eec0 Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 26 Oct 2023 14:28:24 -0400 Subject: [PATCH 3/3] more renaming --- src/main/resources/sam.conf | 8 ++++---- .../dsde/workbench/sam/config/AppConfig.scala | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/resources/sam.conf b/src/main/resources/sam.conf index 3da09f16b..8c0ad2d9e 100644 --- a/src/main/resources/sam.conf +++ b/src/main/resources/sam.conf @@ -163,10 +163,10 @@ db { password = ${?POSTGRES_PASSWORD} } - sam_bad_query { - poolName = "sam_background" - poolInitialSize = 5 - poolMaxSize = 5 + sam_read_replica { + poolName = "sam_read_replica" + poolInitialSize = 8 + poolMaxSize = 8 poolConnectionTimeoutMillis = 5000 driver = "org.postgresql.Driver" url = ${?POSTGRES_READ_URL} diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala index 2d737721c..e6e1a4ee1 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala @@ -149,7 +149,7 @@ object AppConfig { config.as[DatabaseConfig]("sam_read"), config.as[DatabaseConfig]("sam_write"), config.as[DatabaseConfig]("sam_background"), - config.as[DatabaseConfig]("sam_bad_query") + config.as[DatabaseConfig]("sam_read_replica") ) }