Disallow instance tag set for invalid instance states
Currently, instance tags can be set at any time during the instance lifecycle, possibly because it does not go through the compute API. This makes the valid instance states for the instance tag update API consistent with the instance metadata update API. If instance tag update is requested outside of the valid states, a 409 conflict error will be returned. Closes-Bug: #1591381 Change-Id: Id53a31654e105854f4942e6d47a1bea90a3e9c3b
This commit is contained in:
melanie witt
parent
05cab2666f
commit
4cb366f831
|
|
@ -14,11 +14,14 @@ import jsonschema
|
|||
|
||||
from webob import exc
|
||||
|
||||
from nova.api.openstack import common
|
||||
from nova.api.openstack.compute.schemas import server_tags as schema
|
||||
from nova.api.openstack.compute.views import server_tags
|
||||
from nova.api.openstack import extensions
|
||||
from nova.api.openstack import wsgi
|
||||
from nova.api import validation
|
||||
from nova import compute
|
||||
from nova.compute import vm_states
|
||||
from nova import exception
|
||||
from nova.i18n import _
|
||||
from nova import objects
|
||||
|
|
@ -35,6 +38,21 @@ def _get_tags_names(tags):
|
|||
class ServerTagsController(wsgi.Controller):
|
||||
_view_builder_class = server_tags.ViewBuilder
|
||||
|
||||
def __init__(self):
|
||||
self.compute_api = compute.API()
|
||||
super(ServerTagsController, self).__init__()
|
||||
|
||||
def _check_instance_in_valid_state(self, context, server_id, action):
|
||||
instance = common.get_instance(self.compute_api, context, server_id)
|
||||
if instance.vm_state not in (vm_states.ACTIVE, vm_states.PAUSED,
|
||||
vm_states.SUSPENDED, vm_states.STOPPED):
|
||||
exc = exception.InstanceInvalidState(attr='vm_state',
|
||||
instance_uuid=instance.uuid,
|
||||
state=instance.vm_state,
|
||||
method=action)
|
||||
common.raise_http_conflict_for_instance_invalid_state(exc, action,
|
||||
server_id)
|
||||
|
||||
@wsgi.Controller.api_version("2.26")
|
||||
@wsgi.response(204)
|
||||
@extensions.expected_errors(404)
|
||||
|
|
@ -66,11 +84,12 @@ class ServerTagsController(wsgi.Controller):
|
|||
return {'tags': _get_tags_names(tags)}
|
||||
|
||||
@wsgi.Controller.api_version("2.26")
|
||||
@extensions.expected_errors((400, 404))
|
||||
@extensions.expected_errors((400, 404, 409))
|
||||
@validation.schema(schema.update)
|
||||
def update(self, req, server_id, id, body):
|
||||
context = req.environ["nova.context"]
|
||||
authorize(context, action='update')
|
||||
self._check_instance_in_valid_state(context, server_id, 'update tag')
|
||||
|
||||
try:
|
||||
jsonschema.validate(id, schema.tag)
|
||||
|
|
@ -113,11 +132,12 @@ class ServerTagsController(wsgi.Controller):
|
|||
return response
|
||||
|
||||
@wsgi.Controller.api_version("2.26")
|
||||
@extensions.expected_errors((400, 404))
|
||||
@extensions.expected_errors((400, 404, 409))
|
||||
@validation.schema(schema.update_all)
|
||||
def update_all(self, req, server_id, body):
|
||||
context = req.environ["nova.context"]
|
||||
authorize(context, action='update_all')
|
||||
self._check_instance_in_valid_state(context, server_id, 'update tags')
|
||||
|
||||
invalid_tags = []
|
||||
for tag in body['tags']:
|
||||
|
|
@ -155,10 +175,11 @@ class ServerTagsController(wsgi.Controller):
|
|||
|
||||
@wsgi.Controller.api_version("2.26")
|
||||
@wsgi.response(204)
|
||||
@extensions.expected_errors(404)
|
||||
@extensions.expected_errors((404, 409))
|
||||
def delete(self, req, server_id, id):
|
||||
context = req.environ["nova.context"]
|
||||
authorize(context, action='delete')
|
||||
self._check_instance_in_valid_state(context, server_id, 'delete tag')
|
||||
|
||||
try:
|
||||
objects.Tag.destroy(context, server_id, id)
|
||||
|
|
@ -169,10 +190,11 @@ class ServerTagsController(wsgi.Controller):
|
|||
|
||||
@wsgi.Controller.api_version("2.26")
|
||||
@wsgi.response(204)
|
||||
@extensions.expected_errors(404)
|
||||
@extensions.expected_errors((404, 409))
|
||||
def delete_all(self, req, server_id):
|
||||
context = req.environ["nova.context"]
|
||||
authorize(context, action='delete_all')
|
||||
self._check_instance_in_valid_state(context, server_id, 'delete tags')
|
||||
|
||||
try:
|
||||
objects.TagList.destroy(context, server_id)
|
||||
|
|
|
|||
|
|
@ -16,12 +16,14 @@ from webob import exc
|
|||
from nova.api.openstack.compute import extension_info
|
||||
from nova.api.openstack.compute import server_tags
|
||||
from nova.api.openstack.compute import servers
|
||||
from nova.compute import vm_states
|
||||
from nova.db.sqlalchemy import models
|
||||
from nova import exception
|
||||
from nova.objects import instance
|
||||
from nova.objects import tag as tag_obj
|
||||
from nova import test
|
||||
from nova.tests.unit.api.openstack import fakes
|
||||
from nova.tests.unit import fake_instance
|
||||
|
||||
UUID = 'b48316c5-71e8-45e4-9884-6c78055b9b13'
|
||||
TAG1 = 'tag1'
|
||||
|
|
@ -31,6 +33,16 @@ TAGS = [TAG1, TAG2, TAG3]
|
|||
NON_EXISTING_UUID = '123'
|
||||
|
||||
|
||||
def return_server(compute_api, context, instance_id, expected_attrs=None):
|
||||
return fake_instance.fake_instance_obj(context, vm_state=vm_states.ACTIVE)
|
||||
|
||||
|
||||
def return_invalid_server(compute_api, context, instance_id,
|
||||
expected_attrs=None):
|
||||
return fake_instance.fake_instance_obj(context,
|
||||
vm_state=vm_states.BUILDING)
|
||||
|
||||
|
||||
class ServerTagsTest(test.TestCase):
|
||||
api_version = '2.26'
|
||||
|
||||
|
|
@ -73,6 +85,7 @@ class ServerTagsTest(test.TestCase):
|
|||
|
||||
@mock.patch('nova.db.instance_tag_set')
|
||||
def test_update_all(self, mock_db_set_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
fake_tags = [self._get_tag(tag) for tag in TAGS]
|
||||
mock_db_set_inst_tags.return_value = fake_tags
|
||||
req = self._get_request(
|
||||
|
|
@ -84,6 +97,7 @@ class ServerTagsTest(test.TestCase):
|
|||
mock_db_set_inst_tags.assert_called_once_with(context, UUID, TAGS)
|
||||
|
||||
def test_update_all_too_many_tags(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
fake_tags = {'tags': [str(i) for i in range(
|
||||
instance.MAX_TAG_COUNT + 1)]}
|
||||
|
||||
|
|
@ -93,6 +107,7 @@ class ServerTagsTest(test.TestCase):
|
|||
req, UUID, body=fake_tags)
|
||||
|
||||
def test_update_all_forbidden_characters(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
req = self._get_request('/v2/fake/servers/%s/tags' % UUID, 'PUT')
|
||||
for tag in ['tag,1', 'tag/1']:
|
||||
self.assertRaises(exc.HTTPBadRequest,
|
||||
|
|
@ -106,6 +121,7 @@ class ServerTagsTest(test.TestCase):
|
|||
req, UUID, body={'tags': [1]})
|
||||
|
||||
def test_update_all_too_long_tag(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
req = self._get_request('/v2/fake/servers/%s/tags' % UUID, 'PUT')
|
||||
tag = "a" * (tag_obj.MAX_TAG_LENGTH + 1)
|
||||
self.assertRaises(exc.HTTPBadRequest, self.controller.update_all,
|
||||
|
|
@ -117,6 +133,13 @@ class ServerTagsTest(test.TestCase):
|
|||
self.controller.update_all,
|
||||
req, UUID, body={'tags': {'tag': 'tag'}})
|
||||
|
||||
def test_update_all_invalid_instance_state(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance',
|
||||
return_invalid_server)
|
||||
req = self._get_request('/v2/fake/servers/%s/tags' % UUID, 'PUT')
|
||||
self.assertRaises(exc.HTTPConflict, self.controller.update_all,
|
||||
req, UUID, body={'tags': TAGS})
|
||||
|
||||
@mock.patch('nova.db.instance_tag_exists')
|
||||
def test_show_non_existing_tag(self, mock_exists):
|
||||
mock_exists.return_value = False
|
||||
|
|
@ -128,6 +151,7 @@ class ServerTagsTest(test.TestCase):
|
|||
@mock.patch('nova.db.instance_tag_add')
|
||||
@mock.patch('nova.db.instance_tag_get_by_instance_uuid')
|
||||
def test_update(self, mock_db_get_inst_tags, mock_db_add_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
mock_db_get_inst_tags.return_value = [self._get_tag(TAG1)]
|
||||
mock_db_add_inst_tags.return_value = self._get_tag(TAG2)
|
||||
|
||||
|
|
@ -144,6 +168,7 @@ class ServerTagsTest(test.TestCase):
|
|||
|
||||
@mock.patch('nova.db.instance_tag_get_by_instance_uuid')
|
||||
def test_update_existing_tag(self, mock_db_get_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
mock_db_get_inst_tags.return_value = [self._get_tag(TAG1)]
|
||||
|
||||
req = self._get_request(
|
||||
|
|
@ -156,6 +181,7 @@ class ServerTagsTest(test.TestCase):
|
|||
|
||||
@mock.patch('nova.db.instance_tag_get_by_instance_uuid')
|
||||
def test_update_tag_limit_exceed(self, mock_db_get_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
fake_tags = [self._get_tag(str(i))
|
||||
for i in range(instance.MAX_TAG_COUNT)]
|
||||
mock_db_get_inst_tags.return_value = fake_tags
|
||||
|
|
@ -167,6 +193,7 @@ class ServerTagsTest(test.TestCase):
|
|||
|
||||
@mock.patch('nova.db.instance_tag_get_by_instance_uuid')
|
||||
def test_update_too_long_tag(self, mock_db_get_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
mock_db_get_inst_tags.return_value = []
|
||||
|
||||
tag = "a" * (tag_obj.MAX_TAG_LENGTH + 1)
|
||||
|
|
@ -177,6 +204,7 @@ class ServerTagsTest(test.TestCase):
|
|||
|
||||
@mock.patch('nova.db.instance_tag_get_by_instance_uuid')
|
||||
def test_update_forbidden_characters(self, mock_db_get_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
mock_db_get_inst_tags.return_value = []
|
||||
for tag in ['tag,1', 'tag/1']:
|
||||
req = self._get_request(
|
||||
|
|
@ -184,8 +212,17 @@ class ServerTagsTest(test.TestCase):
|
|||
self.assertRaises(exc.HTTPBadRequest, self.controller.update,
|
||||
req, UUID, tag, body=None)
|
||||
|
||||
def test_update_invalid_instance_state(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance',
|
||||
return_invalid_server)
|
||||
req = self._get_request(
|
||||
'/v2/fake/servers/%s/tags/%s' % (UUID, TAG1), 'PUT')
|
||||
self.assertRaises(exc.HTTPConflict, self.controller.update, req, UUID,
|
||||
TAG1, body=None)
|
||||
|
||||
@mock.patch('nova.db.instance_tag_delete')
|
||||
def test_delete(self, mock_db_delete_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
req = self._get_request(
|
||||
'/v2/fake/servers/%s/tags/%s' % (UUID, TAG2), 'DELETE')
|
||||
context = req.environ["nova.context"]
|
||||
|
|
@ -194,6 +231,8 @@ class ServerTagsTest(test.TestCase):
|
|||
|
||||
@mock.patch('nova.db.instance_tag_delete')
|
||||
def test_delete_non_existing_tag(self, mock_db_delete_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
|
||||
def fake_db_delete_tag(context, instance_uuid, tag):
|
||||
self.assertEqual(UUID, instance_uuid)
|
||||
self.assertEqual(TAG1, tag)
|
||||
|
|
@ -205,13 +244,29 @@ class ServerTagsTest(test.TestCase):
|
|||
self.assertRaises(exc.HTTPNotFound, self.controller.delete,
|
||||
req, UUID, TAG1)
|
||||
|
||||
def test_delete_invalid_instance_state(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance',
|
||||
return_invalid_server)
|
||||
req = self._get_request(
|
||||
'/v2/fake/servers/%s/tags/%s' % (UUID, TAG2), 'DELETE')
|
||||
self.assertRaises(exc.HTTPConflict, self.controller.delete, req, UUID,
|
||||
TAG1)
|
||||
|
||||
@mock.patch('nova.db.instance_tag_delete_all')
|
||||
def test_delete_all(self, mock_db_delete_inst_tags):
|
||||
self.stub_out('nova.api.openstack.common.get_instance', return_server)
|
||||
req = self._get_request('/v2/fake/servers/%s/tags' % UUID, 'DELETE')
|
||||
context = req.environ["nova.context"]
|
||||
self.controller.delete_all(req, UUID)
|
||||
mock_db_delete_inst_tags.assert_called_once_with(context, UUID)
|
||||
|
||||
def test_delete_all_invalid_instance_state(self):
|
||||
self.stub_out('nova.api.openstack.common.get_instance',
|
||||
return_invalid_server)
|
||||
req = self._get_request('/v2/fake/servers/%s/tags' % UUID, 'DELETE')
|
||||
self.assertRaises(exc.HTTPConflict, self.controller.delete_all, req,
|
||||
UUID)
|
||||
|
||||
def test_show_non_existing_instance(self):
|
||||
req = self._get_request(
|
||||
'/v2/fake/servers/%s/tags/%s' % (NON_EXISTING_UUID, TAG1), 'GET')
|
||||
|
|
|
|||
Loading…
Reference in New Issue