Conversation
qdm12
force-pushed
the
conntrack
branch
3 times, most recently
from
February 26, 2026 19:56
efe0cf6 to
02d2ed6
Compare
qdm12
force-pushed
the
conntrack
branch
2 times, most recently
from
February 26, 2026 20:52
c2a8ef9 to
39800c7
Compare
… requirement since some systems don't show what they support reliably
qdm12
force-pushed
the
master
branch
2 times, most recently
from
March 16, 2026 13:48
2c06921 to
9a5995f
Compare
qdm12
force-pushed
the
master
branch
4 times, most recently
from
May 3, 2026 04:29
40f126b to
44d5104
Compare
On older kernels (4.4.x), the conntrack netlink delete message is not supported, so the flush fails with a raw netlink error like "netlink receive: invalid argument", even when the module probe succeeds. That error used to abort the firewall enabling, preventing gluetun from starting (#3152). flushExistingConnections is now fully best-effort: any try failure falls back to the next one, and if all tries fail, a warning is logged instead of an error being returned, since killing the existing connections is an optimization, not a requirement for the firewall to function. Also: - add Warnf to the firewall Logger interface - fix the stale doc comment on AcceptOutputPublicOnlyNewTraffic - fix the broken firewall.NewConfig call in the pmtud tcp integration test - add unit tests for the fallback behavior
Master refactored the iptables package (single mutex, inline errors, removed sentinels) after the conntrack branch forked, so the merge left conntrack's code referencing removed symbols: - AcceptOutputPublicOnlyNewTraffic locked the removed ip6tablesMutex (master unified locking on a single iptablesMutex) - parse.go and list.go referenced the removed ErrIptablesCommandMalformed and ErrChainRuleMalformed sentinels; inlined the errors in master's style (final messages unchanged) - pmtud now checks both ErrKernelModuleMissing and ErrMarkMatchModuleMissing, so TCP PMTUD gracefully aborts whether the mark module is missing at the libxt_mark.so stat precheck or at iptables runtime - fix the stale 6-arg firewall.NewConfig call in pmtud_integration_test.go, which master added after the branch forked - fix stale ip6tablesMutex comments in atomic.go
There was a problem hiding this comment.
Pull request overview
This PR moves conntrack cleanup into firewall setup and adds iptables fallbacks when netlink flushing is unavailable.
Changes:
- Adds conntrack probing and
AF_UNSPECflushing. - Adds iptables marking, reject/drop fallbacks, and parser support.
- Updates firewall wiring, PMTUD handling, and tests.
Review blockers (critical):
iptables.gomust return unclassified IPv4 setup errors instead of falling through to the IPv6 no-op.- Replace or safely manage the global
PUBLIC_ONLYchain name. - Return
ErrConntrackNetlinkNotSupportedon unsupported platforms instead of panicking. - Avoid unconditionally calling the panic-prone non-Linux
mod.Probe.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Summary |
|---|---|
internal/pmtud/tcp/tcp_integration_test.go |
Updates firewall construction and skip diagnostics. |
internal/pmtud/tcp/mss.go |
Handles missing kernel modules. |
internal/pmtud/tcp/helpers_test.go |
Updates test firewall construction. |
internal/pmtud/pmtud.go |
Handles kernel-module failures. |
internal/pmtud/pmtud_integration_test.go |
Updates firewall construction. |
internal/netlink/netlink.go |
Probes conntrack netlink support. |
internal/netlink/conntrack_unspecified.go |
Defines unsupported-platform conntrack behavior. |
internal/netlink/conntrack_linux.go |
Updates conntrack flushing. |
internal/firewall/iptables/tcp.go |
Refines mark-module error handling. |
internal/firewall/iptables/parse.go |
Parses new iptables operations and targets. |
internal/firewall/iptables/parse_test.go |
Updates parser expectations. |
internal/firewall/iptables/list.go |
Parses connmark and reject rules. |
internal/firewall/iptables/iptables.go |
Adds fallback firewall rules. |
internal/firewall/iptables/ip6tables.go |
Handles missing kernel modules. |
internal/firewall/iptables/firewall.go |
Adds a kernel-module error sentinel. |
internal/firewall/iptables/atomic.go |
Updates mutex documentation. |
internal/firewall/interfaces.go |
Extends firewall and logger interfaces. |
internal/firewall/flush.go |
Implements connection-cleanup fallback orchestration. |
internal/firewall/flush_test.go |
Tests fallback orchestration. |
internal/firewall/firewall.go |
Injects the netlink dependency. |
internal/firewall/enable.go |
Runs connection cleanup during enablement. |
cmd/gluetun/main.go |
Wires netlink into firewall setup. |
Suppressed comments (3)
internal/firewall/iptables/iptables.go:259
- These exceptions only recognize RFC1918/ULA/link-local/loopback, but
routing.LocalNetworkis built from arbitrary directly connected route prefixes (including globally routed IPv6 LANs). When conntrack flushing fails and the permanent mark fallback is installed, an existing connection to such a configured local network hitsPUBLIC_ONLYand is dropped before the later local-subnet ACCEPT rule. Include the configured local prefixes in the exceptions.
ipv6PrivatePrefixes := []netip.Prefix{
netip.MustParsePrefix("fc00::/7"),
netip.MustParsePrefix("fe80::/10"),
netip.MustParsePrefix("::1/128"),
}
internal/firewall/iptables/parse.go:310
- For the new
-m connmark --mark 0x567syntax, this branch returns at thestrings.HasPrefixcase before consuming the mark value. The outer parser then stores0x567ininstruction.mark, whileparseChainRuleOptionalFieldsstores the listed rule inrule.connMark, sodeleteIPTablesRulecannot match connmark rules. Parse the--markvalue intoinstruction.connMarkhere, including the optional inversion marker.
case "connmark":
consumed++
switch {
case len(fields[consumed:]) == 0 || strings.HasPrefix(fields[consumed], "-"):
// end or another flag
internal/pmtud/tcp/mss.go:46
- This new
missingMarkModulebranch does not reach callers. When every destination returns this error,mssremains zero and the function returns the genericall ... unreachableerror below; when any destination succeeds, it returns nil. Consequentlytcp.PathMTUDiscovernever receivesErrKernelModuleMissing, so the new ICMP fallback inpmtud.gois ineffective for this case. Preserve and return the stored module error in the no-MSS path, and cover it with a test.
missingMarkModule := errors.Is(result.err, iptables.ErrKernelModuleMissing) ||
errors.Is(result.err, iptables.ErrMarkMatchModuleMissing)
switch {
case err != nil: // error already occurred for another findMSS goroutine
case missingMarkModule:
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
WIP
Do this: #3152 (comment)
Issue
#3152
Assertions