mgr/smb: move logic validating cluster changes to handler

When I first wrote the internal module for working with serialized
objects in the internal store it seems to make sense to do some
validation in the cluster class. At the time it was one of the few
places in the code that would have both the new and old cluster objects
for comparison. However, the code evolved and the handler module grew
more responsibility for validation especially as it needed to do
cross-object validation (do the cluster and share values align, etc).
In addition, I initially couldn't find the code that handled the
validation because I forgot there was validation in internal.py.

Move the logic used to validate that certain properties of a cluster are
unchanged into handler.py and out of internal.py. This makes the classes
in internal.py even more regular as a bonus.

Signed-off-by: John Mulligan <jmulligan@redhat.com>

diff --git a/src/pybind/mgr/smb/handler.py b/src/pybind/mgr/smb/handler.py
index fe82564ee72e0d12f1c5dc6081fc3fece394e485..9a62722d255ec547a6233e313867ede9f430ba1c 100644 (file)
--- a/src/pybind/mgr/smb/handler.py
+++ b/src/pybind/mgr/smb/handler.py
@@ -813,6 +813,8 @@ def _check_cluster_removed(cluster: ClusterRef, staging: _Staging) -> None:
 def _check_cluster_present(cluster: ClusterRef, staging: _Staging) -> None:
     assert isinstance(cluster, resources.Cluster)
     cluster.validate()
+    if not staging.is_new(cluster):
+        _check_cluster_modifications(cluster, staging)
     for auth_ref in _auth_refs(cluster):
         auth = staging.get_join_auth(auth_ref)
         if (
@@ -841,6 +843,34 @@ def _check_cluster_present(cluster: ClusterRef, staging: _Staging) -> None:
             )
 
 
+def _check_cluster_modifications(
+    cluster: resources.Cluster, staging: _Staging
+) -> None:
+    """cluster has some fields we do not permit changing after the cluster has
+    been created.
+    """
+    prev = ClusterEntry.from_store(
+        staging.destination_store, cluster.cluster_id
+    ).get_cluster()
+    if cluster.auth_mode != prev.auth_mode:
+        raise ErrorResult(
+            cluster,
+            'auth_mode value may not be changed',
+            status={'existing_auth_mode': prev.auth_mode},
+        )
+    if cluster.auth_mode == AuthMode.ACTIVE_DIRECTORY:
+        assert prev.domain_settings
+        if not cluster.domain_settings:
+            # should not occur
+            raise ErrorResult(cluster, "domain settings missing from cluster")
+        if cluster.domain_settings.realm != prev.domain_settings.realm:
+            raise ErrorResult(
+                cluster,
+                'domain/realm value may not be changed',
+                status={'existing_domain_realm': prev.domain_settings.realm},
+            )
+
+
 def _parse_earmark(earmark: str) -> dict:
     parts = earmark.split('.')
 

diff --git a/src/pybind/mgr/smb/internal.py b/src/pybind/mgr/smb/internal.py
index 3571ed44400e548c075ac26148063190e2e73a94..57e7a0c0278a801b85ea66e1aca6edb3493a84e6 100644 (file)
--- a/src/pybind/mgr/smb/internal.py
+++ b/src/pybind/mgr/smb/internal.py
@@ -4,7 +4,7 @@ resources that the internal store holds.
 from typing import Collection, Tuple, Type, TypeVar
 
 from . import resources
-from .enums import AuthMode, ConfigNS, State
+from .enums import ConfigNS, State
 from .proto import (
     ConfigEntry,
     ConfigStore,
@@ -14,7 +14,6 @@ from .proto import (
     Simplifiable,
 )
 from .resources import SMBResource
-from .results import ErrorResult
 from .utils import one
 
 T = TypeVar('T')
@@ -108,43 +107,6 @@ class ClusterEntry(ResourceEntry):
     def get_cluster(self) -> resources.Cluster:
         return self.get_resource_type(resources.Cluster)
 
-    def create_or_update(self, resource: Simplifiable) -> State:
-        assert isinstance(resource, resources.Cluster)
-        try:
-            previous = self.config_entry.get()
-        except KeyError:
-            previous = None
-        current = resource.to_simplified()
-        if current == previous:
-            return State.PRESENT
-        elif previous is None:
-            self.config_entry.set(current)
-            return State.CREATED
-        # cluster is special in that is has some fields that we do not
-        # permit changing.
-        prev = getattr(
-            resources.Cluster, '_resource_config'
-        ).object_from_simplified(previous)
-        if resource.auth_mode != prev.auth_mode:
-            raise ErrorResult(
-                resource,
-                'auth_mode value may not be changed',
-                status={'existing_auth_mode': prev.auth_mode},
-            )
-        if resource.auth_mode == AuthMode.ACTIVE_DIRECTORY:
-            assert resource.domain_settings
-            assert prev.domain_settings
-            if resource.domain_settings.realm != prev.domain_settings.realm:
-                raise ErrorResult(
-                    resource,
-                    'domain/realm value may not be changed',
-                    status={
-                        'existing_domain_realm': prev.domain_settings.realm
-                    },
-                )
-        self.config_entry.set(current)
-        return State.UPDATED
-
 
 class ShareEntry(ResourceEntry):
     """Share resource getter/setter for the smb internal data store(s)."""