Fix current host IP address being reported as remote host

Author: MetRonnie

cylc/flow/hostuserutil.py

@@ -50,7 +50,12 @@
 import socket
 import sys
 from time import time
-from typing import List, Optional, Tuple
+from typing import (
+    Dict,
+    List,
+    Optional,
+    Tuple,
+)
 
 from cylc.flow.cfgspec.glbl_cfg import glbl_cfg
 
@@ -81,13 +86,21 @@ def get_inst(cls, new=False, expire=None):
             cls._instance = cls(expire)
         return cls._instance
 
-    def __init__(self, expire):
+    def __init__(self, expire: float):
         self.expire_time = time() + expire
-        self._host = None  # preferred name of localhost
-        self._host_exs = {}  # host: socket.gethostbyname_ex(host), ...
-        self._remote_hosts = {}  # host: is_remote, ...
-        self.user_pwent = None
-        self.remote_users = {}
+        self._host: Optional[str] = None  # preferred name of localhost
+        self._host_exs: Dict[  # host: socket.gethostbyname_ex(host), ...
+            str, Tuple[str, List[str], List[str]]
+        ] = {}
+        self._remote_hosts: Dict[str, bool] = {}  # host: is_remote, ...
+        self.user_pwent: Optional[pwd.struct_passwd] = None
+        self.remote_users: Dict[str, bool] = {}
+
+        # On MacOS we have seen different results of socket.gethostbyname_ex()
+        # before and after calling socket.getfqdn() for the 1st time. See
+        # https://github.com/actions/runner-images/issues/8649#issuecomment-1855919367
+        # Call it here at init to ensure we get consistent results from now on.
+        socket.getfqdn()
 
     @staticmethod
     def get_local_ip_address(target):
@@ -117,23 +130,31 @@ def get_host_ip_by_name(target):
     def _get_host_info(
         self, target: Optional[str] = None
     ) -> Tuple[str, List[str], List[str]]:
-        """Return the extended info of the current host."""
+        """Return the extended info of the current host.
+
+        This should return the same result for all possible names or
+        IP addresses of the same host, as well as caching the results,
+        unlike socket.gethostbyname_ex() alone.
+        """
         if target is None:
             target = socket.getfqdn()
-        if IS_MAC_OS and target in {
-            '1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.'
-            '0.0.0.0.0.0.ip6.arpa',
-            '1.0.0.127.in-addr.arpa',
-        }:
-            # Python's socket bindings don't play nicely with mac os
-            # so by default we get the above ip6.arpa address from
-            # socket.getfqdn, note this does *not* match `hostname -f`.
-            # https://github.com/cylc/cylc-flow/issues/2689
-            # https://github.com/cylc/cylc-flow/issues/3595
-            target = socket.gethostname()
         if target not in self._host_exs:
             try:
-                self._host_exs[target] = socket.gethostbyname_ex(target)
+                if IS_MAC_OS and target in {
+                    '1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.'
+                    '0.0.0.0.0.0.ip6.arpa',
+                    '1.0.0.127.in-addr.arpa',
+                }:
+                    # Python's socket bindings don't play nicely with mac os
+                    # so by default we get the above ip6.arpa address from
+                    # socket.getfqdn, note this does *not* match `hostname -f`.
+                    # https://github.com/cylc/cylc-flow/issues/2689
+                    # https://github.com/cylc/cylc-flow/issues/3595
+                    name = socket.gethostname()
+                else:
+                    # Normalise the name or IP address to a FQDN
+                    name = socket.getfqdn(socket.gethostbyaddr(target)[0])
+                self._host_exs[target] = socket.gethostbyname_ex(name)
             except IOError as exc:
                 if exc.filename is None:
                     exc.filename = target
@@ -190,26 +211,32 @@ def _get_user_pwent(self):
             self.remote_users.update(((self.user_pwent.pw_name, False),))
         return self.user_pwent
 
-    def is_remote_host(self, name):
-        """Return True if name has different IP address than the current host.
+    def is_remote_host(self, host: Optional[str]) -> bool:
+        """Return True if the host is not the current host.
+
+        If the given host's primary name does not match the current host's or
+        'localhost', the host is considered remote.
+
+        Args:
+            host: Either a host name or an IP address.
 
         Return False if name is None.
         Return True if host is unknown.
 
         """
-        if name not in self._remote_hosts:
-            if not name or name.startswith("localhost"):
-                # e.g. localhost4.localdomain4
-                self._remote_hosts[name] = False
+        if not host:
+            return False
+        if host not in self._remote_hosts:
+            try:
+                host_name = self._get_host_info(host)[0].lower()
+            except IOError:
+                self._remote_hosts[host] = True
             else:
-                try:
-                    host_info = self._get_host_info(name)
-                except IOError:
-                    self._remote_hosts[name] = True
-                else:
-                    self._remote_hosts[name] = (
-                        host_info != self._get_host_info())
-        return self._remote_hosts[name]
+                this_name = self._get_host_info()[0].lower()
+                self._remote_hosts[host] = (
+                    host_name not in {this_name, 'localhost'}
+                )
+        return self._remote_hosts[host]
 
     def is_remote_user(self, name):
         """Return True if name is not a name of the current user.
@@ -281,9 +308,9 @@ def is_remote_platform(platform):
     return HostUtil.get_inst()._is_remote_platform(platform)
 
 
-def is_remote_host(name):
+def is_remote_host(host: Optional[str]) -> bool:
     """Shorthand for HostUtil.get_inst().is_remote_host(name)."""
-    return HostUtil.get_inst().is_remote_host(name)
+    return HostUtil.get_inst().is_remote_host(host)
 
 
 def is_remote_user(name):

tests/unit/test_hostuserutil.py

@@ -16,32 +16,47 @@
 
 import os
 import re
+from secrets import token_hex
+import socket
 
 import pytest
 
 from cylc.flow.hostuserutil import (
+    HostUtil,
     get_fqdn_by_host,
     get_host,
+    get_host_ip_by_name,
     get_user,
     get_user_home,
     is_remote_host,
-    is_remote_user
+    is_remote_user,
 )
 
 
+LOCALHOST_ALIASES = socket.gethostbyname_ex('localhost')[1]
+
+
 def test_is_remote_user_on_current_user():
     """is_remote_user with current user."""
     assert not is_remote_user(None)
     assert not is_remote_user(os.getenv('USER'))
 
 
-def test_is_remote_host_on_localhost(monkeypatch):
+@pytest.mark.parametrize(
+    'host',
+    [
+        None,
+        'localhost',
+        pytest.param(os.getenv('HOSTNAME'), id="HOSTNAME-env-var"),
+        pytest.param(get_host(), id="get_host()"),
+        pytest.param(get_host_ip_by_name('localhost'), id="localhost-ip"),
+        pytest.param(get_host_ip_by_name(get_host()), id="get_host-ip"),
+        *LOCALHOST_ALIASES,
+    ],
+)
+def test_is_remote_host__localhost(host):
     """is_remote_host with localhost."""
-    assert not is_remote_host(None)
-    assert not is_remote_host('localhost')
-    assert not is_remote_host('localhost4.localhost42')
-    assert not is_remote_host(os.getenv('HOSTNAME'))
-    assert not is_remote_host(get_host())
+    assert not is_remote_host(host)
 
 
 def test_get_fqdn_by_host_on_bad_host():
@@ -73,3 +88,16 @@ def test_get_user():
 def test_get_user_home():
     """get_user_home."""
     assert os.getenv('HOME') == get_user_home()
+
+
+def test_get_host_info__basic():
+    hu = HostUtil(expire=3600)
+    assert hu._get_host_info() == socket.gethostbyname_ex(socket.getfqdn())
+    # Check it handles IP address:
+    ip = get_host_ip_by_name('localhost')
+    assert hu._get_host_info(ip) == socket.gethostbyname_ex('localhost')
+    # Check raised exception for bad host:
+    bad_host = f'nonexist{token_hex(8)}.com'
+    with pytest.raises(IOError) as exc:
+        hu._get_host_info(bad_host)
+    assert bad_host in str(exc.value)

tests/unit/test_task_remote_mgr.py

@@ -17,15 +17,30 @@
 from contextlib import suppress
 from pathlib import Path
 from time import sleep
+from typing import (
+    Any,
+    Optional,
+)
+from unittest.mock import (
+    MagicMock,
+    Mock,
+)
+
 import pytest
-from typing import (Any, Optional)
-from unittest.mock import MagicMock, Mock
 
 from cylc.flow.exceptions import PlatformError
 from cylc.flow.network.client_factory import CommsMeth
 from cylc.flow.task_remote_mgr import (
-    REMOTE_FILE_INSTALL_DONE, REMOTE_INIT_IN_PROGRESS, TaskRemoteMgr)
-from cylc.flow.workflow_files import WorkflowFiles, get_workflow_srv_dir
+    REMOTE_FILE_INSTALL_DONE,
+    REMOTE_INIT_IN_PROGRESS,
+    TaskRemoteMgr,
+)
+from cylc.flow.workflow_files import (
+    WorkflowFiles,
+    get_workflow_srv_dir,
+)
+
+from .test_hostuserutil import LOCALHOST_ALIASES
 
 
 Fixture = Any
@@ -325,8 +340,9 @@ def test_eval_platform_bad(task_remote_mgr_eval):
     'eval_str, remote_cmd_map, expected',
     [
         *shared_eval_params,
-        pytest.param(
-            'localhost4.localdomain4', {}, 'localhost', id="localhost_variant"
+        *(
+            pytest.param(alias, {}, 'localhost', id=alias)
+            for alias in LOCALHOST_ALIASES
         ),
         pytest.param(
             '`other cmd`', {'other cmd': 'nicole brennan'}, 'nicole brennan',