From eab0bb4a9483dd0ced39e934fdf171f9515fd5ac Mon Sep 17 00:00:00 2001 From: ssongliu Date: Wed, 2 Sep 2026 14:48:47 +0800 Subject: [PATCH] fix: repair firewall forwarding migration (#13689) --- agent/init/firewall/firewall.go | 7 +- .../migrations/utils/firewall_transfer.go | 107 +++++++++++------ .../firewall/forwarding/providers/iptables.go | 111 ++++++++++++++++-- 3 files changed, 178 insertions(+), 47 deletions(-) diff --git a/agent/init/firewall/firewall.go b/agent/init/firewall/firewall.go index ea9c4eabc..b92bc466c 100644 --- a/agent/init/firewall/firewall.go +++ b/agent/init/firewall/firewall.go @@ -37,6 +37,9 @@ func Init() { global.LOG.Errorf("transfer legacy forwarding rules failed, err: %v", err) return } + if err := initForwardingRules(ctx); err != nil { + global.LOG.Warnf("restore forwarding rules failed, manual synchronization is available, err: %v", err) + } if !needInit() { repairIptablesIPv6BaseChains(clientName) return @@ -44,10 +47,6 @@ func Init() { defer initDockerPortGuard(ctx) InitPingStatus() global.LOG.Info("initializing firewall settings...") - if err := initForwardingRules(ctx); err != nil { - global.LOG.Errorf("restore forwarding rules failed, err: %v", err) - return - } if clientName == "nftables" { if err := nftables_helper.Restore(); err != nil { global.LOG.Errorf("restore nftables rules failed, err: %v", err) diff --git a/agent/init/migration/migrations/utils/firewall_transfer.go b/agent/init/migration/migrations/utils/firewall_transfer.go index 61460dd92..dceb5ebeb 100644 --- a/agent/init/migration/migrations/utils/firewall_transfer.go +++ b/agent/init/migration/migrations/utils/firewall_transfer.go @@ -25,6 +25,11 @@ type legacyFirewalldForward struct { spec string } +type legacyFirewalldForwardFailure struct { + spec string + err error +} + type firewallTransferSource struct { rules []forwarding.Rule firewalld []legacyFirewalldForward @@ -159,16 +164,14 @@ func loadLegacyFirewallForwarding() (firewallTransferSource, error) { if err != nil { return firewallTransferSource{}, err } - for _, item := range firewalldRules { - if _, err := forwarding.NormalizeRule(item.rule); err != nil { - if global.LOG != nil { - global.LOG.Warnf("skip unsupported legacy firewalld forwarding rule %q: %v", item.spec, err) - } - continue + supportedRules, cleanupRules, failures := selectSupportedLegacyFirewalldForwarding(firewalldRules) + for _, failure := range failures { + if global.LOG != nil { + global.LOG.Warnf("skip unsupported legacy firewalld forwarding rule %q: %v", failure.spec, failure.err) } - source.rules = append(source.rules, item.rule) - source.firewalld = append(source.firewalld, item) } + source.rules = append(source.rules, supportedRules...) + source.firewalld = append(source.firewalld, cleanupRules...) if len(source.rules) > 0 { source.provider = "iptables" } @@ -185,6 +188,23 @@ func loadLegacyFirewallForwarding() (firewallTransferSource, error) { return source, nil } +func selectSupportedLegacyFirewalldForwarding(items []legacyFirewalldForward) ( + []forwarding.Rule, []legacyFirewalldForward, []legacyFirewalldForwardFailure, +) { + rules := make([]forwarding.Rule, 0, len(items)) + cleanup := make([]legacyFirewalldForward, 0, len(items)) + failures := make([]legacyFirewalldForwardFailure, 0) + for _, item := range items { + if _, err := forwarding.NormalizeRule(item.rule); err != nil { + failures = append(failures, legacyFirewalldForwardFailure{spec: item.spec, err: err}) + continue + } + rules = append(rules, item.rule) + cleanup = append(cleanup, item) + } + return rules, cleanup, failures +} + func listLegacyIptablesForwarding() ([]forwarding.Rule, error) { exists, err := iptables_helper.CheckChainExist(iptables_helper.NatTab, forwarding.ChainPreRouting) if err != nil { @@ -261,40 +281,55 @@ func listLegacyFirewalldForwarding() ([]legacyFirewalldForward, error) { if err != nil { return nil, fmt.Errorf("list legacy firewalld forwarding rules: %w", err) } - return parseLegacyFirewalldForwarding(stdout), nil + rules, failures := parseLegacyFirewalldForwarding(stdout) + for _, failure := range failures { + if global.LOG != nil { + global.LOG.Warnf("skip unsupported legacy firewalld forwarding rule %q: %v", failure.spec, failure.err) + } + } + return rules, nil } -func parseLegacyFirewalldForwarding(stdout string) []legacyFirewalldForward { +func parseLegacyFirewalldForwarding(stdout string) ([]legacyFirewalldForward, []legacyFirewalldForwardFailure) { result := make([]legacyFirewalldForward, 0) - for _, line := range strings.Split(stdout, "\n") { - spec := strings.TrimSpace(line) - if !strings.HasPrefix(spec, "port=") { + failures := make([]legacyFirewalldForwardFailure, 0) + for _, spec := range strings.Fields(stdout) { + item, err := parseLegacyFirewalldForward(spec) + if err != nil { + failures = append(failures, legacyFirewalldForwardFailure{spec: spec, err: err}) continue } - port, rest, ok := strings.Cut(strings.TrimPrefix(spec, "port="), ":proto=") - if !ok { - continue - } - protocol, rest, ok := strings.Cut(rest, ":toport=") - if !ok { - continue - } - targetPort, targetIP, ok := strings.Cut(rest, ":toaddr=") - if !ok { - continue - } - if targetIP == "" { - targetIP = "127.0.0.1" - } - result = append(result, legacyFirewalldForward{ - rule: forwarding.Rule{ - Family: forwarding.FamilyIPv4, Protocol: protocol, Port: port, - TargetIP: targetIP, TargetPort: targetPort, - }, - spec: spec, - }) + result = append(result, item) } - return result + return result, failures +} + +func parseLegacyFirewalldForward(spec string) (legacyFirewalldForward, error) { + if !strings.HasPrefix(spec, "port=") { + return legacyFirewalldForward{}, errors.New("missing port field") + } + port, rest, ok := strings.Cut(strings.TrimPrefix(spec, "port="), ":proto=") + if !ok { + return legacyFirewalldForward{}, errors.New("missing protocol field") + } + protocol, rest, ok := strings.Cut(rest, ":toport=") + if !ok { + return legacyFirewalldForward{}, errors.New("missing target port field") + } + targetPort, targetIP, ok := strings.Cut(rest, ":toaddr=") + if !ok { + return legacyFirewalldForward{}, errors.New("missing target address field") + } + if targetIP == "" { + targetIP = "127.0.0.1" + } + return legacyFirewalldForward{ + rule: forwarding.Rule{ + Family: forwarding.FamilyIPv4, Protocol: protocol, Port: port, + TargetIP: targetIP, TargetPort: targetPort, + }, + spec: spec, + }, nil } func cleanupLegacyFirewalldForwarding(rules []legacyFirewalldForward) error { diff --git a/agent/utils/firewall/forwarding/providers/iptables.go b/agent/utils/firewall/forwarding/providers/iptables.go index 58f7b6d74..8b848a203 100644 --- a/agent/utils/firewall/forwarding/providers/iptables.go +++ b/agent/utils/firewall/forwarding/providers/iptables.go @@ -149,6 +149,9 @@ func (l *iptablesNATAdapter) Reconcile(rules []forwarding.Rule) error { if family == forwarding.FamilyIPv6 && !l.backend.IPv6Available() { continue } + if err := l.batchEnsureChains(family); err != nil { + return err + } script, err := buildIptablesForwardRestoreScript(byFamily[family]) if err != nil { return err @@ -315,29 +318,52 @@ func buildIptablesForwardLifecycleScript(outputs map[string]string, create bool) items := []struct{ table, parent, chain string }{ {iptables_helper.NatTab, "PREROUTING", forwarding.ChainPreRouting}, {iptables_helper.NatTab, "POSTROUTING", forwarding.ChainPostRouting}, - {iptables_helper.FilterTab, "FORWARD", forwarding.ChainForward}, } byTable := make(map[string][]string, 2) for _, item := range items { output := outputs[item.table] chainExists := containsExactLine(output, "-N "+item.chain) - bindingExists := containsExactLine(output, "-A "+item.parent+" -j "+item.chain) + binding := "-A " + item.parent + " -j " + item.chain + bindingCount := countExactLines(output, binding) if create { if !chainExists { byTable[item.table] = append(byTable[item.table], "-N "+item.chain) } - if !bindingExists { + if bindingCount == 0 { byTable[item.table] = append(byTable[item.table], "-A "+item.parent+" -j "+item.chain) } continue } - if bindingExists { + for range bindingCount { byTable[item.table] = append(byTable[item.table], "-D "+item.parent+" -j "+item.chain) } if chainExists { byTable[item.table] = append(byTable[item.table], "-F "+item.chain, "-X "+item.chain) } } + + filterOutput := outputs[iptables_helper.FilterTab] + filterChainExists := containsExactLine(filterOutput, "-N "+forwarding.ChainForward) + filterBinding := "-A FORWARD -j " + forwarding.ChainForward + filterBindingCount := countExactLines(filterOutput, filterBinding) + if create { + if !filterChainExists { + byTable[iptables_helper.FilterTab] = append(byTable[iptables_helper.FilterTab], "-N "+forwarding.ChainForward) + } + if !forwardBindingEffective(filterOutput) { + for range filterBindingCount { + byTable[iptables_helper.FilterTab] = append(byTable[iptables_helper.FilterTab], "-D FORWARD -j "+forwarding.ChainForward) + } + byTable[iptables_helper.FilterTab] = append(byTable[iptables_helper.FilterTab], canonicalForwardBindingRule(filterOutput)) + } + } else { + for range filterBindingCount { + byTable[iptables_helper.FilterTab] = append(byTable[iptables_helper.FilterTab], "-D FORWARD -j "+forwarding.ChainForward) + } + if filterChainExists { + byTable[iptables_helper.FilterTab] = append(byTable[iptables_helper.FilterTab], "-F "+forwarding.ChainForward, "-X "+forwarding.ChainForward) + } + } var script strings.Builder for _, table := range []string{iptables_helper.NatTab, iptables_helper.FilterTab} { lines := byTable[table] @@ -353,6 +379,68 @@ func buildIptablesForwardLifecycleScript(outputs map[string]string, create bool) return script.String() } +func countExactLines(output, want string) int { + count := 0 + for _, line := range strings.Split(output, "\n") { + if strings.TrimSpace(line) == want { + count++ + } + } + return count +} + +func forwardBindingEffective(output string) bool { + binding := "-A FORWARD -j " + forwarding.ChainForward + bindingPosition := 0 + terminalPosition := 0 + position := 0 + bindings := 0 + for _, line := range strings.Split(output, "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "-A FORWARD ") { + continue + } + position++ + if line == binding { + bindings++ + bindingPosition = position + } + if terminalPosition == 0 && isUnconditionalForwardTerminal(line) { + terminalPosition = position + } + } + return bindings == 1 && (terminalPosition == 0 || bindingPosition < terminalPosition) +} + +func canonicalForwardBindingRule(output string) string { + binding := "-A FORWARD -j " + forwarding.ChainForward + position := 1 + for _, line := range strings.Split(output, "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "-A FORWARD ") || line == binding { + continue + } + if isUnconditionalForwardTerminal(line) { + return fmt.Sprintf("-I FORWARD %d -j %s", position, forwarding.ChainForward) + } + position++ + } + return "-A FORWARD -j " + forwarding.ChainForward +} + +func isUnconditionalForwardTerminal(line string) bool { + fields := strings.Fields(line) + if len(fields) < 4 || fields[0] != "-A" || fields[1] != "FORWARD" || fields[2] != "-j" { + return false + } + switch fields[3] { + case "ACCEPT", "DROP", "REJECT", "RETURN": + return true + default: + return false + } +} + func containsExactLine(output, want string) bool { for _, line := range strings.Split(output, "\n") { if strings.TrimSpace(line) == want { @@ -444,12 +532,13 @@ func (l *iptablesNATAdapter) familyInitStatus(family string) (bool, bool, error) if err != nil { return false, false, fmt.Errorf("list %s filter initialization rules: %w", label, err) } - filterInit, filterBind := checkInitAndBind( + filterInit, _ := checkInitAndBind( []string{"-N " + forwarding.ChainForward}, - []string{"-A FORWARD -j " + forwarding.ChainForward}, + nil, strings.Split(filterRules, "\n"), ) - return natInit && filterInit, forwardingEnabled && natBind && filterBind, nil + filterBind := forwardBindingEffective(filterRules) + return natInit && filterInit, forwardingEnabled && natBind && filterInit && filterBind, nil } func (l *iptablesNATAdapter) FamilyStatus(family string) (bool, bool, error) { @@ -483,6 +572,14 @@ func containsExactRule(lines []string, rule string) bool { } func (l *iptablesNATAdapter) Replay() error { + for _, family := range []string{forwarding.FamilyIPv4, forwarding.FamilyIPv6} { + if family == forwarding.FamilyIPv6 && !l.backend.IPv6Available() { + continue + } + if err := l.batchEnsureChains(family); err != nil { + return err + } + } for _, item := range []struct { table string chain string