diff --git a/manila/api/v2/shares.py b/manila/api/v2/shares.py index 7ae03cbc57..d6e8305d21 100644 --- a/manila/api/v2/shares.py +++ b/manila/api/v2/shares.py @@ -664,6 +664,10 @@ def _validate_metadata_for_update(self, req, share_id, metadata, persistent_keys = [] current_share_metadata = db.share_metadata_get(context, share_id) + + new_set_once = self.share_api.validate_set_once_metadata( + context, share_id, metadata) + if delete: _metadata = metadata for key in persistent_keys: @@ -676,7 +680,7 @@ def _validate_metadata_for_update(self, req, share_id, metadata, _metadata = current_share_metadata.copy() _metadata.update(metadata_copy) - return _metadata + return _metadata, new_set_once # NOTE: (ashrod98) original metadata method and policy overrides @wsgi.Controller.api_version("2.0") @@ -691,15 +695,16 @@ def create_metadata(self, req, resource_id, body): if not self.is_valid_body(body, 'metadata'): expl = _('Malformed request body') raise exc.HTTPBadRequest(explanation=expl) - _metadata = self._validate_metadata_for_update(req, resource_id, - body['metadata'], - delete=False) + _metadata, new_set_once = self._validate_metadata_for_update( + req, resource_id, body['metadata'], delete=False) body['metadata'] = _metadata metadata = self._create_metadata(req, resource_id, body) context = req.environ['manila.context'] self.share_api.update_share_from_metadata(context, resource_id, metadata.get('metadata')) + self.share_api.update_share_from_set_once_metadata( + context, resource_id, new_set_once) return metadata @wsgi.Controller.api_version("2.0") @@ -708,14 +713,16 @@ def update_all_metadata(self, req, resource_id, body): if not self.is_valid_body(body, 'metadata'): expl = _('Malformed request body') raise exc.HTTPBadRequest(explanation=expl) - _metadata = self._validate_metadata_for_update(req, resource_id, - body['metadata']) + _metadata, new_set_once = self._validate_metadata_for_update( + req, resource_id, body['metadata']) body['metadata'] = _metadata metadata = self._update_all_metadata(req, resource_id, body) context = req.environ['manila.context'] self.share_api.update_share_from_metadata(context, resource_id, metadata.get('metadata')) + self.share_api.update_share_from_set_once_metadata( + context, resource_id, new_set_once) return metadata @wsgi.Controller.api_version("2.0") @@ -724,15 +731,16 @@ def update_metadata_item(self, req, resource_id, body, key): if not self.is_valid_body(body, 'meta'): expl = _('Malformed request body') raise exc.HTTPBadRequest(explanation=expl) - _metadata = self._validate_metadata_for_update(req, resource_id, - body['metadata'], - delete=False) + _metadata, new_set_once = self._validate_metadata_for_update( + req, resource_id, body['metadata'], delete=False) body['metadata'] = _metadata metadata = self._update_metadata_item(req, resource_id, body, key) context = req.environ['manila.context'] self.share_api.update_share_from_metadata(context, resource_id, metadata.get('metadata')) + self.share_api.update_share_from_set_once_metadata( + context, resource_id, new_set_once) return metadata @wsgi.Controller.api_version("2.0") diff --git a/manila/common/config.py b/manila/common/config.py index 596598ce4f..11a96fba36 100644 --- a/manila/common/config.py +++ b/manila/common/config.py @@ -146,6 +146,13 @@ '(element of the list is , ' 'i.e max_files) can be passed to share drivers as part ' 'of metadata create/update operations.'), + cfg.ListOpt('driver_set_once_metadata', + default=['nfs_full_permission'], + help='Metadata keys that can be set only once per share ' + 'lifetime. On the first set the update is passed to ' + 'the share driver. Any subsequent attempt to update ' + 'a key in this list returns HTTP 400. ' + 'Example: nfs_full_permission'), cfg.ListOpt('driver_updatable_subnet_metadata', default=[], help='Metadata keys that will decide which share network ' diff --git a/manila/exception.py b/manila/exception.py index 682cf86257..c059b98296 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -658,6 +658,12 @@ class InvalidMetadataSize(Invalid): message = _("Invalid metadata size.") +class MetadataSetOnceViolation(Invalid): + message = _("Metadata key '%(key)s' can only be set once and already " + "has value '%(current_value)s'. Updating set-once metadata " + "is not allowed.") + + class SecurityServiceNotFound(NotFound): message = _("Security service %(security_service_id)s could not be found.") diff --git a/manila/share/api.py b/manila/share/api.py index 7ed9bfefbd..cce071ed7b 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -612,6 +612,41 @@ def update_share_from_metadata(self, context, share_id, metadata): self.share_rpcapi.update_share_from_metadata(context, share, driver_metadata) + def validate_set_once_metadata(self, context, share_id, metadata): + """Raise MetadataSetOnceViolation if a set-once key is being re-set. + + Returns a dict of set-once key/value pairs that are new (not yet in + the DB), so the caller can forward them to the driver after the DB + write without a second DB query. + """ + set_once_keys = getattr(CONF, 'driver_set_once_metadata', []) + if not set_once_keys: + return {} + existing = self.db.share_metadata_get(context, share_id) + new_set_once = {} + for key in set_once_keys: + if key not in metadata: + continue + if key in existing: + raise exception.MetadataSetOnceViolation( + key=key, current_value=existing[key]) + new_set_once[key] = metadata[key] + return new_set_once + + def update_share_from_set_once_metadata(self, context, share_id, + new_set_once_metadata): + """Pass new set-once metadata keys to the driver. + + Expects only the keys that were not previously in the DB (as returned + by validate_set_once_metadata). No DB re-query is performed here + because the caller already has the pre-write snapshot of what is new. + """ + if not new_set_once_metadata: + return + share = self.get(context, share_id) + self.share_rpcapi.update_share_from_metadata( + context, share, new_set_once_metadata) + def update_share_network_subnet_from_metadata(self, context, share_network_id, share_network_subnet_id, diff --git a/manila/share/drivers/netapp/dataontap/client/client_cmode.py b/manila/share/drivers/netapp/dataontap/client/client_cmode.py index b69b672083..009c3ef5e8 100644 --- a/manila/share/drivers/netapp/dataontap/client/client_cmode.py +++ b/manila/share/drivers/netapp/dataontap/client/client_cmode.py @@ -2862,6 +2862,29 @@ def update_volume_snapshot_policy(self, volume_name, snapshot_policy): } self.send_request('volume-modify-iter', api_args) + @na_utils.trace + def set_volume_unix_permissions(self, volume_name, unix_permissions): + """Set unix permissions on the specified volume root.""" + api_args = { + 'query': { + 'volume-attributes': { + 'volume-id-attributes': { + 'name': volume_name, + }, + }, + }, + 'attributes': { + 'volume-attributes': { + 'volume-security-attributes': { + 'volume-security-unix-attributes': { + 'permissions': unix_permissions, + }, + }, + }, + }, + } + self.send_request('volume-modify-iter', api_args) + @na_utils.trace def set_sis_config(self, volume_name, api_args): api_args.update({'path': '/vol/%s' % volume_name}) diff --git a/manila/share/drivers/netapp/dataontap/client/client_cmode_rest.py b/manila/share/drivers/netapp/dataontap/client/client_cmode_rest.py index 3c3c52d14d..336ddc2945 100644 --- a/manila/share/drivers/netapp/dataontap/client/client_cmode_rest.py +++ b/manila/share/drivers/netapp/dataontap/client/client_cmode_rest.py @@ -1219,6 +1219,16 @@ def update_volume_snapshot_policy(self, volume_name, snapshot_policy): # update snapshot policy self.send_request(f'/storage/volumes/{uuid}', 'patch', body=body) + @na_utils.trace + def set_volume_unix_permissions(self, volume_name, unix_permissions): + """Set unix permissions on the specified volume root.""" + volume = self._get_volume_by_args(vol_name=volume_name) + uuid = volume['uuid'] + body = { + 'nas.unix_permissions': unix_permissions, + } + self.send_request(f'/storage/volumes/{uuid}', 'patch', body=body) + @na_utils.trace def reset_autosize_attributes(self, aggr, volume_name): '''Reset autosize attributes according to Volume type (RW or DP) diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index 85c84369ba..5e8e16e824 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -6400,6 +6400,29 @@ def update_volume_snapshot_policy(self, share, snapshot_policy, vserver_client.update_volume_snapshot_policy(share_name, snapshot_policy) + @na_utils.trace + def update_nfs_full_permission(self, share, value, share_server=None): + """Set unix permissions to 0777 on the share volume. + + Triggered by the metadata key 'nfs_full_permission' = 'true'. + The value 'false' is intentionally a no-op here; the API layer + prevents re-setting a key that was already written so this method + is only called with value='true'. + """ + value = value.lower() + if value not in ('true', 'false'): + err_msg = _("Invalid nfs_full_permission value '%s'. " + "Accepted values are 'true' or 'false'.") % value + raise exception.NetAppException(err_msg) + if value == 'false': + return + share_name = self._get_backend_share_name(share['id']) + vserver, vserver_client = self._get_vserver(share_server=share_server) + volume = vserver_client.get_volume(share_name) + volume_type = volume.get('type') + if volume_type != 'dp': + vserver_client.set_volume_unix_permissions(share_name, '0777') + @na_utils.trace def update_showmount(self, showmount, share_server=None): showmount = showmount.lower() @@ -6425,6 +6448,7 @@ def update_share_from_metadata(self, context, share, metadata, metadata_update_func_map = { "snapshot_policy": "update_volume_snapshot_policy", "cross_volume_dedupe": "update_cross_volume_dedupe", + "nfs_full_permission": "update_nfs_full_permission", } for k, v in metadata.items(): diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index a7cbcee5da..695c3f7e9b 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -2323,11 +2323,12 @@ def test_create_metadata(self): body = {'metadata': {'key1': 'val1', 'key2': 'val2'}} mock_validate = self.mock_object( self.controller, '_validate_metadata_for_update', - mock.Mock(return_value=body['metadata'])) + mock.Mock(return_value=(body['metadata'], {}))) mock_create = self.mock_object( self.controller, '_create_metadata', mock.Mock(return_value=body)) self.mock_object(share_api.API, 'update_share_from_metadata') + self.mock_object(share_api.API, 'update_share_from_set_once_metadata') req = fakes.HTTPRequest.blank( '/v2/shares/%s/metadata' % id) @@ -2343,11 +2344,12 @@ def test_update_all_metadata(self): body = {'metadata': {'key1': 'val1', 'key2': 'val2'}} mock_validate = self.mock_object( self.controller, '_validate_metadata_for_update', - mock.Mock(return_value=body['metadata'])) + mock.Mock(return_value=(body['metadata'], {}))) mock_update = self.mock_object( self.controller, '_update_all_metadata', mock.Mock(return_value=body)) self.mock_object(share_api.API, 'update_share_from_metadata') + self.mock_object(share_api.API, 'update_share_from_set_once_metadata') req = fakes.HTTPRequest.blank( '/v2/shares/%s/metadata' % id) @@ -2365,6 +2367,69 @@ def test_delete_metadata(self): self.controller.delete_metadata(req, id, 'fake_key') mock_delete.assert_called_once_with(req, id, 'fake_key') + def test_create_metadata_set_once_blocks_re_set(self): + share_id = 'fake_share_id' + self.mock_object(db, 'share_metadata_get', + mock.Mock( + return_value={'nfs_full_permission': 'true'})) + self.mock_object( + share_api.API, 'update_share_from_metadata') + self.mock_object( + share_api.API, 'update_share_from_set_once_metadata') + self.mock_object( + share_api.API, 'validate_set_once_metadata', + mock.Mock(side_effect=exception.MetadataSetOnceViolation( + key='nfs_full_permission', current_value='true'))) + + body = {'metadata': {'nfs_full_permission': 'false'}} + req = fakes.HTTPRequest.blank('/v2/shares/%s/metadata' % share_id) + self.assertRaises(exception.MetadataSetOnceViolation, + self.controller.create_metadata, + req, share_id, body) + + def test_update_metadata_item_set_once_blocks_re_set(self): + share_id = 'fake_share_id' + self.mock_object(db, 'share_metadata_get', + mock.Mock( + return_value={'nfs_full_permission': 'true'})) + self.mock_object( + share_api.API, 'update_share_from_metadata') + self.mock_object( + share_api.API, 'update_share_from_set_once_metadata') + self.mock_object( + share_api.API, 'validate_set_once_metadata', + mock.Mock(side_effect=exception.MetadataSetOnceViolation( + key='nfs_full_permission', current_value='true'))) + + body = {'metadata': {'nfs_full_permission': 'false'}, + 'meta': {'nfs_full_permission': 'false'}} + req = fakes.HTTPRequest.blank( + '/v2/shares/%s/metadata/nfs_full_permission' % share_id) + self.assertRaises(exception.MetadataSetOnceViolation, + self.controller.update_metadata_item, + req, share_id, body, 'nfs_full_permission') + + def test_create_metadata_set_once_first_set_allowed(self): + share_id = 'fake_share_id' + body = {'metadata': {'nfs_full_permission': 'true'}} + CONF.set_override('driver_set_once_metadata', ['nfs_full_permission']) + self.mock_object(db, 'share_metadata_get', + mock.Mock(return_value={})) + self.mock_object(self.controller, '_create_metadata', + mock.Mock(return_value=body)) + mock_updatable = self.mock_object( + share_api.API, 'update_share_from_metadata') + mock_set_once = self.mock_object( + share_api.API, 'update_share_from_set_once_metadata') + + req = fakes.HTTPRequest.blank('/v2/shares/%s/metadata' % share_id) + result = self.controller.create_metadata(req, share_id, body) + + self.assertEqual(body, result) + mock_updatable.assert_called_once() + mock_set_once.assert_called_once_with( + mock.ANY, share_id, {'nfs_full_permission': 'true'}) + def _fake_access_get(self, ctxt, access_id): diff --git a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py index fe845b6f43..cbf70aaf8a 100644 --- a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py @@ -3862,6 +3862,32 @@ def test_update_volume_snapshot_policy(self): self.client.send_request.assert_called_once_with( 'volume-modify-iter', volume_modify_iter_api_args) + def test_set_volume_unix_permissions(self): + self.mock_object(self.client, 'send_request') + + self.client.set_volume_unix_permissions(fake.SHARE_NAME, '0777') + + expected_args = { + 'query': { + 'volume-attributes': { + 'volume-id-attributes': { + 'name': fake.SHARE_NAME, + }, + }, + }, + 'attributes': { + 'volume-attributes': { + 'volume-security-attributes': { + 'volume-security-unix-attributes': { + 'permissions': '0777', + }, + }, + }, + }, + } + self.client.send_request.assert_called_once_with( + 'volume-modify-iter', expected_args) + def test_enable_dedup(self): self.mock_object(self.client, 'send_request') diff --git a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode_rest.py b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode_rest.py index ef99fc7c1f..8197365997 100644 --- a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode_rest.py +++ b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode_rest.py @@ -3238,6 +3238,19 @@ def test_update_volume_snapshot_policy(self): 'patch', body=body) mock_get_vol.assert_called_once_with(vol_name='fake_volume_name') + def test_set_volume_unix_permissions(self): + return_uuid = {'uuid': 'fake_uuid'} + mock_get_vol = self.mock_object(self.client, '_get_volume_by_args', + mock.Mock(return_value=return_uuid)) + mock_sr = self.mock_object(self.client, 'send_request') + + self.client.set_volume_unix_permissions('fake_volume_name', '0777') + + body = {'nas.unix_permissions': '0777'} + mock_sr.assert_called_once_with('/storage/volumes/fake_uuid', + 'patch', body=body) + mock_get_vol.assert_called_once_with(vol_name='fake_volume_name') + @ddt.data(True, False) def test_update_volume_efficiency_attributes(self, status): response = { diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index 8fd5109353..02d81612af 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -10289,6 +10289,45 @@ def test_update_share_from_metadata(self): mock_update_volume_snapshot_policy.assert_called_once_with( share_instance, "daily", share_server=None) + def test_update_share_from_metadata_nfs_full_permission(self): + metadata = {"nfs_full_permission": "true"} + share_instance = fake.SHARE_INSTANCE + mock_update = self.mock_object(self.library, + 'update_nfs_full_permission') + + self.library.update_share_from_metadata(self.context, share_instance, + metadata) + + mock_update.assert_called_once_with(share_instance, "true", + share_server=None) + + def test_update_nfs_full_permission_true(self): + share_instance = fake.SHARE_INSTANCE + share_name = self.library._get_backend_share_name(share_instance['id']) + mock_set_perms = self.mock_object(self.client, + 'set_volume_unix_permissions') + self.mock_object(self.library, '_get_vserver', + mock.Mock(return_value=( + fake.VSERVER1, self.client))) + + self.library.update_nfs_full_permission(share_instance, 'true') + + mock_set_perms.assert_called_once_with(share_name, '0777') + + def test_update_nfs_full_permission_false_is_noop(self): + mock_set_perms = self.mock_object(self.client, + 'set_volume_unix_permissions') + + self.library.update_nfs_full_permission(fake.SHARE_INSTANCE, 'false') + + mock_set_perms.assert_not_called() + + def test_update_nfs_full_permission_invalid_value(self): + self.assertRaises( + exception.NetAppException, + self.library.update_nfs_full_permission, + fake.SHARE_INSTANCE, 'maybe') + def test_update_share_network_subnet_from_metadata(self): metadata = { "showmount": "true", diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index b572030425..688294aaf3 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -745,6 +745,57 @@ def test_update_share_from_metadata(self): mock_call.assert_called_once_with( self.context, 'fake_share', backend_metadata) + def test_validate_set_once_metadata_raises_on_re_set(self): + CONF.set_default('driver_set_once_metadata', ['nfs_full_permission']) + self.mock_object(self.api.db, 'share_metadata_get', + mock.Mock( + return_value={'nfs_full_permission': 'true'})) + + self.assertRaises( + exception.MetadataSetOnceViolation, + self.api.validate_set_once_metadata, + self.context, 'fake_id', {'nfs_full_permission': 'false'}) + + def test_validate_set_once_metadata_allows_first_set(self): + CONF.set_default('driver_set_once_metadata', ['nfs_full_permission']) + self.mock_object(self.api.db, 'share_metadata_get', + mock.Mock(return_value={})) + + result = self.api.validate_set_once_metadata( + self.context, 'fake_id', {'nfs_full_permission': 'true'}) + + self.assertEqual({'nfs_full_permission': 'true'}, result) + + def test_validate_set_once_metadata_no_config_is_noop(self): + CONF.set_default('driver_set_once_metadata', []) + + result = self.api.validate_set_once_metadata( + self.context, 'fake_id', {'nfs_full_permission': 'true'}) + self.assertEqual({}, result) + + def test_update_share_from_set_once_metadata_calls_driver(self): + new_set_once = {'nfs_full_permission': 'true'} + + self.mock_object(self.api, 'get', + mock.Mock(return_value='fake_share')) + mock_rpc = self.mock_object(self.api.share_rpcapi, + 'update_share_from_metadata') + + self.api.update_share_from_set_once_metadata( + self.context, 'fake_id', new_set_once) + + mock_rpc.assert_called_once_with( + self.context, 'fake_share', {'nfs_full_permission': 'true'}) + + def test_update_share_from_set_once_metadata_empty_is_noop(self): + mock_rpc = self.mock_object(self.api.share_rpcapi, + 'update_share_from_metadata') + + self.api.update_share_from_set_once_metadata( + self.context, 'fake_id', {}) + + mock_rpc.assert_not_called() + @ddt.data(True, False) def test_create_public_and_private_share(self, is_public): share, share_data = self._setup_create_mocks(is_public=is_public) diff --git a/releasenotes/notes/driver-set-once-metadata-nfs-full-permission.yaml b/releasenotes/notes/driver-set-once-metadata-nfs-full-permission.yaml new file mode 100644 index 0000000000..945ed58046 --- /dev/null +++ b/releasenotes/notes/driver-set-once-metadata-nfs-full-permission.yaml @@ -0,0 +1,9 @@ +--- +features: + - | + Added a new configuration option ``driver_set_once_metadata``. Metadata + keys listed here can be written to a share exactly **once** over its + lifetime. The value is persisted in the DB and forwarded to the share + driver so it can apply the corresponding backend change. Any subsequent + attempt to update or overwrite a key that is already present in the + share's metadata returns HTTP 400 (Bad Request).