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.