Merge lp:~julian-edwards/maas/acl-for-dns-trigger-rewrite into lp:~maas-committers/maas/trunk

[Julian Edwards]

Connect save and delete signals for Networks so that the DNS config is rewritten. This is so the networks ACL for DNS will get updated when networks change.

[Jeroen T. Vermeulen (jtv)]

It looks to me as if test_writes_dns_when_network_edited should be called test_writes_dns_when_network_created. Why does it go through the form?

I still don't like signals for this kind of thing. Do we know what we'll be doing during network auto-discovery? Will we implicitly be firing off a bunch of DNS server restarts concurrently, which may then get confused and break? It would make more sense to me to set some kind of “dirty” flag on DNS, and process that separately.

Also, consider triggering the signal only on specific field changes. Updating the description on a network, or saving it without changes, or perhaps updating some future field that may see routine changes, probably needn't activate the whole DNS machinery.

review: Approve

[Julian Edwards (julian-edwards)]

Thanks for the review!

On Monday 01 Sep 2014 03:20:57 you wrote:
> Review: Approve
>
> It looks to me as if test_writes_dns_when_network_edited should be called
> test_writes_dns_when_network_created. Why does it go through the form?

1. It's not just creating, it's any editing (which includes creating).
2. Using the form just covers actual code use style. I didn't want to rely on
any factory side effects.

> I still don't like signals for this kind of thing. Do we know what we'll be
> doing during network auto-discovery? Will we implicitly be firing off a
> bunch of DNS server restarts concurrently, which may then get confused and
> break? It would make more sense to me to set some kind of “dirty” flag on
> DNS, and process that separately.

I hate signals too, but there's not much choice. I think we've covered this
to death already.

Given that this will condense into an RPC job at some point, the easiest (and
I'd argue *right) place to coalesce jobs is right there. For now I figured
the rate at which Networks are changed will be absolutely tiny.

> Also, consider triggering the signal only on specific field changes.
> Updating the description on a network, or saving it without changes, or
> perhaps updating some future field that may see routine changes, probably
> needn't activate the whole DNS machinery.

Again, the number of changes to Networks will be tiny, it's not worth it IMO,
especially as we add more fields that *will* require a trigger would
potentially get missed.

cheers.

[Jeroen T. Vermeulen (jtv)]

>> It looks to me as if test_writes_dns_when_network_edited should be called
>> test_writes_dns_when_network_created. Why does it go through the form?
>
> 1. It's not just creating, it's any editing (which includes creating).

That's my point. You're not testing that, but the name of the test makes it seem as if you do.

[Julian Edwards (julian-edwards)]

On Monday 01 Sep 2014 03:45:08 you wrote:
> >> It looks to me as if test_writes_dns_when_network_edited should be called
> >> test_writes_dns_when_network_created. Why does it go through the form?
> >
> > 1. It's not just creating, it's any editing (which includes creating).
>
> That's my point. You're not testing that, but the name of the test makes it
> seem as if you do.

I that case I don't see your point. I thought the name was clear enough.

=== modified file 'src/maasserver/dns/connect.py'
--- src/maasserver/dns/connect.py	2014-08-15 09:44:42 +0000
+++ src/maasserver/dns/connect.py	2014-09-01 01:38:17 +0000
@@ -24,6 +24,7 @@
 from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
 from maasserver.models import (
     Config,
+    Network,
     Node,
     NodeGroup,
     NodeGroupInterface,
@@ -103,6 +104,19 @@
     write_full_dns_config()
 
 
+@receiver(post_save, sender=Network)
+def dns_post_save_Network(sender, instance, **kwargs):
+    """When a network is added/changed, put it in the DNS trusted networks."""
+    from maasserver.dns.config import write_full_dns_config
+    write_full_dns_config()
+
+
+@receiver(post_delete, sender=Network)
+def dns_post_delete_Network(sender, instance, **kwargs):
+    from maasserver.dns.config import write_full_dns_config
+    write_full_dns_config()
+
+
 Config.objects.config_changed_connect("upstream_dns", dns_setting_changed)
 Config.objects.config_changed_connect(
     "windows_kms_host", dns_setting_changed)

=== modified file 'src/maasserver/tests/test_forms_network.py'
--- src/maasserver/tests/test_forms_network.py	2014-08-19 15:17:01 +0000
+++ src/maasserver/tests/test_forms_network.py	2014-09-01 01:38:17 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = []
 
+from maasserver.dns import config as dns_config_module
 from maasserver.forms import NetworkForm
 from maasserver.models import (
     MACAddress,
@@ -22,6 +23,7 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
+from maastesting.matchers import MockCalledOnceWith
 from netaddr import IPNetwork
 
 
@@ -140,3 +142,26 @@
                 'netmask': [message],
             },
             form.errors)
+
+    def test_writes_dns_when_network_edited(self):
+        write_full_dns_config = self.patch(
+            dns_config_module, "write_full_dns_config")
+        network = factory.getRandomNetwork()
+        name = factory.make_name('network')
+        definition = {
+            'name': name,
+            'description': factory.make_string(),
+            'ip': "%s" % network.cidr.ip,
+            'netmask': "%s" % network.netmask,
+            'vlan_tag': factory.make_vlan_tag(),
+        }
+        form = NetworkForm(data=definition)
+        form.save()
+        self.assertThat(write_full_dns_config, MockCalledOnceWith())
+
+    def test_writes_dns_when_network_deleted(self):
+        network = factory.make_network()
+        write_full_dns_config = self.patch(
+            dns_config_module, "write_full_dns_config")
+        network.delete()
+        self.assertThat(write_full_dns_config, MockCalledOnceWith())