Fix is_hostname_safe for RFC compliance

In the move from Node name supporting just a hostname to now
supporting a FQDN, we missed a few changes to keep this function
being RFC compliant.  This patch enhances the code and tests for
this, and migrates the database to the longer name field format.

Change-Id: Ib75c8b138d7165aedc74efead80d3fb755cab87b
Closes-Bug: #1433832

ironic/common/utils.py

@@ -184,16 +184,26 @@ def is_hostname_safe(hostname):
         * http://en.wikipedia.org/wiki/Hostname
         * http://tools.ietf.org/html/rfc952
         * http://tools.ietf.org/html/rfc1123
-
+    Allowing for hostnames, and hostnames + domains.
-    Also allow "." because what kind of hostname doesn't allow that.
 
     :param hostname: The hostname to be validated.
     :returns: True if valid. False if not.
 
     """
-    m = '^[a-z0-9]([a-z0-9\-\.]{0,61}[a-z0-9])?$'
+    if not isinstance(hostname, six.string_types) or len(hostname) > 255:
-    return (isinstance(hostname, six.string_types) and
+        return False
-           (re.match(m, hostname) is not None))
+
+    # Periods on the end of a hostname are ok, but complicates the
+    # regex so we'll do this manually
+    if hostname.endswith('.'):
+        hostname = hostname[:-1]
+
+    host = '[a-z0-9]([a-z0-9\-]{0,61}[a-z0-9])?'
+    domain = '[a-z0-9\-_]{0,62}[a-z0-9]'
+
+    m = '^' + host + '(\.' + domain + ')*$'
+
+    return re.match(m, hostname) is not None
 
 
 def validate_and_normalize_mac(address):

ironic/db/sqlalchemy/alembic/versions/2fb93ffd2af1_increase_node_name_length.py

@@ -0,0 +1,42 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""increase-node-name-length
+
+Revision ID: 2fb93ffd2af1
+Revises: 4f399b21ae71
+Create Date: 2015-03-18 17:08:11.470791
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '2fb93ffd2af1'
+down_revision = '4f399b21ae71'
+
+
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy.dialects import mysql
+
+
+def upgrade():
+    op.alter_column('nodes', 'name',
+               existing_type=mysql.VARCHAR(length=63),
+               type_=sa.String(length=255),
+               existing_nullable=True)
+
+
+def downgrade():
+    op.alter_column('nodes', 'name',
+               existing_type=sa.String(length=255),
+               type_=mysql.VARCHAR(length=63),
+               existing_nullable=True)

ironic/db/sqlalchemy/models.py

@@ -154,7 +154,7 @@ class Node(Base):
     #             filter on it more efficiently, even though it is
     #             user-settable, and would otherwise be in node.properties.
     instance_uuid = Column(String(36), nullable=True)
-    name = Column(String(63), nullable=True)
+    name = Column(String(255), nullable=True)
     chassis_id = Column(Integer, ForeignKey('chassis.id'), nullable=True)
     power_state = Column(String(15), nullable=True)
     target_power_state = Column(String(15), nullable=True)

ironic/tests/db/sqlalchemy/test_migrations.py

@@ -374,6 +374,12 @@ class MigrationCheckersMixin(object):
         self.assertIsInstance(nodes.c.clean_step.type,
                               sqlalchemy.types.String)
 
+    def _check_2fb93ffd2af1(self, engine, data):
+        # TODO(mrda): Currently database migration tests aren't running
+        # But once that has been resolved, add a new db migration test here
+        # for increasing node name length. See bug 1438531
+        pass
+
     def test_upgrade_and_version(self):
         with patch_with_engine(self.engine):
             self.migration_api.upgrade('head')

ironic/tests/test_utils.py

@@ -342,6 +342,7 @@ class GenericUtilsTestCase(base.TestCase):
         self.assertFalse(utils.is_hostname_safe('-spam'))
         self.assertFalse(utils.is_hostname_safe('spam-'))
         self.assertTrue(utils.is_hostname_safe('spam-eggs'))
+        self.assertFalse(utils.is_hostname_safe('spam_eggs'))
         self.assertFalse(utils.is_hostname_safe('spam eggs'))
         self.assertTrue(utils.is_hostname_safe('spam.eggs'))
         self.assertTrue(utils.is_hostname_safe('9spam'))
@@ -360,6 +361,18 @@ class GenericUtilsTestCase(base.TestCase):
         # Need to ensure a binary response for success or fail
         self.assertIsNotNone(utils.is_hostname_safe('spam'))
         self.assertIsNotNone(utils.is_hostname_safe('-spam'))
+        self.assertTrue(utils.is_hostname_safe('www.rackspace.com'))
+        self.assertTrue(utils.is_hostname_safe('www.rackspace.com.'))
+        self.assertTrue(utils.is_hostname_safe('http._sctp.www.example.com'))
+        self.assertTrue(utils.is_hostname_safe('mail.pets_r_us.net'))
+        self.assertTrue(utils.is_hostname_safe('mail-server-15.my_host.org'))
+        self.assertFalse(utils.is_hostname_safe('www.nothere.com_'))
+        self.assertFalse(utils.is_hostname_safe('www.nothere_.com'))
+        self.assertFalse(utils.is_hostname_safe('www..nothere.com'))
+        long_str = 'a' * 63 + '.' + 'b' * 63 + '.' + 'c' * 63 + '.' + 'd' * 63
+        self.assertTrue(utils.is_hostname_safe(long_str))
+        self.assertFalse(utils.is_hostname_safe(long_str + '.'))
+        self.assertFalse(utils.is_hostname_safe('a' * 255))
 
     def test_validate_and_normalize_mac(self):
         mac = 'AA:BB:CC:DD:EE:FF'