fix(orchestrator): harden shutdown network cleanup (#3000)

Return network slots synchronously during sandbox cleanup instead of
fire-and-forget goroutines, guard firewall teardown against missing
iptables state, and preserve ErrClosed when returning slots to a closed
pool.

Author: wj-e2b

packages/orchestrator/pkg/sandbox/network/firewall.go

@@ -3,6 +3,7 @@
 package network
 
 import (
+	"errors"
 	"fmt"
 	"net/netip"
 	"slices"
@@ -114,7 +115,16 @@ func (fw *Firewall) Close() error {
 	fw.mu.Lock()
 	defer fw.mu.Unlock()
 
-	return fw.conn.CloseLasting()
+	fw.conn.DelTable(&nftables.Table{
+		Name:   tableName,
+		Family: nftables.TableFamilyINet,
+	})
+	deleteErr := fw.conn.Flush()
+	if errors.Is(deleteErr, unix.ENOENT) {
+		deleteErr = nil
+	}
+
+	return errors.Join(deleteErr, fw.conn.CloseLasting())
 }
 
 // tapIfaceMatch returns expressions that match packets from the tap interface.

packages/orchestrator/pkg/sandbox/network/network.go

@@ -7,18 +7,75 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"os"
 	"runtime"
 	"strconv"
 
 	"github.com/coreos/go-iptables/iptables"
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netns"
 	"go.uber.org/zap"
+	"golang.org/x/sys/unix"
 
 	"github.com/e2b-dev/infra/packages/shared/pkg/logger"
 )
 
-func (s *Slot) CreateNetwork(ctx context.Context) error {
+type notExistError interface {
+	IsNotExist() bool
+}
+
+type multiUnwrapError interface {
+	Unwrap() []error
+}
+
+func ignoreExpectedAbsent(err error, isExpected func(error) bool) bool {
+	if err == nil {
+		return true
+	}
+
+	var joined multiUnwrapError
+	if errors.As(err, &joined) {
+		for _, child := range joined.Unwrap() {
+			if !ignoreExpectedAbsent(child, isExpected) {
+				return false
+			}
+		}
+
+		return true
+	}
+
+	return isExpected(err)
+}
+
+func isIPTablesNotExist(err error) bool {
+	var notExist notExistError
+
+	return errors.As(err, &notExist) && notExist.IsNotExist()
+}
+
+func isRouteNotExist(err error) bool {
+	return errors.Is(err, unix.ESRCH) || errors.Is(err, unix.ENOENT)
+}
+
+func isLinkNotExist(err error) bool {
+	var linkNotFound netlink.LinkNotFoundError
+
+	return errors.As(err, &linkNotFound) || errors.Is(err, unix.ENODEV) || errors.Is(err, unix.ENOENT)
+}
+
+func isNamespaceNotExist(err error) bool {
+	return os.IsNotExist(err) || errors.Is(err, unix.ENOENT)
+}
+
+func appendUnlessExpectedAbsentf(errs *[]error, err error, isExpected func(error) bool, format string) {
+	if ignoreExpectedAbsent(err, isExpected) {
+		return
+	}
+
+	*errs = append(*errs, fmt.Errorf(format, err))
+}
+
+func (s *Slot) CreateNetwork(ctx context.Context) (retErr error) {
 	// Prevent thread changes so we can safely manipulate with namespaces
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
@@ -29,10 +86,19 @@ func (s *Slot) CreateNetwork(ctx context.Context) error {
 		return fmt.Errorf("cannot get current (host) namespace: %w", err)
 	}
 
+	cleanupNeeded := false
 	defer func() {
-		err = netns.Set(hostNS)
-		if err != nil {
-			logger.L().Error(ctx, "error resetting network namespace back to the host namespace", zap.Error(err))
+		restoreErr := netns.Set(hostNS)
+		if restoreErr != nil {
+			logger.L().Error(ctx, "error resetting network namespace back to the host namespace", zap.Error(restoreErr))
+		}
+
+		if retErr != nil && cleanupNeeded {
+			if restoreErr != nil {
+				retErr = errors.Join(retErr, fmt.Errorf("error resetting network namespace back to the host namespace before cleanup: %w", restoreErr))
+			} else if cleanupErr := s.RemoveNetwork(); cleanupErr != nil {
+				retErr = errors.Join(retErr, fmt.Errorf("error cleaning up partially created network: %w", cleanupErr))
+			}
 		}
 
 		err = hostNS.Close()
@@ -46,6 +112,7 @@ func (s *Slot) CreateNetwork(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("cannot create new namespace: %w", err)
 	}
+	cleanupNeeded = true
 
 	defer ns.Close()
 
@@ -273,85 +340,67 @@ func (s *Slot) RemoveNetwork() error {
 	} else {
 		// Delete host forwarding rules
 		err = tables.Delete("filter", "FORWARD", "-i", s.VethName(), "-o", defaultGateway, "-j", "ACCEPT")
-		if err != nil {
-			errs = append(errs, fmt.Errorf("error deleting host forwarding rule to default gateway: %w", err))
-		}
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "error deleting host forwarding rule to default gateway: %w")
 
 		err = tables.Delete("filter", "FORWARD", "-i", defaultGateway, "-o", s.VethName(), "-j", "ACCEPT")
-		if err != nil {
-			errs = append(errs, fmt.Errorf("error deleting host forwarding rule from default gateway: %w", err))
-		}
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "error deleting host forwarding rule from default gateway: %w")
 
 		// Delete host postrouting rules
 		err = tables.Delete("nat", "POSTROUTING", "-s", s.HostCIDR(), "-o", defaultGateway, "-j", "MASQUERADE")
-		if err != nil {
-			errs = append(errs, fmt.Errorf("error deleting host postrouting rule: %w", err))
-		}
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "error deleting host postrouting rule: %w")
 
 		// Delete hyperloop proxy redirect rule
 		err = tables.Delete(
 			"nat", "PREROUTING", "-i", s.VethName(),
 			"-p", "tcp", "-d", s.config.OrchestratorInSandboxIPAddress, "--dport", "80",
 			"-j", "REDIRECT", "--to-port", s.hyperloopPort,
 		)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("error deleting sandbox hyperloop proxy redirect rule: %w", err))
-		}
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "error deleting sandbox hyperloop proxy redirect rule: %w")
 
 		// Delete changes made by egress proxy
 		err = s.egressProxy.OnSlotDelete(s, tables)
-		if err != nil {
-			errs = append(errs, err)
-		}
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "%w")
 	}
 
 	// Delete routing from host to FC namespace
 	err = netlink.RouteDel(&netlink.Route{
 		Gw:  s.VpeerIP(),
 		Dst: s.HostNet(),
 	})
-	if err != nil {
-		errs = append(errs, fmt.Errorf("error deleting route from host to FC: %w", err))
-	}
+	appendUnlessExpectedAbsentf(&errs, err, isRouteNotExist, "error deleting route from host to FC: %w")
 
 	// Delete veth device
 	// We explicitly delete the veth device from the host namespace because even though deleting
 	// is deleting the device there may be a race condition when creating a new veth device with
 	// the same name immediately after deleting the namespace.
 	veth, err := netlink.LinkByName(s.VethName())
 	if err != nil {
-		errs = append(errs, fmt.Errorf("error finding veth: %w", err))
+		appendUnlessExpectedAbsentf(&errs, err, isLinkNotExist, "error finding veth: %w")
 	} else {
 		err = netlink.LinkDel(veth)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("error deleting veth device: %w", err))
-		}
+		appendUnlessExpectedAbsentf(&errs, err, isLinkNotExist, "error deleting veth device: %w")
 	}
 
-	// Delete NFS proxy redirect rule
-	err = tables.Delete("nat", "PREROUTING",
-		"--in-interface", s.VethName(), "--protocol", "tcp",
-		"--destination", s.config.OrchestratorInSandboxIPAddress, "--dport", "2049",
-		"--jump", "REDIRECT", "--to-port", strconv.Itoa(int(s.config.NFSProxyPort)),
-	)
-	if err != nil {
-		errs = append(errs, fmt.Errorf("error deleting sandbox NFS proxy redirect rule: %w", err))
-	}
+	if tables != nil {
+		// Delete NFS proxy redirect rule
+		err = tables.Delete("nat", "PREROUTING",
+			"--in-interface", s.VethName(), "--protocol", "tcp",
+			"--destination", s.config.OrchestratorInSandboxIPAddress, "--dport", "2049",
+			"--jump", "REDIRECT", "--to-port", strconv.Itoa(int(s.config.NFSProxyPort)),
+		)
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "error deleting sandbox NFS proxy redirect rule: %w")
 
-	// Delete portmapper redirect rule
-	err = tables.Delete("nat", "PREROUTING",
-		"--in-interface", s.VethName(), "--protocol", "tcp",
-		"--destination", s.config.OrchestratorInSandboxIPAddress, "--dport", "111",
-		"--jump", "REDIRECT", "--to-port", strconv.Itoa(int(s.config.PortmapperPort)),
-	)
-	if err != nil {
-		errs = append(errs, fmt.Errorf("error deleting sandbox portmapper redirect rule: %w", err))
+		// Delete portmapper redirect rule
+		err = tables.Delete("nat", "PREROUTING",
+			"--in-interface", s.VethName(), "--protocol", "tcp",
+			"--destination", s.config.OrchestratorInSandboxIPAddress, "--dport", "111",
+			"--jump", "REDIRECT", "--to-port", strconv.Itoa(int(s.config.PortmapperPort)),
+		)
+		appendUnlessExpectedAbsentf(&errs, err, isIPTablesNotExist, "error deleting sandbox portmapper redirect rule: %w")
 	}
 
 	err = netns.DeleteNamed(s.NamespaceID())
-	if err != nil {
-		errs = append(errs, fmt.Errorf("error deleting namespace: %w", err))
-	}
+	appendUnlessExpectedAbsentf(&errs, err, isNamespaceNotExist, "error deleting namespace: %w")
 
 	return errors.Join(errs...)
 }

packages/orchestrator/pkg/sandbox/network/network_test.go

@@ -0,0 +1,49 @@
+//go:build linux
+
+package network
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
+)
+
+type fakeNotExistError struct {
+	notExist bool
+}
+
+func (e fakeNotExistError) Error() string {
+	return "fake iptables error"
+}
+
+func (e fakeNotExistError) IsNotExist() bool {
+	return e.notExist
+}
+
+func TestIgnoreExpectedAbsentHandlesWrappedAndJoinedErrors(t *testing.T) {
+	t.Parallel()
+
+	wrapped := fmt.Errorf("wrapped: %w", fakeNotExistError{notExist: true})
+	joined := errors.Join(wrapped, fakeNotExistError{notExist: true})
+
+	require.True(t, ignoreExpectedAbsent(joined, isIPTablesNotExist))
+	require.False(t, ignoreExpectedAbsent(errors.Join(joined, errors.New("boom")), isIPTablesNotExist))
+	require.False(t, ignoreExpectedAbsent(fakeNotExistError{notExist: false}, isIPTablesNotExist))
+}
+
+func TestExpectedAbsentClassifiers(t *testing.T) {
+	t.Parallel()
+
+	require.True(t, isRouteNotExist(fmt.Errorf("route delete failed: %w", unix.ESRCH)))
+	require.False(t, isRouteNotExist(unix.EPERM))
+
+	require.True(t, isLinkNotExist(fmt.Errorf("link delete failed: %w", unix.ENODEV)))
+	require.False(t, isLinkNotExist(unix.EPERM))
+
+	require.True(t, isNamespaceNotExist(&os.PathError{Op: "remove", Path: "/var/run/netns/missing", Err: unix.ENOENT}))
+	require.False(t, isNamespaceNotExist(unix.EPERM))
+}