Merge ~ltrager/maas:interface_param_dont_allow_unconfigured into maas:master

[Lee Trager]

Don't allow unconfigured or disabled interfaces from being set as a script parameter.

[Alberto Donato (ack)]

A few questions/comments inline

review: Needs Information

5a59707... by Lee Trager on 2019-07-23

ack fixes

[Lee Trager (ltrager)]

Thanks for the review, responses inline :)

94754d4... by Lee Trager on 2019-07-24

ack fixes

[Newell Jensen (newell-jensen)]

+1

review: Approve

diff --git a/src/maasserver/forms/parameters.py b/src/maasserver/forms/parameters.py
index 034f353..ceb9284 100644
--- a/src/maasserver/forms/parameters.py
+++ b/src/maasserver/forms/parameters.py
@@ -20,6 +20,7 @@ from django.forms import (
     Field,
     Form,
 )
+from maasserver.enum import IPADDRESS_TYPE
 from maasserver.utils.dns import validate_url
 from maasserver.utils.forms import set_form_error
 from maasserver.utils.mac import is_mac
@@ -44,6 +45,20 @@ class ParametersForm(Form):
         self._node = node
         self._script = script
 
+    def _get_interfaces(self):
+        """Return a queryset of the Interfaces that can be used."""
+        interfaces = self._node.interface_set.filter(
+            children_relationships=None,
+            enabled=True)
+        interfaces = interfaces.exclude(
+            ip_addresses__alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        # Exclude all IPAddresses of type DISCOVERED before checking if an
+        # Interface doesn't have IPAddresses. If done as one excludes
+        # DISCOVERED IPAddresses won't be filtered out before isnull is
+        # checked.
+        return interfaces.exclude(
+            ip_addresses__isnull=True, ip_addresses__ip=None)
+
     def clean_parameters(self):
         """Validate the parameters set in the embedded YAML within the script.
         """
@@ -267,7 +282,7 @@ class ParametersForm(Form):
     def _validate_and_clean_storage_all(
             self, param_name, param, result_params, ret):
         """Validate and clean storage input when set to all."""
-        if not self._node.physicalblockdevice_set.exists():
+        if len(self._node.physicalblockdevice_set) == 0:
             # Use 'all' as a place holder until the disks get added during
             # commissioning.
             clean_param = copy.deepcopy(result_params)
@@ -319,7 +334,7 @@ class ParametersForm(Form):
             qs = self._node.physicalblockdevice_set.filter(
                 Q(name=i) | Q(name=os.path.basename(i)) |
                 Q(model=i) | Q(serial=i) | Q(tags__overlap=[i]))
-            if not qs.exists():
+            if len(qs) == 0:
                 set_form_error(
                     self, param_name, "Unknown storage device for %s(%s)" % (
                         self._node.fqdn, self._node.system_id))
@@ -344,15 +359,15 @@ class ParametersForm(Form):
     def _validate_and_clean_interface_all(
             self, param_name, param, result_params, ret):
         """Validate and clean interface input when set to all."""
-        if not self._node.interface_set.exists():
-            # Use 'all' as a place holder until the disks get added during
+        interfaces = self._get_interfaces()
+        if len(interfaces) == 0:
+            # Use 'all' as a place holder until the Interfaces get added during
             # commissioning.
             clean_param = copy.deepcopy(result_params)
             clean_param[param_name] = param
             ret.append(clean_param)
         else:
-            for interface in self._node.interface_set.filter(
-                    children_relationships=None):
+            for interface in interfaces:
                 clean_param = copy.deepcopy(result_params)
                 clean_param[param_name] = copy.deepcopy(param)
                 clean_param[param_name]['value'] = self._interface_to_dict(
@@ -361,10 +376,9 @@ class ParametersForm(Form):
 
     def _validate_and_clean_interface_id(
             self, param_name, param, result_params, ret):
-        """Validate and clean storage input when id."""
+        """Validate and clean interface input when id."""
         try:
-            interface = self._node.interface_set.get(
-                children_relationships=None, id=int(param['value']))
+            interface = self._get_interfaces().get(id=int(param['value']))
         except ObjectDoesNotExist:
             set_form_error(
                 self, param_name, 'Interface id does not exist')
@@ -381,12 +395,12 @@ class ParametersForm(Form):
         value = param['value']
         for i in value.split(','):
             if ':' in i:
-                # Allow users to specify a disk using the vendor and product.
+                # Allow users to specify an Interface using the vendor and
+                # product.
                 vendor, product = i.split(':')
                 try:
-                    interface = self._node.interface_set.get(
-                        vendor=vendor, product=product,
-                        children_relationships=None)
+                    interface = self._get_interfaces().get(
+                        vendor=vendor, product=product)
                 except ObjectDoesNotExist:
                     pass
                 else:
@@ -397,14 +411,13 @@ class ParametersForm(Form):
                     ret.append(clean_param)
                     continue
 
-            qs = self._node.interface_set.filter(children_relationships=None)
             if is_mac(i):
-                qs = qs.filter(mac_address=i)
+                qs = self._get_interfaces().filter(mac_address=i)
             else:
-                qs = qs.filter(
+                qs = self._get_interfaces().filter(
                     Q(name=i) | Q(vendor=i) | Q(product=i) |
                     Q(tags__overlap=[i]))
-            if not qs.exists():
+            if len(qs) == 0:
                 set_form_error(
                     self, param_name, "Unknown interface for %s(%s)" % (
                         self._node.fqdn, self._node.system_id))
diff --git a/src/maasserver/forms/tests/test_parameters.py b/src/maasserver/forms/tests/test_parameters.py
index 7da3034..bf94f65 100644
--- a/src/maasserver/forms/tests/test_parameters.py
+++ b/src/maasserver/forms/tests/test_parameters.py
@@ -7,7 +7,10 @@ __all__ = []
 
 import random
 
-from maasserver.enum import INTERFACE_TYPE
+from maasserver.enum import (
+    INTERFACE_TYPE,
+    IPADDRESS_TYPE,
+)
 from maasserver.forms.parameters import ParametersForm
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -578,8 +581,18 @@ class TestParametersForm(MAASServerTestCase):
 
     def test__input_interface_all(self):
         node = factory.make_Node()
-        for _ in range(3):
-            factory.make_Interface(node=node)
+        subnet = factory.make_Subnet()
+        usable_interfaces = [
+            factory.make_Interface(node=node, subnet=subnet)
+            for _ in range(3)
+        ]
+        # Unconfigured or disabled interfaces
+        factory.make_Interface(node=node, enabled=False)
+        factory.make_Interface(node=node, link_connected=False)
+        discovered = factory.make_Interface(node=node, subnet=subnet)
+        discovered_ip = discovered.ip_addresses.first()
+        discovered_ip.alloc_type = IPADDRESS_TYPE.DISCOVERED
+        discovered_ip.save()
         script = factory.make_Script(parameters={
             'interface': {'type': 'interface'},
         })
@@ -587,8 +600,8 @@ class TestParametersForm(MAASServerTestCase):
             data={'interface': 'all'}, script=script, node=node)
         self.assertTrue(form.is_valid(), form.errors)
         input = form.cleaned_data['input']
-        self.assertEquals(node.interface_set.count(), len(input))
-        for interface in node.interface_set.all():
+        self.assertEquals(len(usable_interfaces), len(input))
+        for interface in usable_interfaces:
             for i in input:
                 if (str(interface.mac_address) ==
                         i['interface']['value']['mac_address']):
@@ -603,9 +616,11 @@ class TestParametersForm(MAASServerTestCase):
 
     def test__input_interface_all_only_includes_children(self):
         node = factory.make_Node(interface=False)
+        subnet = factory.make_Subnet()
         bond = factory.make_Interface(
-            node=node, iftype=INTERFACE_TYPE.BOND, parents=[
-                factory.make_Interface(node=node) for _ in range(2)])
+            node=node, iftype=INTERFACE_TYPE.BOND, subnet=subnet, parents=[
+                factory.make_Interface(node=node)
+                for _ in range(2)])
         script = factory.make_Script(parameters={
             'interface': {'type': 'interface'},
         })
@@ -624,8 +639,9 @@ class TestParametersForm(MAASServerTestCase):
 
     def test__input_interface_id(self):
         node = factory.make_Node()
+        subnet = factory.make_Subnet()
         for _ in range(3):
-            factory.make_Interface(node=node)
+            factory.make_Interface(node=node, subnet=subnet)
         script = factory.make_Script(parameters={
             'interface': {'type': 'interface'},
         })
@@ -681,10 +697,25 @@ class TestParametersForm(MAASServerTestCase):
                 'interface': ['Interface id does not exist'],
             }, form.errors)
 
+    def test__input_interface_id_errors_on_unconfigured_or_disabled(self):
+        node = factory.make_Node()
+        bad_interface = random.choice([
+            factory.make_Interface(node=node, enabled=False),
+            factory.make_Interface(node=node, link_connected=False),
+        ])
+        script = factory.make_Script(parameters={
+            'interface': {'type': 'interface'},
+        })
+        form = ParametersForm(
+            data={'interface': bad_interface.id},
+            script=script, node=node)
+        self.assertFalse(form.is_valid())
+
     def test__input_interface_list(self):
         node = factory.make_Node()
+        subnet = factory.make_Subnet()
         for _ in range(10):
-            factory.make_Interface(node=node)
+            factory.make_Interface(node=node, subnet=subnet)
         script = factory.make_Script(parameters={
             'interface': {'type': 'interface'},
         })
@@ -754,6 +785,20 @@ class TestParametersForm(MAASServerTestCase):
                     node.fqdn, node.system_id)],
             }, form.errors)
 
+    def test__input_interface_name_errors_on_unconfigured_or_disabled(self):
+        node = factory.make_Node()
+        bad_interface = random.choice([
+            factory.make_Interface(node=node, enabled=False),
+            factory.make_Interface(node=node, link_connected=False),
+        ])
+        script = factory.make_Script(parameters={
+            'interface': {'type': 'interface'},
+        })
+        form = ParametersForm(
+            data={'interface': bad_interface.name},
+            script=script, node=node)
+        self.assertFalse(form.is_valid())
+
     def test__input_url_validates_required(self):
         script = factory.make_Script(parameters={'url': {
             'type': 'url',