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

Jump to bottom

Return usable URL from remoteapprest app start #568

Merged
merged 9 commits into from
Sep 7, 2021
Merged

Return usable URL from remoteapprest app start #568

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
efac6e2
Extend remoteapprest to add startupdata option
Sep 3, 2021
5c2b79e
Change public container name to return usable URL
Sep 6, 2021
e1f5363
Revert unexpected changes
Sep 6, 2021
4c91d5a
Pycharm reformats code without consent
Sep 6, 2021
c09a8e0
Avoid starting the container if startupdata is provided but not allowed
Sep 6, 2021
2f7bc26
Fix url check and tests
Sep 6, 2021
1198470
Merge branch '565-extend-rest-api' into 567-fix-returned-url
Sep 6, 2021
c90c28c
Extend tests to check output URL
Sep 7, 2021
c163030
Extend _generate_container_url_id to accept an image name
Sep 7, 2021
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
33 changes: 28 additions & 5 deletions remoteappmanager/cli/remoteapprest/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import requests
import requests.utils
import json
from urllib.parse import urljoin
from urllib.parse import urljoin, urlsplit

import click

Expand Down Expand Up @@ -137,24 +137,47 @@ def available(ctx):

@app.command()
@click.argument("identifier")
@click.option("--startupdata", default=None)
@click.pass_context
def start(ctx, identifier):
def start(ctx, identifier, startupdata):
"""Starts a container for application identified by IDENTIFIER.
If ``startupdata`` is provided, the container will open the given file at
startup.
If a container is already running, restarts it."""
cred = ctx.obj.credentials
url, username, cookies = cred.url, cred.username, cred.cookies

request_url = urljoin(url,
"/user/{}/api/v1/containers/".format(username))

payload = json.dumps(dict(
mapping_id=identifier
))
payload_dict = dict(mapping_id=identifier)
if startupdata is not None:
# First make sure that the allow_startup_data policy is True
check_url = urljoin(url,
"/user/{}/api/v1/applications/".format(username))
response = requests.get(check_url, cookies=cookies, verify=False)
apps_data = json.loads(response.content.decode("utf-8"))
app_data = apps_data["items"][identifier]
allow_startup_data = app_data["image"]["policy"]["allow_startup_data"]
if allow_startup_data:
payload_dict.update(
configurables=dict(startupdata=dict(startupdata=startupdata)))
else:
raise click.ClickException(
"The 'allow_startup_data' policy is False for the current "
"user.\nExiting.")

payload = json.dumps(payload_dict)

response = requests.post(request_url, payload, cookies=cookies,
verify=False)
if response.status_code == 201:
location = response.headers["Location"]
parsed = urlsplit(location)
path, port = parsed.path.split("_")
port = port.rstrip("/")
path = "".join(path.split("/api/v1"))
location = f"{parsed.scheme}://{parsed.hostname}:{port}{path}"
print(location)
flongford marked this conversation as resolved.
Show resolved Hide resolved


Expand Down
80 changes: 80 additions & 0 deletions remoteappmanager/cli/tests/test_remoteapprest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,17 +130,97 @@ def test_app_running(self):

def test_app_start(self):
with mock.patch('requests.post') as mock_post, \
mock.patch("requests.get") as mock_get, \
mock.patch("remoteappmanager.cli.remoteapprest.__main__."
"Credentials.from_file") as mock_from_file:

mock_from_file.return_value = self.get_mock_credentials()

response = [mock.Mock()]
response[0].content = json.dumps(
{'items': {'1': {"image": {
"policy": {"allow_startup_data": True}
}}}}
).encode("utf-8")

mock_get.side_effect = response

self._remoteapprest("app start 1")
self.assertEqual(mock_post.call_args[0][0],
"/user/bar/api/v1/containers/")
self.assertEqual(mock_post.call_args[0][1],
json.dumps({"mapping_id": "1"}))

self._remoteapprest("app start 1 --startupdata=/test")
self.assertEqual(mock_post.call_args[0][0],
"/user/bar/api/v1/containers/")
self.assertEqual(
mock_post.call_args[0][1],
json.dumps(
{"mapping_id": "1",
"configurables": {
"startupdata": {"startupdata": "/test"}
}}
))

def test_app_start_allow_startup_data(self):
with mock.patch("requests.get") as mock_get, \
mock.patch("remoteappmanager.cli.remoteapprest.__main__."
"Credentials.from_file") as mock_from_file:

mock_from_file.return_value = self.get_mock_credentials()

# allow_startup_data is False, so this will fail

response = [mock.Mock()]
response[0].content = json.dumps(
{'items': {'1': {"image": {
"policy": {"allow_startup_data": False}
}}}}
).encode("utf-8")

mock_get.side_effect = response

argstring = "app start 1 --startupdata=/test"
runner = CliRunner()
result = runner.invoke(remoteapprest.cli,
argstring.split(),
catch_exceptions=True)

self.assertEqual(result.exit_code, 1)
self.assertIn("The 'allow_startup_data' policy is False for the "
"current user",
result.output)

# Removing the --startupdata option should work just fine

argstring = "app start 1"
runner = CliRunner()
result = runner.invoke(remoteapprest.cli,
argstring.split(),
catch_exceptions=True)

self.assertEqual(result.exit_code, -1)

# Same if the allow_startup_data policy is set to True

response = [mock.Mock()]
response[0].content = json.dumps(
{'items': {'1': {"image": {
"policy": {"allow_startup_data": True}
}}}}
).encode("utf-8")

mock_get.side_effect = response

argstring = "app start 1 --startupdata=/test"
runner = CliRunner()
result = runner.invoke(remoteapprest.cli,
argstring.split(),
catch_exceptions=True)

self.assertEqual(result.exit_code, -1)

def test_app_stop(self):
with mock.patch('requests.delete') as mock_delete, \
mock.patch("remoteappmanager.cli.remoteapprest.__main__."
Expand Down
6 changes: 4 additions & 2 deletions remoteappmanager/docker/container_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

container_urlpath = without_end_slash(
url_path_join(base_urlpath,
"containers",
Expand Down Expand Up @@ -413,6 +413,8 @@ def _start_container(self,
yield self.stop_and_remove_container(container_id)
raise e

extended_id = f"{container_url_id}_{port}"

container = Container(
docker_id=container_id,
name=container_name,
Expand All @@ -421,7 +423,7 @@ def _start_container(self,
mapping_id=mapping_id,
ip=ip,
port=port,
url_id=container_url_id,
url_id=extended_id,
urlpath=container_urlpath,
)

Expand Down