From efac6e2e0b5471aa6897b80aa657429184f1f85a Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Fri, 3 Sep 2021 11:51:24 +0100 Subject: [PATCH 1/8] Extend remoteapprest to add startupdata option --- remoteappmanager/cli/remoteapprest/__main__.py | 14 ++++++++++---- remoteappmanager/cli/tests/test_remoteapprest.py | 12 ++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/remoteappmanager/cli/remoteapprest/__main__.py b/remoteappmanager/cli/remoteapprest/__main__.py index 21ad2ecd9..fda785a5f 100644 --- a/remoteappmanager/cli/remoteapprest/__main__.py +++ b/remoteappmanager/cli/remoteapprest/__main__.py @@ -137,9 +137,12 @@ 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 @@ -147,9 +150,12 @@ def start(ctx, identifier): 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: + payload_dict.update( + configurables=dict(startupdata=dict(startupdata=startupdata))) + + payload = json.dumps(payload_dict) response = requests.post(request_url, payload, cookies=cookies, verify=False) diff --git a/remoteappmanager/cli/tests/test_remoteapprest.py b/remoteappmanager/cli/tests/test_remoteapprest.py index 611d21cc8..8f2e7e1e3 100644 --- a/remoteappmanager/cli/tests/test_remoteapprest.py +++ b/remoteappmanager/cli/tests/test_remoteapprest.py @@ -141,6 +141,18 @@ def test_app_start(self): 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_stop(self): with mock.patch('requests.delete') as mock_delete, \ mock.patch("remoteappmanager.cli.remoteapprest.__main__." From 5c2b79ed26f854ae59912da74c8d6b110305b63b Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Mon, 6 Sep 2021 11:43:15 +0100 Subject: [PATCH 2/8] Change public container name to return usable URL --- .../cli/remoteapprest/__main__.py | 7 ++++- remoteappmanager/docker/container_manager.py | 27 +++++++------------ 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/remoteappmanager/cli/remoteapprest/__main__.py b/remoteappmanager/cli/remoteapprest/__main__.py index fda785a5f..ec804d0e2 100644 --- a/remoteappmanager/cli/remoteapprest/__main__.py +++ b/remoteappmanager/cli/remoteapprest/__main__.py @@ -4,7 +4,7 @@ import requests import requests.utils import json -from urllib.parse import urljoin +from urllib.parse import urljoin, urlsplit import click @@ -161,6 +161,11 @@ def start(ctx, identifier, startupdata): 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) diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py index f785cc35b..544bea61c 100644 --- a/remoteappmanager/docker/container_manager.py +++ b/remoteappmanager/docker/container_manager.py @@ -349,11 +349,9 @@ 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] container_urlpath = without_end_slash( - url_path_join(base_urlpath, - "containers", - container_url_id)) + url_path_join(base_urlpath, "containers", container_url_id)) container_name = _generate_container_name(self.realm, user_name, mapping_id) @@ -407,23 +405,16 @@ def _start_container(self, try: ip, port = yield from self._get_ip_and_port(container_id) except Exception as e: - self.log.exception( - "Could not retrieve ip/port information " - "for container {}".format(container_id)) + self.log.exception("Could not retrieve ip/port information " + "for container {}".format(container_id)) yield self.stop_and_remove_container(container_id) raise e - container = Container( - docker_id=container_id, - name=container_name, - image_name=image_name, - image_id=image_id, - mapping_id=mapping_id, - ip=ip, - port=port, - url_id=container_url_id, - urlpath=container_urlpath, - ) + extended_id = f"{container_url_id}_{port}" + + container = Container(docker_id=container_id, name=container_name, image_name=image_name, + image_id=image_id, mapping_id=mapping_id, ip=ip, port=port, url_id=extended_id, + urlpath=container_urlpath, ) self.log.info( ("Started container '{}' (id: {}). " From e1f536380501f2a0fcbb124d46146bc79bdc2925 Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Mon, 6 Sep 2021 11:48:28 +0100 Subject: [PATCH 3/8] Revert unexpected changes --- remoteappmanager/docker/container_manager.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py index 544bea61c..a5ff71af7 100644 --- a/remoteappmanager/docker/container_manager.py +++ b/remoteappmanager/docker/container_manager.py @@ -416,15 +416,9 @@ def _start_container(self, image_id=image_id, mapping_id=mapping_id, ip=ip, port=port, url_id=extended_id, urlpath=container_urlpath, ) - self.log.info( - ("Started container '{}' (id: {}). " - "Exported port reachable at {}:{}").format( - container_name, - container_id, - ip, - port - ) - ) + self.log.info(("Started container '{}' (id: {}). " + "Exported port reachable at {}:{}").format(container_name, container_id, ip, + port)) return container From 4c91d5aca7e11537673d3da951b44de24581b31c Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Mon, 6 Sep 2021 11:49:18 +0100 Subject: [PATCH 4/8] Pycharm reformats code without consent --- remoteappmanager/docker/container_manager.py | 35 +++++++++++++++----- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py index a5ff71af7..b70edf383 100644 --- a/remoteappmanager/docker/container_manager.py +++ b/remoteappmanager/docker/container_manager.py @@ -351,7 +351,9 @@ def _start_container(self, container_url_id = image_name.split("/")[-1] container_urlpath = without_end_slash( - url_path_join(base_urlpath, "containers", container_url_id)) + url_path_join(base_urlpath, + "containers", + container_url_id)) container_name = _generate_container_name(self.realm, user_name, mapping_id) @@ -405,20 +407,35 @@ def _start_container(self, try: ip, port = yield from self._get_ip_and_port(container_id) except Exception as e: - self.log.exception("Could not retrieve ip/port information " - "for container {}".format(container_id)) + self.log.exception( + "Could not retrieve ip/port information " + "for container {}".format(container_id)) 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, image_name=image_name, - image_id=image_id, mapping_id=mapping_id, ip=ip, port=port, url_id=extended_id, - urlpath=container_urlpath, ) + container = Container( + docker_id=container_id, + name=container_name, + image_name=image_name, + image_id=image_id, + mapping_id=mapping_id, + ip=ip, + port=port, + url_id=extended_id, + urlpath=container_urlpath, + ) - self.log.info(("Started container '{}' (id: {}). " - "Exported port reachable at {}:{}").format(container_name, container_id, ip, - port)) + self.log.info( + ("Started container '{}' (id: {}). " + "Exported port reachable at {}:{}").format( + container_name, + container_id, + ip, + port + ) + ) return container From c09a8e049066d8a9fa40d4ccd5a3b4674e9160b7 Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Mon, 6 Sep 2021 14:16:54 +0100 Subject: [PATCH 5/8] Avoid starting the container if startupdata is provided but not allowed --- .../cli/remoteapprest/__main__.py | 16 ++++- .../cli/tests/test_remoteapprest.py | 58 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/remoteappmanager/cli/remoteapprest/__main__.py b/remoteappmanager/cli/remoteapprest/__main__.py index fda785a5f..535014447 100644 --- a/remoteappmanager/cli/remoteapprest/__main__.py +++ b/remoteappmanager/cli/remoteapprest/__main__.py @@ -152,8 +152,20 @@ def start(ctx, identifier, startupdata): payload_dict = dict(mapping_id=identifier) if startupdata is not None: - payload_dict.update( - configurables=dict(startupdata=dict(startupdata=startupdata))) + # First make sure that the allow_startup_data policy is True + request_url = urljoin(url, + "/user/{}/api/v1/applications/".format(username)) + response = requests.get(request_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) diff --git a/remoteappmanager/cli/tests/test_remoteapprest.py b/remoteappmanager/cli/tests/test_remoteapprest.py index 8f2e7e1e3..8af4e7647 100644 --- a/remoteappmanager/cli/tests/test_remoteapprest.py +++ b/remoteappmanager/cli/tests/test_remoteapprest.py @@ -153,6 +153,64 @@ def test_app_start(self): }} )) + 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__." From 2f7bc268c6d71b670ef6f8a866f3e68c7554a764 Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Mon, 6 Sep 2021 14:30:03 +0100 Subject: [PATCH 6/8] Fix url check and tests --- remoteappmanager/cli/remoteapprest/__main__.py | 6 +++--- remoteappmanager/cli/tests/test_remoteapprest.py | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/remoteappmanager/cli/remoteapprest/__main__.py b/remoteappmanager/cli/remoteapprest/__main__.py index 535014447..260a1b401 100644 --- a/remoteappmanager/cli/remoteapprest/__main__.py +++ b/remoteappmanager/cli/remoteapprest/__main__.py @@ -153,9 +153,9 @@ def start(ctx, identifier, startupdata): payload_dict = dict(mapping_id=identifier) if startupdata is not None: # First make sure that the allow_startup_data policy is True - request_url = urljoin(url, - "/user/{}/api/v1/applications/".format(username)) - response = requests.get(request_url, cookies=cookies, verify=False) + 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"] diff --git a/remoteappmanager/cli/tests/test_remoteapprest.py b/remoteappmanager/cli/tests/test_remoteapprest.py index 8af4e7647..2ea23f7a9 100644 --- a/remoteappmanager/cli/tests/test_remoteapprest.py +++ b/remoteappmanager/cli/tests/test_remoteapprest.py @@ -130,11 +130,21 @@ 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/") From c90c28c5c6de19a91d977b03b0c8be44ef9ecf33 Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Tue, 7 Sep 2021 10:15:53 +0100 Subject: [PATCH 7/8] Extend tests to check output URL --- .../cli/tests/test_remoteapprest.py | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/remoteappmanager/cli/tests/test_remoteapprest.py b/remoteappmanager/cli/tests/test_remoteapprest.py index 2ea23f7a9..d39f23038 100644 --- a/remoteappmanager/cli/tests/test_remoteapprest.py +++ b/remoteappmanager/cli/tests/test_remoteapprest.py @@ -144,12 +144,20 @@ def test_app_start(self): ).encode("utf-8") mock_get.side_effect = response + mock_post.return_value.status_code = 201 + mock_post.return_value.headers = { + "Location": "http://127.0.0.1:8000" + "/user/bar/api/v1/containers/test_49165" + } - self._remoteapprest("app start 1") + _, output = 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.assertEqual( + output.strip(), + "http://127.0.0.1:49165/user/bar/containers/test") self._remoteapprest("app start 1 --startupdata=/test") self.assertEqual(mock_post.call_args[0][0], @@ -162,9 +170,13 @@ def test_app_start(self): "startupdata": {"startupdata": "/test"} }} )) + self.assertEqual( + output.strip(), + "http://127.0.0.1:49165/user/bar/containers/test") def test_app_start_allow_startup_data(self): - with mock.patch("requests.get") as mock_get, \ + 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: @@ -180,6 +192,10 @@ def test_app_start_allow_startup_data(self): ).encode("utf-8") mock_get.side_effect = response + mock_post.return_value.status_code = 201 + mock_post.return_value.headers = { + "Location": "http://127.0.0.1:8000" + "/user/bar/api/v1/containers/test_49165"} argstring = "app start 1 --startupdata=/test" runner = CliRunner() @@ -200,7 +216,10 @@ def test_app_start_allow_startup_data(self): argstring.split(), catch_exceptions=True) - self.assertEqual(result.exit_code, -1) + self.assertEqual(result.exit_code, 0) + self.assertEqual( + result.output.strip(), + "http://127.0.0.1:49165/user/bar/containers/test") # Same if the allow_startup_data policy is set to True @@ -219,7 +238,10 @@ def test_app_start_allow_startup_data(self): argstring.split(), catch_exceptions=True) - self.assertEqual(result.exit_code, -1) + self.assertEqual(result.exit_code, 0) + self.assertEqual( + result.output.strip(), + "http://127.0.0.1:49165/user/bar/containers/test") def test_app_stop(self): with mock.patch('requests.delete') as mock_delete, \ From c16303094629759a229320c0c1ec505b79a64a88 Mon Sep 17 00:00:00 2001 From: Roberto Preste Date: Tue, 7 Sep 2021 10:23:37 +0100 Subject: [PATCH 8/8] Extend _generate_container_url_id to accept an image name --- remoteappmanager/docker/container_manager.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py index b70edf383..6586b08ca 100644 --- a/remoteappmanager/docker/container_manager.py +++ b/remoteappmanager/docker/container_manager.py @@ -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 = image_name.split("/")[-1] + container_url_id = _generate_container_url_id(image_name) container_urlpath = without_end_slash( url_path_join(base_urlpath, "containers", @@ -693,8 +693,22 @@ def _generate_container_name(realm, user_name, mapping_id): ) -def _generate_container_url_id(): - """Generates a unique string to identify the container through a url""" +def _generate_container_url_id(image_name=None): + """ If a complete image name is provided, parses and returns the image + name, otherwise generates a unique string to identify the container. + + Parameters + ---------- + image_name : str, default None + The image name, in the format simphony-remote/simphony-application. + + Returns + ------- + str + The container identifier. + """ + if image_name: + return image_name.split("/")[-1] return uuid.uuid4().hex