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

darwin.builder: init #206951

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Conversation

Gabriella439
Copy link
Contributor

Fixes #108984

This originates from:

https://github.com/Gabriella439/macos-builder

… which in turn originates from:

https://github.com/YorikSar/nixos-vm-on-macos

Once this change is built and cached, the end user only needs to run:

$ nix run nixpkgs#darwin.builder

… to create a Linux builder on macOS

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Dec 20, 2022
@Gabriella439 Gabriella439 changed the title Upstream macos-builder project into Nixpkgs darwin.builder: init Dec 20, 2022
@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 Dec 20, 2022
@Gabriella439
Copy link
Contributor Author

Note that this is still missing documentation, which I will complete before merging this

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Dec 20, 2022
@domenkozar
Copy link
Member

Thanks a lot for getting this in a frinedly state ❤️

… as suggested by @domenkozar

Co-authored-by: Domen Kožar <[email protected]>
Comment on lines +7 to +8
This also requires that port 22 on your machine is free (since Nix does not
permit specifying a non-default SSH port for builders).
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to change this via the root's ssh_config

Comment on lines +1 to +7
-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
QyNTUxOQAAACCQVnMW/wZWqrdWrjrRPhfEFFq1KLYguagSflLhFnVQmwAAAJASuMMnErjD
JwAAAAtzc2gtZWQyNTUxOQAAACCQVnMW/wZWqrdWrjrRPhfEFFq1KLYguagSflLhFnVQmw
AAAEDIN2VWFyggtoSPXcAFy8dtG1uAig8sCuyE21eMDt2GgJBWcxb/Blaqt1auOtE+F8QU
WrUotiC5qBJ+UuEWdVCbAAAACnJvb3RAbml4b3MBAgM=
-----END OPENSSH PRIVATE KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

I am not in favor of hardcoding a private key. What alternatives do we have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of a good alternative here. However, in this case I think it's not an issue because this is only use as the SSH host key and not the client's key (which is still handled securely)

networking.nameservers = [ "8.8.8.8" ];

nix.settings = {
auto-optimise-store = true;
Copy link
Member

Choose a reason for hiding this comment

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

I have turned this off since a while to reduce io load but didn't do benchmarking. Is the difference noticeable? Not sure if we should turn it on by default or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually am not too attached to this setting. I don't remember why I added it

Comment on lines +65 to +69
installCredentials = pkgs.writeShellScript "install-credentials" ''
KEYS="''${1}"
INSTALL=${hostPkgs.coreutils}/bin/install
"''${INSTALL}" -g nixbld -m 600 "''${KEYS}/${user}_${keyType}" ${privateKey}
"''${INSTALL}" -g nixbld -m 644 "''${KEYS}/${user}_${keyType}.pub" ${publicKey}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
installCredentials = pkgs.writeShellScript "install-credentials" ''
KEYS="''${1}"
INSTALL=${hostPkgs.coreutils}/bin/install
"''${INSTALL}" -g nixbld -m 600 "''${KEYS}/${user}_${keyType}" ${privateKey}
"''${INSTALL}" -g nixbld -m 644 "''${KEYS}/${user}_${keyType}.pub" ${publicKey}
installCredentials = let
install = ${hostPkgs.coreutils}/bin/install;
in pkgs.writeShellScript "install-credentials" ''
"${install}" -g nixbld -m 600 "''${1}/${user}_${keyType}" ${privateKey}
"${install}" -g nixbld -m 644 "''${1}/${user}_${keyType}.pub" ${publicKey}

If the goal is to make the script as easy as possible then this is in the end as easy as it gets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I deliberately did not do it that way because I wanted the script to be easier to read. In particular, I wanted the script to fit within 80 columns

Copy link
Member

@SuperSandro2000 SuperSandro2000 Dec 24, 2022

Choose a reason for hiding this comment

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

Something does not get easier to read if it fits within 80 columns but has two extra lines.

KEYS="$(nix-store --add "$KEYS")" ${config.system.build.vm}/bin/run-nixos-vm
'';

system.stateVersion = "22.05";
Copy link
Member

Choose a reason for hiding this comment

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

This should be taken from nixpkgs instead of being hardcoded here or set once on initialization.

@domenkozar
Copy link
Member

I'm going to merge this because it improves the current state a lot and further improvements can be done once people start to use it.

@domenkozar domenkozar merged commit bcc8d11 into NixOS:master Dec 21, 2022
@github-actions
Copy link
Contributor

Successfully created backport PR #207097 for release-22.11.

@erikarvstedt erikarvstedt mentioned this pull request Dec 23, 2022
13 tasks
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 24, 2022

@domenkozar there is no time pressure here at all and new features should not be merged with barely any review just because people desperately want it.

Doing major redesigns of new features straight after they where rushed to be merged is suboptimal for anyone that started to use the feature already.

SuperSandro2000 added a commit to SuperSandro2000/nixpkgs that referenced this pull request Dec 24, 2022
@SuperSandro2000
Copy link
Member

Did the review comments myself in #207486

@domenkozar
Copy link
Member

@domenkozar there is no time pressure here at all and new features should not be merged with barely any review just because people desperately want it.

Doing major redesigns of new features straight after they where rushed to be merged is suboptimal for anyone that started to use the feature already.

It's not about time pressure, it's about getting this work to the devs to test and moving the bikeshedding into a step that can happen in parallel.

@roberth
Copy link
Member

roberth commented Dec 24, 2022

crucially, getting hydra to put it in cache.nixos.org

Gabriella439 added a commit to Gabriella439/nixpkgs that referenced this pull request Dec 26, 2022
… until we can figure out why it's being rebuilt for seemingly
unrelated changes.

See: NixOS#206951 (comment)
@Gabriella439
Copy link
Contributor Author

Here's the PR to disable the build for now if people need this to be reverted: #207776

@figsoda
Copy link
Member

figsoda commented Dec 26, 2022

not sure if disabling the build is needed since Mic92/nixpkgs-review#291 is merged and nixpkgs-review users will not have to rebuild darwin.builder anymore, the only downside after nixpkgs-review is updated afaik would be the github labels which imo is not a big deal

@zowoq
Copy link
Contributor

zowoq commented Dec 26, 2022

github labels which imo is not a big deal

ofborg telling contributors that a zero rebuild PR causes one rebuild on each darwin platforms is rather confusing.

@Gabriella439
Copy link
Contributor Author

Just to clarify: is darwin.builder being rebuilt on these unrelated PRs or is it a bug in ofborg?

@raphaelr
Copy link
Contributor

I can reproduce this locally:

$ nix show-derivation -f . darwin.builder --system aarch64-darwin > a
$ git commit --allow-empty -m test
$ nix show-derivation -f . darwin.builder --system aarch64-darwin > b
$ diff -u a b
--- a   2022-12-26 23:11:31.175857599 +0100
+++ b   2022-12-26 23:11:53.008634417 +0100
@@ -1,5 +1,5 @@
 {
-  "/nix/store/9fgpywcl1qgk5yrwwq9b829pncw4jrhi-create-builder.drv": {
+  "/nix/store/qj88p4c8b9nw8z6cg4cf425l5y4d1hah-create-builder.drv": {
     "args": [
       "-e",
       "/nix/store/9krlzvny65gdc8s7kpb6lkx8cd02c25b-default-builder.sh"
@@ -34,7 +34,7 @@
       "mesonFlags": "",
       "name": "create-builder",
       "nativeBuildInputs": "",
-      "out": "/nix/store/5ggib8k7z9k6ryzpnhijah8apsc071la-create-builder",
+      "out": "/nix/store/cxb5sjl4klf0l8b0r3qh5z4r1a9ldzqa-create-builder",
       "outputs": "out",
       "passAsFile": "buildCommand text",
       "patches": "",
@@ -44,13 +44,13 @@
       "stdenv": "/nix/store/s5rvpdf77v47cqy1n56wr5xlj3b7vzmm-stdenv-darwin",
       "strictDeps": "",
       "system": "aarch64-darwin",
-      "text": "#!/nix/store/x380x9d7gqh4ig9h50a7wx08p0s6622f-bash-5.1-p16/bin/bash\nKEYS=\"${KEYS:-./keys}\"\n/nix/store/npcq66nqv2wpzp3ma5s63vafnnivphxb-coreutils-9.1/bin/mkdir --parent \"${KEYS}\"\nPRIVATE_KEY=\"${KEYS}/builder_ed25519\"\nPUBLIC_KEY=\"${PRIVATE_KEY}.pub\"\nif [ ! -e \"${PRIVATE_KEY}\" ] || [ ! -e \"${PUBLIC_KEY}\" ]; then\n    /nix/store/npcq66nqv2wpzp3ma5s63vafnnivphxb-coreutils-9.1/bin/rm --force -- \"${PRIVATE_KEY}\" \"${PUBLIC_KEY}\"\n    /nix/store/kgp4br70j8w4k4lgsv8sk22hb51km36s-openssh-9.1p1/bin/ssh-keygen -q -f \"${PRIVATE_KEY}\" -t ed25519 -N \"\" -C 'builder@localhost'\nfi\nif ! /nix/store/2m2jha21jq3f51g0rd42y385v5vfkgih-diffutils-3.8/bin/cmp \"${PUBLIC_KEY}\" /etc/nix/builder_ed25519.pub; then\n  (set -x; sudo --reset-timestamp /nix/store/grd27v1gb7s0hqq7wrnqw88m60mlr8wk-install-credentials \"${KEYS}\")\nfi\nKEYS=\"$(nix-store --add \"$KEYS\")\" /nix/store/pd3vpvsg7sqri0pr9qpjr38g61wik1rz-nixos-vm/bin/run-nixos-vm\n\n"
+      "text": "#!/nix/store/x380x9d7gqh4ig9h50a7wx08p0s6622f-bash-5.1-p16/bin/bash\nKEYS=\"${KEYS:-./keys}\"\n/nix/store/npcq66nqv2wpzp3ma5s63vafnnivphxb-coreutils-9.1/bin/mkdir --parent \"${KEYS}\"\nPRIVATE_KEY=\"${KEYS}/builder_ed25519\"\nPUBLIC_KEY=\"${PRIVATE_KEY}.pub\"\nif [ ! -e \"${PRIVATE_KEY}\" ] || [ ! -e \"${PUBLIC_KEY}\" ]; then\n    /nix/store/npcq66nqv2wpzp3ma5s63vafnnivphxb-coreutils-9.1/bin/rm --force -- \"${PRIVATE_KEY}\" \"${PUBLIC_KEY}\"\n    /nix/store/kgp4br70j8w4k4lgsv8sk22hb51km36s-openssh-9.1p1/bin/ssh-keygen -q -f \"${PRIVATE_KEY}\" -t ed25519 -N \"\" -C 'builder@localhost'\nfi\nif ! /nix/store/2m2jha21jq3f51g0rd42y385v5vfkgih-diffutils-3.8/bin/cmp \"${PUBLIC_KEY}\" /etc/nix/builder_ed25519.pub; then\n  (set -x; sudo --reset-timestamp /nix/store/grd27v1gb7s0hqq7wrnqw88m60mlr8wk-install-credentials \"${KEYS}\")\nfi\nKEYS=\"$(nix-store --add \"$KEYS\")\" /nix/store/yphpd9i8w0vg0p3p70yck0vmmcdzq4fi-nixos-vm/bin/run-nixos-vm\n\n"
     },
     "inputDrvs": {
-      "/nix/store/7a0chkp8maxy1j20f3f937ikmxjkxisi-nixos-vm.drv": [
+      "/nix/store/c47v02plj8ziz3j0x13p7hdz8j0d7cxr-openssh-9.1p1.drv": [
         "out"
       ],
-      "/nix/store/c47v02plj8ziz3j0x13p7hdz8j0d7cxr-openssh-9.1p1.drv": [
+      "/nix/store/cn0bc111q4ha76ggmpfhywwpw411x3id-nixos-vm.drv": [
         "out"
       ],
       "/nix/store/g2npqs9fic2vvrxf117czf1j1936d63c-install-credentials.drv": [
@@ -74,7 +74,7 @@
     ],
     "outputs": {
       "out": {
-        "path": "/nix/store/5ggib8k7z9k6ryzpnhijah8apsc071la-create-builder"
+        "path": "/nix/store/cxb5sjl4klf0l8b0r3qh5z4r1a9ldzqa-create-builder"
       }
     },
     "system": "aarch64-darwin"

If it's git commits that's causing this, then I assume the darwin.builder needs to hard code a git revision, like in release.nix.

@Gabriella439
Copy link
Contributor Author

@raphaelr: Thank you! I can reproduce now, too. I think the reason I could not originally reproduce was that I was using nix path-info --derivation instead of nix-instantiate

Gabriella439 added a commit to Gabriella439/nixpkgs that referenced this pull request Dec 27, 2022
See the discussion starting here:

NixOS#206951 (comment)

The `darwin.builder` derivation had a gratuitous dependency
on the current Nixpkgs revision due to
`config.system.nixos.revision`.  Setting the revision explicitly
to null fixes this problem and prevents the derivation from being
rebuilt on every change to Nixpkgs.
@Gabriella439
Copy link
Contributor Author

This should fix the gratuitous rebuilds: #207902

github-actions bot pushed a commit that referenced this pull request Dec 27, 2022
See the discussion starting here:

#206951 (comment)

The `darwin.builder` derivation had a gratuitous dependency
on the current Nixpkgs revision due to
`config.system.nixos.revision`.  Setting the revision explicitly
to null fixes this problem and prevents the derivation from being
rebuilt on every change to Nixpkgs.

(cherry picked from commit 474198f)
@NiklasGollenstede
Copy link
Contributor

Just a few thoughts when I read the new manual entry:

  • The manual entry says that port 22 needs to be free on the host, which I think it true in the current implementation. But it should work to set virtualisation.forwardPorts.*.host.address to 127.0.0.42 (any address starting with 127. (or maybe even a single IPv6?)) and use that address in the builder spec. Any other address (including localhost and any external interfaces) would have port 22 available for other SSH things.

  • If qemu ist started with serial on the local terminal (which I am not sure if it is a good idea here) one might as well auto-login for debugging purposes (otherwise the TTY is kinda pointless): services.getty.autologinUser = "root".
    Also, with the TTY open, ctrl-a + x immediately kills the VM.

  • And since I read the term bikeshedding above: darwin.builder does not at all imply that it builds NixOS things, only that it is a builder that has something to do with darwin/MacOS

@Gabriella439
Copy link
Contributor Author

@NiklasGollenstede: The only reason it says port 22 is required is because I thought it was a limitation of Nix's remote builder functionality (rather than a limitation of the VM). Are you saying that there's a way for Nix to specify a non-default port for a remote builder in the builder spec?

I like the autologinUser idea, though, and created a pull request: #208772

@NiklasGollenstede
Copy link
Contributor

Are you saying that there's a way for Nix to specify a non-default port for a remote builder in the builder spec?

That would be nice, but no. I am pretty sure that such an option would be defined in either of these two files:
https://github.com/NixOS/nix/blob/master/src/libstore/ssh-store.cc
https://github.com/NixOS/nix/blob/master/src/libstore/legacy-ssh-store.cc
(I don't really understand why Nix doesn't allow some way of just supplying verbatim options to openssh (at least with nix-build, it is simply calling ssh, which needs to be on the PATH that nix-build get's called with). Maybe to not commit to using the openssh CLI?)

What I was saying is that currently (I assume this is the default when not supplying virtualisation.forwardPorts.*.host.address) qemu is binding (and thus blocking) port 22 on all IPv4 (and v6?) addresses and interfaces, while a single (v4 or v6) address on only the loopback is enough for a local nix command to connect to. Even without a firewall, others in your network will no longer be able to use your VM, and other services can listen on TCP port 22, as long as they specify an interfacee/address other than the one used by the VM (or (implicitly) a wildcard that includes it, like "", *, 0.0.0.0, etc, depending on the API).

@Gabriella439
Copy link
Contributor Author

@NiklasGollenstede: It turns out that an unprivileged qemu process attempting to bind a privileged port can bind to an host address of 0.0.0.0 but not 127.0.0.1, which is pretty cursed:

https://developer.apple.com/forums/thread/674179

The good news is that you don't actually need to open up the firewall, so I put up a PR to document that: #208792

@NiklasGollenstede
Copy link
Contributor

macOS: You can have all the addresses.
developer: That's cool. I'd like to have that one!
macOS: no.

There may be an implementation reason, but looking from the outside, that's incredibly weird behavior.

If macOS has some mechanism to prompt for firewall exceptions, (and it is now documented to deny that), then at least the security aspect of not binding all addresses is largely a mute point (assuming that on macOS users don't usually (or can't) disable the firewall).
While the port does not need to be "open" in the firewall, it still needs to be "free", as in "not bound to on and address by any process" (e.g. you can't have sshd running on its default port).

@roberth
Copy link
Member

roberth commented Jan 2, 2023

I thought it was a limitation of Nix's remote builder functionality (rather than a limitation of the VM)

I've configured Port in the ssh_config before. You may want to use the global client config, /etc/ssh/ssh_config, for this, or you may have to configure it for both root (the nix-daemon) and the user.

@NiklasGollenstede
Copy link
Contributor

I've configured Port in the ssh_config before.

Right. That should work. (When I was researching this previously, modifying config files was not an option (and it is quite contrary to Nix's philosophy that it is required).)

To do this without also modifying /etc/hosts (or whatever it takes to locally define a host name on macOS), you can define the builder's address as 127.x.y.z (where x.y.z is anything other than 0.0.1 and then use that address in the ssh_config.

Gabriella439 added a commit to Gabriella439/nixpkgs that referenced this pull request Jan 2, 2023
… as suggested by @NiklasGollenstede in:

NixOS#206951 (comment)

This simplifies the user experience for logging into and
debugging the machine and also simplifies the instructions for
shutting down the machine gracefully.
Gabriella439 added a commit that referenced this pull request Jan 4, 2023
… as suggested by @NiklasGollenstede in:

#206951 (comment)

This simplifies the user experience for logging into and
debugging the machine and also simplifies the instructions for
shutting down the machine gracefully.
sloane-shark pushed a commit to sloane-shark/nixpkgs that referenced this pull request Feb 17, 2023
… as suggested by @NiklasGollenstede in:

NixOS#206951 (comment)

This simplifies the user experience for logging into and
debugging the machine and also simplifies the instructions for
shutting down the machine gracefully.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allowing NixOS VM's to be run on macOS
8 participants