Merge pull request #57294 from phlogistonjohn/jjm-smb-free-customize

mgr/smb: add custom config options to share and cluster resources

Reviewed-by: Adam King <adking@redhat.com>

doc/mgr/smb.rst

@@ -357,6 +357,21 @@ custom_dns
 placement
     Optional. A Ceph Orchestration :ref:`placement specifier
     <orchestrator-cli-placement-spec>`.  Defaults to one host if not provided
+custom_smb_share_options
+    Optional mapping. Specify key-value pairs that will be directly added to
+    the global ``smb.conf`` options (or equivalent) of a Samba server.  Do
+    *not* use this option unless you are prepared to debug the Samba instances
+    yourself.
+
+    This option is meant for developers, feature investigators, and other
+    advanced users to take more direct control of a share's options without
+    needing to make changes to the Ceph codebase. Entries in this map should
+    match parameters in ``smb.conf`` and their values. A special key
+    ``_allow_customization`` must appear somewhere in the mapping with the
+    value of ``i-take-responsibility-for-all-samba-configuration-errors`` as an
+    indicator that the user is aware that using this option can easily break
+    things in ways that the Ceph team can not help with. This special key will
+    automatically be removed from the list of options passed to Samba.
 
 
 .. _join-source-fields:
@@ -465,6 +480,20 @@ cephfs
     provider
         Optional. One of ``samba-vfs`` or ``kcephfs`` (``kcephfs`` is not yet
         supported) . Selects how CephFS storage should be provided to the share
+custom_smb_share_options
+    Optional mapping. Specify key-value pairs that will be directly added to
+    the ``smb.conf`` (or equivalent) of a Samba server.  Do *not* use this
+    option unless you are prepared to debug the Samba instances yourself.
+
+    This option is meant for developers, feature investigators, and other
+    advanced users to take more direct control of a share's options without
+    needing to make changes to the Ceph codebase. Entries in this map should
+    match parameters in ``smb.conf`` and their values. A special key
+    ``_allow_customization`` must appear somewhere in the mapping with the
+    value of ``i-take-responsibility-for-all-samba-configuration-errors`` as an
+    indicator that the user is aware that using this option can easily break
+    things in ways that the Ceph team can not help with. This special key will
+    automatically be removed from the list of options passed to Samba.
 
 The following is an example of a share:
 

src/pybind/mgr/smb/handler.py

@@ -978,7 +978,7 @@ def _generate_share(
         share.cephfs.subvolume,
         share.cephfs.path,
     )
-    return {
+    cfg = {
         # smb.conf options
         'options': {
             'path': path,
@@ -992,6 +992,12 @@ def _generate_share(
             'x:ceph:id': f'{share.cluster_id}.{share.share_id}',
         }
     }
+    # extend share with custom options
+    custom_opts = share.cleaned_custom_smb_share_options
+    if custom_opts:
+        cfg['options'].update(custom_opts)
+        cfg['options']['x:ceph:has_custom_options'] = 'yes'
+    return cfg
 
 
 def _generate_config(
@@ -1016,7 +1022,7 @@ def _generate_config(
         for share in shares
     }
 
-    return {
+    cfg: Dict[str, Any] = {
         'samba-container-config': 'v0',
         'configs': {
             cluster.cluster_id: {
@@ -1042,6 +1048,14 @@ def _generate_config(
         },
         'shares': share_configs,
     }
+    # insert global custom options
+    custom_opts = cluster.cleaned_custom_smb_global_options
+    if custom_opts:
+        # isolate custom config opts into a section for cleanliness
+        gname = f'{cluster.cluster_id}_custom'
+        cfg['configs'][cluster.cluster_id]['globals'].append(gname)
+        cfg['globals'][gname] = {'options': dict(custom_opts)}
+    return cfg
 
 
 def _generate_smb_service_spec(

src/pybind/mgr/smb/module.py

@@ -6,7 +6,7 @@ import orchestrator
 from ceph.deployment.service_spec import PlacementSpec, SMBSpec
 from mgr_module import MgrModule, Option
 
-from . import cli, fs, handler, mon_store, rados_store, resources
+from . import cli, fs, handler, mon_store, rados_store, resources, results
 from .enums import AuthMode, JoinSourceType, UserGroupSourceType
 from .proto import AccessAuthorizer, Simplified
 
@@ -59,11 +59,18 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         )
 
     @cli.SMBCommand('apply', perm='rw')
-    def apply_resources(self, inbuf: str) -> handler.ResultGroup:
+    def apply_resources(self, inbuf: str) -> results.ResultGroup:
         """Create, update, or remove smb configuration resources based on YAML
         or JSON specs
         """
-        return self._handler.apply(resources.load_text(inbuf))
+        try:
+            return self._handler.apply(resources.load_text(inbuf))
+        except resources.InvalidResourceError as err:
+            # convert the exception into a result and return it as the only
+            # item in the result group
+            return results.ResultGroup(
+                [results.InvalidResourceResult(err.resource_data, str(err))]
+            )
 
     @cli.SMBCommand('cluster ls', perm='r')
     def cluster_ls(self) -> List[str]:
@@ -82,7 +89,7 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         define_user_pass: Optional[List[str]] = None,
         custom_dns: Optional[List[str]] = None,
         placement: Optional[str] = None,
-    ) -> handler.Result:
+    ) -> results.Result:
         """Create an smb cluster"""
         domain_settings = None
         user_group_settings = None
@@ -181,7 +188,7 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         return self._handler.apply(to_apply, create_only=True).squash(cluster)
 
     @cli.SMBCommand('cluster rm', perm='rw')
-    def cluster_rm(self, cluster_id: str) -> handler.Result:
+    def cluster_rm(self, cluster_id: str) -> results.Result:
         """Remove an smb cluster"""
         cluster = resources.RemovedCluster(cluster_id=cluster_id)
         return self._handler.apply([cluster]).one()
@@ -207,7 +214,7 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         share_name: str = '',
         subvolume: str = '',
         readonly: bool = False,
-    ) -> handler.Result:
+    ) -> results.Result:
         """Create an smb share"""
         share = resources.Share(
             cluster_id=cluster_id,
@@ -223,7 +230,7 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         return self._handler.apply([share], create_only=True).one()
 
     @cli.SMBCommand('share rm', perm='rw')
-    def share_rm(self, cluster_id: str, share_id: str) -> handler.Result:
+    def share_rm(self, cluster_id: str, share_id: str) -> results.Result:
         """Remove an smb share"""
         share = resources.RemovedShare(
             cluster_id=cluster_id, share_id=share_id

src/pybind/mgr/smb/resourcelib.py

@@ -83,6 +83,7 @@ from typing import (
     Callable,
     Dict,
     Hashable,
+    Iterator,
     List,
     Optional,
     Tuple,
@@ -91,6 +92,7 @@ from typing import (
 import dataclasses
 import logging
 import sys
+from contextlib import contextmanager
 from itertools import chain
 
 from .proto import Self, Simplified
@@ -304,6 +306,7 @@ class Resource:
         self.resource_cls = cls
         self.fields: Dict[str, Field] = {}
         self._on_condition: Optional[Callable[..., bool]] = None
+        self._on_construction_error: Optional[Callable[..., Exception]] = None
 
         for fld in dataclasses.fields(self.resource_cls):
             self.fields[fld.name] = Field.create(fld)
@@ -317,6 +320,12 @@ class Resource:
         """Set a condition function."""
         self._on_condition = cond
 
+    def on_construction_error(self, cond: Callable[..., Exception]) -> None:
+        """Set a function to handle/convert exceptions that occur while
+        constructing objects from simplified data.
+        """
+        self._on_construction_error = cond
+
     def type_name(self) -> str:
         """Return the name of the type managed by this resource."""
         return self.resource_cls.__name__
@@ -330,16 +339,29 @@ class Resource:
         """Given a dict-based unstructured data object return the structured
         object-based equivalent.
         """
-        kw = {}
-        for fld in self.fields.values():
-            value = self._object_field_from_simplified(fld, data)
-            if value is not _unset:
-                kw[fld.name] = value
-        obj = self.resource_cls(**kw)
-        validate = getattr(obj, 'validate', None)
-        if validate:
-            validate()
-        return obj
+        with self._structuring_error_hook(self.resource_cls, data):
+            kw = {}
+            for fld in self.fields.values():
+                value = self._object_field_from_simplified(fld, data)
+                if value is not _unset:
+                    kw[fld.name] = value
+            obj = self.resource_cls(**kw)
+            validate = getattr(obj, 'validate', None)
+            if validate:
+                validate()
+            return obj
+
+    @contextmanager
+    @_xt
+    def _structuring_error_hook(
+        self, resource_cls: Any, data: Simplified
+    ) -> Iterator[None]:
+        try:
+            yield
+        except Exception as err:
+            if self._on_construction_error:
+                raise self._on_construction_error(err, data) from err
+            raise
 
     @_xt
     def _object_field_from_simplified(

src/pybind/mgr/smb/resources.py

@@ -32,6 +32,20 @@ def _present(data: Simplified) -> bool:
     return _get_intent(data) == Intent.PRESENT
 
 
+class InvalidResourceError(ValueError):
+    def __init__(self, msg: str, data: Simplified) -> None:
+        super().__init__(msg)
+        self.resource_data = data
+
+    @classmethod
+    def wrap(cls, err: Exception, data: Simplified) -> Exception:
+        if isinstance(err, ValueError) and not isinstance(
+            err, resourcelib.ResourceTypeError
+        ):
+            return cls(str(err), data)
+        return err
+
+
 class _RBase:
     # mypy doesn't currently (well?) support class decorators adding methods
     # so we use a base class to add this method to all our resource classes.
@@ -104,6 +118,7 @@ class RemovedShare(_RBase):
     @resourcelib.customize
     def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource:
         rc.on_condition(_removed)
+        rc.on_construction_error(InvalidResourceError.wrap)
         return rc
 
 
@@ -119,6 +134,7 @@ class Share(_RBase):
     readonly: bool = False
     browseable: bool = True
     cephfs: Optional[CephFSStorage] = None
+    custom_smb_share_options: Optional[Dict[str, str]] = None
 
     def __post_init__(self) -> None:
         # if name is not given explicitly, take it from the share_id
@@ -138,6 +154,7 @@ class Share(_RBase):
         # currently only cephfs is supported
         if self.cephfs is None:
             raise ValueError('a cephfs configuration is required')
+        validation.check_custom_options(self.custom_smb_share_options)
 
     @property
     def checked_cephfs(self) -> CephFSStorage:
@@ -147,8 +164,13 @@ class Share(_RBase):
     @resourcelib.customize
     def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource:
         rc.on_condition(_present)
+        rc.on_construction_error(InvalidResourceError.wrap)
         return rc
 
+    @property
+    def cleaned_custom_smb_share_options(self) -> Optional[Dict[str, str]]:
+        return validation.clean_custom_options(self.custom_smb_share_options)
+
 
 @resourcelib.component()
 class JoinAuthValues(_RBase):
@@ -226,6 +248,7 @@ class RemovedCluster(_RBase):
     @resourcelib.customize
     def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource:
         rc.on_condition(_removed)
+        rc.on_construction_error(InvalidResourceError.wrap)
         return rc
 
     def validate(self) -> None:
@@ -277,6 +300,7 @@ class Cluster(_RBase):
     domain_settings: Optional[DomainSettings] = None
     user_group_settings: Optional[List[UserGroupSource]] = None
     custom_dns: Optional[List[str]] = None
+    custom_smb_global_options: Optional[Dict[str, str]] = None
     # embedded orchestration placement spec
     placement: Optional[WrappedPlacementSpec] = None
 
@@ -304,12 +328,18 @@ class Cluster(_RBase):
                 raise ValueError(
                     'domain settings not supported for user auth mode'
                 )
+        validation.check_custom_options(self.custom_smb_global_options)
 
     @resourcelib.customize
     def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource:
         rc.on_condition(_present)
+        rc.on_construction_error(InvalidResourceError.wrap)
         return rc
 
+    @property
+    def cleaned_custom_smb_global_options(self) -> Optional[Dict[str, str]]:
+        return validation.clean_custom_options(self.custom_smb_global_options)
+
 
 @resourcelib.resource('ceph.smb.join.auth')
 class JoinAuth(_RBase):
@@ -332,6 +362,7 @@ class JoinAuth(_RBase):
     @resourcelib.customize
     def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource:
         rc.linked_to_cluster.quiet = True
+        rc.on_construction_error(InvalidResourceError.wrap)
         return rc
 
 
@@ -356,6 +387,7 @@ class UsersAndGroups(_RBase):
     @resourcelib.customize
     def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource:
         rc.linked_to_cluster.quiet = True
+        rc.on_construction_error(InvalidResourceError.wrap)
         return rc
 
 

src/pybind/mgr/smb/results.py

@@ -1,4 +1,4 @@
-from typing import Iterator, List, Optional
+from typing import Iterable, Iterator, List, Optional
 
 import errno
 
@@ -56,13 +56,38 @@ class ErrorResult(Result, Exception):
         super().__init__(src, success=False, msg=msg, status=status)
 
 
+class InvalidResourceResult(Result):
+    def __init__(
+        self,
+        resource_data: Simplified,
+        msg: str = '',
+        status: Optional[Simplified] = None,
+    ) -> None:
+        self.resource_data = resource_data
+        self.success = False
+        self.msg = msg
+        self.status = status
+
+    def to_simplified(self) -> Simplified:
+        ds: Simplified = {}
+        ds['resource'] = self.resource_data
+        ds['success'] = self.success
+        if self.msg:
+            ds['msg'] = self.msg
+        if self.status:
+            ds.update(self.status)
+        return ds
+
+
 class ResultGroup:
     """Result of applying multiple smb resource updates to the system."""
 
     # Compatible with object formatter, thus suitable for being returned
     # directly to mgr module.
-    def __init__(self) -> None:
-        self._contents: List[Result] = []
+    def __init__(
+        self, initial_results: Optional[Iterable[Result]] = None
+    ) -> None:
+        self._contents: List[Result] = list(initial_results or [])
 
     def append(self, result: Result) -> None:
         self._contents.append(result)

src/pybind/mgr/smb/tests/test_validation.py

@@ -75,3 +75,38 @@ def test_valid_path(value, valid):
     else:
         with pytest.raises(ValueError):
             smb.validation.check_path(value)
+
+
+def _ovr(value):
+    value[
+        smb.validation.CUSTOM_CAUTION_KEY
+    ] = smb.validation.CUSTOM_CAUTION_VALUE
+    return value
+
+
+@pytest.mark.parametrize(
+    "value,errmatch",
+    [
+        ({"foo": "bar"}, "lack"),
+        (_ovr({"foo": "bar"}), ""),
+        (_ovr({"foo": "bar", "zip": "zap"}), ""),
+        (_ovr({"mod:foo": "bar", "zip": "zap"}), ""),
+        (_ovr({"foo\n": "bar"}), "newlines"),
+        (_ovr({"foo": "bar\n"}), "newlines"),
+        (_ovr({"[foo]": "bar\n"}), "brackets"),
+    ],
+)
+def test_check_custom_options(value, errmatch):
+    if not errmatch:
+        smb.validation.check_custom_options(value)
+    else:
+        with pytest.raises(ValueError, match=errmatch):
+            smb.validation.check_custom_options(value)
+
+
+def test_clean_custom_options():
+    orig = {'foo': 'bar', 'big': 'bad', 'bugs': 'bongo'}
+    updated = _ovr(dict(orig))
+    smb.validation.check_custom_options(updated)
+    assert smb.validation.clean_custom_options(updated) == orig
+    assert smb.validation.clean_custom_options(None) is None

src/pybind/mgr/smb/validation.py

@@ -1,3 +1,5 @@
+from typing import Dict, Optional
+
 import posixpath
 import re
 
@@ -60,3 +62,44 @@ def check_path(value: str) -> None:
     """Raise ValueError if value is not a valid share path."""
     if not valid_path(value):
         raise ValueError(f'{value!r} is not a valid share path')
+
+
+CUSTOM_CAUTION_KEY = '_allow_customization'
+CUSTOM_CAUTION_VALUE = (
+    'i-take-responsibility-for-all-samba-configuration-errors'
+)
+
+
+def check_custom_options(opts: Optional[Dict[str, str]]) -> None:
+    """Raise ValueError if a custom configuration options dict is not valid."""
+    if opts is None:
+        return
+    if opts.get(CUSTOM_CAUTION_KEY) != CUSTOM_CAUTION_VALUE:
+        raise ValueError(
+            'options lack custom override permission key and value'
+            f' (review documentation pertaining to {CUSTOM_CAUTION_KEY})'
+        )
+    for key, value in opts.items():
+        if '[' in key or ']' in key:
+            raise ValueError(
+                f'custom option key may not contain square brackets: {key!r}'
+            )
+        if '\n' in key:
+            raise ValueError(
+                f'custom option key may not contain newlines: {key!r}'
+            )
+        if '\n' in value:
+            raise ValueError(
+                f'custom option value may not contain newlines: {key!r}'
+            )
+
+
+def clean_custom_options(
+    opts: Optional[Dict[str, str]]
+) -> Optional[Dict[str, str]]:
+    """Return a version of the custom options dictionary cleaned of special
+    validation parameters.
+    """
+    if opts is None:
+        return None
+    return {k: v for k, v in opts.items() if k != CUSTOM_CAUTION_KEY}