add IPACL support to the Lightbits Cinder driver

Lightbits storage clusters have the ability to block access to a
volume unless it comes from a specific IP, via a non-standard
extension to NVMe/TCP called "IPACL", which augments the standard
string-based ACLs. This patch adds support for Lightbits IPACL to the
Lightbits Cinder driver. We get the IPs to set from the os_brick
running on the nova node that will be accessing the volumes (see the
corresponding Lightbits connector os_brick patch) and set them when
updating the volume's ACLs on creation and on volume update due to
attachment and disconnection.
os_brick patch: https://review.opendev.org/c/openstack/os-brick/+/903574

Depends-on: Ia22322bb8a8097900d5509b6c540355eb474f19d
Change-Id: Id2540cbb9567f8242c76806318cdde9a80e791e1

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

@@ -63,6 +63,7 @@ FAKE_LIGHTOS_CLUSTER_INFO: Dict[str, str] = {
 }
 
 FAKE_CLIENT_HOSTNQN = "hostnqn1"
+FAKE_HOST_IPS = ['10.10.0.1']
 VOLUME_BACKEND_NAME = "lightos_backend"
 RESERVED_PERCENTAGE = 30
 DEVICE_SCAN_ATTEMPTS_DEFAULT = 5
@@ -85,6 +86,7 @@ class InitiatorConnectorFactoryMocker:
 class InitialConnectorMock:
     nqn = FAKE_CLIENT_HOSTNQN
     found_discovery_client = True
+    host_ips = FAKE_HOST_IPS
 
     def get_hostnqn(self):
         return self.__class__.nqn
@@ -92,15 +94,18 @@ class InitialConnectorMock:
     def find_dsc(self):
         return self.__class__.found_discovery_client
 
+    def get_host_ips(self):
+        return self.__class__.host_ips
+
     def get_connector_properties(self, root):
         return dict(nqn=self.__class__.nqn,
-                    found_dsc=self.__class__.found_discovery_client)
+                    found_dsc=self.__class__.found_discovery_client,
+                    host_ips=self.__class__.host_ips)
 
 
 def get_connector_properties():
     connector = InitialConnectorMock()
-    return dict(nqn=connector.get_hostnqn(),
+    return connector.get_connector_properties(None)
-                found_dsc=connector.find_dsc())
 
 
 def get_vol_etag(volume):
@@ -172,6 +177,9 @@ class DBMock(object):
             volume["size"] = kwargs["size"]
         if kwargs.get("acl", None):
             volume["acl"] = {'values': kwargs.get('acl')}
+
+        if kwargs.get("ip_acl", None):
+            volume["IPAcl"] = {'values': kwargs.get('ip_acl')}
         volume["ETag"] = get_vol_etag(volume)
         return httpstatus.OK, volume
 
@@ -202,7 +210,7 @@ class DBMock(object):
         return httpstatus.OK, vol
 
     def update_volume(self, **kwargs):
-        assert ("project_name" in kwargs and kwargs["project_name"]), \
+        assert "project_name" in kwargs and kwargs["project_name"], \
             "project_name must be provided"
 
     def create_snapshot(self, snapshot) -> Tuple[int, Dict]:
@@ -275,6 +283,7 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
         configuration.lightos_default_compression_enabled = (
             DEFAULT_COMPRESSION)
         configuration.lightos_default_num_replicas = 3
+        configuration.lightos_use_ipacl = True
         configuration.num_volume_device_scan_tries = (
             DEVICE_SCAN_ATTEMPTS_DEFAULT)
         configuration.lightos_api_service_timeout = LIGHTOS_API_SERVICE_TIMEOUT
@@ -314,6 +323,10 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
                 return (httpstatus.OK, FAKE_LIGHTOS_CLUSTER_INFO)
             elif cmd == "create_volume":
                 project_name = kwargs["project_name"]
+                ipacl = (
+                    {'values': ['ALLOW_NONE']}
+                    if self.driver.configuration.lightos_use_ipacl
+                    else {'values': ['ALLOW_ANY']})
                 volume = {
                     "project_name": project_name,
                     "name": kwargs["name"],
@@ -322,6 +335,7 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
                     "compression": kwargs["compression"],
                     "src_snapshot_name": kwargs["src_snapshot_name"],
                     "acl": {'values': kwargs.get('acl')},
+                    "IPAcl": ipacl,
                     "state": "Available",
                 }
                 volume["ETag"] = get_vol_etag(volume)
@@ -396,6 +410,20 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
         self.driver.delete_volume(volume)
         db.volume_destroy(self.ctxt, volume.id)
 
+    def test_create_volume_ipacl_off(self):
+        """Test that lightos_client succeed. ipacl false"""
+        self.driver.configuration.lightos_use_ipacl = False
+        self.driver.do_setup(None)
+
+        vol_type = test_utils.create_volume_type(self.ctxt, self,
+                                                 name='my_vol_type')
+        volume = test_utils.create_volume(self.ctxt, size=4,
+                                          volume_type_id=vol_type.id)
+
+        self.driver.create_volume(volume)
+        self.driver.delete_volume(volume)
+        db.volume_destroy(self.ctxt, volume.id)
+
     def test_create_volume_same_volume_twice_succeed(self):
         """Test succeed to create an exiting volume."""
         self.driver.do_setup(None)
@@ -418,6 +446,10 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
         def send_cmd_mock(cmd, **kwargs):
             if cmd == "create_volume":
                 project_name = kwargs["project_name"]
+                ipacl = (
+                    {'values': ['ALLOW_NONE']}
+                    if self.driver.configuration.lightos_use_ipacl
+                    else {'values': ['ALLOW_ANY']})
                 volume = {
                     "project_name": project_name,
                     "name": kwargs["name"],
@@ -426,6 +458,7 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
                     "compression": kwargs["compression"],
                     "src_snapshot_name": kwargs["src_snapshot_name"],
                     "acl": {'values': kwargs.get('acl')},
+                    "IPAcl": ipacl,
                     "state": vol_state,
                 }
                 volume["ETag"] = get_vol_etag(volume)
@@ -462,6 +495,10 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
         def send_cmd_mock(cmd, **kwargs):
             if cmd == "create_volume":
                 project_name = kwargs["project_name"]
+                ipacl = (
+                    {'values': ['ALLOW_NONE']}
+                    if self.driver.configuration.lightos_use_ipacl
+                    else {'values': ['ALLOW_ANY']})
                 volume = {
                     "project_name": project_name,
                     "name": kwargs["name"],
@@ -470,6 +507,7 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
                     "compression": kwargs["compression"],
                     "src_snapshot_name": kwargs["src_snapshot_name"],
                     "acl": {'values': kwargs.get('acl')},
+                    "IPAcl": ipacl,
                     "state": "Migrating",
                 }
                 volume["ETag"] = get_vol_etag(volume)
@@ -641,6 +679,7 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
     def test_initialize_connection(self):
         InitialConnectorMock.nqn = "hostnqn1"
         InitialConnectorMock.found_discovery_client = True
+        InitialConnectorMock.host_ips = FAKE_HOST_IPS
         self.driver.do_setup(None)
         vol_type = test_utils.create_volume_type(self.ctxt, self,
                                                  name='my_vol_type')
@@ -657,6 +696,37 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
         self.assertEqual(
             self.db.data['projects']['default']['volumes'][0]['UUID'],
             connection_props['data']['uuid'])
+        self.assertEqual(
+            self.db.data['projects']['default']['volumes'][0]['IPAcl'],
+            {'values': FAKE_HOST_IPS})
+
+        self.driver.delete_volume(volume)
+        db.volume_destroy(self.ctxt, volume.id)
+
+    def test_initialize_connection_ipacl_disabled(self):
+        self.driver.configuration.lightos_use_ipacl = False
+        InitialConnectorMock.nqn = "hostnqn1"
+        InitialConnectorMock.found_discovery_client = True
+        self.driver.do_setup(None)
+        vol_type = test_utils.create_volume_type(self.ctxt, self,
+                                                 name='my_vol_type')
+        volume = test_utils.create_volume(self.ctxt, size=4,
+                                          volume_type_id=vol_type.id)
+        self.driver.create_volume(volume)
+
+        connection_props = \
+            self.driver.initialize_connection(volume,
+                                              get_connector_properties())
+        self.assertIn('driver_volume_type', connection_props)
+        self.assertEqual('lightos', connection_props['driver_volume_type'])
+        self.assertEqual(FAKE_LIGHTOS_CLUSTER_INFO['subsystemNQN'],
+                         connection_props['data']['subsysnqn'])
+        self.assertEqual(
+            self.db.data['projects']['default']['volumes'][0]['UUID'],
+            connection_props['data']['uuid'])
+        self.assertEqual(
+            self.db.data['projects']['default']['volumes'][0]['IPAcl'],
+            {'values': ['ALLOW_ANY']})
 
         self.driver.delete_volume(volume)
         db.volume_destroy(self.ctxt, volume.id)
@@ -668,6 +738,10 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
         def send_cmd_mock(cmd, **kwargs):
             if cmd == "create_volume":
                 project_name = kwargs["project_name"]
+                ipacl = (
+                    {'values': ['ALLOW_NONE']}
+                    if self.driver.configuration.lightos_use_ipacl
+                    else {'values': ['ALLOW_ANY']})
                 volume = {
                     "project_name": project_name,
                     "name": kwargs["name"],
@@ -676,6 +750,7 @@ class LightOSStorageVolumeDriverTest(test.TestCase):
                     "compression": kwargs["compression"],
                     "src_snapshot_name": kwargs["src_snapshot_name"],
                     "acl": {'values': kwargs.get('acl')},
+                    "IPAcl": ipacl,
                     "state": "Migrating",
                 }
                 volume["ETag"] = get_vol_etag(volume)

cinder/volume/drivers/lightos.py

@@ -25,6 +25,7 @@ from urllib.parse import urlparse
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import importutils
+from oslo_utils import netutils
 from oslo_utils import units
 import requests
 import urllib3
@@ -77,7 +78,16 @@ lightos_opts = [
     cfg.IntOpt('lightos_api_service_timeout',
                default=30,
                help='The default amount of time (in seconds) to wait for'
-               ' an API endpoint response.')
+               ' an API endpoint response.'),
+    cfg.BoolOpt('lightos_use_ipacl',
+                default=True,
+                help='IPACL work in conjunction with the standard NVME ACL.'
+                ' A host must be in both the IPACL and the ACL of a volume to'
+                ' access that volume. Cinder always sets the volume`s ACL.'
+                ' If lightos_use_ipacl is set to True, Cinder will also add'
+                ' the host`s IP addresses to a volume IPACL. If set to'
+                ' False, any IP address may access the volume. The default'
+                ' is True.'),
 ]
 
 CONF = cfg.CONF
@@ -152,6 +162,9 @@ class LightOSConnection(object):
                                   'acl': {
                                       'values': kwargs.get('acl'),
                                   },
+                                  'IPAcl': {
+                                      'values': kwargs.get('ip_acl'),
+                                  },
                                   'sourceSnapshotUUID': kwargs.get(
                                       'src_snapshot_uuid'),
                                   'sourceSnapshotName': kwargs.get(
@@ -170,6 +183,9 @@ class LightOSConnection(object):
                                   'acl': {
                                       'values': kwargs.get('acl'),
                                   },
+                                  'IPAcl': {
+                                      'values': kwargs.get('ip_acl'),
+                                  },
                               }),
 
             'extend_volume': ('PUT',
@@ -598,6 +614,7 @@ class LightOSVolumeDriver(driver.VolumeDriver):
                                    src_snapshot_lightos_name=None):
         """Create a new LightOS volume for this openstack volume."""
         (compression, num_replicas, _) = self._get_volume_specs(os_volume)
+        vol_ipAcl = ['ALLOW_NONE'] if self.use_ip_acl() else ['ALLOW_ANY']
         return self.cluster.send_cmd(
             cmd='create_volume',
             project_name=project_name,
@@ -607,7 +624,8 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             n_replicas=num_replicas,
             compression=compression,
             src_snapshot_name=src_snapshot_lightos_name,
-            acl=['ALLOW_NONE']
+            acl=['ALLOW_NONE'],
+            ip_acl=vol_ipAcl
         )
 
     def _get_lightos_uuid(self, project_name, volume):
@@ -1034,17 +1052,22 @@ class LightOSVolumeDriver(driver.VolumeDriver):
 
         return server_properties
 
-    def set_volume_acl(self, project_name, lightos_uuid, acl, etag):
+    def set_volume_acl(self, project_name, lightos_uuid, acl, ip_acl, etag):
         return self.cluster.send_cmd(
             cmd='update_volume',
             project_name=project_name,
             timeout=self.logical_op_timeout,
             volume_uuid=lightos_uuid,
             acl=acl,
+            ip_acl=ip_acl,
             etag=etag
         )
 
-    def __add_volume_acl(self, project_name, lightos_volname, acl_to_add):
+    def use_ip_acl(self):
+        return self.configuration.lightos_use_ipacl
+
+    def __add_volume_acl(self, project_name, lightos_volname, acl_to_add,
+                         host_ips):
         (status, data) = self._get_lightos_volume(project_name,
                                                   self.logical_op_timeout,
                                                   vol_name=lightos_volname)
@@ -1063,7 +1086,13 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             LOG.warning('Got LightOS volume without ACL?! data: %s', data)
             return False
 
+        ip_acl = data.get('IPAcl')
+        if self.use_ip_acl() and not ip_acl:
+            LOG.warning('Got LightOS volume without IP ACL?! data: %s', data)
+            return False
+
         acl = acl.get('values', [])
+        ip_acl = ip_acl.get('values', [])
 
         # remove ALLOW_NONE and add our acl_to_add if not already there
         if 'ALLOW_NONE' in acl:
@@ -1071,15 +1100,53 @@ class LightOSVolumeDriver(driver.VolumeDriver):
         if acl_to_add not in acl:
             acl.append(acl_to_add)
 
+        if 'ALLOW_NONE' in ip_acl:
+            ip_acl.remove('ALLOW_NONE')
+
+        if self.use_ip_acl():
+            ip_acl = list(set(ip_acl).union(set(host_ips)))
+        else:
+            ip_acl = ['ALLOW_ANY']
+
+        # The max (16) elemenets are allowed in IPACL.
+        # if elements are more than 16 then remove
+        # less-frequently used IPv6 address(s), and IPv4 if needed.
+
+        ipv4addrs = [addr for addr in ip_acl if netutils.is_valid_ipv4(addr)]
+        ipv6addrs = [addr for addr in ip_acl if netutils.is_valid_ipv6(addr)]
+
+        IpAcl_size = 16
+        if len(ipv4addrs) > IpAcl_size:
+            LOG.warning(
+                'IPv4 address(es) are more than maximum (%s)'
+                ' allowed in IP-ACL of volume, therefore reducing'
+                ' IPv4 address(es) written to IP-ACL of volume %s'
+                ' of project %s', IpAcl_size, lightos_volname,
+                project_name)
+
+            ip_acl = ipv4addrs[0: IpAcl_size]
+
+        elif len(ip_acl) > IpAcl_size:
+            LOG.warning(
+                'Combined IPv4 and IPv6 address(es) are more than'
+                ' maximum (%s) allowed in IP-ACL of volume, therefore'
+                ' reducing IPv6 address(es) written to IP-ACL of'
+                ' volume %s of project %s', IpAcl_size,
+                lightos_volname, project_name)
+
+            ipv6addrs_count = IpAcl_size - len(ipv4addrs)
+            ip_acl = ipv4addrs + (ipv6addrs[0: ipv6addrs_count])
+
         return self.set_volume_acl(
             project_name,
             lightos_uuid,
             acl,
+            ip_acl,
             etag=data.get(
                 'ETag',
                 ''))
 
-    def add_volume_acl(self, project_name, volume, acl_to_add):
+    def add_volume_acl(self, project_name, volume, acl_to_add, host_ips):
         LOG.debug(
             'add_volume_acl got volume %s project %s acl %s',
             volume,
@@ -1090,13 +1157,15 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             self.__add_volume_acl,
             project_name,
             lightos_volname,
-            acl_to_add)
+            acl_to_add,
+            host_ips)
 
     def __remove_volume_acl(
             self,
             project_name,
             lightos_volname,
-            acl_to_remove):
+            acl_to_remove,
+            host_ips):
         (status, data) = self._get_lightos_volume(project_name,
                                                   self.logical_op_timeout,
                                                   vol_name=lightos_volname)
@@ -1138,19 +1207,41 @@ class LightOSVolumeDriver(driver.VolumeDriver):
         if not acl:
             acl.append('ALLOW_NONE')
 
+        ip_acl = data.get('IPAcl')
+        if self.use_ip_acl() and not ip_acl:
+            LOG.warning('Got LightOS volume without IP ACL?! data: %s', data)
+            return False
+
+        ip_acl = ip_acl.get('values')
+        if self.use_ip_acl() and not ip_acl:
+            LOG.warning(
+                'Got LightOS volume without IP ACL values?! data: %s', data)
+            return False
+
+        for ip in host_ips:
+            try:
+                ip_acl.remove(ip)
+            except ValueError:
+                LOG.warning(
+                    'Could not find matching ip %s in ip-acl of volume %s ',
+                    ip, lightos_volname)
+
+        if not ip_acl:
+            ip_acl.append('ALLOW_NONE')
+
         return self.set_volume_acl(
             project_name,
             lightos_uuid,
             acl,
-            etag=data.get(
+            ip_acl,
-                'ETag',
+            etag=data.get('ETag', ''))
-                ''))
 
     def __overwrite_volume_acl(
             self,
             project_name,
             lightos_volname,
-            acl):
+            acl,
+            host_ips):
         status, data = self._get_lightos_volume(project_name,
                                                 self.logical_op_timeout,
                                                 vol_name=lightos_volname)
