Add network library #299409

djacu · 2024-03-27T04:32:57Z

Description of changes

See #258250 for discussion about why this would be beneficial to nixpkgs.

This PR was created at the request of @infinisil to be smaller and include a design document.
The design document is fairly terse. Any comments or feedback are welcome to help improve it.

Considerations for this PR and differences from the previous PR.

Going forward, the number of functions exposed will be significantly less. Rather than exporting many functions, a few functions (currently one) that create IPv4Address attrsets will be exposed.
The attrset will have (lazy) attributes that accomplish the same functions as in the previous PR.
There are now tests and a design document similar to the fileset and path libraries.
Currently only IPv4 is implement to keep the PR small. IPv6 will be implemented in the future.
The number of attributes in the IPv4Address attrset is limited to keep the PR small. More attributes will be implemented in the future.

Things done

Tested, as applicable:
- NixOS test(s) (look inside nixos/tests)
- and/or package tests
- or, for functions and "core" functionality, tests in lib/tests or pkgs/test
Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Added the first exposed function that creates IPv4Address structures from CIDR strings. Added basic tests to check the exposed function.

…tion.

tests.

…ests.

…om a CIDR and added more tests.

… a subnet mask and added tests.

…from an address and prefix length.

…ddress attrset. Specifying that it takes a string allows for other ingestion methods in the future.

aanderse · 2024-04-01T17:12:26Z

@djacu in your previous PR you included a subnetMaskToBitMask function which was very useful to me - any chance of including that functionality here?

for example...

let
  droplet = ...; # JSON information from my vps provider
in
{
  networking.interfaces.eth0.ipv4 = {
    addresses = [
      {
        address = droplet.public.ipv4.ip_address;
        prefixLength = with net.ipv4; subnetMaskToBitMask (cidrToIpAddress droplet.public.ipv4.netmask);
      }
    ];
  };
}

djacu · 2024-04-01T18:04:41Z

@djacu in your previous PR you included a subnetMaskToBitMask function which was very useful to me - any chance of including that functionality here?

for example...

let
  droplet = ...; # JSON information from my vps provider
in
{
  networking.interfaces.eth0.ipv4 = {
    addresses = [
      {
        address = droplet.public.ipv4.ip_address;
        prefixLength = with net.ipv4; subnetMaskToBitMask (cidrToIpAddress droplet.public.ipv4.netmask);
      }
    ];
  };
}

@aanderse yes! The plan is incrementally add many of the features in the previous PR and some new ones I found while looking at other implementations.

roberth

A few suggestions. I haven't reviewed the IPv4 logic itself too thoroughly, and I think it'd be great if someone with more day to day exposure to these concepts could review that.

lib/network/internal.nix

lib/tests/test-with-nix.nix

lib/network/internal.nix

lib/network/tests.sh

lib/default.nix

infinisil

This is going in the right direction!

lib/network/README.md

lib/network/default.nix

lib/network/internal.nix

infinisil · 2024-04-09T16:45:01Z

lib/network/internal.nix

+    */
+    _encode =
+      num:
+      if num < 0 then


With the _parseCIDR approach detailed above, this function won't have to check bounds anymore, since you can internally guarantee it won't be called with invalid arguments.

For the time being this is true, but I have plans to add things in the future where having the bounds check will be necessary. Currently, the only part that requires it is subnet mask and I've already removed that per your other suggestion. So I'll remove this for now and we can hash it out in a future PR when subnet mask comes back in.

Sound good?

lib/network/internal.nix

infinisil · 2024-04-09T16:52:38Z

lib/network/internal.nix

+      _parse "192.168.0.1/24"
+      => {
+        address = "192.168.0.1";
+        cidr = "192.168.0.1/24";


Suggested change

cidr = "192.168.0.1/24";

Doesn't seem to be in the result.

You sure? I just created a test that fails to check the _parse function and it definitely is in there.

:!./tests.sh test case at ./tests.sh:138 failed: internal.ipv4._parse "192.168.0.1" "24" should have evaluated to "192.168.0.1": "192.168.0.1" but it evaluated to { address = "192.168.0.1"; cidr = "192.168.0.1/24"; prefixLength = "24"; subnetMask = "255.255.255.0"; } shell returned 1

infinisil · 2024-04-09T16:55:46Z

lib/network/internal.nix

+      ```nix
+      _parse "192.168.0.1/24"
+      => {
+        address = "192.168.0.1";


Shouldn't this return something like [ 192 168 0 1 ], such that other functions can more easily work with the data instead of having to re-parse it?

After thinking about it, that is a good idea. But I think this should be stored in an internal variable like _address with the string representation stored in address. The reason being that the general use case for the IP address is in it's string representation. But yeah having the internal representation as a list of ints is probably the best state to store it internally.

infinisil · 2024-04-09T16:56:00Z

lib/network/internal.nix

+      => {
+        address = "192.168.0.1";
+        cidr = "192.168.0.1/24";
+        prefixLength = "24";


Suggested change

prefixLength = "24";

prefixLength = 24;

Similarly

Update upper bound check for ipv4 range Co-authored-by: Silvan Mosberger <github@infinisil.com>

…the default.nix file.

… which has been removed for now.

djacu · 2024-04-12T06:58:50Z

So this is the line in default.nix that is causing the manuals to fail to build. If I remove it, it builds.

  - [`lib.network.ipv4.fromCidrString`](#function-library-lib.network.ipv4.fromCidrString):

I think it is because it the fromCidrString doc comment is not rendering. But I have no idea why... 🙃

djacu · 2024-04-13T04:23:26Z

So this is the line in default.nix that is causing the manuals to fail to build. If I remove it, it builds.
  - [`lib.network.ipv4.fromCidrString`](#function-library-lib.network.ipv4.fromCidrString):
I think it is because it the fromCidrString doc comment is not rendering. But I have no idea why... 🙃

I may have found out why -> https://matrix.to/#/!avYyleMexqjFHoqrME:nixos.org/$L7FoTeNQhJOOeZRdrv-kWY4IEe2_FBALlI7f8BX2qOw?via=nixos.org&via=matrix.org&via=nixos.dev

It seems at the moment, function references can't be nested.

infinisil · 2024-04-13T13:41:00Z

They can, but you need to use ipv4.fromCidrString = for now. This is also how lib.path.subpath.isValid works:

nixpkgs/lib/path/default.nix

Line 447 in f8e37ff

subpath.isValid =

djacu · 2024-04-29T16:27:33Z

@infinisil I've responded to you comments/suggestions. Not everything is resolved as I need more feedback from you.

Can you take a look when you have a chance?

nixos-discourse · 2024-05-02T00:40:29Z

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

https://discourse.nixos.org/t/announcing-our-google-summer-of-code-participants/44618/3

RaitoBezarius

This doesn't parse IPv4, but a subset of IPv4: 192.1 is not valid with this parser, though, it's a valid IP address.

djacu · 2024-05-02T03:14:02Z

This doesn't parse IPv4, but a subset of IPv4: 192.1 is not valid with this parser, though, it's a valid IP address.

Please provide sources next time. This is a deep rabbit hole of of RFCs and specifications.

RFC 791 specifies "Addresses are fixed length of four octets (32 bits)." in Section 2.3. RFC 6943 Section 3.1.1 also defines an address this way. It also references RFC3493 Section 6.1 and several functions from IEEE 1003.1 for alternative implementations including octal and hexadecimal representations as well as representations with less than 4 parts. However, it categorizes these short hand forms as "loose" forms and mentions their issues with ambiguity and security. This ambiguity would have to be resolved before an implementation could be submitted.

Additionally, I referenced Python, Rust, and Haskell as references. As far as I can tell, none of them implement this short hand notation and only parse addresses where all 4 octets are present.
Python's implementation
Rust's implementation
Haskell's implementation

Finally, this is a MVP version of #258250 which was reduced in size and scope at @infinisil's request (and came with some design improvements). Adding short hand IP address parsing should not be included at this time to keep the scope small. It could be added as a feature in the future after a design has been submitted to mitigate the ambiguities. However, since 3 other major languages don't implement this, I don't see why we should either.

aanderse · 2024-05-03T18:30:43Z

hey @djacu 👋

thank you for all of the work you have put into this! it looks like a fair bit of effort on your part to address all the helpful comments from @infinisil and @roberth! ❤️

i also appreciate the research you did in order to respond to @RaitoBezarius! it seems like you covered the concern quite well and have demonstrated established norms, all of which make sense to me from my years of experience

is everyone happy with merging this? i think it would be very helpful to have this PR merged so it doesn't cause any problems for the GSoC student who intends to continue with the great work you've started here ... i would hate to see a GSoC student or Dan do a bunch of redundant work

@infinisil - all good? 🙂

Janik-Haag · 2024-05-03T20:51:23Z

hey @aanderse I think there is still a bunch of improvements that should be made in this pr.
@djacu and me already talked in DMs and he is happy passing the task on to @woojiq (the GSoC contributor).

You can see a rough timeline here: https://summerofcode.withgoogle.com/organizations/nixos-foundation/programs/2024/timeline

aanderse · 2024-05-03T22:13:48Z

@Janik-Haag awesome to hear! thanks for the update, i'm looking forward to tracking this

RaitoBezarius · 2024-05-04T08:31:58Z

This doesn't parse IPv4, but a subset of IPv4: 192.1 is not valid with this parser, though, it's a valid IP address.

Please provide sources next time. This is a deep rabbit hole of of RFCs and specifications.

Apologies, I lack of time these last days.

RFC 791 specifies "Addresses are fixed length of four octets (32 bits)." in Section 2.3. RFC 6943 Section 3.1.1 also defines an address this way. It also references RFC3493 Section 6.1 and several functions from IEEE 1003.1 for alternative implementations including octal and hexadecimal representations as well as representations with less than 4 parts. However, it categorizes these short hand forms as "loose" forms and mentions their issues with ambiguity and security. This ambiguity would have to be resolved before an implementation could be submitted.

Additionally, I referenced Python, Rust, and Haskell as references. As far as I can tell, none of them implement this short hand notation and only parse addresses where all 4 octets are present. Python's implementation Rust's implementation Haskell's implementation

Finally, this is a MVP version of #258250 which was reduced in size and scope at @infinisil's request (and came with some design improvements). Adding short hand IP address parsing should not be included at this time to keep the scope small. It could be added as a feature in the future after a design has been submitted to mitigate the ambiguities. However, since 3 other major languages don't implement this, I don't see why we should either.

I do not think that 3 major languages not implementing this is ground enough for rejection, my rationale is that any parser we should have in lib should always be liberal in the inputs and strict in the output. A program that is known to handle this is ping or mtr for example. I do not think I would focus on major implementations but on classical networking tools as this is a network library.

We could adopt ping parser for example.

I didn't understand this was an MVP, I will retract my feedback then.

thought it was a complete library; it's only a MVP

Pamplemousse · 2024-05-13T17:39:44Z

lib/network/default.nix

+  - `prefixLength`: A prefix length.
+  - `subnetMask`: A subnet mask.
+
+  - [`lib.network.ipv4.fromCidrString`](#function-library-lib.network.ipv4.fromCidrString):


I am not really familiar with the documentation syntax, but it looks like L19-21 content redundant with documentation of function below... Does it really need to be duplicated?

Does this comment even get rendered?

I think fromCidrString was not meant to be part of the above list.

roberth · 2024-05-13T20:02:44Z

lib/network/default.nix

+  - `prefixLength`: A prefix length.
+  - `subnetMask`: A subnet mask.
+
+  - [`lib.network.ipv4.fromCidrString`](#function-library-lib.network.ipv4.fromCidrString):


Does this comment even get rendered?

roberth · 2024-05-13T20:03:50Z

lib/network/default.nix

+  - `prefixLength`: A prefix length.
+  - `subnetMask`: A subnet mask.
+
+  - [`lib.network.ipv4.fromCidrString`](#function-library-lib.network.ipv4.fromCidrString):


I think fromCidrString was not meant to be part of the above list.

roberth · 2024-05-13T20:06:48Z

lib/network/internal.nix

+        prefixLength
+      ];
+      inherit address prefixLength;
+    };


Consider setting _type = "ipv4Address"; for runtime type checking. This can improve error messages when addresses end up in wrong places; useful even if this library doesn't check _type.

This also reminds me that a type in the module system would be nice (for later perhaps).
It could parse both stringly definitions and pass through _type-ed definitions.

djacu added 13 commits March 25, 2024 20:56

Started the design document for the network library.

ad9df7e

Added the first exposed function that creates IPv4Address structures from CIDR strings. Added basic tests to check the exposed function.

Updated network library using official nixfmt.

79dc1bc

Added network tests into the library tests.

33747ae

Swapped the subnet mask function. Added tests for the new encode func…

5730535

…tion.

Added more tests

4cb8f2d

Document and test the pow function.

5a963ec

Added more logic for the ipv4 encode function. Added docs and more

51de0d0

tests.

Documented the function that verifies valid prefix length and added t…

ee3d0f8

…ests.

Reworked the logic of the function that verifies the prefix length fr…

de10daa

…om a CIDR and added more tests.

Added documentation for the function that converts a prefix length to…

5898ff8

… a subnet mask and added tests.

Added documentation for the function that makes IPv4Address attrsets …

f5ab94e

…from an address and prefix length.

Added more to the readme.

52eef00

Changed function name that ingests a cidr and converts it to an IPv4A…

0e3651b

…ddress attrset. Specifying that it takes a string allows for other ingestion methods in the future.

djacu requested a review from infinisil as a code owner March 27, 2024 04:32

github-actions bot added the 6.topic: lib The Nixpkgs function library label Mar 27, 2024

djacu mentioned this pull request Mar 27, 2024

Add network library with IPv4 conversion functions. #258250

Closed

12 tasks

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 27, 2024

roberth reviewed Apr 1, 2024

View reviewed changes

lib/network/internal.nix Outdated Show resolved Hide resolved

lib/tests/test-with-nix.nix Outdated Show resolved Hide resolved

lib/network/internal.nix Outdated Show resolved Hide resolved

lib/network/tests.sh Outdated Show resolved Hide resolved

lib/default.nix Show resolved Hide resolved

djacu added 3 commits April 2, 2024 09:00

Revert test-with-nix.nix back to original state.

ab3707a

Added network tests back in test-with-nix.nix.

e1f9b01

Added entry in doc/default.nix.

7e55177

github-actions bot added the 8.has: documentation label Apr 2, 2024

djacu added 2 commits April 2, 2024 10:58

Changed _makeIPv4 to _parse to be more descriptive and consistent.

5ea3344

Added a function to verify ipv4 addresses and more tests.

6116326

infinisil requested changes Apr 9, 2024

View reviewed changes

djacu and others added 2 commits April 11, 2024 14:54

Switch to foldl' per review recommendation.

3a09ebe

Update lib/network/internal.nix

18e64c7

Update upper bound check for ipv4 range Co-authored-by: Silvan Mosberger <github@infinisil.com>

djacu added 7 commits April 11, 2024 17:01

Move the documentation of the IPv4Address attrset from the readme to …

24c77da

…the default.nix file.

Add doc comment for ipv4.fromCidrString

f28ca5a

Remove subnet mask from the library for the initial PR.

b7c93e1

Removing _encode internal function as it was only used by subnet mask…

a8ab014

… which has been removed for now.

Clean up tests to limit the references to internal functions.

61b60a3

Fix working in tests.

ff6f0e2

inherit cleanup since subnet mask was removed.

c27b3d6

djacu requested a review from roberth April 12, 2024 06:25

Fix network api so the manual renders.

b871844

RaitoBezarius previously requested changes May 2, 2024

View reviewed changes

Pamplemousse reviewed May 13, 2024

View reviewed changes

roberth reviewed May 13, 2024

View reviewed changes

woojiq mentioned this pull request Jun 10, 2024

lib.network: add ipv6 parser #318712

Merged

3 tasks

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network library #299409

Add network library #299409

djacu commented Mar 27, 2024

aanderse commented Apr 1, 2024

djacu commented Apr 1, 2024

roberth left a comment

infinisil left a comment

infinisil Apr 9, 2024

djacu Apr 12, 2024

infinisil Apr 9, 2024

djacu Apr 12, 2024

infinisil Apr 9, 2024

djacu Apr 12, 2024

infinisil Apr 9, 2024

djacu commented Apr 12, 2024

djacu commented Apr 13, 2024

infinisil commented Apr 13, 2024

djacu commented Apr 29, 2024

nixos-discourse commented May 2, 2024

RaitoBezarius left a comment

djacu commented May 2, 2024

aanderse commented May 3, 2024

Janik-Haag commented May 3, 2024

aanderse commented May 3, 2024

RaitoBezarius commented May 4, 2024

Pamplemousse May 13, 2024

roberth May 13, 2024

roberth May 13, 2024

roberth May 13, 2024

roberth May 13, 2024

roberth May 13, 2024

roberth May 13, 2024

Add network library #299409

Are you sure you want to change the base?

Add network library #299409

Conversation

djacu commented Mar 27, 2024

Description of changes

Things done

aanderse commented Apr 1, 2024

djacu commented Apr 1, 2024

roberth left a comment

Choose a reason for hiding this comment

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djacu commented Apr 12, 2024

djacu commented Apr 13, 2024

infinisil commented Apr 13, 2024

djacu commented Apr 29, 2024

nixos-discourse commented May 2, 2024

RaitoBezarius left a comment

Choose a reason for hiding this comment

djacu commented May 2, 2024

aanderse commented May 3, 2024

Janik-Haag commented May 3, 2024

aanderse commented May 3, 2024

RaitoBezarius commented May 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment