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

Conversation

mrkdeng
Copy link
Contributor

@mrkdeng mrkdeng commented Feb 7, 2023

Which issue(s) does this change fix?

#4453

Why is this change necessary?

  • This change adds support for .NET lambda container builds. It allows mounting source directory with write permissions when building in container because .NET builds write output to the source directory.
  • In most cases on posix system, Docker Daemon is running as root. This change solves this problem by running docker as a non-root user.

How does it address the issue?

  • Add a new sam build CLI argument --mount-with [READ|WRITE] that enables mounting source code directory with READ/WRITE permissions. Then run Docker as a non-root user (unless current user is root) if mount with write, which gives user the least privilege
  • .NET lambda container builds always need --mount-with WRITE to be specified. If building without explicit --mount-with, a confirmation dialog pops up and asks write permissions to source code directory
  • This change is backward compatible and not breaking since container builds for .NET was not supported. For other runtimes, they mount source code as READ only by default, unless with intently using --mount-with WRITE

What side effects does this change have?

There is permission issue when running .NET image as non-root user #7868. This change relies on docker images to set environment variables such as DOTNET_CLI_HOME, XDG_DATA_HOME

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. area/build sam build command area/local/invoke sam local invoke command area/local/start-api sam local start-api command area/local/start-invoke labels Feb 7, 2023
@mrkdeng mrkdeng marked this pull request as ready for review February 7, 2023 19:07
@mrkdeng mrkdeng requested a review from a team as a code owner February 7, 2023 19:07
@mrkdeng mrkdeng requested review from hawflau and jfuss February 7, 2023 19:07
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

Did a first pass with some feedback.

Thank you for adding integration tests but please make sure you are also adding unit tests to cover your changes.

samcli/lib/build/app_builder.py Outdated Show resolved Hide resolved
samcli/local/docker/container.py Outdated Show resolved Hide resolved
samcli/local/docker/container.py Outdated Show resolved Hide resolved
samcli/local/docker/lambda_build_container.py Outdated Show resolved Hide resolved
samcli/local/docker/lambda_build_container.py Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/lib/build/app_builder.py Outdated Show resolved Hide resolved
samcli/local/docker/container.py Outdated Show resolved Hide resolved
samcli/local/docker/utils.py Outdated Show resolved Hide resolved
@jfuss jfuss added stage/in-progress A fix is being worked on and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Feb 9, 2023
@mrkdeng
Copy link
Contributor Author

mrkdeng commented Feb 9, 2023

@Beau-Gosse-dev @jfuss @mndeveci thank you for reviewing this PR. Push two commits with one addressing comments, another one updating/adding unit tests. Will look into windows-integ failure

samcli/commands/build/build_context.py Show resolved Hide resolved
samcli/lib/build/workflow_config.py Outdated Show resolved Hide resolved
samcli/local/docker/container.py Outdated Show resolved Hide resolved
samcli/local/docker/container.py Show resolved Hide resolved
samcli/local/docker/container.py Outdated Show resolved Hide resolved
samcli/commands/build/utils.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Show resolved Hide resolved
samcli/lib/build/app_builder.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/commands/build/utils.py Outdated Show resolved Hide resolved
samcli/lib/build/app_builder.py Outdated Show resolved Hide resolved
samcli/lib/build/workflow_config.py Show resolved Hide resolved
@@ -78,6 +78,7 @@ def __init__(
locate_layer_nested: bool = False,
hook_name: Optional[str] = None,
build_in_source: Optional[bool] = None,
mount_with=MountMode.READ.value,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mount_with=MountMode.READ.value,
mount_with: MountMode = MountMode.READ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Click.Choice takes [str] only. Workaround: Convert str to enum in BuildContext.init()

self._mount_with = MountMode(mount_with)

samcli/commands/build/build_context.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/commands/build/command.py Outdated Show resolved Hide resolved
samcli/commands/build/utils.py Show resolved Hide resolved
samcli/local/docker/effective_user.py Outdated Show resolved Hide resolved
samcli/local/docker/lambda_build_container.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments, looking forward to ship this feature soon! 🥳

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks @mrkdeng for handling all the feedback along the way. I know that can be a lot and time consuming but I think we ended up in a good state as a result.

@mndeveci mndeveci added this pull request to the merge queue Mar 15, 2023
Merged via the queue into aws:develop with commit bd76bfd Mar 15, 2023
@mrkdeng mrkdeng deleted the mount_with_write branch March 22, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build sam build command area/local/invoke sam local invoke command area/local/start-api sam local start-api command area/local/start-invoke pr/external stage/in-progress A fix is being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants