Disallow modification of in use Volume Types

Modifying a Volume Type doesn't actually update any
volumes that are currently using said Volume Type. Given
that this is an admin only operation it was never really
too much of a concern.

There has been some issues reported however where an admin
modified the type while volumes were using it, then ran into
problems becuase the volumes didn't have the expected settings
described by the type.

This change adds a check before updating/deleting extra-specs of
a volume type.  If there are any volumes currently assigned to
the type being modified/deleted the operation will fail with an
InvalidRequest immediately when attempting the call.

To maintain backward compatability incase someobdy is using this
for something and they are really really sure they want to
continue doing so, we also add a config option to allow the
old behavior but default to NOT allowing it:
  'allow_inuse_volume_type_modification=False'

APIImpact

Change-Id: Iaea721e13a3903cae60cc3fb3acfad03bd173a6b
Closes-Bug: #1667071

cinder/api/contrib/types_extra_specs.py

@@ -15,19 +15,37 @@
 
 """The volume types extra specs extension"""
 
+from oslo_config import cfg
+from oslo_log import log as logging
+from oslo_log import versionutils
 from six.moves import http_client
 import webob
 
 from cinder.api import common
 from cinder.api import extensions
 from cinder.api.openstack import wsgi
+from cinder import context as ctxt
 from cinder import db
 from cinder import exception
-from cinder.i18n import _
+from cinder.i18n import _, _LW
 from cinder import rpc
 from cinder import utils
 from cinder.volume import volume_types
 
+LOG = logging.getLogger(__name__)
+
+extraspec_opts = [
+    cfg.BoolOpt('allow_inuse_volume_type_modification',
+                default=False,
+                deprecated_for_removal=True,
+                help="DEPRECATED: Allow the ability to modify the "
+                     "extra-spec settings of an in-use volume-type."),
+
+]
+
+CONF = cfg.CONF
+CONF.register_opts(extraspec_opts)
+
 authorize = extensions.extension_authorizer('volume', 'types_extra_specs')
 
 
@@ -52,9 +70,27 @@ class VolumeTypeExtraSpecsController(wsgi.Controller):
         self._check_type(context, type_id)
         return self._get_extra_specs(context, type_id)
 
+    def _allow_update(self, context, type_id):
+        if (not CONF.allow_inuse_volume_type_modification):
+            vols = db.volume_get_all(
+                ctxt.get_admin_context(),
+                limit=1,
+                filters={'volume_type_id': type_id})
+            if len(vols):
+                expl = _('Volume Type is currently in use.')
+                raise webob.exc.HTTPBadRequest(explanation=expl)
+        else:
+            msg = _LW("The option 'allow_inuse_volume_type_modification' "
+                      "is deprecated and will be removed in a future "
+                      "release.  The default behavior going forward will "
+                      "be to disallow modificaton of in-use types.")
+            versionutils.report_deprecated_feature(LOG, msg)
+        return
+
     def create(self, req, type_id, body=None):
         context = req.environ['cinder.context']
         authorize(context)
+        self._allow_update(context, type_id)
 
         self.assert_valid_body(body, 'extra_specs')
 
@@ -75,6 +111,8 @@ class VolumeTypeExtraSpecsController(wsgi.Controller):
     def update(self, req, type_id, id, body=None):
         context = req.environ['cinder.context']
         authorize(context)
+        self._allow_update(context, type_id)
+
         if not body:
             expl = _('Request body empty')
             raise webob.exc.HTTPBadRequest(explanation=expl)
@@ -115,6 +153,7 @@ class VolumeTypeExtraSpecsController(wsgi.Controller):
         context = req.environ['cinder.context']
         self._check_type(context, type_id)
         authorize(context)
+        self._allow_update(context, type_id)
 
         # Not found exception will be handled at the wsgi level
         db.volume_type_extra_specs_delete(context, type_id, id)

cinder/opts.py

@@ -18,6 +18,8 @@ from cinder import objects
 objects.register_all()
 
 from cinder.api import common as cinder_api_common
+from cinder.api.contrib import types_extra_specs as \
+    cinder_api_contrib_typesextraspecs
 from cinder.api.middleware import auth as cinder_api_middleware_auth
 from cinder.api.views import versions as cinder_api_views_versions
 from cinder.backup import api as cinder_backup_api
@@ -212,6 +214,7 @@ def list_opts():
         ('DEFAULT',
             itertools.chain(
                 cinder_api_common.api_common_opts,
+                cinder_api_contrib_typesextraspecs.extraspec_opts,
                 [cinder_api_middleware_auth.use_forwarded_for_opt],
                 cinder_api_views_versions.versions_opts,
                 cinder_backup_api.backup_api_opts,

cinder/tests/unit/api/contrib/test_types_extra_specs.py

@@ -16,6 +16,7 @@
 #    under the License.
 
 import mock
+from oslo_config import cfg
 import webob
 
 from cinder.api.contrib import types_extra_specs
@@ -25,6 +26,8 @@ from cinder.tests.unit.api import fakes
 from cinder.tests.unit import fake_constants as fake
 import cinder.wsgi
 
+CONF = cfg.CONF
+
 
 def return_create_volume_type_extra_specs(context, volume_type_id,
                                           extra_specs):
@@ -249,3 +252,30 @@ class VolumeTypesExtraSpecsTest(test.TestCase):
     def test_create_invalid_too_many_key(self):
         body = {"key1": "value1", "ke/y2": "value2", "key3": "value3"}
         self._extra_specs_create_bad_body(body=body)
+
+    def test_create_volumes_exist(self):
+        self.mock_object(cinder.db,
+                         'volume_type_extra_specs_update_or_create',
+                         return_create_volume_type_extra_specs)
+        body = {"extra_specs": {"key1": "value1"}}
+        req = fakes.HTTPRequest.blank(self.api_path)
+        with mock.patch.object(
+                cinder.db,
+                'volume_get_all',
+                return_value=['a']):
+            req = fakes.HTTPRequest.blank('/v2/%s/types/%s/extra_specs' % (
+                fake.PROJECT_ID, fake.VOLUME_TYPE_ID))
+            req.method = 'POST'
+
+            body = {"extra_specs": {"key1": "value1"}}
+            req = fakes.HTTPRequest.blank(self.api_path)
+            self.assertRaises(webob.exc.HTTPBadRequest,
+                              self.controller.create,
+                              req,
+                              fake.VOLUME_ID, body)
+
+            # Again but with conf set to allow modification
+            CONF.set_default('allow_inuse_volume_type_modification', True)
+            res_dict = self.controller.create(req, fake.VOLUME_ID, body)
+            self.assertEqual({'extra_specs': {'key1': 'value1'}},
+                             res_dict)

releasenotes/notes/bug-1667071-dc6407f40a1f7d15.yaml

@@ -0,0 +1,13 @@
+---
+fixes:
+  - Modifying the extra-specs of an in use Volume Type was something that
+    we've unintentionally allowed.  The result is unexpected or unknown
+    volume behaviors in cases where a type was modified while a volume was
+    assigned that type.  This has been particularly annoying for folks that
+    have assigned the volume-type to a different/new backend device.
+
+    In case there are customers using this "bug" we add a config option to
+    retain the bad behavior "allow_inuse_volume_type_modification", with a
+    default setting of False (Don't allow).  Note this config option is being
+    introduced as deprecated and will be removed in a future release.  It's
+    being provided as a bridge to not break upgrades without notice.