fix(router): nft tables limit number of peers source #4852
Conversation
|
|
WalkthroughReplaces single bulk ipset initialization with batched adds in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as createIpSet
participant K as kernel ipset
participant L as logger
rect rgba(70,130,180,0.06)
C->>K: Create set (metadata)
end
rect rgba(60,179,113,0.06)
C->>K: AddElements(initialElements ≤ maxElements)
K-->>C: OK / error
C->>K: Flush set
C-->>L: Log initial creation (count)
end
alt remaining elements exist
loop for each sublist (≤ maxElements)
rect rgba(255,215,0,0.04)
C->>K: SetAddElements(sublist)
K-->>C: OK / error (error includes sublist count)
C->>K: Flush set
C-->>L: Log sublist progress (count)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
417-424: Consider more idiomatic batching loop.The loop logic is correct but uses
subEnd += maxSizewith external declaration, which is less common than the standard pattern. This makes it slightly harder to verify correctness at a glance.Consider the more idiomatic approach:
- var subEnd int maxSize := 200 for subStart := 0; subStart < len(sources); subStart += maxSize { - subEnd += maxSize + subEnd := subStart + maxSize if subEnd > len(sources) { subEnd = len(sources) } subSources := sources[subStart:subEnd]This keeps
subEndscoped to the loop iteration and makes the relationship betweensubStartandsubEndexplicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/nftables/router_linux.go (2)
client/firewall/manager/firewall.go (1)
Network(63-66)client/firewall/manager/set.go (2)
Set(15-18)NewPrefixSet(42-58)
🔇 Additional comments (2)
client/firewall/nftables/router_linux.go (2)
337-340: LGTM: Clean refactoring to extract source handling logic.The extraction into
applySources()improves code organization. The error handling is correct since the helper already wraps errors appropriately.
405-415: LGTM: Single source handling is correct.The special case for 0.0.0.0/0 (skipping source matching) and the direct prefix usage for other single sources are both handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/manager_linux_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/nftables/manager_linux_test.go (8)
client/firewall/nftables/router_linux_test.go (1)
NFTABLES(28-28)client/firewall/create_linux.go (1)
NFTABLES(28-28)client/firewall/nftables/manager_linux.go (1)
Create(47-67)client/firewall/iptables/manager_linux.go (1)
Create(39-61)client/proto/daemon.pb.go (3)
Network(2079-2088)Network(2101-2101)Network(2116-2118)client/firewall/manager/firewall.go (2)
Network(63-66)ActionAccept(57-57)client/firewall/manager/protocol.go (1)
ProtocolTCP(9-9)client/firewall/manager/port.go (1)
Port(10-16)
🔇 Additional comments (1)
client/firewall/nftables/manager_linux_test.go (1)
9-9: strconv import is correctly scoped and usedThe new
strconvimport is used only in the added test and looks appropriate; no issues here.
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
2 times, most recently
from
c0eb4ab to
d5172b4
Compare
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
e13e2c8 to
d87ef88
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/firewall/nftables/manager_linux_test.go (1)
389-437: Test logic looks good; consider renaming away from “10kPrefixes”.The test setup (nftables/iptables checks, manager lifecycle, ~6k prefixes generation, route filter + iptables-save verification) looks sound and exercises the intended large-prefix path.
Minor nit: the function name says
...For10kPrefixesbut the current loop bounds generate 24×254 = 6,096 prefixes. Either adjust the name to something more generic (e.g.ForManyPrefixes/For6kPrefixes) or tweak the counts to better match the name.client/firewall/nftables/router_linux.go (1)
332-352: Critical: batched source sets are combined with AND semantics in a single rule, so rules with >200 sources will never match.
applySourcescurrently batches sources into groups of 200, creates afirewall.Setper batch, and then appends the resultingLookupexpressions fromapplyNetworkinto a single rule:
- For multiple sources, each batch produces something equivalent to
ip saddr @set_ias a separate match in the same rule (viagetIpSetExprs).- In nftables, multiple match statements in a single rule are conjunctive: if any match fails, evaluation proceeds to the next rule, not to the next match within the same rule. All matches in a rule must succeed for the verdict to be reached. (manpages.opensuse.org)
- Since the batched sets are disjoint subsets, no source address can be contained in all of them. A packet’s source can be in at most one
set_i, so at least oneLookupwill fail and the rule will never match for any source once you cross the batching threshold.Net effect: for
len(sources) > 200, the added route rule becomes effectively dead — it passes creation and avoids the netlink size error, but no traffic will ever match it. That’s a regression in correctness compared to “failing loudly” on too many sources.To restore correct OR semantics while respecting size limits, you’ll need to restructure this so that each batch is not a separate
Lookupwithin the same rule, for example:
- Create one route rule per batch (same destination/proto/ports/action, different source set), and adjust the
rulesbookkeeping to track multiple nftables rules for a single logical route rule key; or- Reduce to a single set per route rule by keeping the large set but inserting elements in smaller chunks (e.g., via
SetAddElements), so the kernel never receives an oversized netlink message, but the final rule still uses just oneLookup.As-is, this change breaks matching for any route with more than 200 source prefixes and should be fixed before merging.
In nftables, if a rule contains multiple separate match statements like `ip saddr @set1` and `ip saddr @set2`, does the rule require both matches to succeed (logical AND), or is there any built-in OR semantics within a single rule? Please confirm the behavior and provide an example from official docs or man pages.Also applies to: 414-448
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
414-448: Clarify behavior for empty sources and 0.0.0.0/0, and tighten batching loop.Minor points in
applySources:
Empty
sourcesslice semantics
- For
len(sources) == 0, the “multiple sources” branch runs but the loop doesn’t execute, soexprsstays empty and you effectively create a rule that matches any source.- For a single
0.0.0.0/0, you also intentionally emit no source expressions.
If “no sources” and “0.0.0.0/0” are both supposed to mean “all sources”, it might be worth making that explicit (either in code comments or via an early return forlen(sources) == 0) so callers aren’t surprised.Batching loop clarity
- Using a running
subEndwithsubEnd += maxSizeworks but is slightly harder to read than computing it fromsubStart:for subStart := 0; subStart < len(sources); subStart += maxSize { subEnd := subStart + maxSize if subEnd > len(sources) { subEnd = len(sources) } subSources := sources[subStart:subEnd] ... }This avoids any risk of accidental misuse of
subEndoutside the loop and makes the chunking logic self-evident.Both are small, non-blocking cleanups, but they would make the intent clearer for future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/firewall/nftables/manager_linux_test.go(1 hunks)client/firewall/nftables/router_linux.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/firewall/nftables/manager_linux_test.go (5)
client/firewall/nftables/manager_linux.go (1)
Create(56-76)client/firewall/iptables/manager_linux.go (1)
Create(39-61)client/iface/iface.go (1)
DefaultMTU(28-28)client/firewall/manager/firewall.go (2)
Network(63-66)ActionAccept(57-57)client/firewall/manager/protocol.go (1)
ProtocolTCP(9-9)
client/firewall/nftables/router_linux.go (2)
client/firewall/manager/firewall.go (1)
Network(63-66)client/firewall/manager/set.go (2)
Set(15-18)NewPrefixSet(42-58)
|
great stuff @nhenneaux, thank you! The team is looking into that |
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
d87ef88 to
6e8d485
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/firewall/nftables/router_linux.go (1)
48-51: Batched source sets are still ANDed within a single rule; >maxPrefixesSet sources will never matchThe new batching via
maxPrefixesSetandapplySourcesavoids oversized ipsets, but as currently wired it breaks matching semantics for more thanmaxPrefixesSetsources:
applySourceschunkssourcesand, for each chunk, builds a Set and callsapplyNetwork, which yields aPayload+Lookuppair per Set.- All of these expressions are concatenated into a single
exprsslice and used in one nftables rule inAddRouteFiltering.- In nftables, sequential
Lookup/Cmpexpressions in the same rule are effectively AND’ed: the packet must be contained in every set to reach the verdict. With disjoint chunks, any given source IP can only belong to one Set, so once you exceedmaxPrefixesSetprefixes the rule can never match.This means route ACLs with more than 500 source prefixes will silently stop matching instead of just being split across batches.
To restore correct OR semantics while still avoiding kernel limits, you likely need to change the batching strategy, for example:
- Keep a single Set per logical rule and batch element insertion only.
- Let
applySources(orapplyNetwork) still use onefirewall.Setfor allsources, and move the batching intocreateIpSet: create an empty set and then add elements in multipleSetAddElementscalls, each under the kernel’s safe limit.- OR via multiple rules instead of multiple Sets in one rule.
- For each chunk, build its own Set and emit a separate nftables rule with identical destination/protocol/ports/action but a different source Set.
- If you go this route, you’ll need to adjust how
r.rulesandDeleteRouteRulemodel a logical route rule (one high-level rule mapping to N nftables rules + ipsets), otherwise you’ll leak rules/sets or fail to delete all of them.As written, this is a functional regression for large source lists and should be fixed before relying on batching in production. Based on nftables expression semantics, not on this repository’s code.
Also applies to: 335-355, 416-451
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
416-430: Clarify single 0.0.0.0/0 and empty-sources handling in applySourcesThe special-casing for a single source has a couple of edge cases worth tightening up:
- For
len(sources) == 1andsources[0].Bits() == 0, you leavesourceat its zero value and still callapplyNetwork(source, sources, true). Depending on howfirewall.Network.IsPrefix/IsSetare implemented, this may be treated as “no source constraint” (which matches the intent of 0.0.0.0/0) or as an invalid network; please confirm this is equivalent to the previous 0.0.0.0/0 behavior and can’t return an error.- When
len(sources) == 0, the function falls into the “multiple sources” branch and returns an emptyexprsslice, effectively creating a rule with no source restriction. If an emptysourceslist is not a valid input, consider explicitly rejecting it (e.g.,return nil, fmt.Errorf("no source prefixes provided")) to avoid accidental “match any source” rules.Making these edge cases explicit will reduce surprises and make the intent around 0.0.0.0/0 and empty inputs clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/firewall/nftables/manager_linux_test.go(1 hunks)client/firewall/nftables/router_linux.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/nftables/router_linux.go (2)
client/firewall/manager/firewall.go (1)
Network(63-66)client/firewall/manager/set.go (2)
Set(15-18)NewPrefixSet(42-58)
🔇 Additional comments (1)
client/firewall/nftables/manager_linux_test.go (1)
389-437: Test is well-structured and unique; minor naming suggestionVerification confirms this test exists as a single definition (no duplication). The structure is solid—proper feature guards, iptables-save verification, preallocated prefix slice, and correct loop bounds.
One optional tidbit: the function name says "6kPrefixes" but generates 6,096 prefixes exactly (
(25–1)×(255–1) = 24×254). If you want to be precise, either adjust the constant values slightly or add a brief comment noting the actual count. This is purely a naming clarity suggestion and not a blocker.
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
67ee4e1 to
e2af8d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/firewall/nftables/router_linux.go (1)
416-450: Batched source sets still use AND semantics; rules won’t match whenlen(sources) > maxPrefixesSet.
applySourcescurrently creates multiplefirewall.Setinstances (one persubSourcesbatch) and appends each batch’sLookupexpressions into a single rule viaapplyNetwork/getIpSetExprs. In nftables, multiple match expressions in one rule are combined with logical AND, so this effectively requires the packet source to belong to every set in the rule.Because each batch contains disjoint prefixes, a given source address can only be in one of these sets; it will pass the
Lookupfor its own set and fail all others, so the rule never matches once you cross a single batch. This is the same logical issue highlighted in the earlier review: batching into multiple sets but keeping them in one rule createsSet1 AND Set2 AND …semantics instead of the desired union/OR.Consequence:
- For
len(sources) <= maxPrefixesSet, behavior is fine (single set).- For
len(sources) > maxPrefixesSet, the rule is installed without netlink errors, but traffic will never match it, effectively breaking route ACLs for large source lists.To fix this you need OR semantics across batches, for example:
- Keep one nftables set per rule and batch element insertion at the netlink level (e.g., create the set empty, then call
SetAddElementsin chunks), or- Create multiple rules, each referencing a single set for a batch of prefixes but sharing the same destination/proto/ports/action, so the chain provides OR semantics via multiple matching rules rather than multiple lookups in one rule.
Whichever approach you choose, ensure that:
ruleKey/DeleteRouteRule/decrementSetCounterremain consistent with how many nft rules and sets are actually created.- Existing tests with large prefix counts verify actual data‑plane behavior (packets from sources beyond the first batch must still match).
🧹 Nitpick comments (2)
client/firewall/nftables/router_linux.go (2)
48-51: ClarifymaxPrefixesSetrationale and comment.The comment mentions a ~3.2k/3.3k threshold, while
maxPrefixesSetis set to 500. That’s a big safety margin but the relation isn’t obvious from the comment alone. Consider briefly explaining why 500 was chosen (e.g., “conservative per‑set limit to keep netlink messages small enough”) or adjusting the comment so future readers don’t try to infer a direct mapping between the 3.2k/3.3k numbers and this constant.
352-355: Preserve error context whenapplySourcesfails.Here you return
errdirectly, whereas most of the file wraps errors with context (e.g.,"apply destination: %w","convert protocol to number: %w"). For consistency and easier debugging, consider wrapping this as well:- exprs, err := r.applySources(sources) - if err != nil { - return nil, err - } + exprs, err := r.applySources(sources) + if err != nil { + return nil, fmt.Errorf("apply sources: %w", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/firewall/nftables/manager_linux_test.go(1 hunks)client/firewall/nftables/router_linux.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/firewall/nftables/manager_linux_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/nftables/router_linux.go (2)
client/firewall/manager/firewall.go (1)
Network(63-66)client/firewall/manager/set.go (2)
Set(15-18)NewPrefixSet(42-58)
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
e2af8d7 to
68d634e
Compare
…ailing at 3277 prefixes on nftables v1.0.9 with Ubuntu 24.04.3 LTS, 6.14.0-35-generic netbirdio#35~24.04.1-Ubuntu
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
68d634e to
778a79a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/firewall/nftables/router_linux.go (1)
415-449: Critical: batched source sets are AND‑ed inside a single rule; rules with >maxPrefixesSetsources will never match.When
len(sources) > maxPrefixesSet,applySources:
- Splits
sourcesinto sub‑slices of at mostmaxPrefixesSet.- For each sub‑slice, builds a
firewall.Setand callsapplyNetworkwithnetwork.Setset.applyNetwork→getIpSet→getIpSetExprs, which returns two expressions per set: aPayload(load addr) followed by aLookupon that set.- All these expressions are appended into a single
exprsslice, whichAddRouteFilteringthen uses to build one nftables rule.Resulting rule shape (simplified):
Payload; Lookup(set_batch_1);Payload; Lookup(set_batch_2);...- destination / proto / ports / verdict.
In nftables, expressions within a rule have AND semantics: if any
Lookup(orCmp) fails, evaluation of that rule aborts and processing moves to the next rule. Since each batch set contains a disjoint subset of prefixes, a source IP can only be in one of these sets. Requiring it to pass allLookupexpressions means no packet can ever satisfy the rule once there is more than one batch.Practically, this means that for any route ACL whose
sourceslength exceedsmaxPrefixesSet, the constructed rule will never match any traffic, silently breaking route filtering for large peer sets.To get OR semantics across batches, you’ll need to change the structure, for example:
- Generate one nftables rule per batch (each with a single set lookup) that shares the same destination/proto/ports/action, and adjust how
r.rulesandDeleteRouteRuletrack multiple nft rules per logical ACL; or- Use a different nftables construct that provides OR semantics across multiple sets, and reflect that in
applySources.Given this affects correctness for large deployments, fixing the OR/AND semantics is critical before relying on batching in production.
Would you double‑check nftables rule semantics here and confirm that a single rule with multiple
Lookupexpressions indeed behaves as AND (i.e., aborts on first failed lookup), and that your tests cover actual packet matching forlen(sources) > maxPrefixesSet, not just successful rule creation?
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
48-51: Hard‑coded batching limit: comment is good, consider making behavior easier to tune.The
maxPrefixesSet=3000 choice is justified in the comment, but it is still a magic number tied to observed behavior (3277). If this limit is expected to vary between kernels/set types over time, consider either:
- exposing it as a package‑level variable or config knob, or
- expanding the comment to capture how it was derived (e.g. kernel version, set type, interval usage), so future maintainers know when they might need to revisit it.
This is non‑blocking, just about maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/firewall/nftables/manager_linux_test.go(1 hunks)client/firewall/nftables/router_linux.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/firewall/nftables/router_linux.go (2)
client/firewall/manager/firewall.go (1)
Network(63-66)client/firewall/manager/set.go (2)
Set(15-18)NewPrefixSet(42-58)
🔇 Additional comments (3)
client/firewall/nftables/manager_linux_test.go (2)
389-437: LGTM! Past feedback has been addressed.The test correctly:
- Checks for
iptables-saveavailability before running (line 394-396)- Uses efficient
AddrFrom4prefix generation instead of string parsing- Has accurate capacity pre-allocation:
(octet2Count-1)*(octet3Count-1) = 24*254 = 6,096matches the actual loop iterations- Uses a reasonable naming convention ("6kPrefixes" for 6,096 prefixes)
This test validates the batching fix for large prefix sets.
439-478: Good edge case coverage.This test validates that
AddRouteFilteringhandles an empty prefix slice gracefully, which is an important edge case for the newapplySourcesbatching logic.client/firewall/nftables/router_linux.go (1)
352-355: Refactor toapplySourcesimproves readability and error context.Delegating source handling from
AddRouteFilteringintoapplySourcescleans up the main function and the new error message with the totallen(sources)provides useful context when debugging failures. This change itself looks sound and keeps the rest of the rule construction logic unchanged.
|
I reproduced the issue with failing at 3277 prefixes on Initial issue is on |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(2 hunks)
🔇 Additional comments (2)
client/firewall/nftables/router_linux.go (2)
48-51: LGTM!The
maxPrefixesSetconstant with a safety margin is appropriate for addressing the nftables limit issue described in the PR.
500-513: LGTM! Batching approach correctly adds elements to a single Set.The batching logic properly adds all elements to the same nftables Set in chunks, avoiding the AND semantics issue flagged in the previous review. The loop correctly uses
min()for bounds checking and includes appropriate logging and error handling.
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
1e6c68d to
cc31226
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
498-498: Consider using leveled logging for consistency.Lines 498 and 512 use
log.Printf, which bypasses the logging framework's level filtering. The rest of the codebase uses structured logging (log.Debugf,log.Infof,log.Tracef).Consider this change for consistency:
- log.Printf("Created new ipset: %s with %d initial prefixes (total prefixes %d)", setName, len(initialElements)/2, len(elements)/2) + log.Infof("Created new ipset: %s with %d initial prefixes (total prefixes %d)", setName, len(initialElements)/2, len(elements)/2)And similarly on line 512:
- log.Printf("Added new prefixes (%d) in ipset: %s", nSubPrefixes, setName) + log.Infof("Added new prefixes (%d) in ipset: %s", nSubPrefixes, setName)This allows operators to control log verbosity via level configuration. Given that this fix handles large peer counts (thousands),
Infofis appropriate for tracking batch progress.Also applies to: 512-512
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(2 hunks)
🔇 Additional comments (2)
client/firewall/nftables/router_linux.go (2)
48-51: LGTM: Conservative batch limit with appropriate margin.The
maxPrefixesSetconstant of 1500 provides a reasonable safety margin below the observed failure threshold of 1638 prefixes. GroupingrefreshRulesMapErrorinto the const block is a clean organizational improvement.
487-514: LGTM: Batching logic correctly implements OR semantics.The batching implementation is sound and correctly solves the nftables limitation:
AddSetcreates the set with the initial batch (up to 3000 elements)SetAddElementsadds subsequent batches to the same set (line 506)- All elements end up in one set, so lookups match if the IP is in any batch (OR semantics)
This directly addresses the reported failure at ~3,280 peers by chunking element additions. The use of
min(maxElements, nElements)on line 490 correctly prevents slice bounds panics for small input sets.Note: The past review comment flagging "Critical: AND semantics" was based on a misunderstanding. The code does not create multiple sets per rule; it populates a single set incrementally, which is the correct approach.
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
cc31226 to
1bff5cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/firewall/nftables/router_linux.go (1)
500-513: Consider reducing log verbosity for multiple batches.The batching logic is correct and handles large prefix sets properly. However, line 512 uses
log.Infoffor each batch, which could create significant log volume when many batches are needed (e.g., 4+ Info messages for 6,375 prefixes).Consider one of these approaches:
- Change line 512 to
log.Debugfto reduce verbosity- Add a summary log at the end instead of per-batch logging
- Keep
log.Infofonly for the final batchExample diff to reduce verbosity:
- log.Infof("Added new prefixes (%d) in ipset: %s", nSubPrefixes, setName) + log.Debugf("Added new prefixes (%d) in ipset: %s", nSubPrefixes, setName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/firewall/nftables/router_linux.go(2 hunks)
🔇 Additional comments (2)
client/firewall/nftables/router_linux.go (2)
48-51: LGTM! Reasonable safety margin for batching.The constant
maxPrefixesSet = 1500provides a safe margin below the observed failure threshold. MovingrefreshRulesMapErrorinto the same const block is a reasonable cleanup.
487-498: LGTM! Initial batch creation is correct.The initial batch logic correctly handles the case where total elements are fewer than
maxElementsusingmin(). The error message and logging are clear and informative.
|
I addressed the issue on multiple ipset not working with OR semantic (I haven't found resources confirming CodeRabbit but it makes sense) It also addresses the issue with other IpSet with too much elements. Limit seems to be 1638 prefixes (3276 elements), it fails above. |
nhenneaux
force-pushed
the
fix(router)-nft-tables-limit-number-of-peers-source
branch
from
1bff5cf to
b1243d1
Compare
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Hi @braginini We have tested the fix in our environment and it seems to work as expected without the error preventing acl with huge number of peers. Could you check this to integrate the fix in the upstream? 🙏 |
3 participants
Describe your changes
Fixing following errors occurring when too much peers as a source by batching nft rule add by 200 sources
3240 peers does not lead to error while 3280 fails.
OS rhel9.7, kernel 5.14.0-570.60.1.el9_6.x86_64, nftables v1.0.9 (Old Doc Yak #3).
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Bug fix
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.