fix: improve network settings application and enhance iptables rule deletion

Signed-off-by: fahed dorgaa <[email protected]>

Author: fahedouch

cmd/nerdctl/container/container_rm_test.go

@@ -0,0 +1,107 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package container
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/containerd/nerdctl/mod/tigron/expect"
+	"github.com/containerd/nerdctl/mod/tigron/require"
+	"github.com/containerd/nerdctl/mod/tigron/test"
+
+	"github.com/containerd/nerdctl/v2/pkg/rootlessutil"
+	"github.com/containerd/nerdctl/v2/pkg/testutil"
+	"github.com/containerd/nerdctl/v2/pkg/testutil/nerdtest"
+)
+
+// TestContainerRmIptables tests that iptables rules are cleared after container deletion
+func TestContainerRmIptables(t *testing.T) {
+	// Check if iptables command exists
+	if _, err := exec.LookPath("iptables"); err != nil {
+		t.Skip("iptables command not found, skipping test")
+	}
+
+	testCase := nerdtest.Setup()
+
+	// This test requires Linux
+	testCase.Require = require.Not(require.Windows)
+
+	testCase.SubTests = []*test.Case{
+		{
+			Description: "Test iptables rules are cleared after container deletion",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				// Create a container with port mapping to ensure iptables rules are created
+				helpers.Ensure("run", "-d", "--name", data.Identifier(), "-p", "8080:80", testutil.NginxAlpineImage)
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				// Make sure container is removed even if test fails
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				containerID := data.Identifier()
+
+				// Check if iptables rules exist for the container in all tables
+				tables := []string{"nat", "filter", "mangle"}
+				rulesFound := false
+
+				for _, table := range tables {
+					iptablesCmd := exec.Command("iptables", "-t", table, "-S")
+					output, err := iptablesCmd.CombinedOutput()
+					if err != nil {
+						t.Logf("Warning: Failed to list iptables rules for table %s: %v", table, err)
+						continue
+					}
+
+					// Check if rules exist for the container
+					iptablesOutput := string(output)
+					if strings.Contains(iptablesOutput, containerID) {
+						rulesFound = true
+						t.Logf("Found iptables rules for container %s in table %s", containerID, table)
+						break
+					}
+				}
+
+				if !rulesFound {
+					t.Fatalf("Expected iptables rules for container %s, but none found", containerID)
+				}
+
+				// Remove the container
+				helpers.Ensure("rm", "-f", containerID)
+
+				// Check all tables to ensure rules are cleared
+				cmd := helpers.Command("run", "--rm", testutil.AlpineImage, "sh", "-c",
+					fmt.Sprintf(`
+					for table in nat filter mangle; do
+						if iptables -t $table -S | grep -q %s; then
+							echo "iptables rules still exist in table $table"
+							exit 1
+						fi
+					done
+					echo "iptables rules cleared"
+					`, containerID))
+
+				return cmd
+			},
+			Expected: test.Expects(0, nil, expect.Contains("iptables rules cleared")),
+		},
+	}
+
+	testCase.Run(t)
+}

pkg/ocihook/ocihook.go

@@ -24,6 +24,7 @@ import (
 	"io"
 	"net"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"time"
@@ -416,7 +417,7 @@ func getIP6AddressOpts(opts *handlerOpts) ([]cni.NamespaceOpts, error) {
 	return nil, nil
 }
 
-func applyNetworkSettings(opts *handlerOpts) error {
+func applyNetworkSettings(opts *handlerOpts) (err error) {
 	portMapOpts, err := getPortMapOpts(opts)
 	if err != nil {
 		return err
@@ -477,6 +478,15 @@ func applyNetworkSettings(opts *handlerOpts) error {
 	if err != nil {
 		return fmt.Errorf("failed to call cni.Setup: %w", err)
 	}
+
+	// Defer CNI configuration removal to ensure idempotency of oci-hook.
+	defer func() {
+		if err != nil {
+			log.L.Warn("Container failed starting. Removing allocated network configuration.")
+			_ = opts.cni.Remove(ctx, opts.fullID, nsPath, namespaceOpts...)
+		}
+	}()
+
 	cniResRaw := cniRes.Raw()
 	for i, cniName := range opts.cniNames {
 		hsMeta.Networks[cniName] = cniResRaw[i]
@@ -620,6 +630,15 @@ func onPostStop(opts *handlerOpts) error {
 			log.L.WithError(err).Errorf("failed to call cni.Remove")
 			return err
 		}
+
+		// 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 {
+			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
+		}
+
 		hs, err := hostsstore.New(opts.dataStore, ns)
 		if err != nil {
 			return err
@@ -640,6 +659,43 @@ 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 {