Skip to content

Commit

Permalink
[DCJ-610] JobModel includes more info on targeted resource (#1779)
Browse files Browse the repository at this point in the history
* [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
  • Loading branch information
okotsopoulos authored Aug 15, 2024
1 parent dfb0741 commit 9175206
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 37 deletions.
23 changes: 22 additions & 1 deletion src/main/java/bio/terra/service/job/JobService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
20 changes: 20 additions & 0 deletions src/main/resources/api/data-repository-openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
144 changes: 108 additions & 36 deletions src/test/java/bio/terra/service/job/JobServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> jobIds = new ArrayList<>();

@Autowired private StairwayJdbcConfiguration stairwayJdbcConfiguration;
Expand Down Expand Up @@ -121,14 +126,7 @@ void enumerateTooLongBackFilterTest() throws Exception {
int numVisibleJobs = 3;
List<JobModel> 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",
Expand All @@ -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<JobModel> 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));
Expand Down Expand Up @@ -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)));
}

Expand All @@ -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)));
}

Expand All @@ -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)));
}

Expand Down Expand Up @@ -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<JobModel> getJobMatcher(JobModel jobModel) {
return samePropertyValuesAs(jobModel, "submitted", "completed");
}
Expand All @@ -324,32 +319,109 @@ private Matcher<JobModel>[] getJobMatchers(List<JobModel> 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<? extends Flight> 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<? extends Flight> clazz, AuthenticatedUserRequest testUser) {
String jobId = jobService.newJob(description, clazz, null, testUser).submit();
String description,
Class<? extends Flight> 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<? extends Flight> 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<? extends Flight> clazz,
AuthenticatedUserRequest testUser,
String resourceId) {
String completedJobId = runFlight(description, clazz, testUser, resourceId);
return jobService.retrieveJob(completedJobId, testUser);
}

private String makeDescription(int ii) {
Expand Down

0 comments on commit 9175206

Please sign in to comment.