Merge lp:~lamont/maas/bug-1543707 into lp:~maas-committers/maas/trunk

[LaMont Jones]

Disallow duplicate space/fabric names; disallow spaces; migrate as needed.

Disallow duplicate space/fabric names; disallow spaces; migrate as needed.

[LaMont Jones (lamont)]

Testing this, I see errors logged because of duplicate space key 0, and 3 of 4 regiond's dead. I need to figure out the right way to deal with that.

[Blake Rouse (blake-rouse)]

Overall this looks good. I think the migration can be more DRY.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~lamont/maas/bug-1543707 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://security.ubuntu.com/ubuntu xenial-security InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease [95.6 kB]
Hit:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main Sources [1,108 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe Sources [7,515 kB]
Get:7 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main amd64 Packages [1,435 kB]
Get:8 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main Translation-en [731 kB]
Get:9 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe amd64 Packages [7,242 kB]
Get:10 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe Translation-en [4,187 kB]
Fetched 22.3 MB in 4s (4,991 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-coverage python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-mock python3-netaddr python3-netifaces python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu1).
archdetect-deb is already the newest version (1.117ubuntu1).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
bind9 is already the newest version (1:9.10.3.dfsg.P4-5).
bind9utils is already the newest version (1:9.10.3.dfsg.P4-5).
build-essential is already the newest version (12.1ubuntu2).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ub...

=== modified file 'docs/changelog.rst'
--- docs/changelog.rst	2016-04-06 13:45:19 +0000
+++ docs/changelog.rst	2016-04-06 17:51:57 +0000
@@ -13,6 +13,10 @@
 
 LP: #1561733    [2.0a3] MAAS no longer detects external DHCP servers
 
+LP: #1543707    MAAS 1.9+ should not allow whitespace characters in space names
+
+LP: #1543968    MAAS 1.9.0 allows non-unique space names
+
 
 2.0.0 (beta1)
 =============

=== added file 'src/maasserver/migrations/builtin/maasserver/0051_space_fabric_unique.py'
--- src/maasserver/migrations/builtin/maasserver/0051_space_fabric_unique.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/migrations/builtin/maasserver/0051_space_fabric_unique.py	2016-04-06 17:51:57 +0000
@@ -0,0 +1,80 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from random import randint
+import re
+
+from django.db import (
+    migrations,
+    models,
+)
+import maasserver.models.fabric
+import maasserver.models.space
+
+
+def fix_model(apps, schema_editor, model_name, prefix):
+    cls = apps.get_model("maasserver", model_name)
+    # Pass 1: assign values to all of the null/empty names, and identify the
+    # duplicates.
+    duplicates = set()
+    for instance in cls.objects.all():
+        if instance.name is None or instance.name == '':
+            instance.name = '%s-%d' % (prefix, instance.id)
+            instance.save()
+        elif cls.objects.filter(name=instance.name).count() > 1:
+            # We know that anything we would be renaming None to is reserved to
+            # only our instance.  That means that we don't have to do all the
+            # renmaes before we check for duplicates.
+            duplicates.add(instance)
+    # Pass 2: Rename anything that was a duplicate.  Because blanks are now
+    # illegal in the name (and should always have been) any instance that we
+    # need to rename to fix duplication will need to have blanks replaced with
+    # hypens.
+    # No preference is given to any instance: all duplicates will get a new
+    # name.
+    for instance in duplicates:
+        # Get rid of the instance, if any.
+        instance.name = re.sub(' ', '-', instance.name)
+        # First, try ${NAME}-${ID}, then go with ${NAME}-${ID}-${RANDOM}
+        name = '%s-%d' % (instance.name, instance.id)
+        if cls.objects.filter(name=name).exists():
+            retries = 20
+            while retries > 0 and cls.objects.filter(name=name).exists():
+                name = '%s-%d-%d' % (
+                    instance.name, instance.id, randint(1, 100000))
+                retries -= 1
+            # We tried, and we give up.  Just change it to the default name.
+            # Note that to get here, we tried ${NAME}-${ID} and 20 other
+            # randomly related names and they _ALL_ had existing collisions in
+            # the data base.
+            if retries == 0:
+                name = '%s-%d' % (prefix, instance.id)
+        instance.name = name
+        instance.save()
+
+def fix_space_names(apps, schema_editor):
+    fix_model(apps, schema_editor, "Space", "space")
+
+def fix_fabric_names(apps, schema_editor):
+    fix_model(apps, schema_editor, "Fabric", "fabric")
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('maasserver', '0050_modify_external_dhcp_on_vlan'),
+    ]
+
+    operations = [
+        migrations.RunPython(fix_space_names),
+        migrations.RunPython(fix_fabric_names),
+        migrations.AlterField(
+            model_name='space',
+            name='name',
+            field=models.CharField(null=True, blank=True, unique=True, validators=[maasserver.models.space.validate_space_name], max_length=256),
+        ),
+        migrations.AlterField(
+            model_name='fabric',
+            name='name',
+            field=models.CharField(null=True, blank=True, unique=True, validators=[maasserver.models.fabric.validate_fabric_name], max_length=256),
+        ),
+    ]

=== modified file 'src/maasserver/models/fabric.py'
--- src/maasserver/models/fabric.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/fabric.py	2016-04-06 17:51:57 +0000
@@ -152,8 +152,10 @@
 
     objects = FabricManager()
 
+    # We don't actually allow blank or null name, but that is enforced in
+    # clean() and save().
     name = CharField(
-        max_length=256, editable=True, null=True, blank=True, unique=False,
+        max_length=256, editable=True, null=True, blank=True, unique=True,
         validators=[validate_fabric_name])
 
     class_type = CharField(
@@ -196,19 +198,32 @@
             name=DEFAULT_VLAN_NAME, vid=DEFAULT_VID, fabric=self)
 
     def save(self, *args, **kwargs):
+        # Name will get set by clean_name() if None or empty, and there is an
+        # id. We just need to handle names here for creation.
         created = self.id is None
-        super(Fabric, self).save(*args, **kwargs)
+        super().save(*args, **kwargs)
+        if self.name is None or self.name == '':
+            # If we got here, then we have a newly created fabric that needs a
+            # default name.
+            self.name = "fabric-%d" % self.id
+            fabric = Fabric.objects.get(id=self.id)
+            fabric.save()
         # Create default VLAN if this is a fabric creation.
         if created:
             self._create_default_vlan()
 
     def clean_name(self):
-        reserved = re.compile('fabric-\d+$')
-        if self.name is not None and reserved.search(self.name):
-            if (self.id is None or self.name != 'fabric-%d' % self.id):
-                raise ValidationError(
-                    {'name': ["Reserved fabric name."]})
+        reserved = re.compile('^fabric-\d+$')
+        if self.name is not None and self.name != '':
+            if reserved.search(self.name):
+                if (self.id is None or self.name != 'fabric-%d' % self.id):
+                    raise ValidationError(
+                        {'name': ["Reserved fabric name."]})
+        elif self.id is not None:
+            # Since we are not creating the fabric, force the (null or empty)
+            # name to be the default name.
+            self.name = "fabric-%d" % self.id
 
     def clean(self, *args, **kwargs):
-        super(Fabric, self).clean(*args, **kwargs)
+        super().clean(*args, **kwargs)
         self.clean_name()

=== modified file 'src/maasserver/models/space.py'
--- src/maasserver/models/space.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/models/space.py	2016-04-06 17:51:57 +0000
@@ -30,7 +30,7 @@
     """Django validator: `value` must be either `None`, or valid."""
     if value is None:
         return
-    namespec = re.compile('^[ \w-]+$')
+    namespec = re.compile('^[\w-]+$')
     if not namespec.search(value):
         raise ValidationError("Invalid space name: %s." % value)
 
@@ -125,8 +125,10 @@
 
     objects = SpaceManager()
 
+    # We don't actually allow blank or null name, but that is enforced in
+    # clean() and save().
     name = CharField(
-        max_length=256, editable=True, null=True, blank=True, unique=False,
+        max_length=256, editable=True, null=True, blank=True, unique=True,
         validators=[validate_space_name])
 
     def __str__(self):
@@ -144,11 +146,27 @@
             return "space-%s" % self.id
 
     def clean_name(self):
-        reserved = re.compile('space-\d+$')
-        if self.name is not None and reserved.search(self.name):
-            if (self.id is None or self.name != 'space-%d' % self.id):
-                raise ValidationError(
-                    {'name': ["Reserved space name."]})
+        reserved = re.compile('^space-\d+$')
+        if self.name is not None and self.name != '':
+            if reserved.search(self.name):
+                if (self.id is None or self.name != 'space-%d' % self.id):
+                    raise ValidationError(
+                        {'name': ["Reserved space name."]})
+        elif self.id is not None:
+            # Since we are not creating the space, force the (null or empty)
+            # name to be the default name.
+            self.name = "space-%d" % self.id
+
+    def save(self, *args, **kwargs):
+        # Name will get set by clean_name() if None or empty, and there is an
+        # id. We just need to handle names here for creation.
+        super().save(*args, **kwargs)
+        if self.name is None or self.name == '':
+            # If we got here, then we have a newly created space that needs a
+            # default name.
+            self.name = "space-%d" % self.id
+            space = Space.objects.get(id=self.id)
+            space.save()
 
     def clean(self, *args, **kwargs):
         super(Space, self).clean(*args, **kwargs)

=== modified file 'src/maasserver/models/tests/test_fabric.py'
--- src/maasserver/models/tests/test_fabric.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/tests/test_fabric.py	2016-04-06 17:51:57 +0000
@@ -164,6 +164,27 @@
         default_fabric = Fabric.objects.get_default_fabric()
         self.assertEqual(0, default_fabric.id)
         self.assertEqual(DEFAULT_FABRIC_NAME, default_fabric.get_name())
+        self.assertEqual(DEFAULT_FABRIC_NAME, default_fabric.name)
+
+    def test_create_sets_name(self):
+        fabric = Fabric.objects.create(name=None)
+        self.assertEqual("fabric-%d" % fabric.id, fabric.name)
+
+    def test_create_does_not_override_name(self):
+        name = factory.make_name()
+        fabric = factory.make_Fabric(name=name)
+        self.assertEqual(name, fabric.name)
+
+    def test_nonreserved_name_does_not_raise_exception(self):
+        fabric = factory.make_Fabric(name='myfabric-33')
+        self.assertEqual("myfabric-33", fabric.name)
+
+    def test_rejects_duplicate_names(self):
+        fabric1 = factory.make_Fabric()
+        self.assertRaises(
+            ValidationError,
+            factory.make_Fabric,
+            name=fabric1.name)
 
     def test_get_default_fabric_is_idempotent(self):
         default_fabric = Fabric.objects.get_default_fabric()

=== modified file 'src/maasserver/models/tests/test_space.py'
--- src/maasserver/models/tests/test_space.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/models/tests/test_space.py	2016-04-06 17:51:57 +0000
@@ -139,6 +139,7 @@
         default_space = Space.objects.get_default_space()
         self.assertEqual(0, default_space.id)
         self.assertEqual(DEFAULT_SPACE_NAME, default_space.get_name())
+        self.assertEqual(DEFAULT_SPACE_NAME, default_space.name)
 
     def test_invalid_name_raises_exception(self):
         self.assertRaises(
@@ -152,6 +153,26 @@
             factory.make_Space,
             name='space-1999')
 
+    def test_create_sets_name(self):
+        space = Space.objects.create(name=None)
+        self.assertEqual("space-%d" % space.id, space.name)
+
+    def test_create_does_not_override_name(self):
+        name = factory.make_name()
+        space = factory.make_Space(name=name)
+        self.assertEqual(name, space.name)
+
+    def test_nonreserved_name_does_not_raise_exception(self):
+        space = factory.make_Space(name='myspace-1999')
+        self.assertEqual("myspace-1999", space.name)
+
+    def test_rejects_duplicate_names(self):
+        space1 = factory.make_Space()
+        self.assertRaises(
+            ValidationError,
+            factory.make_Space,
+            name=space1.name)
+
     def test_get_default_space_is_idempotent(self):
         default_space = Space.objects.get_default_space()
         default_space2 = Space.objects.get_default_space()