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: Env should not be escaped #1064

Merged
merged 1 commit into from
Apr 9, 2024
Merged

fix: Env should not be escaped #1064

merged 1 commit into from
Apr 9, 2024

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Mar 26, 2024

According to https://github.com/opencontainers/image-spec/blob/main/config.md:

Entries are in the format of VARNAME=VARVALUE. These values act as defaults and are merged with any specified when creating a container.

The VARVALUE part does not understand shell escape or quotes.

@domenkozar
Copy link
Member

We should really have some tests for these, can you show an example?

@Atry
Copy link
Contributor Author

Atry commented Mar 29, 2024

Use --entrypoint bash to skip mk-shell-bin's envScript, which overrides the environment variables, hidding this bug.

cd examples/fly.io && nix run ../.. -- container copy shell && docker run --rm -it --entrypoint bash shell:latest -c 'ls "$DEVENV_ROOT"'
• Building shell container ...
• Using Cachix: devenv
warning: creating lock file '/examples/fly.io/devenv.lock'
trace: warning: `vendorSha256` is deprecated. Use `vendorHash` instead
/nix/store/i98cgbww6qifd626wzvn8jyz00va80nz-image-shell.json
✔ Building shell container in 2.0s.
warning: creating lock file '/examples/fly.io/devenv.lock'
trace: warning: `vendorSha256` is deprecated. Use `vendorHash` instead
• Running /nix/store/amca1wlni4szd4p8drmf6accq31ml58n-copy-container /nix/store/i98cgbww6qifd626wzvn8jyz00va80nz-image-shell.json false 

Copying container /nix/store/i98cgbww6qifd626wzvn8jyz00va80nz-image-shell.json to docker-daemon:shell:latest

Getting image source signatures
Copying blob 79833476e9bf done   | 
Copying blob a460459aeca2 done   | 
Copying config 3f15e603df done   | 
Writing manifest to image destination
✔ Copying shell container in 2.8s.
ls: cannot access "'/env'": No such file or directory

You can see the single quotes around DEVENV_ROOT and other environment variables. The single quotes are generated by lib.escapeShellArg.

@Atry
Copy link
Contributor Author

Atry commented Apr 2, 2024

This bug also affects docker exec because docker exec uses neither Entrypoint nor envScript.

docker exec "$(docker run --detach --rm -it shell:latest sleep infinity)" env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=661680970c87
DEVENV_DOTFILE='/env/.devenv'
DEVENV_PROFILE='/nix/store/ra30qvay59hqy3j9fxwrb0ks20l6ifbw-devenv-profile'
DEVENV_ROOT='/env'
DEVENV_RUNTIME='/tmp/devenv-078a6e4'
DEVENV_STATE='/env/.devenv/state'
HOME=/env
USER=user

You can see the environment variables are incorrect.

@Atry
Copy link
Contributor Author

Atry commented Apr 8, 2024

@domenkozar Can we fix this?

@domenkozar
Copy link
Member

I'm afraid this will break other things, we for sure added escaping to fix something in the past.

@Atry
Copy link
Contributor Author

Atry commented Apr 8, 2024 via email

@Atry
Copy link
Contributor Author

Atry commented Apr 8, 2024

The current Env settings are simply broken. We did not notice it because they only take effect on either docker exec or a custom entrypoint, because docker run with the default entrypoint takes envionment variables from mk-shell-bin's envScript, not the image's Env.

@domenkozar domenkozar merged commit 808caa6 into cachix:main Apr 9, 2024
227 of 247 checks passed
@Atry Atry deleted the patch-5 branch April 9, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants