From e39088c30e315fa8d00e6baf1090839f71bf711a Mon Sep 17 00:00:00 2001
From: Sebastian Wagner <sebastian.wagner@suse.com>
Date: Thu, 19 Mar 2020 14:11:38 +0100
Subject: [PATCH] python-common: prevent ServiceSpec of wrong type

Some Python foo to make sure, we don't have an object
like `ServiceSpec('rgw')` of type `ServiceSpec`. Now we have:

>>> type(ServiceSpec('rgw')) == type(RGWSpec('rgw'))

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
---
 src/pybind/mgr/cephadm/tests/test_cephadm.py  |  6 ++-
 src/pybind/mgr/orchestrator/_interface.py     | 11 +++--
 .../ceph/deployment/service_spec.py           | 47 ++++++++++++++-----
 3 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py
index ae5b8c0b977..f6bbad9d2bc 100644
--- a/src/pybind/mgr/cephadm/tests/test_cephadm.py
+++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py
@@ -98,7 +98,7 @@ class TestCephadm(object):
 
             c = cephadm_module.describe_service()
             out = [o.to_json() for o in wait(cephadm_module, c)]
-            assert out == [
+            expected = [
                 {
                     'placement': {'hosts': [{'hostname': 'test', 'name': '', 'network': ''}]},
                     'service_id': 'name',
@@ -111,11 +111,15 @@ class TestCephadm(object):
                         'count': 1,
                         'hosts': [{'hostname': 'test', 'name': '', 'network': ''}]
                     },
+                    'rgw_realm': 'r',
+                    'rgw_zone': 'z',
                     'service_id': 'r.z',
                     'service_type': 'rgw',
                     'status': {'running': 0, 'size': 1}
                 }
             ]
+            assert out == expected
+            assert [ServiceDescription.from_json(o).to_json() for o in expected] == expected
 
     def test_device_ls(self, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
diff --git a/src/pybind/mgr/orchestrator/_interface.py b/src/pybind/mgr/orchestrator/_interface.py
index 2a7ff288e1f..299fa541277 100644
--- a/src/pybind/mgr/orchestrator/_interface.py
+++ b/src/pybind/mgr/orchestrator/_interface.py
@@ -1396,14 +1396,15 @@ class ServiceDescription(object):
     @classmethod
     @handle_type_error
     def from_json(cls, data: dict):
-        status = data.pop('status', {})
-        spec = ServiceSpec.from_json(data)
+        c = data.copy()
+        status = c.pop('status', {})
+        spec = ServiceSpec.from_json(c)
 
-        c = status.copy()
+        c_status = status.copy()
         for k in ['last_refresh', 'created']:
             if k in c:
-                c[k] = datetime.datetime.strptime(c[k], DATEFMT)
-        return cls(spec=spec, **c)
+                c_status[k] = datetime.datetime.strptime(c_status[k], DATEFMT)
+        return cls(spec=spec, **c_status)
 
 
 class InventoryFilter(object):
diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py
index 45474c07aa8..8266c310730 100644
--- a/src/python-common/ceph/deployment/service_spec.py
+++ b/src/python-common/ceph/deployment/service_spec.py
@@ -190,10 +190,11 @@ class PlacementSpec(object):
 
     @classmethod
     def from_json(cls, data):
-        hosts = data.get('hosts', [])
+        c = data.copy()
+        hosts = c.get('hosts', [])
         if hosts:
-            data['hosts'] = [HostPlacementSpec.from_json(host) for host in hosts]
-        _cls = cls(**data)
+            c['hosts'] = [HostPlacementSpec.from_json(host) for host in hosts]
+        _cls = cls(**c)
         _cls.validate()
         return _cls
 
@@ -326,6 +327,34 @@ class ServiceSpec(object):
     KNOWN_SERVICE_TYPES = 'alertmanager crash grafana mds mgr mon nfs ' \
                           'node-exporter osd prometheus rbd-mirror rgw'.split()
 
+    @classmethod
+    def _cls(cls, service_type):
+        from ceph.deployment.drive_group import DriveGroupSpec
+
+        ret = {
+            'rgw': RGWSpec,
+            'nfs': NFSServiceSpec,
+            'osd': DriveGroupSpec
+        }.get(service_type, cls)
+        if ret == ServiceSpec and not service_type:
+            raise ServiceSpecValidationError('Spec needs a "service_type" key.')
+        return ret
+
+    def __new__(cls, *args, **kwargs):
+        """
+        Some Python foo to make sure, we don't have an object
+        like `ServiceSpec('rgw')` of type `ServiceSpec`. Now we have:
+
+        >>> type(ServiceSpec('rgw')) == type(RGWSpec('rgw'))
+        True
+
+        """
+        if cls != ServiceSpec:
+            return object.__new__(cls)
+        service_type = kwargs.get('service_type', args[0] if args else None)
+        sub_cls = cls._cls(service_type)
+        return object.__new__(sub_cls)
+
     def __init__(self,
                  service_type,     # type: str
                  service_id=None,  # type: Optional[str]
@@ -350,17 +379,9 @@ class ServiceSpec(object):
         Initialize 'ServiceSpec' object data from a json structure
         :param json_spec: A valid dict with ServiceSpec
         """
-        from ceph.deployment.drive_group import DriveGroupSpec
 
         service_type = json_spec.get('service_type', '')
-        _cls = {
-            'rgw': RGWSpec,
-            'nfs': NFSServiceSpec,
-            'osd': DriveGroupSpec
-        }.get(service_type, cls)
-
-        if _cls == ServiceSpec and not service_type:
-            raise ServiceSpecValidationError('Spec needs a "service_type" key.')
+        _cls = cls._cls(service_type)
 
         return _cls._from_json_impl(json_spec)  # type: ignore
 
@@ -415,7 +436,7 @@ def servicespec_validate_add(self: ServiceSpec):
 
 
 class NFSServiceSpec(ServiceSpec):
-    def __init__(self, service_id, pool=None, namespace=None, placement=None,
+    def __init__(self, service_id=None, pool=None, namespace=None, placement=None,
                  service_type='nfs', unmanaged=False):
         assert service_type == 'nfs'
         super(NFSServiceSpec, self).__init__(
-- 
2.39.5