[sample #12451] [v2] Per Pod/LB IP proxy ARP

[stevegaossou]

Benchmark sample reproducing the as-first-reviewed state of projectcalico/calico#12451 (by @MichalFupso).

This PR's diff equals what the human reviewers first saw: fork point bbf9d864bc8b → earliest human-reviewed commit b9eb6ddda003. The original PR drew 85 human top-level review comments — the ground truth to compare the Council's feedback against.

🤖 Generated for the Council of Claudes benchmark.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

🔎 Council of Claudes — Correctness

Note

Correctness lens · bugs · completeness · concurrency · edge cases

3 potential correctness issues

• Manager lifecycle: the new proxy neighbor managers spin long-lived listener goroutines, but there’s no clear shutdown path when Felix stops. InternalDataplane doesn’t appear to cancel these contexts on shutdown, which can leave sockets/goroutines open until process exit. Consider wiring a cleanup in the dataplane’s Stop/teardown to call m.cancel() for each manager (you already store them in dp.proxyNeighManagers) so listeners are deterministically stopped.

• Feature enable check is case-sensitive in one place. Config parsing allows case-insensitive values; comparing with a case-sensitive string can leave the feature disabled when users set a different case form.

• FV test: invalid use of new() with a constant value for HostSubnetNeighResponses. This won’t compile.

Line-specific findings:

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

[github-actions]

🧪 Council of Claudes — Maintainability & Tests

Tip

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

Well-tested overall with strong UT/FV/E2E coverage; a few maintainability nits and one small missing unit test around config gating

• Good job on shipping comprehensive tests at three layers:
  • Unit tests thoroughly exercise ARP/NDP listeners, reconciliation, GARP/UNA, pool filtering, LB VIP selection, and live-migration.
  • FV drives the calc graph end-to-end including the new Service status → ServiceUpdate path.
  • E2E adds dual-stack coverage with realistic L2 topology and skips appropriately via RequiresKindNetwork.
• The new manager is a large single file (~700 lines) but logically cohesive. Consider modest refactors over time (e.g., isolating ARP vs NDP listener code) if it grows further.
• Two maintainability improvements would reduce future friction:
  1. Avoid stringly-typed config checks (use consistent parsing/enum and case-insensitive comparisons) and centralize gating in one place.
  2. Deduplicate docker-network/subnet helper functions shared between e2e and FV into a common utility to reduce drift.
• One targeted test ask: add a small unit test around the HostSubnetNeighResponses gating in NewIntDataplaneDriver so a future change doesn’t accidentally register the manager when Disabled.

Line-specific findings:

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

[github-actions]

🛡️ Council of Claudes — Security

Caution

Security lens · validation · secrets · authz · isolation

3 potential security issues

• Architectural observation: This feature exposes pod and LoadBalancer IPs at L2 to any hosts on the same subnet when enabled. Even if Calico policy still applies at L3/L4, clusters that assume pods are not directly reachable from the host LAN may find this weakens isolation. Carefully consider the default and provide clear operator guidance. Also verify that the interface monitor feeding ifaceAddrsUpdate honors Felix’s InterfaceExclude setting so sensitive uplinks can be excluded from proxy ARP/NDP.

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

[github-actions]

Council of Claudes — Nell

Important

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

2 potential correctness issues

• Behavioural default: Enabling HostSubnetNeighResponses by default feels risky; I think we usually default new dataplane-affecting features to Disabled and then flip later once hardened. Worth confirming intent?
• Error handling: newProxyNeighManager panics on netlink init failure. IIUC this would take down Felix rather than propagate a clean error to the dataplane loop/startup; can we return err up instead?

Also, a few simplifications/clean‑ups:

• We now have near-duplicate helpers (docker network inspect + subnet offset math) in both e2e and felix/fv; can we deduplicate into a small shared test util?
• Case/normalisation: we compare config and service types by exact string. Given the config parser is case-insensitive, maybe normalise once (or use strings.EqualFold) to avoid surprises.
• InternalDataplane stores proxyNeighManagers but doesn't use the slice later; do we need to keep that field?

Finally, is the plumb‑through from FelixConfiguration->felix/config.Config->dataplane complete for this field? I see the CRD field, docs, and the Config field/tag, and it’s passed into int_dataplane.Config, but I don’t see any conversion code changes — suspect it’s automatic, but worth a quick double‑check that a non-default YAML actually reaches configParams.HostSubnetNeighResponses.

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

[github-actions]

Council of Claudes — Casey

Warning

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

1 robustness issue (panic in felix), plus a few naming/clarity items and small maintainability nits. One policy question: do we really want this enabled by default?

• Naming/clarity: “Neigh” reads a bit cryptic to me across the API, config, file/type names, and docs. Could we spell out “Neighbor” (and be consistent with American English “neighbor” vs “neighbour”) and/or lean on a term like “ProxyNeighbor” to capture ARP+NDP more directly? WDYT?
• API/defaults: Perhaps I’ve forgotten the context — is there a reason this ships enabled-by-default? Responding to ARP/NDP from the host is a behavior change that might surprise environments relying on L3 reachability/BGP-only paths. I could be swayed either way, but I’d expect “Disabled” as the safer initial default.
• Manager enable check: We’re string-matching “PodsAndLoadBalancers” in a few places — can we avoid magic strings (use constants or normalize to a bool)?
• Regex safety: Building a regexp from WorkloadIfacePrefixes without quoting can misinterpret metacharacters in prefixes; we should QuoteMeta.
• Tests: e2e/FV look generally good and labeled rather than self-skipping — thanks. One question on FV: adding a new external image (netshoot) tends to add flake surface. Is there a way to reuse our existing infra image or a lighter image we already pull?

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

[github-actions]

🔎 Correctness — The feature enable check is case-sensitive: if config.HostSubnetNeighResponses is set via env/YAML with different casing (allowed by the config schema), this equality will fail and the feature won’t start. Suggest using a case-insensitive check, consistent with other flags (e.g., FloatingIPsEnabled above):

if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") {
    // ...
}

