From 263c0f8e73071486ebb87f4dbdfdd4f440299008 Mon Sep 17 00:00:00 2001
From: John Mulligan <jmulligan@redhat.com>
Date: Fri, 11 Oct 2024 14:33:19 -0400
Subject: [PATCH] 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>
---
 src/pybind/mgr/smb/handler.py  | 30 +++++++++++++++++++++++++
 src/pybind/mgr/smb/internal.py | 40 +---------------------------------
 2 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/src/pybind/mgr/smb/handler.py b/src/pybind/mgr/smb/handler.py
index fe82564ee72..9a62722d255 100644
--- 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 3571ed44400..57e7a0c0278 100644
--- 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)."""
-- 
2.39.5