Log warning if ipmi_username/ipmi_password missing

Even though an empty ipmi_username and/or ipmi_password is valid, it is
often an error in configuration. Log a warning if either value is
missing.

Add unit tests showing that an unspecified username or an empty string
username are treated the same and will cause no user to be specified.

Add unit tests showing that an unspecified password or an empty string
password are treated the same and will cause a NULL (\0) password to be
used.

Change-Id: I384e210da3216fb9335c4c782d69e606026d4b97

ironic/drivers/modules/ipmitool.py

@@ -269,6 +269,13 @@ def _parse_driver_info(node):
     protocol_version = str(info.get('ipmi_protocol_version', '2.0'))
     force_boot_device = info.get('ipmi_force_boot_device', False)
 
+    if not username:
+        LOG.warning(_LW('ipmi_username is not defined or empty for node %s: '
+                        'NULL user will be utilized.') % node.uuid)
+    if not password:
+        LOG.warning(_LW('ipmi_password is not defined or empty for node %s: '
+                        'NULL password will be utilized.') % node.uuid)
+
     if protocol_version not in VALID_PROTO_VERSIONS:
         valid_versions = ', '.join(VALID_PROTO_VERSIONS)
         raise exception.InvalidParameterValue(_(

ironic/tests/unit/drivers/modules/test_ipmitool.py

@@ -617,6 +617,33 @@ class IPMIToolPrivateMethodTestCase(db_base.DbTestCase):
         ret = ipmi._parse_driver_info(node)
         self.assertEqual(623, ret['dest_port'])
 
+    @mock.patch.object(ipmi.LOG, 'warning', spec_set=True, autospec=True)
+    def test__parse_driver_info_undefined_credentials(
+            self, mock_log, mock_sleep):
+        info = dict(INFO_DICT)
+        del info['ipmi_username']
+        del info['ipmi_password']
+        node = obj_utils.get_test_node(self.context, driver_info=info)
+        ipmi._parse_driver_info(node)
+        calls = [
+            mock.call(u'ipmi_username is not defined or empty for node '
+                      u'1be26c0b-03f2-4d2e-ae87-c02d7f33c123: NULL user will '
+                      u'be utilized.'),
+            mock.call(u'ipmi_password is not defined or empty for node '
+                      u'1be26c0b-03f2-4d2e-ae87-c02d7f33c123: NULL password '
+                      u'will be utilized.'),
+        ]
+        mock_log.assert_has_calls(calls)
+
+    @mock.patch.object(ipmi.LOG, 'warning', spec_set=True, autospec=True)
+    def test__parse_driver_info_have_credentials(
+            self, mock_log, mock_sleep):
+        """Ensure no warnings generated if have credentials"""
+        info = dict(INFO_DICT)
+        node = obj_utils.get_test_node(self.context, driver_info=info)
+        ipmi._parse_driver_info(node)
+        self.assertFalse(mock_log.called)
+
     @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
@@ -845,6 +872,8 @@ class IPMIToolPrivateMethodTestCase(db_base.DbTestCase):
     @mock.patch.object(utils, 'execute', autospec=True)
     def test__exec_ipmitool_without_username(
             self, mock_exec, mock_pwf, mock_support, mock_sleep):
+        # An undefined username is treated the same as an empty username and
+        # will cause no user (-U) to be specified.
         self.info['username'] = None
         pw_file_handle = tempfile.NamedTemporaryFile()
         pw_file = pw_file_handle.name
@@ -866,6 +895,94 @@ class IPMIToolPrivateMethodTestCase(db_base.DbTestCase):
         self.assertTrue(mock_pwf.called)
         mock_exec.assert_called_once_with(*args)
 
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    @mock.patch.object(ipmi, '_make_password_file', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__exec_ipmitool_with_empty_username(
+            self, mock_exec, mock_pwf, mock_support, mock_sleep):
+        # An empty username is treated the same as an undefined username and
+        # will cause no user (-U) to be specified.
+        self.info['username'] = ""
+        pw_file_handle = tempfile.NamedTemporaryFile()
+        pw_file = pw_file_handle.name
+        file_handle = open(pw_file, "w")
+        args = [
+            'ipmitool',
+            '-I', 'lanplus',
+            '-H', self.info['address'],
+            '-L', self.info['priv_level'],
+            '-f', file_handle,
+            'A', 'B', 'C',
+        ]
+
+        mock_support.return_value = False
+        mock_pwf.return_value = file_handle
+        mock_exec.return_value = (None, None)
+        ipmi._exec_ipmitool(self.info, 'A B C')
+        mock_support.assert_called_once_with('timing')
+        self.assertTrue(mock_pwf.called)
+        mock_exec.assert_called_once_with(*args)
+
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    @mock.patch.object(ipmi, '_make_password_file', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__exec_ipmitool_without_password(
+            self, mock_exec, mock_pwf, mock_support, mock_sleep):
+        # An undefined password is treated the same as an empty password and
+        # will cause a NULL (\0) password to be used"""
+        self.info['password'] = None
+        pw_file_handle = tempfile.NamedTemporaryFile()
+        pw_file = pw_file_handle.name
+        file_handle = open(pw_file, "w")
+        args = [
+            'ipmitool',
+            '-I', 'lanplus',
+            '-H', self.info['address'],
+            '-L', self.info['priv_level'],
+            '-U', self.info['username'],
+            '-f', file_handle,
+            'A', 'B', 'C',
+        ]
+
+        mock_support.return_value = False
+        mock_pwf.return_value = file_handle
+        mock_exec.return_value = (None, None)
+        ipmi._exec_ipmitool(self.info, 'A B C')
+        mock_support.assert_called_once_with('timing')
+        self.assertTrue(mock_pwf.called)
+        mock_exec.assert_called_once_with(*args)
+        mock_pwf.assert_called_once_with('\0')
+
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    @mock.patch.object(ipmi, '_make_password_file', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__exec_ipmitool_with_empty_password(
+            self, mock_exec, mock_pwf, mock_support, mock_sleep):
+        # An empty password is treated the same as an undefined password and
+        # will cause a NULL (\0) password to be used"""
+        self.info['password'] = ""
+        pw_file_handle = tempfile.NamedTemporaryFile()
+        pw_file = pw_file_handle.name
+        file_handle = open(pw_file, "w")
+        args = [
+            'ipmitool',
+            '-I', 'lanplus',
+            '-H', self.info['address'],
+            '-L', self.info['priv_level'],
+            '-U', self.info['username'],
+            '-f', file_handle,
+            'A', 'B', 'C',
+        ]
+
+        mock_support.return_value = False
+        mock_pwf.return_value = file_handle
+        mock_exec.return_value = (None, None)
+        ipmi._exec_ipmitool(self.info, 'A B C')
+        mock_support.assert_called_once_with('timing')
+        self.assertTrue(mock_pwf.called)
+        mock_exec.assert_called_once_with(*args)
+        mock_pwf.assert_called_once_with('\0')
+
     @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)