Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AJ-1438: disable most entity APIs for Azure workspaces #2641

Merged
merged 6 commits into from
Dec 10, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.broadinstitute.dsde.rawls.entities

import bio.terra.workspace.model.CloudPlatform
import org.broadinstitute.dsde.rawls.RawlsExceptionWithErrorReport
import org.broadinstitute.dsde.rawls.config.DataRepoEntityProviderConfig
import org.broadinstitute.dsde.rawls.dataaccess.datarepo.DataRepoDAO
@@ -9,7 +10,7 @@ import org.broadinstitute.dsde.rawls.entities.base.{EntityProvider, EntityProvid
import org.broadinstitute.dsde.rawls.entities.datarepo.{DataRepoEntityProvider, DataRepoEntityProviderBuilder}
import org.broadinstitute.dsde.rawls.entities.exceptions.DataEntityException
import org.broadinstitute.dsde.rawls.entities.local.{LocalEntityProvider, LocalEntityProviderBuilder}
import org.broadinstitute.dsde.rawls.model.ErrorReport
import org.broadinstitute.dsde.rawls.model.{ErrorReport, WorkspaceType}

import java.time.Duration
import scala.concurrent.{ExecutionContext, Future}
@@ -43,6 +44,12 @@ class EntityManager(providerBuilders: Set[EntityProviderBuilder[_ <: EntityProvi

def resolveProvider(requestArguments: EntityRequestArguments): Try[EntityProvider] = {

if (!WorkspaceType.RawlsWorkspace.equals(requestArguments.workspace.workspaceType)) {
throw new DataEntityException(
s"This API is disabled for ${CloudPlatform.AZURE} workspaces. Contact support for alternatives."
)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the heart of the PR: throw an error for non-RawlsWorkspace workspaces.

// soon: look up the reference name to ensure it exists.
// for now, this simplistic logic illustrates the approach: choose the right builder for the job.
val targetTag = if (requestArguments.dataReference.isDefined) {
Original file line number Diff line number Diff line change
@@ -540,44 +540,31 @@ class EntityService(protected val ctx: RawlsRequestContext,
}
}

def copyEntities(entityCopyDef: EntityCopyDefinition,
uri: Uri,
linkExistingEntities: Boolean
): Future[EntityCopyResponse] =
getV2WorkspaceContextAndPermissions(entityCopyDef.destinationWorkspace,
SamWorkspaceActions.write,
Some(WorkspaceAttributeSpecs(all = false))
) flatMap { destWorkspaceContext =>
getV2WorkspaceContextAndPermissions(entityCopyDef.sourceWorkspace,
SamWorkspaceActions.read,
Some(WorkspaceAttributeSpecs(all = false))
) flatMap { sourceWorkspaceContext =>
dataSource.inTransaction { dataAccess =>
for {
sourceAD <- DBIO.from(
samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, sourceWorkspaceContext.workspaceId, ctx)
)
destAD <- DBIO.from(
samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, destWorkspaceContext.workspaceId, ctx)
)
result <- authDomainCheck(sourceAD.toSet, destAD.toSet) flatMap { _ =>
val entityNames = entityCopyDef.entityNames
val entityType = entityCopyDef.entityType
val copyResults = traceDBIOWithParent("checkAndCopyEntities", ctx)(s1 =>
dataAccess.entityQuery.checkAndCopyEntities(sourceWorkspaceContext,
destWorkspaceContext,
entityType,
entityNames,
linkExistingEntities,
s1
)
)
copyResults
}
} yield result
}
}
}
def copyEntities(entityCopyDef: EntityCopyDefinition, linkExistingEntities: Boolean): Future[EntityCopyResponse] =
for {
destWsCtx <- getV2WorkspaceContextAndPermissions(entityCopyDef.destinationWorkspace,
SamWorkspaceActions.write,
Some(WorkspaceAttributeSpecs(all = false))
)
sourceWsCtx <- getV2WorkspaceContextAndPermissions(entityCopyDef.sourceWorkspace,
SamWorkspaceActions.read,
Some(WorkspaceAttributeSpecs(all = false))
)
sourceAD <- samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, sourceWsCtx.workspaceId, ctx)
destAD <- samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, destWsCtx.workspaceId, ctx)
_ = authDomainCheck(sourceAD.toSet, destAD.toSet)
entityRequestArguments = EntityRequestArguments(destWsCtx, ctx, None, None)
entityProvider <- entityManager.resolveProviderFuture(entityRequestArguments)
entityCopyResponse <- entityProvider
.copyEntities(sourceWsCtx,
destWsCtx,
entityCopyDef.entityType,
entityCopyDef.entityNames,
linkExistingEntities,
ctx
)
.recover(bigQueryRecover)
} yield entityCopyResponse
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While moving the important implementation inside the LocalEntityProvider, I took the opportunity to refactor and simplify this method. It is now one big for-yield instead of a mix of flatMaps and for-yields, AND it removes outbound REST calls from Rawls to Sam from being inside a database transaction.


def batchUpdateEntitiesInternal(workspaceName: WorkspaceName,
entityUpdates: Seq[EntityUpdateDefinition],
Original file line number Diff line number Diff line change
@@ -7,11 +7,14 @@ import org.broadinstitute.dsde.rawls.model.{
AttributeEntityReference,
AttributeValue,
Entity,
EntityCopyDefinition,
EntityCopyResponse,
EntityQuery,
EntityQueryResponse,
EntityTypeMetadata,
RawlsRequestContext,
SubmissionValidationEntityInputs
SubmissionValidationEntityInputs,
Workspace
}

import scala.concurrent.Future
@@ -70,4 +73,12 @@ trait EntityProvider {
def batchUpdateEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]]

def batchUpsertEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]]

