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 <dansmith@redhat.com>

Partial implement blueprint destroy-instance-with-datavolume
Change-Id: I2cbe37b65ceac2efb3b252460dc01d17474e6343

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)

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 <swap policy> is checked. We expect <swap policy> 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',

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": "",

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):
+    def test_swap_volume_attach_policy(self, mock_swap_volume):
-        rule_name = self.policy_root % "update"
+        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.

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 = (

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``.