Skip to content

docker: do not import configuration from the base image #27634

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 26, 2017
Merged

docker: do not import configuration from the base image #27634

merged 1 commit into from
Jul 26, 2017
+3 −0

Conversation

nlewo
Copy link
Member

@nlewo nlewo commented Jul 25, 2017

Motivation for this change

Fix #27632

Things done

Please check what applies. Note that these are not hard requirements but mereley serve as information for reviewers.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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.

Sorry, something went wrong.

lo1tuma
lo1tuma reviewed Jul 26, 2017
View reviewed changes
pkgs/build-support/docker/default.nix Outdated
@@ -434,6 +434,8 @@ rec {
if [[ -n "$fromImage" ]]; then
echo "Unpacking base image..."
tar -C image -xpf "$fromImage"
# Do not import the base image configuration
rm -f image/*.json
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be enough to delete the manifest.json. Which is new since docker/moby image spec v1.2.
The problem is that the manifest.json describes which layers should be used and buildImage doesn’t create such a file.
So it definitely makes sense to not import the manifest.json from the base image 👍. I don’t think it is necessary to delete all json files.

Copy link
Member Author

Choose a reason for hiding this comment

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

For instance, this is the structure of the image of ubuntu + bash:

tar -tf /nix/store/91vm2lng91f313bnngi87qg3grxynjk0-docker-image-bashOnUbuntu.tar.gz
./
./ef590a0764fab1ffa7a6b08499de5033937c003a283af521371b69a8e72a11bd/
./ef590a0764fab1ffa7a6b08499de5033937c003a283af521371b69a8e72a11bd/layer.tar
./ef590a0764fab1ffa7a6b08499de5033937c003a283af521371b69a8e72a11bd/VERSION
./ef590a0764fab1ffa7a6b08499de5033937c003a283af521371b69a8e72a11bd/json
./1fdb68b6ffa4ad4ae4bfacdbfd6595ef9128d0379eb1a6b36470ac1ec547e40a/
./1fdb68b6ffa4ad4ae4bfacdbfd6595ef9128d0379eb1a6b36470ac1ec547e40a/layer.tar
./1fdb68b6ffa4ad4ae4bfacdbfd6595ef9128d0379eb1a6b36470ac1ec547e40a/VERSION
./1fdb68b6ffa4ad4ae4bfacdbfd6595ef9128d0379eb1a6b36470ac1ec547e40a/json
./54333f1de4ed2730bea18e49605b2ea8f8a2689db213ece94db6ccbc8cf279a6.json
./repositories
./7dd07830e2ffed07a198f014fa9c272d2ca898a9dce00919e0662db7635e80fd/
./7dd07830e2ffed07a198f014fa9c272d2ca898a9dce00919e0662db7635e80fd/layer.tar
./7dd07830e2ffed07a198f014fa9c272d2ca898a9dce00919e0662db7635e80fd/VERSION
./7dd07830e2ffed07a198f014fa9c272d2ca898a9dce00919e0662db7635e80fd/json
./b72351754cb42e486d4b35699b73170a9e7560001abe727de5ea376adcb66c8b/
./b72351754cb42e486d4b35699b73170a9e7560001abe727de5ea376adcb66c8b/layer.tar
./b72351754cb42e486d4b35699b73170a9e7560001abe727de5ea376adcb66c8b/VERSION
./b72351754cb42e486d4b35699b73170a9e7560001abe727de5ea376adcb66c8b/json
./5c0fae2698664beefe0c37cf9815bc8173afc87965bec5455dd4addbb2d23847/
./5c0fae2698664beefe0c37cf9815bc8173afc87965bec5455dd4addbb2d23847/layer.tar
./5c0fae2698664beefe0c37cf9815bc8173afc87965bec5455dd4addbb2d23847/VERSION
./5c0fae2698664beefe0c37cf9815bc8173afc87965bec5455dd4addbb2d23847/json
./manifest.json
./c4dd515170301a4220cf3006bd541bb22dd4bddebe0297f8cd42e28c2667d8ef/
./c4dd515170301a4220cf3006bd541bb22dd4bddebe0297f8cd42e28c2667d8ef/layer.tar
./c4dd515170301a4220cf3006bd541bb22dd4bddebe0297f8cd42e28c2667d8ef/VERSION
./c4dd515170301a4220cf3006bd541bb22dd4bddebe0297f8cd42e28c2667d8ef/json

What would be deleted by this patch is the file 54333f1de4ed2730bea18e49605b2ea8f8a2689db213ece94db6ccbc8cf279a6.json.
But the file manifest.json is also wrong since it is inherited from the base image.
So, I think both of these files could be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the manifest.json should be definitely deleted. The other file is the image config of the parent image, which could be (accordingly to the spec) optionally referenced in the manifest.json, but as buildImage doesn’t (yet) produce such a file it would be also ok the delete the config of the parent.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lo1tuma Ok, the latest patch did this.

FYI, regarding the manifest.json and image config, I started to work a bit on them https://github.com/nlewo/nixpkgs-contrail/blob/master/jobset.nix#L40 since they are required to push the image to a v2 registry.
I will try to upstream these changes later.

Copy link
Member

@lo1tuma lo1tuma Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I also made a patch to make buildImage produce image spec v1.2 compatible images, see holidaycheck@ece4233

Before submitting this patch to upstream I still want to implement a test for dockerTools.

@nlewo
docker: do not import configuration and manifest from the base image

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
48c4c29 
Fix NixOS#27632.
@nlewo nlewo force-pushed the docker-remove-config branch from 263890f to 48c4c29 Compare July 26, 2017 15:02
@globin
Copy link
Member

globin commented Jul 26, 2017

Verified this works with a similar testcase to the one in the issue:

{ pkgs ? import <nixpkgs> {} }:

rec {
  ubuntu = pkgs.dockerTools.pullImage {
    imageName = "ubuntu";
    imageTag = "14.04";
    sha256 = "1gqs9xzlayaqq2wgdcp3fdg3fws50vy5a69xkxa40b0dasa9i3mk";
  };
  bash = pkgs.dockerTools.buildImage {
    name = "bash";
    contents = pkgs.bashInteractive;
  };
  bashOnUbuntu = pkgs.dockerTools.buildImage {
    name = "bash-on-ubuntu";
    contents = pkgs.bashInteractive;
    fromImage = ubuntu;
  };
}

nix-build -I nixpkgs=. test.nix
docker load < result-2
docker run bash-on-ubuntu ls | grep nix

@globin globin merged commit 0a4c430 into NixOS:master Jul 26, 2017
@nlewo
Copy link
Member Author

nlewo commented Jul 26, 2017

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lo1tuma lo1tuma

@NeQuissimus NeQuissimus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nlewo @globin @lo1tuma @NeQuissimus