Don’t forget to import "strings".

[github-actions]

🔎 Correctness — This line won’t compile: new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) attempts to use a constant as a type. Use a pointer to the constant value instead:

mode := api.HostSubnetNeighResponsesPodsAndLoadBalancers
fc.Spec.HostSubnetNeighResponses = &mode
// or, if available:
// fc.Spec.HostSubnetNeighResponses = ptr.To(api.HostSubnetNeighResponsesPodsAndLoadBalancers)

[github-actions]

🔎 Correctness — The manager starts listener goroutines and stores them in dp.proxyNeighManagers, but there’s no corresponding shutdown to cancel contexts/close sockets when Felix stops. This risks leaking goroutines/file descriptors across teardown. Recommend invoking a cleanup during dataplane shutdown (e.g., in InternalDataplane.Stop or equivalent) to iterate dp.proxyNeighManagers and call a new Stop/Close method or expose/call m.cancel() to ensure listeners terminate.

[github-actions]

🔎 Correctness — NeighborSolicit uses rdisc6 to send a Neighbor Solicitation. rdisc6 is typically for Router Solicitation/Discovery; for NS/NA, the ndisc6 tool (from the same suite) is usually used. On some images/environments rdisc6 may not emit NS and could cause spurious failures. Consider switching to a tool that explicitly sends a single NS (e.g., ndisc6 <addr> <iface>), or verify netshoot’s rdisc6 behavior and adjust invocation accordingly.

[github-actions]

🧪 Maintainability & Tests — The manager is enabled with a direct string comparison against "PodsAndLoadBalancers". This is brittle (case sensitivity) and spreads stringly-typed policy across components.

• Suggestion:
  • Normalize once at construction and pass an enum/bool to the dataplane. For example, in StartDataplaneDriver, parse with strings.EqualFold and set a typed field:
    • Add to intdataplane.Config: HostSubnetNeighResponsesEnabled bool
    • In driver.go: HostSubnetNeighResponsesEnabled: strings.EqualFold(configParams.HostSubnetNeighResponses, "PodsAndLoadBalancers")
    • Here: if config.HostSubnetNeighResponsesEnabled { ... }
  • Alternatively, if you prefer strings, at least use strings.EqualFold to match the parsing semantics used for FloatingIPs.

[github-actions]

🧪 Maintainability & Tests — HostSubnetNeighResponses is plumbed through as a raw string while FloatingIPs is normalized via strings.EqualFold. This inconsistency invites regressions when values differ in case.

• Suggestion: Normalize to a boolean/enum at this boundary, matching how FloatingIPs is handled:
  dpConfig.HostSubnetNeighResponsesEnabled = strings.EqualFold(configParams.HostSubnetNeighResponses, string(apiv3.HostSubnetNeighResponsesPodsAndLoadBalancers))
  and pass the normalized value into intdataplane.Config.

[github-actions]

🧪 Maintainability & Tests — HostSubnetNeighResponses is stored in intdataplane.Config as a string. This leaks API details into a low-level component and forces string comparisons later.

• Suggestion: Change to a typed/normalized field (e.g., HostSubnetNeighResponsesEnabled bool or an internal enum) and keep string decoding at the config/driver boundary.

[github-actions]

🧪 Maintainability & Tests — Untested branching: the config gate for enabling the proxy-neighbor manager (and the IPv6 sub-manager) isn’t directly covered by a unit/integration test. Since this is the primary on/off switch, accidental regressions here would be easy to miss.

• Ask for a focused unit test (no FV/E2E needed) that:
  • Constructs an InternalDataplane with HostSubnetNeighResponses disabled and asserts no proxyNeighManagers are registered.
  • Constructs one with HostSubnetNeighResponses enabled and IPv6Enabled true and asserts two managers are registered (v4 + v6).
    This exercises the only branch that’s not covered by your current suite and will guard future refactors.

[github-actions]

🧪 Maintainability & Tests — v6SubnetWithSuffix logic (IPv6 math via netip) overlaps functionally with FV’s local helpers. Consolidate into a shared test utility as noted above to prevent divergence and reduce mental overhead.

[github-actions]

🧪 Maintainability & Tests — Starting/stopping listeners logs at Info for every interface. On large hosts this can get noisy and drown out higher-signal logs.

• Suggestion: Consider downgrading “Started/Stopped proxy neighbor listener” to Debug, keeping a one-time “feature enabled” Info log when the manager registers.

[github-actions]

🛡️ Security — Default enables proxy ARP/NDP on host subnets (PodsAndLoadBalancers). Risk: unexpected exposure of pod and VIP addresses to L2-adjacent networks, potentially weakening network isolation and increasing attack surface (scanning/recon, lateral-movement attempts). Mitigation: change default to Disabled; require explicit opt-in. Alternatively, gate by additional scoping (e.g., an allowlist of host interfaces or CIDR filters) and document risks prominently.

[github-actions]

🛡️ Security — Pool filtering only checks encapsulation mode (isNoEncapPool) and ignores AllowedUses. Risk: proxy ARP/NDP may respond for any IP within any no-encap pool, regardless of intended use, increasing chance of claiming unintended addresses (especially for LB VIPs). Mitigation: extend checks to ensure the IP belongs to a pool whose AllowedUses includes the relevant purpose (Workload for pod IPs is implicit via IPAM; LoadBalancer for VIPs should be enforced).

[github-actions]

🛡️ Security — runARPListener replies to every matching ARP request without rate limiting. Risk: L2-adjacent attacker can flood ARP requests for tracked IPs, consuming CPU in user space (potential local DoS). Mitigations: add basic per-interface/global rate limiting/backoff, or consider leveraging kernel-managed proxy_arp entries where feasible; at minimum, log and drop when exceeding thresholds.

[github-actions]

