Repository: vantage6/vantage6
Issue: [Bug] When a node comes online, VPN IP address is not always updated

**Describe the bug**
A node that uses VPN gets a certain IP address to communicate over VPN. A node without VPN has no IP address registered.

When a node first connects with VPN, but later connects without VPN, the IP address that is stored in the database is still the (then incorrect) VPN IP address. This should be fixed.

Also, we should check if the VPN IP is always saved to the database at the right times. It should be updated when the VPN connects or disconnects.

Changed files:
vantage6-node/vantage6/node/socket.py, vantage6-node/vantage6/node/server_io.py, vantage6-node/vantage6/node/__init__.py, vantage6-server/vantage6/server/resource/node.py, vantage6-node/vantage6/node/docker/vpn_manager.py, vantage6-node/vantage6/node/docker/docker_manager.py, vantage6-node/vantage6/node/proxy_server.py

diff --git a/vantage6-node/vantage6/node/__init__.py b/vantage6-node/vantage6/node/__init__.py
index 9f5ca4b7..61192cf7 100644
--- a/vantage6-node/vantage6/node/__init__.py
+++ b/vantage6-node/vantage6/node/__init__.py
@@ -54,7 +54,7 @@ from vantage6.node.globals import (
     NODE_PROXY_SERVER_HOSTNAME, SLEEP_BTWN_NODE_LOGIN_TRIES,
     TIME_LIMIT_RETRY_CONNECT_NODE, TIME_LIMIT_INITIAL_CONNECTION_WEBSOCKET
 )
-from vantage6.node.server_io import NodeClient
+from vantage6.node.node_client import NodeClient
 from vantage6.node import proxy_server
 from vantage6.node.util import get_parent_id
 from vantage6.node.docker.docker_manager import DockerManager
@@ -108,13 +108,13 @@ class Node:
         self._using_encryption = None
 
         # initialize Node connection to the server
-        self.server_io = NodeClient(
+        self.client = NodeClient(
             host=self.config.get('server_url'),
             port=self.config.get('port'),
             path=self.config.get('api_path')
         )
 
-        self.log.info(f"Connecting server: {self.server_io.base_path}")
+        self.log.info(f"Connecting server: {self.client.base_path}")
 
         # Authenticate with the server, obtaining a JSON Web Token.
         # Note that self.authenticate() blocks until it succeeds.
@@ -160,7 +160,7 @@ class Node:
             isolated_network_mgr=isolated_network_mgr,
             vpn_manager=self.vpn_manager,
             tasks_dir=self.__tasks_dir,
-            client=self.server_io,
+            client=self.client,
             proxy=self.squid
         )
 
@@ -222,8 +222,8 @@ class Node:
         if debug_mode:
             self.log.debug("Debug mode enabled for proxy server")
             proxy_server.app.debug = True
-        proxy_server.app.config["SERVER_IO"] = self.server_io
-        proxy_server.server_url = self.server_io.base_path
+        proxy_server.app.config["SERVER_IO"] = self.client
+        proxy_server.server_url = self.client.base_path
 
         # set up proxy server logging
         log_level = getattr(logging, self.config["logging"]["level"].upper())
@@ -260,11 +260,10 @@ class Node:
 
     def sync_task_queue_with_server(self) -> None:
         """ Get all unprocessed tasks from the server for this node."""
-        assert self.server_io.cryptor, "Encrpytion has not been setup"
+        assert self.client.cryptor, "Encrpytion has not been setup"
 
         # request open tasks from the server
-        task_results = self.server_io.get_results(state="open",
-                                                  include_task=True)
+        task_results = self.client.get_results(state="open", include_task=True)
         self.log.debug(task_results)
 
         # add the tasks to the queue
@@ -282,7 +281,7 @@ class Node:
             Task identifier
         """
         # fetch (open) result for the node with the task_id
-        task_results = self.server_io.get_results(
+        task_results = self.client.get_results(
             include_task=True,
             state='open',
             task_id=task_id
@@ -328,9 +327,9 @@ class Node:
         self.log.info("Starting task {id} - {name}".format(**task))
 
         # notify that we are processing this task
-        self.server_io.set_task_start_time(taskresult["id"])
+        self.client.set_task_start_time(taskresult["id"])
 
-        token = self.server_io.request_token_for_container(
+        token = self.client.request_token_for_container(
             task["id"],
             task["image"]
         )
@@ -366,9 +365,7 @@ class Node:
             # (as the task is not started at all, unlike other crashes, it will
             # never finish and hence not be set to finished)
             update['finished_at'] = datetime.datetime.now().isoformat()
-        self.server_io.patch_results(
-            id_=taskresult['id'], result=update
-        )
+        self.client.patch_results(id_=taskresult['id'], result=update)
 
         # ensure that the /tasks namespace is connected. This may take a while
         # (usually < 5s) when the socket just (re)connected
@@ -387,12 +384,12 @@ class Node:
         self.socketIO.emit(
             'algorithm_status_change',
             data={
-                'node_id': self.server_io.whoami.id_,
+                'node_id': self.client.whoami.id_,
                 'status': task_status,
                 'result_id': taskresult['id'],
                 'task_id': task['id'],
-                'collaboration_id': self.server_io.collaboration_id,
-                'organization_id': self.server_io.whoami.organization_id,
+                'collaboration_id': self.client.collaboration_id,
+                'organization_id': self.client.whoami.organization_id,
                 'parent_id': get_parent_id(task),
             },
             namespace='/tasks',
@@ -402,21 +399,12 @@ class Node:
             # Save port of VPN client container at which it redirects traffic
             # to the algorithm container. First delete any existing port
             # assignments in case algorithm has crashed
-            self.server_io.request(
+            self.client.request(
                 'port', params={'result_id': taskresult['id']}, method="DELETE"
             )
             for port in vpn_ports:
                 port['result_id'] = taskresult['id']
-                self.server_io.request('port', method='POST', json=port)
-
-            # Save IP address of VPN container
-            # FIXME BvB 2023-02-21: node IP is now updated when task is started
-            # but this should be done when VPN connection is established
-            node_id = self.server_io.whoami.id_
-            node_ip = self.vpn_manager.get_vpn_ip()
-            self.server_io.request(
-                f"node/{node_id}", json={"ip": node_ip}, method="PATCH"
-            )
+                self.client.request('port', method='POST', json=port)
 
     def __listening_worker(self) -> None:
         """
@@ -458,13 +446,13 @@ class Node:
                 self.socketIO.emit(
                     'algorithm_status_change',
                     data={
-                        'node_id': self.server_io.whoami.id_,
+                        'node_id': self.client.whoami.id_,
                         'status': results.status,
                         'result_id': results.result_id,
                         'task_id': results.task_id,
-                        'collaboration_id': self.server_io.collaboration_id,
+                        'collaboration_id': self.client.collaboration_id,
                         'organization_id':
-                            self.server_io.whoami.organization_id,
+                            self.client.whoami.organization_id,
                         'parent_id': results.parent_id,
                     },
                     namespace='/tasks',
@@ -476,7 +464,7 @@ class Node:
                 # FIXME: why are we retrieving the result *again*? Shouldn't we
                 # just store the task_id when retrieving the task the first
                 # time?
-                response = self.server_io.request(
+                response = self.client.request(
                     f"result/{results.result_id}"
                 )
                 task_id = response.get("task").get("id")
@@ -488,7 +476,7 @@ class Node:
                     )
                     return
 
-                response = self.server_io.request(f"task/{task_id}")
+                response = self.client.request(f"task/{task_id}")
 
                 init_org_id = response.get("initiator")
                 if not init_org_id:
@@ -497,7 +485,7 @@ class Node:
                         " not be retrieved!"
                     )
 
-                self.server_io.patch_results(
+                self.client.patch_results(
                     id_=results.result_id,
                     result={
                         'result': results.data,
@@ -514,9 +502,9 @@ class Node:
         """ Print error message when node cannot find the server """
         self.log.warning(
             "Could not connect to the server. Retrying in 10 seconds")
-        if self.server_io.host == 'http://localhost' and running_in_docker():
+        if self.client.host == 'http://localhost' and running_in_docker():
             self.log.warn(
-                f"You are trying to reach the server at {self.server_io.host}."
+                f"You are trying to reach the server at {self.client.host}."
                 " As your node is running inside a Docker container, it cannot"
                 " reach localhost on your host system. Probably, you have to "
                 "change your serverl URL to http://host.docker.internal "
@@ -524,7 +512,7 @@ class Node:
             )
         else:
             self.log.debug("Are you sure the server can be reached at "
-                           f"{self.server_io.base_path}?")
+                           f"{self.client.base_path}?")
 
     def authenticate(self) -> None:
         """
@@ -540,7 +528,7 @@ class Node:
         while i < TIME_LIMIT_RETRY_CONNECT_NODE / SLEEP_BTWN_NODE_LOGIN_TRIES:
             i = i + 1
             try:
-                self.server_io.authenticate(api_key)
+                self.client.authenticate(api_key)
 
             except AuthenticationException as e:
                 msg = "Authentication failed: API key is wrong!"
@@ -563,13 +551,13 @@ class Node:
                 break
 
         if success:
-            self.log.info(f"Node name: {self.server_io.name}")
+            self.log.info(f"Node name: {self.client.name}")
         else:
             self.log.critical('Unable to authenticate. Exiting')
             exit(1)
 
         # start thread to keep the connection alive by refreshing the token
-        self.server_io.auto_refresh_token()
+        self.client.auto_refresh_token()
 
     def private_key_filename(self) -> Path:
         """Get the path to the private key."""
@@ -592,7 +580,7 @@ class Node:
 
     def setup_encryption(self) -> None:
         """ Setup encryption if the node is part of encrypted collaboration """
-        encrypted_collaboration = self.server_io.is_encrypted_collaboration()
+        encrypted_collaboration = self.client.is_encrypted_collaboration()
         encrypted_node = self.config['encryption']["enabled"]
 
         if encrypted_collaboration != encrypted_node:
@@ -602,11 +590,11 @@ class Node:
         if encrypted_collaboration:
             self.log.warn('Enabling encryption!')
             private_key_file = self.private_key_filename()
-            self.server_io.setup_encryption(private_key_file)
+            self.client.setup_encryption(private_key_file)
 
         else:
             self.log.warn('Disabling encryption!')
-            self.server_io.setup_encryption(None)
+            self.client.setup_encryption(None)
 
     def _set_task_dir(self, ctx) -> None:
         """
@@ -800,6 +788,7 @@ class Node:
         vpn_manager = VPNManager(
             isolated_network_mgr=isolated_network_mgr,
             node_name=self.ctx.name,
+            node_client=self.client,
             vpn_volume_name=vpn_volume_name,
             vpn_subnet=self.config.get('vpn_subnet'),
             alpine_image=custom_alpine or legacy_alpine,
@@ -843,7 +832,7 @@ class Node:
             next_mode = VPNConnectMode.REFRESH_KEYPAIR
         elif connect_mode == VPNConnectMode.REFRESH_KEYPAIR:
             self.log.debug("Refreshing VPN keypair...")
-            do_try = self.server_io.refresh_vpn_keypair(ovpn_file=ovpn_file)
+            do_try = self.client.refresh_vpn_keypair(ovpn_file=ovpn_file)
             next_mode = VPNConnectMode.REFRESH_COMPLETE
         elif connect_mode == VPNConnectMode.REFRESH_COMPLETE:
             self.log.debug("Requesting new VPN configuration file...")
@@ -876,7 +865,7 @@ class Node:
             Whether or not configuration file was successfully obtained
         """
         # get the ovpn configuration from the server
-        success, ovpn_config = self.server_io.get_vpn_config()
+        success, ovpn_config = self.client.get_vpn_config()
         if not success:
             self.log.warn("Obtaining VPN configuration file not successful!")
             self.log.warn("Disabling node-to-node communication via VPN")
@@ -912,8 +901,8 @@ class Node:
         NodeTaskNamespace.node_worker_ref = self
 
         self.socketIO.connect(
-            url=f'{self.server_io.host}:{self.server_io.port}',
-            headers=self.server_io.headers,
+            url=f'{self.client.host}:{self.client.port}',
+            headers=self.client.headers,
             wait=False
         )
 
@@ -928,8 +917,8 @@ class Node:
             time.sleep(1)
             i += 1
 
-        self.log.info(f'Connected to host={self.server_io.host} on port='
-                      f'{self.server_io.port}')
+        self.log.info(f'Connected to host={self.client.host} on port='
+                      f'{self.client.port}')
 
         self.log.debug("Starting thread for to ping the server to notify this"
                        " node is online.")
@@ -1006,13 +995,13 @@ class Node:
             List of dictionaries with information on killed task (keys:
             result_id, task_id and parent_id)
         """
-        if kill_info['collaboration_id'] != self.server_io.collaboration_id:
+        if kill_info['collaboration_id'] != self.client.collaboration_id:
             self.log.debug(
                 "Not killing tasks as this node is in another collaboration."
             )
             return []
         elif 'node_id' in kill_info and \\
-                kill_info['node_id'] != self.server_io.whoami.id_:
+                kill_info['node_id'] != self.client.whoami.id_:
             self.log.debug(
                 "Not killing tasks as instructions to kill tasks were directed"
                 " at another node in this collaboration.")
@@ -1021,11 +1010,11 @@ class Node:
         # kill specific task if specified, else kill all algorithms
         kill_list = kill_info.get('kill_list')
         killed_algos = self.__docker.kill_tasks(
-            org_id=self.server_io.whoami.organization_id, kill_list=kill_list
+            org_id=self.client.whoami.organization_id, kill_list=kill_list
         )
         # update status of killed tasks
         for killed_algo in killed_algos:
-            self.server_io.patch_results(
+            self.client.patch_results(
                 id_=killed_algo.result_id, result={'status': TaskStatus.KILLED}
             )
         return killed_algos
diff --git a/vantage6-node/vantage6/node/docker/docker_manager.py b/vantage6-node/vantage6/node/docker/docker_manager.py
index 2bed58c2..31e165ee 100644
--- a/vantage6-node/vantage6/node/docker/docker_manager.py
+++ b/vantage6-node/vantage6/node/docker/docker_manager.py
@@ -28,7 +28,7 @@ from vantage6.node.docker.docker_base import DockerBaseManager
 from vantage6.node.docker.vpn_manager import VPNManager
 from vantage6.node.docker.task_manager import DockerTaskManager
 from vantage6.node.docker.squid import Squid
-from vantage6.node.server_io import NodeClient
+from vantage6.node.node_client import NodeClient
 from vantage6.node.docker.exceptions import (
     UnknownAlgorithmStartFail,
     PermanentAlgorithmStartFail,
diff --git a/vantage6-node/vantage6/node/docker/vpn_manager.py b/vantage6-node/vantage6/node/docker/vpn_manager.py
index 7da1c72b..cdadf289 100644
--- a/vantage6-node/vantage6/node/docker/vpn_manager.py
+++ b/vantage6-node/vantage6/node/docker/vpn_manager.py
@@ -12,11 +12,12 @@ from vantage6.common.globals import APPNAME, VPN_CONFIG_FILE
 from vantage6.common.docker.addons import (
     remove_container_if_exists, remove_container, pull_if_newer
 )
+from vantage6.common.docker.network_manager import NetworkManager
 from vantage6.node.globals import (
     MAX_CHECK_VPN_ATTEMPTS, NETWORK_CONFIG_IMAGE, VPN_CLIENT_IMAGE,
     FREE_PORT_RANGE, DEFAULT_ALGO_VPN_PORT, ALPINE_IMAGE
 )
-from vantage6.common.docker.network_manager import NetworkManager
+from vantage6.node.node_client import NodeClient
 from vantage6.node.docker.docker_base import DockerBaseManager
 
 
@@ -28,8 +29,8 @@ class VPNManager(DockerBaseManager):
     log = logging.getLogger(logger_name(__name__))
 
     def __init__(self, isolated_network_mgr: NetworkManager,
-                 node_name: str, vpn_volume_name: str, vpn_subnet: str,
-                 alpine_image: str | None = None,
+                 node_name: str, node_client: NodeClient, vpn_volume_name: str,
+                 vpn_subnet: str, alpine_image: str | None = None,
                  vpn_client_image: str | None = None,
                  network_config_image: str | None = None) -> None:
         """
@@ -56,6 +57,7 @@ class VPNManager(DockerBaseManager):
 
         self.vpn_client_container_name = f'{APPNAME}-{node_name}-vpn-client'
         self.vpn_volume_name = vpn_volume_name
+        self.client = node_client
         self.subnet = vpn_subnet
         self.alpine_image = ALPINE_IMAGE if not alpine_image \\
             else alpine_image
@@ -138,6 +140,9 @@ class VPNManager(DockerBaseManager):
         else:
             raise ConnectionError("VPN connection not established!")
 
+        # send VPN IP address to server
+        self.send_vpn_ip_to_server()
+
         # check that the VPN connection IP address is part of the subnet
         # defined in the node configuration. If not, the VPN connection would
         # not work.
@@ -201,6 +206,9 @@ class VPNManager(DockerBaseManager):
         self.log.debug("Stopping and removing the VPN client container")
         remove_container(self.vpn_client_container, kill=True)
 
+        # clear VPN IP address from server
+        self.send_vpn_ip_to_server()
+
         # Clean up host network changes. We have added two rules to the front
         # of the DOCKER-USER chain. We now execute more or less the same
         # commands, but with -D (delete) instead of -I (insert)
@@ -246,6 +254,22 @@ class VPNManager(DockerBaseManager):
                 "Could not get VPN IP: VPN is not connected!")
         return vpn_interface[0]['addr_info'][0]['local']
 
+    def send_vpn_ip_to_server(self) -> None:
+        """
+        Send VPN IP address to the server
+        """
+        node_id = self.client.whoami.id_
+        if self.has_vpn:
+            node_ip = self.get_vpn_ip()
+            self.client.request(
+                f"node/{node_id}", json={"ip": node_ip}, method="PATCH"
+            )
+        else:
+            # VPN is disconnected, send NULL IP address
+            self.client.request(
+                f"node/{node_id}", json={"clear_ip": True}, method="PATCH"
+            )
+
     def forward_vpn_traffic(self, helper_container: Container,
                             algo_image_name: str) -> list[dict] | None:
         """
diff --git a/vantage6-node/vantage6/node/server_io.py b/vantage6-node/vantage6/node/node_client.py
similarity index 100%
rename from vantage6-node/vantage6/node/server_io.py
rename to vantage6-node/vantage6/node/node_client.py
diff --git a/vantage6-node/vantage6/node/proxy_server.py b/vantage6-node/vantage6/node/proxy_server.py
index 6710c0cf..afcc0351 100644
--- a/vantage6-node/vantage6/node/proxy_server.py
+++ b/vantage6-node/vantage6/node/proxy_server.py
@@ -15,7 +15,7 @@ from requests import Response
 from flask import Flask, request, jsonify
 
 from vantage6.common import bytes_to_base64s, base64s_to_bytes, logger_name
-from vantage6.node.server_io import NodeClient
+from vantage6.node.node_client import NodeClient
 
 # Initialize FLASK
 app = Flask(__name__)
@@ -150,13 +150,13 @@ def decrypt_result(result: dict) -> dict:
     dict
         Result dict with the `result` decrypted
     """
-    server_io: NodeClient = app.config.get('SERVER_IO')
+    client: NodeClient = app.config.get('SERVER_IO')
 
     # if the result is a None, there is no need to decrypt that..
     try:
         if result['result']:
             result["result"] = bytes_to_base64s(
-                server_io.cryptor.decrypt_str_to_bytes(
+                client.cryptor.decrypt_str_to_bytes(
                     result["result"]
                 )
             )
@@ -200,8 +200,8 @@ def proxy_task():
         Response from the vantage6 server
     """
     # We need the server io for the decryption of the results
-    server_io: NodeClient = app.config.get("SERVER_IO")
-    if not server_io:
+    client: NodeClient = app.config.get("SERVER_IO")
+    if not client:
         log.error("Task proxy request received but proxy server was not "
                   "initialized properly.")
         return jsonify({'msg': 'Proxy server not initialized properly'}), 500
@@ -252,8 +252,8 @@ def proxy_task():
         public_key = response.json().get("public_key")
 
         # Encrypt the input field
-        server_io: NodeClient = app.config.get("SERVER_IO")
-        organization["input"] = server_io.cryptor.encrypt_bytes_to_str(
+        client: NodeClient = app.config.get("SERVER_IO")
+        organization["input"] = client.cryptor.encrypt_bytes_to_str(
             base64s_to_bytes(input_),
             public_key
         )
@@ -262,7 +262,7 @@ def proxy_task():
                   f"{organization_id}!")
         return organization
 
-    if server_io.is_encrypted_collaboration():
+    if client.is_encrypted_collaboration():
 
         log.debug("Applying end-to-end encryption")
         data["organizations"] = [encrypt_input(o) for o in organizations]
@@ -294,8 +294,8 @@ def proxy_task_result(id: int) -> Response:
         Reponse from the vantage6 server
     """
     # We need the server io for the decryption of the results
-    server_io = app.config.get("SERVER_IO")
-    if not server_io:
+    client = app.config.get("SERVER_IO")
+    if not client:
         return jsonify({'msg': 'Proxy server not initialized properly'}),\\
             HTTPStatus.INTERNAL_SERVER_ERROR
 
@@ -335,8 +335,8 @@ def proxy_results(id: int) -> Response:
         Response of the vantage6 server
     """
     # We need the server io for the decryption of the results
-    server_io: NodeClient = app.config.get("SERVER_IO")
-    if not server_io:
+    client: NodeClient = app.config.get("SERVER_IO")
+    if not client:
         return {'msg': 'Proxy server not initialized properly'},\\
             HTTPStatus.INTERNAL_SERVER_ERROR
 
diff --git a/vantage6-node/vantage6/node/socket.py b/vantage6-node/vantage6/node/socket.py
index 6e5f0528..2ba46c49 100644
--- a/vantage6-node/vantage6/node/socket.py
+++ b/vantage6-node/vantage6/node/socket.py
@@ -93,7 +93,7 @@ class NodeTaskNamespace(ClientNamespace):
         self.log.warning("Your token is no longer valid... reconnecting")
         self.node_worker_ref.socketIO.disconnect()
         self.log.debug("Old socket connection terminated")
-        self.node_worker_ref.server_io.refresh_token()
+        self.node_worker_ref.client.refresh_token()
         self.log.debug("Token refreshed")
         self.node_worker_ref.connect_to_socket()
         self.log.debug("Connected to socket")
@@ -119,11 +119,11 @@ class NodeTaskNamespace(ClientNamespace):
                     'result_id': killed.result_id,
                     'task_id': killed.task_id,
                     'collaboration_id':
-                        self.node_worker_ref.server_io.collaboration_id,
-                    'node_id': self.node_worker_ref.server_io.whoami.id_,
+                        self.node_worker_ref.client.collaboration_id,
+                    'node_id': self.node_worker_ref.client.whoami.id_,
                     'status': TaskStatus.KILLED,
                     'organization_id':
-                        self.node_worker_ref.server_io.whoami.organization_id,
+                        self.node_worker_ref.client.whoami.organization_id,
                     'parent_id': killed.parent_id,
                 },
                 namespace='/tasks'
diff --git a/vantage6-server/vantage6/server/resource/node.py b/vantage6-server/vantage6/server/resource/node.py
index 2ad5c063..358c169f 100644
--- a/vantage6-server/vantage6/server/resource/node.py
+++ b/vantage6-server/vantage6/server/resource/node.py
@@ -487,6 +487,9 @@ class Node(NodeBase):
                   ip:
                     type: string
                     description: The node's VPN IP address
+                  clear_ip:
+                    type: boolean
+                    description: Clear the node's VPN IP address
 
         responses:
           200:
@@ -564,6 +567,8 @@ class Node(NodeBase):
         ip = data.get('ip')
         if ip:
             node.ip = ip
+        elif data.get('clear_ip'):
+            node.ip = None
 
         node.save()
         return node_schema.dump(node).data, HTTPStatus.OK