[sample #12602] feat(bpf): implement connection limit enforcement (QoS max connections)#218
[sample #12602] feat(bpf): implement connection limit enforcement (QoS max connections)#218stevegaossou wants to merge 1 commit into
Conversation
Mirrors the diff the human reviewers first saw on projectcalico#12602 ('feat(bpf): implement connection limit enforcement (QoS max connections)' by @coutinhop) for the Council of Claudes benchmark. base(fork)=28ca14a6bd3e2a50aff9c279a8f6e44c85b080f6 head(as-reviewed)=c265a80b36e2fa6a9750b4eafee390918262bf4a
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| static CALI_BPF_INLINE void qos_connlimit_decrement_for_ct(struct calico_ct_value *v) | ||
| { | ||
| __u32 cl_flags = ct_value_get_flags(v); | ||
| if (cl_flags & CALI_CT_FLAG_CONNLIMIT_DEC) { |
There was a problem hiding this comment.
🔎 Correctness — The DEC-flag guard is not atomic, which can lead to double-decrement. Two CPUs can concurrently enter qos_connlimit_decrement_for_ct(), both read cl_flags without CALI_CT_FLAG_CONNLIMIT_DEC set, then both proceed to set the flag and decrement once, resulting in a -2 adjustment for a single close. The same race can also occur between packet-path decrement and the cleanup program. Suggest making the “set-and-check” atomic:
- Reinterpret the contiguous flags bytes as a single 32-bit word and use an atomic OR (e.g., __sync_fetch_and_or) to set CALI_CT_FLAG_CONNLIMIT_DEC; if the returned old value already had the bit set, return without decrementing.
Example:
// flags, flags2, flags3, flags4 are contiguous; treat as u32.
volatile __u32 *flags32 = (volatile __u32 *)&v->flags;
__u32 old = __sync_fetch_and_or(flags32, CALI_CT_FLAG_CONNLIMIT_DEC);
if (old & CALI_CT_FLAG_CONNLIMIT_DEC)
return;
Then proceed to compute pod_ifindex and decrement. This avoids double-decrements across CPUs and between packet-path and cleanup.
| } | ||
| } | ||
|
|
||
| // prepareUpdate reads the existing QoS map entry and returns the key bytes |
There was a problem hiding this comment.
🔎 Correctness — prepareUpdate reconstructs the entire QoS value and BatchUpdate() writes the whole struct. Between the unlocked Get() and the locked Update, the BPF fast path may legitimately update packet-rate fields (PacketRateTokens/PacketRateLastUpdate). The scanner then overwrites those with the stale snapshot, corrupting the packet-rate state. To fix:
- Avoid full-struct overwrites. Either:
- Keep current_count in a separate BPF map so the scanner can update it independently; or
- Introduce a userspace update path that acquires the QoS-entry spinlock and performs an in-kernel RMW of only the count fields (there isn’t a kernel primitive for “partial update,” so this typically implies a different data layout or helper); or
- At minimum, read the value under the same lock that protects the write (not available via Get), which points back to needing a different layout or helper.
As-is, enabling both packet-rate and connlimit for the same WEP/direction risks recurrent token-field stomping.
| return bpfEndpointManager.GetConnLimitedPodInfo() | ||
| } | ||
| if conntrackScannerV4 != nil { | ||
| conntrackScannerV4.AddUnlocked(bpfconntrack.NewConnLimitScanner( |
There was a problem hiding this comment.
🔎 Correctness — ConnLimitScanner is added to both the IPv4 and IPv6 CT scanners, but each instance writes current_count to the same QoS key (ifindex+direction) independently. This causes the two scanners to race and clobber each other’s counts; whichever scanner runs last will drop the other protocol’s tally. Suggested fixes:
- Aggregate: make a single shared aggregator object (or a single ConnLimitScanner registered once) that receives entries from both v4 and v6 CT scans, sums by (ifindex, direction), and performs one batched write per iteration; or
- Split fields: record per-protocol counts in separate fields (e.g., CurrentCountV4/CurrentCountV6) and have the BPF fast path enforce against the sum; or
- Serialize: ensure only one of the two scanners performs the write, combining both maps’ counts before updating.
There was a problem hiding this comment.
This was resolved in a later commit.
| CALI_DEBUG("connlimit: retransmission of rejected connection"); | ||
| reject = true; | ||
| } | ||
| if (reject) { |
There was a problem hiding this comment.
🔎 Correctness — The rejection path claims to “reject with TCP RST,” but the logic only tail-calls PROG_INDEX_TCP_RST when skb_refresh_validate_ptrs() fails; otherwise it falls through to “deny”. Unless the deny path is guaranteed to generate a TCP RST for SYNs here (e.g., via a pre-set SEND_RESET or equivalent), this may silently drop instead of RSTing, contradicting the log and the iptables/nft behavior. Please verify that the normal deny path sends a TCP RST for this case; if not, unconditionally jump to PROG_INDEX_TCP_RST (after ensuring pointers are valid) or set whatever flag the deny program checks to craft a RST.
There was a problem hiding this comment.
Should have been fixed in a later commit.
There was a problem hiding this comment.
🔎 Council of Claudes — Correctness
Note
Correctness lens · bugs · completeness · concurrency · edge cases
4 potential correctness issues
High-level/whole-PR observations:
- The ConnLimitScanner writes a full QoS map value back to the kernel. If packet-rate limiting is also enabled on the same entry, the scanner can race with the BPF fast path’s token/last-update maintenance and overwrite those fields with a stale snapshot. This risks breaking packet-rate limiting when both features are enabled on the same WEP/direction. Consider isolating connection counts in a separate map, or introducing a kernel-side helper or RMW primitive (under the QoS value’s spinlock) that can update only current_count (and not touch packet-rate fields).
- Two independent scanners (v4 and v6) both write current_count to the same QoS key (ifindex+direction). Their counts are not aggregated, so the last writer “wins” and drops the other protocol’s contribution. This will undercount when both IPv4 and IPv6 connections are present. Consider a single aggregator (shared between the two CT scans) that sums both protocols before a single write, or splitting the counts by protocol (new fields) and deriving the limit check from the sum in BPF.
Line-specific findings:
🤖 Council of Claudes · 4 inline comment(s)
| ret; \ | ||
| }) | ||
|
|
||
| #define ct_value_clear_flags(v, f) do { \ |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The new ct_value_clear_flags macro isn’t used in this diff. Unused macros in BPF headers add surface/maintenance overhead and can go stale. Consider removing it until needed, or add a brief comment explaining its planned use to justify keeping it.
| // IterationStart / Check / IterationEnd. The scanner only exists as a drift | ||
| // safety net for silent CT-entry purges (half-close, idle TCPEstablished, | ||
| // network partition), so a ~30s recovery window is adequate. | ||
| const connLimitScannerRunEveryN = 3 |
There was a problem hiding this comment.
🧪 Maintainability & Tests — connLimitScannerRunEveryN is a hard-coded constant with production impact and test coupling. Suggest making it injectable (e.g., NewConnLimitScannerWithOptions(..., downsampleN int)) or at least a package-level var guarded by a setter in tests. This reduces the need for magic sleeps and makes future tuning safer without code changes.
| return qosKey.AsBytes(), newVal.AsBytes(), true | ||
| } | ||
|
|
||
| // ipToString converts a net.IP to a string key for map lookup. |
There was a problem hiding this comment.
🧪 Maintainability & Tests — ipToString uses string(ip bytes) as the map key. That is fine but non-obvious, and easy to accidentally change to net.IP.String() elsewhere. Please:
- Add a brief comment stating “key is raw 4/16-byte IP, not human-readable”.
- Consider exporting a small helper in a shared place (or duplicating the helper with identical logic and comment in the provider) so both the scanner and provider use the same conversion.
| HasEgressLimit: hasEgress, | ||
| } | ||
|
|
||
| for _, ipNet := range wep.Ipv4Nets { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — buildConnLimitPodInfo constructs keys with string(ip4)/string(ip16). This must exactly match the scanner’s keying to avoid silent mismatches. Please add a short comment here to call out the “raw byte string” convention and consider reusing a common helper (or moving the helper here and importing it in the scanner to avoid divergence).
There was a problem hiding this comment.
Dont want to be too verbose on comments.
| } | ||
| } | ||
|
|
||
| // GetConnLimitedPodInfo returns a snapshot of pod IPs to their connection |
There was a problem hiding this comment.
🧪 Maintainability & Tests — GetConnLimitedPodInfo returns the underlying map (referenced by an atomic pointer). Callers could mutate it, breaking the “snapshot” expectation. Either:
- Return a shallow copy to protect against accidental mutation, or
- Document explicitly: “returned map must be treated as immutable; ownership remains with the dataplane and will be replaced atomically.”
| // counter, starting it non-zero. Flush conntrack | ||
| // and wait one scan cycle for the recount to drop | ||
| // to 0 before opening the phase-2 connections. | ||
| By("Flushing conntrack entries until egress count is 0") |
There was a problem hiding this comment.
🧪 Maintainability & Tests — These FV tests use fixed sleeps (e.g., time.Sleep(12s) inside an Eventually loop) to wait for the recount. This couples the test to current scan periods and the downsampling ratio; it risks future flakes. Suggest extracting a helper that:
- Repeatedly triggers the recount preconditions and polls the QoS count until stable or timeout, without fixed sleeps; or
- Reads Felix’s configured CT scan period and computes an appropriate wait dynamically; or
- Exposes/injects the downsample ratio to shorten/stabilize the wait in test environments.
| globalData.Flags |= libbpf.GlobalsEgressPacketRateConfigured | ||
| } | ||
|
|
||
| if ap.IngressConnLimitConfigured { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — New global flags GlobalsIngressConnLimitConfigured/GlobalsEgressConnLimitConfigured are set here but not verified in tests. Consider adding a small unit test around AttachPoint.Configure to assert the flags are present when limits are configured (similar to the existing packet-rate flags), or extend an existing test to catch accidental regressions in wiring.
There was a problem hiding this comment.
I think too many tests is a overkill.
| Name: "cali_qos", | ||
| Flags: unix.BPF_F_NO_PREALLOC, | ||
| Version: 1, | ||
| Version: 2, |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Map version bumped to 2 and Value layout extended — good. Please add a brief comment near MapParams (or in the map package README/design) describing the migration expectation (versioned map name behavior, clean re-create on version change), to aid future maintainers making further changes to this map’s layout.
There was a problem hiding this comment.
This again is too verbose and it is understandable from our map design. Dont want a comment here.
There was a problem hiding this comment.
🧪 Council of Claudes — Maintainability & Tests
Tip
Maintainability & Tests lens · simplicity · tests · docs · idioms
Well-tested with thorough UT and FV coverage, but missing required BPF design doc updates and a few maintainability nits.
- Please update the BPF sub-design docs per the repo’s doc-update rule. This PR introduces new CT flags (ConnLimitIn/Out/Rej/Dec), new map fields in cali_qos (max_connections, current_count), and new packet-path decisions (first-SYN accounting, decrement on FIN/RST/cleanup). These must be captured under felix/design:
- bpf-overview.md (cross-cutting packet-path and fast-path cost notes),
- the sub-design covering QoS/maps (document cali_qos layout v2, ownership of fields, update paths),
- the sub-design covering conntrack/flowstate (new CT flags, where they’re set/cleared, and interactions with cleanup).
Ready-to-paste prompts: - @copilot update felix/design/bpf-overview.md "QoS connection limits" to describe first-SYN accounting at WEP ingress, reject-on-retransmit, decrement on FIN/RST, and cleanup-time recount/decrement.
- @copilot update felix/design/bpf-qos.md "cali_qos v2: connection limit fields" to document Value layout (max_connections/current_count), update sources (BPF TC vs scanner), and batch update behavior.
- @copilot update felix/design/bpf-conntrack-flowstate.md "Connlimit CT flags" to add CALI_CT_FLAG_CONNLIMIT_{INGRESS,EGRESS,REJECTED,DEC} — when set, where used (tc.c accept/reject path, cleanup), and scanner skip rules.
- The new scanner/downsampling logic is hard-coded (N=3) and the FV tests rely on wall-clock sleeps that are tightly coupled to the current scan period. Consider making the downsampling ratio injectable (constructor param or package var) to ease future tuning and reduce flake risk in tests.
- Keying by “string(net.IP bytes)” is subtle. Both the scanner and provider build keys this way — good — but it’s easy to regress. Consider centralizing a small helper (with tests) and add comments calling out the byte-string representation to avoid accidental human-readable string conversions later.
- Minor: one new macro appears unused; consider removing until needed to keep the BPF header surface lean.
Line-specific findings:
🤖 Council of Claudes · 8 inline comment(s)
|
|
||
| qosVal = qos.NewValue(qosPacketRate, qosPacketBurst, qosTokens, qosLastUpdate) | ||
| // Connection limit fields | ||
| var maxConnections, currentCount int32 |
There was a problem hiding this comment.
🛡️ Security — Unchecked downcast of configured max-connections can disable enforcement. Here maxConnections := int32(wep.QosControls.IngressMaxConnections) truncates an int64 into int32 without bounds checking. In the BPF fast path, qos->max_connections <= 0 disables connlimit, so a very large configured limit (above MaxInt32) could wrap negative and effectively turn off enforcement, weakening isolation and allowing more connections than intended.
Mitigation:
- Validate and clamp before storing: if value < 0 → 0; if value > math.MaxInt32 → set to math.MaxInt32 (or reject the config and log).
- Consider rejecting out-of-range values at the API/validation layer so they never reach dataplane code.
Example:
max := wep.QosControls.IngressMaxConnections
if max < 0 { max = 0 }
if max > math.MaxInt32 { log.Warn("IngressMaxConnections too large; clamping"); max = math.MaxInt32 }
maxConnections = int32(max)
|
|
||
| qosVal = qos.NewValue(qosPacketRate, qosPacketBurst, qosTokens, qosLastUpdate) | ||
| // Connection limit fields | ||
| var maxConnections, currentCount int32 |
There was a problem hiding this comment.
🛡️ Security — Same unchecked downcast for egress max-connections. A too-large int64 EgressMaxConnections would wrap to negative in int32, and qos_connlimit_check_and_increment() treats max_connections <= 0 as “no limit,” disabling enforcement.
Mitigation: Apply the same clamping/validation as for ingress before converting to int32; reject or clamp out-of-range inputs, and log.
| * flag), otherwise allow (the connection was already accepted and counted). | ||
| * The Go-side CT scanner periodically recounts and corrects drift. | ||
| */ | ||
| if (CALI_F_TO_WEP && !policy_skipped && |
There was a problem hiding this comment.
🛡️ Security — Bypassing ingress connlimit when policy is skipped may weaken enforcement. The check if (CALI_F_TO_WEP && !policy_skipped && …) means that when policy_skipped is true, ingress connlimit isn’t enforced for TCP SYNs to WEP. If policy “skip” can occur for traffic you still want limited (e.g., certain allowed/bypass cases), this creates a path to exceed intended connection limits.
Mitigation: Confirm design intent. If connlimit should apply irrespective of policy evaluation, drop the !policy_skipped condition; otherwise, document clearly that bypassed/“policy skipped” traffic is exempt from connection limits and ensure that’s acceptable.
There was a problem hiding this comment.
🛡️ Council of Claudes — Security
Caution
Security lens · validation · secrets · authz · isolation
1 potential security issue. Overall, the change introduces BPF-side connection-limit enforcement and a userspace drift-correction scanner. Most logic is local and guarded (spinlocks, BPF_F_LOCK) and avoids obvious race/double-counting. No plaintext secrets or unsafe execs in production paths. Main concern is an unchecked int64→int32 downcast for configured limits, which can silently disable enforcement for large values.
- Architectural note: Please verify the intended semantics for the ingress enforcement guard that requires !policy_skipped. If “policy skipped” packets to WEP are possible in production (e.g., certain bypass/failsafe cases), this condition would also skip connlimit enforcement for those SYNs; that may weaken the intended isolation/fairness guarantees.
🤖 Council of Claudes · 3 inline comment(s)
| } | ||
| applied, err := s.qosMap.BatchUpdate(batchK, batchV, unix.BPF_F_LOCK) | ||
| if err != nil { | ||
| log.WithError(err). |
There was a problem hiding this comment.
Nell — IterationEnd logs a Warn on BatchUpdate failure and continues. Return error here? If the scanner infra can’t surface it, maybe at least increment a metric or mark health so we notice repeated recount failures? Otherwise we silently drift. (Not a blocker if we intentionally keep this best-effort.)
| ret; \ | ||
| }) | ||
|
|
||
| #define ct_value_clear_flags(v, f) do { \ |
There was a problem hiding this comment.
Nell — ct_value_clear_flags() isn’t used anywhere in this PR. OK to leave this in? (just checking you intended it). If not, maybe drop it to avoid dead code.
| return nil | ||
| } | ||
|
|
||
| // buildConnLimitPodInfo builds the pod IP → ConnLimitPodInfo mapping from |
There was a problem hiding this comment.
Nell — buildConnLimitPodInfo: we ignore net.ParseCIDR errors and continue. Is that OK here? If invalid CIDRs can occur transiently, fine; otherwise a debug log might help diagnose misconfigured endpoints. WDYT?
| * BPF program (from_hep, from_wep, conntrack_cleanup, ...), not just the pod's | ||
| * own WEP program. | ||
| */ | ||
| static CALI_BPF_INLINE void qos_connlimit_decrement(__u32 ifindex, __u32 direction) |
There was a problem hiding this comment.
Nell — qos_connlimit_decrement takes direction as a raw 0/1. Worth tiny named constants (e.g. CONNLIMIT_DIR_EGRESS=0, CONNLIMIT_DIR_INGRESS=1) to avoid mix-ups, no?
| // serving as the sole decrement mechanism (connections that close or time out | ||
| // simply aren't counted on the next scan). | ||
| // connLimitScannerRunEveryN downsamples the scanner relative to the parent CT | ||
| // scan loop. With timeouts.ScanPeriod = 10s and N = 3 the scanner does a real |
There was a problem hiding this comment.
Nell — Comment says with ScanPeriod=10s and N=3 we recount “roughly every 30s” — but the NewIntDataplaneDriver comment below says “roughly every 60s”. Which do we intend?
| } | ||
|
|
||
| // Add connection limit scanner as a low-frequency drift safety net. | ||
| // It piggybacks on the CT scan loop but downsamples its recount work |
There was a problem hiding this comment.
Nell — This comment says “roughly every 60s” while connlimit_scanner.go says “~30s” for N=3 and 10s scan period. Let’s pick one so future readers don’t wonder.
| Name: "cali_qos", | ||
| Flags: unix.BPF_F_NO_PREALLOC, | ||
| Version: 1, | ||
| Version: 2, |
There was a problem hiding this comment.
Nell — Map version bumped to 2 — thanks. We also changed the C-side CALI_MAP(cali_qos, 2, …) — good. Maybe add a short note in the design (per above) covering the ABI change and bump rationale.
| * rejected without re-checking the counter (which may have | ||
| * changed due to scanner recounts). */ | ||
| { | ||
| bool sltd = src_lt_dest(&ctx->state->ip_src, |
There was a problem hiding this comment.
Nell — On ingress reject, we best-effort mark the CT entry with CONNLIMIT_REJECTED. If cali_ct_lookup_elem() returns NULL we skip the mark (fine), but then we’ll re-check the counter on retransmits rather than unconditionally reject. Is that acceptable, or do we want a stricter behaviour? (I think it’s fine as-is, but wanted to check intent.)
| // packet-rate fields and max_connections). Returns changed=false when the | ||
| // existing count already matches `count`, or when the read fails. | ||
| func (s *ConnLimitScanner) prepareUpdate(ifindex, direction uint32, count int32) (keyBytes, valBytes []byte, changed bool) { | ||
| qosKey := qos.NewKey(ifindex, direction) |
There was a problem hiding this comment.
Nell — prepareUpdate() reads the existing QoS value and returns no-op on read failure — which means we won’t create a missing QoS entry even for a limited pod. Is that the intent? (IIUC creation is handled by the dataplane writer in doApplyPolicy; just confirming ValueFromBytes tolerates a short/empty read: does qos.ValueFromBytes() handle len==0 safely?)
| // and wait one scan cycle for the recount to drop | ||
| // to 0 before opening the phase-2 connections. | ||
| By("Flushing conntrack entries until egress count is 0") | ||
| Eventually(func() int32 { |
There was a problem hiding this comment.
Nell — We flush conntrack then sleep a fixed 12s and Expect the recount to hit 0. Suspect this could be a little flaky under load. Worth a slightly longer Eventually window and/or checking the per-scan downsampling factor to avoid tight coupling to timings? (not blocking)
There was a problem hiding this comment.
Council of Claudes — Nell
Important
Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments
2 potential correctness issues; a few nits on dead code, naming, and comment accuracy. Big-picture: this adds a new dataplane mechanic (new CT flags, QoS map fields, packet-path RST-on-over-limit) — per repo rules, I think we need a felix/design update covering the new flags, map fields, and when/where counters are incremented/decremented (packet path vs cleanup vs scanner). WDYT about adding a short section to bpf-overview.md plus a topic sub-design?
-
Doc-update rule: This changes the BPF packet path and CT flags (new CONNLIMIT_* bits), and extends the QoS map ABI. Per .github/instructions/bpf.instructions.md, can we add:
- A note in bpf-overview.md about connection-limit enforcement: where the check runs (WEP ingress first SYN and WEP egress new-flow), when we RST, and how we avoid double-counting.
- A short section in the conntrack sub-design (or a new “bpf-qos-connlimit.md”) describing the four new CT flags, the increment/decrement rules (packet path close vs cleanup), and the recount scanner’s role (and lock interaction with BPF_F_LOCK).
- Map ABI note: cali_qos v2 adds MaxConnections and CurrentCount; bump rationale and upgrade behaviour.
For convenience:
@copilot update felix/design/bpf-overview.md "QoS: connection limit" to document the ingress/egress check points, RST-on-limit behaviour, and how retransmits are handled (CONNLIMIT_REJECTED).
@copilot update felix/design/bpf-conntrack-flowstate.md "Flags" to list CALI_CT_FLAG_CONNLIMIT_{INGRESS,EGRESS,REJECTED,DEC} — where they are set/consumed and the decrement paths (packet vs cleanup).
@copilot update felix/design/bpf-qos.md "cali_qos v2" to describe the new fields, locking, BPF_F_LOCK use by userspace, and upgrade/versioning. -
Tests: great to see both UT and FV coverage, including the cleanup/dec paths. The scanner downsampling comment in scanner.go says ~30s with N=3; the NewIntDataplaneDriver comment says ~60s. Let’s align those.
Line-specific findings
🤖 Council of Claudes · 10 inline comment(s)
| static CALI_BPF_INLINE void qos_connlimit_decrement_for_ct(struct calico_ct_value *v) | ||
| { | ||
| __u32 cl_flags = ct_value_get_flags(v); | ||
| if (cl_flags & CALI_CT_FLAG_CONNLIMIT_DEC) { |
There was a problem hiding this comment.
Casey — I think there’s a race here? qos_connlimit_decrement_for_ct uses a “set DEC then decrement” heuristic to be idempotent across packet-path close and cleanup. But ct_value_set_flags(v, …) isn’t synchronized, and I don’t see a per-entry lock around the CT map value, so two CPUs could both read flags with DEC unset, both set it, and both call qos_connlimit_decrement(). Do you know how we avoid double-decrement in that interleaving? If not, maybe:
- Set DEC under a bpf_spin_lock inside the QoS entry itself and store a per-CT unique marker? That’s probably overkill.
- Or, move the “DEC already set?” check behind a synchronize point that can’t race (e.g., update the CT value in-map with a flagged write and re-read/check)? Not very nice either.
- At minimum, can we justify (in a comment) why the window can’t occur in practice (e.g., cleanup path and TCP close can’t race on the same entry), or add a small mitigation?
WDYT?
| // simply aren't counted on the next scan). | ||
| // connLimitScannerRunEveryN downsamples the scanner relative to the parent CT | ||
| // scan loop. With timeouts.ScanPeriod = 10s and N = 3 the scanner does a real | ||
| // recount roughly every 30s; the intervening 2 iterations early-return in |
There was a problem hiding this comment.
Casey — nit: This says “roughly every 30s” for N=3 with a 10s scan.
The comment added in int_dataplane.go below says “roughly every 60s”. I think we should standardize on one value (and maybe name the interval in terms of “ScanPeriod * N” to avoid future drift).
| // Add connection limit scanner as a low-frequency drift safety net. | ||
| // It piggybacks on the CT scan loop but downsamples its recount work | ||
| // internally (see connLimitScannerRunEveryN in connlimit_scanner.go), | ||
| // so the actual recount runs roughly every 60s. It covers silent |
There was a problem hiding this comment.
Casey — nit: This says “roughly every 60s”, but connLimitScannerRunEveryN is 3 and the parent loop defaults to ~10s, so it’s ~30s right now. Maybe reword to “runs every ScanPeriod*N (~30s with default settings)”?
| // so the actual recount runs roughly every 60s. It covers silent | |
| // ... downsamples its recount work internally (see connLimitScannerRunEveryN), | |
| // so the actual recount runs every ScanPeriod * N (~30s with the default 10s ScanPeriod). |
| ret; \ | ||
| }) | ||
|
|
||
| #define ct_value_clear_flags(v, f) do { \ |
There was a problem hiding this comment.
Casey — Do we actually need ct_value_clear_flags() yet? I don’t see any callers in this PR. I’m a little bit skeptical of adding unused helpers — they tend to rot. Fine to leave if there’s an imminent use, though; otherwise I think we can drop it and add it back when it’s needed.
| @@ -234,7 +234,12 @@ var _ = infrastructure.DatastoreDescribe( | |||
| if _, ok := infra.(*infrastructure.EtcdDatastoreInfra); ok && BPFMode() { | |||
There was a problem hiding this comment.
Casey — We are trying to avoid having tests self-skip. I think this should fail explicitly with a clear reason or be guarded by an explicit selector, e.g., a RequiresEtcd+RequiresBPF label we can turn off in CI. As-is, skips can mask whole areas going untested.
For example:
Fail("This suite requires Kubernetes datastore (not etcd) when running in BPF mode.")
Or wrap the describe with an infra-aware WithRequires... helper?
| keyStr := bytesToHexString(key.AsBytes()) | ||
|
|
||
| args := []string{"bash", "-c", fmt.Sprintf(`bpftool map dump name cali_qos -j | jq '.[].elements[] | select(.key | join(" ") == "%s") | .value | join(" ")'`, keyStr)} | ||
| args := []string{"bash", "-c", fmt.Sprintf(`bpftool map dump name %s -j | jq '.[].elements[] | select(.key | join(" ") == "%s") | .value | join(" ")'`, qosMapName, keyStr)} |
There was a problem hiding this comment.
Casey — I wonder about relying on bpftool+jq string parsing in FVs. It works, but using our Go wrappers (felix/bpf/qos.MapParams + a real map handle) would be more robust and avoids shelling out. Not a blocker, but WDYT about wiring a small helper that opens the QoS map and reads the entry by key directly?
| Eventually(func() int32 { | ||
| tc.Felixes[0].Exec("calico-bpf", "conntrack", "clean") | ||
| tc.Felixes[1].Exec("calico-bpf", "conntrack", "clean") | ||
| time.Sleep(12 * time.Second) |
There was a problem hiding this comment.
Casey — This explicit sleep inside an Eventually loop is sort-of a crazy long timeout in aggregate — it’ll cost up to a minute under retry and will be flaky across CI variance. I think we should either:
- Expose a way to trigger a recount (e.g., a test-only knob to set connLimitScannerRunEveryN=1 or a signal to the scanner), or
- Poll without fixed sleeps and rely on the scanner’s own period (but that implies a single, longer Eventually, not sleep-per-try).
We’ve had these “sleep in a loop” patterns sneakily add hours across suites. Can we avoid the Sleep here?
| @@ -623,7 +653,11 @@ var _ = infrastructure.DatastoreDescribe( | |||
|
|
|||
| By("Stopping persistent connections") | |||
| for i := range len(pcs) { | |||
There was a problem hiding this comment.
Casey — nit: We’re using the Go 1.22 “range over int” form here: for i := range len(pcs). Are our FV images pinned to a Go toolchain that supports that everywhere? If not, probably safer to stick to the familiar for i := range pcs (and use nil checks for the inner body as you’re already doing).
| for i := range len(pcs) { | |
| for i := range pcs { | |
| if pcs[i] == nil { | |
| continue | |
| } | |
| pcs[i].Stop() | |
| pcs[i] = nil | |
| } |
| * flag), otherwise allow (the connection was already accepted and counted). | ||
| * The Go-side CT scanner periodically recounts and corrects drift. | ||
| */ | ||
| if (CALI_F_TO_WEP && !policy_skipped && |
There was a problem hiding this comment.
Casey — Question: Should this ingress enforcement path be gated by INGRESS_CONN_LIMIT_CONFIGURED too? I see qos_connlimit_check_and_increment() no-ops if there’s no map entry / max<=0, so functionally it’s fine; I’m just wondering if we can avoid the map lookup entirely when the global flag says “no ingress connlimit configured for this attach point.” No big deal though.
| if wep.QosControls.IngressPacketRate > 0 { | ||
| // Ingress packet rate is configured | ||
| ap.IngressPacketRateConfigured = true | ||
| // Handle ingress QoS (packet rate + connection limit) in a single map entry |
There was a problem hiding this comment.
Casey — This ingress/egress QoS update logic is duplicated with only small differences. Could we refactor into a small helper like:
func (m *bpfEndpointManager) programQoSEntry(ifindex int, ingress bool, prate, pburst int, maxConns int) errorIt’d reduce the chance of the two code paths drifting (and help with future tweaks like version bumps), but I could be swayed either way. WDYT?
There was a problem hiding this comment.
Council of Claudes — Casey
Warning
Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness
6 substantive issues; 1 likely race in decrement path, test discipline/timeouts, doc-update, and a few nits
- I think this needs a design/doc update. This touches the BPF dataplane packet path (new CT flags, new QoS map fields, enforcement at accepted/new-flow entrypoints) and adds a new scanner. Per the repo’s BPF doc-update rule, can we add/update the relevant felix/design/bpf-*.md sections (overview + a focused section covering the new connlimit flags, map fields, and when increments/decrements happen)? Happy to suggest a concrete @copilot prompt if helpful.
- I am a little bit skeptical of the test timing strategy here — there are a few long Eventuals and an explicit Sleep inside an Eventually loop. These can sneakily add minutes to the FV suite and be quite flaky across CI noise. I think we should either expose a way to trigger the recount (or dial the scanner frequency down for FV via a knob) rather than sleeping/polling in-band.
- We are trying to avoid having tests self-skip. There’s still a Skip based on infra type in this suite. I think this should fail explicitly with a clear reason (or be guarded by a selector/label).
- Minor, but the scanner run-interval comments disagree (~30s vs ~60s). Worth standardizing so future readers don’t get confused.
- Implementation nit: there’s a new ct_value_clear_flags() macro that doesn’t seem to be used — can this be dropped until it is needed?
- Style nit: FV is shelling out to bpftool/jq to read the QoS map. It works, but I wonder if we can lean on our Go map wrappers from felix/bpf/qos (MapParams/Map) and avoid parsing JSON in-string? Less brittle and faster, but I could be swayed either way.
🤖 Council of Claudes · 10 inline comment(s)
There was a problem hiding this comment.
Pull request overview
This PR adds BPF dataplane support for QoS max-connection limits by extending the QoS map value to include connlimit state, enforcing limits in the TC programs, and adding a userspace conntrack scanner to periodically recount and correct drift. It also updates FV tooling/tests to exercise the new connlimit behavior, including explicit TCP RST closes for fast-path counter-decrement verification.
Changes:
- Extend the
cali_qosBPF map (version bump) to carrymax_connectionsandcurrent_count, and plumb new “connlimit configured” globals flags. - Enforce ingress/egress connection limits in the BPF TC programs, including CT flagging and decrement paths (FIN/RST + CT cleanup).
- Add a connlimit recount scanner + tests, and update FV test utilities/tests (including a new
--send-rstoption) to validate counter behavior.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| felix/fv/workload/workload.go | Adds SendRST option plumbing into persistent connection creation. |
| felix/fv/test-connection/test-connection.go | Adds --send-rst flag and implements RST-on-close behavior in TCP driver. |
| felix/fv/qos_controls_test.go | Expands QoS FV tests to validate BPF connlimit counters and multiple close/cleanup scenarios. |
| felix/fv/connectivity/conncheck.go | Plumbs SendRST through to test-connection invocation for persistent connections. |
| felix/dataplane/linux/int_dataplane.go | Wires the new connlimit scanner into the BPF conntrack scanner loop. |
| felix/dataplane/linux/bpf_ep_mgr.go | Programs connlimit fields into the QoS map and exposes a snapshot of connlimited pods for the scanner. |
| felix/bpf/ut/qos_test.go | Updates QoS UT for the new QoS map value layout. |
| felix/bpf/tc/attach.go | Adds attach-point flags and exports them to BPF globals for connlimit enablement. |
| felix/bpf/qos/map.go | Bumps QoS map version and extends value encoding/decoding with connlimit fields. |
| felix/bpf/libbpf/libbpf.go | Adds libbpf globals flag constants for connlimit configured bits. |
| felix/bpf/libbpf/libbpf_stub.go | Mirrors new globals flag constants in the stub implementation. |
| felix/bpf/conntrack/v4/map.go | Adds new connlimit-related CT flag constants and names. |
| felix/bpf/conntrack/connlimit_scanner.go | New: recount scanner that periodically corrects connlimit counters using CT map state. |
| felix/bpf/conntrack/connlimit_scanner_test.go | New: unit tests for connlimit scanner counting, skipping, batching, and downsampling behavior. |
| felix/bpf-gpl/qos.h | Extends QoS value struct and adds connlimit check/increment + decrement helpers. |
| felix/bpf-gpl/tc.c | Adds ingress and egress connlimit enforcement hooks in TC programs and CT flag plumbing. |
| felix/bpf-gpl/globals.h | Adds new globals flag bits for ingress/egress connlimit configured. |
| felix/bpf-gpl/bpf.h | Exposes globals macros for connlimit configured flags. |
| felix/bpf-gpl/conntrack.h | Adds connlimit decrement-on-close logic and ingress SYN flagging logic to CT handling. |
| felix/bpf-gpl/conntrack_types.h | Adds new connlimit CT flags and a CT result bit for “first SYN” connlimit handling. |
| felix/bpf-gpl/conntrack_cleanup.c | Decrements connlimit counters when CT cleanup deletes expired entries. |
| struct calico_qos_key key = { | ||
| .ifindex = ctx->skb->ifindex, | ||
| #if CALI_F_INGRESS | ||
| .ingress = 1, | ||
| #else // CALI_F_EGRESS | ||
| .ingress = 0, | ||
| #endif | ||
| }; |
| // connection limit | ||
| __s32 max_connections; // 0 = no limit, >0 = limit | ||
| __s32 current_count; // maintained by BPF (increment on SYN) and scanner (recount) | ||
| }; |
| /* qos_connlimit_check_and_increment atomically checks the connection limit for | ||
| * the current interface and direction using the cali_qos map. If below the | ||
| * limit, increments the counter and returns 0 (allow). If at or above the | ||
| * limit, returns -1 (reject). Returns 0 if no limit is configured for this | ||
| * interface+direction (no map entry or max_connections <= 0). | ||
| * | ||
| * Only called for TCP SYN on WEP interfaces. | ||
| */ |
| // The BPF dataplane increments the count on new TCP SYN. This scanner | ||
| // periodically recounts all established connections and overwrites the count, | ||
| // serving as the sole decrement mechanism (connections that close or time out | ||
| // simply aren't counted on the next scan). |
| // Add connection limit scanner as a low-frequency drift safety net. | ||
| // It piggybacks on the CT scan loop but downsamples its recount work | ||
| // internally (see connLimitScannerRunEveryN in connlimit_scanner.go), | ||
| // so the actual recount runs roughly every 60s. It covers silent | ||
| // CT-entry purges that the BPF fast-path can't observe: half-close, |
| @@ -1185,6 +1192,7 @@ func tcpForceV6(ip net.IP, port int) (net.Conn, error) { | |||
| type connectedTCP struct { | |||
| @@ -62,6 +62,7 @@ Options: | |||
| --stdin Read and send data from stdin | |||
| --timeout=<seconds> Exit after timeout if pong not received | |||
| --sleep=<seconds> How long to sleep before seding another ping | |||
Benchmark sample reproducing the as-first-reviewed state of projectcalico/calico#12602 (by @coutinhop).
This PR's diff equals what the human reviewers first saw: fork point
28ca14a6bd3e→ earliest human-reviewed commitc265a80b36e2. The original PR drew 10 human top-level review comments — the ground truth to compare the Council's feedback against.🤖 Generated for the Council of Claudes benchmark.