From 35c0a1df7e9b71a1b08bc5322b57836b0641c122 Mon Sep 17 00:00:00 2001 From: svalouch Date: Thu, 9 Jan 2020 08:03:19 +0100 Subject: [PATCH] move set_mountpoint to ZFS, use new permission helper handling for create_fileset --- src/simplezfs/exceptions.py | 3 ++- src/simplezfs/pe_helper.py | 2 +- src/simplezfs/zfs.py | 41 +++++++++++++++++++++++----- src/simplezfs/zfs_cli.py | 53 ++++++++++--------------------------- 4 files changed, 52 insertions(+), 47 deletions(-) diff --git a/src/simplezfs/exceptions.py b/src/simplezfs/exceptions.py index a6201ae..904620f 100644 --- a/src/simplezfs/exceptions.py +++ b/src/simplezfs/exceptions.py @@ -61,7 +61,8 @@ class ExternalPEHelperException(PEHelperException): ''' Indicates a problem when running the external helper script. ''' - def __init__(self, message: str, returncode: Optional[int], stdout: Optional[str] = None, stderr: Optional[str] = None) -> None: + def __init__(self, message: str, returncode: Optional[int], stdout: Optional[str] = None, + stderr: Optional[str] = None) -> None: ''' :param message: The message to carry. :param returncode: The programs return code. diff --git a/src/simplezfs/pe_helper.py b/src/simplezfs/pe_helper.py index 0037f2e..f1a9fe3 100644 --- a/src/simplezfs/pe_helper.py +++ b/src/simplezfs/pe_helper.py @@ -7,7 +7,7 @@ import subprocess from typing import List, Optional from .exceptions import PEHelperException, ExternalPEHelperException -from .validation import validate_dataset_path, validate_pool_name, validate_property_value +from .validation import validate_dataset_path, validate_pool_name class PEHelperBase: diff --git a/src/simplezfs/zfs.py b/src/simplezfs/zfs.py index 7974592..6a5abba 100644 --- a/src/simplezfs/zfs.py +++ b/src/simplezfs/zfs.py @@ -158,11 +158,10 @@ class ZFS: Sets or changes the mountpoint property of a fileset. While this can be achieved using the generic function :func:`~ZFS.set_property`, it allows for using the privilege escalation (PE) helper if so desired. - If the ``use_pe_helper`` *property* is set and the argument is None, an attempt is made to manipulate the - property and the helper is used only if that fails. If the argument is ``True`` and no helper is set, a normal - attempt is made as well but an error is returned if that does not work due to permissions. If the argument is - ``False``, a normal attempt is made and the helper not used even if the property is ``True`` and a helper is - set, instead returning an error. + The argument ``use_pe_helper`` can overwrite the property of the same name. If the argument is None, the + properties value will be assumed. In any case, the function attempts to set the property on its own first. If + that fails, it evaluates if the PE helper should be used, and will error out if it should be used but has not + been set. If the helper fails, a :class:`~simplezfs.exceptions.PEHelperException` is raised. :param fileset: The fileset to modify. :param mountpoint: The new value for the ``mountpoint`` property. @@ -171,7 +170,24 @@ class ZFS: :raises DatasetNotFound: if the fileset could not be found. :raises ValidationError: if validating the parameters failed. ''' - raise NotImplementedError(f'{self} has not implemented this function') + ds_type = self.get_property(fileset, 'type') + if ds_type != 'filesystem': + raise ValidationError(f'Dataset is not a filesystem and can\'t have its mountpoint set') + + try: + self.set_property(fileset, 'mountpoint', mountpoint) + except PermissionError as e: + if self.pe_helper is not None: + real_use_pe_helper = use_pe_helper if use_pe_helper is not None else self.use_pe_helper + + if real_use_pe_helper: + log.info(f'Permission error when setting mountpoint for "{fileset}", retrying using PE helper') + self.pe_helper.zfs_set_mountpoint(fileset, mountpoint) + else: + log.error(f'Permission error when setting mountpoint for "{fileset}" and not using PE helper') + raise e + else: + log.error(f'Permission error when setting mountpoint for "{fileset}" and PE helper is not set') def set_property(self, dataset: str, key: str, value: str, *, metadata: bool = False, overwrite_metadata_namespace: Optional[str] = None) -> None: @@ -377,6 +393,12 @@ class ZFS: This action requires the ``pe_helper`` on Linux when not running as `root`. + .. note:: + + If the function raises a ``PermissionError`` or ``PEHelperException``, the fileset may have been created, + but is missing its mountpoint property (along with it not being mounted) or other properties to which the + user has no permission to change (not in ``zfs allow``). + :param name: Name of the new fileset (complete path in the ZFS hierarchy). :ppram mountpoint: Convenience parameter for setting/overwriting the moutpoint property :param properties: Dict of native properties to set. @@ -416,6 +438,13 @@ class ZFS: Create a new volume of the given ``size`` (in bytes). If ``sparse`` is **True**, a sparse volume (also known as thin provisioned) will be created. If ``blocksize`` is given, it overwrites the ``blocksize`` property. + .. note:: + + Please read the note in :func:`~simplezfs.ZFS.create_filesystem` for permission handling for filesystems. + Generally, if the user does not have permission to set certain properties, the dataset may or may not have + been created but is missing the properties. It is up to the user of the library to clean up after + catching a ``PermissionError`. + :param name: Name of the new volume (complete path in the ZFS hierarchy). :param size: The size (in `bytes`) for the new volume. :param sparse: Whether to create a sparse volume. Requires ``size`` to be set. diff --git a/src/simplezfs/zfs_cli.py b/src/simplezfs/zfs_cli.py index 8ebad78..bf0a4f4 100644 --- a/src/simplezfs/zfs_cli.py +++ b/src/simplezfs/zfs_cli.py @@ -120,28 +120,6 @@ class ZFSCli(ZFS): res.append(Dataset.from_string(name.strip())) return res - def set_mountpoint(self, fileset: str, mountpoint: str, *, use_pe_helper: Optional[bool] = False) -> None: - real_use_pe_helper = use_pe_helper if use_pe_helper is not None else self.use_pe_helper - ds_type = self.get_property(fileset, 'type') - if ds_type != 'filesystem': - raise ValidationError('Given fileset is not a filesystem, can\'t set mountpoint') - - if not real_use_pe_helper: - self.set_property(fileset, 'mountpoint', mountpoint) - else: - if not self.pe_helper: - raise ValidationError('PE helper should be used, but is not defined') - - args = [self.pe_helper, 'set_mountpoint', fileset, mountpoint] - log.debug(f'set_mountpoint: executing {args}') - proc = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding='utf-8') - log.debug(f'set_mountpoint returncode: {proc.returncode}') - log.debug(f'set_mountpoint stdout: {proc.stdout}') - log.debug(f'set_mountpoint stderr: {proc.stderr}') - # TODO log output - if proc.returncode != 0 or len(proc.stderr) > 0: - self.handle_command_error(proc, fileset) - def handle_command_error(self, proc: subprocess.CompletedProcess, dataset: str = None) -> None: ''' Handles errors that occured while running a command. @@ -151,6 +129,7 @@ class ZFSCli(ZFS): :todo: propper exception! :raises DatasetNotFound: If zfs could not find the dataset it was requested to work with. :raises PropertyNotFound: If the could not find the property it was asked to work with. + :raises PermissionError: If zfs denied the operation, or if only root is allowed to carry it out. :raises Exception: tmp ''' if 'dataset does not exist' in proc.stderr: @@ -164,6 +143,8 @@ class ZFSCli(ZFS): raise PropertyNotFound('invalid property') elif 'permission denied' in proc.stderr: raise PermissionError(proc.stderr) + elif 'filesystem successfully created, but it may only be mounted by root' in proc.stderr: + raise PermissionError(proc.stderr) raise Exception(f'Command execution "{" ".join(proc.args)}" failed: {proc.stderr}') def _set_property(self, dataset: str, key: str, value: str, is_metadata: bool) -> None: @@ -278,32 +259,26 @@ class ZFSCli(ZFS): if self.use_pe_helper: # The mountpoint property may be set, in which case we can run the PE helper. If it is not # set, we'd need to compute it based on the parent, but for now we simply error out. - try: + if properties and 'mountpoint' in properties: mp = properties['mountpoint'] - except KeyError: + if self.pe_helper is not None: + log.info(f'Fileset {name} was created, using pe_helper to set the mountpoint') + self.pe_helper.zfs_set_mountpoint(name, mp) + else: + msg = 'Fileset created partially but no PE helper set' + log.error(msg) + raise PermissionError(msg) + else: msg = 'Mountpoint property not set, can\'t run pe_helper' log.error(msg) raise PermissionError(msg) - log.info(f'Fileset {name} was created, using pe_helper to set the mountpoint') else: log.info(f'Fileset {name} was created, but could not be mounted due to not running as root') raise PermissionError(proc.stderr) - - if self.use_pe_helper: - try: - mp = properties['mountpoint'] - except KeyError: - raise ValidationError('Mountpoint not found in properties') - self._execute_pe_helper('create', recursive=recursive, mountpoint=properties['mountpoint'], name=name) else: - args = [self.__exe, 'create'] - - if recursive: - args += ['-p'] - - args += [name] - print(f'Executing {args}') + log.info('Filesystem created successfully') + return self.get_dataset_info(name) elif dataset_type == DatasetType.VOLUME: assert size is not None