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

nixos/podman: Add docker socket support #123841

Merged
merged 5 commits into from
May 30, 2021
Merged

Conversation

roberth
Copy link
Member

@roberth roberth commented May 20, 2021

Motivation for this change

Podman supports the Docker API.
Enable more compatibility options to use podman as a docker replacement.

  • Make the Podman socket usable via podman group
  • Add option to symlink from Docker to Podman socket
  • Add options for networked socket with TLS client certificate authentication

I've successfully deployed with docker-compose via these Docker interfaces all the way to the Podman socket.
It's a promising solution since Podman promises systemd container support and Docker has regressed in this regard.

cc @NixOS/podman

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.

@roberth roberth requested review from adisbladis, cpcloud and zowoq May 20, 2021 23:05
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 20, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 20, 2021
@r-rmcgibbo

This comment has been minimized.

@roberth roberth changed the title Podman socket nixos/podman: Add docker socket support May 20, 2021
@roberth
Copy link
Member Author

roberth commented May 20, 2021

@GrahamcOfBorg test podman podman-tls-ghostunnel

@zowoq
Copy link
Contributor

zowoq commented May 20, 2021

Two quick comments:

Can we test docker compose?

Can you add yourself as a maintainer for the podman compat test (or add yourself to the podman team)?

I'll be able to do a proper review later today.

@roberth
Copy link
Member Author

roberth commented May 21, 2021

Can we test docker compose?

That will be a nice addition. I have to do more (unplanned) work on arion as well, so let's not delay this PR.

Can you add yourself as a maintainer for the podman compat test

Done.

(or add yourself to the podman team)?

Thanks, but sadly I'm not in a position to commit to more maintenance responsibilities.

"L! /run/docker.sock - - - - /run/podman/podman.sock"
];

services.ghostunnel = lib.mkIf cfg.networkSocket.enable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the ghostunnel stuff here and in the test, seems odd to add have the implementation of a specific proxy in the podman module. Can't it live somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is the ghostunnel stuff here

Good point. I've moved it into a separate module and it's much cleaner now.
I've also added myself as a maintainer there.

and in the test

I'm a hesitant to split the tests up further, because of the VM startup overhead. I'm not too concerned about Hydra (It's like 10 to 20 seconds perhaps) but about running the tests locally.
I did create a new test for the socket-based compatibility stuff, because the "core" podman tests in nixosTests.podman must work without the compatibility stuff.

seems odd to add have the implementation of a specific proxy in the podman module. Can't it live somewhere else?

Having a module make an implementation choice isn't anything new. For instance, we have acme modules and options that are implemented by the lego client. In other words, it's ok for a module to be opinionated. I don't care about the choice of TLS server and I don't think swapping it out would cause any issues.

@zowoq
Copy link
Contributor

zowoq commented May 21, 2021

Sorry but that isn't what I meant. Why can't this be handled by generic method via ghostunnel without the needing a podman.networksocket option?

@roberth
Copy link
Member Author

roberth commented May 21, 2021

Sorry but that isn't what I meant. Why can't this be handled by generic method via ghostunnel without the needing a podman.networksocket option?

It adds value. You don't have to look up the details of where the socket is, how the ghostunnel syntax works, etc. It provides a single open source solution for this small problem. It also makes the possibility of exposing Podman via TLS discoverable in the NixOS option docs.

@zowoq
Copy link
Contributor

zowoq commented May 21, 2021

I'm a strong no on this as it is, sorry. I don't agree with the opinionated config and having this much of the implementation under the podman module.

@cpcloud
Copy link
Contributor

cpcloud commented May 21, 2021

Is there some way the networkSocket option can allow for a different proxy, defaulting to ghostunnel, so we can have the best of both worlds (sane defaults and optionality with respect to the choice of proxy)?

@roberth
Copy link
Member Author

roberth commented May 21, 2021

Is there some way the networkSocket option can allow for a different proxy, defaulting to ghostunnel, so we can have the best of both worlds (sane defaults and optionality with respect to the choice of proxy)?

That's possible. You could even use the module system's type merging to make it open for extension. I'd lay it out as follows:

  • podman-network-socket.nix, declaring the options, including a new enum option virtualization.podman.networkSocket.server
  • podman-network-socket-ghostunnel.nix, extending the server type with "ghostunnel" and conditionally providing the implementation. mkIf (cfg.server == "ghostunnel") etc.

Providing other server integrations will be a matter of adding another module similar to the -ghostunnel.nix one.

If that sounds ok to you, I'll implement it that way.

@cpcloud
Copy link
Contributor

cpcloud commented May 21, 2021

That sounds reasonable to me. @zowoq How does that sound?

@zowoq
Copy link
Contributor

zowoq commented May 22, 2021

I'd lay it out as follows ...

This sound okay if the networkSocket.server enum option defaults to null so that ghostunnel needs to be explicitly selected, this would make it reasonably clear that it's a third party package/module and not podman itself.

I'll also ask that the test be split so that the core / first-party local socket be tested separately from the non-core / third party network socket. I'm fine with the local tests being added to the current core podman tests, we should also add a TODO for including docker compose test as part of the core tests as well.

@roberth
Copy link
Member Author

roberth commented May 24, 2021

if the networkSocket.server enum option defaults to null so that ghostunnel needs to be explicitly selected,

Done, but the default is unset rather than null. This has the same effect, but won't let people set null, because that is not a server.

@roberth roberth requested a review from zowoq May 24, 2021 06:50
@roberth roberth requested a review from zowoq May 24, 2021 10:10
Copy link
Contributor

@zowoq zowoq left a comment

Choose a reason for hiding this comment

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

Couple of nits but otherwise this looks okay, thanks for making the changes. I would like to wait a couple of days before we merge to allow the other podman maintainers a chance to comment.

Also, could you please clean up the history on this branch before this is merged? With this spread over eleven commits it's adding a bit of unnecessary noise to these files.

nixos/tests/podman-tls-ghostunnel.nix Outdated Show resolved Hide resolved
nixos/tests/podman.nix Outdated Show resolved Hide resolved
environment.systemPackages = [
# Installs the docker _client_ only
# Normally, you'd want `virtualisation.docker.enable = true;`.
pkgs.docker
Copy link
Contributor

@zowoq zowoq May 27, 2021

Choose a reason for hiding this comment

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

Opened #124561 so we can use a client only package.

Copy link
Contributor

@zowoq zowoq left a comment

Choose a reason for hiding this comment

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

One question, feel free to merge after it's resolved.

Edit: Needs to be rebased as well, sorry.

environment.systemPackages = [
# Installs the docker _client_ only
# Normally, you'd want `virtualisation.docker.enable = true;`.
pkgs.docker
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to pkgs.docker-client?

@ofborg ofborg bot requested a review from zowoq May 30, 2021 09:34
@roberth roberth merged commit 774fe18 into NixOS:master May 30, 2021
@roberth
Copy link
Member Author

roberth commented May 30, 2021

🎉 Thank you so much @zowoq!

@buckley310
Copy link
Contributor

@roberth Do you happen to recall why the tmpfiles rule is configured to only run on boot? (L! vs L)
Just checking that there's a reason. If so, I may do a PR to mention in the option description so people know to reboot.

@roberth
Copy link
Member Author

roberth commented Aug 1, 2023

@buckley310

only run on boot? (L! vs L)

I don't recall and it may well be a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants