Allow spaces in host names in the storwize driver

Storwize/SVC naming rules allow host objects to have spaces in their
names. The SSH injection filter will currently reject such names, and so
we surround the names with quotes. Please note that this doesn't allow
to have an arbitrary character in there except a space (anything else
will be cleaned up by the driver or rejected by the SSH injection filter
up the call chain).

Change-Id: If9aec40fe34293031f08d759dd930d73ead456f5
Closes-Bug: #1270204

diff --git a/cinder/tests/test_storwize_svc.py b/cinder/tests/test_storwize_svc.py
index 2d8719f2d2db81b7ca991963998097fedfeebacc..af3a14ed603834bd7d6f431abcc963cbcf3ad7d4 100644 (file)
--- a/cinder/tests/test_storwize_svc.py
+++ b/cinder/tests/test_storwize_svc.py
@@ -533,7 +533,7 @@ port_speed!N/A
         volume_info['uid'] = ('ABCDEF' * 3) + ('0' * 14) + volume_info['id']
 
         if 'name' in kwargs:
-            volume_info['name'] = kwargs['name'].strip('\'\'')
+            volume_info['name'] = kwargs['name'].strip('\'\"')
         else:
             volume_info['name'] = 'vdisk' + volume_info['id']
 
@@ -603,7 +603,7 @@ port_speed!N/A
 
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol_name = kwargs['obj'].strip('\'\'')
+        vol_name = kwargs['obj'].strip('\'\"')
 
         if vol_name not in self._volumes_list:
             return self._errors['CMMVC5753E']
@@ -623,7 +623,7 @@ port_speed!N/A
     def _cmd_expandvdisksize(self, **kwargs):
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol_name = kwargs['obj'].strip('\'\'')
+        vol_name = kwargs['obj'].strip('\'\"')
 
         # Assume unit is gb
         if 'size' not in kwargs:
@@ -816,7 +816,7 @@ port_speed!N/A
     def _cmd_addhostport(self, **kwargs):
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        host_name = kwargs['obj'].strip('\'\'')
+        host_name = kwargs['obj'].strip('\'\"')
 
         if host_name not in self._hosts_list:
             return self._errors['CMMVC5753E']
@@ -828,11 +828,11 @@ port_speed!N/A
     def _cmd_chhost(self, **kwargs):
         if 'chapsecret' not in kwargs:
             return self._errors['CMMVC5707E']
-        secret = kwargs['obj'].strip('\'\'')
+        secret = kwargs['obj'].strip('\'\"')
 
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        host_name = kwargs['obj'].strip('\'\'')
+        host_name = kwargs['obj'].strip('\'\"')
 
         if host_name not in self._hosts_list:
             return self._errors['CMMVC5753E']
@@ -845,7 +845,7 @@ port_speed!N/A
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
 
-        host_name = kwargs['obj'].strip('\'\'')
+        host_name = kwargs['obj'].strip('\'\"')
         if host_name not in self._hosts_list:
             return self._errors['CMMVC5753E']
 
@@ -875,9 +875,10 @@ port_speed!N/A
             else:
                 return ('', '')
         else:
-            if kwargs['obj'] not in self._hosts_list:
+            host_name = kwargs['obj'].strip('\'\"')
+            if host_name not in self._hosts_list:
                 return self._errors['CMMVC5754E']
-            host = self._hosts_list[kwargs['obj']]
+            host = self._hosts_list[host_name]
             rows = []
             rows.append(['id', host['id']])
             rows.append(['name', host['host_name']])
@@ -931,15 +932,15 @@ port_speed!N/A
 
         if 'host' not in kwargs:
             return self._errors['CMMVC5707E']
-        mapping_info['host'] = kwargs['host'].strip('\'\'')
+        mapping_info['host'] = kwargs['host'].strip('\'\"')
 
         if 'scsi' not in kwargs:
             return self._errors['CMMVC5707E']
-        mapping_info['lun'] = kwargs['scsi'].strip('\'\'')
+        mapping_info['lun'] = kwargs['scsi'].strip('\'\"')
 
         if 'obj' not in kwargs:
             return self._errors['CMMVC5707E']
-        mapping_info['vol'] = kwargs['obj'].strip('\'\'')
+        mapping_info['vol'] = kwargs['obj'].strip('\'\"')
 
         if mapping_info['vol'] not in self._volumes_list:
             return self._errors['CMMVC5753E']
@@ -967,11 +968,11 @@ port_speed!N/A
     def _cmd_rmvdiskhostmap(self, **kwargs):
         if 'host' not in kwargs:
             return self._errors['CMMVC5707E']
-        host = kwargs['host'].strip('\'\'')
+        host = kwargs['host'].strip('\'\"')
 
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol = kwargs['obj'].strip('\'\'')
+        vol = kwargs['obj'].strip('\'\"')
 
         mapping_ids = []
         for v in self._mappings_list.itervalues():
