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

feat: support dotnet lambda container builds #4665

Merged
merged 37 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
d93a5ba
allow use container for dotnet with write permission to source code d…
mrkdeng Dec 14, 2022
31b7083
Merge branch 'develop' into develop
mrkdeng Dec 14, 2022
986a804
fix typo
mrkdeng Dec 14, 2022
654b0dc
Merge branch 'aws:develop' into develop
mrkdeng Jan 24, 2023
59e5ff6
Merge branch 'aws:develop' into mount_with_write
mrkdeng Jan 27, 2023
c7c4149
major code changes to allow mount with write
mrkdeng Feb 1, 2023
84c623b
remove dotnet env vars and add logic to make tmp dir on host
mrkdeng Feb 6, 2023
3a48b99
integration tests
mrkdeng Feb 7, 2023
871e9d2
consist help text and click confirm
mrkdeng Feb 7, 2023
628da19
cleaner approach to make tmp dir on host, refactor regarding dev guide
mrkdeng Feb 9, 2023
1c30b3f
update and add new unit tests
mrkdeng Feb 9, 2023
f7a013d
fix failed unit tests
mrkdeng Feb 13, 2023
ef34e17
Merge branch 'aws:develop' into develop
mrkdeng Feb 13, 2023
f03cd46
Merge branch 'develop' into mount_with_write
mrkdeng Feb 13, 2023
89b2414
update test comments
mrkdeng Feb 13, 2023
99d785e
update CONFIG and remove unnecessary Json dump
mrkdeng Feb 14, 2023
b9432ea
update integration tests
mrkdeng Feb 17, 2023
f3716f8
Merge branch 'develop' into mount_with_write
mrkdeng Feb 17, 2023
05ba6cc
Merge branch 'mount_with_write' of https://github.com/mrkdeng/aws-sam…
mrkdeng Feb 17, 2023
6819eed
Merge branch 'develop' into mount_with_write
mrkdeng Feb 20, 2023
ccde63c
pass ruff tests
mrkdeng Feb 20, 2023
60ceabf
Merge branch 'develop' into mount_with_write
mrkdeng Feb 22, 2023
888aadf
change mount with click to enum
mrkdeng Feb 23, 2023
db30045
add debug logging and remove unused exception
mrkdeng Feb 23, 2023
461202d
update unit and integration tests
mrkdeng Feb 23, 2023
ca2e77b
update click
mrkdeng Feb 23, 2023
6c7217e
add MountMode enum and set default to READ
mrkdeng Feb 24, 2023
8a22584
add unit tests for prompting
mrkdeng Feb 24, 2023
4196279
default mount_with to READ in all places
mrkdeng Feb 24, 2023
ccfb879
early exit
mrkdeng Feb 28, 2023
a2f15e6
better use of enum and constant, update unit tests
mrkdeng Mar 2, 2023
d00e842
Merge branch 'develop' into mount_with_write
mrkdeng Mar 3, 2023
6a2afca
Merge branch 'mount_with_write' of https://github.com/mrkdeng/aws-sam…
mrkdeng Mar 3, 2023
280b160
make tmp dir on sam build dir on host
mrkdeng Mar 6, 2023
2ba803b
Merge branch 'develop' into mount_with_write
mndeveci Mar 7, 2023
7f5d6a4
Merge branch 'develop' into mount_with_write
mndeveci Mar 10, 2023
bc63a8a
Merge branch 'develop' into mount_with_write
mndeveci Mar 15, 2023
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
24 changes: 19 additions & 5 deletions samcli/commands/build/build_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import click

from samcli.commands.build.utils import prompt_user_to_enable_mount_with_write_if_needed
from samcli.lib.build.bundler import EsbuildBundlerManager
from samcli.lib.providers.sam_api_provider import SamApiProvider
from samcli.lib.telemetry.event import EventTracker
Expand Down Expand Up @@ -77,7 +78,7 @@ def __init__(
locate_layer_nested: bool = False,
hook_name: Optional[str] = None,
build_in_source: Optional[bool] = None,
mount_with_write: bool = False,
mount_with: Optional[str] = None,
) -> None:
"""
Initialize the class
Expand Down Expand Up @@ -133,8 +134,8 @@ def __init__(
Name of the hook package
build_in_source: Optional[bool]
Set to True to build in the source directory.
mount_with_write: bool
Mount source code directory with write permissions when building inside container.
mount_with: Optional[str]
Mount mode of source code directory when building inside container.
"""

