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.

Sign up for GitHub

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

Jump to bottom

nixos/pppd: init #71105

Merged
merged 3 commits into from Oct 16, 2019
Merged

nixos/pppd: init #71105

merged 3 commits into from Oct 16, 2019

Conversation

danderson
Copy link
Contributor

@danderson danderson commented Oct 14, 2019 •
edited

Motivation for this change

pppd is used to control a lot of home internet connections. I want to use NixOS as my home router, and I need pppd to bring up my FTTH internet connection. It will also be useful to people using DSL (PPPoE or PPPoA) and classic dial-up internet.

pppd is already available in nix as the ppp derivation. It's used by other nixos modules like pptpd, but there's no module for running PPP sessions directly. This module fixes that.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

(Other tests)

  • Configured and ran a pppd service, and successfully got internet access using this module.
Notify maintainers

cc @falsifian (because you're the maintainer of the ppp package, which this module service-ifies)

@danderson
Copy link
Contributor Author

One style question I didn't know how to answer: what should the commit message be? CONTRIBUTING.md says that it should be init at <version>, but this commit only adds a nixos module, which doesn't have a version.

@alexarice
Copy link
Contributor

The commit title is fine

alexarice
alexarice reviewed Oct 14, 2019
View reviewed changes
maintainers/maintainer-list.nix Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix
services.pppd = {
package = mkOption {
default = pkgs.ppp;
defaultText = "pkgs.ppp";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary, I thought the document system could cope with this

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 don't know, I was copying other nixos modules that do this with their pkg reference. I'm happy to remove it and see what happens when the docs go live?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can't hurt so leave it

Copy link
Contributor

Choose a reason for hiding this comment

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

There is probably a way to locally build the docs but I'm not quite sure how if you wanted to check

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'll look around tomorrow for a way to build the options search engine locally and check for this.

nixos/modules/services/networking/pppd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/pppd.nix Show resolved Hide resolved
alexarice
alexarice suggested changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Sorry I didn't notice this earlier. You haven't added you're module to module-list.nix so it won't be available.

@danderson
Copy link
Contributor Author

Doh! I've been testing on my router with an import directly in configuration.nix, that's why. Added to module-list.nix. Thanks for catching that!

@flokli
Copy link
Contributor

flokli commented Oct 14, 2019

@danderson can you also add a small nixos test with 2 vms ping each other through a ppp(d) connection?

@danderson
Copy link
Contributor Author

Sure! I've never set up a pppoe+pppd server, so it might take me a bit, but I'm working on it.

@danderson
Copy link
Contributor Author

I've added a test as requested. One VM runs a PPPoE server, and the other uses this pppd module to connect as a client. Test verifies bidirectional ping reachability through the PPP link. PTAL?

@flokli
Copy link
Contributor

flokli commented Oct 15, 2019

@GrahamcOfBorg build nixosTests.pppd

@danderson
nixos/pppd: init
997a6f6
@danderson
nixos/tests/pppd: init
ae02b3d 
This test creates a PPPoE link between two machines, and verifies
that the machines can ping each other.
@flokli
Copy link
Contributor

flokli commented Oct 15, 2019

@GrahamcOfBorg test pppd

@danderson
Copy link
Contributor Author

Anything else we need before merging this? The PR history says I requested a review from @infinisil . I don't think I did? But I can't edit the reviewer list, and I welcome more eyes on this if that's the next step :)

@flokli
Copy link
Contributor

flokli commented Oct 16, 2019

@danderson Infinisil is marked as a codeowner for the module system.

I wanted to await the nixos test results from CI, and forgot about it.

Thanks for the ping!

@flokli flokli merged commit fff04a0 into NixOS:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli approved these changes

@alexarice alexarice alexarice approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danderson @alexarice @flokli