@@ -1170,11 +1261,12 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             project_name,
             lightos_uuid,
             acl,
+            host_ips,
             etag=data.get(
                 'ETag',
                 ''))
 
-    def remove_volume_acl(self, project_name, volume, acl_to_remove):
+    def remove_volume_acl(self, project_name, volume, acl_to_remove, host_ips):
         lightos_volname = self._lightos_volname(volume)
         LOG.debug('remove_volume_acl volume %s project %s acl %s',
                   volume, project_name, acl_to_remove)
@@ -1182,7 +1274,8 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             self.__remove_volume_acl,
             project_name,
             lightos_volname,
-            acl_to_remove)
+            acl_to_remove,
+            host_ips)
 
     def remove_all_volume_acls(self, project_name, volume):
         lightos_volname = self._lightos_volname(volume)
@@ -1192,9 +1285,11 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             self.__overwrite_volume_acl,
             project_name,
             lightos_volname,
+            ['ALLOW_NONE'],
             ['ALLOW_NONE'])
 
-    def update_volume_acl(self, func, project_name, lightos_volname, acl):
+    def update_volume_acl(self, func, project_name, lightos_volname, acl,
+                          host_ips):
         # loop because lightos api is async
         end = time.time() + self.logical_op_timeout
         first_iteration = True
@@ -1202,7 +1297,7 @@ class LightOSVolumeDriver(driver.VolumeDriver):
             if not first_iteration:
                 time.sleep(1)
             first_iteration = False
-            res = func(project_name, lightos_volname, acl)
+            res = func(project_name, lightos_volname, acl, host_ips)
             if not isinstance(res, tuple):
                 LOG.debug('Update_volume: func %s(%s project %s) failed',
                           func, lightos_volname, project_name)
@@ -1410,6 +1505,10 @@ class LightOSVolumeDriver(driver.VolumeDriver):
     def initialize_connection(self, volume, connector):
         hostnqn = connector.get('nqn')
         found_dsc = connector.get('found_dsc')
+        host_ips = connector.get('host_ips', [])
+        LOG.info('Current host hostNQN is %s and IP(s) are %s',
+                 hostnqn,
+                 host_ips)
         LOG.debug(
             'initialize_connection: connector hostnqn is %s found_dsc %s',
             hostnqn,
@@ -1424,9 +1523,14 @@ class LightOSVolumeDriver(driver.VolumeDriver):
                    'client, aborting' % (connector))
             raise exception.VolumeBackendAPIException(message=_(msg))
 
+        if not host_ips:
+            msg = 'Connector (%s) did not find host IPs, aborting' % (
+                connector)
+            raise exception.VolumeBackendAPIException(message=_(msg))
+
         lightos_volname = self._lightos_volname(volume)
         project_name = self._get_lightos_project_name(volume)
-        success = self.add_volume_acl(project_name, volume, hostnqn)
+        success = self.add_volume_acl(project_name, volume, hostnqn, host_ips)
         if not success or not self._wait_for_volume_acl(
                 project_name, lightos_volname, hostnqn, True):
             msg = ('Could not add ACL for hostnqn %s LightOS volume'
@@ -1439,6 +1543,7 @@ class LightOSVolumeDriver(driver.VolumeDriver):
     def terminate_connection(self, volume, connector, **kwargs):
         force = 'force' in kwargs
         hostnqn = connector.get('nqn') if connector else None
+        host_ips = connector.get('host_ips', []) if connector else []
         LOG.debug(
             'terminate_connection: force %s kwargs %s hostnqn %s',
             force,
@@ -1462,7 +1567,8 @@ class LightOSVolumeDriver(driver.VolumeDriver):
 
         lightos_volname = self._lightos_volname(volume)
         project_name = self._get_lightos_project_name(volume)
-        success = self.remove_volume_acl(project_name, volume, hostnqn)
+        success = self.remove_volume_acl(project_name, volume, hostnqn,
+                                         host_ips)
         if not success or not self._wait_for_volume_acl(
                 project_name, lightos_volname, hostnqn, False):
             LOG.warning(

releasenotes/notes/lightbits-volume-ipacl-23da3aa469689817.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    Lightbits driver:  Added a new configuration option
+    ``lightos_use_ipacl``, defaulting to true. When set to true, the
+    Cinder driver will restrict access to each volume to the IP
+    addresses of the host machine that the volume is attached to.