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.

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

nixos/docker-registry: add configuration options to support image deletions #37871

Merged
merged 4 commits into from May 2, 2018
Merged

nixos/docker-registry: add configuration options to support image deletions #37871

merged 4 commits into from May 2, 2018

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 26, 2018 •
edited

Motivation for this change

The main purpose of this PR originally prepared by @ironpinguin and finished by myself was to allow deletions of image revisions in a docker registry.
The original module used the sample configuration provided by docker/distribution which disabled this feature. We decided to generate a JSON-based config based on a Nix object that can be filled with values from the configuration. Furthermore the testcase ensures now that images can be deleted.

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.

Ma27
Ma27 commented Mar 26, 2018
View reviewed changes
Copy link
Member Author

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

just found two issues, will fix them and document the breaking change with extraConfig in the release notes

nixos/modules/services/misc/docker-registry.nix Outdated
script = ''
${pkgs.docker-distribution}/bin/registry serve \
${pkgs.docker-distribution.out}/share/go/src/github.com/docker/distribution/cmd/registry/config-example.yml
environment = cfg.extraConfig;
Copy link
Member Author

Choose a reason for hiding this comment

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

actually we should drop this and merge the extraConfig with the configuration tree

nixos/modules/services/misc/docker-registry.nix Outdated

redisPassword = mkOption {
type = types.str;
default = "asecret";
Copy link
Member Author

Choose a reason for hiding this comment

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

we should not use set a secret as default here

@Ma27 Ma27 force-pushed the docker-registry-enhancements branch from 1982095 to 3e2e57d Compare March 26, 2018 14:55
@Ma27
Copy link
Member Author

Ma27 commented Mar 27, 2018

/cc @globin

@Ma27
Copy link
Member Author

Ma27 commented Apr 8, 2018

@fpletz @globin: @ironpinguin and I implemented basic support for garbage collection (including an enhanced testcase, would be cool if you could have a look at it) :)

nlewo
nlewo reviewed Apr 9, 2018
View reviewed changes
nixos/tests/docker-registry.nix
'ls -l /var/lib/docker-registry/docker/registry/v2/blobs/sha256/*/*/data'
);

$client1->succeed("docker push registry:8080/scratch");
Copy link
Member

Choose a reason for hiding this comment

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

Why are you pushing the image again?
Wouldn't be sufficient to do this test just before starting the garbage collector (around line 40)?

Copy link
Member Author

Choose a reason for hiding this comment

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

we wanted to ensure that after running gc and confirming that all blobs were purged that the registry is still functional akd can receive new images

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks

@Ma27
Copy link
Member Author

Ma27 commented Apr 22, 2018

is there anything that keeps us from merging this patch?

@Ma27 Ma27 force-pushed the docker-registry-enhancements branch from db0d5c7 to a71c4c1 Compare April 30, 2018 07:10
@Ma27
Copy link
Member Author

Ma27 commented Apr 30, 2018

@globin @fpletz I rebased the patches onto the latest master (to resolve some merge conflicts in the release notes), could you have a look at this pls? :)

globin
globin requested changes May 1, 2018
View reviewed changes
nixos/modules/services/misc/docker-registry.nix Outdated

redisUrl = mkOption {
type = types.str;
default = "localhost:6479";
Copy link
Member

Choose a reason for hiding this comment

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

6379?

@Ma27 Ma27 force-pushed the docker-registry-enhancements branch from a71c4c1 to b2b6a1b Compare May 1, 2018 12:54
@Ma27
Copy link
Member Author

Ma27 commented May 1, 2018

@globin good catch - thanks!

@Ma27 Ma27 force-pushed the docker-registry-enhancements branch from b2b6a1b to f7f8cd3 Compare May 1, 2018 13:14
Ma27 and others added 2 commits May 1, 2018 15:23
@Ma27
nixos/docker-registry: cleanup module definition & enhance testcase
593dc45 
The following changes have been applied:

- the property `http.headers.X-Content-Type-Options` must a list of
  strings rather than a serialized list
- instead of `/etc/docker/registry/config.yml` the configuration will be
  written with `pkgs.writeText` and the store path will be used to run
  the registry. This reduces the risk of possible impurities by relying
  on the Nix store only.
- cleaned up the property paths to easy readability and reduce the
  verbosity.
- enhanced the testcase to ensure that digests can be deleted as well
- the `services.docker-registry.extraConfig` object will be merged with
  `registryConfig`

/cc @ironpinguin
@ironpinguin @Ma27
nixos/docker-registry: Add support for garbage collector to docker re…
afd3136 
…gistry
@Ma27 Ma27 force-pushed the docker-registry-enhancements branch from f7f8cd3 to afd3136 Compare May 1, 2018 13:24
@globin globin merged commit b28d151 into NixOS:master May 2, 2018
@Ma27 Ma27 deleted the docker-registry-enhancements branch May 2, 2018 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlewo nlewo nlewo left review comments

@globin globin globin approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Ma27 @globin @nlewo @GrahamcOfBorg @ironpinguin