dockerTools: fixes extraCommands for mkRootLayer. #40947

samueldr · 2018-05-23T00:01:13Z

The extraCommands was, previously, simply put in the body of the script
using nix expansion ${extraCommands} (which looks exactly like bash
expansion!).

This causes issues like in #34779 where scripts will eventually create
invalid bash.

The solution used was to use $extraCommands the way mkPureLayer
does, by using the variable from the environment instead of expanding
it inside the script.

Fixes #34779

Motivation for this change

See #34779, this fixes it.

Things done

✔️ Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- ✔️ NixOS
- 🆖 macOS
- ❌ other Linux distributions
✔️ Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
❌ Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
🆖 Tested execution of all binary files (usually in ./result/bin/)
✔️ Fits CONTRIBUTING.md.

This was verified using this:

nix-build -I nixpkgs=/path/to/checkout/of/nixpkgs repro.nix

Using this repro.nix:

with import <nixpkgs> {};
  dockerTools.buildImageWithNixDb {
    name = "nix";
    contents = [
      coreutils
      nix
    ];
    runAsRoot = ''
      #!${pkgs.stdenv.shell}
      echo "everything is working fine"
    '';
    config = {
      Env = [ "NIX_PAGER=cat" ];
    };
  }

This is now verified in a nixos test

NIX_PATH=nixpkgs=/path/to/checkout/of/nixpkgs nix-build '<nixpkgs/nixos/tests/docker-tools.nix>'

Apply the test before the fix to verify the test tests for regression.

cc @nlewo (who seemed to work on the docker tooling a bunch)

nlewo · 2018-05-23T08:32:21Z

Thanks for your investigation!

I think your patch is not working since the extraCommands would not be present in the mkRootLayer build environment: the image is built because extraCommands are not evaluated :)
This is why a run-as-root script is created in the mkRootLayer function. So you should also create such kind of script for the extraCommands attribute.

Regarding the regression test, you could also reproduce with this expression (which is maybe more close to the issue since is not related to buildImageWithNixDb)

runAsRootExtraCommands = buildImage {
    name = "runAsRootExtraCommands";
    contents = [ pkgs.coreutils ];
    runAsRoot = ''echo "(runAsRoot)" > runAsRoot'';
    extraCommands = ''echo "extraCommand" > extraCommands'';
  };

At run time, we could check these files exist:

docker run runasrootextracommands cat extraCommands
docker run runasrootextracommands cat runAsRoot

samueldr · 2018-05-23T14:10:54Z

(Changed as WIP as I will verify the images later)

Thanks for the comments!

samueldr · 2018-05-24T01:13:19Z

Oh wow, thanks for the tip on how to write a more proper test. I'm not used to docker, so I didn't know what and how to test. It's an elegant and simple way to check if the commands ran.

I'm not sure whether the unshare bit from runAsRoot is needed. I think it would not since the previous embedding didn't do anything like it.

I have, once again, checked the new test with and without the change. It does fail before my new fix, and does pass after the new fix. I tested it also with the old fix, and it failed, so you were right!

The PR has been updated accordingly.

nlewo · 2018-05-24T06:40:48Z

nixos/tests/docker-tools.nix

+    contents = [ pkgs.coreutils ];
+    # The parens here are to create problematic bash to embed and eval. In case
+    # this is *embedded* into the script (with nix expansion) the initial quotes
+    # will close the string and the following parens are unexpected


You can also mention the issue you've opened

nlewo · 2018-05-24T06:42:40Z

pkgs/build-support/docker/default.nix

-    let runAsRootScript = shellScript "run-as-root.sh" runAsRoot;
+    let
+      runAsRootScript = shellScript "run-as-root.sh" runAsRoot;
+      extraCommandsScript = shellScript "extracommands.sh" extraCommands;


Could you call it extra-commands.sh since run-as-root.sh is "dashed"

nlewo · 2018-05-24T07:02:00Z

Thanks for you changes. There are just some minor comments.
I also think we don't have to use unshare for extraCommands.

The extraCommands was, previously, simply put in the body of the script using nix expansion `${extraCommands}` (which looks exactly like bash expansion!). This causes issues like in NixOS#34779 where scripts will eventually create invalid bash. The solution is to use a script like `run-as-root`. * * * Fixes NixOS#34779

