fix: Fix runtime file locations for cloud-init (canonical#4820)

Update various hard-coded filepaths. Also make sure we
bootstrap our Paths() config correctly so that we read
from the configured rundir.

Co-authored-by: Mina Galić <freebsd@igalic.co>
Sponsored by: The FreeBSD Foundation

Fixes canonicalGH-4766

cloudinit/cmd/devel/logs.py

@@ -16,7 +16,6 @@
 from typing import NamedTuple
 
 from cloudinit.cmd.devel import read_cfg_paths
-from cloudinit.helpers import Paths
 from cloudinit.stages import Init
 from cloudinit.subp import ProcessExecutionError, subp
 from cloudinit.temp_utils import tempdir
@@ -28,7 +27,8 @@
     write_file,
 )
 
-CLOUDINIT_RUN_DIR = "/run/cloud-init"
+PATHS = read_cfg_paths()
+CLOUDINIT_RUN_DIR = PATHS.run_dir
 
 
 class ApportFile(NamedTuple):
@@ -144,7 +144,7 @@ def _copytree_rundir_ignore_files(curdir, files):
     ]
     if os.getuid() != 0:
         # Ignore root-permissioned files
-        ignored_files.append(Paths({}).lookups["instance_data_sensitive"])
+        ignored_files.append(PATHS.lookups["instance_data_sensitive"])
     return ignored_files
 
 

cloudinit/cmd/devel/render.py

@@ -18,6 +18,7 @@
 )
 
 NAME = "render"
+CLOUDINIT_RUN_DIR = read_cfg_paths().run_dir
 
 LOG = logging.getLogger(__name__)
 
@@ -40,8 +41,8 @@ def get_parser(parser=None):
         "--instance-data",
         type=str,
         help=(
-            "Optional path to instance-data.json file. Defaults to"
-            " /run/cloud-init/instance-data.json"
+            "Optional path to instance-data.json file. "
+            f"Defaults to {CLOUDINIT_RUN_DIR}"
         ),
     )
     parser.add_argument(

cloudinit/cmd/main.py

@@ -697,12 +697,10 @@ def main_single(name, args):
         return 0
 
 
-def status_wrapper(name, args, data_d=None, link_d=None):
-    if data_d is None:
-        paths = read_cfg_paths()
-        data_d = paths.get_cpath("data")
-    if link_d is None:
-        link_d = os.path.normpath("/run/cloud-init")
+def status_wrapper(name, args):
+    paths = read_cfg_paths()
+    data_d = paths.get_cpath("data")
+    link_d = os.path.normpath(paths.run_dir)
 
     status_path = os.path.join(data_d, "status.json")
     status_link = os.path.join(link_d, "status.json")

cloudinit/helpers.py

@@ -15,7 +15,7 @@
 from io import StringIO
 from time import time
 
-from cloudinit import persistence, type_utils, util
+from cloudinit import persistence, settings, type_utils, util
 from cloudinit.settings import CFG_ENV_NAME, PER_ALWAYS, PER_INSTANCE, PER_ONCE
 
 LOG = logging.getLogger(__name__)
@@ -307,7 +307,7 @@ def __init__(self, path_cfgs: dict, ds=None):
         self.cfgs = path_cfgs
         # Populate all the initial paths
         self.cloud_dir: str = path_cfgs.get("cloud_dir", "/var/lib/cloud")
-        self.run_dir: str = path_cfgs.get("run_dir", "/run/cloud-init")
+        self.run_dir: str = path_cfgs.get("run_dir", settings.DEFAULT_RUN_DIR)
         self.instance_link: str = os.path.join(self.cloud_dir, "instance")
         self.boot_finished: str = os.path.join(
             self.instance_link, "boot-finished"

cloudinit/settings.py

@@ -16,7 +16,7 @@
 
 CLEAN_RUNPARTS_DIR = "/etc/cloud/clean.d"
 
-RUN_CLOUD_CONFIG = "/run/cloud-init/cloud.cfg"
+DEFAULT_RUN_DIR = "/run/cloud-init"
 
 # What u get if no config is provided
 CFG_BUILTIN = {

cloudinit/stages.py

@@ -40,10 +40,10 @@
 from cloudinit.reporting import events
 from cloudinit.settings import (
     CLOUD_CONFIG,
+    DEFAULT_RUN_DIR,
     PER_ALWAYS,
     PER_INSTANCE,
     PER_ONCE,
-    RUN_CLOUD_CONFIG,
 )
 from cloudinit.sources import NetworkConfigSource
 
@@ -275,15 +275,36 @@ def read_cfg(self, extra_fns=None):
             self._cfg = self._read_cfg(extra_fns)
 
     def _read_cfg(self, extra_fns):
-        no_cfg_paths = helpers.Paths({}, self.datasource)
+        """read and merge our configuration"""
+        # No config is passed to Paths() here because we don't yet have a
+        # config to pass. We must bootstrap a config to identify
+        # distro-specific run_dir locations. Once we have the run_dir
+        # we re-read our config with a valid Paths() object. This code has to
+        # assume the location of /etc/cloud/cloud.cfg && /etc/cloud/cloud.cfg.d
+
+        initial_config = self._read_bootstrap_cfg(extra_fns, {})
+        paths = initial_config.get("system_info", {}).get("paths", {})
+
+        # run_dir hasn't changed so we can safely return the config
+        if paths.get("run_dir") in (DEFAULT_RUN_DIR, None):
+            return initial_config
+
+        # run_dir has changed so re-read the config to get a valid one
+        # using the new location of run_dir
+        return self._read_bootstrap_cfg(extra_fns, paths)
+
+    def _read_bootstrap_cfg(self, extra_fns, bootstrapped_config: dict):
+        no_cfg_paths = helpers.Paths(bootstrapped_config, self.datasource)
         instance_data_file = no_cfg_paths.get_runpath(
             "instance_data_sensitive"
         )
         merger = helpers.ConfigMerger(
             paths=no_cfg_paths,
             datasource=self.datasource,
             additional_fns=extra_fns,
-            base_cfg=fetch_base_config(instance_data_file=instance_data_file),
+            base_cfg=fetch_base_config(
+                no_cfg_paths.run_dir, instance_data_file=instance_data_file
+            ),
         )
         return merger.cfg
 
@@ -498,6 +519,9 @@ def is_new_instance(self):
         return ret
 
     def fetch(self, existing="check"):
+        """optionally load datasource from cache, otherwise discover
+        datasource
+        """
         return self._get_data_source(existing=existing)
 
     def instancify(self):
@@ -1076,11 +1100,11 @@ def should_run_on_boot_event():
             return
 
 
-def read_runtime_config():
-    return util.read_conf(RUN_CLOUD_CONFIG)
+def read_runtime_config(run_dir: str):
+    return util.read_conf(os.path.join(run_dir, "cloud.cfg"))
 
 
-def fetch_base_config(*, instance_data_file=None) -> dict:
+def fetch_base_config(run_dir: str, *, instance_data_file=None) -> dict:
     return util.mergemanydict(
         [
             # builtin config, hardcoded in settings.py.
@@ -1090,7 +1114,7 @@ def fetch_base_config(*, instance_data_file=None) -> dict:
                 CLOUD_CONFIG, instance_data_file=instance_data_file
             ),
             # runtime config. I.e., /run/cloud-init/cloud.cfg
-            read_runtime_config(),
+            read_runtime_config(run_dir),
             # Kernel/cmdline parameters override system config
             util.read_conf_from_cmdline(),
         ],

config/cloud.cfg.tmpl

@@ -328,7 +328,7 @@ system_info:
     templates_dir: /etc/cloud/templates/
 {% elif is_bsd %}
   paths:
-    run_dir: /var/run/
+    run_dir: /var/run/cloud-init/
 {% endif %}
 {% if variant == "debian" %}
   package_mirrors:

sysvinit/freebsd/cloudinitlocal.tmpl

@@ -6,7 +6,7 @@
 ``cloudinitlocal`` purposefully does not depend on ``dsidentify``.
 That makes it easy for image builders to disable ``dsidentify``.
 #}
-# REQUIRE: ldconfig mountcritlocal
+# REQUIRE: ldconfig cleanvar
 # BEFORE:  NETWORKING cloudinit cloudconfig cloudfinal
 
 . /etc/rc.subr

sysvinit/freebsd/dsidentify.tmpl

@@ -2,13 +2,7 @@
 #!/bin/sh
 
 # PROVIDE: dsidentify
-{#
-once we are correctly using ``paths.run_dir`` / ``paths.get_runpath()`` in the
-python code-base, we can start thinking about how to bring that into
-``ds-identify`` itself, and then!, then we can depend on (``REQUIRE``)
-``var_run`` instead of ``mountcritlocal`` here.
-#}
-# REQUIRE: mountcritlocal
+# REQUIRE: cleanvar
 # BEFORE:  cloudinitlocal
 
 . /etc/rc.subr

tests/unittests/test_cli.py

@@ -15,9 +15,10 @@
 mock = test_helpers.mock
 
 M_PATH = "cloudinit.cmd.main."
+Tmpdir = namedtuple("Tmpdir", ["tmpdir", "link_d", "data_d"])
 
 
-@pytest.fixture(autouse=False)
+@pytest.fixture()
 def mock_get_user_data_file(mocker, tmpdir):
     yield mocker.patch(
         "cloudinit.cmd.devel.logs._get_user_data_file",
@@ -33,6 +34,19 @@ def disable_setup_logging():
         yield
 
 
+@pytest.fixture()
+def mock_status_wrapper(mocker, tmpdir):
+    link_d = os.path.join(tmpdir, "link")
+    data_d = os.path.join(tmpdir, "data")
+    with mocker.patch(
+        "cloudinit.cmd.main.read_cfg_paths",
+        return_value=mock.Mock(get_cpath=lambda _: data_d),
+    ), mocker.patch(
+        "cloudinit.cmd.main.os.path.normpath", return_value=link_d
+    ):
+        yield Tmpdir(tmpdir, link_d, data_d)
+
+
 class TestCLI:
     def _call_main(self, sysv_args=None):
         if not sysv_args:
@@ -59,29 +73,29 @@ def _call_main(self, sysv_args=None):
             ),
         ],
     )
-    def test_status_wrapper_errors(self, action, name, match, caplog, tmpdir):
-        data_d = tmpdir.join("data")
-        link_d = tmpdir.join("link")
+    def test_status_wrapper_errors(
+        self, action, name, match, caplog, mock_status_wrapper
+    ):
         FakeArgs = namedtuple("FakeArgs", ["action", "local", "mode"])
         my_action = mock.Mock()
 
         myargs = FakeArgs((action, my_action), False, "bogusmode")
         with pytest.raises(ValueError, match=match):
-            cli.status_wrapper(name, myargs, data_d, link_d)
+            cli.status_wrapper(name, myargs)
         assert [] == my_action.call_args_list
 
     @mock.patch("cloudinit.cmd.main.atomic_helper.write_json")
     def test_status_wrapper_init_local_writes_fresh_status_info(
         self,
         m_json,
-        tmpdir,
+        mock_status_wrapper,
     ):
         """When running in init-local mode, status_wrapper writes status.json.
 
         Old status and results artifacts are also removed.
         """
-        data_d = tmpdir.join("data")
-        link_d = tmpdir.join("link")
+        data_d = mock_status_wrapper.data_d
+        link_d = mock_status_wrapper.link_d
         # Write old artifacts which will be removed or updated.
         for _dir in data_d, link_d:
             test_helpers.populate_dir(
@@ -95,7 +109,7 @@ def myaction(name, args):
             return "SomeDatasource", ["an error"]
 
         myargs = FakeArgs(("ignored_name", myaction), True, "bogusmode")
-        cli.status_wrapper("init", myargs, data_d, link_d)
+        cli.status_wrapper("init", myargs)
         # No errors reported in status
         status_v1 = m_json.call_args_list[1][0][1]["v1"]
         assert status_v1.keys() == {
@@ -117,14 +131,14 @@ def myaction(name, args):
 
     @mock.patch("cloudinit.cmd.main.atomic_helper.write_json")
     def test_status_wrapper_init_local_honor_cloud_dir(
-        self, m_json, mocker, tmpdir
+        self, m_json, mocker, mock_status_wrapper
     ):
         """When running in init-local mode, status_wrapper honors cloud_dir."""
-        cloud_dir = tmpdir.join("cloud")
+        cloud_dir = mock_status_wrapper.tmpdir.join("cloud")
         paths = helpers.Paths({"cloud_dir": str(cloud_dir)})
         mocker.patch(M_PATH + "read_cfg_paths", return_value=paths)
-        data_d = cloud_dir.join("data")
-        link_d = tmpdir.join("link")
+        data_d = mock_status_wrapper.data_d
+        link_d = mock_status_wrapper.link_d
 
         FakeArgs = namedtuple("FakeArgs", ["action", "local", "mode"])
 
@@ -133,7 +147,7 @@ def myaction(name, args):
             return "SomeDatasource", ["an_error"]
 
         myargs = FakeArgs(("ignored_name", myaction), True, "bogusmode")
-        cli.status_wrapper("init", myargs, link_d=link_d)  # No explicit data_d
+        cli.status_wrapper("init", myargs)  # No explicit data_d
 
         # Access cloud_dir directly
         status_v1 = m_json.call_args_list[1][0][1]["v1"]
@@ -243,7 +257,12 @@ def test_modules_subcommand_parser(self, m_status_wrapper, subcommand):
     )
     @mock.patch("cloudinit.stages.Init._read_cfg", return_value={})
     def test_conditional_subcommands_from_entry_point_sys_argv(
-        self, m_read_cfg, subcommand, capsys, mock_get_user_data_file, tmpdir
+        self,
+        m_read_cfg,
+        subcommand,
+        capsys,
+        mock_get_user_data_file,
+        mock_status_wrapper,
     ):
         """Subcommands from entry-point are properly parsed from sys.argv."""
         expected_error = f"usage: cloud-init {subcommand}"
@@ -264,7 +283,9 @@ def test_conditional_subcommands_from_entry_point_sys_argv(
             "status",
         ],
     )
-    def test_subcommand_parser(self, subcommand, mock_get_user_data_file):
+    def test_subcommand_parser(
+        self, subcommand, mock_get_user_data_file, mock_status_wrapper
+    ):
         """cloud-init `subcommand` calls its subparser."""
         # Provide -h param to `subcommand` to avoid having to mock behavior.
         out = io.StringIO()