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

dockerTools.streamLayeredImage: expose conf / streamScript in passthru #116119

Conversation

lbpdt
Copy link
Contributor

@lbpdt lbpdt commented Mar 12, 2021

Motivation for this change

This is useful to create wrappers around streamLayeredImage and tweak
conf after it has been generated.

Specifically, we want to update the image tag (repo_tag in conf)
with a unique suffix based on the short hash of the
streamLayeredImage derivation.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This is useful to create wrappers around `streamLayeredImage` and tweak
`conf` after it has been generated.

Specifically, we want to update the image tag (`repo_tag` in `conf`)
with a unique suffix based on the short hash of the
`streamLayeredImage` derivation.
@lbpdt lbpdt requested a review from roberth as a code owner March 12, 2021 23:50
@lbpdt
Copy link
Contributor Author

lbpdt commented Mar 12, 2021

@GrahamcOfBorg test docker-tools

@roberth
Copy link
Member

roberth commented Mar 13, 2021

I don't think we should expose those implementation details. The format of this "conf" file is likely to change, for example when new layering solutions become available (#48462) or when other changes are made, for example to support all buildImage features.
Perhaps you could add support for your use case directly? That way we can also have a test, ensuring that future changes to dockerTools don't break the tagging solution.

@lbpdt
Copy link
Contributor Author

lbpdt commented Mar 21, 2021

I can understand how exposing internals is not ideal but I didn't find another way of solving my problem. Maybe you will have an idea though...

Here's my problem: similar to how each derivation maps to a unique hash in the store, I would like to ensure that an image tag uniquely identifies an image build. If that property holds, it implies that images can be immutable on our docker registry, as you would never have a valid reason for overwriting an image tag on the registry. Indeed if the image tag you want to push is already on the registry, you have the guarantee that it is the same as your local one.

To implement this, we expose conf and stream from streamLayeredImage and write a wrapper around it. That wrapper updates conf by suffixing repo_tag with a unique string based on the hash of the streamLayeredImage derivation in the store. That way, each unique invocation of streamLayeredImage creates a unique image tag.

Would you see another way of implementing this without exposing the internals of streamLayeredImage?

@roberth
Copy link
Member

roberth commented Mar 22, 2021

We could probably bake your solution into streamLayeredImage with a flag to enable it.

@lbpdt
Copy link
Contributor Author

lbpdt commented Mar 22, 2021

I wasn't sure how reusable it could be, but happy to send a PR with the feature if you think it is.

@roberth
Copy link
Member

roberth commented Mar 23, 2021

Actually this sounds a lot like the default imageTag behavior. Is that not sufficient?

nix-repl> i = dockerTools.streamLayeredImage { name = "test"; config.Cmd = hello; }             

nix-repl> i.imageTag                                                                
"cp3qpvzqfz05rx3kvj04lxpdkpaxzr0p"

nix-repl> i = dockerTools.streamLayeredImage { name = "test"; config.Cmd = [hello "--help"]; }

nix-repl> i.imageTag                                                                           
"wpvyhqr2gs3jq3rxv7yfpqdi37kq12d6"

It's currently based only on the conf derivation output hash, so indeed it does not capture changes in the streaming script. We could fix that by moving the conf build (the custom json) into the result (stream script wrapper) derivation. It may also be useful to prefix a human-understandable string.

These changes can be made without increasing the interface surface area too much, they're testable and seem quite useful. Do you need something else?

@stale
Copy link

stale bot commented Sep 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2021
@roberth
Copy link
Member

roberth commented Sep 21, 2021

No answer in months. Will reopen if answered.

@roberth roberth closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants