Fix IPv6 for Cinder NetApp ONTAP drivers

NetApp ONTAP driver currently have some issues with IPv6:
 - The URL is not properly formatted when using in management path
 - NFS driver breaks when handling IPv6 addresses
 - iSCSI driver creates a URL that is not properly formatted when
 using IPv6 addresses

This patch fixes all issues related to IPv6 on NetApp ONTAP drivers.

Closes-bug: 1788419
Closes-bug: 1788460
Change-Id: I6eeca47997c7134d6604874bea48eab7cab6c1a2

cinder/tests/unit/volume/drivers/netapp/dataontap/client/test_api.py

@@ -21,6 +21,7 @@ Tests for NetApp API layer
 import ddt
 from lxml import etree
 import mock
+from oslo_utils import netutils
 import paramiko
 import six
 from six.moves import urllib
@@ -237,6 +238,25 @@ class NetAppApiServerTests(test.TestCase):
 
         self.root.send_http_request(na_element)
 
+    @ddt.data('192.168.1.0', '127.0.0.1', '0.0.0.0',
+              '::ffff:8', 'fdf8:f53b:82e4::53', '2001::1',
+              'fe80::200::abcd', '2001:0000:4136:e378:8000:63bf:3fff:fdd2')
+    def test__get_url(self, host):
+        port = '80'
+        root = netapp_api.NaServer(host, port=port)
+
+        protocol = root.TRANSPORT_TYPE_HTTP
+        url = root.URL_FILER
+
+        if netutils.is_valid_ipv6(host):
+            host = netutils.escape_ipv6(host)
+
+        result = '%s://%s:%s/%s' % (protocol, host, port, url)
+
+        url = root._get_url()
+
+        self.assertEqual(result, url)
+
 
 class NetAppApiElementTransTests(test.TestCase):
     """Test case for NetApp API element translations."""

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

@@ -30,8 +30,10 @@ HOST_NAME = 'fake.host.name'
 BACKEND_NAME = 'fake_backend_name'
 POOL_NAME = 'aggr1'
 SHARE_IP = '192.168.99.24'
+IPV6_ADDRESS = 'fe80::6e40:8ff:fe8a:130'
 EXPORT_PATH = '/fake/export/path'
 NFS_SHARE = '%s:%s' % (SHARE_IP, EXPORT_PATH)
+NFS_SHARE_IPV6 = '[%s]:%s' % (IPV6_ADDRESS, EXPORT_PATH)
 HOST_STRING = '%s@%s#%s' % (HOST_NAME, BACKEND_NAME, POOL_NAME)
 NFS_HOST_STRING = '%s@%s#%s' % (HOST_NAME, BACKEND_NAME, NFS_SHARE)
 AGGREGATE = 'aggr1'
@@ -242,9 +244,7 @@ ISCSI_TARGET_DETAILS_LIST = [
 ]
 
 IPV4_ADDRESS = '192.168.14.2'
-IPV6_ADDRESS = 'fe80::6e40:8ff:fe8a:130'
 NFS_SHARE_IPV4 = IPV4_ADDRESS + ':' + EXPORT_PATH
-NFS_SHARE_IPV6 = IPV6_ADDRESS + ':' + EXPORT_PATH
 
 RESERVED_PERCENTAGE = 7
 MAX_OVER_SUBSCRIPTION_RATIO = 19.0

cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py

@@ -323,18 +323,16 @@ class NetAppNfsDriverTestCase(test.TestCase):
         self.driver._delete_file.assert_called_once_with(snapshot.volume_id,
                                                          snapshot.name)
 
-    def test__get_volume_location(self):
+    @ddt.data(fake.NFS_SHARE, fake.NFS_SHARE_IPV6)
+    def test__get_volume_location(self, provider):
         volume_id = fake.VOLUME_ID
-        self.mock_object(self.driver, '_get_host_ip',
+
-                         return_value='168.124.10.12')
+        self.mock_object(self.driver, '_get_provider_location',
-        self.mock_object(self.driver, '_get_export_path',
+                         return_value=provider)
-                         return_value='/fake_mount_path')
 
         retval = self.driver._get_volume_location(volume_id)
 
