diff --git a/aws_lambda_builders/actions.py b/aws_lambda_builders/actions.py index 491bcc5a6..7c9155ddf 100644 --- a/aws_lambda_builders/actions.py +++ b/aws_lambda_builders/actions.py @@ -132,13 +132,26 @@ def __init__(self, source_dir, dest_dir): self._dest_dir = dest_dir def execute(self): + # Match CopySourceAction, which this replaces for layers: a dependencies directory that was + # never created (download_dependencies=False against a missing cache) is skipped, not fatal. + if not os.path.isdir(self._source_dir): + LOG.warning("Skipping link operation since source %s does not exist", self._source_dir) + return + source_files = set(os.listdir(self._source_dir)) for source_file in source_files: source_path = Path(self._source_dir, source_file) destination_path = Path(self._dest_dir, source_file) - if destination_path.exists(): + if destination_path.is_symlink() or destination_path.is_file(): + # is_symlink() is checked first and deliberately: a dangling symlink is not + # exists(), so the previous exists() check left it in place and os.symlink then + # failed with FileExistsError. os.remove(destination_path) + elif destination_path.is_dir(): + # A real directory left behind by an earlier copying build. os.remove cannot remove + # it, and leaving it would shadow the symlink we are about to create. + shutil.rmtree(destination_path) else: os.makedirs(destination_path.parent, exist_ok=True) utils.create_symlink_or_copy(str(source_path), str(destination_path)) diff --git a/aws_lambda_builders/utils.py b/aws_lambda_builders/utils.py index d18ccf0a9..46b0af16d 100644 --- a/aws_lambda_builders/utils.py +++ b/aws_lambda_builders/utils.py @@ -15,6 +15,30 @@ LOG = logging.getLogger(__name__) +def _materialize_symlinked_destination(destination: str) -> None: + """ + Replace a symlinked destination with a real copy of what it points at. + + A linking build leaves symlinks into the shared dependencies directory. Copying into one would + follow the link and write outside the destination tree, mutating a cache that later builds + reuse. Materialising it first keeps the merge where the caller asked for it, which is what a + copying build did. + """ + if not os.path.islink(destination): + return + + LOG.debug("Replacing symlinked destination %s with a real copy before copying into it", destination) + link_target = os.path.realpath(destination) + os.unlink(destination) + + if os.path.isdir(link_target): + copytree(link_target, destination) + elif os.path.isfile(link_target): + os.makedirs(os.path.dirname(destination), exist_ok=True) + shutil.copy2(link_target, destination) + # A dangling link leaves nothing to preserve; the caller creates the destination itself. + + def copytree( source: str, destination: str, @@ -48,6 +72,8 @@ def copytree( LOG.warning("Skipping copy operation since source %s does not exist", source) return + _materialize_symlinked_destination(destination) + if not os.path.exists(destination): LOG.debug("Creating target folders at %s", destination) os.makedirs(destination) @@ -86,6 +112,10 @@ def copytree( elif os.path.isdir(new_source): copytree(new_source, new_destination, ignore=ignore, include=include, maintain_symlinks=maintain_symlinks) else: + # copy2 opens the destination for writing and would follow a symlink into the shared + # dependencies directory; it replaces the whole file, so unlinking loses nothing. + if os.path.islink(new_destination): + os.unlink(new_destination) LOG.debug("Copying source file (%s) to destination (%s)", new_source, new_destination) shutil.copy2(new_source, new_destination) @@ -210,7 +240,21 @@ def create_symlink_or_copy(source: str, destination: str) -> None: "consider enabling the necessary settings or privileges on your system to support symbolic links.", exc_info=ex if LOG.isEnabledFor(logging.DEBUG) else None, ) - copytree(source, destination) + if os.path.islink(destination): + # A leftover link is one reason os.symlink raised: the guard above misses a dangling one, + # which is not exists(). Copying through it would write outside the destination tree. + LOG.debug("Removing existing symlink at destination %s before copying", destination) + os.unlink(destination) + # A dependencies directory holds top-level files as well as packages (six.py, *.pth), and + # copytree assumes its source is a directory -- it would makedirs a folder named six.py and + # then raise NotADirectoryError on listdir. + if os.path.isdir(source): + copytree(source, destination) + elif os.path.isfile(source): + os.makedirs(os.path.dirname(destination), exist_ok=True) + shutil.copy2(source, destination) + else: + LOG.warning("Skipping copy operation since source %s does not exist", source) def _is_within_directory(directory: Union[str, os.PathLike], target: Union[str, os.PathLike]) -> bool: diff --git a/aws_lambda_builders/workflows/python_pip/workflow.py b/aws_lambda_builders/workflows/python_pip/workflow.py index d6c84e198..8b72577a8 100644 --- a/aws_lambda_builders/workflows/python_pip/workflow.py +++ b/aws_lambda_builders/workflows/python_pip/workflow.py @@ -115,8 +115,19 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim # folder if self.dependencies_dir and self.combine_dependencies: # when copying downloaded dependencies back to artifacts folder, don't exclude anything - # symlinking python dependencies is disabled for now since it is breaking sam local commands - if False and is_experimental_build_improvements_enabled(self.experimental_flags): + # + # Symlinking is only safe for layers. A layer's artifacts are packed into a tarball + # (which dereferences symlinks) before they reach the local invoke container, whereas a + # function's artifacts are bind-mounted at /var/task, where a symlink pointing outside + # the mount dangles unless the caller passes `sam local invoke --mount-symlinks`. That + # option does not exist on `sam local start-api` / `start-lambda`, so linking function + # dependencies would break those commands -- which is why this was disabled wholesale in + # https://github.com/aws/aws-lambda-builders/pull/391. Keep copying for functions. + # + # The links are absolute, so they only resolve on the machine that built them. SAM CLI's + # container build (`sam build --use-container`) never sends a dependencies_dir over + # JSON-RPC, so it cannot reach this branch; a caller that does must share the path. + if self.is_building_layer and is_experimental_build_improvements_enabled(self.experimental_flags): self._actions.append(LinkSourceAction(self.dependencies_dir, artifacts_dir)) else: self._actions.append(CopySourceAction(self.dependencies_dir, artifacts_dir)) diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index e85b12778..a4229ff3f 100644 --- a/tests/unit/test_actions.py +++ b/tests/unit/test_actions.py @@ -1,3 +1,5 @@ +import os +import tempfile from pathlib import Path from unittest import TestCase from unittest.mock import ANY, patch @@ -13,6 +15,7 @@ CleanUpAction, DependencyManager, LinkSinglePathAction, + LinkSourceAction, ) @@ -265,6 +268,65 @@ def _convert_strings_to_paths(source_dest_list): return map(lambda item: (Path(item[0]), Path(item[1])), source_dest_list) +class TestLinkSourceAction(TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.source_dir = Path(self._tmp.name, "deps") + self.dest_dir = Path(self._tmp.name, "artifacts") + (self.source_dir / "somepkg").mkdir(parents=True) + (self.source_dir / "somepkg" / "__init__.py").write_text("hello") + (self.source_dir / "six.py").write_text("six") + self.dest_dir.mkdir() + + def _execute(self): + LinkSourceAction(str(self.source_dir), str(self.dest_dir)).execute() + + def _assert_linked(self): + for name in ("somepkg", "six.py"): + destination = self.dest_dir / name + self.assertTrue(destination.is_symlink(), f"{name} should be a symlink") + self.assertEqual(os.path.realpath(destination), str((self.source_dir / name).resolve())) + self.assertEqual((self.dest_dir / "somepkg" / "__init__.py").read_text(), "hello") + + def test_links_into_an_empty_destination(self): + self._execute() + self._assert_linked() + + def test_replaces_a_real_directory_left_by_an_earlier_copying_build(self): + # A build that copied dependencies leaves real directories behind. Without --clean they + # survive into the next build, and os.remove() cannot remove a directory. + stale = self.dest_dir / "somepkg" + stale.mkdir() + (stale / "__init__.py").write_text("stale") + (self.dest_dir / "six.py").write_text("stale") + + self._execute() + self._assert_linked() + + def test_replaces_a_dangling_symlink(self): + # A dangling symlink is not exists(), so it used to be left in place and os.symlink then + # raised FileExistsError, which create_symlink_or_copy swallowed into a full copy. + (self.dest_dir / "six.py").symlink_to(self._tmp.name + "/gone") + self.assertFalse((self.dest_dir / "six.py").exists()) + + self._execute() + self._assert_linked() + + def test_is_idempotent(self): + self._execute() + self._execute() + self._assert_linked() + + def test_skips_a_source_that_does_not_exist(self): + # CopySourceAction warns and continues here; the layer path must not turn that into a failure. + missing = Path(self._tmp.name, "never-created") + + LinkSourceAction(str(missing), str(self.dest_dir)).execute() + + self.assertEqual(os.listdir(self.dest_dir), []) + + class TestLinkSinglePathAction(TestCase): @patch("aws_lambda_builders.actions.os.makedirs") @patch("aws_lambda_builders.utils.create_symlink_or_copy") diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index aecb86797..19a1bbeb4 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -1,4 +1,6 @@ +import os import platform +import tempfile from pathlib import Path from unittest import TestCase @@ -29,6 +31,9 @@ def test_must_create_symlink_with_absolute_path(self, patched_copy_tree, patched @patch("aws_lambda_builders.utils.copytree") def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched_path): pathced_os.symlink.side_effect = OSError("Unable to create symlink") + # Without this the mocked Path makes the already-a-symlink branch truthy and the function + # returns before it ever calls os.symlink. + patched_path.return_value.exists.return_value = False source_path = "source/path" destination_path = "destination/path" @@ -40,14 +45,137 @@ def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched @patch("aws_lambda_builders.utils.Path") @patch("aws_lambda_builders.utils.os") @patch("aws_lambda_builders.utils.copytree") - def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched_path): + def test_must_not_copy_when_symlink_succeeds(self, patched_copy_tree, pathced_os, patched_path): + # As above: without this the already-a-symlink early return is taken and os.symlink is never reached. + patched_path.return_value.exists.return_value = False + source_path = "source/path" destination_path = "destination/path" utils.create_symlink_or_copy(source_path, destination_path) - pathced_os.symlink.assert_not_called() + pathced_os.symlink.assert_called_once() patched_copy_tree.assert_not_called() + def test_falls_back_to_copying_a_top_level_file(self): + # A dependencies directory holds files as well as packages, and copytree cannot copy a file. + with tempfile.TemporaryDirectory() as tmp: + source = Path(tmp, "six.py") + source.write_text("body") + destination = Path(tmp, "artifacts", "six.py") + + with patch("aws_lambda_builders.utils.os.symlink", side_effect=OSError("privilege not held")): + utils.create_symlink_or_copy(str(source), str(destination)) + + self.assertTrue(destination.is_file()) + self.assertEqual(destination.read_text(), "body") + + def test_falls_back_to_copying_a_package_directory(self): + with tempfile.TemporaryDirectory() as tmp: + source = Path(tmp, "somepkg") + source.mkdir() + (source / "__init__.py").write_text("body") + destination = Path(tmp, "artifacts", "somepkg") + + with patch("aws_lambda_builders.utils.os.symlink", side_effect=OSError("privilege not held")): + utils.create_symlink_or_copy(str(source), str(destination)) + + self.assertTrue(destination.is_dir()) + self.assertEqual((destination / "__init__.py").read_text(), "body") + + def test_fallback_skips_a_source_that_does_not_exist(self): + # maintain_symlinks passes raw os.readlink() output, which is often relative to the link + # rather than the CWD (npm's node_modules/.bin entries), so the fallback must skip it. + with tempfile.TemporaryDirectory() as tmp: + destination = Path(tmp, "artifacts", "tsc") + + with patch("aws_lambda_builders.utils.os.symlink", side_effect=OSError("privilege not held")): + utils.create_symlink_or_copy("../typescript/bin/tsc", str(destination)) + + self.assertFalse(destination.exists()) + + def test_fallback_does_not_copy_through_a_dangling_destination_link(self): + # A dangling link at the destination is not exists(), so the already-a-symlink guard misses + # it and os.symlink raises FileExistsError. The fallback must not then copy through it. + with tempfile.TemporaryDirectory() as tmp: + source = Path(tmp, "six.py") + source.write_text("body") + outside = Path(tmp, "outside.txt") + destination = Path(tmp, "artifacts", "six.py") + destination.parent.mkdir() + destination.symlink_to(str(outside)) + + utils.create_symlink_or_copy(str(source), str(destination)) + + self.assertFalse(outside.exists(), "copy followed a dangling link outside the destination tree") + self.assertFalse(destination.is_symlink()) + self.assertEqual(destination.read_text(), "body") + + +class Test_copytree(TestCase): + def test_does_not_write_through_a_symlinked_destination(self): + """A linking build leaves symlinks into the shared dependencies directory. Copying the + source tree over a colliding name must stay inside the destination tree rather than + following the link and mutating a cache that later builds reuse.""" + with tempfile.TemporaryDirectory() as tmp: + deps = Path(tmp, "deps", "requests") + deps.mkdir(parents=True) + (deps / "__init__.py").write_text("dependency") + + artifacts = Path(tmp, "artifacts") + artifacts.mkdir() + os.symlink(str(deps), str(artifacts / "requests")) + + source = Path(tmp, "source", "requests") + source.mkdir(parents=True) + (source / "my_helper.py").write_text("user code") + + utils.copytree(str(Path(tmp, "source")), str(artifacts)) + + self.assertFalse((deps / "my_helper.py").exists(), "source leaked into the dependencies directory") + self.assertFalse((artifacts / "requests").is_symlink()) + self.assertEqual((artifacts / "requests" / "my_helper.py").read_text(), "user code") + self.assertEqual((artifacts / "requests" / "__init__.py").read_text(), "dependency") + + def test_does_not_write_through_a_symlinked_file_destination(self): + with tempfile.TemporaryDirectory() as tmp: + deps = Path(tmp, "deps") + deps.mkdir() + (deps / "six.py").write_text("dependency") + + artifacts = Path(tmp, "artifacts") + artifacts.mkdir() + os.symlink(str(deps / "six.py"), str(artifacts / "six.py")) + + source = Path(tmp, "source") + source.mkdir() + (source / "six.py").write_text("user code") + + utils.copytree(str(source), str(artifacts)) + + self.assertEqual( + (deps / "six.py").read_text(), "dependency", "source leaked into the dependencies directory" + ) + self.assertFalse((artifacts / "six.py").is_symlink()) + self.assertEqual((artifacts / "six.py").read_text(), "user code") + + def test_does_not_create_a_file_through_a_dangling_symlink(self): + with tempfile.TemporaryDirectory() as tmp: + outside = Path(tmp, "outside", "six.py") + outside.parent.mkdir() + + artifacts = Path(tmp, "artifacts") + artifacts.mkdir() + os.symlink(str(outside), str(artifacts / "six.py")) + + source = Path(tmp, "source") + source.mkdir() + (source / "six.py").write_text("user code") + + utils.copytree(str(source), str(artifacts)) + + self.assertFalse(outside.exists(), "copy followed a dangling link outside the destination tree") + self.assertEqual((artifacts / "six.py").read_text(), "user code") + class TestDecode(TestCase): def test_does_not_crash_non_utf8_encoding(self): diff --git a/tests/unit/workflows/python_pip/test_workflow.py b/tests/unit/workflows/python_pip/test_workflow.py index b8ec16da8..85b2d1eaf 100644 --- a/tests/unit/workflows/python_pip/test_workflow.py +++ b/tests/unit/workflows/python_pip/test_workflow.py @@ -12,14 +12,24 @@ @parameterized_class( - ("experimental_flags",), + ("experimental_flags", "is_building_layer"), [ - ([]), - ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE]), + ([], False), + ([], True), + ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], False), + ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], True), ], ) class TestPythonPipWorkflow(TestCase): experimental_flags = [] + is_building_layer = False + + @property + def expects_linked_dependencies(self): + """Dependencies are symlinked only when the build performance flag is on AND we are + building a layer -- a function's artifacts are bind-mounted into the local invoke + container, where symlinks pointing outside the mount dangle.""" + return bool(self.experimental_flags) and self.is_building_layer def setUp(self): self.osutils = OSUtils() @@ -121,10 +131,10 @@ def test_workflow_sets_up_actions_without_download_dependencies_with_dependencie dependencies_dir="dep", download_dependencies=False, experimental_flags=self.experimental_flags, + is_building_layer=self.is_building_layer, ) self.assertEqual(len(self.workflow.actions), 2) - # symlinking python dependencies is disabled for now since it is breaking sam local commands - if False and self.experimental_flags: + if self.expects_linked_dependencies: self.assertIsInstance(self.workflow.actions[0], LinkSourceAction) else: self.assertIsInstance(self.workflow.actions[0], CopySourceAction) @@ -143,12 +153,12 @@ def test_workflow_sets_up_actions_with_download_dependencies_and_dependencies_di dependencies_dir="dep", download_dependencies=True, experimental_flags=self.experimental_flags, + is_building_layer=self.is_building_layer, ) self.assertEqual(len(self.workflow.actions), 4) self.assertIsInstance(self.workflow.actions[0], CleanUpAction) self.assertIsInstance(self.workflow.actions[1], PythonPipBuildAction) - # symlinking python dependencies is disabled for now since it is breaking sam local commands - if False and self.experimental_flags: + if self.expects_linked_dependencies: self.assertIsInstance(self.workflow.actions[2], LinkSourceAction) else: self.assertIsInstance(self.workflow.actions[2], CopySourceAction) @@ -193,6 +203,30 @@ def test_workflow_sets_up_actions_without_combine_dependencies(self): self.assertIsInstance(self.workflow.actions[1], PythonPipBuildAction) self.assertIsInstance(self.workflow.actions[2], CopySourceAction) + def test_layer_links_dependencies_while_function_copies_them(self): + """The layer/function distinction is the whole reason linking is safe at all, so assert the + contrast on inputs that are otherwise identical -- otherwise a future change that drops + is_building_layer still passes every other test in this class.""" + + def actions_for(is_building_layer): + osutils_mock = Mock(spec=self.osutils) + osutils_mock.file_exists.return_value = True + return PythonPipWorkflow( + "source", + "artifacts", + "scratch_dir", + "manifest", + runtime="python3.9", + osutils=osutils_mock, + dependencies_dir="dep", + download_dependencies=True, + experimental_flags=[EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], + is_building_layer=is_building_layer, + ).actions + + self.assertIsInstance(actions_for(is_building_layer=True)[2], LinkSourceAction) + self.assertIsInstance(actions_for(is_building_layer=False)[2], CopySourceAction) + @patch("aws_lambda_builders.workflows.python_pip.workflow.PythonPipBuildAction") def test_must_build_with_architecture(self, PythonPipBuildActionMock): self.workflow = PythonPipWorkflow(