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 <[email protected]>

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

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,23 +29,31 @@ 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",
 			Action:      packetfilter.RuleActionMss,
 			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",
+		}))
 	})
 })