-
Notifications
You must be signed in to change notification settings - Fork 752
implement preserving env from host into vm in shell command #3830
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
implement preserving env from host into vm in shell command #3830
Conversation
810b261 to
d9e9da9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM and seems to work.
I do have some feedback though...
(I also ran out of time and did not review the tests)
| "XAUTHORITY", | ||
| "XDG_*", | ||
| "_*", // Variables starting with underscore are typically internal | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would sort the builtin list alphabetically, so it is easier to see which variables are filtered.
And I think we should add more variables, but it is of course subject to discussion. Some suggestions:
LD_*
DYLD_*
SHLVL
UID
EUID
GROUP
GID
HOSTNAME
Edit: I had LINES and COLUMNS in the list originally, but changed my mind and removed them.
Not sure about excluding shell-specific variables:
BASH*
ZSH*
FPATH
ZDOTDIR
98e8fb3 to
523b2bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements environment variable propagation from the host machine to the guest VM in shell commands, addressing the ability to preserve selected environment variables when executing commands within Lima VMs.
- Adds environment filtering utilities with configurable blocklist/allowlist support
- Introduces
--preserve-envflag to shell commands for controlled environment propagation - Updates nerdctl.lima wrapper script to use the new preserve-env functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/envutil/envutil.go | New utility package for filtering environment variables with builtin blocklist and pattern matching |
| pkg/envutil/envutil_test.go | Comprehensive test coverage for environment filtering functionality |
| pkg/limainfo/limainfo.go | Adds shell environment blocklist to Lima info structure |
| cmd/limactl/shell.go | Implements --preserve-env flag and environment variable injection in shell commands |
| cmd/nerdctl.lima | Updates wrapper script to use --preserve-env flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions.
523b2bf to
53e21f5
Compare
53e21f5 to
dd731fa
Compare
jandubois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More feedback.
Meetings coming up, so again didn't review tests yet.
dd731fa to
35c0bb5
Compare
jandubois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
I only have a few nit-picks, that I don't care if you want to address them or not, but I do think the tests should cover that the allow list takes precedence over the block list.
|
@lima-vm/maintainers I think this PR is almost ready to be merged. Please add some feedback about the default block list! Do you agree with all the entries, or should some not be blocked (like the shell-specific ones)? Or are there setting that are missing? We can still update the list after the PR is merged, but I think once 2.0.0 is released, any further changes would be breaking backwards compatibility. |
35c0bb5 to
71e227b
Compare
jandubois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
One thing I stumbled over when reviewing the latest test update was that I expected allowList to come before blockList in the arguments:
filterEnvironmentWithLists(env, blockList, allowList []string)
Both because the allow list takes precedence, and because it comes alphabetically first.
But maybe that is just a personal preference. Feel free to either change or ignore, I'm also fine with merging as-is.
I will leave the PR open for another day or so to give other maintainers a chance to comment on the DefaultBlockList.
Signed-off-by: olalekan odukoya <odukoyaonline@gmail.com>
71e227b to
35d82ea
Compare
jandubois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
| // GetDefaultBlockList returns a copy of the default block list. | ||
| func GetDefaultBlockList() []string { | ||
| return slices.Clone(defaultBlockList) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this function to the top, right after the var defaultBlockList?
Usually, unexported functions go below exported ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it makes sense to have this function close to the definition because it is a small exported function directly tied to the variable.
In general we don't follow a rules that unexported functions go to the bottom. Most of the code seems to follow the pattern of defining helper functions before the exported function that uses them.
| "ZDOTDIR", | ||
| "ZSH*", | ||
| "_*", // Variables starting with underscore are typically internal | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this list taken from somewhere else?
Where is the upstream of this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no upstream afaik, we just made it up. Which is why I asked for feedback on it. Did check it against my own setup on macOS, and the blocks make sense to me. I'm unsure if we are blocking too much with the BASH* and ZSH* blocks.
| #!/bin/sh | ||
| set -eu | ||
| exec lima nerdctl "$@" | ||
| exec limactl shell --preserve-env default nerdctl "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a comment to explain the reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about docker.lima, etc. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was an oversight. I only mentioned nerdctl.lima because it always runs in the VM, but didn't think about docker.lima because I always have a local client.
So yes, docker.lima needs it too. I don't know about the others, but maybe we should add it to all of them.
In some ways you could argue that --preserve-env should be the default unless the instance is --plain, but I don't think we want to make this change because it is not backwards compatible (and having it work differently based on instance type is confusing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--preserve-env should be the default
Scary to leak the credentials by default
| shellCmd.Flags().String("shell", "", "Shell interpreter, e.g. /bin/bash") | ||
| shellCmd.Flags().String("workdir", "", "Working directory") | ||
| shellCmd.Flags().Bool("reconnect", false, "Reconnect to the SSH session") | ||
| shellCmd.Flags().Bool("preserve-env", false, "Propagate environment variables to the shell") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an integration test and the document to clarify the block list
| } | ||
|
|
||
| func getBlockList() []string { | ||
| blockEnv := os.Getenv("LIMA_SHELLENV_BLOCK") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update https://lima-vm.io/docs/config/environment-variables/
|
Late to the party, but please check the comments |
| "LD_*", | ||
| "LOGNAME", | ||
| "OLDPWD", | ||
| "PATH", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And *PATH ? (e.g., GOPATH)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work with the current wildcard implementation (only the last character can be an *), but would require expanded syntax, like suggested in #3852.
I'm not sure we should block *PATH by default, I think the rule should be to block all variables that would be incorrect inside the VM (which is why I was hesitant to include the BASH* and ZSH* ones). So we don't want to override settings that the VM shell would normally provide.
Is there a reason that the GOPATH should not be exported into the VM? It would allow you to take advantage of the package cache on the host.
⚠️ **CAUTION: this is a major update, indicating a breaking change!**⚠️ This MR contains the following updates: | Package | Update | Change | |---|---|---| | [lima-vm/lima](https://github.com/lima-vm/lima) | major | `v1.2.2` -> `v2.0.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>lima-vm/lima (lima-vm/lima)</summary> ### [`v2.0.1`](https://github.com/lima-vm/lima/releases/tag/v2.0.1) [Compare Source](lima-vm/lima@v2.0.0...v2.0.1) #### Changes - Binary release artifacts: - Fix a regression in v2.0.0 `level=fatal msg="template \"_images/<IMAGE>.yaml\" not found"` ([#​4313](lima-vm/lima#4313), thanks to [@​vvoland](https://github.com/vvoland)) - Misc: - pkg/networks/usernet: use `SIGINT` instead of `SIGKILL` ([#​4310](lima-vm/lima#4310), thanks to [@​norio-nomura](https://github.com/norio-nomura)) Full changes: <https://github.com/lima-vm/lima/milestone/64?closed=1> #### Usage ```console $ limactl create $ limactl start ... INFO[0029] READY. Run `lima` to open the shell. $ lima uname Linux ``` *** The binaries were built automatically on GitHub Actions. The build log is available for 90 days: <https://github.com/lima-vm/lima/actions/runs/19137304035> The sha256sum of the SHA256SUMS file itself is `25ad222fa1cf91a85ef7be67664f2ba65228a5d82a39be1adbbe842096854e24` . *** Release manager: [@​AkihiroSuda](https://github.com/AkihiroSuda) ### [`v2.0.0`](https://github.com/lima-vm/lima/releases/tag/v2.0.0) [Compare Source](lima-vm/lima@v1.2.2...v2.0.0) This is the second major release of Lima, featuring the support for [pluggable VM drivers](https://lima-vm.io/docs/dev/drivers/), [GPU acceleration](https://lima-vm.io/docs/config/gpu/), and [MCP](https://lima-vm.io/docs/config/ai/outside/mcp/). This release also commemorates the promotion of the project from CNCF [Sandbox](https://www.cncf.io/sandbox-projects/) to [Incubating](https://www.cncf.io/projects/) 🎉. #### Highlights - [Experimental plug-in subsystem for VM driver infrastructure](https://lima-vm.io/docs/dev/drivers/). This will help implementing third-party plugins without modifying the code base of Lima. Thanks to [GSoC 2025](https://gist.github.com/unsuman/ff31a323ecef2289bf065882726ed7f0) contributor [@​unsuman](https://github.com/unsuman) . - [Experimental krunkit VM driver](https://lima-vm.io/docs/config/vmtype/krunkit/) for supporting GPU acceleration ([#​4137](lima-vm/lima#4137), thanks to [@​unsuman](https://github.com/unsuman)) - [Experimental integration for Model Context Protocol (MCP)](https://lima-vm.io/docs/config/ai/outside/) ([#​3744](lima-vm/lima#3744)). i.e., Lima can be now used as a sandbox for AI agents such as Gemini. - Add `limactl (start|restart) --progress` flag to show the progress of provisioning ([#​3846](lima-vm/lima#3846), [#​3915](lima-vm/lima#3915), thanks to [@​olamilekan000](https://github.com/olamilekan000) [@​norio-nomura](https://github.com/norio-nomura)) - Add `limactl shell --preserve-env` flag to propagate env vars from the host to VM ([#​3830](lima-vm/lima#3830), thanks to [@​olamilekan000](https://github.com/olamilekan000)) #### Other notable changes - `/tmp/lima` is no longer mounted by default ([#​3951](lima-vm/lima#3951)) - SSH port is no longer hard-coded to 60022 for the "default" instance ([#​3780](lima-vm/lima#3780)) - Forward UDP ports by default ([#​4054](lima-vm/lima#4054)) - Support CLI plugins ([#​3834](lima-vm/lima#3834), [#​4009](lima-vm/lima#4009), thanks to [@​olamilekan000](https://github.com/olamilekan000)) - Support custom URL scheme plugins ([#​3937](lima-vm/lima#3937), thanks to [@​jandubois](https://github.com/jandubois)). `template://default` is now recommended to be written as `template:default`. The old form is still supported. ##### Details - VM driver infrastructure: - [Experimental plug-in subsystem for VM driver infrastructure](https://lima-vm.io/docs/dev/drivers/) ([multiple MRs](https://github.com/lima-vm/lima/pulls?q=is%3Apr+milestone%3Av2.0.0+is%3Aclosed+label%3Aarea%2Fvmdrivers), thanks to [@​unsuman](https://github.com/unsuman)) - krunkit: - [Experimental krunkit VM driver](https://lima-vm.io/docs/config/vmtype/krunkit/) for supporting GPU acceleration ([#​4137](lima-vm/lima#4137), thanks to [@​unsuman](https://github.com/unsuman)) - VZ: - Support Rosetta AOT Caching with CDI ([#​3858](lima-vm/lima#3858), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - Support accelerating SSH using `AF_VSOCK` ([#​3979](lima-vm/lima#3979), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - QEMU: - Fallback to TCG when KVM is not available on Linux hosts ([#​4204](lima-vm/lima#4204)) - MCP: - [Experimental integration for Model Context Protocol (MCP)](https://lima-vm.io/docs/config/ai/outside/) ([#​3744](lima-vm/lima#3744)). Lima now provides MCP tools for reading, writing, and executing local files using a VM sandbox. Known to work with Google Gemini CLI. - `limactl` CLI: - Add `limactl (start|restart) --progress` flag to show the progress of provisioning ([#​3846](lima-vm/lima#3846), [#​3915](lima-vm/lima#3915), thanks to [@​olamilekan000](https://github.com/olamilekan000) [@​norio-nomura](https://github.com/norio-nomura)) - Add `limactl (create|start|edit) --port-forward` flag for static port forwarding ([#​3699](lima-vm/lima#3699), thanks to [@​Horiodino](https://github.com/Horiodino)). Usually not needed, but useful for instances created with `--plain`. - Add `limactl (create|start|edit) --ssh-port` flag ([#​3791](lima-vm/lima#3791)) - Add `limactl (create|start|edit) --mount-only` flag ([#​3947](lima-vm/lima#3947)). Similar to `--mount`, but overrides the existing mounts. Useful for mounting `$(pwd)`. - Support specifying `--set` multiple times in `limactl (create|start|edit)` ([#​4197](lima-vm/lima#4197), thanks to [@​AndiDog](https://github.com/AndiDog)) - Add `limactl shell --preserve-env` flag to propagate env vars from the host to VM ([#​3830](lima-vm/lima#3830), thanks to [@​olamilekan000](https://github.com/olamilekan000)). See also [`LIMA_SHELLENV_ALLOW`](https://lima-vm.io/docs/config/environment-variables/#lima_shellenv_allow) and [`LIMA_SHELLENV_BLOCK`](https://lima-vm.io/docs/config/environment-variables/#lima_shellenv_block). - Support CLI plugins ([#​3834](lima-vm/lima#3834), [#​4009](lima-vm/lima#4009), thanks to [@​olamilekan000](https://github.com/olamilekan000)) - Support custom URL scheme plugins ([#​3937](lima-vm/lima#3937), thanks to [@​jandubois](https://github.com/jandubois)). `template://default` is now recommended to be written as `template:default`. The old form is still supported. - Add `limactl copy --backend=rsync` flag as an alternative to `scp` backend ([#​3143](lima-vm/lima#3143), thanks to [@​olamilekan000](https://github.com/olamilekan000)) - Add `limactl list--yq` and `limactl info --yq` flags ([#​3998](lima-vm/lima#3998), thanks to [@​jandubois](https://github.com/jandubois)) - Add `limactl rename OLD NEW` ([#​4207](lima-vm/lima#4207)) - Deprecate `--yes` and introduce `limactl (clone|rename|edit|shell) --start` instead ([#​4108](lima-vm/lima#4108), [#​4285](lima-vm/lima#4285), thanks to [@​Horiodino](https://github.com/Horiodino) [@​nlordell](https://github.com/nlordell)) - YAML: - Migrate `cpuType` to `vmOpts.qemu` ([#​3500](lima-vm/lima#3500), thanks to [@​unsuman](https://github.com/unsuman)) - Add `yq` provision mode ([#​3892](lima-vm/lima#3892), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - Prohibit relative paths in YAML ([#​3950](lima-vm/lima#3950)). Relative paths were never intended to be supported, but they were accidentally allowed due to a regression in v1.1.0. The CLI command `limactl (create|start|edit) --mount DIR` still supports relative paths. - Default template: - Remove `/tmp/lima` mount ([#​3951](lima-vm/lima#3951)) - Stop hardcoding SSH port 60022 ([#​3780](lima-vm/lima#3780)) - Network: - Enable mDNS for vzNAT and socket\_vmnet ([#​4272](lima-vm/lima#4272), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - Port forwarding: - Support port forwarding in plain mode ([#​3699](lima-vm/lima#3699), thanks to [@​Horiodino](https://github.com/Horiodino)) - Support host sockets in gRPC port forwarder ([#​4008](lima-vm/lima#4008), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - Forward UDP ports by default ([#​4054](lima-vm/lima#4054)) - Eliminated 3-second delay for detecting ports ([#​4066](lima-vm/lima#4066)) - Removed iptables watcher for `sudo nerdctl run -p ...` ([#​4107](lima-vm/lima#4107)). `sudo nerdctl run -p ...` now requires nerdctl v2.1.6 or later. - Improved performance of gRPC forwarder ([#​4247](lima-vm/lima#4247), thanks to [@​balajiv113](https://github.com/balajiv113)) - Support UDP in Kubernetes ([#​4233](lima-vm/lima#4233)) - Change default of `guestIPMustBeZero` to `true` when `guestIP` is `0.0.0.0` ([#​4221](lima-vm/lima#4221), thanks to [@​jandubois](https://github.com/jandubois)) - Build system: - Remove `Kconfig` and `config.mk`, in favor of Makefile variables ([#​3732](lima-vm/lima#3732)) - Support Fedora, RHEL, and relevant host distributions ([#​4228](lima-vm/lima#4228), thanks to [@​valdela1](https://github.com/valdela1)) - Templates: - `alpine`, `alpine-iso`: update to Alpine 3.22 ([#​4184](lima-vm/lima#4184), [#​4190](lima-vm/lima#4190), thanks to [@​jandubois](https://github.com/jandubois)) - `debian`: update to Debian 13 ([#​4029](lima-vm/lima#4029), thanks to [@​unsuman](https://github.com/unsuman)) - `docker`, `docker-rootful`: Enable containerd image store ([#​3941](lima-vm/lima#3941), thanks to [@​norio-nomura](https://github.com/norio-nomura)) - `fedora`: update to Fedora 43 ([#​4255](lima-vm/lima#4255)) - `opensuse`: update to openSUSE Leap 16 ([#​4203](lima-vm/lima#4203)) - `oraclelinux`: update to Oracle Linux 10 ([#​4236](lima-vm/lima#4236), thanks to [@​valdela1](https://github.com/valdela1)) - `ubuntu`, `default`: update Ubuntu to 25.10 ([#​4202](lima-vm/lima#4202)) - `k0s`: New template ([#​3728](lima-vm/lima#3728), thanks to [@​plandem](https://github.com/plandem)) - `experimental/ubuntu-next`: update to Ubuntu 26.04 pre-release ([#​4311](lima-vm/lima#4311)) - Project: - Invite Ansuman Sahoo ([@​unsuman](https://github.com/unsuman)) as a Reviewer ([#​4003](lima-vm/lima#4003), thanks to [@​jandubois](https://github.com/jandubois)) - Promote from CNCF Sandbox to Incubating ([#​4201](lima-vm/lima#4201)) Full changes: <https://github.com/lima-vm/lima/milestone/59?closed=1> Thanks to [@​AndiDog](https://github.com/AndiDog) [@​Horiodino](https://github.com/Horiodino) [@​afbjorklund](https://github.com/afbjorklund) [@​alexandear](https://github.com/alexandear) [@​ashwat287](https://github.com/ashwat287) [@​balajiv113](https://github.com/balajiv113) [@​bonifaido](https://github.com/bonifaido) [@​dharsanb](https://github.com/dharsanb) [@​gnawhleinad](https://github.com/gnawhleinad) [@​iamleot](https://github.com/iamleot) [@​jandubois](https://github.com/jandubois) [@​kachick](https://github.com/kachick) [@​muchzill4](https://github.com/muchzill4) [@​ningmingxiao](https://github.com/ningmingxiao) [@​nlordell](https://github.com/nlordell) [@​norio-nomura](https://github.com/norio-nomura) [@​olamilekan000](https://github.com/olamilekan000) [@​plandem](https://github.com/plandem) [@​stek29](https://github.com/stek29) [@​unsuman](https://github.com/unsuman) [@​valdela1](https://github.com/valdela1) [@​vax-r](https://github.com/vax-r) [@​vishalanarase](https://github.com/vishalanarase) [@​zyfy29](https://github.com/zyfy29) #### EOL of v1.2 Lima v1.2 will continue to receive security updates and critical bug fixes until **2026-02-06** (3 months from now). See also <https://lima-vm.io/docs/releases/>. #### Usage ```console $ limactl create $ limactl start ... INFO[0029] READY. Run `lima` to open the shell. $ lima uname Linux ``` *** The binaries were built automatically on GitHub Actions. The build log is available for 90 days: <https://github.com/lima-vm/lima/actions/runs/19130682878> The sha256sum of the SHA256SUMS file itself is `112f1ef1d9850e29b4be425ca71e8b6ac686f593ff741164885b51fbd6919ca6` . *** Release manager: [@​AkihiroSuda](https://github.com/AkihiroSuda) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjQxLjE3My4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
implements issue