Merge pull request #1821 from dbungert/merge-2023-10-04

Merge 2023 10 04

subiquity/models/subiquity.py

@@ -434,7 +434,7 @@ def _cloud_init_files(self):
             ("etc/cloud/ds-identify.cfg", "policy: enabled\n", 0o644),
         ]
         # Add cloud-init clean hooks to support golden-image creation.
-        cfg_files = ["/" + path for (path, _content, _cmode) in files]
+        cfg_files = ["/" + path for (path, _content, _mode) in files]
         cfg_files.extend(self.network.rendered_config_paths())
         if lsb_release()["release"] not in ("20.04", "22.04"):
             cfg_files.append("/etc/cloud/cloud-init.disabled")
@@ -467,10 +467,10 @@ def configure_cloud_init(self):
         if self.source.current.variant == "core":
             # can probably be supported but requires changes
             return
-        for path, content, cmode in self._cloud_init_files():
+        for path, content, mode in self._cloud_init_files():
             path = os.path.join(self.target, path)
             os.makedirs(os.path.dirname(path), exist_ok=True)
-            write_file(path, content, cmode=cmode)
+            write_file(path, content, mode=mode)
 
     def _media_info(self):
         if os.path.exists("/cdrom/.disk/info"):

subiquity/server/controllers/cmdlist.py

@@ -16,6 +16,7 @@
 import asyncio
 import os
 import shlex
+import shutil
 from typing import List, Sequence, Union
 
 import attr
@@ -25,7 +26,7 @@
 from subiquity.server.controller import NonInteractiveController
 from subiquitycore.async_helpers import run_bg_task
 from subiquitycore.context import with_context
-from subiquitycore.utils import arun_command
+from subiquitycore.utils import arun_command, orig_environ
 
 
 @attr.s(auto_attribs=True)
@@ -78,6 +79,15 @@ async def run(self, context):
         env = self.env()
         for i, cmd in enumerate(tuple(self.builtin_cmds) + tuple(self.cmds)):
             desc = cmd.desc()
+
+            # If the path to the command isn't found on the snap we should
+            # drop the snap specific environment variables.
+            command = shlex.split(desc)[0]
+            path = shutil.which(command)
+            if path is not None:
+                if not path.startswith("/snap"):
+                    env = orig_environ(env)
+
             with context.child("command_{}".format(i), desc):
                 args = cmd.as_args_list()
                 if self.syslog_id:

subiquity/server/controllers/install.py

@@ -689,7 +689,9 @@ async def postinstall(self, *, context):
         autoinstall_config = "#cloud-config\n" + yaml.dump(
             {"autoinstall": self.app.make_autoinstall()}
         )
-        write_file(autoinstall_path, autoinstall_config)
+        # As autoinstall-user-data contains a password hash, we want this file
+        # to have a very restrictive mode and ownership.
+        write_file(autoinstall_path, autoinstall_config, mode=0o400, group="root")
         try:
             if self.supports_apt():
                 packages = await self.get_target_packages(context=context)

subiquity/server/controllers/shutdown.py

@@ -83,6 +83,20 @@ async def _run(self):
         elif self.app.state == ApplicationState.DONE:
             await self.shutdown()
 
+    async def copy_cloud_init_logs(self, target_logs):
+        # Preserve ephemeral boot cloud-init logs if applicable
+        cloudinit_logs = (
+            "/var/log/cloud-init.log",
+            "/var/log/cloud-init-output.log",
+        )
+        for logfile in cloudinit_logs:
+            if not os.path.exists(logfile):
+                continue
+            set_log_perms(logfile)
+            await self.app.command_runner.run(
+                ["cp", "-a", logfile, "/var/log/installer"]
+            )
+
     @with_context()
     async def copy_logs_to_target(self, context):
         if self.opts.dry_run and "copy-logs-fail" in self.app.debug_flags:
@@ -96,24 +110,12 @@ async def copy_logs_to_target(self, context):
         if self.opts.dry_run:
             os.makedirs(target_logs, exist_ok=True)
         else:
-            # Preserve ephemeral boot cloud-init logs if applicable
-            cloudinit_logs = [
-                cloudinit_log
-                for cloudinit_log in (
-                    "/var/log/cloud-init.log",
-                    "/var/log/cloud-init-output.log",
-                )
-                if os.path.exists(cloudinit_log)
-            ]
-            if cloudinit_logs:
-                await self.app.command_runner.run(
-                    ["cp", "-a"] + cloudinit_logs + ["/var/log/installer"]
-                )
+            await self.copy_cloud_init_logs(target_logs)
             await self.app.command_runner.run(
                 ["cp", "-aT", "/var/log/installer", target_logs]
             )
             # Close the permissions from group writes on the target.
-            set_log_perms(target_logs, isdir=True, group_write=False)
+            set_log_perms(target_logs, group_write=False)
 
         journal_txt = os.path.join(target_logs, "installer-journal.txt")
         try:

subiquity/server/controllers/source.py

