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

Nftables implementation #1881

Merged

Conversation

thomasferrandiz
Copy link
Contributor

@thomasferrandiz thomasferrandiz commented Feb 22, 2024

Description

Todos

  • Tests
  • Documentation
  • Release note

Release Note

This PR adds experimental support for natively using nftables when masquerading traffic.
Set `"EnableNFTables": true` to activate this feature.

@thomasferrandiz thomasferrandiz force-pushed the nftables-implementation branch 8 times, most recently from cdf9f83 to 00863cb Compare March 6, 2024 08:56
@thomasferrandiz thomasferrandiz marked this pull request as ready for review March 6, 2024 09:11
Copy link
Collaborator

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

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

looks very good!

Makefile Show resolved Hide resolved
e2e/run-e2e-tests.sh Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
pkg/trafficmngr/nftables/nftables.go Show resolved Hide resolved
pkg/trafficmngr/nftables/nftables_windows.go Outdated Show resolved Hide resolved
pkg/trafficmngr/nftables/utils.go Show resolved Hide resolved
pkg/trafficmngr/iptables/iptables_windows.go Show resolved Hide resolved
@thomasferrandiz thomasferrandiz force-pushed the nftables-implementation branch 2 times, most recently from 601f55b to fb437ca Compare March 12, 2024 11:01
Copy link
Contributor

@dereknola dereknola left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/trafficmngr/nftables/nftables.go Outdated Show resolved Hide resolved
pkg/trafficmngr/nftables/nftables.go Outdated Show resolved Hide resolved
log.Infof("Changing default FORWARD chain policy to ACCEPT")
tx := nftm.nftv4.NewTransaction()

//TODO how to express that the default is drop?
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to DROP by default? I think we can add a simple "drop" rule at the end of the chain. I saw that on the class the policy value is missing it should be something like this type filter hook forward priority -100; policy drop;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we don't need to drop by default. we just need to make sure that the flannel traffic is forwarded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I was confused by the question. It shouldn't drop but it should continue the chain in case it's not matching the flannel subnet using the default configuration of the node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's the default behavior so it should be fine as it is.
From the comment in flannel code, the forward rules seem needed for old versions of docker anyway.

This PR allows flannel to use nftables natively instead of iptables.
This is used essentially to masquerade traffic coming from the pods.

The PR also fixes the clean-up mechanism in the iptables implementation.
@thomasferrandiz thomasferrandiz merged commit 41a4a4d into flannel-io:master Mar 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants