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/pixiecore: init #83406

Merged
merged 2 commits into from Apr 2, 2020
Merged

nixos/pixiecore: init #83406

merged 2 commits into from Apr 2, 2020

Conversation

bbigras
Copy link
Contributor

@bbigras bbigras commented Mar 26, 2020

Motivation for this change

This should only be merged after #83373 . You can use that PR to review the package part. This is one is for the module.

This is a rebase of #62113 (the module part).

I also added support for the "api" mode and added openFirewall and extraArguments options.

I tried to address all the reviews from the old abandoned PR.

The module was requested in #49740

@Lassulus @aanderse @Ekleog

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.

Copy link
Contributor

@danderson danderson left a comment

Choose a reason for hiding this comment

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

PR LGTM. Always happy to see Pixiecore in the wild :)

pkgs/tools/networking/pixiecore/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
@bbigras bbigras force-pushed the mod_pixiecore branch 2 times, most recently from bcdf812 to be01b2b Compare March 26, 2020 04:51
@bbigras
Copy link
Contributor Author

bbigras commented Mar 26, 2020

Thank you very much @danderson and @samueldr . I made all the changes.

@ofborg ofborg bot requested a review from danderson March 26, 2020 05:17
@Mic92
Copy link
Member

Mic92 commented Mar 26, 2020

It would be also great if you could update the wiki after that.

@Mic92 Mic92 requested a review from Lassulus March 26, 2020 08:28
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
@bbigras
Copy link
Contributor Author

bbigras commented Mar 26, 2020

Thanks for the review @Profpatsch . Is it OK like that or should I use <literal> around port and statusPort?

EDIT: I also made initrd and cmdLine optional since you can boot netboot.xyz with only:

kernel = "https://boot.netboot.xyz/ipxe/netboot.xyz.lkrn";

https://github.com/NixOS/nixpkgs/blob/3ae89bc8a19ac92f9b7e772c2da1cd7d76f285d2/nixos/modules/services/networking/pixiecore.nix#L15-L21

It would be also great if you could update the wiki after that.

Yep. I also have another PR in the work that might be nice.

nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pixiecore.nix Outdated Show resolved Hide resolved
@bbigras
Copy link
Contributor Author

bbigras commented Mar 26, 2020

I made the changes. Thanks.

I'll squash the commits at the end. Or I think you guys can do it automatically while merging.

@bbigras
Copy link
Contributor Author

bbigras commented Mar 26, 2020

What's that grahamcofborg-eval test failing?

@samueldr
Copy link
Member

The "Details" link has the trace from Nix.

image

@bbigras
Copy link
Contributor Author

bbigras commented Mar 27, 2020

Thanks. I made the change and squashed the commits.

@bbigras
Copy link
Contributor Author

bbigras commented Mar 28, 2020

I made the changes. Thanks.

Should cap_net_raw be added only when dhcpNoBind is used?

@Mic92
Copy link
Member

Mic92 commented Mar 28, 2020

I made the changes. Thanks.

Should cap_net_raw be added only when dhcpNoBind is used?

It is not needed otherwise. So it sounds like a good idea.

@Lassulus
Copy link
Member

are both low low ports (67 and 69) used by dhcp?

@danderson
Copy link
Contributor

Port 69 is TFTP. Pixiecore does a fairly elaborate chainloading operation (described in https://github.com/danderson/netboot/blob/master/pixiecore/README.booting.md) to ensure that it works with the widest possible variety of weird and broken firmwares.

TL;DR: a PXE-booting client hits port 67 for DHCP, then port 4011 (BINL, an MS proprietary fork of PXE but that's a de-facto standard), then port 69 (TFTP to download iPXE with HTTP support), then the HTTP port (to finally download boot instructions, kernel+initrd, and finish the netboot).

Co-authored-by: raunovv <rauno@oyenetwork.com>
Co-Authored-By: Jörg Thalheim <Mic92@users.noreply.github.com>
@bbigras
Copy link
Contributor Author

bbigras commented Mar 30, 2020

I made the change.

Is it OK like this?

AmbientCapabilities = let
capacilities = [ "cap_net_bind_service" ] ++ optional cfg.dhcpNoBind "cap_net_raw";
in concatStringsSep " " capacilities;
ExecStart =

Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

LGTM from my side, if @Mic92 is okay with it.

@Mic92 Mic92 merged commit 5448216 into NixOS:master Apr 2, 2020
@bbigras bbigras deleted the mod_pixiecore branch April 2, 2020 13:31
@bbigras
Copy link
Contributor Author

bbigras commented Apr 2, 2020

Thanks everyone! ❤️

Can someone close #62113 and #49740 please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants