[change] Auto add default and required templates to existing devices #480

Closes #480

Author: pandafy

docs/user/templates.rst

@@ -79,7 +79,7 @@ Default Templates
     :alt: Templates enabled by default
 
 When templates are flagged as **"Enabled by default"**, they will be
-automatically assigned to new devices.
+automatically assigned to new and existing devices.
 
 This is a very powerful feature: **once default templates are correctly
 configured to implement the use case you need, you will only have to
@@ -95,11 +95,10 @@ default templates without the need of manual intervention from the network
 operators.
 
 An organization specific template flagged as default will be automatically
-assigned to any new device which will be created in the same organization.
+assigned to all existing and new devices in the same organization.
 
 A shared default template instead will be automatically assigned to all
-the new devices which will be created in the system, regardless of
-organization.
+existing and new devices in the system, regardless of organization.
 
 .. _required_templates:
 

openwisp_controller/config/base/template.py

@@ -1,4 +1,5 @@
 import json
+import logging
 from collections import OrderedDict
 from copy import copy
 
@@ -7,14 +8,19 @@
 from django.utils.translation import gettext_lazy as _
 from jsonfield import JSONField
 from netjsonconfig.exceptions import ValidationError as NetjsonconfigValidationError
-from swapper import get_model_name
+from swapper import get_model_name, load_model
 from taggit.managers import TaggableManager
 
 from ...base import ShareableOrgMixinUniqueName
 from ..settings import DEFAULT_AUTO_CERT
-from ..tasks import update_template_related_config_status
+from ..tasks import (
+    auto_add_template_to_existing_config,
+    update_template_related_config_status,
+)
 from .base import BaseConfig
 
+logger = logging.getLogger(__name__)
+
 TYPE_CHOICES = (("generic", _("Generic")), ("vpn", _("VPN-client")))
 
 