self._resource_identifier = resource_identifier
Expand Down Expand Up @@ -174,7 +175,7 @@ def __init__(
self._locate_layer_nested = locate_layer_nested
self._hook_name = hook_name
self._build_in_source = build_in_source
self._mount_with_write = mount_with_write
self._mount_with = mount_with

def __enter__(self) -> "BuildContext":
self.set_up()
Expand Down Expand Up @@ -238,6 +239,19 @@ def run(self):

self._stacks = self._handle_build_pre_processing()

# boolean value indicates if mount with write or not, defaults to READ ONLY
mount_with_write = False
if self._use_container and not self._mount_with:
# if self._mount_with is None, means user does not specify mount mode
# check the need of mounting with write permissions and prompt user to enable it if needed
mount_with_write = prompt_user_to_enable_mount_with_write_if_needed(
self.get_resources_to_build(),
self.base_dir,
)
elif self._mount_with:
# user specify mount mode using CLI
mount_with_write = True if self._mount_with.lower() == "write" else False

try:
builder = ApplicationBuilder(
self.get_resources_to_build(),
Expand All @@ -255,7 +269,7 @@ def run(self):
build_images=self._build_images,
combine_dependencies=not self._create_auto_dependency_layer,
build_in_source=self._build_in_source,
mount_with_write=self._mount_with_write,
mount_with_write=mount_with_write,
)
except FunctionNotFound as ex:
raise UserException(str(ex), wrapped_from=ex.__class__.__name__) from ex
Expand Down
19 changes: 9 additions & 10 deletions samcli/commands/build/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,11 @@
"By default the functions and layers are built in sequence",
)
@click.option(
"--mount-with-write/--mount-with-read",
"-w",
default=False,
is_flag=True,
help="Enable mounting with write permissions when building functions/layers inside container. "
"Some files in source code directory may be changed or added by the build process. "
"--mount-with",
"-mw",
type=click.Choice(["READ", "WRITE"], case_sensitive=False),
mrkdeng marked this conversation as resolved.
Show resolved Hide resolved
help="Optional. Specify mount mode for building functions/layers inside container. "
"If mount with write permissions, some files in source code directory may be changed/added by the build process. "
mrkdeng marked this conversation as resolved.
Show resolved Hide resolved
"By default the source code directory is read only.",
cls=ContainerOptions,
)
Expand Down Expand Up @@ -185,7 +184,7 @@ def cli(
config_env: str,
hook_name: Optional[str],
skip_prepare_infra: bool,
mount_with_write: bool,
mount_with: Optional[str],
) -> None:
"""
`sam build` command entry point
Expand Down Expand Up @@ -216,7 +215,7 @@ def cli(
exclude,
hook_name,
None, # TODO: replace with build_in_source once it's added as a click option
mount_with_write,
mount_with,
) # pragma: no cover


Expand All @@ -242,7 +241,7 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements
exclude: Optional[Tuple[str, ...]],
hook_name: Optional[str],
build_in_source: Optional[bool],
mount_with_write: bool,
mount_with: Optional[str],
) -> None:
"""
Implementation of the ``cli`` method
Expand Down Expand Up @@ -288,7 +287,7 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements
aws_region=click_ctx.region,
hook_name=hook_name,
build_in_source=build_in_source,
mount_with_write=mount_with_write,
mount_with=mount_with,
mndeveci marked this conversation as resolved.
Show resolved Hide resolved
) as ctx:
ctx.run()

Expand Down
58 changes: 55 additions & 3 deletions samcli/commands/build/utils.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,65 @@
"""
Utilities for sam build command
"""
import pathlib

import click

from samcli.lib.build.workflow_config import CONFIG
from samcli.lib.build.workflow_config import CONFIG, get_workflow_config
from samcli.lib.providers.provider import ResourcesToBuildCollector


def prompt_user_to_enable_mount_with_write(config: CONFIG, source_dir: str) -> bool:
def prompt_user_to_enable_mount_with_write_if_needed(
resources_to_build: ResourcesToBuildCollector,
base_dir: str,
) -> bool:
"""
First check if mounting with write permissions is needed for building inside container or not. If it is needed, then
prompt user to choose if enables mounting with write permissions or not.

Parameters
----------
resources_to_build:
Resource to build inside container

base_dir : str
Path to the base directory

Returns
-------
bool
True, if user enabled mounting with write permissions.
"""

functions = resources_to_build.functions
layers = resources_to_build.layers
runtime = None
specified_workflow = None
code_uri = None

if len(functions) > 0:
function = functions[0]
runtime = function.runtime
code_uri = function.codeuri
# get specified_workflow if metadata exists
metadata = function.metadata
specified_workflow = metadata.get("BuildMethod", None) if metadata else None
elif len(layers) > 0:
layer = layers[0]
specified_workflow = layer.build_method
code_uri = layer.codeuri
mrkdeng marked this conversation as resolved.
Show resolved Hide resolved

if code_uri:
code_dir = str(pathlib.Path(base_dir, code_uri).resolve())
config = get_workflow_config(runtime, code_dir, base_dir, specified_workflow)
# prompt if mount with write is needed
if config.must_mount_with_write_in_container:
return _prompt(config, code_dir)

return False


def _prompt(config: CONFIG, source_dir: str) -> bool:
"""
Prompt user to choose if enables mounting with write permissions or not when building lambda functions/layers

Expand All @@ -28,7 +80,7 @@ def prompt_user_to_enable_mount_with_write(config: CONFIG, source_dir: str) -> b
f"\nBuilding functions with {config.language} inside containers needs "
f"mounting with write permissions to the source code directory {source_dir}. "
f"Some files in this directory may be changed or added by the build process. "
f"Pass --mount-with-write to `sam build` CLI to avoid this confirmation. "
f"Pass `--mount-with WRITE` to `sam build` CLI to avoid this confirmation. "
f"\nWould you like to enable mounting with write permissions? "
):
return True
Expand Down
3 changes: 0 additions & 3 deletions samcli/lib/build/app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
)
from aws_lambda_builders.builder import LambdaBuilder
from aws_lambda_builders.exceptions import LambdaBuilderError
from samcli.commands.build.utils import prompt_user_to_enable_mount_with_write
from samcli.lib.build.build_graph import FunctionBuildDefinition, LayerBuildDefinition, BuildGraph
from samcli.lib.build.build_strategy import (
DefaultBuildStrategy,
Expand Down Expand Up @@ -897,8 +896,6 @@ def _build_function_on_container(
"Docker is unreachable. Docker needs to be running to build inside a container."
)

if not self._mount_with_write and config.must_mount_with_write_in_container:
self._mount_with_write = prompt_user_to_enable_mount_with_write(config, source_dir)
# If we are printing debug logs in SAM CLI, the builder library should also print debug logs
log_level = LOG.getEffectiveLevel()

Expand Down
4 changes: 0 additions & 4 deletions samcli/lib/build/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ def __init__(self, container_name: str, error_msg: str) -> None:
Exception.__init__(self, msg.format(container_name=container_name, error_msg=error_msg))


class ContainerBuildNotSupported(Exception):
pass


class BuildError(Exception):
def __init__(self, wrapped_from: str, msg: str) -> None:
self.wrapped_from = wrapped_from
Expand Down
32 changes: 0 additions & 32 deletions samcli/lib/build/workflow_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,38 +222,6 @@ def get_workflow_config(
) from ex


def supports_build_in_container(config: CONFIG) -> Tuple[bool, Optional[str]]:
"""
Given a workflow config, this method provides a boolean on whether the workflow can run within a container or not.

