Skip to content

Add network library with IPv4 conversion functions.#258250

Closed
djacu wants to merge 1 commit intoNixOS:masterfrom
djacu:add-networking-lib
Closed

Add network library with IPv4 conversion functions.#258250
djacu wants to merge 1 commit intoNixOS:masterfrom
djacu:add-networking-lib

Conversation

@djacu
Copy link
Copy Markdown
Member

@djacu djacu commented Sep 30, 2023 •
edited
Loading

Description of changes

Adds a network library to lib with functions for IPv4 conversions. Many core and supplementary modules can benefit from simple Nix functions that do IPv4 cidr/address/mask conversions. Examples include:

Example of how configuration can be improved using the new network library. This helps minimize the chance for typographical errors.

# before
services.dhcpd4.extraConfig = ''
  option subnet-mask 255.255.255.0;
  option broadcast-address 192.168.1.255;
  option routers 192.168.1.5;
  subnet 192.168.1.0 netmask 255.255.255.0 {
    range 192.168.1.100 192.168.1.200;
  }
''
# after
services.dhcpd4.extraConfig = let
  props = lib.networking.ipv4.getNetworkProperties "192.168.1.5/24";
in
  ''
    option subnet-mask ${props.subnetMask};
    option broadcast-address ${props.broadcast};
    option routers ${props.ipAddress};
    subnet ${props.networkId} netmask ${props.subnetMask} {
      range ${props.firstUsableIp} ${props.lastUsableIp};
    }
  ''

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.11 Release Notes (or backporting 23.05 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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Sep 30, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 30, 2023
@ajs124
Copy link
Copy Markdown
Member

ajs124 commented Sep 30, 2023

what about ipv6?

@djacu
Copy link
Copy Markdown
Member Author

djacu commented Sep 30, 2023

what about ipv6?

  • This PR is already quite large.
  • It is complete for v4 (at least as much as I could consider).
  • I didn't want to write for both if this was going to be rejected.

In short; next PR. Or someone else can add it if they feel inclined. :)

infinisil
infinisil requested changes Oct 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Nice, this would be very handy!

I think it's a bit too big all together though, could you split this into smaller PRs? That would make incremental reviews and merges much easier.

Ideally we'd also have a design document like we have for the path library and the fileset library. This would include arguments for why the library is design the way it is, such as why these specific functios were chosen, that alternatives would be worse, etc.

In particular I feel like the amount of conversion functions is a bit too much, could this be improved?

@nixos-discourse
Copy link
Copy Markdown

nixos-discourse commented Oct 10, 2023

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/manipulate-ip-addresses-in-nix-lang/33363/5

@djacu
Copy link
Copy Markdown
Member Author

djacu commented Oct 17, 2023

Nice, this would be very handy!

I think it's a bit too big all together though, could you split this into smaller PRs? That would make incremental reviews and merges much easier.

Ideally we'd also have a design document like we have for the path library and the fileset library. This would include arguments for why the library is design the way it is, such as why these specific functios were chosen, that alternatives would be worse, etc.

In particular I feel like the amount of conversion functions is a bit too much, could this be improved?

Sure I'm happy to break it into multiple PRs and add a design document. How many chunks should I split it into or how small should I make the PRs?

I worked with @kylerisse on designing the library. All the functions that go into getNetworkProperties are useful and necessary for people in networking (so all the cidrTo* functions). So everything but ipAndBitMaskToCidr and ipAndSubnetMaskToCidr are necesarry; these functions let you go in reverse which might be helpful to others.

@infinisil
Copy link
Copy Markdown
Member

infinisil commented Nov 6, 2023

How many chunks should I split it into or how small should I make the PRs?

The smallest chunks possible such that each still has a good use case (and explain that use case in the PR).

astro
astro reviewed Nov 23, 2023
View reviewed changes
lib/network.nix
cidrToBitMask "192.168.70.9/15"
=> 15
*/
cidrToBitMask = cidr: let
Copy link
Copy Markdown
Contributor

@astro astro Nov 23, 2023

Choose a reason for hiding this comment

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

How certain are you about the term bitmask? I would expect "11111111111111110000000000000000".

In IPv6 lingo that number after the slash is named prefix length.

Copy link
Copy Markdown
Member Author

@djacu djacu Nov 27, 2023

Choose a reason for hiding this comment

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

Not very. Prefix length is better. I think it's same in both ipv4 and ipv6, no?

@yu-re-ka
Copy link
Copy Markdown
Contributor

yu-re-ka commented Dec 11, 2023 •
edited
Loading

A very hacky ipv4 and ipv6 library can be found here:
https://cyberchaos.dev/cyberchaoscreatures/nixlib/-/blob/main/lib/ipUtil/default.nix?ref_type=heads

@aanderse
Copy link
Copy Markdown
Member

aanderse commented Mar 7, 2024

@djacu any chance you are still working on this? ❤️

@djacu
Copy link
Copy Markdown
Member Author

djacu commented Mar 16, 2024

@djacu any chance you are still working on this? ❤️

Hey sorry

I got swamped helping plan scale and nixcon na. I started working on a design document. I was hoping to get back to it next week.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@djacu djacu mentioned this pull request Mar 27, 2024
Closed
2 tasks
@djacu
Copy link
Copy Markdown
Member Author

djacu commented Mar 27, 2024

At the request of @infinisil, I have created a new (smaller) PR with a design document.
Additionally, I think the new implementation is better.
See -> #299409

@djacu djacu closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@infinisil infinisil infinisil requested changes

@edolstra edolstra Awaiting requested review from edolstra

+1 more reviewer

@astro astro astro left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@djacu @ajs124 @nixos-discourse @infinisil @yu-re-ka @aanderse @astro @wegank