@@ -61,7 +67,7 @@ class AbstractTemplate(ShareableOrgMixinUniqueName, BaseConfig):
         default=False,
         db_index=True,
         help_text=_(
-            "whether new configurations will have this template enabled by default"
+            "whether all configurations will have this template enabled by default"
         ),
     )
     required = models.BooleanField(
@@ -124,14 +130,31 @@ def pre_save_handler(cls, instance, *args, **kwargs):
             # the old configuration may become invalid, raising an exception.
             # Setting the checksum to None forces related configurations to update.
             current_checksum = None
-        instance._update_related_config_status = instance.checksum != current_checksum
+        instance._should_update_related_config_status = (
+            instance.checksum != current_checksum
+        )
+
+        # Check if template is becoming default or required
+        if (instance.default and not current.default) or (
+            instance.required and not current.required
+        ):
+            instance._should_add_to_existing_configs = True
 
     @classmethod
     def post_save_handler(cls, instance, created, *args, **kwargs):
-        if not created and getattr(instance, "_update_related_config_status", False):
+        if not created and getattr(
+            instance, "_should_update_related_config_status", False
+        ):
             transaction.on_commit(
                 lambda: update_template_related_config_status.delay(instance.pk)
             )
+        # Auto-add template to existing configs if it's new or became default/required
+        if getattr(instance, "_should_add_to_existing_configs", False) or (
+            created and (instance.default or instance.required)
+        ):
+            transaction.on_commit(
+                lambda: auto_add_template_to_existing_config.delay(str(instance.pk))
+            )
 
     def _update_related_config_status(self):
         # use atomic to ensure any code bound to
@@ -153,6 +176,44 @@ def _update_related_config_status(self):
                 if config.pk in changing_status:
                     config._send_config_status_changed_signal()
 
+    def _auto_add_to_existing_configs(self):
+        """
+        When a template is ``default`` or ``required``, adds the template
+        to each relevant ``Config`` object
+        """
+        Config = load_model("config", "Config")
+
+        # Only proceed if template is default or required
+        if not (self.default or self.required):
+            return
+
+        # use atomic to ensure any code bound to
+        # be executed via transaction.on_commit
+        # is executed after the whole block
+        with transaction.atomic():
+            # Exclude deactivating or deactivated configs
+            configs = (
+                Config.objects.select_related("device")
+                .filter(
+                    backend=self.backend,
+                )
+                .exclude(
+                    models.Q(status__in=["deactivating", "deactivated"])
+                    | models.Q(templates__id=self.pk)
+                )
+            )
+            if self.organization_id:
+                configs = configs.filter(device__organization_id=self.organization_id)
+            for config in configs.iterator():
+                try:
+                    config.templates.add(self)
+                except Exception as e:
+                    # Log error but continue with other configs
+                    logger.exception(
+                        f"Failed to add template {self.pk} to "
+                        f"config {config.pk}: {e}"
+                    )
+
     def clean(self, *args, **kwargs):
         """
         * validates org relationship of VPN if present

openwisp_controller/config/migrations/0001_squashed_0002_config_settings_uuid.py

@@ -294,7 +294,7 @@ class Migration(migrations.Migration):
                         db_index=True,
                         default=False,
                         help_text=(
-                            "whether new configurations will have this "
+                            "whether all configurations will have this "
                             "template enabled by default"
                         ),
                         verbose_name="enabled by default",

openwisp_controller/config/tasks.py

@@ -37,6 +37,26 @@ def update_template_related_config_status(template_pk):
         )
 
 
+@shared_task(soft_time_limit=7200)
+def auto_add_template_to_existing_config(template_pk):
+    Template = load_model("config", "Template")
+    try:
+        template = Template.objects.get(pk=template_pk)
+    except ObjectDoesNotExist as e:
+        logger.warning(
+            f'auto_add_template_to_existing_config("{template_pk}") failed: {e}'
+        )
+        return
+    try:
+        template._auto_add_to_existing_configs()
+    except SoftTimeLimitExceeded:
+        logger.error(
+            "soft time limit hit while executing "
+            f"_auto_add_to_existing_configs for {template} "
+            f"(ID: {template_pk})"
+        )
+
+
 @shared_task(soft_time_limit=1200)
 def create_vpn_dh(vpn_pk):
     """

openwisp_controller/config/tests/test_template.py

@@ -14,6 +14,7 @@
 
 from .. import settings as app_settings
 from ..signals import config_modified, config_status_changed
+from ..tasks import auto_add_template_to_existing_config
 from ..tasks import logger as task_logger
 from ..tasks import update_template_related_config_status
 from .utils import CreateConfigTemplateMixin, TestVpnX509Mixin
@@ -517,6 +518,31 @@ def test_regression_preventing_from_fixing_invalid_conf(self):
         template.full_clean()
         template.save()
 
+    def test_auto_add_to_existing_configs_error_handling(self):
+        with self.subTest("Template is not default or required"):
+            template = self._create_template(
+                name="test-template", required=False, default=False
+            )
+            with mock.patch.object(transaction, "atomic") as mocked_atomic:
+                template._auto_add_to_existing_configs()
+            mocked_atomic.assert_not_called()
+
+        with self.subTest("config.templates.add(template) raises error"):
+            config = self._create_config(device=self._create_device(name="test-device"))
+            template.required = True
+            template.full_clean()
+            template.save()
+            with mock.patch.object(
+                Config.templates.related_manager_cls,
+                "add",
+                side_effect=ValueError("Incompatible template"),
+            ), mock.patch("logging.Logger.exception") as mocked_logger:
+                template._auto_add_to_existing_configs()
+            mocked_logger.assert_called_once_with(
+                f"Failed to add template {template.pk} to config {config.pk}:"
+                " Incompatible template"
+            )
+
 
 class TestTemplateTransaction(
     CreateConfigTemplateMixin,
@@ -749,3 +775,153 @@ def test_task_timeout(self, mocked_update_related_config_status):
             template.save()
             mocked_error.assert_called_once()
         mocked_update_related_config_status.assert_called_once()
+
+    def _create_env_for_adding_template_to_existing_config(self):
+        org1 = self._get_org()
+        org1_config = self._create_config(organization=org1, status="applied")
+        org2 = self._create_org(name="org2", slug="org2")
+        org2_config = self._create_config(organization=org2, status="applied")
+        org2_deactivating_config = self._create_config(
+            status="deactivating",
+            device=self._create_device(
+                name="test-deactivating",
+                mac_address="11:22:33:44:55:67",
+                organization=org2,
+            ),
+        )
+        org2_deactivated_config = self._create_config(
+            status="deactivated",
+            device=self._create_device(
+                name="test-deactivated",
+                mac_address="11:22:33:44:55:68",
+                organization=org2,
+            ),
+        )
+        return (
+            org1_config,
+            org2_config,
+            org2_deactivating_config,
+            org2_deactivated_config,
+            org1,
+            org2,
+        )
+
+    def _assert_template_added_to_existing_config(
+        self,
+        template,
+        org1_config,
+        org2_config,
+        org2_deactivating_config,
+        org2_deactivated_config,
+    ):
+        org1_config.refresh_from_db()
+        self.assertEqual(org1_config.status, "modified")
+        self.assertIn(template, org1_config.templates.all())
+        org2_config.refresh_from_db()
+        self.assertEqual(org2_config.status, "modified")
+        self.assertIn(template, org2_config.templates.all())
+
+        org2_deactivating_config.refresh_from_db()
+        self.assertEqual(org2_deactivating_config.status, "deactivating")
+        self.assertEqual(org2_deactivating_config.templates.count(), 0)
+
+        org2_deactivated_config.refresh_from_db()
+        self.assertEqual(org2_deactivated_config.status, "deactivated")
+        self.assertEqual(org2_deactivated_config.templates.count(), 0)
+
+    def test_required_template_added_to_existing_config(self):
+        (
+            org1_config,
+            org2_config,
+            org2_deactivating_config,
+            org2_deactivated_config,
+            _,
+            _,
+        ) = self._create_env_for_adding_template_to_existing_config()
+        shared_template = self._create_template(
+            name="shared-template", required=True, default=True
+        )
+        self._assert_template_added_to_existing_config(
+            shared_template,
+            org1_config,
+            org2_config,
+            org2_deactivating_config,
+            org2_deactivated_config,
+        )
+
+    def test_default_template_added_to_existing_config(self):
+        (
+            org1_config,
+            org2_config,
+            org2_deactivating_config,
+            org2_deactivated_config,
+            _,
+            _,
+        ) = self._create_env_for_adding_template_to_existing_config()
+        default_template = self._create_template(name="default-template", default=True)
+        self._assert_template_added_to_existing_config(
+            default_template,
+            org1_config,
+            org2_config,
+            org2_deactivating_config,
+            org2_deactivated_config,
+        )
+
+    def test_update_existing_template_to_be_required(self):
+        template = self._create_template(
+            name="existing-template", required=False, default=False
+        )
+        config = self._create_config(organization=self._get_org())
+        self.assertNotIn(template, config.templates.all())
+
+        template.required = True
+        template.full_clean()
+        template.save()
+
+        config.refresh_from_db()
+        self.assertEqual(config.status, "modified")
+        self.assertIn(template, config.templates.all())
+
+    def test_update_existing_template_to_be_default(self):
+        template = self._create_template(
+            name="existing-template", required=False, default=False
+        )
+        config = self._create_config(organization=self._get_org())
+        self.assertNotIn(template, config.templates.all())
+
+        template.default = True
+        template.full_clean()
+        template.save()
+
+        config.refresh_from_db()
+        self.assertEqual(config.status, "modified")
+        self.assertIn(template, config.templates.all())
+
+    @mock.patch.object(auto_add_template_to_existing_config, "delay")
+    def test_auto_add_template_to_existing_config_not_triggered(self, mocked_task):
+        """
+        Ensure that auto_add_template_to_existing_config task is not triggered
+        when a required/default template is updated.
+        """
+        with self.subTest("Updating default template does not trigger task"):
+            default_template = self._create_template(
+                name="default-template", default=True
+            )
+            mocked_task.assert_called_once_with(str(default_template.pk))
+            mocked_task.reset_mock()
+
+            default_template.config["interfaces"][0]["name"] = "eth1"
+            default_template.full_clean()
+            default_template.save()
+            mocked_task.assert_not_called()
+
+        with self.subTest("Updating required template does not trigger task"):
+            required_template = self._create_template(
+                name="required-template", required=True
+            )
+            mocked_task.assert_called_once_with(str(required_template.pk))
+            mocked_task.reset_mock()
+            required_template.config["interfaces"][0]["name"] = "eth1"
+            required_template.full_clean()
+            required_template.save()
+            mocked_task.assert_not_called()

tests/openwisp2/sample_config/migrations/0001_initial.py

@@ -527,7 +527,7 @@ class Migration(migrations.Migration):
                         db_index=True,
                         default=False,
                         help_text=(
-                            "whether new configurations will have this "
+                            "whether all configurations will have this "
                             "template enabled by default"
                         ),
                         verbose_name="enabled by default",