nftables support for ipmasq and portmap by danwinship · Pull Request #935 · containernetworking/plugins

danwinship · 2023-07-31T14:00:06Z

This adds nftables backends to the two remaining iptables-only bits in containernetworking/plugins; ipmasq (used by ptp and bridge) and portmap. For now, they are both set up to use iptables by default, unless you explicitly request nftables via the network config, or you run the plugin on a host that has nft installed but not iptables (eg, future RHEL 10 hosts).

The firewall plugin currently has "iptables" and "firewalld" backends. In theory it could have a separate explicit "nftables" backend, but I didn't do that here.

The ipmasq and portmap code implement nftables via https://github.com/danwinship/nftables, which is a library I started for the planned kube-proxy nftables backend, and also plan to use for various other nftables ports (eg, cri-o). It might move from danwinship/ to somewhere more generic at some point, but it might not.

After I started working on this branch, I discovered that the bridge plugin was actually already using nftables via pkg/link/spoofcheck.go, using the https://github.com/networkplumbing/go-nft library. Using two separate nftables libraries is probably not great, so we may want to fix that eventually. The libraries have somewhat similar APIs up to a point, then diverge completely because go-nft uses the JSON API to /sbin/nft and therefore has to use the (poorly-documented) JSON rule schema, while danwinship/nftables uses the plain text API to /sbin/nft and so uses "normal" nft rules. eg, compare:

func (sc *SpoofChecker) matchIfaceJumpToChainRule(chain, toChain string) *schema.Rule {
        return &schema.Rule{
                Family: schema.FamilyBridge,
                Table:  natTableName,
                Chain:  chain,
                Expr: []schema.Statement{
                        {Match: &schema.Match{
                                Op:    schema.OperEQ,
                                Left:  schema.Expression{RowData: []byte(`{"meta":{"key":"iifname"}}`)
},
                                Right: schema.Expression{String: &sc.iface},
                        }},
                        {Verdict: schema.Verdict{Jump: &schema.ToTarget{Target: toChain}}},
                },
                Comment: ruleComment(sc.refID),
        }
}

which in danwinship/nftables syntax would become:

func (sc *SpoofChecker) matchIfaceJumpToChainRule(chain, toChain string) *nftables.Rule {
        return &nftables.Rule{
                Chain: chain,
                Rule: nftables.Concat(
                        "iifname", sc.iface,
                        "jump", toChain,
                ),
                Comment: ruleComment(sc.refID),
        }
}

("Draft" PR because not fully-tested yet...)

danwinship · 2023-08-01T15:19:03Z

  [FAILED] Unexpected error:
      <*errors.errorString | 0xc0003bdb20>: 
      running [/usr/sbin/iptables -t nat -D POSTROUTING -s 10.1.2.2 -j CNI-43a5a67926c1a665ff4c21b7 -m comment --comment name: "testConfig" id: "dummy-0" --wait]: exit status 2: iptables v1.8.7 (nf_tables): Chain 'CNI-43a5a67926c1a665ff4c21b7' does not exist

sigh, this is what coreos/go-iptables#107 was supposed to fix but I guess I fixed it for iptables-legacy but not iptables-nft?

