Fix SAN generic driver ssh whitespaced commands

SAN driver was sending commands over ssh with whitespaces between
every character resulting in commands like "u n a m e - s" instead
of "uname -s" (example from bug report).

As the bug reporter noted, the problem was that there were two joins
in the process of mounting the command. The first one got the
tuple and joined into a string. The second one joined a whitespace
between each character of the string.

Additionally, this behavior made utils.ssh_injection a bad check,
because it expected a list of args used in 'for arg in cmd_list' and
what it got was a single string. This resulted in the check being made
over the single characters instead of each arg.

All this is to explain why I chose to remove the first join instead of
the second one.

An unit test for this issue was created. SAN driver had no unit test.

Change-Id: I8e8624f2b82c26522936adf05e637a7bbe34a069
Closes-Bug: #1422604

diff --git a/cinder/tests/test_san.py b/cinder/tests/test_san.py
new file mode 100644 (file)
index 0000000..9516901
--- /dev/null
+++ b/cinder/tests/test_san.py
@@ -0,0 +1,55 @@
+#    Copyright 2015 OpenStack Foundation
+#
+#    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.
+#
+
+import mock
+
+from cinder import test
+from cinder.volume import configuration
+from cinder.volume.drivers.san import san
+
+
+class SanDriverTestCase(test.TestCase):
+    """Tests for SAN driver"""
+
+    def __init__(self, *args, **kwargs):
+        super(SanDriverTestCase, self).__init__(*args, **kwargs)
+
+    def setUp(self):
+        super(SanDriverTestCase, self).setUp()
+        self.configuration = mock.Mock(spec=configuration.Configuration)
+        self.configuration.san_is_local = False
+        self.configuration.san_ip = "10.0.0.1"
+        self.configuration.san_login = "admin"
+        self.configuration.san_password = "password"
+        self.configuration.san_ssh_port = 22
+        self.configuration.san_thin_provision = True
+        self.configuration.san_private_key = 'private_key'
+        self.configuration.ssh_min_pool_conn = 1
+        self.configuration.ssh_max_pool_conn = 5
+        self.configuration.ssh_conn_timeout = 30
+
+    @mock.patch.object(san.processutils, 'ssh_execute')
+    @mock.patch.object(san.ssh_utils, 'SSHPool')
+    @mock.patch.object(san.utils, 'check_ssh_injection')
+    def test_ssh_formatted_command(self, mock_check_ssh_injection,
+                                   mock_ssh_pool, mock_ssh_execute):
+        driver = san.SanDriver(configuration=self.configuration)
+        cmd_list = ['uname', '-s']
+        expected_cmd = 'uname -s'
+        driver.san_execute(*cmd_list)
+        # get the same used mocked item from the pool
+        with driver.sshpool.item() as ssh_item:
+            mock_ssh_execute.assert_called_with(ssh_item, expected_cmd,
+                                                check_exit_code=None)

diff --git a/cinder/volume/drivers/san/san.py b/cinder/volume/drivers/san/san.py
index f5adc86b8f30095d1a41550a2e79d5fda21e8680..cd564998f9a79582a0fa8e9ec84cc6593dd5e008 100644 (file)
--- a/cinder/volume/drivers/san/san.py
+++ b/cinder/volume/drivers/san/san.py
@@ -16,7 +16,7 @@
 Default Driver for san-stored volumes.
 
 The unique thing about a SAN is that we don't expect that we can run the volume
-controller on the SAN hardware.  We expect to access it over SSH or some API.
+controller on the SAN hardware. We expect to access it over SSH or some API.
 """
 
 import random
@@ -24,11 +24,11 @@ import random
 from eventlet import greenthread
 from oslo_concurrency import processutils
 from oslo_config import cfg
+from oslo_log import log as logging
 from oslo_utils import excutils
 
 from cinder import exception
 from cinder.i18n import _, _LE
-from cinder.openstack.common import log as logging
 from cinder import ssh_utils
 from cinder import utils
 from cinder.volume import driver
@@ -98,8 +98,7 @@ class SanDriver(driver.VolumeDriver):
             return utils.execute(*cmd, **kwargs)
         else:
             check_exit_code = kwargs.pop('check_exit_code', None)
-            command = ' '.join(cmd)
-            return self._run_ssh(command, check_exit_code)
+            return self._run_ssh(cmd, check_exit_code)
 
     def _run_ssh(self, cmd_list, check_exit_code=True, attempts=1):
         utils.check_ssh_injection(cmd_list)