From fcf586366287155dfb1007643bd6cc513300285e Mon Sep 17 00:00:00 2001 From: zhangbailin Date: Wed, 4 Mar 2020 18:12:27 +0800 Subject: [PATCH] Separate update and swap volume policies This patch changes the volume update policy to be 'rule:system_admin_or_owner' to allow instance owners to update attachment details like delete_on_termination. It creates a new volume swap policy element with the old admin-only behavior, and makes the volume update code check the appropriate policy based on what action is being performed. Co-Authored-By: Dan Smith Partial implement blueprint destroy-instance-with-datavolume Change-Id: I2cbe37b65ceac2efb3b252460dc01d17474e6343 --- nova/api/openstack/compute/volumes.py | 19 ++++--- nova/policies/volumes_attachments.py | 18 +++++- nova/tests/unit/fake_policy.py | 1 + nova/tests/unit/policies/test_volumes.py | 56 ++++++++++++++++++- nova/tests/unit/test_policy.py | 3 +- ...olicy-for-attachment-e4c20d4907a52fa7.yaml | 8 +++ 6 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/separate-update-and-swap-volume-policy-for-attachment-e4c20d4907a52fa7.yaml diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index 6129fa2d5477..49bca827f4ef 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -465,23 +465,26 @@ class VolumeAttachmentController(wsgi.Controller): def update(self, req, server_id, id, body): context = req.environ['nova.context'] instance = common.get_instance(self.compute_api, context, server_id) - # TODO(danms): For now, use the existing admin-only policy for update. - # Later, split off the swap_volume permission and check the correct - # policy based on what is being asked by the client. - context.can(va_policies.POLICY_ROOT % 'update', - target={'project_id': instance.project_id}) - attachment = body['volumeAttachment'] volume_id = attachment['volumeId'] only_swap = not api_version_request.is_supported(req, '2.85') + + # NOTE(brinzhang): If the 'volumeId' requested by the user is + # different from the 'id' in the url path, or only swap is allowed by + # the microversion, we should check the swap volume policy. + # otherwise, check the volume update policy. + if only_swap or id != volume_id: + context.can(va_policies.POLICY_ROOT % 'swap', target={}) + else: + context.can(va_policies.POLICY_ROOT % 'update', + target={'project_id': instance.project_id}) + if only_swap: # NOTE(danms): Original behavior is always call swap on PUT - # FIXME(danms): Check the swap volume policy here self._update_volume_swap(req, instance, id, body) else: # NOTE(danms): New behavior is update any supported attachment # properties first, and then call swap if volumeId differs - # FIXME(danms): Check the volume attachment update policy here self._update_volume_regular(req, instance, id, body) if id != volume_id: self._update_volume_swap(req, instance, id, body) diff --git a/nova/policies/volumes_attachments.py b/nova/policies/volumes_attachments.py index eab7e2f3f6ac..7b229f598fca 100644 --- a/nova/policies/volumes_attachments.py +++ b/nova/policies/volumes_attachments.py @@ -57,8 +57,24 @@ volumes_attachments_policies = [ scope_types=['system', 'project']), policy.DocumentedRuleDefault( name=POLICY_ROOT % 'update', + check_str=base.PROJECT_MEMBER_OR_SYSTEM_ADMIN, + description="""Update a volume attachment. +New 'update' policy about 'swap + update' request (which is possible +only >2.85) only is checked. We expect to be +always superset of this policy permission. +""", + operations=[ + { + 'method': 'PUT', + 'path': + '/servers/{server_id}/os-volume_attachments/{volume_id}' + } + ], + scope_types=['system', 'project']), + policy.DocumentedRuleDefault( + name=POLICY_ROOT % 'swap', check_str=base.SYSTEM_ADMIN, - description="Update a volume attachment", + description="Update a volume attachment with a different volumeId", operations=[ { 'method': 'PUT', diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index 2524c305070f..961cd1a5b1d4 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -113,6 +113,7 @@ policy_data = """ "os_compute_api:os-volumes-attachments:show": "", "os_compute_api:os-volumes-attachments:create": "", "os_compute_api:os-volumes-attachments:update": "", + "os_compute_api:os-volumes-attachments:swap":"", "os_compute_api:os-volumes-attachments:delete": "", "os_compute_api:os-availability-zone:list": "", "os_compute_api:os-availability-zone:detail": "", diff --git a/nova/tests/unit/policies/test_volumes.py b/nova/tests/unit/policies/test_volumes.py index 69579dbc0c33..a61b12385ed3 100644 --- a/nova/tests/unit/policies/test_volumes.py +++ b/nova/tests/unit/policies/test_volumes.py @@ -19,6 +19,7 @@ from nova.api.openstack.compute import volumes as volumes_v21 from nova.compute import vm_states from nova import exception from nova import objects +from nova.objects import block_device as block_device_obj from nova.policies import volumes_attachments as va_policies from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_block_device @@ -160,6 +161,19 @@ class VolumeAttachPolicyTest(base.BasePolicyTest): rule_name, self.controller.create, self.req, FAKE_UUID, body=body) + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + def test_update_volume_attach_policy(self, mock_bdm_save): + rule_name = self.policy_root % "update" + req = fakes.HTTPRequest.blank('', version='2.85') + body = {'volumeAttachment': { + 'volumeId': FAKE_UUID_A, + 'delete_on_termination': True}} + self.common_policy_check(self.admin_or_owner_authorized_contexts, + self.admin_or_owner_unauthorized_contexts, + rule_name, self.controller.update, + req, FAKE_UUID, + FAKE_UUID_A, body=body) + @mock.patch('nova.compute.api.API.detach_volume') def test_delete_volume_attach_policy(self, mock_detach_volume): rule_name = self.policy_root % "delete" @@ -169,14 +183,52 @@ class VolumeAttachPolicyTest(base.BasePolicyTest): self.req, FAKE_UUID, FAKE_UUID_A) @mock.patch('nova.compute.api.API.swap_volume') - def test_update_volume_attach_policy(self, mock_swap_volume): - rule_name = self.policy_root % "update" + def test_swap_volume_attach_policy(self, mock_swap_volume): + rule_name = self.policy_root % "swap" body = {'volumeAttachment': {'volumeId': FAKE_UUID_B}} self.common_policy_check(self.admin_authorized_contexts, self.admin_unauthorized_contexts, rule_name, self.controller.update, self.req, FAKE_UUID, FAKE_UUID_A, body=body) + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + @mock.patch('nova.compute.api.API.swap_volume') + def test_swap_volume_attach_policy_failed(self, + mock_swap_volume, + mock_bdm_save): + """Policy check fails for swap + update due to swap policy failure. + """ + rule_name = self.policy_root % "swap" + req = fakes.HTTPRequest.blank('', version='2.85') + req.environ['nova.context'].user_id = 'other-user' + self.policy.set_rules({rule_name: "user_id:%(user_id)s"}) + body = {'volumeAttachment': {'volumeId': FAKE_UUID_B, + 'delete_on_termination': True}} + exc = self.assertRaises( + exception.PolicyNotAuthorized, self.controller.update, + req, FAKE_UUID, FAKE_UUID_A, body=body) + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule_name, + exc.format_message()) + mock_swap_volume.assert_not_called() + mock_bdm_save.assert_not_called() + + @mock.patch.object(block_device_obj.BlockDeviceMapping, 'save') + @mock.patch('nova.compute.api.API.swap_volume') + def test_pass_swap_and_update_volume_attach_policy(self, + mock_swap_volume, + mock_bdm_save): + rule_name = self.policy_root % "swap" + req = fakes.HTTPRequest.blank('', version='2.85') + body = {'volumeAttachment': {'volumeId': FAKE_UUID_B, + 'delete_on_termination': True}} + self.common_policy_check(self.admin_authorized_contexts, + self.admin_unauthorized_contexts, + rule_name, self.controller.update, + req, FAKE_UUID, FAKE_UUID_A, body=body) + mock_swap_volume.assert_called() + mock_bdm_save.assert_called() + class VolumeAttachScopeTypePolicyTest(VolumeAttachPolicyTest): """Test os-volume-attachments APIs policies with system scope enabled. diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index b89fd675d5dc..01fa4e2cd54e 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -360,7 +360,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase): "os_compute_api:os-console-auth-tokens", "os_compute_api:os-quota-class-sets:update", "os_compute_api:os-server-external-events:create", -"os_compute_api:os-volumes-attachments:update", +"os_compute_api:os-volumes-attachments:swap", "os_compute_api:servers:create:zero_disk_flavor", "os_compute_api:servers:migrations:index", "os_compute_api:servers:migrations:show", @@ -445,6 +445,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase): "os_compute_api:os-volumes", "os_compute_api:os-volumes-attachments:create", "os_compute_api:os-volumes-attachments:delete", +"os_compute_api:os-volumes-attachments:update", ) self.allow_all_rules = ( diff --git a/releasenotes/notes/separate-update-and-swap-volume-policy-for-attachment-e4c20d4907a52fa7.yaml b/releasenotes/notes/separate-update-and-swap-volume-policy-for-attachment-e4c20d4907a52fa7.yaml new file mode 100644 index 000000000000..d9d94ef5f985 --- /dev/null +++ b/releasenotes/notes/separate-update-and-swap-volume-policy-for-attachment-e4c20d4907a52fa7.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + With microversion 2.85, existing policy ``os-volumes-attachments:update`` + is used for updating the resources with the change in its default value + from ``SYSTEM_ADMIN`` to ``PROJECT_MEMBER_OR_SYSTEM_ADMIN``. + New policy ``os-volumes-attachments:swap`` is introduced for swapping + the attachment of servers with default to ``SYSTEM_ADMIN``.