Merge pull request ceph#66041 from rkachach/fix_issue_73625

mgr/cephadm: add tombstones to persist certs info after mgr failover

Reviewed-by: Adam King <[email protected]>

Author: adk3798

src/pybind/mgr/cephadm/tests/test_certmgr.py

@@ -773,13 +773,12 @@ def _fake_prefix_store(prefix):
         assert 'iscsi_ssl_key' in key_store
         assert isinstance(key_store['iscsi_ssl_key'], dict) and len(key_store['iscsi_ssl_key']) == 0
 
-        # Global-scoped key with bad JSON should also not be instantiated (no PrivKey object)
-        # Depending on how you seed known names, it might be absent OR present but falsy.
-        # Accept either: absent OR not a PrivKey instance.
-        if 'mgmt_gateway_ssl_key' in key_store:
-            assert not isinstance(key_store['mgmt_gateway_ssl_key'], PrivKey)
-        else:
-            assert 'mgmt_gateway_ssl_key' not in key_store
+        # Global-scoped key with bad JSON should NOT be hydrated
+        assert 'mgmt_gateway_ssl_key' in key_store
+        obj = key_store.get('mgmt_gateway_ssl_key')
+        # It should be a PrivKey (because globals are pre-seeded) but remain empty/falsy
+        assert isinstance(obj, PrivKey)
+        assert not bool(obj)  # still the tombstone, not parsed content
 
         messages = [r.getMessage() for r in caplog.records
                     if r.levelno >= logging.WARNING and r.name == "cephadm.tlsobject_store"]
@@ -1163,3 +1162,107 @@ def test_validate_tlsobject_name(self):
             self.store._validate_tlsobject_name("per_host1")
         with self.assertRaises(TLSObjectException):
             self.store._validate_tlsobject_name("per_service1")
+
+    def test_register_does_not_double_write_on_second_call(self):
+        """
+        Calling register_object_name() twice for the same name should not create a new KV value
+        nor change the existing one (basic idempotency).
+        """
+        name = "per_service_twice"
+        key = f"{self.store.store_prefix}{name}"
+
+        # First register → writes {}
+        self.store.register_object_name(name, TLSObjectScope.SERVICE)
+        first_val = self.mgr.store.get(key)
+        assert first_val is not None
+        assert json.loads(first_val) == {}
+
+        # Second register → value should remain exactly the same
+        self.store.register_object_name(name, TLSObjectScope.SERVICE)
+        second_val = self.mgr.store.get(key)
+        assert second_val == first_val
+
+    def test_register_writes_tombstone_for_all_scopes(self):
+        """
+        register_object_name() must persist a tombstone for any scope:
+          - SERVICE/HOST → "{}" (empty per-target map)
+          - GLOBAL       → minimal JSON for an empty TLS object
+        """
+        # Fresh names (not present in objects_by_name yet)
+        svc_name = "per_service_new"
+        host_name = "per_host_new"
+        glob_name = "global_cert_new"
+
+        svc_key = f"{self.store.store_prefix}{svc_name}"
+        host_key = f"{self.store.store_prefix}{host_name}"
+        glob_key = f"{self.store.store_prefix}{glob_name}"
+
+        # Sanity: no keys yet
+        assert svc_key not in self.mgr.store
+        assert host_key not in self.mgr.store
+        assert glob_key not in self.mgr.store
+
+        # Act
+        self.store.register_object_name(svc_name, TLSObjectScope.SERVICE)
+        self.store.register_object_name(host_name, TLSObjectScope.HOST)
+        self.store.register_object_name(glob_name, TLSObjectScope.GLOBAL)
+
+        # Assert KV tombstones
+        assert svc_key in self.mgr.store
+        assert host_key in self.mgr.store
+        assert glob_key in self.mgr.store
+
+        # SERVICE/HOST → empty dict
+        assert json.loads(self.mgr.store[svc_key]) == {}
+        assert json.loads(self.mgr.store[host_key]) == {}
+
+        # GLOBAL → minimal JSON for empty MockTLSObject
+        expected_global_min = MockTLSObject.to_json(MockTLSObject())
+        assert json.loads(self.mgr.store[glob_key]) == expected_global_min
+
+        # Also assert scope lists updated & in-memory skeletons created
+        assert svc_name in self.store.service_scoped_objects
+        assert host_name in self.store.host_scoped_objects
+        assert glob_name in self.store.global_scoped_objects
+
+        assert isinstance(self.store.objects_by_name[svc_name], dict) and self.store.objects_by_name[svc_name] == {}
+        assert isinstance(self.store.objects_by_name[host_name], dict) and self.store.objects_by_name[host_name] == {}
+        # For globals, register() seeds an entry; it will be assigned the empty object later when loaded/saved.
+        # We only need to ensure the name exists in the registry.
+        assert glob_name in self.store.objects_by_name
+
+    def test_register_is_idempotent_and_does_not_overwrite_existing_kv(self):
+        """
+        If a store key already has content, register_object_name() must not clobber it.
+        """
+        # Pre-seed non-empty SERVICE and HOST entries, and a GLOBAL entry
+        svc_name = "per_service_existing"
+        host_name = "per_host_existing"
+        glob_name = "global_existing"
+
+        svc_key = f"{self.store.store_prefix}{svc_name}."
+        host_key = f"{self.store.store_prefix}{host_name}."
+        glob_key = f"{self.store.store_prefix}{glob_name}."
+
+        pre_svc_val = {"svcA": MockTLSObject.to_json(MockTLSObject("svc-seed", True, False))}
+        pre_host_val = {"hostA": MockTLSObject.to_json(MockTLSObject("host-seed", True, False))}
+        pre_glob_val = MockTLSObject.to_json(MockTLSObject("glob-seed", True, False))
+
+        self.mgr.set_store(svc_key, json.dumps(pre_svc_val))
+        self.mgr.set_store(host_key, json.dumps(pre_host_val))
+        self.mgr.set_store(glob_key, json.dumps(pre_glob_val))
+
+        # Act: register (should be a no-op on KV content)
+        self.store.register_object_name(svc_name, TLSObjectScope.SERVICE)
+        self.store.register_object_name(host_name, TLSObjectScope.HOST)
+        self.store.register_object_name(glob_name, TLSObjectScope.GLOBAL)
+
+        # Assert KV unchanged
+        assert json.loads(self.mgr.store[svc_key]) == pre_svc_val
+        assert json.loads(self.mgr.store[host_key]) == pre_host_val
+        assert json.loads(self.mgr.store[glob_key]) == pre_glob_val
+
+        # And registry entries exist
+        assert svc_name in self.store.objects_by_name
+        assert host_name in self.store.objects_by_name
+        assert glob_name in self.store.objects_by_name

