[sample #12960] [v2] felix: periodically re-announce proxy-neighbor IPs

[stevegaossou]

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 commit 3e604dac594a. 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.

[Copilot]

Pull request overview

Adds a new Felix configuration knob to periodically re-announce proxied neighbor entries (gratuitous ARP / unsolicited NA) when LocalSubnetL2Reachability is enabled, wiring it through Felix config/API/CRDs and implementing the periodic refresh in the proxy neighbor manager.

Changes:

• Introduces LocalSubnetL2ReachabilityRefreshInterval as a Felix config parameter across API types, CRDs/manifests, and docs.
• Implements periodic re-announcement in proxy_neigh_mgr driven by the new interval.
• Adds unit tests to validate periodic ARP/NA refresh behavior and correct config wiring.

Reviewed changes

Copilot reviewed 23 out of 26 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
manifests/v3_projectcalico_org.yaml	Adds the new FelixConfiguration field to rendered manifests schema.
manifests/v1_crd_projectcalico_org.yaml	Adds the new FelixConfiguration field to rendered CRD manifests schema.
manifests/operator-crds.yaml	Adds the new FelixConfiguration field to operator CRDs bundle.
manifests/flannel-migration/calico.yaml	Adds the new FelixConfiguration field to flannel-migration manifest schema.
manifests/crds.yaml	Adds the new FelixConfiguration field to CRDs manifest schema.
manifests/canal.yaml	Adds the new FelixConfiguration field to canal manifest schema.
manifests/calico.yaml	Adds the new FelixConfiguration field to calico manifest schema.
manifests/calico-vxlan.yaml	Adds the new FelixConfiguration field to vxlan manifest schema.
manifests/calico-v3-crds.yaml	Adds the new FelixConfiguration field to v3 CRDs manifest schema.
manifests/calico-typha.yaml	Adds the new FelixConfiguration field to typha manifest schema.
manifests/calico-policy-only.yaml	Adds the new FelixConfiguration field to policy-only manifest schema.
manifests/calico-bpf.yaml	Adds the new FelixConfiguration field to BPF manifest schema.
libcalico-go/config/crd/crd.projectcalico.org_felixconfigurations.yaml	Adds the new field to libcalico-go CRD YAML.
felix/docs/config-params.md	Documents the new parameter (env var, defaults, schema).
felix/docs/config-params.json	Updates generated config parameter metadata for the new field.
felix/dataplane/linux/proxy_neigh_mgr.go	Implements periodic re-announcement loop and refactors announce logic.
felix/dataplane/linux/proxy_neigh_mgr_test.go	Adds tests covering periodic refresh and config wiring.
felix/dataplane/linux/int_dataplane.go	Extends internal dataplane config to carry the new interval.
felix/dataplane/driver.go	Wires parsed Felix config into internal dataplane config.
felix/config/config_params.go	Defines the new Felix config parameter and its default/encoding.
api/pkg/openapi/generated.openapi.go	Updates OpenAPI schema for the new FelixConfiguration field.
api/pkg/client/applyconfiguration_generated/projectcalico/v3/felixconfigurationspec.go	Adds applyconfiguration field + fluent setter for the new param.
api/pkg/client/applyconfiguration_generated/internal/internal.go	Updates applyconfiguration internal schema YAML for the new field.
api/pkg/apis/projectcalico/v3/zz_generated.deepcopy.go	Updates deepcopy for the new FelixConfigurationSpec pointer field.
api/pkg/apis/projectcalico/v3/felixconfig.go	Adds the new API field to FelixConfigurationSpec.
api/config/crd/projectcalico.org_felixconfigurations.yaml	Adds the new field to the API CRD YAML schema.

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

[github-actions]

🔎 Council of Claudes — Correctness

Note

Correctness lens · bugs · completeness · concurrency · edge cases

1 potential correctness issue. Overall the implementation of periodic re-announcement is coherent: the interval is plumbed from API -> felix config -> int dataplane -> proxy neighbor manager, the listener does one-shot announces on add and periodic re-announces without rejoining groups, and zero/negative intervals disable the periodic behavior. One thing I’d like to verify (not shown in this diff) is that the proxy-neighbor manager is only constructed/started when LocalSubnetL2Reachability is enabled, otherwise this would spin a background goroutine doing needless work even though no IPs will ever be owned.

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

🧪 Council of Claudes — Maintainability & Tests

Tip

Maintainability & Tests lens · simplicity · tests · docs · idioms

Well-tested overall; minor test flake risks and a few maintainability nits

• Good coverage: the new config surface is plumbed through, docs/CRDs are regenerated, and UTs cover IPv4 and IPv6 periodic re-announcement, group membership, and config propagation.
• Tests lean on wall-clock timing with tight intervals. To reduce future flakes on slower/loaded CI, consider relaxing timeouts, asserting monotonic growth instead of exact equality at a point in time, or injecting a time source/ticker for deterministic control.
• There’s a small mismatch between code comments (“zero or negative disables”) and user-facing docs (“Set to 0 to disable”). Aligning these avoids future confusion.
• Optional simplification: the periodic check could use a ticker (or an injected time source) to decouple from readTimeout in a more idiomatic way, making tests easier and avoiding drift if readTimeout changes.
• Consider adding a focused UT in felix/config for the new param’s default and parsing (there are usually table-driven tests that assert defaults and overrides). The dataplane unit tests check propagation but not the parser/defaults directly.

_{🤖 Council of Claudes · 4 inline comment(s)}

[github-actions]

🛡️ Council of Claudes — Security

Caution

Security lens · validation · secrets · authz · isolation

1 potential security issue

• Architectural observation: The new periodic gratuitous ARP/unsolicited NDP re‑announcement feature is controlled by a cluster-scoped FelixConfiguration field. If an actor with RBAC to modify this object configures an extremely aggressive interval and/or the node owns a large number of proxied IPs, this can generate substantial L2 broadcast/multicast traffic. That poses a potential availability risk (L2 DoS/flood, switch CAM/neighbor cache churn) to the local segment. Consider adding sane lower bounds, jitter, and/or pacing to reduce blast radius even under misconfiguration.

_{🤖 Council of Claudes · 2 inline comment(s)}

[github-actions]

Council of Claudes — Nell

Important

Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments

1 potential flakiness issue; a couple of small polish items and one behaviour question

• Overall looks tidy: config is plumbed end-to-end, comments are useful, and we have unit tests for both IPv4 and IPv6 re-announcement (nice).
• Main concern is test tightness — the 100ms Eventually windows feel racy relative to the announce/read intervals. I think these may flake on slower CI.
• One small behaviour question: do we want to reset the periodic timer after a reconcile so we don’t do a near back-to-back re-announce right after the initial announce?
• Minor logging/naming nits below; nothing blocking.

_{🤖 Council of Claudes · 4 inline comment(s)}

[github-actions]

Council of Claudes — Casey

Warning

Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness

1 potential test flake; a couple of small API/validation nits

• I think the overall approach looks good — threading a pointer Duration through the API and a concrete time.Duration into felix, and driving periodic GARP/UNA from the listener loop feels like the right place.
• I am a little bit skeptical of the exact-equality Eventually in the new unit test — it’s watching a monotonically increasing counter and could miss the instant it equals 1. I think that can flake.
• Minor doc/validation mismatch: the code treats negative intervals as “disabled” (since >0 gates sending), but the docs say “Set to 0 to disable.” I wonder if we should clamp negatives to zero at parse-time or validate non-negative in the API so behavior matches the docs?

_{🤖 Council of Claudes · 3 inline comment(s)}

[github-actions]

🧪 Maintainability & Tests — The “disables periodic re-announcement when the config interval is zero” test only checks the internal field, not the behaviour.

Suggestion:

• Add a behavioural test (IPv4 or IPv6) with LocalSubnetL2ReachabilityRefreshInterval=0 that:
  • Waits for the initial announce, then
  • Asserts Consistently over several intervals that no additional writes occur.

[github-actions]

🧪 Maintainability & Tests — Manual interval bookkeeping (time.Since(lastAnnounce) >= announceInterval) inside the listener loop couples accuracy to readTimeout and requires bespoke timing logic.

Suggestion:

• Consider a time.Ticker (or injected clock/ticker) and add a select case on ticker.C. This:
  • Simplifies the loop,
  • Decouples periodicity from readTimeout,
  • Makes tests easier/less flaky by allowing a stubbed time source.
• If you keep the current approach, consider a brief comment warning that announce cadence is “interval ± readTimeout” so future maintainers don’t tighten assertions unexpectedly.

[github-actions]

🧪 Maintainability & Tests — reannounceAll parses string IPs back to netip.Addr each cycle. This repeats parsing work and introduces potential parse failures.

Suggestion:

• If feasible, store announced as set.Set[netip.Addr] (or maintain both string and parsed forms) to avoid parse-on-each-iteration and reduce error handling here. This also simplifies call sites using netip.Addr.

[github-actions]

🧪 Maintainability & Tests — New config param is added with default and docs, but there’s no unit test here asserting:

• default value (120s),
• parsing from YAML/env,
• zero disables behavior at the config layer.

Suggestion:

• If there’s an existing table-driven UT suite for config parsing/defaults, add this param to it. This catches regressions in the config surface independent of dataplane wiring.

[github-actions]

🛡️ Security — The LocalSubnetL2ReachabilityRefreshInterval accepts arbitrary durations with default 120s and no lower bound. A very small interval (e.g., milliseconds) could cause frequent re-announcements across many IPs, creating an L2 broadcast/multicast flood and availability impact. While the listener loop wakes at least each read timeout (1s), on busy loops it may fire more frequently; and even at 1 Hz, re-announcing thousands of IPs per second can stress the segment.
Mitigation:

• Enforce a minimum interval (for example ≥1s or ≥5s) at parse/validation time; clamp values below the floor and log a warning.
• Optionally enforce a maximum sensible interval to avoid pathological “almost never refresh” behavior if that matters operationally.

[github-actions]

🛡️ Security — reannounceAll() iterates over every owned IP and sends an ARP/NA for each immediately. On hosts proxying large IP sets, this concentrates a large number of broadcasts/multicasts into a single loop iteration, potentially causing microbursts that impact switch/neighbor tables and link capacity.
Mitigation:

• Batch or throttle re-announcements (e.g., cap number per iteration, schedule across multiple iterations, or insert short delays) to smooth traffic.
• Add metrics/logging on number of re-announced IPs and announce duration to detect/configure safely.

[github-actions]

Nell — nit: The schema type here is a duration string; would “Set to 0s” be clearer than “Set to 0”? We use “2m0s” elsewhere, so this keeps it consistent.

Suggested change

	// to disable periodic re-announcement, leaving only the one-shot announce when an
	// forwarding tables warm even when the set of proxied IPs is unchanged. Set to 0s

[github-actions]

Nell — Anchor the clock at start is good to avoid an immediate re-announce, but do we also want to reset lastAnnounce after applyDesiredState? Otherwise, a reconcile that triggers the initial announce could be followed very soon by the periodic re-announce if the interval is nearly up. Maybe that’s fine (gratuitous ARP/NA are cheap), but WDYT?

[github-actions]

Nell — I think 100ms here is quite tight relative to testAnnounceInterval (40ms) and scheduler jitter — likely to flake in CI. Bump the Eventually timeout?

Suggested change

	}, "100ms").Should(BeNumerically(">=", 2))
	}, "300ms").Should(BeNumerically(">=", 2))

[github-actions]

Nell — nit: Consistently without an explicit duration uses Ginkgo’s default (short). Perhaps be explicit so the intent is clear and resilient on slower runners?

Suggested change

	Consistently(func() []netip.Addr {
	Consistently(func() []netip.Addr {
	return ndpConns["eth0"].getJoinedGroups()
	}, "300ms").Should(ConsistOf(want))

[github-actions]

Casey — I think this Eventually(Equal(1)) on a monotonically increasing write count can flake — if the poll misses the brief “1” state and observes “2” first, it’ll never converge. For counters, we usually assert “>= 1” first and then “>= 2” within a bounded window. WDYT about something like:

// Wait for the first write (initial announce).
Eventually(func() int { return len(arpClients["eth0"].getWrites()) }).Should(BeNumerically(">=", 1))

// Then ensure refreshes are happening.
Eventually(func() int {
    return len(arpClients["eth0"].getWrites())
}, "200ms").Should(BeNumerically(">=", 2))

That avoids a race on the exact value and gives a bit more slack for CI scheduling. You could also use Consistently with a small duration to assert it keeps growing if you want to be stricter.

[github-actions]

Casey — Docs say “Set to 0 to disable,” but the code path here disables for any interval <= 0 (since it only fires when announceInterval > 0). I think we should either (a) clamp negatives to zero where we parse the config, or (b) explicitly document “0 or less disables” for consistency. Personally I’d lean to clamping at parse-time so the API validation/docs can stay “non-negative”.

A couple of places we could address this:

• Add a min=0 validation on the API field if we want to prevent negatives there.
• Or, in felix config load, normalize negative durations to zero before plumbing into the dataplane.

Fine to leave the runtime guard as-is either way; it’s just the API/UX bit I’m flagging.

[github-actions]

Casey — Question: anchoring the first periodic tick at listener start means we won’t refresh until one full interval after the initial add-time announce. Was that intentional? If we wanted a “top-up” shortly after add (some switches/hosts age quickly), we could consider starting the clock at the time of the initial announce instead (or setting lastAnnounce to time.Now().Add(-announceInterval/2)). I’m not necessarily against the current choice — just checking on the rationale.