@@ -13,7 +13,6 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import contextlib
 import logging
 import os
 from typing import Any, Optional
@@ -79,6 +78,7 @@ def __init__(self, app):
         self._handler = None
         self.source_path: Optional[str] = None
         self.ai_source_id: Optional[str] = None
+        self._configured: bool = False
 
     def make_autoinstall(self):
         return {
@@ -148,10 +148,31 @@ def get_handler(
 
     async def configured(self):
         await super().configured()
+        self._configured = True
         self.app.base_model.set_source_variant(self.model.current.variant)
 
     async def POST(self, source_id: str, search_drivers: bool = False) -> None:
-        self.model.search_drivers = search_drivers
-        with contextlib.suppress(KeyError):
-            self.model.current = self.model.get_matching_source(source_id)
-        await self.configured()
+        # Marking the source model configured has an effect on many of the
+        # other controllers. Oftentimes, it would involve cancelling and
+        # restarting various operations.
+        # Let's try not to trigger the event again if we are not changing any
+        # of the settings.
+        changed = False
+        if self.model.search_drivers != search_drivers:
+            changed = True
+            self.model.search_drivers = search_drivers
+
+        try:
+            new_source = self.model.get_matching_source(source_id)
+        except KeyError:
+            # TODO going forward, we should probably stop ignoring unmatched
+            # sources.
+            log.warning("unable to find '%s' in sources catalog", source_id)
+            pass
+        else:
+            if self.model.current != new_source:
+                changed = True
+                self.model.current = new_source
+
+        if changed or not self._configured:
+            await self.configured()

subiquity/server/controllers/tests/test_cmdlist.py

@@ -0,0 +1,84 @@
+# Copyright 2023 Canonical, Ltd.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from unittest import IsolatedAsyncioTestCase, mock
+
+from subiquity.server.controllers import cmdlist
+from subiquity.server.controllers.cmdlist import (
+    CmdListController,
+    Command,
+    EarlyController,
+    LateController,
+)
+from subiquitycore.tests.mocks import make_app
+from subiquitycore.utils import orig_environ
+
+
+@mock.patch.object(cmdlist, "orig_environ", side_effect=orig_environ)
+@mock.patch.object(cmdlist, "arun_command")
+class TestCmdListController(IsolatedAsyncioTestCase):
+    controller_type = CmdListController
+
+    def setUp(self):
+        self.controller = self.controller_type(make_app())
+        self.controller.cmds = [Command(args="some-command", check=False)]
+        snap_env = {
+            "LD_LIBRARY_PATH": "/var/lib/snapd/lib/gl",
+        }
+        self.mocked_os_environ = mock.patch.dict("os.environ", snap_env)
+
+    @mock.patch("shutil.which", return_value="/usr/bin/path/to/bin")
+    async def test_no_snap_env_on_call(
+        self,
+        mocked_shutil,
+        mocked_arun,
+        mocked_orig_environ,
+    ):
+        with self.mocked_os_environ:
+            await self.controller.run()
+            args, kwargs = mocked_arun.call_args
+            call_env = kwargs["env"]
+
+            mocked_orig_environ.assert_called()
+            self.assertNotIn("LD_LIBRARY_PATH", call_env)
+
+    @mock.patch("shutil.which", return_value="/snap/path/to/bin")
+    async def test_with_snap_env_on_call(
+        self,
+        mocked_shutil,
+        mocked_arun,
+        mocked_orig_environ,
+    ):
+        with self.mocked_os_environ:
+            await self.controller.run()
+            args, kwargs = mocked_arun.call_args
+            call_env = kwargs["env"]
+
+            mocked_orig_environ.assert_not_called()
+            self.assertIn("LD_LIBRARY_PATH", call_env)
+
+
+class TestEarlyController(TestCmdListController):
+    controller_type = EarlyController
+
+    def setUp(self):
+        super().setUp()
+
+
+class TestLateController(TestCmdListController):
+    controller_type = LateController
+
+    def setUp(self):
+        super().setUp()

subiquity/server/controllers/tests/test_filesystem.py

@@ -91,8 +91,6 @@ def setUp(self):
         self.app = make_app()
         self.app.opts.bootloader = "UEFI"
         self.app.command_runner = mock.AsyncMock()
-        self.app.report_start_event = mock.Mock()
-        self.app.report_finish_event = mock.Mock()
         self.app.prober = mock.Mock()
         self.app.prober.get_storage = mock.AsyncMock()
         self.app.block_log_dir = "/inexistent"
@@ -1177,8 +1175,6 @@ def setUp(self):
         self.app = make_app()
         self.app.command_runner = mock.AsyncMock()
         self.app.opts.bootloader = "UEFI"
-        self.app.report_start_event = mock.Mock()
-        self.app.report_finish_event = mock.Mock()
         self.app.prober = mock.Mock()
         self.app.prober.get_storage = mock.AsyncMock()
         self.app.snapdapi = snapdapi.make_api_client(AsyncSnapd(get_fake_connection()))

subiquity/server/controllers/tests/test_install.py

@@ -34,8 +34,6 @@ def setUp(self):
         self.controller = InstallController(make_app())
         self.controller.write_config = unittest.mock.Mock()
         self.controller.app.note_file_for_apport = Mock()
-        self.controller.app.report_start_event = Mock()
-        self.controller.app.report_finish_event = Mock()
 
         self.controller.model.target = "/target"
 
@@ -199,8 +197,6 @@ def test_generic_config(self):
 class TestInstallController(unittest.IsolatedAsyncioTestCase):
     def setUp(self):
         self.controller = InstallController(make_app())
-        self.controller.app.report_start_event = Mock()
-        self.controller.app.report_finish_event = Mock()
         self.controller.model.target = tempfile.mkdtemp()
         os.makedirs(os.path.join(self.controller.model.target, "etc/grub.d"))
         self.addCleanup(shutil.rmtree, self.controller.model.target)

subiquity/server/controllers/tests/test_refresh.py

@@ -26,8 +26,6 @@
 class TestRefreshController(SubiTestCase):
     def setUp(self):
         self.app = make_app()
-        self.app.report_start_event = mock.Mock()
-        self.app.report_finish_event = mock.Mock()
         self.app.note_data_for_apport = mock.Mock()
         self.app.prober = mock.Mock()
         self.app.snapdapi = snapdapi.make_api_client(AsyncSnapd(get_fake_connection()))

subiquity/server/controllers/tests/test_snaplist.py

@@ -14,7 +14,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import unittest
-from unittest.mock import AsyncMock, Mock
+from unittest.mock import AsyncMock
 
 import requests
 
@@ -31,8 +31,6 @@ def setUp(self):
         self.model = SnapListModel()
         self.app = make_app()
         self.app.snapd = AsyncMock()
-        self.app.report_start_event = Mock()
-        self.app.report_finish_event = Mock()
 
         self.loader = SnapdSnapInfoLoader(
             self.model, self.app.snapd, "server", self.app.context

subiquitycore/file_util.py

@@ -29,7 +29,7 @@
 log = logging.getLogger("subiquitycore.file_util")
 
 
-def set_log_perms(target, *, isdir=True, group_write=False, mode=None):
+def set_log_perms(target, *, group_write=False, mode=None, group=_DEF_GROUP):
     if os.getuid() != 0:
         log.warning(
             "set_log_perms: running as non-root - not adjusting"
@@ -39,27 +39,24 @@ def set_log_perms(target, *, isdir=True, group_write=False, mode=None):
         return
     if mode is None:
         mode = _DEF_PERMS_FILE
-        if isdir:
+        if os.path.isdir(target):
             mode |= 0o110
         if group_write:
             mode |= 0o020
     os.chmod(target, mode)
-    os.chown(target, -1, grp.getgrnam(_DEF_GROUP).gr_gid)
+    os.chown(target, 0, grp.getgrnam(group).gr_gid)
 
 
 @contextlib.contextmanager
-def open_perms(filename, *, cmode=None):
-    if cmode is None:
-        cmode = _DEF_PERMS_FILE
-
+def open_perms(filename, **kwargs):
     tf = None
     try:
         dirname = os.path.dirname(filename)
         os.makedirs(dirname, exist_ok=True)
         tf = tempfile.NamedTemporaryFile(dir=dirname, delete=False, mode="w")
         yield tf
         tf.close()
-        set_log_perms(tf.name, mode=cmode)
+        set_log_perms(tf.name, **kwargs)
         os.rename(tf.name, filename)
     except OSError as e:
         if tf is not None:

subiquitycore/log.py

@@ -23,7 +23,7 @@ def setup_logger(dir, base="subiquity"):
     os.makedirs(dir, exist_ok=True)
     # Create the log directory in such a way that users in the group may
     # write to this directory in the installation environment.
-    set_log_perms(dir, isdir=True, group_write=True)
+    set_log_perms(dir, group_write=True)
 
     logger = logging.getLogger("")
     logger.setLevel(logging.DEBUG)
@@ -34,7 +34,7 @@ def setup_logger(dir, base="subiquity"):
         nopid_file = os.path.join(dir, "{}-{}.log".format(base, level))
         logfile = "{}.{}".format(nopid_file, os.getpid())
         handler = logging.FileHandler(logfile)
-        set_log_perms(logfile, isdir=False, group_write=False)
+        set_log_perms(logfile, group_write=False)
         # os.symlink cannot replace an existing file or symlink so create
         # it and then rename it over.
         tmplink = logfile + ".link"

subiquitycore/tests/mocks.py

@@ -43,4 +43,9 @@ def make_app(model=None):
     app.opts = mock.Mock()
     app.opts.dry_run = True
     app.scale_factor = 1000
+    app.echo_syslog_id = None
+    app.log_syslog_id = None
+    app.report_start_event = mock.Mock()
+    app.report_finish_event = mock.Mock()
+
     return app