src/pybind/mgr/cephadm/tlsobject_store.py

@@ -1,6 +1,5 @@
 from typing import (Any,
                     Dict,
-                    Union,
                     List,
                     Tuple,
                     Optional,
@@ -33,12 +32,41 @@ def __init__(self, mgr: 'CephadmOrchestrator',
         self.mgr: CephadmOrchestrator = mgr
         self.cephadm_signed_object_checker = cephadm_signed_obj_checker
         self.tlsobject_class = tlsobject_class
-        all_known_objects_names = [item for sublist in known_objects_names.values() for item in sublist]
-        self.objects_by_name: Dict[str, Any] = {key: {} for key in all_known_objects_names}
         self.service_scoped_objects = known_objects_names[TLSObjectScope.SERVICE]
         self.host_scoped_objects = known_objects_names[TLSObjectScope.HOST]
         self.global_scoped_objects = known_objects_names[TLSObjectScope.GLOBAL]
         self.store_prefix = f'{TLSOBJECT_STORE_PREFIX}{tlsobject_class.STORAGE_PREFIX}.'
+        # initialize objects by name for the different scopes
+        self.objects_by_name: Dict[str, Any] = {}
+        for n in self.service_scoped_objects + self.host_scoped_objects:
+            self.objects_by_name[n] = {}
+        for n in self.global_scoped_objects:
+            self.objects_by_name[n] = self.tlsobject_class()
+
+    def _kv_key(self, obj_name: str) -> str:
+        return self.store_prefix + obj_name
+
+    def _set_store(self, obj_name: str, payload: Any) -> None:
+        self.mgr.set_store(self._kv_key(obj_name), json.dumps(payload))
+
+    def ensure_tombstone(self, obj_name: str, scope: TLSObjectScope) -> None:
+        """
+        Idempotently ensure a tombstone (empty) KV entry for obj_name so the
+        name is rediscovered after manager restarts (see load method).
+
+        - SERVICE/HOST → `{}` (empty per-target map)
+        - GLOBAL       → minimal JSON for an empty TLS object
+        """
+
+        # Do nothing if the TLS object name already exists in the store
+        if self.mgr.get_store_prefix(self._kv_key(obj_name)):
+            return
+
+        if scope in (TLSObjectScope.SERVICE, TLSObjectScope.HOST):
+            self._set_store(obj_name, {})
+        elif scope == TLSObjectScope.GLOBAL:
+            empty = self.tlsobject_class()  # falsy empty instance
+            self._set_store(obj_name, self.tlsobject_class.to_json(empty))
 
     def register_object_name(self, obj_name: str, scope: TLSObjectScope) -> None:
         """
@@ -50,8 +78,15 @@ def register_object_name(self, obj_name: str, scope: TLSObjectScope) -> None:
             ValueError: If an invalid scope is provided.
         """
         if obj_name not in self.objects_by_name:
-            # Initialize an empty dictionary to track TLS objects for this obj_name
-            self.objects_by_name[obj_name] = {}
+            # Initialize an empty dictionary/TLSobj to track TLS objects for this obj_name
+            if scope in (TLSObjectScope.SERVICE, TLSObjectScope.HOST):
+                self.objects_by_name[obj_name] = {}
+            elif scope == TLSObjectScope.GLOBAL:
+                self.objects_by_name[obj_name] = self.tlsobject_class()
+            else:
+                raise ValueError(f"Invalid TLSObjectScope '{scope}' for obj_name '{obj_name}'")
+            # Initialize an empty tombstone for the TLS object(s) in the store
+            self.ensure_tombstone(obj_name, scope)
 
         # Add to the appropriate scope list
         if scope == TLSObjectScope.SERVICE and obj_name not in self.service_scoped_objects:
@@ -105,18 +140,16 @@ def save_tlsobject(self, obj_name: str,
         self._validate_tlsobject_name(obj_name, service_name, host)
         tlsobject = self.tlsobject_class(tlsobject, user_made, editable)
         scope, target = self.get_tlsobject_scope_and_target(obj_name, service_name, host)
-        j: Union[str, Dict[Any, Any], None] = None
         if scope in (TLSObjectScope.SERVICE, TLSObjectScope.HOST):
             self.objects_by_name[obj_name][target] = tlsobject
-            j = {
+            serialized_targets = {
                 key: self.tlsobject_class.to_json(self.objects_by_name[obj_name][key])
                 for key in self.objects_by_name[obj_name]
             }
-            self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+            self._set_store(obj_name, serialized_targets)
         elif scope == TLSObjectScope.GLOBAL:
             self.objects_by_name[obj_name] = tlsobject
-            j = self.tlsobject_class.to_json(tlsobject)
-            self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+            self._set_store(obj_name, self.tlsobject_class.to_json(tlsobject))
         else:
             logger.error(f'Trying to save TLS object name {obj_name} with a not-supported/unknown TLSObjectScope scope {scope.value}')
 
@@ -155,28 +188,27 @@ def rm_tlsobject(self, obj_name: str, service_name: Optional[str] = None, host:
         """
         self._validate_tlsobject_name(obj_name, service_name, host)
         scope, target = self.get_tlsobject_scope_and_target(obj_name, service_name, host)
-        j: Union[str, Dict[Any, Any], None] = None
         if scope in (TLSObjectScope.SERVICE, TLSObjectScope.HOST):
             if obj_name in self.objects_by_name and target in self.objects_by_name[obj_name]:
                 del self.objects_by_name[obj_name][target]
-                j = {
+                serialized_targets = {
                     key: self.tlsobject_class.to_json(self.objects_by_name[obj_name][key])
                     for key in self.objects_by_name[obj_name]
                 }
-                self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+                self._set_store(obj_name, serialized_targets)
                 return True
         elif scope == TLSObjectScope.GLOBAL:
             self.objects_by_name[obj_name] = self.tlsobject_class()
-            j = self.tlsobject_class.to_json(self.objects_by_name[obj_name])
-            self.mgr.set_store(self.store_prefix + obj_name, json.dumps(j))
+            serialized_obj = self.tlsobject_class.to_json(self.objects_by_name[obj_name])
+            self._set_store(obj_name, serialized_obj)
             return True
         else:
             raise TLSObjectException(f'Attempted to remove {self.tlsobject_class.__name__.lower()} for unknown obj_name {obj_name}')
         return False
 
     def _validate_tlsobject_name(self, obj_name: str, service_name: Optional[str] = None, host: Optional[str] = None) -> None:
         cred_type = self.tlsobject_class.__name__.lower()
-        if obj_name not in self.objects_by_name.keys():
+        if obj_name not in self.objects_by_name:
             raise TLSObjectException(f'Attempted to access {cred_type} for unknown TLS object name {obj_name}')
         if obj_name in self.host_scoped_objects and not host:
             raise TLSObjectException(f'Need host to access {cred_type} for TLS object {obj_name}')
@@ -205,39 +237,72 @@ def list_tlsobjects(self) -> List[Tuple[str, TLSObjectProtocol, Optional[str]]]:
         return tlsobjects
 
     def load(self) -> None:
+
+        def _kv_preview(key: str, raw: str, n: int = 20) -> Tuple[str, int, str]:
+            # Safe, short, escaped preview for logs
+            return key, (len(raw) if raw else 0), (raw[:n] if raw else "")
+
         for k, v in self.mgr.get_store_prefix(self.store_prefix).items():
             obj_name = k[len(self.store_prefix):]
             is_cephadm_signed_object = self.cephadm_signed_object_checker(obj_name)
             if not is_cephadm_signed_object and obj_name not in self.objects_by_name:
-                logger.warning(f"TLSObjectStore: Discarding unknown obj_name '{obj_name}'")
+                logger.warning("TLSObjectStore: Discarding unknown obj_name %r", obj_name)
                 continue
 
             try:
                 tls_object_targets = json.loads(v)
+                if not isinstance(tls_object_targets, dict):
+                    key_preview, vlen, vstart = _kv_preview(k, v)
+                    logger.error(
+                        "TLSObjectStore: Invalid data structure for object %r. "
+                        "Expected dict but got %s. key=%r, len=%d, startswith=%r",
+                        obj_name, type(tls_object_targets).__name__,
+                        key_preview, vlen, vstart,
+                    )
+                    continue
             except json.JSONDecodeError as e:
+                key_preview, vlen, vstart = _kv_preview(k, v)
                 logger.warning(
-                    f"TLSObjectStore: Cannot parse JSON for '{obj_name}': "
-                    f"key={k}, len={len(v) if v else 0}, startswith={v[:20]!r}, error={e}"
+                    "TLSObjectStore: Cannot parse JSON for %r: key=%r, len=%d, startswith=%r, error=%r",
+                    obj_name, key_preview, vlen, vstart, e,
                 )
                 continue
             except Exception as e:
+                key_preview, vlen, vstart = _kv_preview(k, v)
                 logger.error(
-                    f"TLSObjectStore: Unexpected error happened while trying to parse JSON for '{obj_name}': "
-                    f"key={k}, len={len(v) if v else 0}, startswith={v[:20]!r}, error={e}"
+                    "TLSObjectStore: Unexpected error while parsing %r: key=%r, len=%d, startswith=%r, error=%r",
+                    obj_name, key_preview, vlen, vstart, e,
+                    exc_info=True,  # include traceback for unexpected errors
                 )
                 continue
 
             if is_cephadm_signed_object or (obj_name in self.service_scoped_objects) or (obj_name in self.host_scoped_objects):
                 if is_cephadm_signed_object and obj_name not in self.host_scoped_objects:
                     self.host_scoped_objects.append(obj_name)
                 self.objects_by_name[obj_name] = {}
-                for target in tls_object_targets:
-                    tlsobject = self.tlsobject_class.from_json(tls_object_targets[target])
-                    if tlsobject:
-                        self.objects_by_name[obj_name][target] = tlsobject
+                for target, payload in tls_object_targets.items():
+                    try:
+                        tlsobject = self.tlsobject_class.from_json(payload)
+                        if tlsobject:  # skip tombstones
+                            self.objects_by_name[obj_name][target] = tlsobject
+                        else:
+                            logger.debug("TLSObjectStore: Skipping tombstone for %r (target %r)", obj_name, target)
+                    except Exception as e:
+                        key_preview, vlen, vstart = _kv_preview(k, str(payload))
+                        logger.warning(
+                            "TLSObjectStore: Failed to decode scoped TLS object %r (target %r): %r. "
+                            "key=%r, len=%d, startswith=%r",
+                            obj_name, target, e, key_preview, vlen, vstart
+                        )
             elif obj_name in self.global_scoped_objects:
-                tlsobject = self.tlsobject_class.from_json(tls_object_targets)
-                if tlsobject:
-                    self.objects_by_name[obj_name] = tlsobject
+                try:
+                    self.objects_by_name[obj_name] = self.tlsobject_class.from_json(tls_object_targets)
+                except Exception as e:
+                    key_preview, vlen, vstart = _kv_preview(k, v)
+                    logger.warning(
+                        "TLSObjectStore: Failed to decode global TLS object %r: %r. "
+                        "key=%r, len=%d, startswith=%r",
+                        obj_name, e, key_preview, vlen, vstart
+                    )
             else:
-                logger.error(f"TLSObjectStore: Found a known TLS object name {obj_name} with unknown scope!")
+                logger.error("TLSObjectStore: Found a known TLS object name %r with unknown scope!", obj_name)

src/pybind/mgr/cephadm/tlsobject_types.py

@@ -156,6 +156,6 @@ def from_json(cls, data: Dict[str, str]) -> 'PrivKey':
             user_made = parse_bool(data.get('user_made', False))
             editable = parse_bool(data.get('editable', False))
         except ValueError as e:
-            raise TLSObjectException(f'Expected field in Cert object to be bool but got another type: {e}')
+            raise TLSObjectException(f'Expected field in PrivKey object to be bool but got another type: {e}')
 
         return cls(key=key, user_made=user_made, editable=editable)