(Was this not failing before? I'm not sure why this change would make the race condition suddenly start happening.)

aojea · 2023-08-21T15:54:48Z

 // SetupIPMasq installs iptables rules to masquerade traffic
 // coming from ip of ipn and going outside of ipn
-func SetupIPMasq(ipn *net.IPNet, chain string, comment string) error {
+func SetupIPMasq(ipn *net.IPNet, pluginName, containerID string) error {


not a lot but seems this is used externally too https://grep.app/search?q=SetupIPMasq

ah, right. Do I have to add a new API rather than changing the old one? I don't see anything about API stability guarantees...

not familiar with this codebase , let's ask @squeed

aojea · 2024-05-27T15:48:21Z

linter fail, I see is still in draft, let me know if you want a review,

danwinship · 2024-07-24T15:10:43Z

OK, finally updated and tested:

ip.SetupIPMasq() and ip.TeardownIPMasq() are now unchanged (and still always use iptables). Instead, I added a new SetupIPMasqForPlugin and TeardownIPMasqForPlugin that support both iptables and nftables.
The bridge and ptp plugins now use iptables by default, or nftables if iptables isn't installed or doesn't work. Or you can force the backend via "ipMasqBackend": "iptables" or "ipMasqBackend": "nftables" in the config.
Likewise, portmap now supports nftables, but still uses iptables by default, unless it's not available/usable, or you pass "backend": "nftables", or you set "conditionsV4" or "conditionsV6" to something that looks like nftables syntax rather than iptables.

(I guess this will need a docs update to the cni.dev repo?)

danwinship · 2024-07-24T15:12:47Z

/assign @squeed

danwinship · 2024-07-24T16:50:19Z

oh, and I could port this to use the same nftables library that spoofcheck uses, if you preferred.

or port the spoofcheck code to use knftables like kube-proxy (and Calico and soon some other stuff)

danwinship · 2024-08-26T14:28:00Z

or port the spoofcheck code to use knftables like kube-proxy

danwinship@17bb5355

squeed · 2024-08-27T08:30:50Z

+
+// hashForContainer returns a unique hash identifying the rules for this container with
+// this plugin
+func hashForContainer(pluginName, containerID string) string {


There is an error in the iptables implementation of this -- this needs to hash (network, ifname, containerid).

pluginName here is NetConf.Name, which is what you mean by network, right? (Yeah, pluginName is a bad variable name... I was assuming it meant NetConf.Type...)

Why do we need ifname?

Why do we need ifname?

Because it is allowed to add and remove the same container to the same network multiple times. In that case, the ifname is the only distinct identifier.

squeed · 2024-08-27T08:32:17Z

+}
+
+func setupIPMasqNFTablesWithInterface(nft knftables.Interface, ipn *net.IPNet, pluginName, containerID string) error {
+	comment := hashForContainer(pluginName, containerID)


The iptables implementation is wrong here -- we need to be able to find all masquerade rules for a given network. Can the comment include network name in plain text?

squeed · 2024-08-27T08:38:09Z

Looks basically good. I'd like to see GC support if possible, since it informs the design the API. Could you add that, even if the top-end API glue isn't there yet.

…sNFTables Signed-off-by: Dan Winship <[email protected]>

Signed-off-by: Dan Winship <[email protected]>

Use `conditionsV4` and `conditionsV6` values that actually look like valid iptables conditions. Signed-off-by: Dan Winship <[email protected]>

Signed-off-by: Dan Winship <[email protected]>

danwinship · 2024-09-12T21:07:53Z

@squeed this is ready for re-review btw; it includes an implementation of GC for nftables ipmasq, which isn't actually plumbed through, but at least gets unit tested.

champtar · 2024-11-06T01:49:28Z

I've started to play with this, found a regression (#1114 / PR #1117), and 2 bugs ( #1115 - PR #1116 / #1118 - PR #1120 )

danwinship force-pushed the nftables branch 2 times, most recently from d6e2a46 to 60eb4e3 Compare July 31, 2023 14:18

aojea reviewed Aug 21, 2023

View reviewed changes

Comment thread pkg/utils/netfilter.go Outdated

aojea reviewed Aug 21, 2023

View reviewed changes

Comment thread pkg/utils/netfilter.go

aojea reviewed Aug 21, 2023

View reviewed changes

danwinship mentioned this pull request Oct 22, 2023

kube-proxy nftables backend kubernetes/kubernetes#121046

Merged

13 tasks

danwinship force-pushed the nftables branch 2 times, most recently from c69144c to e48682c Compare May 25, 2024 12:45

danwinship force-pushed the nftables branch from e48682c to ee4aa40 Compare July 24, 2024 15:02

danwinship marked this pull request as ready for review July 24, 2024 15:02

danwinship force-pushed the nftables branch from ee4aa40 to 81950d7 Compare July 24, 2024 15:36

aojea reviewed Jul 24, 2024

View reviewed changes

Comment thread go.mod Outdated

squeed self-requested a review August 19, 2024 15:07

squeed reviewed Aug 27, 2024

View reviewed changes

Comment thread go.mod Outdated

squeed reviewed Aug 27, 2024

View reviewed changes

danwinship mentioned this pull request Aug 28, 2024

update deps (go-iptables, cni) #1083

Merged

Vendor nftables library, add utils.SupportsIPTables and utils.Support…

b039dca

…sNFTables Signed-off-by: Dan Winship <[email protected]>

danwinship force-pushed the nftables branch from 81950d7 to 15ff25f Compare August 29, 2024 13:09

danwinship added 2 commits August 29, 2024 09:41

Add nftables implementation of ipmasq

28cffdc

Signed-off-by: Dan Winship <[email protected]>

Add a backend abstraction to the portmap plugin

ec2b162

Signed-off-by: Dan Winship <[email protected]>

danwinship force-pushed the nftables branch 2 times, most recently from a833549 to 043253e Compare August 29, 2024 13:49

danwinship mentioned this pull request Aug 29, 2024

Document nftables backends for portmap and ipmasq containernetworking/cni.dev#143

Merged

aojea reviewed Sep 3, 2024

View reviewed changes

Comment thread plugins/meta/portmap/main.go

aojea reviewed Sep 3, 2024

View reviewed changes

Comment thread plugins/meta/portmap/portmap_test.go Outdated

danwinship added 2 commits September 3, 2024 08:25

Fix portmap unit tests

a3373d8

Use `conditionsV4` and `conditionsV6` values that actually look like valid iptables conditions. Signed-off-by: Dan Winship <[email protected]>

Add nftables backend to portmap

94e23e7

Signed-off-by: Dan Winship <[email protected]>

danwinship force-pushed the nftables branch from 043253e to 94e23e7 Compare September 3, 2024 12:25

squeed approved these changes Sep 16, 2024

View reviewed changes

squeed merged commit 01a94e1 into containernetworking:main Sep 16, 2024

danwinship deleted the nftables branch September 16, 2024 21:33

dtrudg mentioned this pull request Oct 4, 2024

Port mapping not working sylabs/singularity#3318

Closed

4 tasks

champtar mentioned this pull request Nov 5, 2024

iptables backend was requested but configuration contains nftables-specific options [conditionsV4] #1114

Closed

Conversation

danwinship commented Jul 31, 2023

Uh oh!

danwinship commented Aug 1, 2023

Uh oh!

Uh oh!

Uh oh!

aojea Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

danwinship Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

aojea Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

aojea commented May 27, 2024

Uh oh!

danwinship commented Jul 24, 2024

Uh oh!

danwinship commented Jul 24, 2024

Uh oh!

danwinship commented Jul 24, 2024

Uh oh!

Uh oh!

danwinship commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

squeed Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

danwinship Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

squeed Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

squeed Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

squeed commented Aug 27, 2024

Uh oh!

Uh oh!

Uh oh!

danwinship commented Sep 12, 2024

Uh oh!

champtar commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danwinship commented Aug 26, 2024 •

edited

Loading

champtar commented Nov 6, 2024 •

edited

Loading