Proxy download from Mesos slave over Singularity. #1817
Conversation
| @Path("/download/{httpPrefix}/{slaveHostname}/port/{port}/path/{path}") | ||
| @Produces(MediaType.APPLICATION_OCTET_STREAM) | ||
| @Operation(summary = "Proxy a file download from a Mesos Slave through Singularity") | ||
| @Consumes(MediaType.APPLICATION_FORM_URLENCODED) |
There was a problem hiding this comment.
Since this is a GET, it shouldn't consume anything.
There was a problem hiding this comment.
-- whoops -- left this there from when I had it as a POST
| @Parameter(required = true, description = "Full file path to file on Mesos slave to be downloaded") @PathParam("path") String fileFullPath | ||
| ) { | ||
|
|
||
| Client client = ClientBuilder.newClient(); |
There was a problem hiding this comment.
I believe we want to inject & use the AsyncHttpClient here instead of using a JAX-RS client, similarly to
There was a problem hiding this comment.
Ah neat, will do. We already have one being injected too which is awesome since creating these clients are expensive operations.
|
|
||
| file.downloadLink = `${config.apiRoot}/tasks/download/${httpPrefix}/${files.slaveHostname}/port/${httpPort}/path/${file.fullPath}`; |
There was a problem hiding this comment.
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).
| @@ -609,4 +613,30 @@ private SingularityTaskShellCommandRequest startShellCommand(SingularityTaskId t | |||
| SingularityTaskId taskIdObj = getTaskIdFromStr(taskId); | |||
| return taskManager.getTaskShellCommandUpdates(new SingularityTaskShellCommandRequestId(taskIdObj, commandName, commandTimestamp)); | |||
| } | |||
|
|
|||
| @GET | |||
| @Path("/download/{httpPrefix}/{slaveHostname}/port/{port}/path/{path}") | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
… passing params that were in Mesos config.
|
This was merged into |
|
🚢 |
Solves problem of being on different network than Mesos slave. Directly proxying through a configurable load balancer can still be looked into.