search

aws / aws-extensions-for-dotnet-cli

Extensions to the dotnet CLI to simplify the process of building and publishing .NET Core applications to AWS services
Apache License 2.0
369 stars 86 forks source link

Update "lambda package" to use non-root user when launching Docker on Linux/MacOS #257

Closed jasonterando closed 1 year ago

jasonterando commented 1 year ago

This is related to Issue #256

Description of Change: Update dotnet lambda package so that it launches Docker as the logged in user when being run in Linux or a MacOS. This will allow dotnet lambda deploy-function to successfully create the .zip package on Linux. This change also has the benfit of not leaving directories around that users have to chown or login in as root to clean up. This will allow non-root developers to successfully use dotnet lambda to package and deploy.

Recap of Issue: When dotnet lambda package today builds a project for linux-x64, it launches a Docker container. Within the container, commands are executed which create /obj and /bin directories using the Docker container's effective user, which is "root". When deploying the package, dotnet lambda deploy-function attempts to build a .zip within these created directories, and fails, unless the active user is logged in as "root", which is not a good thing to do when you are developing.

Implementation Details:

  1. Add class PosixUserHelper to assist with running id to get the user's User ID and Group ID (unit test PosixUserHelperTest.cs)
  2. Update DockerCLIWrapper to call PosixUserHelper to determine if running under Unix/MacOS and, if so, add "-u" parameter to specify user and group IDs. Also, change the default locations of .NET and .NuGet to /tmp so that non-root users can install
  3. Update DockerCLIWrapper to build a list of arguments using List instead of individual variables that are concatenated together. This will make it easier in the future to add new arguments

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

jasonterando commented 1 year ago

Hi, thanks for the feedback, and thanks for confirming it's good on Windows. If I'm reading correctly, seems like I need to address the following:

  1. Add comment to assignment of .NET CLI and NuGet home environment variables: No problem, will do
  2. Change uint to int, maybe make nullable: Happy to set to int, but did not see Nullable set up in Amazon.Lambda.Tools.csproj, so I think I would have to keep track of whether they are set using a boolean. Come to think of it, this is probably fine, I can set the defaults for uid and gid to zero (root) and display a warning for Linux or MacOS if I can't use id to get the info
  3. If there's a preference to do two separate calls to get user and group ID versus a single parsed via regex, that's cool. Whatever gets the PR approved :)
  4. GetValueAsDictionary should have been a separate PR, I must have included this by mistake, will address.

Assuming I do #1, #2 and #4 -- and we decide on #3, is there anything else you need from me to proceed? Thanks!

Beau-Gosse-dev commented 1 year ago

Thanks for the quick follow up @jasonterando! The only thing I would add is that there are still some changes for using dictionary in the CLI args. I didn't look at those changes too closely, but seems like all of those could go in another PR. I'll need one of the repo contributors to review this too, since I don't have write access to this repository.

jasonterando commented 1 year ago

Okee doke - I think I finally have the dictionary/parameters thing completely cleared out (I learned my lesson, I will remember next time to not do a PR from local master branch, sorry about the hassle). Let me know if there is anything else before you feed this up the food chain.

Beau-Gosse-dev commented 1 year ago

@normj Is there some from this repo who can review this? I also reproduced the problem on Ubuntu and verified that this fixes it.

normj commented 1 year ago

@normj Is there some from this repo who can review this? I also reproduced the problem on Ubuntu and verified that this fixes it.

Back from winter holidays and added comments.

jasonterando commented 1 year ago

Hi, I've posted an update with changes as follows:

  1. Replaced single call to "id" w/ regex parsing to separate calls to "id" for user ID and group ID
  2. Failure to call "id" will be logged but ignored
  3. If the user is root user and group user no changes will be made to the Docker command
  4. Launching the "id" process has been re-implemented using a class to facilitate unit testing (may be reused for other launched processes); added unit tests to simulate "id" success and failure regardless of OS running unit tests
  5. References to System.Runtime.InteropServices "using" statements and OS checks are wrapped with "#if NETCOREAPP3_1_OR_GREATER"

Unit and integration test all run ok. I do not have Visual Studio to confirm things are running OK there.

Beau-Gosse-dev commented 1 year ago

FWIW, I tested the latest commit 625bd4f on Ubuntu as root and non-root and both worked. When running as root, the -u argument isn't passed, as expected.

jasonterando commented 1 year ago

Hi, is there anything else required for this review?

normj commented 1 year ago

@jasonterando Nothing more you need to do right now. The change looks good to me and I have run the code through its paces. I need to get somebody else on the team to double check the change and assuming nothing else found we can get the change released.

normj commented 1 year ago

This was released this morning in version 5.6.3 of Amazon.Lambda.Tools