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

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 bbigras:mod_pixiecore branch 2 times, most recently from bcdf812 to be01b2b Mar 26, 2020
@bbigras
Copy link
Contributor Author

@bbigras 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 Mar 26, 2020
@Mic92
Copy link
Contributor

@Mic92 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 Mar 26, 2020
@bbigras bbigras force-pushed the bbigras:mod_pixiecore branch from be01b2b to 3ae89bc Mar 26, 2020
@bbigras
Copy link
Contributor Author

@bbigras 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";

openFirewall = mkOption {
type = types.bool;
default = false;
description = ''
Open ports (67, 69 UDP and 4011, 'port', 'statusPort' TCP) in the firewall for Pixiecore.
'';
};

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 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 bbigras commented Mar 26, 2020

What's that grahamcofborg-eval test failing?

@samueldr
Copy link
Member

@samueldr samueldr commented Mar 27, 2020

The "Details" link has the trace from Nix.

image

@bbigras bbigras force-pushed the bbigras:mod_pixiecore branch from 0eeb784 to 72f83e5 Mar 27, 2020
@bbigras
Copy link
Contributor Author

@bbigras bbigras commented Mar 27, 2020

Thanks. I made the change and squashed the commits.

@bbigras bbigras force-pushed the bbigras:mod_pixiecore branch from 72f83e5 to 9e7d4a4 Mar 28, 2020
@bbigras bbigras force-pushed the bbigras:mod_pixiecore branch from 1f6ba20 to d125677 Mar 28, 2020
@bbigras
Copy link
Contributor Author

@bbigras bbigras commented Mar 28, 2020

I made the changes. Thanks.

Should cap_net_raw be added only when dhcpNoBind is used?

@Mic92
Copy link
Contributor

@Mic92 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
Contributor

@Lassulus Lassulus commented Mar 28, 2020

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

@danderson
Copy link
Contributor

@danderson danderson commented Mar 28, 2020

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 bbigras force-pushed the bbigras:mod_pixiecore branch from d125677 to b1ee662 Mar 30, 2020
@bbigras
Copy link
Contributor Author

@bbigras 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

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

@Mic92 Mic92 merged commit 5448216 into NixOS:master Apr 2, 2020
1 check was pending
1 check was pending
grahamcofborg-eval Checking original out paths
Details
@ofborg ofborg bot added the 10.rebuild-linux: 1 label Apr 2, 2020
@bbigras bbigras deleted the bbigras:mod_pixiecore branch Apr 2, 2020
@bbigras
Copy link
Contributor Author

@bbigras bbigras commented Apr 2, 2020

Thanks everyone! ❤️

Can someone close #62113 and #49740 please?

Tomahna added a commit to Tomahna/nixpkgs that referenced this pull request Apr 6, 2020
Co-authored-by: raunovv <rauno@oyenetwork.com>
Co-authored-by: Jörg Thalheim <joerg@thalheim.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.