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

[CORE-40] Add cross-billing project spend report API #3107

Merged
merged 37 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
c0ef330
add getownerworkspaces
calypsomatic Oct 31, 2024
1a396b9
update getOwnerWorkspaces, start to add getBilling
calypsomatic Nov 1, 2024
3a4dc82
interim getBillingForWorkspaces
calypsomatic Nov 1, 2024
56cf2bc
update getbillingforworkspaces to get what we need
calypsomatic Nov 4, 2024
cc2c3bd
update getbilling and query
calypsomatic Nov 6, 2024
2adf61b
remove unnecessary changes
calypsomatic Nov 6, 2024
775616e
update getbillingworkspaces to be one sql query
calypsomatic Nov 7, 2024
414207c
start putting it all together
calypsomatic Nov 7, 2024
c5c736a
add new spendreport api
calypsomatic Nov 7, 2024
81f843b
query roughly working
calypsomatic Nov 8, 2024
0895f80
Merge branch 'develop' into core-40-spend
calypsomatic Nov 14, 2024
a023bbb
update return value of cross billing spend report
calypsomatic Nov 18, 2024
0c6915b
update how to extract spend report results
calypsomatic Nov 19, 2024
9a8694b
add test skeleton
calypsomatic Nov 19, 2024
554256b
get workspaces/bps from sam instead
calypsomatic Nov 19, 2024
9085110
Merge branch 'develop' into core-40-spend
calypsomatic Nov 19, 2024
8f52a84
some cleanup
calypsomatic Nov 19, 2024
b5fa11f
Merge branch 'develop' into core-40-spend
calypsomatic Nov 21, 2024
9fd6bc0
fix unit tests
calypsomatic Nov 21, 2024
9ad4c7e
move logic around and expand test
calypsomatic Nov 21, 2024
b07bbb1
compare spend report permission to workspace ownership
calypsomatic Nov 21, 2024
a0e9897
check own action on workspaces
calypsomatic Nov 22, 2024
9da40c0
fix rebase
calypsomatic Nov 22, 2024
6a03274
get workspaces by spend report action
calypsomatic Nov 22, 2024
2200b62
Merge branch 'develop' into core-40-spend
calypsomatic Nov 22, 2024
e1e31a1
add pagination
calypsomatic Nov 22, 2024
e4a13db
include credits and deal with edge cases
calypsomatic Nov 25, 2024
85ad0f2
start to add trace
calypsomatic Nov 25, 2024
c6d966f
Merge branch 'develop' into core-40-spend
calypsomatic Nov 25, 2024
0c4cbe1
scalafmt
calypsomatic Nov 25, 2024
7d9f379
pr comments round 1
calypsomatic Nov 26, 2024
20dfd8c
pr comments round 2
calypsomatic Nov 26, 2024
cdc0b53
Merge branch 'develop' into core-40-spend
calypsomatic Nov 27, 2024
a6b900f
remove extraneous todo
calypsomatic Nov 27, 2024
e079f56
include public workspaces
calypsomatic Dec 4, 2024
780974f
hierarchical to flat
calypsomatic Dec 4, 2024
9dcfd0f
whoops
calypsomatic Dec 4, 2024
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
49 changes: 49 additions & 0 deletions core/src/main/resources/swagger/api-docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,55 @@ paths:
$ref: '#/components/schemas/ErrorReport'
500:
$ref: '#/components/responses/RawlsInternalError'
/api/billing/v2/spendReport:
get:
tags:
- billing_v2
summary: get spend report for all workspaces user has owner access to
description: get spend report for all workspaces user has owner access to
operationId: getSpendReportAllWorkspaces
parameters:
- name: startDate
in: query
description: start date of report (YYYY-MM-DD). Data included in report will start at 12 AM UTC on this date.
required: true
schema:
type: string
format: date
- name: endDate
in: query
description: end date of report (YYYY-MM-DD). Data included in report will end at 11:59 PM UTC on this date.
required: true
schema:
type: string
format: date
responses:
200:
description: Success
content:
'application/json':
schema:
$ref: '#/components/schemas/SpendReport'
400:
description: invalid spend report parameters
content:
'application/json':
schema:
$ref: '#/components/schemas/ErrorReport'
403:
description: You must be a project owner to view the spend report of a project
content:
'application/json':
schema:
$ref: '#/components/schemas/ErrorReport'
404:
description: The specified billing project could not be found
content:
'application/json':
schema:
$ref: '#/components/schemas/ErrorReport'
500:
$ref: '#/components/responses/RawlsInternalError'
/api/billing/v2/{projectId}/spendReportConfiguration:
get:
tags:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,8 @@ object Boot extends IOApp with LazyLogging {
billingRepository,
billingProfileManagerDAO,
samDAO,
spendReportingServiceConfig
spendReportingServiceConfig,
workspaceServiceConstructor
)

val billingAdminServiceConstructor: RawlsRequestContext => BillingAdminService =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,17 @@ import org.broadinstitute.dsde.rawls.model._
import org.broadinstitute.dsde.rawls.util.{FutureSupport, Retry}
import org.broadinstitute.dsde.workbench.client.sam
import org.broadinstitute.dsde.workbench.client.sam.api._
import org.broadinstitute.dsde.workbench.client.sam.model.{
FilteredHierarchicalResourcePolicy,
ListResourcesV2200Response
}
import org.broadinstitute.dsde.workbench.client.sam.{ApiCallback, ApiClient, ApiException}
import org.broadinstitute.dsde.workbench.model.{WorkbenchEmail, WorkbenchGroupName}

import java.time.Instant
import java.time.temporal.ChronoUnit
import java.util
import java.util.List
import scala.concurrent.duration.FiniteDuration
import scala.concurrent.{ExecutionContext, Future, Promise}
import scala.jdk.CollectionConverters._
Expand Down Expand Up @@ -513,12 +518,54 @@ class HttpSamDAO(baseSamServiceURL: String, rawlsCredential: RawlsCredential, ti
}
}

override def listResourcesWithActions(resourceTypeName: SamResourceTypeName,
action: SamResourceAction,
ctx: RawlsRequestContext
): Future[Seq[SamUserResource]] =
retry(when401or5xx) { () =>
val callback = new SamApiCallback[ListResourcesV2200Response]("listResourcesV2")

resourcesApi(ctx).listResourcesV2Async(
/* format = */ "hierarchical",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* format = */ "hierarchical",
/* format = */ "flat",

hierarchical will not return actions that are included in roles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, with hierarchical I get a policies object like so:

     "policies": [
        {
          "actions": [],
          "inherited": false,
          "isPublic": false,
          "policy": "owner",
          "roles": [
            {
              "actions": [
                "read_spend_report"
              ],
              "role": "owner"
            }
          ]
        }
      ]

but with flat it's just:

      "policies": [
        {
          "inherited": false,
          "isPublic": false,
          "policy": "owner"
        }
      ]

Looks like I want hierarchical?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, this does not seem right, I will look at what sam is doing

Copy link
Contributor

Choose a reason for hiding this comment

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

a full resource in the response for flat looks like

{
      "actions": [
        "read_spend_report"
      ],
      "authDomainGroups": [],
      "missingAuthDomainGroups": [],
      "policies": [
        {
          "inherited": false,
          "isPublic": false,
          "policy": "owner"
        }
      ],
      "resourceId": "eaacdc66-9ac5-4dd9-aeae-1a2c98f1eda8",
      "resourceType": "spend-profile",
      "roles": [
        "owner"
      ]
    }

The actions are not included in the policies. The is probably what it means to be flat.

The full resource for a hierarchical response looks like

{
      "authDomainGroups": [],
      "missingAuthDomainGroups": [],
      "policies": [
        {
          "actions": [],
          "inherited": false,
          "isPublic": false,
          "policy": "owner",
          "roles": [
            {
              "actions": [
                "read_spend_report"
              ],
              "role": "owner"
            }
          ]
        }
      ],
      "resourceId": "7900ff92-a1f0-4c3e-b051-e549e2211e32",
      "resourceType": "spend-profile"
    }

The actions are both at policy and role level. Also note there could be multiple policies that contain the action. Flat is really what you want because it hides all the details about how user has the action on the resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this further, flat vs hierarchical does not matter because nothing is used of the response other than the resource id. But hierarchical is much harder to use correctly. So I would either have this listResourcesWithActions function return only a list of resource ids or switch to flat

/* resourceTypes = */ util.List.of(resourceTypeName.value),
/* policies = */ util.List.of(),
/* roles = */ util.List.of,
/* actions = */ util.List.of(action.value),
/* includePublic = */ false,
Copy link
Contributor

Choose a reason for hiding this comment

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

will the includePublic = false prevent seeing spend reports for public workspaces, even if the calling user is an owner? As in, will our support/comms team be unable to see spend for our public workspaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll change it to true.

callback
)

callback.future.map { resourcesResponse =>
resourcesResponse.getFilteredResourcesHierarchicalResponse
.getResources()
.asScala
.map { resource =>
SamUserResource(
resource.getResourceId,
toSamRolesAndActions(resource.getPolicies()), // TODO what to use here?
toSamRolesAndActions(resource.getPolicies()), // What are all these three things?
toSamRolesAndActions(resource.getPolicies()),
calypsomatic marked this conversation as resolved.
Show resolved Hide resolved
resource.getAuthDomainGroups.asScala.map(WorkbenchGroupName).toSet,
resource.getMissingAuthDomainGroups.asScala.map(WorkbenchGroupName).toSet
)
}
calypsomatic marked this conversation as resolved.
Show resolved Hide resolved
.toSeq
}
}

private def toSamRolesAndActions(rolesAndActions: sam.model.RolesAndActions) =
SamRolesAndActions(
rolesAndActions.getRoles.asScala.map(SamResourceRole).toSet,
rolesAndActions.getActions.asScala.map(SamResourceAction).toSet
)

private def toSamRolesAndActions(policies: util.List[FilteredHierarchicalResourcePolicy]) = {
val scalaPolicies = policies.asScala.toList
val roles = scalaPolicies.flatMap(_.getRoles.asScala)
val actions = scalaPolicies.flatMap(_.getActions.asScala)
SamRolesAndActions(roles.map(role => SamResourceRole(role.toString)).toSet, actions.map(SamResourceAction).toSet)
}

override def getPetServiceAccountKeyForUser(googleProject: GoogleProjectId,
userEmail: RawlsUserEmail
): Future[String] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ trait SamDAO {

def listUserResources(resourceTypeName: SamResourceTypeName, ctx: RawlsRequestContext): Future[Seq[SamUserResource]]

def listResourcesWithActions(resourceTypeName: SamResourceTypeName,
action: SamResourceAction,
ctx: RawlsRequestContext
): Future[Seq[SamUserResource]]

def listPoliciesForResource(resourceTypeName: SamResourceTypeName,
resourceId: String,
ctx: RawlsRequestContext
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,16 @@ trait RawlsBillingProjectComponent {
.result
.map(_.headOption.map(RawlsBillingProjectRecord.toBillingProjectSpendExport))

def getBillingProjectsSpendConfiguration(
billingProjectNames: Seq[RawlsBillingProjectName]
): ReadAction[Seq[Option[BillingProjectSpendExport]]] =
rawlsBillingProjectQuery
.withProjectNames(billingProjectNames)
.result
.map(projectRecords =>
projectRecords.map(record => Some(RawlsBillingProjectRecord.toBillingProjectSpendExport(record)))
)

calypsomatic marked this conversation as resolved.
Show resolved Hide resolved
def insertOperations(operations: Seq[RawlsBillingProjectOperationRecord]): WriteAction[Unit] =
(rawlsBillingProjectOperationQuery ++= operations).map(_ => ())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,9 @@ trait WorkspaceComponent {
def listWithBillingProject(billingProject: RawlsBillingProjectName): ReadAction[Seq[Workspace]] =
workspaceQuery.withBillingProject(billingProject).read

def listWithBillingProjects(billingProjects: List[RawlsBillingProjectName]): ReadAction[Seq[Workspace]] =
workspaceQuery.withBillingProjects(billingProjects).read

def getTags(queryString: Option[String],
limit: Option[Int] = None,
ownerIds: Option[Seq[UUID]] = None
Expand Down Expand Up @@ -644,6 +647,9 @@ trait WorkspaceComponent {
def withBillingProject(projectName: RawlsBillingProjectName): WorkspaceQueryType =
query.filter(_.namespace === projectName.value)

def withBillingProjects(projectNames: List[RawlsBillingProjectName]): WorkspaceQueryType =
query.filter(_.namespace.inSetBind(projectNames.map(_.value)))

def withGoogleProjectId(googleProjectId: GoogleProjectId): WorkspaceQueryType =
query.filter(_.googleProjectId === googleProjectId.value)

Expand Down
Loading
Loading