[req-changes] Added first review changes

- Removed Zerotier read-only fields from the schema and utilized
only the 'network_id' as the system-defined variable for the Zerotier network.
- Fixed the issue of unwanted execution of "unsaved-changes.js" caused by
the different order of configuration received from the API and the configuration defined in the VPN backend.
- Enabled OpenWISP to control IP assignment using the subnet and IP fields of the VPN model.
- Moved API requests for delete and update operations for Zerotier
to background celery tasks, while the create operation remains synchronous.
- Implemented basic error handling for Zerotier service API calls
(further improvements will be made in the next review).
- Updated existing migrations to include updated help text.
- Fixed JS to hide unnecessary fields.

Author: Aryamanz29

openwisp_controller/config/api/zerotier_service.py

@@ -14,9 +14,11 @@ def _get_endpoint(self, property, operation, id):
         }
         return _API_ENDPOINTS.get(property).get(operation)
 
-    def __init__(self, host, token):
+    def __init__(self, host, token, subnet='', ip=''):
         self.host = host
         self.token = token
+        self.subnet = subnet and subnet.subnet
+        self.ip_address = ip and ip.ip_address
         self.url = f'http://{host}'
         self.headers = {
             'X-ZT1-Auth': self.token,
@@ -32,19 +34,41 @@ def _get_repsonse(self, repsonse):
             'clientId',
             'rulesSource',
             'ssoEnabled',
+            'creationTime',
+            'name',
+            'nwid',
+            'objtype',
+            'revision',
+            'routes',
+            'ipAssignmentPools',
         ]
         for field in _redundant_fields:
             if field in repsonse.keys():
                 del repsonse[field]
         return repsonse
 
+    def _add_routes_and_ip_assignment(self, config):
+        config['routes'] = [{'target': str(self.subnet), 'via': self.ip_address}]
+        try:
+            ip_end = str(self.subnet[-2])
+            ip_start = str(self.subnet[1])
+        # In case of prefix length 32 (ipv4)
+        # or 128 (ipv6) only single host is available
+        except IndexError:
+            ip_end = str(self.subnet[0])
+            ip_start = str(self.subnet[0])
+
+        config['ipAssignmentPools'] = [{"ipRangeEnd": ip_end, "ipRangeStart": ip_start}]
+        return config
+
     def create_network(self, config):
         # Call /status first to obtain
         # the `node_id` of the controller
         url = f'{self.url}/status'
         response = requests.get(url, headers=self.headers)
         node_id = response.json()['address']
         url = f"{self.url}{self._get_endpoint('network', 'create', node_id)}"
+        config = self._add_routes_and_ip_assignment(config)
         response = requests.post(url, json=config, headers=self.headers, timeout=5)
         if response.status_code != 200:
             raise ValidationError(
@@ -56,29 +80,13 @@ def create_network(self, config):
             )
         return self._get_repsonse(response.json())
 
-    def update_network(self, config):
-        network_id = config.get('id')
+    def update_network(self, config, network_id):
         url = f"{self.url}{self._get_endpoint('network', 'update', network_id)}"
+        config = self._add_routes_and_ip_assignment(config)
         response = requests.post(url, json=config, headers=self.headers, timeout=5)
-        if response.status_code != 200:
-            raise ValidationError(
-                {
-                    'ZerotierServiceAPI create network error': (
-                        f'({response.status_code}) {response.reason}'
-                    )
-                }
-            )
-        return self._get_repsonse(response.json())
+        return response, self._get_repsonse(response.json())
 
     def delete_network(self, network_id):
         url = f"{self.url}{self._get_endpoint('network', 'delete', network_id)}"
-        print(url)
         response = requests.delete(url, headers=self.headers)
-        if response.status_code not in (200, 404):
-            raise ValidationError(
-                {
-                    'ZerotierServiceAPI delete network error': (
-                        f'({response.status_code}) {response.reason}'
-                    )
-                }
-            )
+        return response

openwisp_controller/config/base/vpn.py

@@ -3,6 +3,7 @@
 import json
 import logging
 import subprocess
+from copy import deepcopy
 
 import shortuuid
 from cache_memoize import cache_memoize
@@ -20,7 +21,12 @@
 from .. import settings as app_settings
 from ..api.zerotier_service import ZerotierService
 from ..signals import vpn_peers_changed, vpn_server_modified
-from ..tasks import create_vpn_dh, trigger_vpn_server_endpoint
+from ..tasks import (
+    create_vpn_dh,
+    trigger_vpn_server_endpoint,
+    trigger_zerotier_server_delete,
+    trigger_zerotier_server_update,
+)
 from .base import BaseConfig
 
 logger = logging.getLogger(__name__)
@@ -118,6 +124,8 @@ class AbstractVpn(ShareableOrgMixinUniqueName, BaseConfig):
     # needed for wireguard
     public_key = models.CharField(blank=True, max_length=44)
     private_key = models.CharField(blank=True, max_length=44)
+    # needed for zerotier
+    network_id = models.CharField(blank=True, max_length=16)
 
     __vpn__ = True
 
@@ -144,6 +152,7 @@ def clean(self, *args, **kwargs):
         self._validate_org_relation('cert')
         self._validate_org_relation('subnet')
         self._validate_subnet_ip()
+        self._validate_authtoken()
 
     def _validate_backend(self):
         if self._state.adding:
@@ -181,7 +190,7 @@ def _validate_keys(self):
             self.private_key = ''
 
     def _validate_subnet_ip(self):
-        if self._is_backend_type('wireguard'):
+        if self._is_backend_type('wireguard') or self._is_backend_type('zerotier'):
             if not self.subnet:
                 raise ValidationError(
                     {'subnet': _('Subnet is required for this VPN backend.')}
@@ -191,6 +200,11 @@ def _validate_subnet_ip(self):
                     {'ip': _('VPN IP address must be within the VPN subnet')}
                 )
 
+    def _validate_authtoken(self):
+        if self._is_backend_type('zerotier'):
+            if not self.auth_token:
+                raise ValidationError({'auth_token': _('Auth token is required')})
+
     def save(self, *args, **kwargs):
         """
         Calls _auto_create_cert() if cert is not set
@@ -199,33 +213,6 @@ def save(self, *args, **kwargs):
         if not created:
             self._check_changes()
         create_dh = False
-        if self._is_backend_type('zerotier'):
-            if not self.auth_token:
-                raise ValidationError(
-                    {'auth_token': _('Zerotier auth token is required')}
-                )
-
-            # The network name is similar
-            # to the VPN backend's name field
-            zerotier_config = self.config.get('zerotier')[0]
-            zerotier_config['name'] = self.name
-
-            if zerotier_config and not zerotier_config.get('id'):
-                response = ZerotierService(self.host, self.auth_token).create_network(
-                    self.config.get('zerotier')[0]
-                )
-                # Append newly created zerotier
-                # controller network configuration
-                self.config = {**self.config, 'zerotier': [response]}
-
-            # When zerotier network already exists
-            if zerotier_config and zerotier_config.get('id'):
-                response = ZerotierService(self.host, self.auth_token).update_network(
-                    self.config.get('zerotier')[0]
-                )
-                # update the existing network configuration
-                self.config = {**self.config, 'zerotier': [response]}
-
         if not self.cert and self.ca:
             self.cert = self._auto_create_cert()
         if self._is_backend_type('openvpn') and not self.dh:
@@ -235,6 +222,13 @@ def save(self, *args, **kwargs):
             self._generate_wireguard_keys()
         if self.subnet and not self.ip:
             self.ip = self._auto_create_ip()
+        if self._is_backend_type('zerotier'):
+            config = deepcopy(self.config['zerotier'][0])
+            config['name'] = self.name
+            if created:
+                self._create_zerotier_server(config)
+            else:
+                self._update_zerotier_server(config)
         super().save(*args, **kwargs)
         if create_dh:
             transaction.on_commit(lambda: create_vpn_dh.delay(self.id))
@@ -256,6 +250,7 @@ def _check_changes(self):
             'dh',
             'public_key',
             'private_key',
+            'network_id',
         ]
         current = self._meta.model.objects.only(*attrs).get(pk=self.pk)
         for attr in attrs:
@@ -284,25 +279,53 @@ class method for ``post_delete`` signal
         """
         if not instance._is_backend_type('zerotier'):
             return
-        network_id = instance.config.get('zerotier')[0].get('id')
-        ZerotierService(instance.host, instance.auth_token).delete_network(network_id)
+        transaction.on_commit(
+            lambda: trigger_zerotier_server_delete.delay(
+                host=instance.host,
+                auth_token=instance.auth_token,
+                network_id=instance.network_id,
+                vpn_id=instance.pk,
+            )
+        )
+
+    def _create_zerotier_server(self, config):
+        server_config = ZerotierService(
+            self.host, self.auth_token, self.subnet, self.ip
+        ).create_network(config)
+        self.network_id = server_config.pop('id', None)
+        self.config = {**self.config, 'zerotier': [server_config]}
+
+    def _update_zerotier_server(self, config):
+        config = config or {}
+        if config and self._is_backend_type('zerotier'):
+            if self.host and self.auth_token:
+                transaction.on_commit(
+                    lambda: trigger_zerotier_server_update.delay(
+                        config=config,
+                        vpn_id=self.pk,
+                    )
+                )
+            else:
+                logger.info(
+                    f'Cannot update configuration of {self.name}'
+                    'Zerotier VPN server, host and authentication token are empty.'
+                )
 
     def update_vpn_server_configuration(instance, **kwargs):
