TSPS-357 Add admin endpoint to get a user's quota and update their quota #162
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,17 @@ | ||
| package bio.terra.pipelines.app.controller; | ||
|
|
||
| import bio.terra.common.exception.BadRequestException; | ||
| import bio.terra.common.iam.SamUser; | ||
| import bio.terra.common.iam.SamUserFactory; | ||
| import bio.terra.pipelines.app.configuration.external.SamConfiguration; | ||
| import bio.terra.pipelines.common.utils.PipelinesEnum; | ||
| import bio.terra.pipelines.db.entities.Pipeline; | ||
| import bio.terra.pipelines.db.entities.UserQuota; | ||
| import bio.terra.pipelines.dependencies.sam.SamService; | ||
| import bio.terra.pipelines.generated.api.AdminApi; | ||
| import bio.terra.pipelines.generated.model.*; | ||
| import bio.terra.pipelines.service.PipelinesService; | ||
| import bio.terra.pipelines.service.QuotasService; | ||
| import io.swagger.annotations.Api; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import org.slf4j.Logger; | ||
|
|
@@ -27,19 +30,22 @@ public class AdminApiController implements AdminApi { | |
| private final HttpServletRequest request; | ||
| private final PipelinesService pipelinesService; | ||
| private final SamService samService; | ||
| private final QuotasService quotasService; | ||
|
|
||
| @Autowired | ||
| public AdminApiController( | ||
| SamConfiguration samConfiguration, | ||
| SamUserFactory samUserFactory, | ||
| HttpServletRequest request, | ||
| PipelinesService pipelinesService, | ||
| SamService samService, | ||
| QuotasService quotasService) { | ||
| this.samConfiguration = samConfiguration; | ||
| this.samUserFactory = samUserFactory; | ||
| this.request = request; | ||
| this.pipelinesService = pipelinesService; | ||
| this.samService = samService; | ||
| this.quotasService = quotasService; | ||
| } | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(AdminApiController.class); | ||
|
|
@@ -58,6 +64,17 @@ public ResponseEntity<ApiAdminPipeline> getPipeline(String pipelineName) { | |
| return new ResponseEntity<>(pipelineToApiAdminPipeline(updatedPipeline), HttpStatus.OK); | ||
| } | ||
|
|
||
| @Override | ||
| public ResponseEntity<ApiAdminQuota> getQuota(String pipelineName, String userId) { | ||
|
There was a problem hiding this comment. just to be super clear, could we rename this |
||
| final SamUser authedUser = getAuthenticatedInfo(); | ||
| samService.checkAdminAuthz(authedUser); | ||
| PipelinesEnum validatedPipelineName = | ||
| PipelineApiUtils.validatePipelineName(pipelineName, logger); | ||
|
|
||
| UserQuota userQuota = quotasService.getQuotaForUserAndPipeline(userId, validatedPipelineName); | ||
| return new ResponseEntity<>(userQuotaToApiAdminQuota(userQuota), HttpStatus.OK); | ||
| } | ||
|
|
||
| @Override | ||
| public ResponseEntity<ApiAdminPipeline> updatePipeline( | ||
| String pipelineName, ApiUpdatePipelineRequestBody body) { | ||
|
|
@@ -74,6 +91,28 @@ public ResponseEntity<ApiAdminPipeline> updatePipeline( | |
| return new ResponseEntity<>(pipelineToApiAdminPipeline(updatedPipeline), HttpStatus.OK); | ||
| } | ||
|
|
||
| @Override | ||
| public ResponseEntity<ApiAdminQuota> updateQuotaLimit( | ||
|
There was a problem hiding this comment. similarly could this be |
||
| String pipelineName, String userId, ApiUpdateQuotaLimitRequestBody body) { | ||
| final SamUser authedUser = getAuthenticatedInfo(); | ||
| samService.checkAdminAuthz(authedUser); | ||
| PipelinesEnum validatedPipelineName = | ||
| PipelineApiUtils.validatePipelineName(pipelineName, logger); | ||
|
|
||
| // Check if a row exists for this user + pipeline. Throw an error if it doesn't | ||
| // A row should exist if a user has run into a quota issue before. | ||
| if (!quotasService.isUserQuotaPresent(userId, validatedPipelineName)) { | ||
|
There was a problem hiding this comment. curious about the thought process behind this approach, versus adding an option to There was a problem hiding this comment. hmmm well calling the other admin endpoint will actually create that row for the user cuz of how the we could write a get function that errors out if the row doesnt exist instead of generating a new one for these admin calls There was a problem hiding this comment. yeah I'm saying we could add an input to |
||
| throw new BadRequestException( | ||
| String.format( | ||
| "User quota not found for user %s and pipeline %s", | ||
| userId, validatedPipelineName.getValue())); | ||
| } | ||
| int newQuotaLimit = body.getQuotaLimit(); | ||
| UserQuota userQuota = quotasService.getQuotaForUserAndPipeline(userId, validatedPipelineName); | ||
| userQuota = quotasService.updateQuotaLimit(userQuota, newQuotaLimit); | ||
| return new ResponseEntity<>(userQuotaToApiAdminQuota(userQuota), HttpStatus.OK); | ||
| } | ||
|
|
||
| public ApiAdminPipeline pipelineToApiAdminPipeline(Pipeline pipeline) { | ||
| return new ApiAdminPipeline() | ||
| .pipelineName(pipeline.getName().getValue()) | ||
|
|
@@ -85,4 +124,12 @@ public ApiAdminPipeline pipelineToApiAdminPipeline(Pipeline pipeline) { | |
| .workspaceGoogleProject(pipeline.getWorkspaceGoogleProject()) | ||
| .wdlMethodVersion(pipeline.getWdlMethodVersion()); | ||
| } | ||
|
|
||
| public ApiAdminQuota userQuotaToApiAdminQuota(UserQuota userQuota) { | ||
| return new ApiAdminQuota() | ||
| .userId(userQuota.getUserId()) | ||
| .pipelineName(userQuota.getPipelineName().getValue()) | ||
| .quotaLimit(userQuota.getQuota()) | ||
| .quotaConsumed(userQuota.getQuotaConsumed()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,4 +75,32 @@ public UserQuota updateQuotaConsumed(UserQuota userQuota, int newQuotaConsumed) | |
| userQuota.setQuotaConsumed(newQuotaConsumed); | ||
| return userQuotasRepository.save(userQuota); | ||
| } | ||
|
|
||
| /** | ||
| * This method updates the quota limit for a given user quota. This should only be called from the | ||
| * Admin Controller | ||
| * | ||
| * @param userQuota - the user quota to update | ||
| * @param newQuotaLimit - the new quota limit | ||
| * @return - the updated user quota | ||
| */ | ||
| public UserQuota updateQuotaLimit(UserQuota userQuota, int newQuotaLimit) { | ||
|
There was a problem hiding this comment. for clarity could we call this |
||
| if (newQuotaLimit < userQuota.getQuotaConsumed()) { | ||
|
There was a problem hiding this comment. so we would allow setting the newQuotaLimit to be equal to the user's current QuotaConsumed, thereby preventing any further activity, is that right? (sounds good to me) |
||
| throw new InternalServerErrorException( | ||
| String.format( | ||
| "New quota limit: %d, is less than the quota consumed: %d", | ||
| newQuotaLimit, userQuota.getQuotaConsumed())); | ||
| } | ||
| userQuota.setQuota(newQuotaLimit); | ||
| return userQuotasRepository.save(userQuota); | ||
| } | ||
|
|
||
| /** | ||
| * @param userId - the user id | ||
| * @param pipelineName - the pipeline name | ||
| * @return - true if the user quota is present, false otherwise | ||
| */ | ||
| public boolean isUserQuotaPresent(String userId, PipelinesEnum pipelineName) { | ||
|
There was a problem hiding this comment. since this doesn't do any other authz checking, can we mark this as also must be called by an admin controller? (just in documentation / function name, as above) There was a problem hiding this comment. i would say this could be called from wherever cuz its not actually altering the database. The other function is altering the database in a way we dont expect the service to be doing There was a problem hiding this comment. that's a good point, sounds good 👍 |
||
| return userQuotasRepository.findByUserIdAndPipelineName(userId, pipelineName).isPresent(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for the user-facing endpoint we call it
quotas, can we be consistent here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea i was following the pattern we made for the admin pipeline path, should i update that to
pipelinesas well?(i like the idea of it matching with our user facing apis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah thanks for fixing pipelines too!