Simplify the API that signals subnet changes to configurations

Currently, the only backend we have (LDAP VPN) already keeps a per-VPN
state, so that is can simply re-generate all subnets in LDAP whenever
something changes.

If a new backend does not keep such a state (e.g. it simply inserts and
deletes static routes), then we could expose a more complicated API:
pre_save, post_save, pre_delete and post_delete events.

coin/configuration/models.py

@@ -71,40 +71,39 @@ class Configuration(PolymorphicModel):
 
 
 @receiver(post_save, sender=IPSubnet)
-def subnet_save_event(sender, **kwargs):
-    """Fires when a subnet is saved (created/modified).  We tell the
+@receiver(post_delete, sender=IPSubnet)
+def subnet_event(sender, **kwargs):
+    """Fires when a subnet is created, modified or deleted.  We tell the
     configuration backend to do whatever it needs to do with it.
 
-    We should use a pre_save signal, so that if anything goes wrong in the
-    backend (exception raised), nothing is actually saved in the database.
-    But it has a big problem: the configuration backend will not see the
-    change, since it has not been saved into the database yet.
-
-    That's why we use a post_save signal instead.  But surprisingly, all
-    is well: if we raise an exception here, the IPSubnet object will not
-    be saved in the database.  But the backend *does* see the new state of
-    the database.  It looks like the database rollbacks if an exception is
-    raised.  Whatever the reason, this is not a documented feature of
-    Django signals.
-    """
-    subnet = kwargs['instance']
-    try:
-        config = subnet.configuration
-        if hasattr(config, 'save_subnet'):
-            config.save_subnet(subnet, kwargs['created'])
-    except ObjectDoesNotExist:
-        pass
+    Note that we could provide a more advanced API to configurations
+    (subnet created, subnet modified, subnet deleted), but this is quite
+    complicated to do.  It's much simpler to simply tell the configuration
+    model that something has changed in IP subnets.  The configuration
+    model can then access the list of its associated subnets (via the
+    "ip_subnet" attribute) to decide for itself what it wants to do.
+
+    We should use a pre_save/pre_delete signal, so that if anything goes
+    wrong in the backend (exception raised), nothing is actually saved in
+    the database: this provides consistency between the database and the
+    backend.  But if we do this, there is a major issue: the configuration
+    backend will not see the new state of subnets by querying the
+    database, since the changes have not been saved into the database yet.
+
+    That's why we use a post_save/post_delete signal instead.  In theory,
+    this is a bad idea, because if the backend fails to do whatever it
+    needs to do, the subnet will be saved into Django's database anyway,
+    causing a desynchronisation with the backend.  But surprisingly, even
+    if not a documented feature of Django's signals, all is well: if we
+    raise an exception here, the IPSubnet object will not be saved in the
+    database.  It looks like the database rollbacks if an exception is
+    raised, which is great (even if undocumented).
 
-
-@receiver(post_delete, sender=IPSubnet)
-def subnet_delete_event(sender, **kwargs):
-    """Fires when a subnet is deleted.  We tell the configuration backend to
-    do whatever it needs to do with it.
     """
     subnet = kwargs['instance']
     try:
         config = subnet.configuration
-        if hasattr(config, 'delete_subnet'):
-            config.delete_subnet(subnet)
+        if hasattr(config, 'subnet_event'):
+            config.subnet_event()
     except ObjectDoesNotExist:
         pass

coin/vpn/models.py

@@ -44,16 +44,14 @@ class VPNConfiguration(CoinLdapSyncMixin, Configuration):
     def get_absolute_url(self):
         return reverse('vpn:details', args=[str(self.pk)])
 
-    # These two methods are part of the general configuration interface.
-    def save_subnet(self, subnet, creation):
+    # This method is part of the general configuration interface.
+    def subnet_event(self):
         self.check_endpoints(delete=True)
-        # We potentially changed the endpoints, so we need to save.
+        # We potentially changed the endpoints, so we need to save.  Also,
+        # saving will update the subnets in the LDAP backend.
         self.full_clean()
         self.save()
 
-    def delete_subnet(self, subnet):
-        self.save_subnet(subnet, False)
-
     def get_subnets(self, version):
         subnets = self.ip_subnet.all()
         return [subnet for subnet in subnets if subnet.inet.version == version]