Parameters
----------
config namedtuple(Capability)
Config specifying the particular build workflow

Returns
-------
tuple(bool, str)
True, if this workflow can be built inside a container. False, along with a reason message if it cannot be.
"""

def _key(c: CONFIG) -> str:
return str(c.language) + str(c.dependency_manager) + str(c.application_framework)

# This information could have beeen bundled inside the Workflow Config object. But we this way because
# ultimately the workflow's implementation dictates whether it can run within a container or not.
# A "workflow config" is like a primary key to identify the workflow. So we use the config as a key in the
# map to identify which workflows can support building within a container.

unsupported: Dict[str, str] = {}

thiskey = _key(config)
if thiskey in unsupported:
return False, unsupported[thiskey]

return True, None


def supports_specified_workflow(specified_workflow: str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am having an hard time understand what this is actually for. Could you describe the use-case for this?

Note: This is probably just a gap in my understanding.

Copy link
Contributor Author

@mrkdeng mrkdeng Feb 14, 2023

Choose a reason for hiding this comment

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

Regarding comment to keep _get_image clean. If this method returns True, the specified_workflow (BuildMethod in .yaml) will overwrite runtime to get docker image. One concern is, dotnet7 is not the only one that has specified_workflow, but we know for sure it is safe because container builds for dotnet wasn't supported. For other runtimes using provided.al2 as runtime, it's hard to say it wouldn't break. So I think this check is necessary before passing down specified_workflow. Lmk if there's a better way to do this.

"""
Given a specified workflow, returns whether it is supported in container builds,
Expand Down
3 changes: 3 additions & 0 deletions samcli/local/docker/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ def create(self):
_root_user_id = "0"
effective_user = EffectiveUser.get_current_effective_user().to_effective_user_str()
if self._mount_with_write and effective_user and effective_user != _root_user_id:
mndeveci marked this conversation as resolved.
Show resolved Hide resolved
LOG.debug("Detect non-root user, will pass argument '--user %s' to container", effective_user)
kwargs["user"] = effective_user

if self._container_opts:
Expand Down Expand Up @@ -282,6 +283,7 @@ def delete(self):
host_tmp_dir_path = pathlib.Path(self._host_tmp_dir)
if host_tmp_dir_path.exists():
shutil.rmtree(self._host_tmp_dir)
mndeveci marked this conversation as resolved.
Show resolved Hide resolved
LOG.debug("Successfully removed temporary directory %s on the host.", self._host_tmp_dir)

self.id = None

Expand All @@ -306,6 +308,7 @@ def start(self, input_data=None):
# Make tmp dir on the host
if self._mount_with_write and self._host_tmp_dir and not os.path.exists(self._host_tmp_dir):
os.mkdir(self._host_tmp_dir)
mndeveci marked this conversation as resolved.
Show resolved Hide resolved
LOG.debug("Successfully created temporary directory %s on the host.", self._host_tmp_dir)

# Get the underlying container instance from Docker API
real_container = self.docker_client.containers.get(self.id)
Expand Down
6 changes: 3 additions & 3 deletions tests/integration/buildcmd/build_integ_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def get_command_list(
hook_name=None,
beta_features=None,
build_in_source=None,
mount_with_write=False,
mount_with=None,
):

command_list = [self.cmd, "build"]
Expand Down Expand Up @@ -127,8 +127,8 @@ def get_command_list(
if build_image:
command_list += ["--build-image", build_image]

if mount_with_write:
command_list += ["--mount-with-write"]
if mount_with:
command_list += ["--mount-with", mount_with]

if exclude:
for f in exclude:
Expand Down
30 changes: 23 additions & 7 deletions tests/integration/buildcmd/test_build_cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,7 @@ class TestBuildCommand_Dotnet_cli_package(BuildIntegBase):
]
)
@pytest.mark.flaky(reruns=3)
def test_with_dotnetcore_in_process(self, runtime, code_uri, mode, architecture="x86_64"):
def test_dotnetcore_in_process(self, runtime, code_uri, mode, architecture="x86_64"):
overrides = {
"Runtime": runtime,
"CodeUri": code_uri,
Expand Down Expand Up @@ -1052,9 +1052,7 @@ def test_with_dotnetcore_in_process(self, runtime, code_uri, mode, architecture=
)
@skipIf(SKIP_DOCKER_TESTS or SKIP_DOCKER_BUILD, SKIP_DOCKER_MESSAGE)
@pytest.mark.flaky(reruns=3)
def test_with_dotnetcore_in_container_mount_with_write_explicit(
self, runtime, code_uri, mode, architecture="x86_64"
):
def test_dotnetcore_in_container_mount_with_write_explicit(self, runtime, code_uri, mode, architecture="x86_64"):
overrides = {
"Runtime": runtime,
"CodeUri": code_uri,
Expand All @@ -1066,7 +1064,7 @@ def test_with_dotnetcore_in_container_mount_with_write_explicit(
self.template_path = self.template_path.replace("template.yaml", "template_build_method_dotnet_7.yaml")

# test with explicit mount_with_write flag
cmdlist = self.get_command_list(use_container=True, parameter_overrides=overrides, mount_with_write=True)
cmdlist = self.get_command_list(use_container=True, parameter_overrides=overrides, mount_with="write")
# env vars needed for testing unless set by dotnet images on public.ecr.aws
cmdlist += ["--container-env-var", "DOTNET_CLI_HOME=/tmp/dotnet"]
cmdlist += ["--container-env-var", "XDG_DATA_HOME=/tmp/xdg"]
Expand Down Expand Up @@ -1127,7 +1125,7 @@ def test_with_dotnetcore_in_container_mount_with_write_explicit(
)
@skipIf(SKIP_DOCKER_TESTS or SKIP_DOCKER_BUILD, SKIP_DOCKER_MESSAGE)
@pytest.mark.flaky(reruns=3)
def test_with_dotnetcore_in_container_mount_with_write_interactive(
def test_dotnetcore_in_container_mount_with_write_interactive(
self,
runtime,
code_uri,
Expand Down Expand Up @@ -1194,7 +1192,25 @@ def test_with_dotnetcore_in_container_mount_with_write_interactive(
@parameterized.expand([("dotnetcore3.1", "Dotnetcore3.1"), ("dotnet6", "Dotnet6")])
@skipIf(SKIP_DOCKER_TESTS or SKIP_DOCKER_BUILD, SKIP_DOCKER_MESSAGE)
@pytest.mark.flaky(reruns=3)
def test_must_fail_on_container_mount_without_write(self, runtime, code_uri):
def test_must_fail_on_container_mount_without_write_explicit(self, runtime, code_uri):
use_container = True
overrides = {
"Runtime": runtime,
"CodeUri": code_uri,
"Handler": "HelloWorld::HelloWorld.Function::FunctionHandler",
}
cmdlist = self.get_command_list(use_container=use_container, parameter_overrides=overrides, mount_with="read")

LOG.info("Running Command: {}".format(cmdlist))
process_execute = run_command(cmdlist, cwd=self.working_dir)

# Must error out, because mounting with write is not allowed
self.assertEqual(process_execute.process.returncode, 1)

@parameterized.expand([("dotnetcore3.1", "Dotnetcore3.1"), ("dotnet6", "Dotnet6")])
@skipIf(SKIP_DOCKER_TESTS or SKIP_DOCKER_BUILD, SKIP_DOCKER_MESSAGE)
@pytest.mark.flaky(reruns=3)
def test_must_fail_on_container_mount_without_write_interactive(self, runtime, code_uri):
use_container = True
overrides = {
"Runtime": runtime,
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/commands/buildcmd/test_build_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ def test_run_build_context(
build_images={},
create_auto_dependency_layer=auto_dependency_layer,
build_in_source=False,
mount_with_write=False,
mount_with="Read",
) as build_context:
build_context.run()

Expand Down
Loading