Merge lp:~rvb/maas/maas-forwarders into lp:~maas-committers/maas/trunk

[Raphaël Badin]

upstream_dns: accept a list of forwarders (and not just *one* forwarder).

This changes only the config form, the backend (in trunk) already supports space-separated IP addresses.
Tested in my lab.

[Mike Pontillo (mpontillo)]

Looks good to me. Thanks for fixing this!

review: Approve

[Christian Reis (kiko)]

On Wed, Jul 01, 2015 at 05:36:25PM -0000, Raphaël Badin wrote:
> + for ip in value.split():

Could we take commas here as well, just to be more accepting?
--
Christian Robottom Reis | [+1] 612 888 4935 | http://launchpad.net/~kiko
Canonical VP Hyperscale | [+55 16] 9 9112 6430

[Mike Pontillo (mpontillo)]

Taking commas would require further changes to the back-end code, since elsewhere we assume it is whitespace-separated.

[Raphaël Badin (rvb)]

> Taking commas would require further changes to the back-end code, since
> elsewhere we assume it is whitespace-separated.

Actually, it's not that big of a deal. Done.

[Mike Pontillo (mpontillo)]

I think I'd prefer a utility method rather than a pre-compiled UPSTREAM_DNS_SEPS, since that code is duplicated in a couple places. But looks good to me.

review: Approve

[Raphaël Badin (rvb)]

> I think I'd prefer a utility method rather than a pre-compiled
> UPSTREAM_DNS_SEPS, since that code is duplicated in a couple places. But looks
> good to me.

Yeah, I've fixed that by normalizing the list.

[Mike Pontillo (mpontillo)]

Nice; thanks for the fixes.

=== modified file 'src/maasserver/dns/tests/test_config.py'
--- src/maasserver/dns/tests/test_config.py	2015-06-15 12:42:07 +0000
+++ src/maasserver/dns/tests/test_config.py	2015-07-02 08:37:08 +0000
@@ -676,12 +676,13 @@
         self.assertEqual([], get_upstream_dns())
 
     def test__returns_list_of_one_address_if_set(self):
-        address = factory.make_ipv4_address()
+        address = factory.make_ip_address()
         Config.objects.set_config("upstream_dns", address)
         self.assertEqual([address], get_upstream_dns())
 
-    def test__returns_list_of_many_address_if_set(self):
-        addresses = [factory.make_ipv4_address(), factory.make_ipv4_address()]
+    def test__returns_list_if_space_separated_ips(self):
+        addresses = [
+            factory.make_ip_address() for _ in range(3)]
         Config.objects.set_config("upstream_dns", " ".join(addresses))
         self.assertEqual(addresses, get_upstream_dns())
 

=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py	2015-06-19 09:53:30 +0000
+++ src/maasserver/fields.py	2015-07-02 08:37:08 +0000
@@ -1,7 +1,7 @@
 # Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Custom model fields."""
+"""Custom model and form fields."""
 
 from __future__ import (
     absolute_import,
@@ -15,6 +15,7 @@
 __all__ = [
     "CIDRField",
     "EditableBinaryField",
+    "IPListFormField",
     "MAASIPAddressField",
     "MAC",
     "MACAddressField",
@@ -576,3 +577,26 @@
         }
         defaults.update(kwargs)
         return super(CIDRField, self).formfield(**defaults)
+
+
+class IPListFormField(CharField):
+    """Accepts a Space/comma separated list of IP addresses.
+
+    This field normalizes the list to a space-separated list.
+    """
+    separators = re.compile('[,\ ]')
+
+    def clean(self, value):
+        if value is None:
+            return None
+        else:
+            ips = re.split(self.separators, value)
+            ips = [ip.strip() for ip in ips if ip.strip() != '']
+            for ip in ips:
+                try:
+                    GenericIPAddressField().clean(ip, model_instance=None)
+                except ValidationError:
+                    raise ValidationError(
+                        "Invalid IP address: %s; Provide a list of "
+                        "space-separated IP addresses" % ip)
+            return ' '.join(ips)

=== modified file 'src/maasserver/forms_settings.py'
--- src/maasserver/forms_settings.py	2015-06-04 14:31:33 +0000
+++ src/maasserver/forms_settings.py	2015-07-02 08:37:08 +0000
@@ -26,6 +26,7 @@
 from django import forms
 from django.core.exceptions import ValidationError
 from maasserver.bootresources import IMPORT_RESOURCES_SERVICE_PERIOD
+from maasserver.fields import IPListFormField
 from maasserver.models.config import (
     Config,
     DEFAULT_OS,
@@ -180,11 +181,11 @@
     },
     'upstream_dns': {
         'default': None,
-        'form': forms.GenericIPAddressField,
+        'form': IPListFormField,
         'form_kwargs': {
             'label': (
                 "Upstream DNS used to resolve domains not managed by this "
-                "MAAS"),
+                "MAAS (space/comma-separated IP addresses)"),
             'required': False,
             'help_text': (
                 "Only used when MAAS is running its own DNS server. This "

=== modified file 'src/maasserver/tests/test_fields.py'
--- src/maasserver/tests/test_fields.py	2015-06-19 09:53:30 +0000
+++ src/maasserver/tests/test_fields.py	2015-07-02 08:37:08 +0000
@@ -16,6 +16,7 @@
 
 import json
 from random import randint
+import re
 
 from django.core import serializers
 from django.core.exceptions import ValidationError
@@ -27,6 +28,7 @@
 from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
 from maasserver.fields import (
     EditableBinaryField,
+    IPListFormField,
     LargeObjectField,
     LargeObjectFile,
     MAC,
@@ -542,3 +544,38 @@
         normalized_cidr = '192.0.2.0/24'
         instance = CIDRTestModel.objects.create(cidr=cidr)
         self.assertEqual(normalized_cidr, reload_object(instance).cidr)
+
+
+class IPListFormFieldTest(MAASServerTestCase):
+
+    def test_accepts_none(self):
+        self.assertIsNone(IPListFormField().clean(None))
+
+    def test_accepts_single_ip(self):
+        ip = factory.make_ip_address()
+        self.assertEquals(ip, IPListFormField().clean(ip))
+
+    def test_accepts_space_separated_ips(self):
+        ips = [factory.make_ip_address() for _ in range(5)]
+        input = ' '.join(ips)
+        self.assertEquals(input, IPListFormField().clean(input))
+
+    def test_accepts_comma_separated_ips(self):
+        ips = [factory.make_ip_address() for _ in range(5)]
+        input = ','.join(ips)
+        self.assertEquals(' '.join(ips), IPListFormField().clean(input))
+
+    def test_rejects_invalid_input(self):
+        invalid = factory.make_name('invalid')
+        input = ' '.join([factory.make_ip_address(), invalid])
+        error = self.assertRaises(
+            ValidationError, IPListFormField().clean, input)
+        self.assertIn("Invalid IP address: %s" % invalid, error.message)
+
+    def test_separators_dont_conflict_with_ipv4_address(self):
+        self.assertIsNone(re.search(
+            IPListFormField.separators, factory.make_ipv4_address()))
+
+    def test_separators_dont_conflict_with_ipv6_address(self):
+        self.assertIsNone(re.search(
+            IPListFormField.separators, factory.make_ipv6_address()))

=== modified file 'src/maasserver/tests/test_forms_settings.py'
--- src/maasserver/tests/test_forms_settings.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/tests/test_forms_settings.py	2015-07-02 08:37:08 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = []
 
+
 from django import forms
 from maasserver.forms_settings import (
     CONFIG_ITEMS,
@@ -70,3 +71,10 @@
             compose_invalid_choice_text(
                 'commissioning_distro_series', field.choices),
             field.error_messages['invalid_choice'])
+
+    def test_upstream_dns_accepts_ip_list(self):
+        field = get_config_field('upstream_dns')
+        ips1 = [factory.make_ip_address() for _ in range(3)]
+        ips2 = [factory.make_ip_address() for _ in range(3)]
+        input = ' '.join(ips1) + ' ' + ','.join(ips2)
+        self.assertEqual(' '.join(ips1 + ips2), field.clean(input))