lib.network: add ipv6 parser by woojiq · Pull Request #318712 · NixOS/nixpkgs

woojiq · 2024-06-10T06:34:18Z

Description of changes

Add a library function to parse and validate an IPv6 address from a
string. It can parse the first two versions of an IPv6 address according
to https://datatracker.ietf.org/doc/html/rfc4291#section-2.2. The third
form "x:x:x:x:x:x.d.d.d.d" is not yet implemented.
Add shell script network.sh to test IPv6 parser functionality.
Copy fromHexString function from the lib: add fromHexString #266705

Mentor: @Janik-Haag

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.
Formatted using nixfmt-rfc-style

Add a 👍 reaction to pull requests you find important.

lib/network/default.nix

mweinelt · 2024-06-17T14:40:55Z

lib/network/default.nix

Suggested change

) "_parseNormalized expected to recieve normalized IPv6 address";

) "_parseNormalized expects a normalized IPv6 address";

But what does "normalized" in this case even mean? I don't think the function name matches its purpose well.

lib/network/default.nix

.gitignore

lib/trivial.nix

lib/tests/network.sh

lib/network/default.nix

Janik-Haag · 2024-06-18T20:32:24Z

To address your questions:

Do I need to somehow hide the internal functions more than just underscoring them?

Yes currently they would all get exposed, you have to put them into a let binding to make them truly internal. Take a look at https://github.com/NixOS/nixpkgs/blob/master/lib/fileset/default.nix#L103 as an example.

Are the doc comments in the correct format (already existing files have different style)?

Some of them are some of them aren't, please take a look at the example

Do I need to pull the ipv4 parser implementation to merge them together? #299409

no, we should keep them separate and preferably go for small prs, e.g. once the parser is fine merge this pr and then do another v6 one for cidr related functions.

woojiq · 2024-06-22T12:21:16Z

@Janik-Haag @mweinelt I refactored the code and moved internal functions to internal.nix. Can you review again please?

Janik-Haag

I think this is getting into shape. There are some smaller points left to do, and you should add a lot more comments explaining different parts, I think. This part of lib currently is very undocumented internally compared to others like lib.fileset or lib.generators.

lib/network/internal.nix

Janik-Haag · 2024-06-26T20:13:39Z

lib/network/internal.nix

Uh, I don't think we want to pass around the function name here, this will only get more confusing to debug because your output would gaslight the user, once other functions rely on the ${apiName} function internally.

Janik-Haag · 2024-06-26T20:15:20Z

lib/network/default.nix

Please remove the "api" part.

Janik-Haag · 2024-06-26T20:32:47Z

lib/network/internal.nix

This should be moved in the same scope as the emptyCount definition

Janik-Haag · 2024-06-26T21:20:54Z

lib/network/internal.nix

We should link to the corresponding RFC definitions, and maybe write a comment explaining them, since these are magic numbers to anyone unfamiliar with networking.

Janik-Haag · 2024-06-26T21:30:09Z

lib/network/internal.nix

We can reuse the zeros variable in line 68 in the concatMap function, because it's doing the same.

Janik-Haag · 2024-06-26T21:43:55Z

lib/network/internal.nix

I think we should be stricter with the user input and don't silently just set some prefix but instead fail saying that it's not a cidr address.
Also, usually the assumption I'm familiar with if you just don't specify a prefix is that you get a /128.

I agree, 128 sounds more logical if prefix length is omitted.

I think we should be stricter with the user input and don't silently just set some prefix but instead fail saying that it's not a cidr address.

I added one fromString function and user can use both default and cidr notation. I don't see a reason to create two separate API functions like fromString and fromCidrString, because this can be easily observed from the passed string - if it contains a /, then we try to parse it as cidr notation.

Co-authored-by: lucasew <lucas59356@gmail.com>

Add a library function to parse and validate an IPv6 address from a string. It can parse the first two versions of an IPv6 address according to https://datatracker.ietf.org/doc/html/rfc4291#section-2.2. The third form "x:x:x:x:x:x.d.d.d.d" is not yet implemented. Optionally parser can accept prefix length (128 is default). Add shell script network.sh to test IPv6 parser functionality.

Janik-Haag

@infinisil as a lib code owner, do you want to give this a review before we merge it?

lib/network/default.nix

nixos-discourse · 2024-07-11T18:40:08Z

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

https://discourse.nixos.org/t/showcase-nixpkgs-network-library/48897/1

This was cherry‐picked from <NixOS#266705> and merged as part of <NixOS#318712>, despite there being a blocking review on the former pointing out these kinds of issues. This documents some of the dodgy behaviour. It also can’t handle negative literals. It might be worth considering deprecating and dropping this, by inlining it into `lib.network.ipv6.fromString`, its only in‐tree user.

This was cherry‐picked from <#266705> and merged as part of <#318712>, despite there being a blocking review on the former pointing out these kinds of issues. This documents some of the dodgy behaviour. It also can’t handle negative literals. It might be worth considering deprecating and dropping this, by inlining it into `lib.network.ipv6.fromString`, its only in‐tree user. (cherry picked from commit 6673e05)

This was cherry‐picked from <NixOS/nixpkgs#266705> and merged as part of <NixOS/nixpkgs#318712>, despite there being a blocking review on the former pointing out these kinds of issues. This documents some of the dodgy behaviour. It also can’t handle negative literals. It might be worth considering deprecating and dropping this, by inlining it into `lib.network.ipv6.fromString`, its only in‐tree user. (cherry picked from commit 6673e05ad065b0f2388c94cc3b3445f9f7da780e)

This was cherry‐picked from <NixOS/nixpkgs#266705> and merged as part of <NixOS/nixpkgs#318712>, despite there being a blocking review on the former pointing out these kinds of issues. This documents some of the dodgy behaviour. It also can’t handle negative literals. It might be worth considering deprecating and dropping this, by inlining it into `lib.network.ipv6.fromString`, its only in‐tree user.

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

woojiq force-pushed the lib-network-ipv6-parser branch from d2bceb3 to 69089c5 Compare June 10, 2024 07:31

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 Jun 10, 2024

mweinelt reviewed Jun 17, 2024

View reviewed changes

Janik-Haag reviewed Jun 18, 2024

View reviewed changes

woojiq force-pushed the lib-network-ipv6-parser branch 2 times, most recently from 8ad8940 to 5f1b84c Compare June 22, 2024 11:53

woojiq marked this pull request as ready for review June 22, 2024 11:58

woojiq requested a review from infinisil as a code owner June 22, 2024 11:58

woojiq marked this pull request as draft June 22, 2024 12:00

woojiq force-pushed the lib-network-ipv6-parser branch from 5f1b84c to 05c8c7b Compare June 22, 2024 12:19

woojiq marked this pull request as ready for review June 22, 2024 12:20

woojiq marked this pull request as draft June 23, 2024 08:36

woojiq force-pushed the lib-network-ipv6-parser branch from 05c8c7b to 1ed35a9 Compare June 23, 2024 17:06

woojiq marked this pull request as ready for review June 23, 2024 17:08

woojiq mentioned this pull request Jun 23, 2024

lib.network: first/last and next addresses in a network #322004

Draft

3 tasks

Janik-Haag suggested changes Jun 26, 2024

View reviewed changes

woojiq force-pushed the lib-network-ipv6-parser branch from 1ed35a9 to 504a79b Compare June 28, 2024 17:40

woojiq and others added 2 commits June 28, 2024 20:53

gitignore: add .helix config directory

9cd5b7a

lib: add fromHexString

52cc703

Co-authored-by: lucasew <lucas59356@gmail.com>

woojiq force-pushed the lib-network-ipv6-parser branch from 504a79b to 9a035ac Compare June 28, 2024 17:54

woojiq force-pushed the lib-network-ipv6-parser branch from 9a035ac to d559eed Compare June 29, 2024 07:02

Janik-Haag reviewed Jul 7, 2024

View reviewed changes

lib/network/default.nix Show resolved Hide resolved

Janik-Haag approved these changes Jul 11, 2024

View reviewed changes

Janik-Haag merged commit c20399e into NixOS:master Jul 11, 2024

woojiq deleted the lib-network-ipv6-parser branch July 11, 2024 19:00

woojiq mentioned this pull request Jul 13, 2024

lib: add fromHexString #266705

Closed

13 tasks

emilazy mentioned this pull request Aug 14, 2025

lib: fix overflowing fromHexString tests and example #433710

Merged

13 tasks

	) "_parseNormalized expected to recieve normalized IPv6 address";
	) "_parseNormalized expects a normalized IPv6 address";

Uh oh!

Conversation

woojiq commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

Uh oh!

mweinelt Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Janik-Haag commented Jun 18, 2024

Uh oh!

woojiq commented Jun 22, 2024

Uh oh!

Janik-Haag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Janik-Haag Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

woojiq Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Janik-Haag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nixos-discourse commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

woojiq commented Jun 10, 2024 •

edited

Loading

woojiq Jun 28, 2024 •

edited

Loading