From ae7612b1a6d4cfae95dcbc42d267f17602e340c9 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Wed, 16 Oct 2024 20:11:04 -0400 Subject: [PATCH] Fix error deleting TCPMSS clamp rule in route agent This warning was observed on route agent restart: 2024-10-15T18:30:34.752Z WRN ..etfilter/adapter.go:120 Packetfilter Unable to delete rule "packetfilter.Rule{Action: MSS, SrcCIDR: 172.31.0.0/16}" from table "Filter", chain "SUBMARINER-FWD-MSSCLAMP": error deleting rule "-s 172.31.0.0/16 -j TCPMSS" from table "filter", chain "SUBMARINER-FWD-MSSCLAMP": running [/usr/sbin/iptables -t filter -D SUBMARINER-FWD-MSSCLAMP -s 172.31.0.0/16 -j TCPMSS --wait 5]: exit status 2: iptables v1.8.8 (nf_tables): TCPMSS target: At least one parameter is required Try `iptables -h' or 'iptables --help' for more information. The problem is that we're not specifying either "--clamp-mss-to-pmtu" or "--set-mss" after "-j TCPMSS". This is due to incorrect parsing of the rule string returned from the iptables command. We're expecting "-p tcp -m tcp --tcp-flags SYN,RST SYN" to be right after "-j TCPMSS" the same as we write it out when appeneding but iptables returns the parameters in a different order with "--clamp-mss-to-pmtu" or "--set-mss" right after "-j TCPMSS", eg "-p tcp -m tcp --tcp-flags SYN,RST SYN -j TCPMSS --set-mss 1500" So we miss parsing the TCPMSS parameter and thus don't set the MssClampType field correctly. Modify the parsing to handle the parameters in any order. Signed-off-by: Tom Pantelis --- pkg/packetfilter/iptables/iptables.go | 35 +++++----------------- pkg/packetfilter/iptables/iptables_test.go | 27 +++++++++++++++-- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/pkg/packetfilter/iptables/iptables.go b/pkg/packetfilter/iptables/iptables.go index b580b7fdf..0a862872f 100644 --- a/pkg/packetfilter/iptables/iptables.go +++ b/pkg/packetfilter/iptables/iptables.go @@ -251,9 +251,9 @@ func mssClampToRuleSpec(ruleSpec *RuleSpec, clampType packetfilter.MssClampType, switch clampType { case packetfilter.UndefinedMSS: case packetfilter.ToPMTU: - *ruleSpec = append(*ruleSpec, "-p", "tcp", "-m", "tcp", "--tcp-flags", "SYN,RST", "SYN", "--clamp-mss-to-pmtu") + *ruleSpec = append(*ruleSpec, "--clamp-mss-to-pmtu", "-p", "tcp", "-m", "tcp", "--tcp-flags", "SYN,RST", "SYN") case packetfilter.ToValue: - *ruleSpec = append(*ruleSpec, "-p", "tcp", "-m", "tcp", "--tcp-flags", "SYN,RST", "SYN", "--set-mss", mssValue) + *ruleSpec = append(*ruleSpec, "--set-mss", mssValue, "-p", "tcp", "-m", "tcp", "--tcp-flags", "SYN,RST", "SYN") } } @@ -322,6 +322,7 @@ func ToRuleSpec(rule *packetfilter.Rule) RuleSpec { return ruleSpec } +//nolint:gocyclo // This function has a lot of small case statements so ignore cyclomatic complexity. func FromRuleSpec(spec []string) *packetfilter.Rule { rule := &packetfilter.Rule{} @@ -350,6 +351,11 @@ func FromRuleSpec(spec []string) *packetfilter.Rule { rule.DPort, i = parseNextTerm(spec, i, noopParse) case "--set-mark": rule.MarkValue, i = parseNextTerm(spec, i, parseMark) + case "--clamp-mss-to-pmtu": + rule.ClampType = packetfilter.ToPMTU + case "--set-mss": + rule.MssValue, i = parseNextTerm(spec, i, noopParse) + rule.ClampType = packetfilter.ToValue case "-j": rule.Action, i = parseNextTerm(spec, i, parseAction) if rule.Action == packetfilter.RuleActionJump { @@ -400,31 +406,6 @@ func parseRuleMatch(spec []string, i int, rule *packetfilter.Rule) int { i += 3 } - case "tcp": - // Parses the form: "-m", "tcp", "--tcp-flags", "SYN,RST", "SYN", "--clamp-mss-to-pmtu" - i = parseTCPSpec(spec, i, rule) - } - - return i -} - -func parseTCPSpec(spec []string, i int, rule *packetfilter.Rule) int { - i++ - for i < len(spec) { - if spec[i] == "--clamp-mss-to-pmtu" { - rule.ClampType = packetfilter.ToPMTU - break - } else if spec[i] == "--set-mss" { - rule.MssValue, i = parseNextTerm(spec, i, noopParse) - rule.ClampType = packetfilter.ToValue - - break - } else if !strings.HasPrefix(spec[i], "--") && strings.HasPrefix(spec[i], "-") { - i-- - break - } - - i++ } return i diff --git a/pkg/packetfilter/iptables/iptables_test.go b/pkg/packetfilter/iptables/iptables_test.go index d82c3bff9..bf8924626 100644 --- a/pkg/packetfilter/iptables/iptables_test.go +++ b/pkg/packetfilter/iptables/iptables_test.go @@ -19,6 +19,8 @@ limitations under the License. package iptables_test import ( + "strings" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/submariner-io/submariner/pkg/packetfilter" @@ -27,7 +29,7 @@ import ( var _ = Describe("Rule conversion", func() { Specify("should correctly convert to and from a rule spec string", func() { - // -m set --match-set src-set src -m set --match-set dest-set dst -j TCPMSS -p tcp -m tcp --tcp-flags SYN,RST SYN --clamp-mss-to-pmtu + // -m set --match-set src-set src -m set --match-set dest-set dst -j TCPMSS --clamp-mss-to-pmtu -p tcp -m tcp --tcp-flags SYN,RST SYN testRuleConversion(&packetfilter.Rule{ SrcSetName: "src-set", DestSetName: "dest-set", @@ -35,15 +37,23 @@ var _ = Describe("Rule conversion", func() { ClampType: packetfilter.ToPMTU, }) - // -m set --match-set src-set src -m set --match-set dest-set dst -j TCPMSS -p tcp -m tcp --tcp-flags SYN,RST SYN --set-mss mss-value + // -m set --match-set src-set src -m set --match-set dest-set dst -j TCPMSS --set-mss mss-value -p tcp -m tcp --tcp-flags SYN,RST SYN testRuleConversion(&packetfilter.Rule{ SrcSetName: "src-set", DestSetName: "dest-set", - MssValue: "mss-value", + MssValue: "1500", Action: packetfilter.RuleActionMss, ClampType: packetfilter.ToValue, }) + // -s 1.2.3.4/32 -j TCPMSS --set-mss mss-value -p tcp -m tcp --tcp-flags SYN,RST SYN + testRuleConversion(&packetfilter.Rule{ + SrcCIDR: "1.2.3.4/32", + MssValue: "1500", + Action: packetfilter.RuleActionMss, + ClampType: packetfilter.ToValue, + }) + // -p udp -m udp -s 171.254.1.0/24 -d 170.254.1.0/24 -o out-iface -i in-iface --dport d-port -j ACCEPT testRuleConversion(&packetfilter.Rule{ Proto: packetfilter.RuleProtoUDP, @@ -94,6 +104,17 @@ var _ = Describe("Rule conversion", func() { TargetChain: "target-chain", Action: packetfilter.RuleActionJump, }) + + // The actual iptables command returns the TCPMSS rule parts in a different order than we write it out so ensure we + // can parse it correctly. + rs := "-s 1.2.3.4/32 -p tcp -m tcp --tcp-flags SYN,RST SYN -j TCPMSS --set-mss 1500" + parsed := iptables.FromRuleSpec(strings.Split(rs, " ")) + Expect(parsed).To(Equal(&packetfilter.Rule{ + SrcCIDR: "1.2.3.4/32", + Action: packetfilter.RuleActionMss, + ClampType: packetfilter.ToValue, + MssValue: "1500", + })) }) })