Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Resolve symlinks on local invoke only #7865

Merged
merged 1 commit into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: samcli but not a blocker

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
Loading