🛡️ Security — runNDPListener replies to Neighbor Solicitations without rate limiting and sends unsolicited NAs for DAD to all-nodes multicast. Risk: high-rate NS/DAD floods from an attacker on L2 can cause CPU/bandwidth load (local DoS) and multicast churn. Mitigation: implement simple rate limiting/burst control per interface/IP, and consider bounding unsolicited NA emissions.

[github-actions]

Nell — We compare msg.Type to "LoadBalancer" as a raw string. K8s uses that exact spelling, but worth considering a constant or at least being explicit about case sensitivity. Do we want to handle unknown casings, or is upstream guaranteed?

[github-actions]

Nell — detectKindNetworkSubnets duplicates the docker network inspect logic also added in felix/fv (inspectDockerNetworkV4). Can we share a small helper (e.g., under a common test utils package) rather than copy?

[github-actions]

Nell — subnetOffset is very close to subnetOffsetV4 in felix/fv. Same thought — can we deduplicate these helpers into a shared test util?

[github-actions]

Casey — I think “Neigh” here is a bit opaque — any reason not to spell this out?

Suggested change

	// +kubebuilder:validation:Enum=Disabled;PodsAndLoadBalancers
	// +kubebuilder:validation:Enum=Disabled;PodsAndLoadBalancers
	type HostSubnetNeighborResponsesMode string
	const (
	HostSubnetNeighborResponsesDisabled HostSubnetNeighborResponsesMode = "Disabled"
	HostSubnetNeighborResponsesPodsAndLoadBalancers HostSubnetNeighborResponsesMode = "PodsAndLoadBalancers"
	)

We tend to prefer descriptive names over abbreviations.

[github-actions]

Casey — Do we still need proxyNeighManagers on the struct? I don’t see it read anywhere — looks like we RegisterManager() and then only keep the slice. Can this be removed?

[github-actions]

Casey — Not sure we want a panic down in felix — can this be an error return instead so we don’t take down the process if netlink init fails?

Suggested change

	nl, err := netlinkshim.NewRealNetlink()
	nl, err := netlinkshim.NewRealNetlink()
	if err != nil {
	log.WithError(err).Error("Failed to create netlink handle for proxy neighbor manager, disabling feature")
	// return a no-op manager, or plumb an error up so the caller can skip registration
	}

Best not to accidentally take down the cluster if we miss something!

[github-actions]

Casey — Small robustness nit: these prefixes aren’t quoted — any regex meta in the configured prefixes could be misinterpreted.

Suggested change

	wlIfacesPattern := "^(" + strings.Join(dpConfig.RulesConfig.WorkloadIfacePrefixes, "|") + ").*"
	quoted := make([]string, len(dpConfig.RulesConfig.WorkloadIfacePrefixes))
	for i, p := range dpConfig.RulesConfig.WorkloadIfacePrefixes {
	quoted[i] = regexp.QuoteMeta(p)
	}
	wlIfacesPattern := "^(" + strings.Join(quoted, "|") + ").*"
	wlIfacesRegexp := regexp.MustCompile(wlIfacesPattern)

[github-actions]

Casey — Could we add a brief note that:

• it only responds for IPs in no-encap pools (Never IPIP/VXLAN), and
• for LoadBalancer VIPs, a single node is selected via hashing to answer ARP/NDP?
  That’ll set expectations about scope and who answers.

[github-actions]

Casey — Is there a reason to introduce a new external image (netshoot) in FV? We try to keep FV image surface small to reduce flakes. If we just need arping/curl, could we reuse an existing infra image or bake the minimal tools into one we already pull? Fine to leave as-is if not practical, though.

[github-actions]

Casey — nit: consistent naming would help find this later — would “proxy_neighbor.go” (and “ProxyNeighbor” in strings) read a bit clearer than “proxy_neigh”? You can take or leave it.

[github-actions]

Casey — Good call to include status.ingress IPs. Question: do we need to dedupe between LoadbalancerIp and LoadbalancerIngressIps when both are present? Probably rare, but easy to append duplicates. Fine to leave if upstream deduping makes it harmless.