Merge pull request ceph#65069 from phlogistonjohn/jjm-fix-ports-remotectl

smb: fix custom ports feature with remote-control sidecar

Reviewed-by: Adam King <[email protected]>
Reviewed-by: Anoop C S <[email protected]>
Reviewed-by: Avan Thakkar <[email protected]>

Author: adk3798

src/cephadm/cephadmlib/daemons/smb.py

@@ -17,6 +17,8 @@
     Tuple,
 )
 
+import ceph.smb.constants
+
 from .. import context_getters
 from .. import daemon_form
 from .. import data_utils
@@ -104,20 +106,35 @@ def conf_interface(self) -> str:
 
 
 class Ports(enum.Enum):
-    SMB = 445
-    SMBMETRICS = 9922
-    CTDB = 4379
-    REMOTE_CONTROL = 54445
+    SMB = ceph.smb.constants.SMB_PORT
+    SMBMETRICS = ceph.smb.constants.SMBMETRICS_PORT
+    CTDB = ceph.smb.constants.CTDB_PORT
+    REMOTE_CONTROL = ceph.smb.constants.REMOTE_CONTROL_PORT
 
     def customized(self, service_ports: Dict[str, int]) -> int:
         """Return a custom port value if it is present in service_ports or the
         default port value if it is not present.
         """
-        port = service_ports.get(self.name.lower())
+        port = service_ports.get(str(self))
         if port:
             return int(port)
         return int(self.value)
 
+    def __str__(self) -> str:
+        # NOTE: mypy is getting the key type below wrong. using reveal_type:
+        # >>> reveal_type(self.SMB)
+        # > note: Revealed type is "builtins.int"
+        # >>> reveal_type(self)
+        # > note: Revealed type is "cephadmlib.daemons.smb.Ports"
+        # maybe newer versions would not hit this issue?
+        names: Dict[Any, str] = {
+            self.SMB: ceph.smb.constants.SMB,
+            self.SMBMETRICS: ceph.smb.constants.SMBMETRICS,
+            self.CTDB: ceph.smb.constants.CTDB,
+            self.REMOTE_CONTROL: ceph.smb.constants.REMOTE_CONTROL,
+        }
+        return names[self]
+
 
 @dataclasses.dataclass(frozen=True)
 class TLSFiles:

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

@@ -144,8 +144,17 @@ def test_check_access_name(value, ok, err_match):
         ({'smb': 4455, 'sbmetrics': 9009}, 'invalid service names'),
         (
             {'smb': 4455, 'smbmetrics': 9999, 'ctdb': 9999},
-            'must not be repeated',
+            'must be unique',
         ),
+        # can't use 445 it's the default smb port!
+        (
+            {'smbmetrics': 445},
+            'must be unique',
+        ),
+        # its valid to swap stuff around (don't do this please)
+        ({'smbmetrics': 4379, 'ctdb': 9922}, None),
+        # remote-control is a valid service name to modify
+        ({'remote-control': 65432}, None),
     ],
 )
 def test_check_custom_ports(value, err_match):

src/pybind/mgr/smb/validation.py

@@ -3,6 +3,8 @@
 import posixpath
 import re
 
+import ceph.smb.constants
+
 # Initially, this regex is pretty restrictive.  But I (JJM) find that
 # starting out more restricitive is better than not because it's generally
 # easier to relax strict rules then discover someone relies on lax rules
@@ -114,14 +116,14 @@ def check_access_name(name: str) -> None:
         raise ValueError('login name may not exceed 128 characters')
 
 
-PORT_SERVICES = {"smb", "ctdb", "smbmetrics"}
 _MAX_PORT = (1 << 16) - 1
 
 
 def check_custom_ports(ports: Optional[Dict[str, int]]) -> None:
     if ports is None:
         return
-    other = set(ports) - PORT_SERVICES
+    _defaults = ceph.smb.constants.DEFAULT_PORTS
+    other = set(ports) - set(_defaults)
     if other:
         raise ValueError(
             "invalid service names for custom ports:"
@@ -132,5 +134,12 @@ def check_custom_ports(ports: Optional[Dict[str, int]]) -> None:
         raise ValueError(
             f'invalid port number(s): {", ".join(sorted(invalid))}'
         )
-    if len(ports) != len(set(ports.values())):
-        raise ValueError('port numbers must not be repeated')
+    # make sure ports are unique per service
+    all_ports = _defaults | ports
+    for port in all_ports.values():
+        using_port = {s for s, p in all_ports.items() if p == port}
+        if len(using_port) > 1:
+            raise ValueError(
+                'port numbers must be unique:'
+                f' {port} used for {", ".join(sorted(using_port))}'
+            )

src/python-common/ceph/deployment/service_spec.py

@@ -35,6 +35,7 @@
 from ceph.deployment.utils import verify_positive_int, verify_non_negative_number
 from ceph.deployment.utils import verify_boolean, verify_enum
 from ceph.utils import is_hex
+from ceph.smb import constants as smbconst
 
 ServiceSpecT = TypeVar('ServiceSpecT', bound='ServiceSpec')
 FuncT = TypeVar('FuncT', bound=Callable)
@@ -3315,8 +3316,8 @@ def convert_list(
 
 class SMBSpec(ServiceSpec):
     service_type = 'smb'
-    _valid_features = {'domain', 'clustered', 'cephfs-proxy', 'remote-control'}
-    _valid_service_names = {'smb', 'smbmetrics', 'ctdb', 'remote-control'}
+    _valid_features = smbconst.FEATURES
+    _valid_service_names = smbconst.SERVICES
     _default_cluster_meta_obj = 'cluster.meta.json'
     _default_cluster_lock_obj = 'cluster.meta.lock'
 
@@ -3430,23 +3431,24 @@ def validate(self) -> None:
                 raise ValueError(
                     f'invalid feature flags: {", ".join(invalid)}'
                 )
-        if 'clustered' in self.features and not self.cluster_meta_uri:
+        _clustered = smbconst.CLUSTERED
+        if _clustered in self.features and not self.cluster_meta_uri:
             # derive a cluster meta uri from config uri by default (if possible)
             self.cluster_meta_uri = self._derive_cluster_uri(
                 self.config_uri,
                 self._default_cluster_meta_obj,
             )
-        if 'clustered' not in self.features and self.cluster_meta_uri:
+        if _clustered not in self.features and self.cluster_meta_uri:
             raise ValueError(
                 'cluster meta uri unsupported when "clustered" feature not set'
             )
-        if 'clustered' in self.features and not self.cluster_lock_uri:
+        if _clustered in self.features and not self.cluster_lock_uri:
             # derive a cluster meta uri from config uri by default (if possible)
             self.cluster_lock_uri = self._derive_cluster_uri(
                 self.config_uri,
                 self._default_cluster_lock_obj,
             )
-        if 'clustered' not in self.features and self.cluster_lock_uri:
+        if _clustered not in self.features and self.cluster_lock_uri:
             raise ValueError(
                 'cluster lock uri unsupported when "clustered" feature not set'
             )
@@ -3465,12 +3467,7 @@ def _derive_cluster_uri(self, uri: str, objname: str) -> str:
         return uri
 
     def _default_ports(self) -> Dict[str, int]:
-        return {
-            'smb': 445,
-            'smbmetrics': 9922,
-            'ctdb': 4379,
-            'remote-control': 54445,
-        }
+        return dict(smbconst.DEFAULT_PORTS)
 
     def service_ports(self) -> Dict[str, int]:
         ports = self._default_ports()
@@ -3479,13 +3476,13 @@ def service_ports(self) -> Dict[str, int]:
         return ports
 
     def metrics_exporter_port(self) -> int:
-        return self.service_ports()['smbmetrics']
+        return self.service_ports()[smbconst.SMBMETRICS]
 
     def get_port_start(self) -> List[int]:
         _ports = self.service_ports()
-        ports = [_ports['smb'], _ports['smbmetrics']]
-        if 'clustered' in self.features:
-            ports.append(_ports['ctdb'])
+        ports = [_ports[smbconst.SMB], _ports[smbconst.SMBMETRICS]]
+        if smbconst.CLUSTERED in self.features:
+            ports.append(_ports[smbconst.CTDB])
         return ports
 
     def strict_cluster_ip_specs(self) -> List[Dict[str, Any]]:

src/python-common/ceph/smb/__init__.py

@@ -0,0 +1,49 @@
+# Shared constant values that apply across all ceph components that manage
+# smb services (mgr module, cephadm mgr module, cephadm binary, etc)
+
+
+# Generic/common names
+SMB = 'smb'
+CTDB = 'ctdb'
+
+
+# Feature names
+CEPHFS_PROXY = 'cephfs-proxy'
+CLUSTERED = 'clustered'
+DOMAIN = 'domain'
+REMOTE_CONTROL = 'remote-control'
+SMBMETRICS = 'smbmetrics'
+
+
+# Features are optional components that can be deployed in a suite of smb
+# related containers. It may run as a separate sidecar or side-effect the
+# configuration of another component.
+FEATURES = {
+    CEPHFS_PROXY,
+    CLUSTERED,
+    DOMAIN,
+    REMOTE_CONTROL,
+}
+
+# Services are components that listen on a "public" network port, to expose
+# network services to remote clients.
+SERVICES = {
+    CTDB,
+    REMOTE_CONTROL,
+    SMB,
+    SMBMETRICS,
+}
+
+
+# Default port values
+SMB_PORT = 445
+SMBMETRICS_PORT = 9922
+CTDB_PORT = 4379
+REMOTE_CONTROL_PORT = 54445
+
+DEFAULT_PORTS = {
+    SMB: SMB_PORT,
+    SMBMETRICS: SMBMETRICS_PORT,
+    CTDB: CTDB_PORT,
+    REMOTE_CONTROL: REMOTE_CONTROL_PORT,
+}