def copyEntities(sourceWorkspaceContext: Workspace,
destWorkspaceContext: Workspace,
entityType: String,
entityNames: Seq[String],
linkExistingEntities: Boolean,
parentContext: RawlsRequestContext
): Future[EntityCopyResponse]
}
Original file line number Diff line number Diff line change
@@ -38,13 +38,16 @@ import org.broadinstitute.dsde.rawls.model.{
AttributeValueList,
AttributeValueRawJson,
Entity,
EntityCopyDefinition,
EntityCopyResponse,
EntityQuery,
EntityQueryResponse,
EntityTypeMetadata,
ErrorReport,
GoogleProjectId,
RawlsRequestContext,
SubmissionValidationEntityInputs
SubmissionValidationEntityInputs,
Workspace
}
import org.broadinstitute.dsde.rawls.util.CollectionUtils
import org.broadinstitute.dsde.rawls.{RawlsException, RawlsExceptionWithErrorReport}
@@ -524,4 +527,13 @@ class DataRepoEntityProvider(snapshotModel: SnapshotModel,

override def batchUpsertEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]] =
throw new UnsupportedEntityOperationException("batch-upsert entities not supported by this provider.")

override def copyEntities(sourceWorkspaceContext: Workspace,
destWorkspaceContext: Workspace,
entityType: String,
entityNames: Seq[String],
linkExistingEntities: Boolean,
parentContext: RawlsRequestContext
): Future[EntityCopyResponse] =
throw new UnsupportedEntityOperationException("copy entities not supported by this provider.")
}
Original file line number Diff line number Diff line change
@@ -26,15 +26,20 @@ import org.broadinstitute.dsde.rawls.model.{
AttributeEntityReference,
AttributeValue,
Entity,
EntityCopyDefinition,
EntityCopyResponse,
EntityQuery,
EntityQueryResponse,
EntityQueryResultMetadata,
EntityTypeMetadata,
ErrorReport,
RawlsRequestContext,
SamResourceTypeNames,
SamWorkspaceActions,
SubmissionValidationEntityInputs,
SubmissionValidationValue,
Workspace
Workspace,
WorkspaceAttributeSpecs
}
import org.broadinstitute.dsde.rawls.util.TracingUtils._
import org.broadinstitute.dsde.rawls.util.{AttributeSupport, CollectionUtils, EntitySupport}
@@ -404,4 +409,22 @@ class LocalEntityProvider(requestArguments: EntityRequestArguments,
override def batchUpsertEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]] =
batchUpdateEntitiesImpl(entityUpdates, upsert = true)

