fix: improve network settings application and enhance iptables rule deletion

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

Author: fahedouch

cmd/nerdctl/container/container_remove_linux_test.go

@@ -0,0 +1,127 @@
+/*
+   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"
+	"strconv"
+	"testing"
+	"time"
+
+	"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"
+)
+
+// iptablesCheckCommand is the shell command to check iptables rules
+const iptablesCheckCommand = "iptables -t nat -S && iptables -t filter -S && iptables -t mangle -S"
+
+// testContainerRmIptablesExecutor is a common executor function for testing iptables rules cleanup
+func testContainerRmIptablesExecutor(data test.Data, helpers test.Helpers) test.TestableCommand {
+	t := helpers.T()
+
+	// Get the actual container ID from the container name and store it in a label
+	containerID := data.Labels().Get("containerID")
+	if containerID == "" {
+		container := nerdtest.InspectContainer(helpers, data.Identifier())
+		containerID = container.ID
+		data.Labels().Set("containerID", containerID)
+	}
+
+	// Remove the container
+	helpers.Ensure("rm", "-f", containerID)
+
+	time.Sleep(1 * time.Second)
+
+	// Create a TestableCommand using helpers.Custom
+	if rootlessutil.IsRootless() {
+		// In rootless mode, we need to enter the rootlesskit network namespace
+		stateDir, err := rootlessutil.RootlessKitStateDir()
+		if err != nil {
+			t.Fatalf("Failed to get rootlesskit state dir: %v", err)
+		}
+
+		childPid, err := rootlessutil.RootlessKitChildPid(stateDir)
+		if err != nil {
+			t.Fatalf("Failed to get rootlesskit child pid: %v", err)
+		}
+
+		// Construct the path to the network namespace
+		uid := os.Getuid()
+		netnsPath := fmt.Sprintf("/run/user/%d/containerd-rootless/netns", uid)
+
+		if netns, err := rootlessutil.DetachedNetNS(); err != nil {
+			t.Fatalf("Failed to get detached network namespace: %v", err)
+		} else {
+			if netns != "" {
+				// First enter the user and mount namespace, then the network namespace
+				return helpers.Custom("nsenter", "-t", strconv.Itoa(childPid), "-U", "-m", "--preserve-credentials",
+					"--", "sh", "-c", fmt.Sprintf("nsenter --net=%s sh -c '%s'", netnsPath, iptablesCheckCommand))
+			}
+			// Enter into RootlessKit namespace using containerd-rootless-setuptool.sh
+			return helpers.Custom("containerd-rootless-setuptool.sh", "nsenter", "--", "sh", "-c", iptablesCheckCommand)
+		}
+	}
+
+	// In non-rootless mode, check iptables rules directly on the host
+	return helpers.Custom("sh", "-c", iptablesCheckCommand)
+}
+
+// TestContainerRmIptables tests that iptables rules are cleared after container deletion
+func TestContainerRmIptables(t *testing.T) {
+	testCase := nerdtest.Setup()
+
+	// Require iptables and containerd-rootless-setuptool.sh commands to be available
+	testCase.Require = require.All(
+		require.Binary("iptables"),
+		require.Binary("containerd-rootless-setuptool.sh"),
+		require.Not(require.Windows),
+		require.Not(nerdtest.Docker),
+	)
+
+	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)
+				nerdtest.EnsureContainerStarted(helpers, identifier)
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				// Make sure container is removed even if test fails
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: testContainerRmIptablesExecutor,
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				// Get the container ID from the label
+				containerID := data.Labels().Get("containerID")
+				return &test.Expected{
+					ExitCode: expect.ExitCodeSuccess,
+					// Verify that the iptables output does not contain the container ID
+					Output: expect.DoesNotContain(containerID),
+				}
+			},
+		},
+	}
+
+	testCase.Run(t)
+}

pkg/ocihook/ocihook.go

@@ -24,6 +24,7 @@ import (
 	"io"
 	"net"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"time"
@@ -418,7 +419,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
@@ -479,6 +480,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]
@@ -622,6 +632,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
@@ -642,6 +661,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 {