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.

Sign up for GitHub

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

Proxy download from Mesos slave over Singularity. #1817

Merged
merged 6 commits into from
Aug 1, 2018
Merged

Proxy download from Mesos slave over Singularity. #1817

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc69460
Proxy file download from Mesos slave through Singularity - needs test…
kdorosh Jul 9, 2018
0ad34c5
Tested SingularityService server side proxy code.
kdorosh Jul 10, 2018
ec9500c
Fixed response header. Tested download from UI over proxy in docker l…
kdorosh Jul 11, 2018
c2d8871
Fix potential null dereference.
kdorosh Jul 11, 2018
d74d500
Injected AsyncHttpClient instead of making new HttpClient and stopped…
kdorosh Jul 11, 2018
a76151f
Switch to query params.
kdorosh Jul 11, 2018
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
30 changes: 30 additions & 0 deletions SingularityService/src/main/java/com/hubspot/singularity/resources/TaskResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import static com.hubspot.singularity.WebExceptions.checkNotFound;
import static com.hubspot.singularity.WebExceptions.notFound;

import java.io.InputStream;
import java.nio.file.Paths;
import java.util.Collections;
import java.util.List;

Expand All @@ -18,6 +20,8 @@
import javax.ws.rs.Produces;
import javax.ws.rs.QueryParam;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.ClientBuilder;
import javax.ws.rs.core.Context;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
Expand Down Expand Up @@ -609,4 +613,30 @@ public List<SingularityTaskShellCommandUpdate> getShellCommandHisotryUpdates(
SingularityTaskId taskIdObj = getTaskIdFromStr(taskId);
return taskManager.getTaskShellCommandUpdates(new SingularityTaskShellCommandRequestId(taskIdObj, commandName, commandTimestamp));
}

@GET
@Path("/download/{httpPrefix}/{slaveHostname}/port/{port}/path/{path}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort of a nit, but a better API design here could be something like GET /download?slaveHostname=x&path=y (since the actual resource being retrieved is a download). The rest of our API doesn't stick to these guidelines religiously, though, so probably not a huge deal. I'll let @ssalinas weigh in as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be better to have them both as PathParams instead of QueryParams since both the slave hostname and the path are both required fields? I did change the URL a bit to better match entity class hierarchy standards. See top answer and comments on this StackOverflow post.

@Produces(MediaType.APPLICATION_OCTET_STREAM)
@Operation(summary = "Proxy a file download from a Mesos Slave through Singularity")
@Consumes(MediaType.APPLICATION_FORM_URLENCODED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a GET, it shouldn't consume anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-- whoops -- left this there from when I had it as a POST

public Response downloadFileOverProxy(
@Parameter(required = true, description = "Https (if available) or http") @PathParam("httpPrefix") String httpPrefix,
@Parameter(required = true, description = "Mesos slave hostname") @PathParam("slaveHostname") String slaveHostname,
@Parameter(required = true, description = "Http port for Mesos slave") @PathParam("port") String httpPort,
@Parameter(required = true, description = "Full file path to file on Mesos slave to be downloaded") @PathParam("path") String fileFullPath
) {

Client client = ClientBuilder.newClient();
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we want to inject & use the AsyncHttpClient here instead of using a JAX-RS client, similarly to

Singularity/SingularityService/src/main/java/com/hubspot/singularity/data/SandboxManager.java

Lines 58 to 63 in c2d8871

Response response = asyncHttpClient
.prepareGet(String.format("http://%s:5051/files/browse", slaveHostname))
.setPerRequestConfig(timeoutConfig)
.addQueryParameter("path", fullPath)
.execute()
.get();
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah neat, will do. We already have one being injected too which is awesome since creating these clients are expensive operations.

String url = String.format("%s://%s:%s/files/download.json?path=%s",
httpPrefix, slaveHostname, httpPort, fileFullPath);
final InputStream responseStream = client.target(url).request().get(InputStream.class);

// Strip file path down to just a file name if we can
java.nio.file.Path filePath = Paths.get(fileFullPath).getFileName();
String fileName = filePath != null ? filePath.toString() : fileFullPath;

final String headerValue = String.format("attachment; filename=\"%s\"", fileName);
return Response.ok(responseStream).header("Content-Disposition", headerValue).build();
}

}
4 changes: 2 additions & 2 deletions SingularityUI/app/components/taskDetail/TaskDetail.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,9 @@ class TaskDetail extends Component {
file.uiPath = file.name;
}

file.fullPath = `${files.fullPathToRoot}/${files.currentDirectory}/${file.name}`;
file.downloadLink = `${httpPrefix}://${files.slaveHostname}:${httpPort}/files/download.json?path=${file.fullPath}`;
file.fullPath = encodeURIComponent(`${files.fullPathToRoot}/${files.currentDirectory}/${file.name}`);

file.downloadLink = `${config.apiRoot}/tasks/download/${httpPrefix}/${files.slaveHostname}/port/${httpPort}/path/${file.fullPath}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

The value of httpPrefix is based on the Singularity configuration, so we shouldn't need to send this as a parameter to the backend (since the backend also has access to that configuration and knows whether the slave communicates over HTTP or HTTPS and which port it's listening on).

file.isRecentlyModified = Date.now() / 1000 - file.mtime <= RECENTLY_MODIFIED_SECONDS;

if (!file.isDirectory) {
Expand Down