Ensure that lun_id is an int for NetApp Drivers

Various NetApp drivers were treating the lun_id as a string
rather than an int.  This was causing attempts to mount NetApp
volumes to Hyper-V nodes to fail as they were checking an integer
type against a unicode type which would fail.

This change casts result_lun to an integer after the value has
gone through the ssh injection attack check.  This way Hyper-V
is able to verify if the found LUN is the target LUN, enabling
mounting of NetApp volumes to Hyper-V.

This change also refactors initialize_connection into some helper
methods so as to enable simpler unit testing of the patchset.

Closes-bug: 1372808

Change-Id: I308b3b2dff315ec33451fb45a30ecd53d5d4c353

cinder/tests/volume/drivers/netapp/dataontap/fakes.py

@@ -58,7 +58,7 @@ FC_FABRIC_MAP = {'fabricB':
                   'initiator_port_wwn_list': ['21000024ff406cc3']}}
 
 FC_TARGET_INFO = {'driver_volume_type': 'fibre_channel',
-                  'data': {'target_lun': '1',
+                  'data': {'target_lun': 1,
                            'initiator_target_map': FC_I_T_MAP,
                            'access_mode': 'rw',
                            'target_wwn': FC_TARGET_WWPNS,

cinder/tests/volume/drivers/netapp/fakes.py

@@ -1,4 +1,5 @@
-# Copyright (c) - 2014, Clinton Knight.  All rights reserved.
+# Copyright (c) - 2014, Clinton Knight  All rights reserved.
+# Copyright (c) - 2014, Rushil Chugh  All rights reserved.
 #
 #    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
@@ -42,3 +43,27 @@ def create_configuration_eseries():
     config = create_configuration()
     config.append_config_values(na_opts.netapp_eseries_opts)
     return config
+
+ISCSI_FAKE_LUN_ID = 1
+
+ISCSI_FAKE_IQN = 'iqn.1993-08.org.debian:01:10'
+
+ISCSI_FAKE_ADDRESS = '10.63.165.216'
+
+ISCSI_FAKE_PORT = '2232'
+
+ISCSI_FAKE_VOLUME = {'id': 'fake_id'}
+
+ISCSI_FAKE_TARGET = {}
+ISCSI_FAKE_TARGET['address'] = ISCSI_FAKE_ADDRESS
+ISCSI_FAKE_TARGET['port'] = ISCSI_FAKE_PORT
+
+ISCSI_FAKE_VOLUME = {'id': 'fake_id', 'provider_auth': 'None stack password'}
+
+FC_ISCSI_TARGET_INFO_DICT = {'target_discovered': False,
+                             'target_portal': '10.63.165.216:2232',
+                             'target_iqn': ISCSI_FAKE_IQN,
+                             'target_lun': ISCSI_FAKE_LUN_ID,
+                             'volume_id': ISCSI_FAKE_VOLUME['id'],
+                             'auth_method': 'None', 'auth_username': 'stack',
+                             'auth_password': 'password'}

cinder/tests/volume/drivers/netapp/test_utils.py

@@ -24,6 +24,7 @@ from oslo.concurrency import processutils as putils
 
 from cinder import exception
 from cinder import test
+import cinder.tests.volume.drivers.netapp.fakes as fake
 from cinder import version
 import cinder.volume.drivers.netapp.utils as na_utils
 
@@ -350,3 +351,34 @@ class OpenStackInfoTestCase(test.TestCase):
         info._update_openstack_info()
 
         self.assertTrue(mock_updt_from_dpkg.called)
+
+    def test_iscsi_connection_properties(self):
+
+        actual_properties = na_utils.get_iscsi_connection_properties(
+            fake.ISCSI_FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME,
+            fake.ISCSI_FAKE_IQN, fake.ISCSI_FAKE_ADDRESS,
+            fake.ISCSI_FAKE_PORT)
+
+        actual_properties_mapped = actual_properties['data']
+
+        self.assertDictEqual(actual_properties_mapped,
+                             fake.FC_ISCSI_TARGET_INFO_DICT)
+
+    def test_iscsi_connection_lun_id_type_str(self):
+        FAKE_LUN_ID = '1'
+
+        actual_properties = na_utils.get_iscsi_connection_properties(
+            FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME, fake.ISCSI_FAKE_IQN,
+            fake.ISCSI_FAKE_ADDRESS, fake.ISCSI_FAKE_PORT)
+
+        actual_properties_mapped = actual_properties['data']
+
+        self.assertIs(type(actual_properties_mapped['target_lun']), int)
+
+    def test_iscsi_connection_lun_id_type_dict(self):
+        FAKE_LUN_ID = {'id': 'fake_id'}
+
+        self.assertRaises(TypeError, na_utils.get_iscsi_connection_properties,
+                          FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME,
+                          fake.ISCSI_FAKE_IQN, fake.ISCSI_FAKE_ADDRESS,
+                          fake.ISCSI_FAKE_PORT)

cinder/volume/drivers/netapp/dataontap/block_base.py

@@ -519,7 +519,6 @@ class NetAppBlockStorageLibrary(object):
         msg = _("Mapped LUN %(name)s to the initiator %(initiator_name)s")
         msg_fmt = {'name': name, 'initiator_name': initiator_name}
         LOG.debug(msg % msg_fmt)
-        iqn = self.zapi_client.get_iscsi_service_details()
         target_details_list = self.zapi_client.get_iscsi_target_details()
         msg = _("Successfully fetched target details for LUN %(name)s and "
                 "initiator %(initiator_name)s")
@@ -537,32 +536,22 @@ class NetAppBlockStorageLibrary(object):
         if not target_details:
             target_details = target_details_list[0]
 
+        (address, port) = (target_details['address'], target_details['port'])
+
         if not target_details['address'] and target_details['port']:
             msg = _('Failed to get target portal for the LUN %s')
             raise exception.VolumeBackendAPIException(data=msg % name)
+
+        iqn = self.zapi_client.get_iscsi_service_details()
         if not iqn:
             msg = _('Failed to get target IQN for the LUN %s')
             raise exception.VolumeBackendAPIException(data=msg % name)
 
-        properties = {}
+        properties = na_utils.get_iscsi_connection_properties(lun_id, volume,
-        properties['target_discovered'] = False
+                                                              iqn, address,
-        (address, port) = (target_details['address'], target_details['port'])
+                                                              port)
-        properties['target_portal'] = '%s:%s' % (address, port)
-        properties['target_iqn'] = iqn
-        properties['target_lun'] = lun_id
-        properties['volume_id'] = volume['id']
 
-        auth = volume['provider_auth']
+        return properties
-        if auth:
-            (auth_method, auth_username, auth_secret) = auth.split()
-            properties['auth_method'] = auth_method
-            properties['auth_username'] = auth_username
-            properties['auth_password'] = auth_secret
-
-        return {
-            'driver_volume_type': 'iscsi',
-            'data': properties,
-        }
 
     def terminate_connection_iscsi(self, volume, connector, **kwargs):
         """Driver entry point to unattach a volume from an instance.
@@ -648,7 +637,7 @@ class NetAppBlockStorageLibrary(object):
 
         target_info = {'driver_volume_type': 'fibre_channel',
                        'data': {'target_discovered': True,
-                                'target_lun': lun_id,
+                                'target_lun': int(lun_id),
                                 'target_wwn': target_wwpns,
                                 'access_mode': 'rw',
                                 'initiator_target_map': initiator_target_map}}

cinder/volume/drivers/netapp/eseries/iscsi.py

@@ -556,7 +556,7 @@ class NetAppEseriesISCSIDriver(driver.ISCSIDriver):
         initiator_name = connector['initiator']
         vol = self._get_latest_volume(volume['name_id'])
         iscsi_details = self._get_iscsi_service_details()
-        iscsi_det = self._get_iscsi_portal_for_vol(vol, iscsi_details)
+        iscsi_portal = self._get_iscsi_portal_for_vol(vol, iscsi_details)
         mapping = self._map_volume_to_host(vol, initiator_name)
         lun_id = mapping['lun']
         self._cache_vol_mapping(mapping)
@@ -566,23 +566,13 @@ class NetAppEseriesISCSIDriver(driver.ISCSIDriver):
         msg = _("Successfully fetched target details for volume %(id)s and "
                 "initiator %(initiator_name)s.")
         LOG.debug(msg % msg_fmt)
-        properties = {}
+        iqn = iscsi_portal['iqn']
-        properties['target_discovered'] = False
+        address = iscsi_portal['ip']
-        properties['target_portal'] = '%s:%s' % (iscsi_det['ip'],
+        port = iscsi_portal['tcp_port']
-                                                 iscsi_det['tcp_port'])
+        properties = na_utils.get_iscsi_connection_properties(lun_id, volume,
-        properties['target_iqn'] = iscsi_det['iqn']
+                                                              iqn, address,
-        properties['target_lun'] = lun_id
+                                                              port)
-        properties['volume_id'] = volume['id']
+        return properties
-        auth = volume['provider_auth']
-        if auth:
-            (auth_method, auth_username, auth_secret) = auth.split()
-            properties['auth_method'] = auth_method
-            properties['auth_username'] = auth_username
-            properties['auth_password'] = auth_secret
-        return {
-            'driver_volume_type': 'iscsi',
-            'data': properties,
-        }
 
     def _get_iscsi_service_details(self):
         """Gets iscsi iqn, ip and port information."""

cinder/volume/drivers/netapp/utils.py

@@ -139,6 +139,27 @@ def log_extra_spec_warnings(extra_specs):
             LOG.warning(msg % args)
 
 
+def get_iscsi_connection_properties(lun_id, volume, iqn,
+                                    address, port):
+
+        properties = {}
+        properties['target_discovered'] = False
+        properties['target_portal'] = '%s:%s' % (address, port)
+        properties['target_iqn'] = iqn
+        properties['target_lun'] = int(lun_id)
+        properties['volume_id'] = volume['id']
+        auth = volume['provider_auth']
+        if auth:
+            (auth_method, auth_username, auth_secret) = auth.split()
+            properties['auth_method'] = auth_method
+            properties['auth_username'] = auth_username
+            properties['auth_password'] = auth_secret
+        return {
+            'driver_volume_type': 'iscsi',
+            'data': properties,
+        }
+
+
 class hashabledict(dict):
     """A hashable dictionary that is comparable (i.e. in unit tests, etc.)"""
     def __hash__(self):