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.

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

Migrate network-setup to nftables and improve it into a better state #4877

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kanpov
Copy link

@kanpov kanpov commented Oct 28, 2024

Changes

network-for-clones is not changed in this PR since it's a whole another can of worms I'd like to deal with later.
These changes are to network-setup:

  • Migrate to nftables

  • Improve the documentation itself to convert it from a rather basic getting started guide to something more usable in the real world:

  • Multiple guests section

  • Guest kernel configuration at kernel level section (ip kernel boot arg)

Reason

Fulfills #4874

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.

  • This functionality cannot be added in rust-vmm.

@kanpov kanpov changed the title Migrate networking documentation to nftables Migrate network-setup to nftables and improve it into a better state Oct 28, 2024
@kanpov kanpov marked this pull request as ready for review October 28, 2024 18:02
@kanpov
Copy link
Author

kanpov commented Oct 28, 2024

I'm welcome to critique of the new sections in the doc, since I think this is the real minimal amount of information needed for a Firecracker user to get a proper networking setup with support for multiple guests and other improvements. Back when I started working with Firecracker, I found this specific area of the docs to be especially frustrating to use (and led to me spending far, far too much of my time developing fcnet to make networking bearable):

  • Why shouldn't I use it if I just have another iface instead of eth0, why not just say you can replace the name?
  • Why no indications on how to support multiple guests?
  • Why no explanations of what the iptables or ip commands actually do for people unfamiliar with NAT, TUN/TAP and gateways?

With this, I've tried to address these.

@bchalios bchalios added the Status: WIP Indicates that an issue is currently being worked on or triaged label Oct 30, 2024
Copy link
Contributor

@pb8o pb8o left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I need to try the commands myself, but looks good so far.

I have a couple of asks: could we squash the commits together? I think it becomes easier to review. And we would like to keep the iptables commands for some time until nft becomes more spread.

Comment on lines +170 to +173
_Advanced:_ If you created a bridge interface, delete it using the following:
```bash
sudo ip link del br0
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This could move to the bridge section below

Copy link
Author

Choose a reason for hiding this comment

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

This is unchanged from the current version, but sure.


Set IPv4 forwarding back to disabled:
```bash
sudo sh -c "echo 0 > /proc/sys/net/ipv4/ip_forward" # usually the default
Copy link
Contributor

Choose a reason for hiding this comment

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

echo 0 | sudo tee /proc/sys/net/ipv4/ip_forward

sudo nft delete table firecracker
```

## Advanced: Multiple guests
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to add another section for multiple guests with namespaces (and restored from snapshots). Could this just reference docs/snapshotting/network-for-clones.md as they will be very similar?

Substituting our values, we get: `ip=172.16.0.2::172.16.0.1:255.255.255.252::eth0:off`. Insert this
at the end of your boot arguments for your microVM, and the guest Linux kernel will automatically
perform the routing configuration done in the _In the Guest_ section without needing `iproute2`
installed in the guest. (This argument doesn't configure DNS, however).
Copy link
Contributor

@pb8o pb8o Oct 30, 2024

Choose a reason for hiding this comment

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

Actually one can configure DNS with it in an additional parameter https://www.kernel.org/doc/html/v6.1/admin-guide/nfs/nfsroot.html?highlight=ip=

This should work ip=172.16.0.2::172.16.0.1:255.255.255.252::eth0:off:8.8.8.8


If you don't have anything else using `iptables` on your machine, clean up those
rules:
## Advanced: Guest network configuration at kernel level
Copy link
Contributor

Choose a reason for hiding this comment

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

Advanced: Guest network configuration using kernel command line

Comment on lines +18 to +21
The `nftables` Linux firewall with the `nft` command should be used instead of
`iptables`, since `iptables` and the associated tools are
[no longer recommended](https://access.redhat.com/solutions/6739041) for use on
production Linux systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most people are still more familiar with iptables than nftables. Can we can keep the equivalent iptables commands with collapsible sections like so:

using iptables iptables X Y Z
<details>
  <summary>using iptables</summary>
<code>iptables X Y Z</code>
</details>

@kanpov
Copy link
Author

kanpov commented Oct 31, 2024

Thanks for your contribution! I need to try the commands myself, but looks good so far.

I have a couple of asks: could we squash the commits together? I think it becomes easier to review. And we would like to keep the iptables commands for some time until nft becomes more spread.

I'll apply the squash and, as for iptables, I like your idea for collapsible blocks as opt-in for using it.

Iptables, however, is most certainly no longer the default and, even when it's used, it's used only through the iptables-nft compat layer which is already itself deprecated on some distros (including RHEL).

So I'll do a collapsible choice between iptables-nft and just nft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: WIP Indicates that an issue is currently being worked on or triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants