From 0a70a969d42a29d2585d2b1433017217338b762f Mon Sep 17 00:00:00 2001 From: Chris Peterson Date: Wed, 25 Oct 2023 15:13:26 -0700 Subject: [PATCH 1/2] Revert "autoinstall: Don't use snap env when invoking early and late commands" This reverts commit 39f1ea9cb679b6e5c5cd098a342e1f80cb5e3901. The fix proposed in this patch caused more issues than it fixed. We will have to revisit this in a more nuanced way in the future. In the meantime users can make use of env directly to strip/modify the subcommand environment. --- subiquity/server/controllers/cmdlist.py | 12 +-- .../server/controllers/tests/test_cmdlist.py | 84 ------------------- .../server/controllers/tests/test_snaplist.py | 2 +- subiquitycore/tests/mocks.py | 5 -- subiquitycore/utils.py | 13 ++- 5 files changed, 7 insertions(+), 109 deletions(-) delete mode 100644 subiquity/server/controllers/tests/test_cmdlist.py diff --git a/subiquity/server/controllers/cmdlist.py b/subiquity/server/controllers/cmdlist.py index be3d6455..b363e5e5 100644 --- a/subiquity/server/controllers/cmdlist.py +++ b/subiquity/server/controllers/cmdlist.py @@ -16,7 +16,6 @@ import asyncio import os import shlex -import shutil from typing import List, Sequence, Union import attr @@ -26,7 +25,7 @@ from subiquity.common.types import ApplicationState 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, orig_environ +from subiquitycore.utils import arun_command @attr.s(auto_attribs=True) @@ -79,15 +78,6 @@ class CmdListController(NonInteractiveController): 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: diff --git a/subiquity/server/controllers/tests/test_cmdlist.py b/subiquity/server/controllers/tests/test_cmdlist.py deleted file mode 100644 index 1191ea6c..00000000 --- a/subiquity/server/controllers/tests/test_cmdlist.py +++ /dev/null @@ -1,84 +0,0 @@ -# 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 . - -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() diff --git a/subiquity/server/controllers/tests/test_snaplist.py b/subiquity/server/controllers/tests/test_snaplist.py index ba12aed7..78f2af49 100644 --- a/subiquity/server/controllers/tests/test_snaplist.py +++ b/subiquity/server/controllers/tests/test_snaplist.py @@ -14,7 +14,7 @@ # along with this program. If not, see . import unittest -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, Mock import requests diff --git a/subiquitycore/tests/mocks.py b/subiquitycore/tests/mocks.py index 663b02b8..74f2f66b 100644 --- a/subiquitycore/tests/mocks.py +++ b/subiquitycore/tests/mocks.py @@ -43,9 +43,4 @@ 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 diff --git a/subiquitycore/utils.py b/subiquitycore/utils.py index 10027d1d..2a283b5f 100644 --- a/subiquitycore/utils.py +++ b/subiquitycore/utils.py @@ -19,7 +19,7 @@ import logging import os import random import subprocess -from typing import Any, Dict, List, Optional, Sequence +from typing import Any, Dict, List, Sequence log = logging.getLogger("subiquitycore.utils") @@ -35,18 +35,15 @@ def _clean_env(env, *, locale=True): return env -def orig_environ(env: Optional[Dict[str, str]]) -> Dict[str, str]: +def orig_environ(env): """Generate an environment dict that is suitable for use for running programs that live outside the snap.""" - if env is None: - env: Dict[str, str] = os.environ - - ret: Dict[str, str] = env.copy() - + env = os.environ + ret = env.copy() for key, val in env.items(): if key.endswith("_ORIG"): - key_to_restore: str = key[: -len("_ORIG")] + key_to_restore = key[: -len("_ORIG")] if val: ret[key_to_restore] = val else: From 6c27d656f2821a583b231eddc05b34a2db54a0e0 Mon Sep 17 00:00:00 2001 From: Chris Peterson Date: Wed, 25 Oct 2023 15:34:23 -0700 Subject: [PATCH 2/2] tests: add shared mocks and remove bad import This commit re-adds some of the shared mock fields for testing and removes a bad import from test_snaplist. These are changes that shouldn't have been part of the previously reverted patch: 0a70a969d42a29d2585d2b1433017217338b762f --- subiquity/server/controllers/tests/test_snaplist.py | 2 +- subiquitycore/tests/mocks.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/subiquity/server/controllers/tests/test_snaplist.py b/subiquity/server/controllers/tests/test_snaplist.py index 78f2af49..ba12aed7 100644 --- a/subiquity/server/controllers/tests/test_snaplist.py +++ b/subiquity/server/controllers/tests/test_snaplist.py @@ -14,7 +14,7 @@ # along with this program. If not, see . import unittest -from unittest.mock import AsyncMock, Mock +from unittest.mock import AsyncMock import requests diff --git a/subiquitycore/tests/mocks.py b/subiquitycore/tests/mocks.py index 74f2f66b..663b02b8 100644 --- a/subiquitycore/tests/mocks.py +++ b/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