refactor(iptables): make rule insertion functions idempotent

Refactored PREROUTING DNAT, POSTROUTING MASQUERADE, and FORWARD ACCEPT rule insertion to first check for existing rules before inserting. This prevents duplicate rules when run multiple times and improves reliability of firewall configuration.

network-go/iptables/iptables.go

@@ -253,13 +253,18 @@ func (m *Manager) deleteMatchingLinesInContainer(pid int, table, chain string, g
 
 // InsertPreroutingRule inserts a DNAT PREROUTING rule on the host
 func (m *Manager) InsertPreroutingRule(sourceIP, proto, sourcePort, targetIP, targetPort, comment string) error {
-	logger.Info("IPTABLES: inserting PREROUTING DNAT rule: src=%s proto=%s sport=%s -> dst=%s dport=%s comment=%q",
+	logger.Info("IPTABLES: checking PREROUTING DNAT rule: src=%s proto=%s sport=%s -> dst=%s dport=%s comment=%q",
 		sourceIP, proto, sourcePort, targetIP, targetPort, comment)
-	patterns := []string{"DNAT", sourcePort, targetIP, targetPort, comment}
-	if err := m.deleteMatchingLines("PREROUTING", "nat", patterns...); err != nil {
-		return fmt.Errorf("failed to delete old PREROUTING rules: %w", err)
+
+	// Idempotent: check if rule already exists
+	existing := m.getLineNumbers("PREROUTING", "nat", comment, "DNAT", targetIP, targetPort)
+	if len(existing) > 0 {
+		logger.Debug("IPTABLES: PREROUTING DNAT rule already exists (lines=%v), skipping", existing)
+		return nil
 	}
 
+	logger.Info("IPTABLES: inserting PREROUTING DNAT rule: src=%s proto=%s sport=%s -> dst=%s dport=%s comment=%q",
+		sourceIP, proto, sourcePort, targetIP, targetPort, comment)
 	args := []string{
 		"-w", "-t", "nat", "-I", "PREROUTING",
 		"-d", sourceIP,
@@ -323,13 +328,18 @@ func (m *Manager) InsertPostroutingMasquerade(sourceCIDR, proto, sourcePort, com
 
 // InsertPostroutingMasqueradeForTarget inserts a MASQUERADE POSTROUTING rule for a target
 func (m *Manager) InsertPostroutingMasqueradeForTarget(targetCIDR, proto, targetPort, comment string) error {
-	logger.Info("IPTABLES: inserting POSTROUTING MASQUERADE rule for target: dst=%s proto=%s dport=%s comment=%q",
+	logger.Info("IPTABLES: checking POSTROUTING MASQUERADE rule for target: dst=%s proto=%s dport=%s comment=%q",
 		targetCIDR, proto, targetPort, comment)
-	patterns := []string{"MASQUERADE", comment, targetCIDR, targetPort}
-	if err := m.deleteMatchingLines("POSTROUTING", "nat", patterns...); err != nil {
-		return fmt.Errorf("failed to delete old POSTROUTING rules: %w", err)
+
+	// Idempotent: check if rule already exists
+	existing := m.getLineNumbers("POSTROUTING", "nat", comment, "MASQUERADE", targetCIDR)
+	if len(existing) > 0 {
+		logger.Debug("IPTABLES: POSTROUTING MASQUERADE for target already exists (lines=%v), skipping", existing)
+		return nil
 	}
 
+	logger.Info("IPTABLES: inserting POSTROUTING MASQUERADE rule for target: dst=%s proto=%s dport=%s comment=%q",
+		targetCIDR, proto, targetPort, comment)
 	args := []string{
 		"-w", "-t", "nat", "-I", "POSTROUTING",
 		"-d", targetCIDR,
@@ -343,10 +353,12 @@ func (m *Manager) InsertPostroutingMasqueradeForTarget(targetCIDR, proto, target
 
 // InsertForwardAccept inserts a FORWARD ACCEPT rule on the host
 func (m *Manager) InsertForwardAccept(chain, sourceIP, targetIP, proto, sourcePort, targetPort, comment string) error {
-	logger.Info("IPTABLES: inserting FORWARD ACCEPT rule: chain=%s src=%s dst=%s proto=%s sport=%s dport=%s comment=%q",
+	logger.Info("IPTABLES: checking FORWARD ACCEPT rule: chain=%s src=%s dst=%s proto=%s sport=%s dport=%s comment=%q",
 		chain, sourceIP, targetIP, proto, sourcePort, targetPort, comment)
+
+	// Idempotent: check if rule already exists
 	var grepPatterns []string
-	grepPatterns = append(grepPatterns, proto)
+	grepPatterns = append(grepPatterns, comment, proto)
 	if sourceIP != "" {
 		grepPatterns = append(grepPatterns, sourceIP)
 	}
@@ -359,11 +371,14 @@ func (m *Manager) InsertForwardAccept(chain, sourceIP, targetIP, proto, sourcePo
 	if targetPort != "" {
 		grepPatterns = append(grepPatterns, targetPort)
 	}
-
-	if err := m.deleteMatchingLines(chain, "", grepPatterns...); err != nil {
-		return fmt.Errorf("failed to delete old FORWARD rules: %w", err)
+	existing := m.getLineNumbers(chain, "", grepPatterns...)
+	if len(existing) > 0 {
+		logger.Debug("IPTABLES: FORWARD ACCEPT rule already exists in %s (lines=%v), skipping", chain, existing)
+		return nil
 	}
 
+	logger.Info("IPTABLES: inserting FORWARD ACCEPT rule: chain=%s src=%s dst=%s proto=%s sport=%s dport=%s comment=%q",
+		chain, sourceIP, targetIP, proto, sourcePort, targetPort, comment)
 	args := []string{"-w", "-I", chain, "-p", proto}
 	if sourceIP != "" {
 		args = append(args, "-s", sourceIP)
@@ -397,31 +412,17 @@ func (m *Manager) DeleteForwardAccept(chain, comment string) error {
 	return nil
 }
 
-// lineExistsInContainer checks if a line matching the patterns exists inside a container namespace
-func (m *Manager) lineExistsInContainer(pid int, table, chain string, patterns ...string) bool {
+// checkContainerChainExists lists rules inside a container namespace and returns
+// the output, or an error if nsenter/iptables fails inside the container.
+func (m *Manager) checkContainerChainExists(pid int, table, chain string) (string, error) {
 	iptPath := m.binary
-
 	nsenterArgs := []string{"-t", fmt.Sprintf("%d", pid), "-n", "--", iptPath, "-w", "-n", "-t", table, "-L", chain}
 	cmd := exec.Command("nsenter", nsenterArgs...)
-	output, err := cmd.Output()
+	output, err := cmd.CombinedOutput()
 	if err != nil {
-		logger.Debug("IPTABLES: lineExistsInContainer list failed for PID %d chain %s: %v", pid, chain, err)
-		return false
+		return "", fmt.Errorf("nsenter iptables list failed for PID %d chain %s/%s: %w (output: %s)", pid, table, chain, err, strings.TrimSpace(string(output)))
 	}
-
-	for _, line := range strings.Split(string(output), "\n") {
-		matchesAll := true
-		for _, pattern := range patterns {
-			if !strings.Contains(line, pattern) {
-				matchesAll = false
-				break
-			}
-		}
-		if matchesAll {
-			return true
-		}
-	}
-	return false
+	return string(output), nil
 }
 
 // InsertPreroutingRuleInContainer inserts a DNAT PREROUTING rule inside a container namespace
@@ -429,16 +430,34 @@ func (m *Manager) InsertPreroutingRuleInContainer(pid int, sourceIP, proto, sour
 	logger.Info("IPTABLES: inserting PREROUTING DNAT rule in container PID %d: src=%s proto=%s dport=%s -> %s:%s comment=%q",
 		pid, sourceIP, proto, sourcePort, targetIP, targetPort, comment)
 
-	// Idempotent check: if the rule already exists, skip
-	if m.lineExistsInContainer(pid, "nat", "PREROUTING", "DNAT", sourcePort, targetIP, comment) {
-		logger.Debug("IPTABLES: PREROUTING DNAT rule already exists in container PID %d (pattern=%s -> %s), skipping", pid, sourcePort, targetIP)
+	// First, try to list the chain inside the container to check state
+	output, err := m.checkContainerChainExists(pid, "nat", "PREROUTING")
+	if err != nil {
+		// Cannot inspect container iptables — return error so caller falls back to host
+		logger.Warn("IPTABLES: cannot check container PREROUTING (PID %d): %v", pid, err)
+		return fmt.Errorf("cannot check container iptables: %w", err)
+	}
+
+	// Idempotent check: scan existing rules for matching patterns
+	ruleExists := false
+	for _, line := range strings.Split(output, "\n") {
+		if strings.Contains(line, "DNAT") &&
+			strings.Contains(line, sourcePort) &&
+			strings.Contains(line, targetIP) &&
+			strings.Contains(line, comment) {
+			ruleExists = true
+			break
+		}
+	}
+	if ruleExists {
+		logger.Info("IPTABLES: PREROUTING DNAT rule already exists in container PID %d (dport=%s -> %s), skipping", pid, sourcePort, targetIP)
 		return nil
 	}
 
-	// Clean up any stale/duplicate rules first
+	// Rule doesn't exist — clean up stale/duplicate rules then insert
 	patterns := []string{"DNAT", sourcePort, targetIP, targetPort, comment}
-	if err := m.deleteMatchingLinesInContainer(pid, "nat", "PREROUTING", patterns...); err != nil {
-		logger.Warn("IPTABLES: failed to delete old container PREROUTING rules: %v (continuing)", err)
+	if delErr := m.deleteMatchingLinesInContainer(pid, "nat", "PREROUTING", patterns...); delErr != nil {
+		logger.Debug("IPTABLES: stale PREROUTING cleanup in container PID %d: %v", pid, delErr)
 	}
 
 	args := []string{
@@ -457,16 +476,32 @@ func (m *Manager) InsertPostroutingMasqueradeInContainer(pid int, sourceCIDR, pr
 	logger.Info("IPTABLES: inserting POSTROUTING MASQUERADE rule in container PID %d: src=%s proto=%s sport=%s comment=%q",
 		pid, sourceCIDR, proto, sourcePort, comment)
 
-	// Idempotent check: if the rule already exists, skip
-	if m.lineExistsInContainer(pid, "nat", "POSTROUTING", "MASQUERADE", comment, sourceCIDR) {
-		logger.Debug("IPTABLES: POSTROUTING MASQUERADE rule already exists in container PID %d (src=%s comment=%s), skipping", pid, sourceCIDR, comment)
+	// First, try to list the chain inside the container to check state
+	output, err := m.checkContainerChainExists(pid, "nat", "POSTROUTING")
+	if err != nil {
+		logger.Warn("IPTABLES: cannot check container POSTROUTING (PID %d): %v", pid, err)
+		return fmt.Errorf("cannot check container iptables: %w", err)
+	}
+
+	// Idempotent check: scan existing rules for matching patterns
+	ruleExists := false
+	for _, line := range strings.Split(output, "\n") {
+		if strings.Contains(line, "MASQUERADE") &&
+			strings.Contains(line, comment) &&
+			strings.Contains(line, sourceCIDR) {
+			ruleExists = true
+			break
+		}
+	}
+	if ruleExists {
+		logger.Info("IPTABLES: POSTROUTING MASQUERADE rule already exists in container PID %d (src=%s), skipping", pid, sourceCIDR)
 		return nil
 	}
 
-	// Clean up any stale/duplicate rules first
+	// Rule doesn't exist — clean up stale/duplicate rules then insert
 	patterns := []string{"MASQUERADE", comment, sourceCIDR, sourcePort}
-	if err := m.deleteMatchingLinesInContainer(pid, "nat", "POSTROUTING", patterns...); err != nil {
-		logger.Warn("IPTABLES: failed to delete old container POSTROUTING rules: %v (continuing)", err)
+	if delErr := m.deleteMatchingLinesInContainer(pid, "nat", "POSTROUTING", patterns...); delErr != nil {
+		logger.Debug("IPTABLES: stale POSTROUTING cleanup in container PID %d: %v", pid, delErr)
 	}
 
 	args := []string{