ipmasq: fix nftables backend

Rename
SetupIPMasqForNetwork -> SetupIPMasqForNetworks
TeardownIPMasqForNetwork -> TeardownIPMasqForNetworks
and have them take []*net.IPNet instead of *net.IPNet.

This allow the nftables backend to cleanup stale rules and recreate all
needed rules in a single transaction, where previously the stale rules
cleanup was breaking all but the last IPNet.

Fixes 61d0786

Signed-off-by: Etienne Champetier <[email protected]>

Author: champtar

pkg/ip/ipmasq_iptables_linux.go

@@ -15,26 +15,33 @@
 package ip
 
 import (
+	"errors"
 	"fmt"
 	"net"
+	"strings"
 
 	"github.com/coreos/go-iptables/iptables"
 
 	"github.com/containernetworking/cni/pkg/types"
 	"github.com/containernetworking/plugins/pkg/utils"
 )
 
-// setupIPMasqIPTables is the iptables-based implementation of SetupIPMasqForNetwork
-func setupIPMasqIPTables(ipn *net.IPNet, network, _, containerID string) error {
+// setupIPMasqIPTables is the iptables-based implementation of SetupIPMasqForNetworks
+func setupIPMasqIPTables(ipns []*net.IPNet, network, _, containerID string) error {
 	// Note: for historical reasons, the iptables implementation ignores ifname.
 	chain := utils.FormatChainName(network, containerID)
 	comment := utils.FormatComment(network, containerID)
-	return SetupIPMasq(ipn, chain, comment)
+	for _, ip := range ipns {
+		if err := SetupIPMasq(ip, chain, comment); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // SetupIPMasq installs iptables rules to masquerade traffic
 // coming from ip of ipn and going outside of ipn.
-// Deprecated: This function only supports iptables. Use SetupIPMasqForNetwork, which
+// Deprecated: This function only supports iptables. Use SetupIPMasqForNetworks, which
 // supports both iptables and nftables.
 func SetupIPMasq(ipn *net.IPNet, chain string, comment string) error {
 	isV6 := ipn.IP.To4() == nil
@@ -87,16 +94,28 @@ func SetupIPMasq(ipn *net.IPNet, chain string, comment string) error {
 	return ipt.AppendUnique("nat", "POSTROUTING", "-s", ipn.IP.String(), "-j", chain, "-m", "comment", "--comment", comment)
 }
 
-// teardownIPMasqIPTables is the iptables-based implementation of TeardownIPMasqForNetwork
-func teardownIPMasqIPTables(ipn *net.IPNet, network, _, containerID string) error {
+// teardownIPMasqIPTables is the iptables-based implementation of TeardownIPMasqForNetworks
+func teardownIPMasqIPTables(ipns []*net.IPNet, network, _, containerID string) error {
 	// Note: for historical reasons, the iptables implementation ignores ifname.
 	chain := utils.FormatChainName(network, containerID)
 	comment := utils.FormatComment(network, containerID)
-	return TeardownIPMasq(ipn, chain, comment)
+
+	var errs []string
+	for _, ipn := range ipns {
+		err := TeardownIPMasq(ipn, chain, comment)
+		if err != nil {
+			errs = append(errs, err.Error())
+		}
+	}
+
+	if errs == nil {
+		return nil
+	}
+	return errors.New(strings.Join(errs, "\n"))
 }
 
 // TeardownIPMasq undoes the effects of SetupIPMasq.
-// Deprecated: This function only supports iptables. Use TeardownIPMasqForNetwork, which
+// Deprecated: This function only supports iptables. Use TeardownIPMasqForNetworks, which
 // supports both iptables and nftables.
 func TeardownIPMasq(ipn *net.IPNet, chain string, comment string) error {
 	isV6 := ipn.IP.To4() == nil

pkg/ip/ipmasq_linux.go

@@ -24,11 +24,11 @@ import (
 	"github.com/containernetworking/plugins/pkg/utils"
 )
 
-// SetupIPMasqForNetwork installs rules to masquerade traffic coming from ip of ipn and
-// going outside of ipn, using a chain name based on network, ifname, and containerID. The
+// SetupIPMasqForNetworks installs rules to masquerade traffic coming from ips of ipns and
+// going outside of ipns, using a chain name based on network, ifname, and containerID. The
 // backend can be either "iptables" or "nftables"; if it is nil, then a suitable default
 // implementation will be used.
-func SetupIPMasqForNetwork(backend *string, ipn *net.IPNet, network, ifname, containerID string) error {
+func SetupIPMasqForNetworks(backend *string, ipns []*net.IPNet, network, ifname, containerID string) error {
 	if backend == nil {
 		// Prefer iptables, unless only nftables is available
 		defaultBackend := "iptables"
@@ -40,27 +40,27 @@ func SetupIPMasqForNetwork(backend *string, ipn *net.IPNet, network, ifname, con
 
 	switch *backend {
 	case "iptables":
-		return setupIPMasqIPTables(ipn, network, ifname, containerID)
+		return setupIPMasqIPTables(ipns, network, ifname, containerID)
 	case "nftables":
-		return setupIPMasqNFTables(ipn, network, ifname, containerID)
+		return setupIPMasqNFTables(ipns, network, ifname, containerID)
 	default:
 		return fmt.Errorf("unknown ipmasq backend %q", *backend)
 	}
 }
 
-// TeardownIPMasqForNetwork undoes the effects of SetupIPMasqForNetwork
-func TeardownIPMasqForNetwork(ipn *net.IPNet, network, ifname, containerID string) error {
+// TeardownIPMasqForNetworks undoes the effects of SetupIPMasqForNetworks
+func TeardownIPMasqForNetworks(ipns []*net.IPNet, network, ifname, containerID string) error {
 	var errs []string
 
 	// Do both the iptables and the nftables cleanup, since the pod may have been
 	// created with a different version of this plugin or a different configuration.
 
-	err := teardownIPMasqIPTables(ipn, network, ifname, containerID)
+	err := teardownIPMasqIPTables(ipns, network, ifname, containerID)
 	if err != nil && utils.SupportsIPTables() {
 		errs = append(errs, err.Error())
 	}
 
-	err = teardownIPMasqNFTables(ipn, network, ifname, containerID)
+	err = teardownIPMasqNFTables(ipns, network, ifname, containerID)
 	if err != nil && utils.SupportsNFTables() {
 		errs = append(errs, err.Error())
 	}

pkg/ip/ipmasq_nftables_linux.go

@@ -72,16 +72,16 @@ func commentForInstance(network, ifname, containerID string) string {
 	return comment
 }
 
-// setupIPMasqNFTables is the nftables-based implementation of SetupIPMasqForNetwork
-func setupIPMasqNFTables(ipn *net.IPNet, network, ifname, containerID string) error {
+// setupIPMasqNFTables is the nftables-based implementation of SetupIPMasqForNetworks
+func setupIPMasqNFTables(ipns []*net.IPNet, network, ifname, containerID string) error {
 	nft, err := knftables.New(knftables.InetFamily, ipMasqTableName)
 	if err != nil {
 		return err
 	}
-	return setupIPMasqNFTablesWithInterface(nft, ipn, network, ifname, containerID)
+	return setupIPMasqNFTablesWithInterface(nft, ipns, network, ifname, containerID)
 }
 
-func setupIPMasqNFTablesWithInterface(nft knftables.Interface, ipn *net.IPNet, network, ifname, containerID string) error {
+func setupIPMasqNFTablesWithInterface(nft knftables.Interface, ipns []*net.IPNet, network, ifname, containerID string) error {
 	staleRules, err := findRules(nft, hashForInstance(network, ifname, containerID))
 	if err != nil {
 		return err
@@ -128,37 +128,39 @@ func setupIPMasqNFTablesWithInterface(nft knftables.Interface, ipn *net.IPNet, n
 	for _, rule := range staleRules {
 		tx.Delete(rule)
 	}
-	ip := "ip"
-	if ipn.IP.To4() == nil {
-		ip = "ip6"
-	}
-
-	// e.g. if ipn is "192.168.1.4/24", then dstNet is "192.168.1.0/24"
-	dstNet := &net.IPNet{IP: ipn.IP.Mask(ipn.Mask), Mask: ipn.Mask}
+	for _, ipn := range ipns {
+		ip := "ip"
+		if ipn.IP.To4() == nil {
+			ip = "ip6"
+		}
 
-	tx.Add(&knftables.Rule{
-		Chain: ipMasqChainName,
-		Rule: knftables.Concat(
-			ip, "saddr", "==", ipn.IP,
-			ip, "daddr", "!=", dstNet,
-			"masquerade",
-		),
-		Comment: knftables.PtrTo(commentForInstance(network, ifname, containerID)),
-	})
+		// e.g. if ipn is "192.168.1.4/24", then dstNet is "192.168.1.0/24"
+		dstNet := &net.IPNet{IP: ipn.IP.Mask(ipn.Mask), Mask: ipn.Mask}
+
+		tx.Add(&knftables.Rule{
+			Chain: ipMasqChainName,
+			Rule: knftables.Concat(
+				ip, "saddr", "==", ipn.IP,
+				ip, "daddr", "!=", dstNet,
+				"masquerade",
+			),
+			Comment: knftables.PtrTo(commentForInstance(network, ifname, containerID)),
+		})
+	}
 
 	return nft.Run(context.TODO(), tx)
 }
 
-// teardownIPMasqNFTables is the nftables-based implementation of TeardownIPMasqForNetwork
-func teardownIPMasqNFTables(ipn *net.IPNet, network, ifname, containerID string) error {
+// teardownIPMasqNFTables is the nftables-based implementation of TeardownIPMasqForNetworks
+func teardownIPMasqNFTables(ipns []*net.IPNet, network, ifname, containerID string) error {
 	nft, err := knftables.New(knftables.InetFamily, ipMasqTableName)
 	if err != nil {
 		return err
 	}
-	return teardownIPMasqNFTablesWithInterface(nft, ipn, network, ifname, containerID)
+	return teardownIPMasqNFTablesWithInterface(nft, ipns, network, ifname, containerID)
 }
 
-func teardownIPMasqNFTablesWithInterface(nft knftables.Interface, _ *net.IPNet, network, ifname, containerID string) error {
+func teardownIPMasqNFTablesWithInterface(nft knftables.Interface, _ []*net.IPNet, network, ifname, containerID string) error {
 	rules, err := findRules(nft, hashForInstance(network, ifname, containerID))
 	if err != nil {
 		return err

pkg/ip/ipmasq_nftables_linux_test.go

@@ -15,6 +15,7 @@
 package ip
 
 import (
+	"net"
 	"strings"
 	"testing"
 
@@ -31,43 +32,55 @@ func Test_setupIPMasqNFTables(t *testing.T) {
 		network     string
 		ifname      string
 		containerID string
-		addr        string
+		addrs       []string
 	}{
 		{
 			network:     "unit-test",
 			ifname:      "eth0",
 			containerID: "one",
-			addr:        "192.168.1.1/24",
+			addrs:       []string{"192.168.1.1/24"},
 		},
 		{
 			network:     "unit-test",
 			ifname:      "eth0",
 			containerID: "two",
-			addr:        "192.168.1.2/24",
+			addrs:       []string{"192.168.1.2/24", "2001:db8::2/64"},
 		},
 		{
 			network:     "unit-test",
 			ifname:      "eth0",
 			containerID: "three",
-			addr:        "192.168.99.5/24",
+			addrs:       []string{"192.168.99.5/24"},
 		},
 		{
 			network:     "alternate",
 			ifname:      "net1",
 			containerID: "three",
-			addr:        "10.0.0.5/24",
+			addrs: []string{
+				"10.0.0.5/24",
+				"10.0.0.6/24",
+				"10.0.1.7/24",
+				"2001:db8::5/64",
+				"2001:db8::6/64",
+				"2001:db8:1::7/64",
+			},
 		},
 	}
 
 	for _, c := range containers {
-		addr, err := netlink.ParseAddr(c.addr)
-		if err != nil {
-			t.Fatalf("failed to parse test addr: %v", err)
+		ipns := []*net.IPNet{}
+		for _, addr := range c.addrs {
+			nladdr, err := netlink.ParseAddr(addr)
+			if err != nil {
+				t.Fatalf("failed to parse test addr: %v", err)
+			}
+			ipns = append(ipns, nladdr.IPNet)
 		}
-		err = setupIPMasqNFTablesWithInterface(nft, addr.IPNet, c.network, c.ifname, c.containerID)
+		err := setupIPMasqNFTablesWithInterface(nft, ipns, c.network, c.ifname, c.containerID)
 		if err != nil {
 			t.Fatalf("error from setupIPMasqNFTables: %v", err)
 		}
+
 	}
 
 	expected := strings.TrimSpace(`
@@ -76,8 +89,14 @@ add chain inet cni_plugins_masquerade masq_checks { comment "Masquerade traffic
 add chain inet cni_plugins_masquerade postrouting { type nat hook postrouting priority 100 ; }
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.1.1 ip daddr != 192.168.1.0/24 masquerade comment "6fd94d501e58f0aa-287fc69eff0574a2, net: unit-test, if: eth0, id: one"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.1.2 ip daddr != 192.168.1.0/24 masquerade comment "6fd94d501e58f0aa-d750b2c8f0f25d5f, net: unit-test, if: eth0, id: two"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::2 ip6 daddr != 2001:db8::/64 masquerade comment "6fd94d501e58f0aa-d750b2c8f0f25d5f, net: unit-test, if: eth0, id: two"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.99.5 ip daddr != 192.168.99.0/24 masquerade comment "6fd94d501e58f0aa-a4d4adb82b669cfe, net: unit-test, if: eth0, id: three"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.0.5 ip daddr != 10.0.0.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.0.6 ip daddr != 10.0.0.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.1.7 ip daddr != 10.0.1.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::5 ip6 daddr != 2001:db8::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::6 ip6 daddr != 2001:db8::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8:1::7 ip6 daddr != 2001:db8:1::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
 add rule inet cni_plugins_masquerade postrouting ip daddr == 224.0.0.0/4  return
 add rule inet cni_plugins_masquerade postrouting ip6 daddr == ff00::/8  return
 add rule inet cni_plugins_masquerade postrouting goto masq_checks
@@ -88,22 +107,18 @@ add rule inet cni_plugins_masquerade postrouting goto masq_checks
 	}
 
 	// Add a new container reusing "one"'s address, before deleting "one"
-	addr, err := netlink.ParseAddr(containers[0].addr)
+	c := containers[0]
+	addr, err := netlink.ParseAddr(c.addrs[0])
 	if err != nil {
 		t.Fatalf("failed to parse test addr: %v", err)
 	}
-	err = setupIPMasqNFTablesWithInterface(nft, addr.IPNet, "unit-test", "eth0", "four")
+	err = setupIPMasqNFTablesWithInterface(nft, []*net.IPNet{addr.IPNet}, "unit-test", "eth0", "four")
 	if err != nil {
 		t.Fatalf("error from setupIPMasqNFTables: %v", err)
 	}
 
 	// Remove "one"
-	c := containers[0]
-	addr, err = netlink.ParseAddr(c.addr)
-	if err != nil {
-		t.Fatalf("failed to parse test addr: %v", err)
-	}
-	err = teardownIPMasqNFTablesWithInterface(nft, addr.IPNet, c.network, c.ifname, c.containerID)
+	err = teardownIPMasqNFTablesWithInterface(nft, []*net.IPNet{addr.IPNet}, c.network, c.ifname, c.containerID)
 	if err != nil {
 		t.Fatalf("error from teardownIPMasqNFTables: %v", err)
 	}
@@ -114,8 +129,14 @@ add table inet cni_plugins_masquerade { comment "Masquerading for plugins from g
 add chain inet cni_plugins_masquerade masq_checks { comment "Masquerade traffic from certain IPs to any (non-multicast) IP outside their subnet" ; }
 add chain inet cni_plugins_masquerade postrouting { type nat hook postrouting priority 100 ; }
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.1.2 ip daddr != 192.168.1.0/24 masquerade comment "6fd94d501e58f0aa-d750b2c8f0f25d5f, net: unit-test, if: eth0, id: two"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::2 ip6 daddr != 2001:db8::/64 masquerade comment "6fd94d501e58f0aa-d750b2c8f0f25d5f, net: unit-test, if: eth0, id: two"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.99.5 ip daddr != 192.168.99.0/24 masquerade comment "6fd94d501e58f0aa-a4d4adb82b669cfe, net: unit-test, if: eth0, id: three"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.0.5 ip daddr != 10.0.0.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.0.6 ip daddr != 10.0.0.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.1.7 ip daddr != 10.0.1.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::5 ip6 daddr != 2001:db8::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::6 ip6 daddr != 2001:db8::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8:1::7 ip6 daddr != 2001:db8:1::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.1.1 ip daddr != 192.168.1.0/24 masquerade comment "6fd94d501e58f0aa-e766de567ef6c543, net: unit-test, if: eth0, id: four"
 add rule inet cni_plugins_masquerade postrouting ip daddr == 224.0.0.0/4  return
 add rule inet cni_plugins_masquerade postrouting ip6 daddr == ff00::/8  return
@@ -150,8 +171,14 @@ add table inet cni_plugins_masquerade { comment "Masquerading for plugins from g
 add chain inet cni_plugins_masquerade masq_checks { comment "Masquerade traffic from certain IPs to any (non-multicast) IP outside their subnet" ; }
 add chain inet cni_plugins_masquerade postrouting { type nat hook postrouting priority 100 ; }
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.1.2 ip daddr != 192.168.1.0/24 masquerade comment "6fd94d501e58f0aa-d750b2c8f0f25d5f, net: unit-test, if: eth0, id: two"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::2 ip6 daddr != 2001:db8::/64 masquerade comment "6fd94d501e58f0aa-d750b2c8f0f25d5f, net: unit-test, if: eth0, id: two"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 192.168.99.5 ip daddr != 192.168.99.0/24 masquerade comment "6fd94d501e58f0aa-a4d4adb82b669cfe, net: unit-test, if: eth0, id: three"
 add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.0.5 ip daddr != 10.0.0.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.0.6 ip daddr != 10.0.0.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip saddr == 10.0.1.7 ip daddr != 10.0.1.0/24 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::5 ip6 daddr != 2001:db8::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8::6 ip6 daddr != 2001:db8::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
+add rule inet cni_plugins_masquerade masq_checks ip6 saddr == 2001:db8:1::7 ip6 daddr != 2001:db8:1::/64 masquerade comment "82783ef24bdc7036-acb19d111858e348, net: alternate, if: net1, id: three"
 add rule inet cni_plugins_masquerade postrouting ip daddr == 224.0.0.0/4  return
 add rule inet cni_plugins_masquerade postrouting ip6 daddr == ff00::/8  return
 add rule inet cni_plugins_masquerade postrouting goto masq_checks

plugins/main/bridge/bridge.go

@@ -668,10 +668,12 @@ func cmdAdd(args *skel.CmdArgs) error {
 		}
 
 		if n.IPMasq {
+			ipns := []*net.IPNet{}
 			for _, ipc := range result.IPs {
-				if err = ip.SetupIPMasqForNetwork(n.IPMasqBackend, &ipc.Address, n.Name, args.IfName, args.ContainerID); err != nil {
-					return err
-				}
+				ipns = append(ipns, &ipc.Address)
+			}
+			if err = ip.SetupIPMasqForNetworks(n.IPMasqBackend, ipns, n.Name, args.IfName, args.ContainerID); err != nil {
+				return err
 			}
 		}
 	} else if !n.DisableContainerInterface {
@@ -807,10 +809,8 @@ func cmdDel(args *skel.CmdArgs) error {
 	}
 
 	if isLayer3 && n.IPMasq {
-		for _, ipn := range ipnets {
-			if err := ip.TeardownIPMasqForNetwork(ipn, n.Name, args.IfName, args.ContainerID); err != nil {
-				return err
-			}
+		if err := ip.TeardownIPMasqForNetworks(ipnets, n.Name, args.IfName, args.ContainerID); err != nil {
+			return err
 		}
 	}