-        if not instance._is_backend_type('wireguard'):
-            return
-        if instance.webhook_endpoint and instance.auth_token:
-            transaction.on_commit(
-                lambda: trigger_vpn_server_endpoint.delay(
-                    endpoint=instance.webhook_endpoint,
-                    auth_token=instance.auth_token,
-                    vpn_id=instance.pk,
+        if instance._is_backend_type('wireguard'):
+            if instance.webhook_endpoint and instance.auth_token:
+                transaction.on_commit(
+                    lambda: trigger_vpn_server_endpoint.delay(
+                        endpoint=instance.webhook_endpoint,
+                        auth_token=instance.auth_token,
+                        vpn_id=instance.pk,
+                    )
+                )
+            else:
+                logger.info(
+                    f'Cannot update configuration of {instance.name} VPN server, '
+                    'webhook endpoint and authentication token are empty.'
                 )
-            )
-        else:
-            logger.info(
-                f'Cannot update configuration of {instance.name} VPN server, '
-                'webhook endpoint and authentication token are empty.'
-            )
 
     def _auto_create_cert(self):
         """
@@ -360,6 +383,8 @@ def get_context(self):
             c['subnet_prefixlen'] = str(self.subnet.subnet.prefixlen)
         if self.ip:
             c['ip_address'] = self.ip.ip_address
+        if self.network_id:
+            c['network_id'] = self.network_id
         c.update(sorted(super().get_context().items()))
         return c
 
@@ -391,6 +416,9 @@ def get_vpn_server_context(self):
                 context_keys['server_ip_network']
             ] = f'{self.ip.ip_address}/{self.subnet.subnet.max_prefixlen}'
             context[context_keys['vpn_subnet']] = str(self.subnet.subnet)
+        if self._is_backend_type('zerotier') and self.network_id:
+            context[context_keys['network_name']] = self.name
+            context[context_keys['network_id']] = self.network_id
         return context
 
     def get_system_context(self):
@@ -422,6 +450,8 @@ def _get_auto_context_keys(self):
             * ip address
         VXLAN:
             * vni (VXLAN Network Identifier)
+        ZeroTier
+            * network_id (ZeroTier Network Identifier)
         """
         pk = self.pk.hex
         context_keys = {
@@ -457,6 +487,9 @@ def _get_auto_context_keys(self):
                     'server_ip_network': 'server_ip_network_{}'.format(pk),
                 }
             )
+        if self._is_backend_type('zerotier'):
+            context_keys.update({'network_id': 'network_id_{}'.format(pk)})
+            context_keys.update({'network_name': 'network_name_{}'.format(pk)})
         return context_keys
 
     def auto_client(self, auto_cert=True, template_backend_class=None):
@@ -492,7 +525,8 @@ def auto_client(self, auto_cert=True, template_backend_class=None):
             # call auto_client and update the config
             elif self._is_backend_type('zerotier') and template_backend_class:
                 auto = backend.auto_client(
-                    host=self.host,
+                    name=self.name,
+                    nwid=self.network_id,
                     server=self.config[config_dict_key][0],
                     **context_keys,
                 )

openwisp_controller/config/migrations/0039_wireguard_vxlan_ipam.py

@@ -46,7 +46,10 @@ class Migration(migrations.Migration):
             name='auth_token',
             field=models.CharField(
                 blank=True,
-                help_text=('Authentication token for triggering "Webhook Endpoint"'),
+                help_text=(
+                    'Authentication token used for triggering '
+                    '"Webhook Endpoint" or for calling "ZerotierService" API'
+                ),
                 max_length=128,
                 null=True,
                 verbose_name='Webhook AuthToken',

openwisp_controller/config/migrations/0048_alter_vpn_auth_token.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.2.19 on 2023-06-26 12:51
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('config', '0047_add_organizationlimits'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='vpn',
+            name='network_id',
+            field=models.CharField(blank=True, max_length=16),
+        ),
+    ]

openwisp_controller/config/migrations/0048_vpn_network_id.py

@@ -50,11 +50,6 @@ django.jQuery(function ($) {
         // For Zerotier VPN backend
         if(backendValue.includes('zerotier')){
             $('label[for="id_auth_token"]').parent().parent().show();
-            $('label[for="id_subnet"]').parent().parent().hide();
-            $('label[for="id_ip"]').parent().parent().hide();
-        } else {
-            $('label[for="id_subnet"]').parent().parent().show();
-            $('label[for="id_ip"]').parent().parent().show();
         }
     };