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/systemd-networkd: add NFTSet related options #332777

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

nixos/systemd-networkd: add NFTSet related options #332777

wants to merge 1 commit into from

Conversation

mvnetbiz
Copy link
Contributor

@mvnetbiz mvnetbiz commented Aug 6, 2024 •
edited
Loading

Description of changes

Allow setting NFTSet= option in systemd-netword network files. This option is for automatically adding the interface's address, prefix, or index to a named nftables set. It is valid under [Address] for static addresses, as well as the various sections for dynamic configuration (DHCP, etc.)

Added a NixOS test that configures a machine that uses NFTSet on a statically configured address section, as well as a DHCPv4 section, and checks that both nft sets are modified by testing nft rules that match on the sets.

Things done

Tested that it works on a system with nftables configured.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 6, 2024
@philiptaron philiptaron self-requested a review August 6, 2024 18:08
@philiptaron
Copy link
Contributor

philiptaron commented Aug 6, 2024 •
edited
Loading

Documentation is here: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html#NFTSet=source:family:table:set

NFT is https://netfilter.org/projects/nftables/index.html

@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Aug 6, 2024

Does this need any validation on the option or a nixos test? This seems similar to recent changes like #318604.

philiptaron
philiptaron reviewed Aug 6, 2024
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I'd love to see a NixOS test that exercised these options. I'd also love to see a validator or a structured Nix type that reflects the four possible fields of source, family, table, and set.

nixos/modules/system/boot/networkd.nix
@@ -737,6 +737,7 @@ let
"ManageTemporaryAddress"
"AddPrefixRoute"
"AutoJoin"
"NFTSet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Address documentation: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html#NFTSet=source:family:table:set

Do you think you need a validator? One seems possible given the documentation above.

nixos/modules/system/boot/networkd.nix
@@ -864,6 +865,7 @@ let
"FallbackLeaseLifetimeSec"
"Label"
"Use6RD"
"NFTSet"
Copy link
Contributor

Choose a reason for hiding this comment

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

DHCPv4 documentation: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html#NFTSet=

nixos/modules/system/boot/networkd.nix
@@ -913,6 +915,7 @@ let
"IAID"
"UseDelegatedPrefix"
"SendRelease"
"NFTSet"
Copy link
Contributor

Choose a reason for hiding this comment

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

DHCPv6 documentation: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html#NFTSet=1

nixos/modules/system/boot/networkd.nix
@@ -937,6 +940,7 @@ let
"Token"
"ManageTemporaryAddress"
"RouteMetric"
"NFTSet"
Copy link
Contributor

Choose a reason for hiding this comment

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

DHCPPrefixDelegation documentation: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html#NFTSet=2

nixos/modules/system/boot/networkd.nix
@@ -964,6 +968,7 @@ let
"UseRoutePrefix"
"Token"
"UsePREF64"
"NFTSet"
Copy link
Contributor

Choose a reason for hiding this comment

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

IPv6AcceptRA documentation: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html#NFTSet=3

@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Aug 6, 2024

I can add a validator.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Aug 6, 2024
@mvnetbiz mvnetbiz requested a review from a team as a code owner August 6, 2024 21:22
@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Aug 6, 2024 •
edited
Loading

I added a check to validate that all fields are present. It looks like one of the more complex checks for a string option. I'm not sure if I should keep it as several assertions like this, or if I should build a string so it's kept to a single trace line? Also not sure if systemd-lib.nix is the appropriate place for this since it will probably stay specific to the one module.

@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Aug 6, 2024

@philiptaron I am not sure how I could use a "structured Nix type" where the option type is already a unitOption.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 6, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 7, 2024
@philiptaron
Copy link
Contributor

@mvnetbiz I'm about to be away from a computer for a week-ish. Please feel free to enlist someone else to keep reviewing! I like the direction I see on my phone but can't sign off due to inability to test.

@mvnetbiz mvnetbiz requested a review from mweinelt August 7, 2024 18:58
@mvnetbiz
nixos/systemd-networkd: add NFTSet related options
c5cd3c2 
Includes NixOS test and validation on required fields.
r-vdp
r-vdp reviewed Aug 7, 2024
View reviewed changes
nixos/tests/systemd-networkd-nftset.nix
# added to another. The sets are used by one rule that blocks connections to
# the static address, and one rule that blocks connections to the DHCP address.
# It is tested that the expected connections succeed or fail from another host.
import ./make-test-python.nix (
Copy link
Contributor

Choose a reason for hiding this comment

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

The manual specifies a different way of defining tests, based on runTest instead of importing this.
See https://nixos.org/manual/nixos/stable/#sec-call-nixos-test-in-nixos

@mvnetbiz mvnetbiz marked this pull request as draft August 8, 2024 00:09
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r-vdp r-vdp r-vdp left review comments

@philiptaron philiptaron philiptaron left review comments

@mweinelt mweinelt Awaiting requested review from mweinelt

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mvnetbiz @philiptaron @r-vdp @wegank