-        self.assertEqual('168.124.10.12:/fake_mount_path', retval)
+        self.assertEqual(provider, retval)
-        self.driver._get_host_ip.assert_called_once_with(volume_id)
-        self.driver._get_export_path.assert_called_once_with(volume_id)
 
     def test__clone_backing_file_for_volume(self):
         self.assertRaises(NotImplementedError,
@@ -507,34 +505,35 @@ class NetAppNfsDriverTestCase(test.TestCase):
 
         self.assertEqual(0, mock_delete.call_count)
 
-    def test_get_export_ip_path_volume_id_provided(self):
+    @ddt.data((fake.NFS_SHARE, fake.SHARE_IP),
-        mock_get_host_ip = self.mock_object(self.driver, '_get_host_ip')
+              (fake.NFS_SHARE_IPV6, fake.IPV6_ADDRESS))
-        mock_get_host_ip.return_value = fake.IPV4_ADDRESS
+    @ddt.unpack
+    def test_get_export_ip_path_volume_id_provided(self, provider_location,
+                                                   ip):
+        mock_get_host_ip = self.mock_object(self.driver,
+                                            '_get_provider_location')
+        mock_get_host_ip.return_value = provider_location
 
-        mock_get_export_path = self.mock_object(
+        expected = (ip, fake.EXPORT_PATH)
-            self.driver, '_get_export_path')
-        mock_get_export_path.return_value = fake.EXPORT_PATH
-
-        expected = (fake.IPV4_ADDRESS, fake.EXPORT_PATH)
 
         result = self.driver._get_export_ip_path(fake.VOLUME_ID)
 
         self.assertEqual(expected, result)
 
-    def test_get_export_ip_path_share_provided(self):
+    @ddt.data((fake.NFS_SHARE, fake.SHARE_IP, fake.EXPORT_PATH),
-        expected = (fake.SHARE_IP, fake.EXPORT_PATH)
+              (fake.NFS_SHARE_IPV6, fake.IPV6_ADDRESS, fake.EXPORT_PATH))
+    @ddt.unpack
+    def test_get_export_ip_path_share_provided(self, share, ip, path):
+        expected = (ip, path)
 
-        result = self.driver._get_export_ip_path(share=fake.NFS_SHARE)
+        result = self.driver._get_export_ip_path(share=share)
 
         self.assertEqual(expected, result)
 
     def test_get_export_ip_path_volume_id_and_share_provided(self):
-        mock_get_host_ip = self.mock_object(self.driver, '_get_host_ip')
+        mock_get_host_ip = self.mock_object(self.driver,
-        mock_get_host_ip.return_value = fake.IPV4_ADDRESS
+                                            '_get_provider_location')
-
+        mock_get_host_ip.return_value = fake.NFS_SHARE_IPV4
-        mock_get_export_path = self.mock_object(
-            self.driver, '_get_export_path')
-        mock_get_export_path.return_value = fake.EXPORT_PATH
 
         expected = (fake.IPV4_ADDRESS, fake.EXPORT_PATH)
 
@@ -547,26 +546,6 @@ class NetAppNfsDriverTestCase(test.TestCase):
         self.assertRaises(exception.InvalidInput,
                           self.driver._get_export_ip_path)
 
-    def test_get_host_ip(self):
-        mock_get_provider_location = self.mock_object(
-            self.driver, '_get_provider_location')
-        mock_get_provider_location.return_value = fake.NFS_SHARE
-        expected = fake.SHARE_IP
-
-        result = self.driver._get_host_ip(fake.VOLUME_ID)
-
-        self.assertEqual(expected, result)
-
-    def test_get_export_path(self):
-        mock_get_provider_location = self.mock_object(
-            self.driver, '_get_provider_location')
-        mock_get_provider_location.return_value = fake.NFS_SHARE
-        expected = fake.EXPORT_PATH
-
-        result = self.driver._get_export_path(fake.VOLUME_ID)
-
-        self.assertEqual(expected, result)
-
     def test_construct_image_url_loc(self):
         img_loc = fake.FAKE_IMAGE_LOCATION
 

cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py

@@ -1129,18 +1129,20 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase):
             fake.EXPORT_PATH, fake.IMAGE_FILE_ID, fake.VOLUME['name'],
             fake.VSERVER_NAME, dest_exists=True)
 
-    def test_get_source_ip_and_path(self):
+    @ddt.data((fake.NFS_SHARE, fake.SHARE_IP),
+              (fake.NFS_SHARE_IPV6, fake.IPV6_ADDRESS))
+    @ddt.unpack
+    def test_get_source_ip_and_path(self, share, ip):
         self.driver._get_ip_verify_on_cluster = mock.Mock(
-            return_value=fake.SHARE_IP)
+            return_value=ip)
 
         src_ip, src_path = self.driver._get_source_ip_and_path(
-            fake.NFS_SHARE, fake.IMAGE_FILE_ID)
+            share, fake.IMAGE_FILE_ID)
 
-        self.assertEqual(fake.SHARE_IP, src_ip)
+        self.assertEqual(ip, src_ip)
         assert_path = fake.EXPORT_PATH + '/' + fake.IMAGE_FILE_ID
         self.assertEqual(assert_path, src_path)
-        self.driver._get_ip_verify_on_cluster.assert_called_once_with(
+        self.driver._get_ip_verify_on_cluster.assert_called_once_with(ip)
-            fake.SHARE_IP)
 
     def test_get_destination_ip_and_path(self):
         self.driver._get_ip_verify_on_cluster = mock.Mock(

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

@@ -24,17 +24,19 @@ ISCSI_FAKE_LUN_ID = 1
 
 ISCSI_FAKE_IQN = 'iqn.1993-08.org.debian:01:10'
 
-ISCSI_FAKE_ADDRESS = '10.63.165.216'
+ISCSI_FAKE_ADDRESS_IPV4 = '10.63.165.216'
+ISCSI_FAKE_ADDRESS_IPV6 = 'fe80::72a4:a152:aad9:30d9'
 
 ISCSI_FAKE_PORT = '2232'
 
 ISCSI_FAKE_VOLUME = {'id': 'fake_id'}
 
 ISCSI_FAKE_TARGET = {}
-ISCSI_FAKE_TARGET['address'] = ISCSI_FAKE_ADDRESS
+ISCSI_FAKE_TARGET['address'] = ISCSI_FAKE_ADDRESS_IPV4
 ISCSI_FAKE_TARGET['port'] = ISCSI_FAKE_PORT
 
 ISCSI_FAKE_VOLUME = {'id': 'fake_id', 'provider_auth': 'None stack password'}
+ISCSI_FAKE_VOLUME_NO_AUTH = {'id': 'fake_id', 'provider_auth': ''}
 
 FC_ISCSI_TARGET_INFO_DICT = {'target_discovered': False,
                              'target_portal': '10.63.165.216:2232',
@@ -44,6 +46,13 @@ FC_ISCSI_TARGET_INFO_DICT = {'target_discovered': False,
                              'auth_method': 'None', 'auth_username': 'stack',
                              'auth_password': 'password'}
 
+FC_ISCSI_TARGET_INFO_DICT_IPV6 = {'target_discovered': False,
+                                  'target_portal':
+                                      '[fe80::72a4:a152:aad9:30d9]:2232',
+                                  'target_iqn': ISCSI_FAKE_IQN,
+                                  'target_lun': ISCSI_FAKE_LUN_ID,
+                                  'volume_id': ISCSI_FAKE_VOLUME['id']}
+
 VOLUME_NAME = 'fake_volume_name'
 VOLUME_ID = 'fake_volume_id'
 VOLUME_TYPE_ID = 'fake_volume_type_id'

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

@@ -38,6 +38,7 @@ from cinder.volume import qos_specs
 from cinder.volume import volume_types
 
 
+@ddt.ddt
 class NetAppDriverUtilsTestCase(test.TestCase):
 
     @mock.patch.object(na_utils, 'LOG', mock.Mock())
@@ -121,7 +122,7 @@ class NetAppDriverUtilsTestCase(test.TestCase):
 
         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_IQN, fake.ISCSI_FAKE_ADDRESS_IPV4,
             fake.ISCSI_FAKE_PORT)
 
         actual_properties_mapped = actual_properties['data']
@@ -134,7 +135,7 @@ class NetAppDriverUtilsTestCase(test.TestCase):
 
         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)
+            fake.ISCSI_FAKE_ADDRESS_IPV4, fake.ISCSI_FAKE_PORT)
 
         actual_properties_mapped = actual_properties['data']
 
@@ -145,9 +146,17 @@ class NetAppDriverUtilsTestCase(test.TestCase):
 
         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_IQN, fake.ISCSI_FAKE_ADDRESS_IPV4,
                           fake.ISCSI_FAKE_PORT)
 
+    def test_iscsi_connection_properties_ipv6(self):
+        actual_properties = na_utils.get_iscsi_connection_properties(
+            '1', fake.ISCSI_FAKE_VOLUME_NO_AUTH, fake.ISCSI_FAKE_IQN,
+            fake.ISCSI_FAKE_ADDRESS_IPV6, fake.ISCSI_FAKE_PORT)
+
+        self.assertDictEqual(actual_properties['data'],
+                             fake.FC_ISCSI_TARGET_INFO_DICT_IPV6)
+
     def test_get_volume_extra_specs(self):
         fake_extra_specs = {'fake_key': 'fake_value'}
         fake_volume_type = {'extra_specs': fake_extra_specs}
@@ -538,6 +547,29 @@ class NetAppDriverUtilsTestCase(test.TestCase):
 
         self.assertIsNone(result)
 
+    @ddt.data(("192.168.99.24:/fake/export/path", "192.168.99.24",
+               "/fake/export/path"),
+              ("127.0.0.1:/", "127.0.0.1", "/"),
+              ("[f180::30d9]:/path_to-export/3.1/this folder", "f180::30d9",
+               "/path_to-export/3.1/this folder"),
+              ("[::]:/", "::", "/"),
+              ("[2001:db8::1]:/fake_export", "2001:db8::1", "/fake_export"))
+    @ddt.unpack
+    def test_get_export_host_junction_path(self, share, host, junction_path):
+        result_host, result_path = na_utils.get_export_host_junction_path(
+            share)
+
+        self.assertEqual(host, result_host)
+        self.assertEqual(junction_path, result_path)
+
+    @ddt.data("192.14.21.0/wrong_export", "192.14.21.0:8080:/wrong_export"
+              "2001:db8::1:/wrong_export",
+              "[2001:db8::1:/wrong_export", "2001:db8::1]:/wrong_export")
+    def test_get_export_host_junction_path_with_invalid_exports(self, share):
+        self.assertRaises(exception.NetAppDriverException,
+                          na_utils.get_export_host_junction_path,
+                          share)
+
 
 class OpenStackInfoTestCase(test.TestCase):
 

cinder/volume/drivers/netapp/dataontap/client/api.py

@@ -26,6 +26,7 @@ from eventlet import semaphore
 
 from lxml import etree
 from oslo_log import log as logging
+from oslo_utils import netutils
 import random
 import six
 from six.moves import urllib
@@ -281,7 +282,12 @@ class NaServer(object):
         return processed_response.get_child_by_name('results')
 
     def _get_url(self):
-        return '%s://%s:%s/%s' % (self._protocol, self._host, self._port,
+        host = self._host
+
+        if netutils.is_valid_ipv6(host):
+            host = netutils.escape_ipv6(host)
+
+        return '%s://%s:%s/%s' % (self._protocol, host, self._port,
                                   self._url)
 
     def _build_opener(self):

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

@@ -31,6 +31,7 @@ import time
 from oslo_concurrency import processutils
 from oslo_config import cfg
 from oslo_log import log as logging
+from oslo_utils import netutils
 from oslo_utils import units
 import six
 from six.moves import urllib
@@ -279,8 +280,13 @@ class NetAppNfsDriver(driver.ManageableVD,
 
     def _get_volume_location(self, volume_id):
         """Returns NFS mount address as <nfs_ip_address>:<nfs_mount_dir>."""
-        nfs_server_ip = self._get_host_ip(volume_id)
+        provider_location = self._get_provider_location(volume_id)
-        export_path = self._get_export_path(volume_id)
+        nfs_server_ip, export_path = na_utils.get_export_host_junction_path(
+            provider_location)
+
+        if netutils.is_valid_ipv6(nfs_server_ip):
+            nfs_server_ip = netutils.escape_ipv6(nfs_server_ip)
+
         return nfs_server_ip + ':' + export_path
 
     def _clone_backing_file_for_volume(self, volume_name, clone_name,
@@ -303,14 +309,6 @@ class NetAppNfsDriver(driver.ManageableVD,
         volume = self.db.volume_get(self._context, volume_id)
         return volume.provider_location
 
-    def _get_host_ip(self, volume_id):
-        """Returns IP address for the given volume."""
-        return self._get_provider_location(volume_id).rsplit(':')[0]
-
-    def _get_export_path(self, volume_id):
-        """Returns NFS export path for the given volume."""
-        return self._get_provider_location(volume_id).rsplit(':')[1]
-
     def _volume_not_present(self, nfs_mount, volume_name):
         """Check if volume exists."""
         try:
@@ -749,7 +747,7 @@ class NetAppNfsDriver(driver.ManageableVD,
                 ip = na_utils.resolve_hostname(host)
                 share_candidates = []
                 for sh in self._mounted_shares:
-                    sh_exp = sh.split(':')[1]
+                    sh_exp = sh.split(':')[-1]
                     if sh_exp == dir:
                         share_candidates.append(sh)
                 if share_candidates:
@@ -864,11 +862,12 @@ class NetAppNfsDriver(driver.ManageableVD,
         """
 
         if volume_id:
-            host_ip = self._get_host_ip(volume_id)
+            provider_location = self._get_provider_location(volume_id)
-            export_path = self._get_export_path(volume_id)
+            host_ip, export_path = na_utils.get_export_host_junction_path(
+                provider_location)
         elif share:
-            host_ip = share.split(':')[0]
+            host_ip, export_path = na_utils.get_export_host_junction_path(
-            export_path = share.split(':')[1]
+                share)
         else:
             raise exception.InvalidInput(
                 'A volume ID or share was not specified.')

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

@@ -187,7 +187,7 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver,
             return
         target_path = '%s' % (volume['name'])
         share = volume_utils.extract_host(volume['host'], level='pool')
-        export_path = share.split(':')[1]
+        __, export_path = na_utils.get_export_host_junction_path(share)
         flex_vol_name = self.zapi_client.get_vol_by_junc_vserver(self.vserver,
                                                                  export_path)
         self.zapi_client.file_assign_qos(flex_vol_name,
@@ -325,9 +325,8 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver,
         vserver_addresses = self.zapi_client.get_operational_lif_addresses()
 
         for share in self._mounted_shares:
+            host, junction_path = na_utils.get_export_host_junction_path(share)
 
-            host = share.split(':')[0]
-            junction_path = share.split(':')[1]
             address = na_utils.resolve_hostname(host)
 
             if address not in vserver_addresses:
@@ -365,7 +364,7 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver,
         ip_vserver = self._get_vserver_for_ip(ip)
         if ip_vserver and shares:
             for share in shares:
-                ip_sh = share.split(':')[0]
+                ip_sh, __ = na_utils.get_export_host_junction_path(share)
                 sh_vserver = self._get_vserver_for_ip(ip_sh)
                 if sh_vserver == ip_vserver:
                     LOG.debug('Share match found for ip %s', ip)
@@ -541,15 +540,17 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver,
                   volume['id'])
 
     def _get_source_ip_and_path(self, nfs_share, file_name):
-        src_ip = self._get_ip_verify_on_cluster(nfs_share.split(':')[0])
+        host, share_path = na_utils.get_export_host_junction_path(nfs_share)
-        src_path = os.path.join(nfs_share.split(':')[1], file_name)
+        src_ip = self._get_ip_verify_on_cluster(host)
+        src_path = os.path.join(share_path, file_name)
+
         return src_ip, src_path
 
     def _get_destination_ip_and_path(self, volume):
         share = volume_utils.extract_host(volume['host'], level='pool')
-        share_ip_and_path = share.split(":")
+        share_ip, share_path = na_utils.get_export_host_junction_path(share)
-        dest_ip = self._get_ip_verify_on_cluster(share_ip_and_path[0])
+        dest_ip = self._get_ip_verify_on_cluster(share_ip)
-        dest_path = os.path.join(share_ip_and_path[1], volume['name'])
+        dest_path = os.path.join(share_path, volume['name'])
 
         return dest_ip, dest_path
 

cinder/volume/drivers/netapp/utils.py

@@ -30,6 +30,7 @@ import socket
 
 from oslo_concurrency import processutils as putils
 from oslo_log import log as logging
+from oslo_utils import netutils
 import six
 
 from cinder import context
@@ -174,6 +175,9 @@ def log_extra_spec_warnings(extra_specs):
 
 def get_iscsi_connection_properties(lun_id, volume, iqn,
                                     address, port):
+    # literal ipv6 address
+    if netutils.is_valid_ipv6(address):
+        address = netutils.escape_ipv6(address)
 
     properties = {}
     properties['target_discovered'] = False
@@ -361,6 +365,30 @@ def get_legacy_qos_policy(extra_specs):
     return dict(policy_name=external_policy_name)
 
 
+def get_export_host_junction_path(share):
+    if '[' in share and ']' in share:
+        try:
+            # ipv6
+            host = re.search('\[(.*)\]', share).group(1)
+            junction_path = share.split(':')[-1]
+        except AttributeError:
+            raise exception.NetAppDriverException(_("Share '%s' is "
+                                                    "not in a valid "
+                                                    "format.") % share)
+    else:
+        # ipv4
+        path = share.split(':')
+        if len(path) == 2:
+            host = path[0]
+            junction_path = path[1]
+        else:
+            raise exception.NetAppDriverException(_("Share '%s' is "
+                                                    "not in a valid "
+                                                    "format.") % share)
+
+    return host, junction_path
+
+
 class hashabledict(dict):
     """A hashable dictionary that is comparable (i.e. in unit tests, etc.)"""
     def __hash__(self):

releasenotes/notes/bugfix-netapp-driver-cinder-ipv6-c3c4d0d6a7d0de91.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - Fixed support for IPv6 on management and data paths for NFS, iSCSI
+    and FCP NetApp ONTAP drivers.