From b245225d5e67120dfe7aee5e941f381846c89423 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Thu, 2 Mar 2017 15:32:47 +0000 Subject: [PATCH] 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 | 41 ++++++++++++++++++- cinder/opts.py | 3 ++ .../api/contrib/test_types_extra_specs.py | 30 ++++++++++++++ .../notes/bug-1667071-dc6407f40a1f7d15.yaml | 13 ++++++ 4 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/bug-1667071-dc6407f40a1f7d15.yaml diff --git a/cinder/api/contrib/types_extra_specs.py b/cinder/api/contrib/types_extra_specs.py index 0011090e35f..210fbae4f1b 100644 --- a/cinder/api/contrib/types_extra_specs.py +++ b/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) diff --git a/cinder/opts.py b/cinder/opts.py index ad00b80a137..0a9a770d5ec 100644 --- a/cinder/opts.py +++ b/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, diff --git a/cinder/tests/unit/api/contrib/test_types_extra_specs.py b/cinder/tests/unit/api/contrib/test_types_extra_specs.py index 7705fbd9280..84620fb358a 100644 --- a/cinder/tests/unit/api/contrib/test_types_extra_specs.py +++ b/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) diff --git a/releasenotes/notes/bug-1667071-dc6407f40a1f7d15.yaml b/releasenotes/notes/bug-1667071-dc6407f40a1f7d15.yaml new file mode 100644 index 00000000000..c9b31d399e3 --- /dev/null +++ b/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.