Skip to content

Commit

Permalink
fix: Resolve symlinks on local invoke only (#7865)
Browse files Browse the repository at this point in the history
SAM CLI shouldn't be resolving/mounting symlinks by default when
building in a container.
  • Loading branch information
valerena authored Feb 6, 2025
1 parent 0287934 commit a643c2c
Show file tree
Hide file tree
Showing 8 changed files with 89 additions and 42 deletions.
3 changes: 2 additions & 1 deletion samcli/lib/build/app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
AWS_SERVERLESS_LAYERVERSION,
)
from samcli.lib.utils.stream_writer import StreamWriter
from samcli.local.docker.container import ContainerContext
from samcli.local.docker.lambda_build_container import LambdaBuildContainer
from samcli.local.docker.manager import ContainerManager, DockerImagePullFailedException
from samcli.local.docker.utils import get_docker_platform, is_docker_reachable
Expand Down Expand Up @@ -968,7 +969,7 @@ def _build_function_on_container(

try:
try:
self._container_manager.run(container)
self._container_manager.run(container, context=ContainerContext.BUILD)
except docker.errors.APIError as ex:
if "executable file not found in $PATH" in str(ex):
raise UnsupportedBuilderLibraryVersionError(
Expand Down
29 changes: 27 additions & 2 deletions samcli/local/docker/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import tempfile
import threading
import time
from enum import Enum
from typing import Dict, Iterator, Optional, Tuple, Union

import docker
Expand Down Expand Up @@ -59,6 +60,11 @@ class ContainerConnectionTimeoutException(Exception):
"""


class ContainerContext(Enum):
BUILD = "build"
INVOKE = "invoke"


class Container:
"""
Represents an instance of a Docker container with a specific configuration. The container is not actually created
Expand Down Expand Up @@ -157,11 +163,14 @@ def __init__(
except NoFreePortsError as ex:
raise ContainerNotStartableException(str(ex)) from ex

def create(self):
def create(self, context):
"""
Calls Docker API to creates the Docker container instance. Creating the container does *not* run the container.
Use ``start`` method to run the container
context: samcli.local.docker.container.ContainerContext
Context for the container management to run (build, invoke)
:return string: ID of the created container
:raise RuntimeError: If this method is called after a container already has been created
"""
Expand All @@ -174,6 +183,7 @@ def create(self):
if self._host_dir:
mount_mode = "rw,delegated" if self._mount_with_write else "ro,delegated"
LOG.info("Mounting %s as %s:%s, inside runtime container", self._host_dir, self._working_dir, mount_mode)
mapped_symlinks = self._create_mapped_symlink_files() if self._resolve_symlinks(context) else {}

_volumes = {
self._host_dir: {
Expand All @@ -182,7 +192,7 @@ def create(self):
"bind": self._working_dir,
"mode": mount_mode,
},
**self._create_mapped_symlink_files(),
**mapped_symlinks,
}

kwargs = {
Expand Down Expand Up @@ -648,3 +658,18 @@ def is_running(self):
return real_container.status == "running"
except docker.errors.NotFound:
return False

def _resolve_symlinks(self, context) -> bool:
"""_summary_
Parameters
----------
context : sacli.local.docker.container.ContainerContext
Context for the container management to run. (build, invoke)
Returns
-------
bool
True, if given these parameters it should resolve symlinks or not
"""
return bool(context != ContainerContext.BUILD)
15 changes: 9 additions & 6 deletions samcli/local/docker/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from samcli.lib.constants import DOCKER_MIN_API_VERSION
from samcli.lib.utils.stream_writer import StreamWriter
from samcli.local.docker import utils
from samcli.local.docker.container import Container
from samcli.local.docker.container import Container, ContainerContext
from samcli.local.docker.lambda_image import LambdaImage

LOG = logging.getLogger(__name__)
Expand Down Expand Up @@ -55,14 +55,16 @@ def is_docker_reachable(self):
"""
return utils.is_docker_reachable(self.docker_client)

def create(self, container):
def create(self, container, context):
"""
Create a container based on the given configuration.
Parameters
----------
container samcli.local.docker.container.Container:
Container to be created
context: samcli.local.docker.container.ContainerContext
Context for the container management to run. (build, invoke)
Raises
------
Expand Down Expand Up @@ -93,9 +95,9 @@ def create(self, container):
LOG.info("Failed to download a new %s image. Invoking with the already downloaded image.", image_name)

container.network_id = self.docker_network_id
container.create()
container.create(context)

def run(self, container, input_data=None):
def run(self, container, context: ContainerContext, input_data=None):
"""
Run a Docker container based on the given configuration.
If the container is not created, it will call Create method to create.
Expand All @@ -104,6 +106,8 @@ def run(self, container, input_data=None):
----------
container: samcli.local.docker.container.Container
Container to create and run
context: samcli.local.docker.container.ContainerContext
Context for the container management to run. (build, invoke)
input_data: str, optional
Input data sent to the container through container's stdin.
Expand All @@ -113,8 +117,7 @@ def run(self, container, input_data=None):
If the Docker image was not available in the server
"""
if not container.is_created():
self.create(container)

self.create(container, context)
container.start(input_data=input_data)

def stop(self, container: Container) -> None:
Expand Down
6 changes: 3 additions & 3 deletions samcli/local/lambdafn/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from samcli.lib.telemetry.metric import capture_parameter
from samcli.lib.utils.file_observer import LambdaFunctionObserver
from samcli.lib.utils.packagetype import ZIP
from samcli.local.docker.container import Container
from samcli.local.docker.container import Container, ContainerContext
from samcli.local.docker.container_analyzer import ContainerAnalyzer
from samcli.local.docker.exceptions import ContainerFailureError, DockerContainerCreationFailedException
from samcli.local.docker.lambda_container import LambdaContainer
Expand Down Expand Up @@ -113,7 +113,7 @@ def create(
)
try:
# create the container.
self._container_manager.create(container)
self._container_manager.create(container, ContainerContext.INVOKE)
return container

except DockerContainerCreationFailedException:
Expand Down Expand Up @@ -174,7 +174,7 @@ def run(

try:
# start the container.
self._container_manager.run(container)
self._container_manager.run(container, ContainerContext.INVOKE)
return container

except KeyboardInterrupt:
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/lib/build_module/test_app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from samcli.lib.utils.packagetype import IMAGE, ZIP
from samcli.lib.utils.stream_writer import StreamWriter
from samcli.local.docker.manager import DockerImagePullFailedException
from samcli.local.docker.container import ContainerContext
from tests.unit.lib.build_module.test_build_graph import generate_function


Expand Down Expand Up @@ -2940,7 +2941,7 @@ def mock_wait_for_logs(stdout, stderr):
build_dir="/build/dir",
)

self.container_manager.run.assert_called_with(container_mock)
self.container_manager.run.assert_called_with(container_mock, context=ContainerContext.BUILD)
self.builder._parse_builder_response.assert_called_once_with(stdout_data, container_mock.image)
container_mock.copy.assert_called_with(response["result"]["artifacts_dir"] + "/.", "artifacts_dir")
self.container_manager.stop.assert_called_with(container_mock)
Expand Down
16 changes: 10 additions & 6 deletions tests/unit/local/docker/test_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from samcli.lib.utils.stream_writer import StreamWriter
from samcli.local.docker.container import (
Container,
ContainerContext,
ContainerResponseException,
ContainerConnectionTimeoutException,
PortAlreadyInUse,
Expand Down Expand Up @@ -76,6 +77,7 @@ def setUp(self):
self.additional_volumes = {"/somepath": {"blah": "blah value"}}
self.container_host = "localhost"
self.container_host_interface = "127.0.0.1"
self.container_context = ContainerContext.BUILD

self.mock_docker_client = Mock()
self.mock_docker_client.containers = Mock()
Expand Down Expand Up @@ -104,7 +106,7 @@ def test_must_create_container_with_required_values(self, mock_resolve_symlinks)
exposed_ports=self.exposed_ports,
)

container_id = container.create()
container_id = container.create(ContainerContext.INVOKE)
self.assertEqual(container_id, generated_id)
self.assertEqual(container.id, generated_id)

Expand All @@ -121,6 +123,7 @@ def test_must_create_container_with_required_values(self, mock_resolve_symlinks)
use_config_proxy=True,
)
self.mock_docker_client.networks.get.assert_not_called()
mock_resolve_symlinks.assert_called_with() # When context is INVOKE

@patch("samcli.local.docker.container.Container._create_mapped_symlink_files")
def test_must_create_container_including_all_optional_values(self, mock_resolve_symlinks):
Expand Down Expand Up @@ -155,7 +158,7 @@ def test_must_create_container_including_all_optional_values(self, mock_resolve_
container_host_interface=self.container_host_interface,
)

container_id = container.create()
container_id = container.create(ContainerContext.BUILD)
self.assertEqual(container_id, generated_id)
self.assertEqual(container.id, generated_id)

Expand All @@ -176,6 +179,7 @@ def test_must_create_container_including_all_optional_values(self, mock_resolve_
container="opts",
)
self.mock_docker_client.networks.get.assert_not_called()
mock_resolve_symlinks.assert_not_called() # When context is BUILD

@patch("samcli.local.docker.utils.os")
@patch("samcli.local.docker.container.Container._create_mapped_symlink_files")
Expand Down Expand Up @@ -216,7 +220,7 @@ def test_must_create_container_translate_volume_path(self, mock_resolve_symlinks
additional_volumes=additional_volumes,
)

container_id = container.create()
container_id = container.create(self.container_context)
self.assertEqual(container_id, generated_id)
self.assertEqual(container.id, generated_id)

Expand Down Expand Up @@ -261,7 +265,7 @@ def test_must_connect_to_network_on_create(self, mock_resolve_symlinks):

container.network_id = network_id

container_id = container.create()
container_id = container.create(self.container_context)
self.assertEqual(container_id, generated_id)

self.mock_docker_client.containers.create.assert_called_with(
Expand Down Expand Up @@ -300,7 +304,7 @@ def test_must_connect_to_host_network_on_create(self, mock_resolve_symlinks):

container.network_id = network_id

container_id = container.create()
container_id = container.create(self.container_context)
self.assertEqual(container_id, generated_id)

self.mock_docker_client.containers.create.assert_called_with(
Expand All @@ -324,7 +328,7 @@ def test_must_fail_if_already_created(self):
container.is_created.return_value = True

with self.assertRaises(RuntimeError):
container.create()
container.create(self.container_context)


class TestContainer_stop(TestCase):
Expand Down
Loading

0 comments on commit a643c2c

Please sign in to comment.