samueldr · 2018-05-24T10:57:18Z

Updated with the relevant changes.

nlewo · 2018-05-24T14:12:43Z

nixos/tests/docker-tools.nix

@@ -16,6 +16,17 @@ import ./make-test.nix ({ pkgs, ... }: {
      };
  };

+  runAsRootExtraCommands = pkgs.dockerTools.buildImage {


Arf, sorry, I haven't seen this image was in this file:/ Could you either move it to docker/examples.nix or in a

let runAsRootExtraCommands =... in import ./make-test.nix....

samueldr · 2018-05-24T16:25:34Z

No worries, I want this to be as best as possible. Though I hope now all is well :)

nlewo · 2018-05-24T18:09:04Z

@GrahamcOfBorg test docker-tools

GrahamcOfBorg · 2018-05-24T18:09:28Z

Success on aarch64-linux

Attempted: tests.docker-tools

No partial log is available.

GrahamcOfBorg · 2018-05-24T18:51:06Z

Success on x86_64-linux (full log)

Attempted: tests.docker-tools

Partial log (click to expand)

syncing
docker: running command: sync
docker# [   85.824394] dhcpcd[773]: docker0: carrier lost
docker: exit status 0
test script finished in 86.93s
cleaning up
killing docker (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/kwhzsf8y4xsjc3vj48wq3vs7zs0sw327-vm-test-run-docker-tools

nlewo · 2018-05-24T19:22:28Z

Thanks!

GrahamcOfBorg added 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0 6.topic: nixos labels May 23, 2018

samueldr changed the title ~~dockerTools: fixes extraCommands for mkRootLayer.~~ WIP: dockerTools: fixes extraCommands for mkRootLayer. May 23, 2018

nlewo added the 2.status: work-in-progress label May 23, 2018

samueldr force-pushed the fix/34779 branch from ee0f7bc to 77538f2 Compare May 24, 2018 01:13

samueldr changed the title ~~WIP: dockerTools: fixes extraCommands for mkRootLayer.~~ dockerTools: fixes extraCommands for mkRootLayer. May 24, 2018

nlewo reviewed May 24, 2018

View reviewed changes

samueldr force-pushed the fix/34779 branch from 77538f2 to d721174 Compare May 24, 2018 10:51

nlewo reviewed May 24, 2018

View reviewed changes

tests/docker-tools: Adds regression test for NixOS#34779

902b059

samueldr force-pushed the fix/34779 branch from d721174 to 902b059 Compare May 24, 2018 16:24

nlewo merged commit 2e98e0c into NixOS:master May 24, 2018

samueldr deleted the fix/34779 branch June 1, 2018 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dockerTools: fixes extraCommands for mkRootLayer. #40947

dockerTools: fixes extraCommands for mkRootLayer. #40947

samueldr commented May 23, 2018 •

edited

nlewo commented May 23, 2018

samueldr commented May 23, 2018 •

edited

samueldr commented May 24, 2018

nlewo May 24, 2018

nlewo May 24, 2018

nlewo commented May 24, 2018

samueldr commented May 24, 2018

nlewo May 24, 2018

samueldr commented May 24, 2018

nlewo commented May 24, 2018

GrahamcOfBorg commented May 24, 2018

GrahamcOfBorg commented May 24, 2018

nlewo commented May 24, 2018

dockerTools: fixes extraCommands for mkRootLayer. #40947

dockerTools: fixes extraCommands for mkRootLayer. #40947

Conversation

samueldr commented May 23, 2018 • edited

Motivation for this change

Things done

nlewo commented May 23, 2018

samueldr commented May 23, 2018 • edited

samueldr commented May 24, 2018

nlewo May 24, 2018

Choose a reason for hiding this comment

nlewo May 24, 2018

Choose a reason for hiding this comment

nlewo commented May 24, 2018

samueldr commented May 24, 2018

nlewo May 24, 2018

Choose a reason for hiding this comment

samueldr commented May 24, 2018

nlewo commented May 24, 2018

GrahamcOfBorg commented May 24, 2018

GrahamcOfBorg commented May 24, 2018

nlewo commented May 24, 2018

samueldr commented May 23, 2018 •

edited

samueldr commented May 23, 2018 •

edited