[sample #12451] Per Pod/LB IP proxy ARP#201
Conversation
Mirrors the diff the human reviewers first saw on projectcalico#12451 ('Per Pod/LB IP proxy ARP' by @MichalFupso) for the Council of Claudes benchmark. base(fork)=bbf9d864bc8b16ec93522d5eaba0b8f2164c5dbe head(as-reviewed)=b9eb6ddda00317605a351d98515bfeea3c9ce1e8
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if !ok { | ||
| return "", fmt.Errorf("invalid parent IPv6 address %s", parent.IP) | ||
| } | ||
| suffix, err := netip.ParsePrefix("::" + suffixCIDR) |
There was a problem hiding this comment.
🔎 Correctness — v6SubnetWithSuffix builds an invalid IPv6 prefix string and will always fail parsing. You concatenate "::" with a suffix that itself includes a leading colon, producing strings like "::::ff00::/122". This causes netip.ParsePrefix to error, and the IPv6 e2e tests will fail early.
Fix: strip the leading colon from the suffix before concatenation, or accept suffix strings without a leading colon. For example:
func v6SubnetWithSuffix(parent *net.IPNet, suffixCIDR string) (string, error) {
parentAddr, ok := netip.AddrFromSlice(parent.IP.To16())
if !ok {
return "", fmt.Errorf("invalid parent IPv6 address %s", parent.IP)
}
suf := strings.TrimPrefix(suffixCIDR, ":")
suffix, err := netip.ParsePrefix("::" + suf)
if err != nil {
return "", fmt.Errorf("invalid suffix %q: %w", suffixCIDR, err)
}
pBytes := parentAddr.As16()
sBytes := suffix.Addr().As16()
for i := range pBytes {
pBytes[i] |= sBytes[i]
}
return fmt.Sprintf("%s/%d", netip.AddrFrom16(pBytes), suffix.Bits()), nil
}Alternatively, redefine the constants without the leading colon and keep the existing code.
There was a problem hiding this comment.
This test is moved to kind k8st infra and moved from here.
| // active from Felix's first calc-graph flush. | ||
| fc := api.NewFelixConfiguration() | ||
| fc.Name = "default" | ||
| fc.Spec.HostSubnetNeighResponses = new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) |
There was a problem hiding this comment.
🔎 Correctness — This assignment does not compile: new() expects a type, but HostSubnetNeighResponsesPodsAndLoadBalancers is a constant value, not a type.
fc.Spec.HostSubnetNeighResponses = new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) // invalidFix by taking the address of the constant (so it escapes to the heap), or use ptr.To:
v := api.HostSubnetNeighResponsesPodsAndLoadBalancers
fc.Spec.HostSubnetNeighResponses = &v
// or
fc.Spec.HostSubnetNeighResponses = ptr.To(api.HostSubnetNeighResponsesPodsAndLoadBalancers)There was a problem hiding this comment.
this is correct in golang 1.26. Copilot keeps commenting about it though.
| // Register the proxy neighbor managers when enabled. They listen on raw sockets | ||
| // and respond to ARP (IPv4) / NDP (IPv6) requests for pod and LB IPs that fall | ||
| // within the same subnet as a host physical interface. | ||
| if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" { |
There was a problem hiding this comment.
🔎 Correctness — Proxy-neighbor managers are enabled based on a case-sensitive string comparison:
if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" {Elsewhere (e.g., FloatingIPsEnabled) you use case-insensitive checks to tolerate different encodings. If the config layer doesn’t guarantee canonical casing, this can silently disable the feature. Consider:
if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") {
...
}
There was a problem hiding this comment.
🔎 Council of Claudes — Correctness
Note
Correctness lens · bugs · completeness · concurrency · edge cases
3 potential correctness issues found
- Please double-check lifecycle management for the new proxyNeigh managers. They create background goroutines with an internal context (m.ctx), but I don’t see a corresponding cancel being called on Felix shutdown or driver teardown. If InternalDataplane doesn’t invoke a Stop/cleanup that calls mgr.cancel(), those goroutines and raw sockets can remain active until process exit. If there’s an existing shutdown hook that should be used, wiring it up to cancel these managers would avoid leaks. If not, consider adding one.
- Enabling proxy-neigh managers is gated with a case-sensitive string compare. The config parser typically normalizes values, but if a different casing slips through, IPv4/IPv6 managers won’t start even though the value is logically “PodsAndLoadBalancers.” Consider using a case-insensitive check, consistent with other booleans (e.g., FloatingIPs).
🤖 Council of Claudes · 3 inline comment(s)
| // Register the proxy neighbor managers when enabled. They listen on raw sockets | ||
| // and respond to ARP (IPv4) / NDP (IPv6) requests for pod and LB IPs that fall | ||
| // within the same subnet as a host physical interface. | ||
| if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Use a constant or a case-insensitive comparison for the mode check instead of a magic string. Today this is:
if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" { ... }
Prefer normalizing once and/or using strings.EqualFold:
if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") { ... }
Also consider defining mode constants alongside the config parsing to avoid repeating string literals.
| ExternalIps: s.Spec.ExternalIPs, | ||
| } | ||
|
|
||
| // Extract LoadBalancer ingress IPs from Status (the actual assigned IPs). |
There was a problem hiding this comment.
🧪 Maintainability & Tests — You’ve extended kubernetesServiceToProto to include Status.LoadBalancer.Ingress IPs, which is great. Recommend adding a small unit test for this function to assert:
- Ingress IPs are included when present (both single and multiple).
- Type filtering still works as expected.
This avoids relying only on FV for this conversion behavior.
| }, | ||
| ) | ||
|
|
||
| // detectKindNetworkSubnets returns the IPv4 and IPv6 subnets configured on the |
There was a problem hiding this comment.
🧪 Maintainability & Tests — detectKindNetworkSubnets re-implements Docker network inspection. There’s a near-duplicate in felix/fv/proxy_neigh_test.go (inspectDockerNetworkV4). Suggest factoring these helpers into a shared test utility package to reduce duplication and future drift.
| }, | ||
| ) | ||
|
|
||
| // inspectDockerNetworkV4 returns the IPv4 subnet configured on the given |
There was a problem hiding this comment.
🧪 Maintainability & Tests — inspectDockerNetworkV4 re-implements Docker network inspection already present in e2e tests (detectKindNetworkSubnets). To keep maintenance low, please move these helpers to a shared test util and reuse them from both FV and E2E.
| return v4, v6, nil | ||
| } | ||
|
|
||
| // subnetOffset returns the CIDR formed by OR-ing the offset's address bits |
There was a problem hiding this comment.
🧪 Maintainability & Tests — subnetOffset duplicates the logic later re-written as subnetOffsetV4 in felix/fv/proxy_neigh_test.go. Consider moving a single implementation (and an IPv6 variant) to a shared test utility to prevent drift and make future bug fixes atomic.
| return nil, nil | ||
| } | ||
|
|
||
| // subnetOffsetV4 returns the CIDR formed by OR-ing the offset's address bits |
There was a problem hiding this comment.
🧪 Maintainability & Tests — subnetOffsetV4 duplicates the IPv4 subnet-offset logic already implemented in E2E. Please unify this helper in a shared test utility to reduce duplication and improve maintainability.
|
|
||
| // Reconcile listeners: start new ones, stop removed ones. | ||
| var startErr error | ||
| for ifaceName := range desiredByIface { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — When starting listeners, CompleteDeferredWork re-marks the manager as dirty and returns the error for retry. Consider adding a unit test that simulates arpFactory/ndpFactory failure to verify:
- dirty is set back to true,
- the error is surfaced on first failure,
- a subsequent CompleteDeferredWork retries and succeeds.
This makes the backoff/retry behavior regression-proof.
| continue | ||
| } | ||
|
|
||
| // If the NS source is the unspecified address (::), this is a |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The NDP path handles DAD (unspecified source) by sending an unsolicited NA to ff02::1 and clearing Solicited. Add a unit test to cover this path to ensure future refactors don’t inadvertently change this subtle but important behavior.
| arpClients = map[string]*mockARPClient{ | ||
| "eth0": newMockARPClient(net.HardwareAddr{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0x01}), | ||
| } | ||
| mgr = newTestProxyNeighManagerWithHostname(nl, arpClients, "node-c") |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The “LB IP on selected node” test hard-codes that 10.0.0.100 maps to node-c. This couples the test to a specific rendezvous-hash outcome and node name, making it fragile if hashing details change. Instead, compute the selected node within the test (e.g., instantiate managers for each node and ask selectNodeForIP) and assert only the selected one advertises.
| arpClients = map[string]*mockARPClient{ | ||
| "eth0": newMockARPClient(net.HardwareAddr{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0x01}), | ||
| } | ||
| mgr = newTestProxyNeighManagerWithHostname(nl, arpClients, "node-a") |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Similarly, the “LB IP on non-selected node” test assumes that node-a is not selected for 10.0.0.100. Derive the non-selected node dynamically to avoid brittleness if hashing inputs or implementation change.
| } | ||
| } | ||
|
|
||
| // Register the proxy neighbor managers when enabled. They listen on raw sockets |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Given this is the only place managers are registered based on HostSubnetNeighResponses, add a small unit/integration test for the configuration gating (e.g., ensure no proxyNeighManager is registered when Disabled). That will guard against accidental regressions in start-up wiring.
| // active from Felix's first calc-graph flush. | ||
| fc := api.NewFelixConfiguration() | ||
| fc.Name = "default" | ||
| fc.Spec.HostSubnetNeighResponses = new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Setting the FelixConfiguration uses new(api.HostSubnetNeighResponsesPodsAndLoadBalancers). Prefer taking the address of a typed variable or using a helper like ptr.To:
mode := api.HostSubnetNeighResponsesPodsAndLoadBalancers
fc.Spec.HostSubnetNeighResponses = &mode
This avoids compilation issues and matches the pattern used in the E2E helper.
| return string(out), err | ||
| } | ||
|
|
||
| // dockerRun runs `docker <args...>` and surfaces any error with the combined |
There was a problem hiding this comment.
🧪 Maintainability & Tests — dockerRun invokes exec.Command without a context or deadline. For long-running or hung docker invocations, this can stall the test runner. Consider using exec.CommandContext with a bounded timeout, or plumb a context parameter, to make failures more diagnosable and tests less flaky.
There was a problem hiding this comment.
🧪 Council of Claudes — Maintainability & Tests
Tip
Maintainability & Tests lens · simplicity · tests · docs · idioms
Missing a few maintainability refinements; tests are broad but have some brittle spots
- Overall, the feature is well-covered: you added unit tests (IPv4/IPv6, pool/iface filtering, live migration, LB selection), an FV to exercise the calc-graph → manager path (including Service status), and E2E that validates L2 adjacency via a docker peer. Nice job covering both pods and LB VIPs and both families.
- The new manager is a single 700+ line file handling state tracking, reconciliation, raw-socket listeners, ARP and NDP logic, hashing, and host-iface CIDR tracking. Consider splitting into smaller files or extracting protocol-specific listener logic (ARP/NDP) to reduce cognitive load and make future changes safer.
- Avoid magic strings for mode checks; normalize once and use constants or case-insensitive comparisons. Today the gating in int_dataplane uses a literal and case-sensitive comparison, which increases maintenance risk if config parsing ever varies case.
- A couple of new helpers (subnet offset and docker network inspection) are duplicated between E2E and FV. Factor them into a shared test util to avoid divergence.
- Tests for LB “selected vs non-selected node” are brittle because they hard-code the expected rendezvous-hash outcome for specific node names and VIPs. If the hash function or inputs change, tests will fail for non-functional reasons. Instead, derive the selected node within the test and assert behavior relative to that, not to hard-coded hostnames.
- Consider adding a simple UT to verify dataplane_passthru includes LoadBalancer ingress IPs in the proto message. Currently only FV exercises that path.
- Add a focused UT for the Disabled mode (manager should not be registered) to catch future regressions in the start-up gating.
- In FV, setting HostSubnetNeighResponses uses new(T) on what appears to be a const identifier; use a pointer to a typed variable or ptr helper consistently (as done in the E2E code) so the test compiles robustly.
- Optionally, add a unit test for the NDP DAD (unspecified source) branch and one that exercises the retry path when listener start fails, so future changes don’t break these subtleties unnoticed.
Line-specific findings:
🤖 Council of Claudes · 13 inline comment(s)
| lbIPs = append(lbIPs, ipStr) | ||
| } | ||
| } | ||
| if msg.LoadbalancerIp != "" && m.isMatchingIPVersion(msg.LoadbalancerIp) { |
There was a problem hiding this comment.
🛡️ Security — Accepts and ARPs/NDPs for user-specified Service.spec.loadBalancerIP without controller approval
- Risk: The manager treats both Status.loadBalancer.ingress IPs and Spec.loadBalancerIP (LoadbalancerIp) as eligible VIPs. While Status ingress IPs are assigned by a privileged controller, Spec.loadBalancerIP can be set by unprivileged namespace users. With this change, a user can pick an arbitrary IP inside any no-encap pool and cause some node to answer ARP/NDP for it, potentially hijacking an address (even one not allocated for LoadBalancer use) on the local L2. There is no check of IPPool AllowedUses here; the code only checks “no encap,” which may include workload-only pools.
- Mitigations:
- Only respond for VIPs present in Status.LoadBalancer.Ingress (controller-approved); ignore Spec.loadBalancerIP entirely in proxy_neigh.
- Alternatively, enforce that any candidate VIP is in an IPPool with AllowedUse=LoadBalancer (you’ll likely need to plumb AllowedUses into the proto.IPAMPoolUpdate and track it here).
- Document and gate an option to allow Spec.loadBalancerIP only when a cluster-admin explicitly enables it.
| // HostSubnetNeighResponses controls whether Felix programs per-IP proxy ARP/NDP | ||
| // neighbour entries on host interfaces for local pod IPs and LB VIPs that overlap the | ||
| // host subnet. [Default: PodsAndLoadBalancers] | ||
| HostSubnetNeighResponses string `config:"oneof(Disabled,PodsAndLoadBalancers);PodsAndLoadBalancers"` |
There was a problem hiding this comment.
🛡️ Security — Default-on setting increases exposure by replying on L2 by default
- Risk: HostSubnetNeighResponses defaults to PodsAndLoadBalancers, turning on ARP/NDP replies across host interfaces out of the box. On clusters where host uplinks are L2-adjacent to broader networks, this can unintentionally expose pod/LB IPs outside the cluster perimeter and may conflict with expected routing- or BGP-only exposure.
- Mitigations:
- Change the default to Disabled, with explicit opt-in.
- If default must remain enabled, pair it with strict interface scoping (allowlist) and clear docs about the exposure impact.
| func (m *proxyNeighManager) OnUpdate(protoBufMsg any) { | ||
| switch msg := protoBufMsg.(type) { | ||
| case *ifaceAddrsUpdate: | ||
| if m.wlIfacesRegexp.MatchString(msg.Name) { |
There was a problem hiding this comment.
🛡️ Security — Potential to reply on unintended host interfaces (missing explicit exclude/allow checks here)
- Risk: The manager ignores workload veths via wlIfacesRegexp but otherwise accepts ifaceAddrsUpdate for any interface name and then opens raw sockets to answer ARP/NDP. If ifaceAddrsUpdate includes management/uplink/bridge interfaces not meant for external exposure (and InterfaceExclude is not applied upstream), this may leak pod/LB reachability onto unintended L2 segments.
- Mitigations:
- Ensure ifaceAddrsUpdate is sourced from an ifacemonitor that already applies Felix’s InterfaceExclude; if not, apply the same exclude in this manager.
- Consider adding a dedicated config knob to allowlist/denylist which host interfaces this feature can bind to.
| return ip.To4() == nil | ||
| } | ||
|
|
||
| func isNoEncapPool(pool *proto.IPAMPool) bool { |
There was a problem hiding this comment.
🛡️ Security — No validation of IPPool AllowedUses when deciding which IPs are eligible
- Risk: isNoEncapPool() and isInNoEncapPool() only check encapsulation modes. A LoadBalancer IP that falls inside a no-encap pool but whose AllowedUses do not include LoadBalancer (e.g., a workload-only pool) would still be accepted, enabling ARP/NDP replies for an IP that was not allocated/approved for LB use.
- Mitigations:
- Track and honor IPPool AllowedUses (augment proto.IPAMPoolUpdate to include them) and only accept:
- Pod IPs from pools allowing Workload (current behavior is implicitly safe as they come from local WEP updates).
- LB VIPs from pools allowing LoadBalancer.
- As a strict alternative for LB VIPs, rely solely on Status.loadBalancer.ingress and ignore Spec.loadBalancerIP (see prior finding).
- Track and honor IPPool AllowedUses (augment proto.IPAMPoolUpdate to include them) and only accept:
There was a problem hiding this comment.
🛡️ Council of Claudes — Security
Caution
Security lens · validation · secrets · authz · isolation
2 potential security issues
-
Architectural posture: Enabling HostSubnetNeighResponses by default materially expands L2 reachability of pods and LoadBalancer VIPs beyond routing/BGP-based exposure. That can surprise operators and weaken a default-deny posture at the network boundary if host uplinks are L2-adjacent to untrusted segments. Consider defaulting to Disabled and/or clearly scoping which host interfaces may reply (e.g., via an explicit allowlist) to reduce unintended exposure.
-
Interface scoping: The new manager filters out workload veths by prefix, but otherwise will reply on any host interface whose subnet matches (depending on what the interface monitor emits). If InterfaceExclude isn’t applied upstream to these updates, this could cause ARP/NDP replies on management/uplink interfaces, exposing pod/LB IPs on unintended L2 segments. Please confirm ifaceAddrsUpdate already honors Felix’s InterfaceExclude; if not, add equivalent filtering (or a dedicated allow/deny list for this feature).
Line-specific findings
🤖 Council of Claudes · 4 inline comment(s)
| // Register the proxy neighbor managers when enabled. They listen on raw sockets | ||
| // and respond to ARP (IPv4) / NDP (IPv6) requests for pod and LB IPs that fall | ||
| // within the same subnet as a host physical interface. | ||
| if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" { |
There was a problem hiding this comment.
Nell — Case-sensitive compare here means a valid lower/upper-cased value won’t enable the feature (config parsing elsewhere is case-insensitive). Use EqualFold (or normalise earlier), no?
if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") {
// ...
}(You’ll need to import strings.)
|
|
||
| func newProxyNeighManager(dpConfig Config, ipVersion uint8) *proxyNeighManager { | ||
| nl, err := netlinkshim.NewRealNetlink() | ||
| if err != nil { |
There was a problem hiding this comment.
Nell — We panic if NewRealNetlink() fails. Return error here? If we plumb an error up from newProxyNeighManager/newProxyNeighManagerWithShims and surface it via StartDataplaneDriver (or even by returning it from CompleteDeferredWork as you already do for listener start failures), we avoid taking Felix down with a panic.
| // active from Felix's first calc-graph flush. | ||
| fc := api.NewFelixConfiguration() | ||
| fc.Name = "default" | ||
| fc.Spec.HostSubnetNeighResponses = new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) |
There was a problem hiding this comment.
Nell — This won’t compile: new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) is using a constant as a type. Should we assign the const to a var and take its address?
| fc.Spec.HostSubnetNeighResponses = new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) | |
| mode := api.HostSubnetNeighResponsesPodsAndLoadBalancers | |
| fc.Spec.HostSubnetNeighResponses = &mode |
|
|
||
| FloatingIPsEnabled bool | ||
|
|
||
| HostSubnetNeighResponses string |
There was a problem hiding this comment.
Nell — nit: Carrying this as a raw string into the dataplane makes the later compare brittle. Would we prefer a parsed enum/bool here (or even normalise in driver.go like we do for FloatingIPs) to make the intent explicit? Not a blocker.
|
|
||
| pkt, _, err := l.arpCli.Read() | ||
| if err != nil { | ||
| if ctx.Err() != nil { |
There was a problem hiding this comment.
Nell — runARPListener/read error: we treat any error as a timeout and continue. Worth logging non-timeout errors at debug once in a while so we don’t mask persistent read errors? (not a blocking concern)
| defer cancel() | ||
| pool := &v3.IPPool{ObjectMeta: metav1.ObjectMeta{Name: name}} | ||
| if err := cli.Delete(ctx, pool); err != nil { | ||
| logrus.WithError(err).WithField("pool", name).Info("Failed to delete IPPool during cleanup") |
There was a problem hiding this comment.
Nell — nit: OK to leave this as best-effort cleanup, but just checking intent — we log-and-skip delete failures here. Do we want to warn (or surface the error) instead of Info so a leaked pool is more visible?
There was a problem hiding this comment.
Council of Claudes — Nell
Important
Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments
3 potential correctness issues
Nice end-to-end story here, and the manager shape looks like it fits the Felix pattern. A few substantive items to tighten up:
- Case-insensitive handling for HostSubnetNeighResponses falls through to a case-sensitive string compare in the dataplane; that’ll make lower-cased values ineffective.
- The proxy-neigh manager constructor panics on netlink failure. I think we should return an error and let the caller surface it (consistent with the rest of the dataplane managers), no?
- In FV, setting HostSubnetNeighResponses with new(...) won’t compile — that’s a constant, not a type.
- Minor: there are two near-identical helpers to inspect a docker network and to OR-in an IPv4 offset CIDR (once in e2e and once in FV). Worth deduplicating into a single test helper, or at least keeping one variant? (non-blocking)
- Minor: dataplane.Config carries HostSubnetNeighResponses as a raw string. Would we prefer a parsed enum/bool to avoid typos drifting into the compare? (already fine, so feel free to leave that to later)
🤖 Council of Claudes · 6 inline comment(s)
| func newProxyNeighManager(dpConfig Config, ipVersion uint8) *proxyNeighManager { | ||
| nl, err := netlinkshim.NewRealNetlink() | ||
| if err != nil { | ||
| log.WithError(err).Panic("Failed to create netlink handle for proxy neighbor manager") |
There was a problem hiding this comment.
Casey — I think we don’t want to panic in felix’s dataplane init path — that can take down the whole node. Best to log and disable the feature (or bubble an error up so the driver can continue without proxy-neigh), similar to other managers.
For example:
nl, err := netlinkshim.NewRealNetlink()
if err != nil {
log.WithError(err).Error("Proxy neighbor manager disabled: failed to create netlink handle")
return nil // or a no-op manager, or gate creation at the caller
}If we keep the constructor signature as-is, another option is to have NewIntDataplaneDriver guard manager construction and continue without it on error.
| allManagers []Manager | ||
| managersWithRouteTables []ManagerWithRouteTables | ||
| managersWithRouteRules []ManagerWithRouteRules | ||
| proxyNeighManagers []*proxyNeighManager |
There was a problem hiding this comment.
Casey — I’m also rather surprised this extra slice is needed — we already register the managers via RegisterManager and track them in allManagers. The proxyNeighManagers []*proxyNeighManager field doesn’t seem to be used anywhere else; can this be removed to avoid redundant state?
If we do keep it, can we clarify its purpose (and where it’s read), or switch it to the existing manager lists?
| if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" { | ||
| proxyNeighMgr4 := newProxyNeighManager(config, 4) | ||
| dp.RegisterManager(proxyNeighMgr4) | ||
| dp.proxyNeighManagers = append(dp.proxyNeighManagers, proxyNeighMgr4) |
There was a problem hiding this comment.
Casey — Related to the above, we append the new managers to dp.proxyNeighManagers here but (AFAICT) never read that slice. I think we can drop these appends (and the field) and just rely on RegisterManager(...) like the other managers. WDYT?
| return framework.WithLabel("ExternalNode") | ||
| } | ||
|
|
||
| // RequiresKindNetwork marks tests that need to attach a docker container to the |
There was a problem hiding this comment.
Casey — I think relying on a manual --ginkgo.skip=KindNetwork is fragile — we’ve been trying to avoid self-skip and to fail explicitly with a clear reason. Could we make this behave like our other “Requires” patterns and fail-fast when the requirement isn’t met, e.g.,
Fail("This test requires local access to the docker daemon and a KIND network; run on a kind cluster or skip via labels")Or, wire it up as a proper “RequiresKindNetwork” capability in the framework so CI selection/skips are explicit and enforced? The key thing IMO is: if the test runs in the wrong place, it should fail loudly with a helpful message.
| By("Ensuring HostSubnetNeighResponses=PodsAndLoadBalancers in default FelixConfiguration") | ||
| DeferCleanup(setHostSubnetNeighResponses(cli, v3.HostSubnetNeighResponsesPodsAndLoadBalancers)) | ||
|
|
||
| By("Spawning an L2-adjacent docker container on the kind network") |
There was a problem hiding this comment.
Casey — I am a little bit skeptical of the test spinning up a docker container on the host network — it sort of breaks our “infra vs test code” boundary. I get why it’s needed here (we need an L2-adjacent peer to exercise ARP/NDP), but can we:
- Centralize this in shared e2e infra (a helper under e2e/pkg/utils or a test-infra wrapper) so the test body isn’t managing docker directly?
- Fail fast if docker isn’t accessible (and tie that to the label), instead of relying on invocation-time skips?
If we keep this here, I think it carries its weight for this feature — just want to make sure it’s discoverable and consistently guarded. WDYT?
| // given docker network. Either return value may be nil if the network has no | ||
| // subnet of that family. | ||
| func detectKindNetworkSubnets(networkName string) (v4, v6 *net.IPNet, err error) { | ||
| out, err := exec.Command("docker", "network", "inspect", networkName).Output() |
There was a problem hiding this comment.
Casey — We’re shelling out to docker network inspect from the test. Similar concern as above — can we at least add an early preflight that verifies docker is reachable and produces a very explicit failure message if not? That’ll make misconfigurations obvious.
| // Both inputs must be IPv4. We normalize to 4-byte form because net.ParseCIDR | ||
| // can return either 4- or 16-byte representations depending on the input | ||
| // string. | ||
| func subnetOffset(parent *net.IPNet, offset string) (string, error) { |
There was a problem hiding this comment.
Casey — nit: This subnetOffset helper duplicates the (very similar) subnetOffsetV4 in the FV test (felix/fv/proxy_neigh_test.go). Could we DRY this up into a single helper within the suite? Even just keeping one variant per suite (e2e vs fv) would help reduce drift.
| return nil, nil | ||
| } | ||
|
|
||
| // subnetOffsetV4 returns the CIDR formed by OR-ing the offset's address bits |
There was a problem hiding this comment.
Casey — Same duplication nit — this is the FV copy of the subnet-offset helper. If we keep both suites, can we pick one implementation per suite to avoid subtle differences over time?
| } | ||
|
|
||
| m.listeners[ifaceName] = l | ||
| log.WithField("iface", ifaceName).Info("Started proxy neighbor listener") |
There was a problem hiding this comment.
Casey — nit: These “Started/Stopped proxy neighbor listener” logs are at Info. On hosts with multiple interfaces, this could get a bit noisy. I think Debug would be more appropriate for per-interface churn.
| log.WithField("iface", ifaceName).Info("Started proxy neighbor listener") | |
| log.WithField("iface", ifaceName).Debug("Started proxy neighbor listener") |
| if l.ndpCli != nil { | ||
| _ = l.ndpCli.Close() | ||
| } | ||
| <-l.done |
There was a problem hiding this comment.
Casey — nit: Same point for the stop log — probably Debug is better.
| <-l.done | |
| log.WithField("iface", l.ifaceName).Debug("Stopped proxy neighbor listener") |
| return | ||
| } | ||
| if err := l.arpCli.WriteTo(pkt, ethernet.Broadcast); err != nil { | ||
| log.WithError(err).WithFields(log.Fields{ |
There was a problem hiding this comment.
Casey — nit: This is a Warn on failed GARP send; might be transient/noisy. Maybe Debug (or Info with rate limiting) to avoid spamming logs in failure loops?
| log.WithError(err).WithFields(log.Fields{ | |
| log.WithError(err).WithFields(log.Fields{ | |
| "iface": l.ifaceName, | |
| "ip": ipStr, | |
| }).Debug("Failed to send GARP") |
| @@ -0,0 +1,659 @@ | |||
| // Copyright (c) 2026 Tigera, Inc. All rights reserved. | |||
There was a problem hiding this comment.
Casey — nit: Copyright year looks off.
| // Copyright (c) 2026 Tigera, Inc. All rights reserved. | |
| // Copyright (c) 2026 Tigera, Inc. All rights reserved. |
Should be the current year.
There was a problem hiding this comment.
Council of Claudes — Casey
Warning
Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness
2 potential correctness/robustness issues; several e2e/infra-boundary concerns
I am a little bit skeptical of a few things here:
- I think the panic in felix’s dataplane constructor is too harsh — best not to accidentally take down the cluster if netlink init fails. We should degrade/disable this feature and continue, or error out cleanly through the driver.
- I wonder about the e2e/FV infra approach — both suites shell out to docker and spin up a sidecar container on the host bridge/kind network. That sort of breaks the “infra vs test code” boundary we try to maintain in e2e/FV. I’m not necessarily against it for this particular feature (it’s hard to test ARP/NDP from inside the cluster…), but I’d like us to be explicit about the requirement and fail loudly when it isn’t met (rather than relying on manual ginkgo skips).
- There’s a new InternalDataplane field (proxyNeighManagers) that doesn’t seem to be used anywhere except to append to; feels like dead state we can remove.
- nit: The listener start/stop logs are at Info; I think that could get chatty on larger hosts — probably better as Debug.
- nit: We’ve duplicated subnet-offset helpers across FV and e2e; can we DRY this into a small helper (even if just within each suite)?
- The default for the new config is PodsAndLoadBalancers — that’s a fairly material behavioral change. I’m a little bit surprised we’re enabling this by default. If we can’t foresee it biting users, fine, but I’d like to understand the rationale and rollout plan (operator defaults, doc callouts, etc.). WDYT?
Line-specific findings
Other notes (not anchored to a diff line):
felix/dataplane/linux/proxy_neigh_mgr.go:1— nit: Same copyright year mismatch here — can we standardize on the current year?felix/dataplane/linux/proxy_neigh_mgr_test.go:1— nit: Same copyright year here.felix/fv/proxy_neigh_test.go:1— nit: Same copyright year here as well.
🤖 Council of Claudes · 12 inline comment(s)
2 participants
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 commitb9eb6ddda003. 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.