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

fix: add cfg.path to containerd-rootless.sh PATH #145

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

isbecker
Copy link
Contributor

@isbecker isbecker commented Sep 1, 2024

This fixes #106.

See my comment in #106 (comment)

This PR adds the ++ cfg.path that I mentioned.

I have tested this change by setting the path in the virtualisation.containerd.rootless config, like so:

virtualisation.containerd.rootless = {
    enable = true;
    nixSnapshotterIntegration = true;
    path = [
      "/usr"
    ];
  };

Since I am running on Ubuntu, I need to add /usr to the path (and containerd-rootless.nix uses lib.makeBinPath which add /bin).

The final containerd-rootless.sh script's PATH includes /usr/bin and now rootlesskit succesfully finds newuidmap on the PATH.

export PATH="/nix/store/ycaw9wiw7s3ky5jdih595yjy7wz5vbyf-containerd-rootless-child/bin:/nix/store/4bj2kxdm1462fzcc2i2s4dn33g2angcc-bash-5.2p32/bin:/nix/store/kisggjnkwyhnq1p12wpjyi1dnymhrczg-iproute2-6.10.0/bin:/nix/store/2irps75xqy0gpj4j543kiv7gafhy89yg-libselinux-3.6-bin/bin:/nix/store/9li94qxgjzhldqc4c27d76an41v06sfz-rootlesskit-2.1.0/bin:/nix/store/yjx4kjhgw6hn66bk5ayp2w8s50nvwcyw-slirp4netns-1.3.1/bin:/nix/store/2lx8zljg14lpyicrb41191sayqa7h0pr-util-linux-2.39.4-bin/bin:/run/wrappers/bin:/usr/bin"

If you want to test this change, you can use the fork, something like so:

    nix-snapshotter = {
      url = "github:isbecker/nix-snapshotter";
      inputs.nixpkgs.follows = "nixpkgs";
    };

Finally, testing an image:

$ nerdctl run ghcr.io/pdtpartners/hello
Hello, world!

@RobbieBuxton RobbieBuxton added the ok-to-test Runs NixOS tests label Sep 1, 2024
@RobbieBuxton
Copy link
Collaborator

RobbieBuxton commented Sep 2, 2024

Apologies if this is a silly question but is there a reason why we can't just add shadow-utils (which provides newuidmap) as a runtime dependency? I think it's su in nixpkgs. That way we don't need to rely on the users local installation. My Linux knowledge is a little limited so I might be completely missing something here?

@RobbieBuxton RobbieBuxton self-requested a review September 2, 2024 08:57
@isbecker
Copy link
Contributor Author

isbecker commented Sep 2, 2024

Hi @RobbieBuxton, thanks for taking a look.

Apologies if this is a silly question but is there a reason why we can't just add shadow-utils (which provides newuidmap) as a runtime dependency? I think it's su in nixpkgs. That way we don't need to rely on the users local installation. My Linux knowledge is a little limited so I might be completely missing something here?

According to this discussion on setting up rootless podman:

Ok, looks like the issue was that the newuidmap binary from shadow package lacks suid bit, and a wrapper located in /run/wrappers/bin/newuidmap should be used instead.

And in nix-snapshotter, you are already doing something similar with /run/wrappers, but that doesn't work for non-nixOS systems, as far as I can tell.

@RobbieBuxton
Copy link
Collaborator

Hi @RobbieBuxton, thanks for taking a look.

Apologies if this is a silly question but is there a reason why we can't just add shadow-utils (which provides newuidmap) as a runtime dependency? I think it's su in nixpkgs. That way we don't need to rely on the users local installation. My Linux knowledge is a little limited so I might be completely missing something here?

According to this discussion on setting up rootless podman:

Ok, looks like the issue was that the newuidmap binary from shadow package lacks suid bit, and a wrapper located in /run/wrappers/bin/newuidmap should be used instead.

And in nix-snapshotter, you are already doing something similar with /run/wrappers, but that doesn't work for non-nixOS systems, as far as I can tell.

Ah that makes sense thanks! I went down the rabbit hole and it definitely seems like it's a bit of a mess on NixOS atm.

Copy link
Collaborator

@RobbieBuxton RobbieBuxton left a comment

Choose a reason for hiding this comment

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

Thanks for looking in this @isbecker! I'm happy to merge if you are @elpdt852

@isbecker
Copy link
Contributor Author

isbecker commented Sep 4, 2024

Just for posterity, here's another reference to the newuidmap issues with shadow

@elpdt852
Copy link
Collaborator

Thanks @isbecker, finally had the time to dig into reviewing this one. It looks like in the podman init PR in home-manager, they opted to just hard code some system paths for distros like Ubuntu. I think the diff for this PR seems reasonable and flexible, extending the cfg.path to the systemd environment which I think is fine.

Appreciate the PR!

@elpdt852 elpdt852 merged commit cb70837 into pdtpartners:main Oct 22, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Runs NixOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

containerd rootless service fails on Linux Mint with home-manager
3 participants