Merge pull request #4835 from haytok/delete-chains-on-nat-table

fix: clean up unused iptables chains not being deleted on container r…

Author: AkihiroSuda

cmd/nerdctl/container/container_remove_linux_test.go

@@ -23,6 +23,8 @@ import (
 	"testing"
 	"time"
 
+	cniutils "github.com/containernetworking/plugins/pkg/utils"
+
 	"github.com/containerd/nerdctl/mod/tigron/expect"
 	"github.com/containerd/nerdctl/mod/tigron/require"
 	"github.com/containerd/nerdctl/mod/tigron/test"
@@ -111,10 +113,15 @@ func TestContainerRmIptables(t *testing.T) {
 			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
 				// Get the container ID from the label
 				containerID := data.Labels().Get("containerID")
+				id := fmt.Sprintf("%s-%s", testutil.Namespace, containerID)
+				chain := cniutils.FormatChainName("bridge", id)
 				return &test.Expected{
 					ExitCode: expect.ExitCodeSuccess,
-					// Verify that the iptables output does not contain the container ID
-					Output: expect.DoesNotContain(containerID),
+					// Verify that the iptables output does not contain the container ID and a chain name generated from the cni name, namespace, and container ID
+					Output: expect.All(
+						expect.DoesNotContain(containerID),
+						expect.DoesNotContain(chain),
+					),
 				}
 			},
 		},

go.mod

@@ -152,6 +152,7 @@ require (
 	github.com/moby/moby/api v1.54.1 // indirect
 	github.com/moby/sys/capability v0.4.0 // indirect
 	go.yaml.in/yaml/v4 v4.0.0-rc.4 // indirect
+	sigs.k8s.io/knftables v0.0.18 // indirect
 )
 
 replace github.com/containerd/nerdctl/mod/tigron v0.0.0 => ./mod/tigron

go.sum

@@ -186,6 +186,8 @@ github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
 github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
+github.com/lithammer/dedent v1.1.0 h1:VNzHMVCBNG1j0fh3OrsFRkVUwStdDArbgBWoPAffktY=
+github.com/lithammer/dedent v1.1.0/go.mod h1:jrXYCQtgg0nJiN+StA2KgR7w6CiQNv9Fd/Z9BP0jIOc=
 github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE=
 github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8=
 github.com/mattn/go-isatty v0.0.21 h1:xYae+lCNBP7QuW4PUnNG61ffM4hVIfm+zUzDuSzYLGs=
@@ -517,6 +519,8 @@ lukechampine.com/blake3 v1.3.0 h1:sJ3XhFINmHSrYCgl958hscfIa3bw8x4DqMP3u1YvoYE=
 lukechampine.com/blake3 v1.3.0/go.mod h1:0OFRp7fBtAylGVCO40o87sbupkyIGgbpv1+M1k1LM6k=
 pgregory.net/rapid v1.2.0 h1:keKAYRcjm+e1F0oAuU5F5+YPAWcyxNNRK2wud503Gnk=
 pgregory.net/rapid v1.2.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04=
+sigs.k8s.io/knftables v0.0.18 h1:6Duvmu0s/HwGifKrtl6G3AyAPYlWiZqTgS8bkVMiyaE=
+sigs.k8s.io/knftables v0.0.18/go.mod h1:f/5ZLKYEUPUhVjUCg6l80ACdL7CIIyeL0DxfgojGRTk=
 sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs=
 sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4=
 tags.cncf.io/container-device-interface v1.1.0 h1:RnxNhxF1JOu6CJUVpetTYvrXHdxw9j9jFYgZpI+anSY=

pkg/ocihook/ocihook.go

@@ -722,7 +722,7 @@ func onPostStop(opts *handlerOpts) error {
 		// opts.cni.Remove has trouble removing network configurations when netns is empty.
 		// Therefore, we force the deletion of iptables rules here to prevent netns exhaustion.
 		// This is a workaround until https://github.com/containernetworking/plugins/pull/1078 is merged.
-		if err := cleanupIptablesRules(opts.fullID); err != nil {
+		if err := cleanupIptablesRules(opts.fullID, opts.cniNames); err != nil {
 			log.L.WithError(err).Warnf("failed to clean up iptables rules for container %s", opts.fullID)
 			// Don't return error here, continue with the rest of the cleanup
 		}
@@ -755,43 +755,6 @@ func onPostStop(opts *handlerOpts) error {
 	return nil
 }
 
-// cleanupIptablesRules cleans up iptables rules related to the container
-func cleanupIptablesRules(containerID string) error {
-	// Check if iptables command exists
-	if _, err := exec.LookPath("iptables"); err != nil {
-		return fmt.Errorf("iptables command not found: %w", err)
-	}
-
-	// Tables to check for rules
-	tables := []string{"nat", "filter", "mangle"}
-
-	for _, table := range tables {
-		// Get all iptables rules for this table
-		cmd := exec.Command("iptables", "-t", table, "-S")
-		output, err := cmd.CombinedOutput()
-		if err != nil {
-			log.L.WithError(err).Warnf("failed to list iptables rules for table %s", table)
-			continue
-		}
-
-		// Find and delete rules related to the container
-		rules := strings.Split(string(output), "\n")
-		for _, rule := range rules {
-			if strings.Contains(rule, containerID) {
-				// Execute delete command
-				deleteCmd := exec.Command("sh", "-c", "--", fmt.Sprintf(`iptables -t %s -D %s`, table, rule[3:]))
-				if deleteOutput, err := deleteCmd.CombinedOutput(); err != nil {
-					log.L.WithError(err).Warnf("failed to delete iptables rule: %s, output: %s", rule, string(deleteOutput))
-				} else {
-					log.L.Debugf("deleted iptables rule: %s", rule)
-				}
-			}
-		}
-	}
-
-	return nil
-}
-
 // writePidFile writes the pid atomically to a file.
 // From https://github.com/containerd/containerd/blob/v1.7.0-rc.2/cmd/ctr/commands/commands.go#L265-L282
 func writePidFile(path string, pid int) error {

pkg/ocihook/ocihook_linux.go

@@ -17,6 +17,12 @@
 package ocihook
 
 import (
+	"fmt"
+	"os/exec"
+	"strings"
+
+	cniutils "github.com/containernetworking/plugins/pkg/utils"
+
 	"github.com/containerd/containerd/v2/contrib/apparmor"
 	"github.com/containerd/log"
 
@@ -37,3 +43,51 @@ func loadAppArmor() {
 		// but the profile was not actually loaded, runc will fail.
 	}
 }
+
+// cleanupIptablesRules cleans up iptables rules related to the container
+func cleanupIptablesRules(containerID string, cniNames []string) error {
+	// Check if iptables command exists
+	if _, err := exec.LookPath("iptables"); err != nil {
+		return fmt.Errorf("iptables command not found: %w", err)
+	}
+
+	// Tables to check for rules
+	tables := []string{"nat", "filter", "mangle"}
+
+	for _, table := range tables {
+		// Get all iptables rules for this table
+		cmd := exec.Command("iptables", "-t", table, "-S")
+		output, err := cmd.CombinedOutput()
+		if err != nil {
+			log.L.WithError(err).Warnf("failed to list iptables rules for table %s", table)
+			continue
+		}
+
+		// Find and delete rules related to the container
+		rules := strings.Split(string(output), "\n")
+		for _, rule := range rules {
+			if strings.Contains(rule, containerID) {
+				// Execute delete command
+				deleteCmd := exec.Command("sh", "-c", "--", fmt.Sprintf(`iptables -t %s -D %s`, table, rule[3:]))
+				if deleteOutput, err := deleteCmd.CombinedOutput(); err != nil {
+					log.L.WithError(err).Warnf("failed to delete iptables rule: %s, output: %s", rule, string(deleteOutput))
+				} else {
+					log.L.Debugf("deleted iptables rule: %s", rule)
+				}
+			}
+		}
+	}
+
+	// Delete CNI chains related to the container
+	for _, cniName := range cniNames {
+		chain := cniutils.FormatChainName(cniName, containerID)
+		deleteCmd := exec.Command("iptables", "-t", "nat", "-X", chain)
+		if deleteOutput, err := deleteCmd.CombinedOutput(); err != nil {
+			log.L.WithError(err).Warnf("failed to delete iptables chain: %s, output: %s", chain, string(deleteOutput))
+		} else {
+			log.L.Debugf("deleted iptables chain: %s", chain)
+		}
+	}
+
+	return nil
+}

pkg/ocihook/ocihook_nolinux.go

@@ -21,3 +21,8 @@ package ocihook
 func loadAppArmor() {
 	//noop
 }
+
+func cleanupIptablesRules(containerID string, cniNames []string) error {
+	//noop
+	return nil
+}