Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error deleting TCPMSS clamp rule in route agent #3187

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

tpantelis
Copy link
Contributor

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 appending 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.

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr3187/tpantelis/iptables_mss_clamp
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Oct 17, 2024
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 <tompantelis@gmail.com>
@vthapar vthapar force-pushed the iptables_mss_clamp branch from 35b7676 to 63aa202 Compare October 17, 2024 07:53
@tpantelis tpantelis added the backport This change requires a backport to eligible release branches label Oct 17, 2024
@tpantelis tpantelis enabled auto-merge (rebase) October 17, 2024 12:51
@tpantelis tpantelis merged commit 3d249ba into submariner-io:devel Oct 17, 2024
54 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr3187/tpantelis/iptables_mss_clamp]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport This change requires a backport to eligible release branches backport-handled ready-to-test When a PR is ready for full E2E testing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants