launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12858
[Merge] lp:~rvb/maas/interface-activation-validation into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/interface-activation-validation into lp:maas with lp:~rvb/maas/nodegroup-ui as a prerequisite.
Commit message:
Add proper validation for the data of a nodegroupinterface.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/interface-activation-validation/+merge/127718
This branch adds proper validation for the data of a nodegroupinterface.
= Pre-imp =
This was discussed with Julian.
= Notes =
I've more the clean_management method on the model, where it belongs.
--
https://code.launchpad.net/~rvb/maas/interface-activation-validation/+merge/127718
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/interface-activation-validation into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-10-03 11:22:24 +0000
+++ src/maasserver/forms.py 2012-10-03 11:22:25 +0000
@@ -663,26 +663,6 @@
'ip_range_high',
)
- def clean_management(self):
- # XXX: rvb 2012-09-18 bug=1052339: Only one "managed" interface
- # is supported per NodeGroup.
- management = self.cleaned_data['management']
- if management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
- other_interfaces = NodeGroupInterface.objects.all()
- # Exclude context if it's already in the database.
- if self.instance and self.instance.id is not None:
- other_interfaces = (
- other_interfaces.exclude(id=self.instance.id))
- # Narrow down to the those that are managed.
- other_managed_interfaces = other_interfaces.exclude(
- management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
- if other_managed_interfaces.exists():
- raise ValidationError(
- {'management': [
- "Another managed interface already exists for this "
- "nodegroup."]})
- return management
-
def save(self, nodegroup=None, commit=True, *args, **kwargs):
interface = super(NodeGroupInterfaceForm, self).save(commit=False)
if nodegroup is not None:
@@ -769,7 +749,7 @@
if len(managed) > 1:
raise ValidationError(
"Only one managed interface can be configured for this "
- "nodegroup")
+ "cluster")
return interfaces
def save(self):
=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py 2012-10-03 11:22:24 +0000
+++ src/maasserver/models/nodegroup.py 2012-10-03 11:22:25 +0000
@@ -147,7 +147,7 @@
max_length=36, unique=True, null=False, blank=False, editable=True)
def __repr__(self):
- return "<NodeGroup %r>" % self.name
+ return "<NodeGroup %s>" % self.uuid
def accept(self):
"""Accept this nodegroup's enlistment."""
=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py 2012-10-03 11:22:24 +0000
+++ src/maasserver/models/nodegroupinterface.py 2012-10-03 11:22:25 +0000
@@ -36,6 +36,7 @@
IPAddress,
IPNetwork,
)
+from netaddr.core import AddrFormatError
class NodeGroupInterface(CleanSave, TimestampedModel):
@@ -89,9 +90,92 @@
return NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT[self.management]
def __repr__(self):
- return "<NodeGroupInterface %r,%s>" % (self.nodegroup, self.interface)
+ return "<NodeGroupInterface %s,%s>" % (
+ self.nodegroup.uuid, self.interface)
+
+ def clean_subnet_mask(self):
+ """subnet_mask cannot be empty if broadcast_ip is defined"""
+ if self.broadcast_ip and not self.subnet_mask:
+ raise ValidationError(
+ {
+ 'subnet_mask': [
+ "'Subnet mask' can't be empty if 'Broadcast ip' is "
+ "defined"]
+ })
+
+ def clean_broadcast_ip(self):
+ """broadcast_ip cannot be empty if subnet_mask is defined"""
+ if not self.broadcast_ip and self.subnet_mask:
+ raise ValidationError(
+ {
+ 'broadcast_ip': [
+ "'Broadcast ip' can't be empty if 'Subnet mask' is "
+ "defined"]
+ })
def clean_network(self):
+ """Validate that the network is valid.
+
+ This validates that the network defined by broadcast_ip and
+ subnet_mask is valid.
+ """
+ try:
+ self.network
+ except AddrFormatError, e:
+ # Technically, this should be a global error but it's
+ # more user-friendly to precisely point out where the error
+ # comes from.
+ raise ValidationError(
+ {
+ 'broadcast_ip': [e.message],
+ 'subnet_mask': [e.message],
+ })
+
+ raise ValidationError(e.message)
+
+ def clean_management(self):
+ # XXX: rvb 2012-09-18 bug=1052339: Only one "managed" interface
+ # is supported per NodeGroup.
+ check_other_interfaces = (
+ self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED and
+ self.nodegroup_id is not None)
+ if check_other_interfaces:
+ other_interfaces = self.nodegroup.nodegroupinterface_set.all()
+ # Exclude context if it's already in the database.
+ if self.id is not None:
+ other_interfaces = (
+ other_interfaces.exclude(id=self.id))
+ # Narrow down to the those that are managed.
+ other_managed_interfaces = other_interfaces.exclude(
+ management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+ if other_managed_interfaces.exists():
+ raise ValidationError(
+ {'management': [
+ "Another managed interface already exists for this "
+ "cluster."]})
+
+ def clean_network_config_if_managed(self):
+ # If management is not 'UNMANAGED', all the network information
+ # should be provided.
+ if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
+ mandatory_fields = [
+ 'interface',
+ 'broadcast_ip',
+ 'subnet_mask',
+ 'router_ip',
+ 'ip_range_low',
+ 'ip_range_high',
+ ]
+ errors = {}
+ for field in mandatory_fields:
+ if not getattr(self, field):
+ errors[field] = [
+ "That field cannot be empty (unless that interface is "
+ "'unmanaged')"]
+ if len(errors) != 0:
+ raise ValidationError(errors)
+
+ def clean_ips_in_network(self):
"""Ensure that the network settings are all congruent.
Specifically, it ensures that the interface address, router address,
@@ -115,6 +199,11 @@
if len(network_errors) != 0:
raise ValidationError(network_errors)
- def clean(self, *args, **kwargs):
- super(NodeGroupInterface, self).clean(*args, **kwargs)
+ def clean_fields(self, *args, **kwargs):
+ super(NodeGroupInterface, self).clean_fields(*args, **kwargs)
+ self.clean_broadcast_ip()
+ self.clean_subnet_mask()
self.clean_network()
+ self.clean_ips_in_network()
+ self.clean_management()
+ self.clean_network_config_if_managed()
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-10-03 09:16:12 +0000
+++ src/maasserver/tests/test_api.py 2012-10-03 11:22:25 +0000
@@ -3040,7 +3040,7 @@
{'interfaces':
[
"Only one managed interface can be configured for "
- "this nodegroup"
+ "this cluster"
]},
),
(response.status_code, json.loads(response.content)))
=== modified file 'src/maasserver/tests/test_fields.py'
--- src/maasserver/tests/test_fields.py 2012-09-30 21:19:08 +0000
+++ src/maasserver/tests/test_fields.py 2012-10-03 11:22:25 +0000
@@ -106,10 +106,10 @@
nodegroup = factory.make_node_group(network=IPNetwork("10/8"))
NodeGroupInterface.objects.create(
nodegroup=nodegroup, ip='10.0.0.2', subnet_mask='255.0.0.0',
- interface='eth71')
+ broadcast_ip='10.0.0.1', interface='eth71')
NodeGroupInterface.objects.create(
nodegroup=nodegroup, ip='10.0.0.3', subnet_mask='255.0.0.0',
- interface='eth72')
+ broadcast_ip='10.0.0.2', interface='eth72')
self.assertEqual(nodegroup, NodeGroupFormField().clean('10.0.0.9'))
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-10-02 13:55:22 +0000
+++ src/maasserver/tests/test_forms.py 2012-10-03 11:22:25 +0000
@@ -665,11 +665,11 @@
def test_NodeGroupInterfaceForm_can_save_fields_being_None(self):
settings = make_interface_settings()
+ settings['management'] = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
for field_name in nullable_fields:
del settings[field_name]
form = NodeGroupInterfaceForm(data=settings)
- nodegroup = factory.make_node_group(
- management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+ nodegroup = factory.make_node_group()
interface = form.save(nodegroup=nodegroup)
field_values = [
getattr(interface, field_name) for field_name in nullable_fields]
@@ -782,7 +782,7 @@
data={'name': name, 'uuid': uuid, 'interfaces': interfaces})
self.assertFalse(form.is_valid())
self.assertIn(
- "Only one managed interface can be configured for this nodegroup",
+ "Only one managed interface can be configured for this cluster",
form._errors['interfaces'][0])
def test_NodeGroupWithInterfacesForm_creates_multiple_interfaces(self):
=== modified file 'src/maasserver/tests/test_nodegroupinterface.py'
--- src/maasserver/tests/test_nodegroupinterface.py 2012-10-03 11:22:24 +0000
+++ src/maasserver/tests/test_nodegroupinterface.py 2012-10-03 11:22:25 +0000
@@ -12,6 +12,7 @@
__metaclass__ = type
__all__ = []
+from django.core.exceptions import ValidationError
from maasserver.enum import (
NODEGROUP_STATUS,
NODEGROUPINTERFACE_MANAGEMENT,
@@ -60,3 +61,83 @@
self.assertEqual(
NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT[interface.management],
interface.display_management())
+
+ def test_clean_ips_in_network_validates_IP(self):
+ network = IPNetwork('192.168.0.3/24')
+ checked_fields = [
+ 'ip',
+ 'router_ip',
+ 'ip_range_low',
+ 'ip_range_high',
+ ]
+ for field in checked_fields:
+ nodegroup = factory.make_node_group(network=network)
+ interface = nodegroup.get_managed_interface()
+ ip = '192.168.2.1'
+ setattr(interface, field, '192.168.2.1')
+ message = (
+ "%s not in the %s network" % (ip, '192.168.0.255/24'))
+ exception = self.assertRaises(
+ ValidationError, interface.full_clean)
+ self.assertEqual(
+ {field: [message]}, exception.message_dict)
+
+ def test_clean_broadcast_ip(self):
+ nodegroup = factory.make_node_group()
+ interface = nodegroup.get_managed_interface()
+ interface.broadcast_ip = ''
+ message = (
+ "'Broadcast ip' can't be empty if 'Subnet mask' is defined")
+ exception = self.assertRaises(ValidationError, interface.full_clean)
+ self.assertEqual(
+ {'broadcast_ip': [message]}, exception.message_dict)
+
+ def test_clean_subnet_mask(self):
+ nodegroup = factory.make_node_group()
+ interface = nodegroup.get_managed_interface()
+ interface.subnet_mask = ''
+ message = (
+ "'Subnet mask' can't be empty if 'Broadcast ip' is defined")
+ exception = self.assertRaises(ValidationError, interface.full_clean)
+ self.assertEqual(
+ {'subnet_mask': [message]}, exception.message_dict)
+
+ def test_clean_network(self):
+ nodegroup = factory.make_node_group(
+ network=IPNetwork('192.168.0.3/24'))
+ interface = nodegroup.get_managed_interface()
+ # Set a bogus subnet mask.
+ interface.subnet_mask = '0.9.0.4'
+ message = 'invalid IPNetwork 192.168.0.255/0.9.0.4'
+ exception = self.assertRaises(ValidationError, interface.full_clean)
+ self.assertEqual(
+ {
+ 'subnet_mask': [message],
+ 'broadcast_ip': [message],
+ },
+ exception.message_dict)
+
+ def test_clean_network_config_if_managed(self):
+ network = IPNetwork('192.168.0.3/24')
+ checked_fields = [
+ 'interface',
+ # When broadcast_ip or subnet_mask are empty, it gets caught
+ # by clean_network.
+ #'broadcast_ip',
+ #'subnet_mask',
+ 'router_ip',
+ 'ip_range_low',
+ 'ip_range_high',
+ ]
+ for field in checked_fields:
+ nodegroup = factory.make_node_group(
+ network=network,
+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+ interface = nodegroup.get_managed_interface()
+ setattr(interface, field, '')
+ exception = self.assertRaises(
+ ValidationError, interface.full_clean)
+ message = (
+ "That field cannot be empty (unless that interface is "
+ "'unmanaged')")
+ self.assertEqual({field: [message]}, exception.message_dict)
Follow ups