override def copyEntities(sourceWorkspaceContext: Workspace,
destWorkspaceContext: Workspace,
entityType: String,
entityNames: Seq[String],
linkExistingEntities: Boolean,
parentContext: RawlsRequestContext
): Future[EntityCopyResponse] =
traceWithParent("checkAndCopyEntities", parentContext)(_ =>
dataSource.inTransaction { dataAccess =>
dataAccess.entityQuery.checkAndCopyEntities(sourceWorkspaceContext,
destWorkspaceContext,
entityType,
entityNames,
linkExistingEntities,
parentContext
)
}
)
}
Original file line number Diff line number Diff line change
@@ -132,14 +132,14 @@ trait WorkspaceSupport {
} yield ()

// can't use withClonedAuthDomain because the Auth Domain -> no Auth Domain logic is different
def authDomainCheck(sourceWorkspaceADs: Set[String], destWorkspaceADs: Set[String]): ReadWriteAction[Boolean] =
def authDomainCheck(sourceWorkspaceADs: Set[String], destWorkspaceADs: Set[String]): Boolean =
// if the source has any auth domains, the dest must also *at least* have those auth domains
if (sourceWorkspaceADs.subsetOf(destWorkspaceADs)) DBIO.successful(true)
if (sourceWorkspaceADs.subsetOf(destWorkspaceADs)) true
else {
val missingGroups = sourceWorkspaceADs -- destWorkspaceADs
val errorMsg =
s"Source workspace has an Authorization Domain containing the groups ${missingGroups.mkString(", ")}, which are missing on the destination workspace"
DBIO.failed(new RawlsExceptionWithErrorReport(ErrorReport(StatusCodes.UnprocessableEntity, errorMsg)))
throw new RawlsExceptionWithErrorReport(ErrorReport(StatusCodes.UnprocessableEntity, errorMsg))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This authDomainCheck method has only one usage - inside copyEntities, which I rewrote above. There was no need for authDomainCheck to return its result wrapped in a ReadWriteAction.


// WorkspaceContext helpers
Original file line number Diff line number Diff line change
@@ -317,7 +317,7 @@ trait EntityApiService extends UserInfoDirectives {
entity(as[EntityCopyDefinition]) { copyDefinition =>
complete {
entityServiceConstructor(ctx)
.copyEntities(copyDefinition, request.uri, linkExistingEntitiesBool)
.copyEntities(copyDefinition, linkExistingEntitiesBool)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why the request URI was passed into copyEntities. It was never used, and I removed it from the argument list for copyEntities.

.map { response =>
if (
response.hardConflicts.isEmpty && (response.softConflicts.isEmpty || linkExistingEntitiesBool)
Original file line number Diff line number Diff line change
@@ -20,6 +20,7 @@ import org.broadinstitute.dsde.rawls.billing.BillingProjectOrchestrator
import org.broadinstitute.dsde.rawls.bucketMigration.BucketMigrationService
import org.broadinstitute.dsde.rawls.dataaccess.{ExecutionServiceCluster, SamDAO}
import org.broadinstitute.dsde.rawls.entities.EntityService
import org.broadinstitute.dsde.rawls.entities.exceptions.DataEntityException
import org.broadinstitute.dsde.rawls.genomics.GenomicsService
import org.broadinstitute.dsde.rawls.metrics.InstrumentationDirectives
import org.broadinstitute.dsde.rawls.model.{ApplicationVersion, ErrorReport, RawlsRequestContext, UserInfo}
@@ -62,6 +63,9 @@ object RawlsApiService extends LazyLogging {
Sentry.captureException(wsmApiException)
}
complete(wsmApiException.getCode -> ErrorReport(wsmApiException).copy(stackTrace = Seq()))
case dataEntityException: DataEntityException =>
// propagate only the message; don't include the stack trace
complete(dataEntityException.code -> ErrorReport(dataEntityException.getMessage))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about removing stack traces from the response in the catch-all case just below this … but that seemed like it could impact use cases I didn't want to think through. So, I'm adding a case just for DataEntityException, which is only thrown by data tables. There's no need to send the stack trace to the end user for these exceptions.

case e: Throwable =>
// so we don't log the error twice when debug is enabled
if (logger.underlying.isDebugEnabled) {