[sample #12960] felix: periodically re-announce proxy-neighbor IPs#200
[sample #12960] felix: periodically re-announce proxy-neighbor IPs#200stevegaossou wants to merge 1 commit into
Conversation
Mirrors the diff the human reviewers first saw on projectcalico#12960 ('felix: periodically re-announce proxy-neighbor IPs' by @mazdakn) for the Council of Claudes benchmark. base(fork)=c7a6e5b59dd761bdbc557eadcd72eb2ace226320 head(as-reviewed)=3e604dac594a360c585e26bb9d0acbd2f36a0f50
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Council of Claudes — Correctness
Note
Correctness lens · bugs · completeness · concurrency · edge cases
No correctness issues found.
Observations:
- The periodic re-announcement logic is correctly gated by a positive interval, anchored to listener start, and operates only on the goroutine-owned announced set — no race or synchronization issues apparent.
- IPv6 handling avoids re-joining the multicast group during refresh and sends UNAs as expected; IPv4 refresh sends GARPs to broadcast.
- Defaults and CRD/OpenAPI/docs updates look consistent. One thing to verify (not necessarily a bug): if Felix expects this parameter to be hot-reloadable at runtime, the current plumbing captures the interval only at manager construction; there’s no live update path to adjust existing listeners. If dynamic updates are required, you’d need to plumb a config change into the running listeners and update their announceInterval.
🤖 Council of Claudes · 0 inline comment(s)
|
|
||
| // The initial announce fires once; the periodic refresh then keeps | ||
| // sending more GARPs for the same IP without any reconcile. | ||
| Eventually(func() int { return len(arpClients["eth0"].getWrites()) }).Should(Equal(1)) |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The IPv4 periodic re-announce test uses tight time-based assertions (Eventually with a 100ms timeout and a 40ms interval). This is prone to CI flake on loaded runners.
- Suggestion: Either:
- Increase the Eventually timeout window to something more forgiving (e.g., 300–500ms) and/or reduce the polling interval for responsiveness, or
- Better: inject a test clock/ticker into the listener (via an interface passed to newProxyNeighManagerWithShims) so you can deterministically advance time in the test and assert re-announces without relying on wall-clock delays.
There was a problem hiding this comment.
This is correct. I had the same concern, and thought about fixing it at some point.
| Expect(mgr.CompleteDeferredWork()).To(Succeed()) | ||
|
|
||
| // Periodic refresh keeps sending unsolicited NAs for the owned IP. | ||
| Eventually(func() int { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Same flakiness concern as the IPv4 test: the IPv6 periodic re-announce test also uses a 100ms Eventually timeout with short intervals. Please increase the timeout or (preferably) use a fake clock/ticker to drive the loop deterministically.
| Expect(mgr.announceInterval).To(Equal(90 * time.Second)) | ||
| }) | ||
|
|
||
| It("disables periodic re-announcement when the config interval is zero", func() { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The “disables periodic re-announcement when the config interval is zero” test only asserts that announceInterval is zero on the manager, not the runtime behavior. This misses regressions where reannounces could still occur.
- Suggestion: Add a behavioral test that:
- Sets interval 0, programs an owned IP, asserts the initial GARP/NA occurs once, and then Consistently over a suitable window (e.g., 300–500ms) verifies no additional writes happen. This will catch accidental re-announce despite a zero interval.
| defer l.closeSocket() | ||
|
|
||
| // Anchor the periodic re-announce clock to listener start so the first | ||
| // refresh fires announceInterval from now, not immediately after the |
There was a problem hiding this comment.
🧪 Maintainability & Tests — runListener anchors lastAnnounce to time.Now and uses time.Since in the loop. Using wall-clock time in logic that’s under test can make UTs brittle and hard to reason about if this logic evolves (e.g., more complex cadence).
- Suggestion: Consider factoring the timing dependency behind a small interface (e.g., a Clock with Now() and a Ticker-like abstraction) and pass it into newProxyNeighManagerWithShims. In production, use the real time; in tests, use a fake clock to deterministically trigger reannounces. This will improve test determinism and future maintainability of the loop logic.
There was a problem hiding this comment.
This is correct, but also similar to the first two comments in the UT. So feels 3 comments about the same issue.
There was a problem hiding this comment.
🧪 Council of Claudes — Maintainability & Tests
Tip
Maintainability & Tests lens · simplicity · tests · docs · idioms
Well-tested and maintainable overall; main risk is timing-based flakiness in new unit tests. Consider a test-time clock/ticker or more generous timeouts. Also add a behavioral test for “interval = 0” to ensure no periodic re-announcement occurs beyond the initial announce.
- Good: The change threads a single new config through API → OpenAPI/CRD/manifests → Felix config → dataplane with consistent naming and defaults. Generated files are updated. The dataplane code factors the behavior cleanly (announce and reannounceAll), and the tests exercise both IPv4 and IPv6 behavior, plus basic config wiring.
- Maintainability: The periodic loop uses wall-clock time (time.Now/Since) and short durations in UTs to prove behavior. That’s a classic source of flakes; long-term, consider injecting a clock/ticker interface into the listener to make the loop deterministic under test.
- Tests: Solid coverage for “does re-announce periodically” and “config value is wired.” Missing a behavioral test that “0 disables periodic re-announce” (currently only asserts the field is zero on the manager). Adding that would catch regressions where the loop might still re-announce when interval==0.
- Comments/Docs: The description for the new config is consistent across API, CRDs, and docs, and clarifies disable-on-zero and the relation to LocalSubnetL2Reachability. Nice.
Line-specific findings:
🤖 Council of Claudes · 4 inline comment(s)
| // neighbor caches and switch forwarding tables warm even when the set of | ||
| // proxied IPs is unchanged. Set to 0 to disable periodic re-announcement, | ||
| // leaving only the one-shot announce when an IP is added. [Default: 120s] | ||
| LocalSubnetL2ReachabilityRefreshInterval time.Duration `config:"seconds;120"` |
There was a problem hiding this comment.
🛡️ Security — The new LocalSubnetL2ReachabilityRefreshInterval lacks bounds/validation, allowing extremely small intervals that could cause sustained L2 broadcast/multicast floods (gratuitous ARPs to broadcast and IPv6 NAs to ff02::1) proportional to the number of proxied IPs. This presents a potential DoS/availability risk on the local subnet if misconfigured. Although the listener loop only checks the interval once per readTimeout (1s in production), a node with many proxied IPs would still transmit a burst of frames for every owned IP each second if the interval is set below that boundary.
Risk:
- Excessive L2 traffic can degrade network performance, trigger switch/IDS protections, and broaden the blast radius of misconfiguration. This is especially acute on noisy subnets or nodes proxying large IP sets.
Suggested mitigations:
- Enforce a sane minimum and maximum in config parsing or at manager construction, e.g. clamp to [5s, 1h] and log a warning if clamped. For example:
- In config parse/validation, reject or clamp values below a minimum (e.g., 5s or 10s).
- Alternatively, clamp in newProxyNeighManagerWithShims before assigning announceInterval:
if announceInterval < 5time.Second {
logrus.Warn("Refresh interval too small; clamping to 5s")
announceInterval = 5time.Second
}
- Rate-limit or spread announcements across ticks to avoid large per-tick bursts (e.g., jitter and per-iteration cap) when many IPs are owned.
- Consider adding OpenAPI/CRD validation with a minimum if feasible (or document hard constraints prominently in config docs and enforce in code).
- Expose metrics for re-announce counts per interface to aid detection of excessive traffic.
There was a problem hiding this comment.
hmm it a good comment to make the developer think about the concern.
There was a problem hiding this comment.
🛡️ Council of Claudes — Security
Caution
Security lens · validation · secrets · authz · isolation
1 potential security issue
- Architectural observation: This change intentionally increases broadcast/multicast ARP/NDP traffic by default (periodic gratuitous ARP/unsolicited NA every 120s) when LocalSubnetL2Reachability is enabled. While useful for resiliency, it can impact L2 domains at scale. Please verify cluster-scoped RBAC around FelixConfiguration remains admin-only. Even with proper RBAC, consider safeguards to prevent accidental misconfiguration leading to excessive L2 traffic (see line-specific finding).
🤖 Council of Claudes · 1 inline comment(s)
| for ip := range l.announced.All() { | ||
| addr, err := netip.ParseAddr(ip) | ||
| if err != nil { | ||
| logrus.WithError(err).WithField("ip", ip).Debug("Failed to parse IP for re-announce") |
There was a problem hiding this comment.
Nell — Do we want to treat this as a Warn (or higher) rather than Debug? IIUC, any string in l.announced should have been derived from a valid netip.Addr earlier, so a parse failure here likely points at a logic bug, and with Debug we might miss it in production. Also, should we drop the bad entry from l.announced so we don’t spin on it every interval?
| logrus.WithError(err).WithField("ip", ip).Debug("Failed to parse IP for re-announce") | |
| logrus.WithError(err).WithField("ip", ip).Warn("Failed to parse IP for re-announce") |
There was a problem hiding this comment.
I like this comment specially about removing it from the map. I thought about how to handle the case. Given that a parsing error here should not happen (since it's already parsed), I did not address it.
Again, it's a good comment to think about the issue.
| // The initial announce fires once; the periodic refresh then keeps | ||
| // sending more GARPs for the same IP without any reconcile. | ||
| Eventually(func() int { return len(arpClients["eth0"].getWrites()) }).Should(Equal(1)) | ||
| Eventually(func() int { |
There was a problem hiding this comment.
Nell — nit: This Eventually window seems a bit tight vs testAnnounceInterval/read loop wakeups (40ms/10ms). To reduce flake, OK to stretch the timeout slightly?
| Eventually(func() int { | |
| }, "200ms").Should(BeNumerically(">=", 2)) |
There was a problem hiding this comment.
Correct. This is similar to the first comments about the flake source in UT. We should either increase time out or inject time.
| mgr.OnUpdate(wepUpdateV6("k8s", "default/pod1", "eth0", "fd00::50/128")) | ||
| Expect(mgr.CompleteDeferredWork()).To(Succeed()) | ||
|
|
||
| // Periodic refresh keeps sending unsolicited NAs for the owned IP. |
There was a problem hiding this comment.
Nell — Same here — would you mind giving the IPv6 case the same extra headroom to avoid CI flakes?
| // Periodic refresh keeps sending unsolicited NAs for the owned IP. | |
| }, "200ms").Should(BeNumerically(">=", 2)) |
| // forwarding tables stay warm even when the desired set is unchanged. | ||
| // The loop wakes at least every readTimeout, so this fires within one | ||
| // readTimeout of the interval elapsing. | ||
| if l.announceInterval > 0 && time.Since(l.lastAnnounce) >= l.announceInterval { |
There was a problem hiding this comment.
Nell — Small question on intent: we anchor lastAnnounce at listener start (so the first periodic fires announceInterval after start), and applyDesiredState does the initial one-shot announce on add. That matches the comment — just checking we don’t also want to reset lastAnnounce on reconcile (e.g., when the desired set changes) to avoid an immediate re-announce right after a bulk add? It’s probably fine as-is, but WDYT?
| NodeZone: felixNodeZone, | ||
| FloatingIPsEnabled: strings.EqualFold(configParams.FloatingIPs, string(apiv3.FloatingIPsEnabled)), | ||
| LocalSubnetL2Reachability: configParams.LocalSubnetL2Reachability, | ||
| LocalSubnetL2ReachabilityRefreshInterval: configParams.LocalSubnetL2ReachabilityRefreshInterval, |
There was a problem hiding this comment.
Nell — IIUC we only construct and run the proxy-neighbour machinery when LocalSubnetL2Reachability is enabled, so passing a non-zero interval here won’t have side-effects when the feature is Disabled. Just checking that’s the current flow — or do we want to gate the interval at source as well?
There was a problem hiding this comment.
it's too nit picking. I don't think Nell would add such a comment:-D
| Expect(mgr.announceInterval).To(Equal(90 * time.Second)) | ||
| }) | ||
|
|
||
| It("disables periodic re-announcement when the config interval is zero", func() { |
There was a problem hiding this comment.
Nell — Non-blocking: We assert announceInterval==0 when unset; do we want a small behavioural test that verifies “no extra writes arrive” over, say, 2× testAnnounceInterval when the interval is zero? Might catch accidental future regressions. Fine to defer.
There was a problem hiding this comment.
Make sense. but also is duplicate.
There was a problem hiding this comment.
Council of Claudes — Nell
Important
Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments
1 potential correctness issue; a couple of small test robustness nits and one logging/severity question
Overall looks neat — the periodic re-announce is plumbed end-to-end (API → Felix config → dataplane manager), docs/defaults line up, and there are unit tests for both v4/v6 paths plus the config wiring. One thing I’d like to double-check is error handling in the periodic loop: we currently swallow a parse failure with a Debug; suspect that’s either impossible (so we can Warn) or indicates a real bug worth surfacing. Minor: the “within 100ms” Eventuallys feel a bit tight/flaky given the timing matrix here; can we give them a touch more headroom? Finally, is the interval intentionally fixed for the lifetime of the manager, or do we plan to react to FelixConfiguration changes at runtime later?
Line-specific findings:
🤖 Council of Claudes · 6 inline comment(s)
| NodeZone: felixNodeZone, | ||
| FloatingIPsEnabled: strings.EqualFold(configParams.FloatingIPs, string(apiv3.FloatingIPsEnabled)), | ||
| LocalSubnetL2Reachability: configParams.LocalSubnetL2Reachability, | ||
| LocalSubnetL2ReachabilityRefreshInterval: configParams.LocalSubnetL2ReachabilityRefreshInterval, |
There was a problem hiding this comment.
Casey — I think we’re passing the refresh interval unconditionally — can you confirm the proxy-neigh manager (and its listeners) are only started when LocalSubnetL2Reachability != Disabled? If not, I think we should either zero the interval when the mode is Disabled or skip creating the manager entirely so we don’t spin a periodic loop that will never announce anything. WDYT?
There was a problem hiding this comment.
nit picking. I don't think Casey would add this comment.
| // Anchor the periodic re-announce clock to listener start so the first | ||
| // refresh fires announceInterval from now, not immediately after the | ||
| // initial announce that applyDesiredState performs below. | ||
| l.lastAnnounce = time.Now() |
There was a problem hiding this comment.
Casey — I wonder about anchoring lastAnnounce at listener start — if a reconcile happens long after startup, we could end up with the initial announce followed almost immediately by the periodic refresh (since the interval since start has already elapsed). Not harmful, but a little bursty. Maybe we should reset lastAnnounce on reconcile (e.g., when we first announce a new desired IP) so the periodic timer spaces out from the initial announce? But I could be swayed either way.
There was a problem hiding this comment.
This could be a comment from Casey.
The comment makes sense, but it's not a hard issue. Addressing it needs keeping announcement time per IP which might not worth it.
| // listener currently owns, refreshing neighbor caches and switch tables without | ||
| // changing group membership. Driven by announceInterval from the listener loop. | ||
| func (l *ifaceListener) reannounceAll() { | ||
| for ip := range l.announced.All() { |
There was a problem hiding this comment.
Casey — nit: We parse the IP on every periodic pass. Doesn’t hurt really, but since announced is goroutine-local you could store netip.Addr instead of string to avoid re-parsing here. Fine to leave as-is, though.
| // neighbor caches and switch forwarding tables warm even when the set of | ||
| // proxied IPs is unchanged. Set to 0 to disable periodic re-announcement, | ||
| // leaving only the one-shot announce when an IP is added. [Default: 120s] | ||
| LocalSubnetL2ReachabilityRefreshInterval time.Duration `config:"seconds;120"` |
There was a problem hiding this comment.
Casey — Docs say “Set to 0 to disable” but the code disables for any value <= 0. I think we should either validate non-negative (and clamp negatives to 0), or update the docs to say “0 or negative disables.” Personally I’d lean toward validating non-negative to avoid surprising users. WDYT?
There was a problem hiding this comment.
API validation should not allow negative values.
| Controls how often Felix re-announces | ||
| (gratuitous ARP / unsolicited NA) every IP it proxies ARP/NDP for when | ||
| LocalSubnetL2Reachability is enabled, keeping neighbor caches and switch | ||
| forwarding tables warm even when the set of proxied IPs is unchanged. Set to 0 |
There was a problem hiding this comment.
Casey — I think this should mention that negative values also disable, to match the code path that checks interval > 0:
| forwarding tables warm even when the set of proxied IPs is unchanged. Set to 0 | |
| forwarding tables warm even when the set of proxied IPs is unchanged. Set to 0 (or a negative value) to disable periodic re-announcement, leaving only the one-shot announce when an IP is added. |
If we decide to validate as non-negative instead, then the code should clamp and we can keep the current docs.
There was a problem hiding this comment.
API validation must not allow negative value.
| Eventually(func() int { return len(arpClients["eth0"].getWrites()) }).Should(Equal(1)) | ||
| Eventually(func() int { | ||
| return len(arpClients["eth0"].getWrites()) | ||
| }, "100ms").Should(BeNumerically(">=", 2)) |
There was a problem hiding this comment.
Casey — This “100ms” Eventually window feels a bit tight given testAnnounceInterval=40ms and scheduler variance — I think we should give this a little more headroom (e.g., 250–500ms) to reduce CI flake risk. We can keep the assertion >=2 as-is, just bump the timeout. WDYT?
There was a problem hiding this comment.
Correc, but also it's duplicate of other comments.
There was a problem hiding this comment.
Council of Claudes — Casey
Warning
Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness
No obvious dataplane correctness bugs, but I think there are a couple of timing/validation questions and a small flake risk in the new unit tests. Also, can we double-check the manager is only active when LocalSubnetL2Reachability is enabled? If not, we might want to gate the interval or the manager startup on that mode.
- I wonder about validation: the docs say “Set to 0 to disable,” but the code path disables for any value <= 0. If we want to keep that behavior, I think we should either (a) validate non-negative and clamp negatives to 0, or (b) document that negatives also disable. Right now they’re a bit out of sync.
- Re: timer alignment: setting lastAnnounce at listener start means a reconcile that happens a long time after startup can cause an immediate (or near-immediate) re-announce after the initial announce. Probably fine, but if we want to avoid back-to-back announces we could reset the periodic clock on reconcile too. WDYT?
- Tests: the “100ms” Eventually window feels a bit tight relative to scheduling variance; I think we should bump it to make CI flake less likely.
- Minor nit/perf: re-parsing IP strings on every periodic pass isn’t costly here, but storing netip.Addr (instead of string) in announced would simplify this loop.
🤖 Council of Claudes · 6 inline comment(s)
There was a problem hiding this comment.
Pull request overview
This PR adds a new Felix configuration knob to periodically re-announce (gratuitous ARP / unsolicited NA) the IPs that Felix is proxying for LocalSubnetL2Reachability, to keep neighbor caches and L2 forwarding tables warm even when the proxied IP set is stable.
Changes:
- Introduces
LocalSubnetL2ReachabilityRefreshInterval/localSubnetL2ReachabilityRefreshIntervalin Felix config/API/CRDs and documents it (default: 120s, 0 disables). - Implements periodic re-announcement in the proxy neighbor manager listener loop.
- Adds unit tests to validate periodic GARP/UNA behavior and config plumbing.
Reviewed changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| manifests/v3_projectcalico_org.yaml | Adds the new FelixConfiguration CRD field to the published v3 manifest schema. |
| manifests/v1_crd_projectcalico_org.yaml | Adds the new FelixConfiguration CRD field to the published v1 CRD manifest schema. |
| manifests/operator-crds.yaml | Adds the new FelixConfiguration CRD field to operator CRD bundle. |
| manifests/flannel-migration/calico.yaml | Adds the new FelixConfiguration CRD field to flannel-migration manifest bundle. |
| manifests/crds.yaml | Adds the new FelixConfiguration CRD field to the generic CRDs manifest bundle. |
| manifests/canal.yaml | Adds the new FelixConfiguration CRD field to canal manifest bundle. |
| manifests/calico.yaml | Adds the new FelixConfiguration CRD field to the main Calico manifest bundle. |
| manifests/calico-vxlan.yaml | Adds the new FelixConfiguration CRD field to the VXLAN manifest bundle. |
| manifests/calico-v3-crds.yaml | Adds the new FelixConfiguration CRD field to the v3 CRDs-only bundle. |
| manifests/calico-typha.yaml | Adds the new FelixConfiguration CRD field to the Calico+Typha manifest bundle. |
| manifests/calico-policy-only.yaml | Adds the new FelixConfiguration CRD field to the policy-only manifest bundle. |
| manifests/calico-bpf.yaml | Adds the new FelixConfiguration CRD field to the eBPF manifest bundle. |
| libcalico-go/config/crd/crd.projectcalico.org_felixconfigurations.yaml | Adds the new field to libcalico-go’s FelixConfiguration CRD schema. |
| felix/docs/config-params.md | Documents the new config parameter and its encodings/defaults. |
| felix/docs/config-params.json | Adds the new config parameter to the generated config parameter index. |
| felix/dataplane/linux/proxy_neigh_mgr.go | Implements periodic re-announcement behavior in ARP/NDP listeners. |
| felix/dataplane/linux/proxy_neigh_mgr_test.go | Adds tests for periodic re-announcement and config propagation. |
| felix/dataplane/linux/int_dataplane.go | Plumbs the new refresh interval through internal dataplane config. |
| felix/dataplane/driver.go | Passes the new config value from Felix config params into the dataplane config. |
| felix/config/config_params.go | Adds the new config parameter to Felix’s runtime config parsing (seconds; default 120). |
| api/pkg/openapi/generated.openapi.go | Updates generated OpenAPI schema to include the new field. |
| api/pkg/client/applyconfiguration_generated/projectcalico/v3/felixconfigurationspec.go | Updates generated applyconfiguration types/builders for the new field. |
| api/pkg/client/applyconfiguration_generated/internal/internal.go | Updates generated applyconfiguration internal schema for the new field. |
| api/pkg/apis/projectcalico/v3/zz_generated.deepcopy.go | Updates generated deepcopy for the new field. |
| api/pkg/apis/projectcalico/v3/felixconfig.go | Adds the new FelixConfiguration API field. |
| api/config/crd/projectcalico.org_felixconfigurations.yaml | Adds the new field to the API CRD schema output. |
Files not reviewed (3)
- api/pkg/apis/projectcalico/v3/zz_generated.deepcopy.go: Generated file
- api/pkg/client/applyconfiguration_generated/internal/internal.go: Generated file
- api/pkg/client/applyconfiguration_generated/projectcalico/v3/felixconfigurationspec.go: Generated file
| // LocalSubnetL2ReachabilityRefreshInterval controls how often Felix re-announces | ||
| // (gratuitous ARP / unsolicited NA) every IP it proxies ARP/NDP for when | ||
| // LocalSubnetL2Reachability is enabled, keeping neighbor caches and switch | ||
| // forwarding tables warm even when the set of proxied IPs is unchanged. Set to 0 | ||
| // to disable periodic re-announcement, leaving only the one-shot announce when an | ||
| // IP is added. [Default: 120s] | ||
| // +optional | ||
| LocalSubnetL2ReachabilityRefreshInterval *metav1.Duration `json:"localSubnetL2ReachabilityRefreshInterval,omitempty" configv1timescale:"seconds"` |
| // The initial announce fires once; the periodic refresh then keeps | ||
| // sending more GARPs for the same IP without any reconcile. | ||
| Eventually(func() int { return len(arpClients["eth0"].getWrites()) }).Should(Equal(1)) | ||
| Eventually(func() int { | ||
| return len(arpClients["eth0"].getWrites()) | ||
| }, "100ms").Should(BeNumerically(">=", 2)) |
Benchmark sample reproducing the as-first-reviewed state of projectcalico/calico#12960 (by @mazdakn).
This PR's diff equals what the human reviewers first saw: fork point
c7a6e5b59dd7→ earliest human-reviewed commit3e604dac594a. The original PR drew 4 human top-level review comments — the ground truth to compare the Council's feedback against.🤖 Generated for the Council of Claudes benchmark.