From 9175206c86d1b7e01ba8bb62e6d71e8d5d4178b3 Mon Sep 17 00:00:00 2001 From: Olivia Kotsopoulos Date: Thu, 15 Aug 2024 13:46:21 -0400 Subject: [PATCH] [DCJ-610] JobModel includes more info on targeted resource (#1779) * [DCJ-610] JobModel includes more info on targeted resource Specifically, iam_resource_type, iam_resource_id, and iam_resource_action. Motivation: when debugging flights on behalf of users, it's not always clear from a flight's description or log spelunking what resource is being targeted. We already pass this information as flight inputs for the majority of our flights, to enable Sam-based permission checks when retrieving information on flights. * Reformat target resource information in JobModel It makes more sense to group the type, ID, and action together in a single JobTargetResourceModel object. Expanded unit tests to exercise cases where these values are not available as input parameters. * test - rename test methods to clarify behavior --- .../bio/terra/service/job/JobService.java | 23 ++- .../api/data-repository-openapi.yaml | 20 +++ .../bio/terra/service/job/JobServiceTest.java | 144 +++++++++++++----- 3 files changed, 150 insertions(+), 37 deletions(-) diff --git a/src/main/java/bio/terra/service/job/JobService.java b/src/main/java/bio/terra/service/job/JobService.java index 3ddedd5724..77e19d1849 100644 --- a/src/main/java/bio/terra/service/job/JobService.java +++ b/src/main/java/bio/terra/service/job/JobService.java @@ -15,6 +15,7 @@ import bio.terra.common.stairway.StairwayComponent; import bio.terra.common.stairway.StairwayLoggingHook; import bio.terra.model.JobModel; +import bio.terra.model.JobTargetResourceModel; import bio.terra.service.auth.iam.IamAction; import bio.terra.service.auth.iam.IamResourceType; import bio.terra.service.auth.iam.IamService; @@ -289,7 +290,27 @@ public JobModel mapFlightStateToJobModel(FlightState flightState) { .jobStatus(jobStatus) .statusCode(statusCode.value()) .submitted(submittedDate) - .completed(completedDate); + .completed(completedDate) + .targetIamResource(createTargetResource(flightState)); + } + + private JobTargetResourceModel createTargetResource(FlightState flightState) { + FlightMap inputParameters = flightState.getInputParameters(); + IamResourceType iamResourceType = + inputParameters.get(JobMapKeys.IAM_RESOURCE_TYPE.getKeyName(), IamResourceType.class); + String iamResourceId = + inputParameters.get(JobMapKeys.IAM_RESOURCE_ID.getKeyName(), String.class); + IamAction iamResourceAction = + inputParameters.get(JobMapKeys.IAM_ACTION.getKeyName(), IamAction.class); + + JobTargetResourceModel targetResource = new JobTargetResourceModel().id(iamResourceId); + if (iamResourceType != null) { + targetResource.setType(iamResourceType.getSamResourceName()); + } + if (iamResourceAction != null) { + targetResource.setAction(iamResourceAction.toString()); + } + return targetResource; } private JobModel.JobStatusEnum getJobStatus(FlightState flightState) { diff --git a/src/main/resources/api/data-repository-openapi.yaml b/src/main/resources/api/data-repository-openapi.yaml index 013cc87c59..d074abb1a5 100644 --- a/src/main/resources/api/data-repository-openapi.yaml +++ b/src/main/resources/api/data-repository-openapi.yaml @@ -6709,8 +6709,28 @@ components: class_name: type: string description: Class name of the flight + target_iam_resource: + $ref: '#/components/schemas/JobTargetResourceModel' description: > Status of job + JobTargetResourceModel: + required: + - type + - id + type: object + description: The IAM resource targeted by a flight + properties: + type: + type: string + description: Type of the flight's target resource + id: + type: string + description: Unique identifier for the flight's target resource + action: + type: string + description: > + The Sam action which a caller must hold on the flight's target resource in order to + retrieve information about this flight ErrorModel: required: - message diff --git a/src/test/java/bio/terra/service/job/JobServiceTest.java b/src/test/java/bio/terra/service/job/JobServiceTest.java index c737d8ba81..0f647eee1d 100644 --- a/src/test/java/bio/terra/service/job/JobServiceTest.java +++ b/src/test/java/bio/terra/service/job/JobServiceTest.java @@ -19,6 +19,7 @@ import bio.terra.common.iam.AuthenticatedUserRequest; import bio.terra.model.JobModel; import bio.terra.model.JobModel.JobStatusEnum; +import bio.terra.model.JobTargetResourceModel; import bio.terra.model.UnlockResourceRequest; import bio.terra.service.auth.iam.IamAction; import bio.terra.service.auth.iam.IamResourceType; @@ -84,6 +85,10 @@ class JobServiceTest { .setToken("token") .build(); + private static final IamResourceType IAM_RESOURCE_TYPE = IamResourceType.DATASET; + private static final String IAM_RESOURCE_ID = UUID.randomUUID().toString(); + private static final IamAction IAM_RESOURCE_ACTION = IamAction.SHARE_POLICY_READER; + private final List jobIds = new ArrayList<>(); @Autowired private StairwayJdbcConfiguration stairwayJdbcConfiguration; @@ -121,14 +126,7 @@ void enumerateTooLongBackFilterTest() throws Exception { int numVisibleJobs = 3; List expectedJobs = IntStream.range(0, numVisibleJobs) - .mapToObj( - i -> - new JobModel() - .id(runFlight(makeDescription(i), makeFlightClass(i), testUser)) - .jobStatus(JobStatusEnum.SUCCEEDED) - .statusCode(HttpStatus.I_AM_A_TEAPOT.value()) - .description(makeDescription(i)) - .className(makeFlightClass(i).getName())) + .mapToObj(this::runFlightAndReturnExpectedJobModel) .toList(); assertThat( "All jobs are returned", @@ -151,16 +149,7 @@ void retrieveTest() throws Exception { // The fids list should be in exactly the same order as the database ordered by submit time. List expectedJobs = - IntStream.range(0, 7) - .mapToObj( - i -> - new JobModel() - .id(runFlight(makeDescription(i), makeFlightClass(i), testUser)) - .jobStatus(JobStatusEnum.SUCCEEDED) - .statusCode(HttpStatus.I_AM_A_TEAPOT.value()) - .description(makeDescription(i)) - .className(makeFlightClass(i).getName())) - .toList(); + IntStream.range(0, 7).mapToObj(this::runFlightAndReturnExpectedJobModel).toList(); // Test single retrieval testSingleRetrieval(expectedJobs.get(2)); @@ -215,7 +204,7 @@ void enumerateJobsPermissionTest() throws Exception { UUID privateDatasetId = UUID.randomUUID(); for (int i = 0; i < 2; i++) { allJobs.add( - runFlightWithParameters( + runFlightAndRetrieveJob( makeDescription(i), makeFlightClass(i), testUser, String.valueOf(privateDatasetId))); } @@ -236,7 +225,7 @@ void enumerateJobsPermissionTest() throws Exception { UUID sharedDatasetId = UUID.randomUUID(); for (int i = 0; i < 2; i++) { allJobs.add( - runFlightWithParameters( + runFlightAndRetrieveJob( makeDescription(i), makeFlightClass(i), testUser, String.valueOf(sharedDatasetId))); } @@ -247,7 +236,7 @@ void enumerateJobsPermissionTest() throws Exception { // Launch 5 jobs as the testUser2 for (int i = 0; i < 5; i++) { allJobs.add( - runFlightWithParameters( + runFlightAndRetrieveJob( makeDescription(i), makeFlightClass(i), testUser2, String.valueOf(sharedDatasetId))); } @@ -315,6 +304,12 @@ void testBadIdRetrieveResult() throws InterruptedException { StairwayException.class, () -> jobService.retrieveJobResult("abcdef", Object.class, null)); } + @Test + void testSubmissionAndRetrieval_noParameters() throws InterruptedException { + JobModel expectedJob = runFlightAndReturnExpectedJobModel(1, false); + testSingleRetrieval(expectedJob); + } + private Matcher getJobMatcher(JobModel jobModel) { return samePropertyValuesAs(jobModel, "submitted", "completed"); } @@ -324,32 +319,109 @@ private Matcher[] getJobMatchers(List jobModels) { return jobModels.stream().map(this::getJobMatcher).toArray(Matcher[]::new); } - // Submit a flight; wait for it to finish; return the flight id + /** + * Submit a flight with optional input parameters and wait for it to finish. + * + * @param i an integer used to construct distinct flight descriptions, determine the flight class + * @param shouldAddParameters whether the job should be submitted with additional input params + * @return the expected JobModel representation of the completed flight + */ + private JobModel runFlightAndReturnExpectedJobModel(int i, boolean shouldAddParameters) { + String description = makeDescription(i); + Class flightClass = makeFlightClass(i); + JobTargetResourceModel targetResource = new JobTargetResourceModel(); + if (shouldAddParameters) { + targetResource + .type(IAM_RESOURCE_TYPE.getSamResourceName()) + .id(IAM_RESOURCE_ID) + .action(IAM_RESOURCE_ACTION.toString()); + } + // Submit a flight with optional input parameters and wait for it to finish. + String jobId = + runFlight(description, flightClass, testUser, IAM_RESOURCE_ID, shouldAddParameters); + + return new JobModel() + .id(jobId) + .jobStatus(JobStatusEnum.SUCCEEDED) + .statusCode(HttpStatus.I_AM_A_TEAPOT.value()) + .description(description) + .className(flightClass.getName()) + .targetIamResource(targetResource); + } + + /** + * Submit a flight with input parameters and wait for it to finish. + * + * @param i an integer used to construct distinct flight descriptions, determine the flight class + * @return the expected JobModel representation of the completed flight + */ + private JobModel runFlightAndReturnExpectedJobModel(int i) { + return runFlightAndReturnExpectedJobModel(i, true); + } + + /** + * Submit a flight with optional input parameters and wait for it to finish. + * + * @param description flight description + * @param clazz flight to submit + * @param testUser user initiating the flight + * @param resourceId ID of the resource targeted by this flight + * @param shouldAddParameters whether the job should be submitted with additional input params + * @return the job ID of the completed flight. + */ private String runFlight( - String description, Class clazz, AuthenticatedUserRequest testUser) { - String jobId = jobService.newJob(description, clazz, null, testUser).submit(); + String description, + Class clazz, + AuthenticatedUserRequest testUser, + String resourceId, + boolean shouldAddParameters) { + JobBuilder jobBuilder = jobService.newJob(description, clazz, null, testUser); + if (shouldAddParameters) { + jobBuilder + .addParameter(JobMapKeys.IAM_RESOURCE_TYPE.getKeyName(), IAM_RESOURCE_TYPE) + .addParameter(JobMapKeys.IAM_RESOURCE_ID.getKeyName(), resourceId) + .addParameter(JobMapKeys.IAM_ACTION.getKeyName(), IAM_RESOURCE_ACTION); + } + String jobId = jobBuilder.submit(); // Poll repeatedly with no breaks: we expect the job to complete quickly. jobService.waitForJob(jobId, 0); jobIds.add(jobId); return jobId; } - private JobModel runFlightWithParameters( + /** + * Submit a flight with input parameters and wait for it to finish. + * + * @param description flight description + * @param clazz flight to submit + * @param testUser user initiating the flight + * @param resourceId ID of the resource targeted by this flight + * @return the job ID of the completed flight. + */ + private String runFlight( String description, Class clazz, AuthenticatedUserRequest testUser, String resourceId) { - String jobId = - jobService - .newJob(description, clazz, null, testUser) - .addParameter(JobMapKeys.IAM_RESOURCE_TYPE.getKeyName(), IamResourceType.DATASET) - .addParameter(JobMapKeys.IAM_RESOURCE_ID.getKeyName(), resourceId) - .addParameter(JobMapKeys.IAM_ACTION.getKeyName(), IamAction.INGEST_DATA) - .submit(); - // Poll repeatedly with no breaks: we expect the job to complete quickly. - jobService.waitForJob(jobId, 0); - jobIds.add(jobId); - return jobService.retrieveJob(jobId, testUser); + return runFlight(description, clazz, testUser, resourceId, true); + } + + /** + * Submit a flight with input parameters and wait for it to finish. + * + * @param description flight description + * @param clazz flight to submit + * @param testUser user initiating the flight + * @param resourceId ID of the resource targeted by this flight + * @return the job representation of the completed flight. + */ + private JobModel runFlightAndRetrieveJob( + String description, + Class clazz, + AuthenticatedUserRequest testUser, + String resourceId) { + String completedJobId = runFlight(description, clazz, testUser, resourceId); + return jobService.retrieveJob(completedJobId, testUser); } private String makeDescription(int ii) {