Return usable URL from remoteapprest app start #568
Conversation
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
==========================================
- Coverage 95.16% 95.07% -0.09%
==========================================
Files 92 92
Lines 4222 4267 +45
Branches 271 273 +2
==========================================
+ Hits 4018 4057 +39
- Misses 144 150 +6
Partials 60 60
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks @robertopreste - I have a question about the potential implications of changing the container ID, but otherwise LGTM.
Just to clarify, is this PR currently is preferred over #567?
| @@ -349,7 +349,7 @@ def _start_container(self, | |||
| '\n'.join('{0} -> {1}'.format(source, target['bind']) | |||
| for source, target in filtered_volumes.items())) | |||
|
|
|||
| container_url_id = _generate_container_url_id() | |||
| container_url_id = image_name.split("/")[-1] | |||
There was a problem hiding this comment.
I am wondering if there are either any security or practical implications of this change, since the container url ID is no longer a random or unique string.
Are there any best practices for Docker container url ids? I couldn't find much information online. Maybe we can just review any potential information that we are exposing here
There was a problem hiding this comment.
I agree, however I don't think we're exposing any sensible data, being it simply the name of the image. Also, it shouldn't pose a problem practically (e.g. name conflicts with other containers), since we run only one container (for a given image) at a time.
I haven't found anything specific about container IDs, so I wouldn't know about best practices, I'm afraid.
There was a problem hiding this comment.
Yes, that makes sense.
To tidy things up, could we extend the _generate_container_url_id to take a image_name keyword argument that is None by default? If the image_name is then provided, then we parse it, if not we just return a random string.
Closes #567
[Includes changes from #566, changes will clean once that PR is merged]
Passing the right port to the response parsed by
remoteapprest app startseems to be quite convoluted, so here is an approach which is a bit of a cheating, but it works and doesn't affect any other functionality.The container name is changed to include the right host port (e.g. "/simphony-remote_49165"), however this new name is not used internally (in fact it doesn't show in the logs); the container URL is still e.g. "http://127.0.0.1:49165/user/rpreste/containers/simphony-paraview/", because it is generated earlier in the process.
The new alternative URL is not functional, but it is sent in the response to the
remoteapprest app startcall, so it can then be parsed and rearranged to return the correct URL.