@@ -992,7 +993,7 @@ port_speed!N/A
 
     # List information about host->vdisk mappings
     def _cmd_lshostvdiskmap(self, **kwargs):
-        host_name = kwargs['obj']
+        host_name = kwargs['obj'].strip('\'\"')
 
         if host_name not in self._hosts_list:
             return self._errors['CMMVC5754E']
@@ -1044,13 +1045,13 @@ port_speed!N/A
 
         if 'source' not in kwargs:
             return self._errors['CMMVC5707E']
-        source = kwargs['source'].strip('\'\'')
+        source = kwargs['source'].strip('\'\"')
         if source not in self._volumes_list:
             return self._errors['CMMVC5754E']
 
         if 'target' not in kwargs:
             return self._errors['CMMVC5707E']
-        target = kwargs['target'].strip('\'\'')
+        target = kwargs['target'].strip('\'\"')
         if target not in self._volumes_list:
             return self._errors['CMMVC5754E']
 
@@ -1211,8 +1212,8 @@ port_speed!N/A
     def _cmd_migratevdisk(self, **kwargs):
         if 'mdiskgrp' not in kwargs or 'vdisk' not in kwargs:
             return self._errors['CMMVC5707E']
-        mdiskgrp = kwargs['mdiskgrp'].strip('\'\'')
-        vdisk = kwargs['vdisk'].strip('\'\'')
+        mdiskgrp = kwargs['mdiskgrp'].strip('\'\"')
+        vdisk = kwargs['vdisk'].strip('\'\"')
 
         if vdisk in self._volumes_list:
             curr_mdiskgrp = self._volumes_list
@@ -1244,13 +1245,13 @@ port_speed!N/A
     def _cmd_addvdiskcopy(self, **kwargs):
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol_name = kwargs['obj'].strip('\'\'')
+        vol_name = kwargs['obj'].strip('\'\"')
         if vol_name not in self._volumes_list:
             return self._errors['CMMVC5753E']
         vol = self._volumes_list[vol_name]
         if 'mdiskgrp' not in kwargs:
             return self._errors['CMMVC5707E']
-        mdiskgrp = kwargs['mdiskgrp'].strip('\'\'')
+        mdiskgrp = kwargs['mdiskgrp'].strip('\'\"')
 
         copy_info = {}
         copy_info['id'] = self._find_unused_id(vol['copies'])
@@ -1297,7 +1298,7 @@ port_speed!N/A
         if 'copy' not in kwargs:
             return self._print_info_cmd(rows=rows, **kwargs)
         else:
-            copy_id = kwargs['copy'].strip('\'\'')
+            copy_id = kwargs['copy'].strip('\'\"')
             if copy_id not in vol['copies']:
                 return self._errors['CMMVC6353E']
             copy = vol['copies'][copy_id]
@@ -1325,10 +1326,10 @@ port_speed!N/A
     def _cmd_rmvdiskcopy(self, **kwargs):
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol_name = kwargs['obj'].strip('\'\'')
+        vol_name = kwargs['obj'].strip('\'\"')
         if 'copy' not in kwargs:
             return self._errors['CMMVC5707E']
-        copy_id = kwargs['copy'].strip('\'\'')
+        copy_id = kwargs['copy'].strip('\'\"')
         if vol_name not in self._volumes_list:
             return self._errors['CMMVC5753E']
         vol = self._volumes_list[vol_name]
@@ -1341,7 +1342,7 @@ port_speed!N/A
     def _cmd_chvdisk(self, **kwargs):
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol_name = kwargs['obj'].strip('\'\'')
+        vol_name = kwargs['obj'].strip('\'\"')
         vol = self._volumes_list[vol_name]
         kwargs.pop('obj')
 
@@ -1363,7 +1364,7 @@ port_speed!N/A
     def _cmd_movevdisk(self, **kwargs):
         if 'obj' not in kwargs:
             return self._errors['CMMVC5701E']
-        vol_name = kwargs['obj'].strip('\'\'')
+        vol_name = kwargs['obj'].strip('\'\"')
         vol = self._volumes_list[vol_name]
 
         if 'iogrp' not in kwargs:
@@ -1505,6 +1506,7 @@ class StorwizeSVCFakeDriver(storwize_svc.StorwizeSVCDriver):
     def _run_ssh(self, cmd, check_exit_code=True, attempts=1):
         try:
             LOG.debug(_('Run CLI command: %s') % cmd)
+            utils.check_ssh_injection(cmd)
             ret = self.fake_storage.execute_command(cmd, check_exit_code)
             (stdout, stderr) = ret
             LOG.debug(_('CLI output:\n stdout: %(stdout)s\n stderr: '
@@ -2329,7 +2331,6 @@ class StorwizeSVCDriverTestCase(test.TestCase):
                             self.driver._state['extent_size'])
         self.driver.migrate_volume(ctxt, volume, host)
         attrs = self.driver._helpers.get_vdisk_attributes(volume['name'])
-        print('AVISHAY ' + str(attrs))
         self.assertIn('openstack3', attrs['mdisk_grp_name'])
         self.driver.delete_volume(volume)
 

diff --git a/cinder/volume/drivers/ibm/storwize_svc/ssh.py b/cinder/volume/drivers/ibm/storwize_svc/ssh.py
index 5ebfb6dbd32714d5889995d38ba0131647188482..a0309b9c490bcbde1cc8bbb0f32e8c60e4964692 100644 (file)
--- a/cinder/volume/drivers/ibm/storwize_svc/ssh.py
+++ b/cinder/volume/drivers/ibm/storwize_svc/ssh.py
@@ -109,12 +109,13 @@ class StorwizeSSH(object):
 
     def mkhost(self, host_name, port_type, port_name):
         port = self._create_port_arg(port_type, port_name)
-        ssh_cmd = ['svctask', 'mkhost', '-force'] + port + ['-name', host_name]
+        ssh_cmd = ['svctask', 'mkhost', '-force'] + port
+        ssh_cmd += ['-name', '"%s"' % host_name]
         return self.run_ssh_check_created(ssh_cmd)
 
     def addhostport(self, host, port_type, port_name):
         port = self._create_port_arg(port_type, port_name)
-        ssh_cmd = ['svctask', 'addhostport', '-force'] + port + [host]
+        ssh_cmd = ['svctask', 'addhostport', '-force'] + port + ['"%s"' % host]
         self.run_ssh_assert_no_output(ssh_cmd)
 
     def lshost(self, host=None):
@@ -122,11 +123,11 @@ class StorwizeSSH(object):
         ssh_cmd = ['svcinfo', 'lshost', '-delim', '!']
         if host:
             with_header = False
-            ssh_cmd.append(host)
+            ssh_cmd.append('"%s"' % host)
         return self.run_ssh_info(ssh_cmd, with_header=with_header)
 
     def add_chap_secret(self, secret, host):
-        ssh_cmd = ['svctask', 'chhost', '-chapsecret', secret, host]
+        ssh_cmd = ['svctask', 'chhost', '-chapsecret', secret, '"%s"' % host]
         self.run_ssh_assert_no_output(ssh_cmd)
 
     def lsiscsiauth(self):
@@ -137,7 +138,7 @@ class StorwizeSSH(object):
         if wwpn:
             ssh_cmd = ['svcinfo', 'lsfabric', '-wwpn', wwpn, '-delim', '!']
         elif host:
-            ssh_cmd = ['svcinfo', 'lsfabric', '-host', host]
+            ssh_cmd = ['svcinfo', 'lsfabric', '-host', '"%s"' % host]
         else:
             msg = (_('Must pass wwpn or host to lsfabric.'))
             LOG.error(msg)
@@ -149,7 +150,7 @@ class StorwizeSSH(object):
 
         If vdisk already mapped and multihostmap is True, use the force flag.
         """
-        ssh_cmd = ['svctask', 'mkvdiskhostmap', '-host', host,
+        ssh_cmd = ['svctask', 'mkvdiskhostmap', '-host', '"%s"' % host,
                    '-scsi', lun, vdisk]
         out, err = self._ssh(ssh_cmd, check_exit_code=False)
         if 'successfully created' in out:
@@ -171,7 +172,7 @@ class StorwizeSSH(object):
         return self.run_ssh_check_created(ssh_cmd)
 
     def rmvdiskhostmap(self, host, vdisk):
-        ssh_cmd = ['svctask', 'rmvdiskhostmap', '-host', host, vdisk]
+        ssh_cmd = ['svctask', 'rmvdiskhostmap', '-host', '"%s"' % host, vdisk]
         self.run_ssh_assert_no_output(ssh_cmd)
 
     def lsvdiskhostmap(self, vdisk):
@@ -179,11 +180,11 @@ class StorwizeSSH(object):
         return self.run_ssh_info(ssh_cmd, with_header=True)
 
     def lshostvdiskmap(self, host):
-        ssh_cmd = ['svcinfo', 'lshostvdiskmap', '-delim', '!', host]
+        ssh_cmd = ['svcinfo', 'lshostvdiskmap', '-delim', '!', '"%s"' % host]
         return self.run_ssh_info(ssh_cmd, with_header=True)
 
     def rmhost(self, host):
-        ssh_cmd = ['svctask', 'rmhost', host]
+        ssh_cmd = ['svctask', 'rmhost', '"%s"' % host]
         self.run_ssh_assert_no_output(ssh_cmd)
 
     def mkvdisk(self, name, size, units, pool, opts, params):