cephadm: _extract_host_info_from_*() refactor

The current implementation doesn't take into account the format is Yaml

This can lead to issue when oob details are passed to host service spec.

For instance, with the following host spec:

```
---
service_type: host
addr: 1.2.3.4
hostname: node1
oob:
  username: root
  password: passw0rd
  addr: 127.0.0.1
```

it is converted to a list like the following:

```
['service_type: host', 'addr: 1.2.3.4', 'hostname: node1',
 'oob:', 'username: root', 'password: passw0rd', 'addr: 127.0.0.1']
```

It was (probably) assumed that the pattern `addr:` would be present only
once. With the introduction of node-proxy, this isn't true anymore.

Now that the cephadm binary can embed some external libraries we can leverage pyyaml.
The idea is to use proper yaml format instead so it is easier to process the data.

Fixes: https://tracker.ceph.com/issues/66165

Signed-off-by: Guillaume Abrioux <[email protected]>

src/cephadm/cephadm.py

@@ -18,7 +18,7 @@
 import time
 import errno
 import ssl
-from typing import Dict, List, Tuple, Optional, Union, Any, Callable, Sequence, TypeVar, cast, Iterable
+from typing import Dict, List, Tuple, Optional, Union, Any, Callable, Sequence, TypeVar, cast
 
 import re
 import uuid
@@ -96,6 +96,7 @@
     try_convert_datetime,
     read_config,
     with_units_to_int,
+    _extract_host_info_from_applied_spec,
 )
 from cephadmlib.file_utils import (
     get_file_timestamp,
@@ -2563,88 +2564,6 @@ def finish_bootstrap_config(
     pass
 
 
-def _extract_host_info_from_applied_spec(f: Iterable[str]) -> List[Dict[str, str]]:
-    # overall goal of this function is to go through an applied spec and find
-    # the hostname (and addr is provided) for each host spec in the applied spec.
-    # Generally, we should be able to just pass the spec to the mgr module where
-    # proper yaml parsing can happen, but for host specs in particular we want to
-    # be able to distribute ssh keys, which requires finding the hostname (and addr
-    # if possible) for each potential host spec in the applied spec.
-
-    specs: List[List[str]] = []
-    current_spec: List[str] = []
-    for line in f:
-        if re.search(r'^---\s+', line):
-            if current_spec:
-                specs.append(current_spec)
-            current_spec = []
-        else:
-            line = line.strip()
-            if line:
-                current_spec.append(line)
-    if current_spec:
-        specs.append(current_spec)
-
-    host_specs: List[List[str]] = []
-    for spec in specs:
-        for line in spec:
-            if 'service_type' in line:
-                try:
-                    _, type = line.split(':')
-                    type = type.strip()
-                    if type == 'host':
-                        host_specs.append(spec)
-                except ValueError as e:
-                    spec_str = '\n'.join(spec)
-                    logger.error(f'Failed to pull service_type from spec:\n{spec_str}. Got error: {e}')
-                break
-            spec_str = '\n'.join(spec)
-            logger.error(f'Failed to find service_type within spec:\n{spec_str}')
-
-    host_dicts = []
-    for s in host_specs:
-        host_dict = _extract_host_info_from_spec(s)
-        # if host_dict is empty here, we failed to pull the hostname
-        # for the host from the spec. This should have already been logged
-        # so at this point we just don't want to include it in our output
-        if host_dict:
-            host_dicts.append(host_dict)
-
-    return host_dicts
-
-
-def _extract_host_info_from_spec(host_spec: List[str]) -> Dict[str, str]:
-    # note:for our purposes here, we only really want the hostname
-    # and address of the host from each of these specs in order to
-    # be able to distribute ssh keys. We will later apply the spec
-    # through the mgr module where proper yaml parsing can be done
-    # The returned dicts from this function should only contain
-    # one or two entries, one (required) for hostname, one (optional) for addr
-    # {
-    #   hostname: <hostname>
-    #   addr: <ip-addr>
-    # }
-    # if we fail to find the hostname, an empty dict is returned
-
-    host_dict = {}  # type: Dict[str, str]
-    for line in host_spec:
-        for field in ['hostname', 'addr']:
-            if field in line:
-                try:
-                    _, field_value = line.split(':')
-                    field_value = field_value.strip()
-                    host_dict[field] = field_value
-                except ValueError as e:
-                    spec_str = '\n'.join(host_spec)
-                    logger.error(f'Error trying to pull {field} from host spec:\n{spec_str}. Got error: {e}')
-
-    if 'hostname' not in host_dict:
-        spec_str = '\n'.join(host_spec)
-        logger.error(f'Could not find hostname in host spec:\n{spec_str}')
-        return {}
-    return host_dict
-
-
 def _distribute_ssh_keys(ctx: CephadmContext, host_info: Dict[str, str], bootstrap_hostname: str) -> int:
     # copy ssh key to hosts in host spec (used for apply spec)
     ssh_key = CEPH_DEFAULT_PUBKEY

src/cephadm/cephadmlib/data_utils.py

@@ -4,15 +4,20 @@
 import os
 import re
 import uuid
+import yaml
+import logging
 
 from configparser import ConfigParser
 
-from typing import Dict, Any, Optional
+from typing import Dict, Any, Optional, Iterable, List
 
 from .constants import DATEFMT, DEFAULT_REGISTRY
 from .exceptions import Error
 
 
+logger = logging.getLogger()
+
+
 def dict_get(
     d: Dict, key: str, default: Any = None, require: bool = False
 ) -> Any:
@@ -197,3 +202,78 @@ def get_legacy_config_fsid(cluster, legacy_dir=None):
         ):
             return config.get('global', 'fsid')
     return None
+
+
+def _extract_host_info_from_applied_spec(
+    f: Iterable[str],
+) -> List[Dict[str, str]]:
+    # overall goal of this function is to go through an applied spec and find
+    # the hostname (and addr is provided) for each host spec in the applied spec.
+    # Generally, we should be able to just pass the spec to the mgr module where
+    # proper yaml parsing can happen, but for host specs in particular we want to
+    # be able to distribute ssh keys, which requires finding the hostname (and addr
+    # if possible) for each potential host spec in the applied spec.
+
+    specs: List[str] = []
+    current_spec: str = ''
+    for line in f:
+        if re.search(r'^---\s+', line):
+            if current_spec:
+                specs.append(current_spec)
+            current_spec = ''
+        else:
+            if line:
+                current_spec += line
+    if current_spec:
+        specs.append(current_spec)
+
+    host_specs: List[Dict[str, Any]] = []
+    for spec in specs:
+        yaml_data = yaml.safe_load(spec)
+        if 'service_type' in yaml_data.keys():
+            if yaml_data['service_type'] == 'host':
+                host_specs.append(yaml_data)
+        else:
+            spec_str = yaml.safe_dump(yaml_data)
+            logger.error(
+                f'Failed to pull service_type from spec:\n{spec_str}.'
+            )
+
+    host_dicts = []
+    for s in host_specs:
+        host_dict = _extract_host_info_from_spec(s)
+        # if host_dict is empty here, we failed to pull the hostname
+        # for the host from the spec. This should have already been logged
+        # so at this point we just don't want to include it in our output
+        if host_dict:
+            host_dicts.append(host_dict)
+
+    return host_dicts
+
+
+def _extract_host_info_from_spec(host_spec: Dict[str, Any]) -> Dict[str, str]:
+    # note:for our purposes here, we only really want the hostname
+    # and address of the host from each of these specs in order to
+    # be able to distribute ssh keys. We will later apply the spec
+    # through the mgr module where proper yaml parsing can be done
+    # The returned dicts from this function should only contain
+    # one or two entries, one (required) for hostname, one (optional) for addr
+    # {
+    #   hostname: <hostname>
+    #   addr: <ip-addr>
+    # }
+    # if we fail to find the hostname, an empty dict is returned
+
+    host_dict = {}  # type: Dict[str, str]
+    for field in ['hostname', 'addr']:
+        try:
+            host_dict[field] = host_spec[field]
+        except KeyError as e:
+            logger.error(
+                f'Error trying to pull {field} from host spec:\n{host_spec}. Got error: {e}'
+            )
+
+    if 'hostname' not in host_dict:
+        logger.error(f'Could not find hostname in host spec:\n{host_spec}')
+        return {}
+    return host_dict