-
Notifications
You must be signed in to change notification settings - Fork 0
[sample #12960] [v2] felix: periodically re-announce proxy-neighbor IPs #240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: coc-sample-12960-v2-base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,6 +380,13 @@ type Config struct { | |
| // selected LoadBalancer VIPs that overlap the host subnet. [Default: Disabled] | ||
| LocalSubnetL2Reachability string `config:"oneof(Disabled,PodsAndLoadBalancers);Disabled"` | ||
|
|
||
| // LocalSubnetL2ReachabilityRefreshInterval controls how often Felix re-announces | ||
| // (gratuitous ARP / unsolicited NA) every IP it proxies ARP/NDP for, 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] | ||
| LocalSubnetL2ReachabilityRefreshInterval time.Duration `config:"seconds;120"` | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧪 Maintainability & Tests — New config param is added with default and docs, but there’s no unit test here asserting:
Suggestion:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛡️ 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.
|
||
|
|
||
| // WindowsManageFirewallRules configures whether or not Felix will program Windows Firewall rules. [Default: Disabled] | ||
| WindowsManageFirewallRules string `config:"oneof(Enabled,Disabled);Disabled"` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,13 +43,15 @@ import ( | |
| "github.com/projectcalico/calico/libcalico-go/lib/set" | ||
| ) | ||
|
|
||
| // readDeadlineInterval bounds how long an ARP/NDP listener blocks in a | ||
| // single socket read before waking to re-check for context cancellation. | ||
| // A read that hits the deadline simply loops | ||
| const readDeadlineInterval = 1 * time.Second | ||
|
|
||
| // ipv6AllNodesMulticast is the IPv6 all-nodes link-local multicast address | ||
| const ipv6AllNodesMulticast = "ff02::1" | ||
| const ( | ||
| // readDeadlineInterval bounds how long an ARP/NDP listener blocks in a | ||
| // single socket read before waking to re-check for context cancellation. | ||
| // A read that hits the deadline simply loops | ||
| readDeadlineInterval = 1 * time.Second | ||
|
|
||
| // ipv6AllNodesMulticast is the IPv6 all-nodes link-local multicast address | ||
| ipv6AllNodesMulticast = "ff02::1" | ||
| ) | ||
|
|
||
| // serviceID identifies a Kubernetes Service by namespace and name. Used as a | ||
| // map key for tracking LoadBalancer service IPs. | ||
|
|
@@ -94,6 +96,10 @@ type proxyNeighManager struct { | |
| // before looping to re-check for cancellation. | ||
| readTimeout time.Duration | ||
|
|
||
| // announceInterval is how often each listener re-announces every IP it owns. | ||
| // Zero or negative disables periodic re-announcement. | ||
| announceInterval time.Duration | ||
|
|
||
| // hostIfaceToCIDRs maps host interface name to the parsed CIDRs on that interface. | ||
| hostIfaceToCIDRs map[string][]net.IPNet | ||
|
|
||
|
|
@@ -178,7 +184,7 @@ func newProxyNeighManager(dpConfig Config, ipVersion uint8) *proxyNeighManager { | |
| return conn, ifi.HardwareAddr, nil | ||
| } | ||
| } | ||
| return newProxyNeighManagerWithShims(dpConfig, ipVersion, nl, af, nf, readDeadlineInterval) | ||
| return newProxyNeighManagerWithShims(dpConfig, ipVersion, nl, af, nf, readDeadlineInterval, dpConfig.LocalSubnetL2ReachabilityRefreshInterval) | ||
| } | ||
|
|
||
| // setIgnoreOutgoing sets PACKET_IGNORE_OUTGOING on the raw socket so the kernel | ||
|
|
@@ -207,6 +213,7 @@ func newProxyNeighManagerWithShims( | |
| af arpClientFactory, | ||
| nf ndpConnFactory, | ||
| readTimeout time.Duration, | ||
| announceInterval time.Duration, | ||
| ) *proxyNeighManager { | ||
| wlIfacesPattern := "^(" + strings.Join(dpConfig.RulesConfig.WorkloadIfacePrefixes, "|") + ").*" | ||
| wlIfacesRegexp := regexp.MustCompile(wlIfacesPattern) | ||
|
|
@@ -217,6 +224,7 @@ func newProxyNeighManagerWithShims( | |
| hostname: dpConfig.Hostname, | ||
| wlIfacesRegexp: wlIfacesRegexp, | ||
| readTimeout: readTimeout, | ||
| announceInterval: announceInterval, | ||
| hostIfaceToCIDRs: make(map[string][]net.IPNet), | ||
| localWorkloadIPs: make(map[types.WorkloadEndpointID][]string), | ||
| lbServiceIPs: make(map[serviceID][]string), | ||
|
|
@@ -522,11 +530,12 @@ func (m *proxyNeighManager) publishDesiredIPs(desiredByIface map[string]set.Set[ | |
| // listener goroutine. | ||
| func (m *proxyNeighManager) startListener(ifaceName string) error { | ||
| l := &ifaceListener{ | ||
| ifaceName: ifaceName, | ||
| readTimeout: m.readTimeout, | ||
| reconcile: make(chan struct{}, 1), | ||
| announced: set.New[string](), | ||
| done: make(chan struct{}), | ||
| ifaceName: ifaceName, | ||
| readTimeout: m.readTimeout, | ||
| announceInterval: m.announceInterval, | ||
| reconcile: make(chan struct{}, 1), | ||
| announced: set.New[string](), | ||
| done: make(chan struct{}), | ||
| } | ||
|
|
||
| l.ctx, l.cancel = context.WithCancel(m.ctx) | ||
|
|
@@ -578,18 +587,27 @@ type ifaceListener struct { | |
|
|
||
| // announced tracks the IPs the goroutine has already announced/joined for, | ||
| // so it can compute the delta against a new desired set. Goroutine-local: | ||
| // only ever touched by the listener goroutine, so it needs no | ||
| // synchronization. | ||
| // only ever touched by the listener goroutine, so it needs no synchronization. | ||
| announced set.Set[string] | ||
|
|
||
| arpCli arpClient // IPv4 only | ||
| ndpCli ndpConn // IPv6 only | ||
| hwAddr net.HardwareAddr | ||
| readTimeout time.Duration | ||
| ctx context.Context | ||
| cancel context.CancelFunc | ||
| done chan struct{} | ||
| failed atomic.Bool | ||
|
|
||
| // announceInterval is how often the goroutine re-announces every owned IP. | ||
| // Zero or negative disables periodic re-announcement (only the initial | ||
| // announce on add fires). | ||
| announceInterval time.Duration | ||
|
|
||
| // lastAnnounce is when the goroutine last re-announced its full owned set. | ||
| // Goroutine-local: compared against announceInterval each loop iteration. | ||
| lastAnnounce time.Time | ||
|
|
||
| ctx context.Context | ||
| cancel context.CancelFunc | ||
| done chan struct{} | ||
| failed atomic.Bool | ||
| } | ||
|
|
||
| // stop cancels the listener goroutine and waits for it to exit. The goroutine | ||
|
|
@@ -663,10 +681,8 @@ func (l *ifaceListener) applyDesiredState() { | |
| } | ||
| if l.ndpCli != nil { | ||
| l.joinNDPGroup(ip) | ||
| l.sendUNA(addr) | ||
| } else { | ||
| l.sendGARP(addr) | ||
| } | ||
| l.announce(addr) | ||
| } | ||
|
|
||
| // Release the multicast subscription for IPs that dropped out of the desired set. | ||
|
|
@@ -682,6 +698,32 @@ func (l *ifaceListener) applyDesiredState() { | |
| l.announced = desired | ||
| } | ||
|
|
||
| // announce sends a single gratuitous ARP (IPv4) or unsolicited NA (IPv6) for | ||
| // addr. It does not touch multicast group membership, so it is safe to call | ||
| // both for a newly desired IP (after joinNDPGroup) and for periodic refresh of | ||
| // an already-joined IP. | ||
| func (l *ifaceListener) announce(addr netip.Addr) { | ||
| if l.ndpCli != nil { | ||
| l.sendUNA(addr) | ||
| } else { | ||
| l.sendGARP(addr) | ||
| } | ||
| } | ||
|
|
||
| // reannounceAll re-sends a gratuitous ARP / unsolicited NA for every IP this | ||
| // listener currently owns, refreshing neighbor caches and switch tables without | ||
| // changing group membership. Driven by announceInterval from the listener loop. | ||
| func (l *ifaceListener) reannounceAll() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛡️ 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.
|
||
| for ip := range l.announced.All() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧪 Maintainability & Tests — reannounceAll parses string IPs back to netip.Addr each cycle. This repeats parsing work and introduces potential parse failures. Suggestion:
|
||
| addr, err := netip.ParseAddr(ip) | ||
| if err != nil { | ||
| logrus.WithError(err).WithField("ip", ip).Debug("Failed to parse IP for re-announce") | ||
| continue | ||
| } | ||
| l.announce(addr) | ||
| } | ||
| } | ||
|
|
||
| // closeSocket leaves any joined NDP multicast groups and closes the raw socket. | ||
| // It runs on the listener goroutine (deferred from runListener) so the socket | ||
| // stays single-owner; stop() only cancels the context and waits. | ||
|
|
@@ -795,6 +837,11 @@ func (l *ifaceListener) runListener(proto string, setDeadline func(time.Time) er | |
| defer close(l.done) | ||
| defer l.closeSocket() | ||
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| logCtx := logrus.WithFields(logrus.Fields{"iface": l.ifaceName, "proto": proto}) | ||
| for { | ||
| if l.ctx.Err() != nil { | ||
|
|
@@ -811,6 +858,15 @@ func (l *ifaceListener) runListener(proto string, setDeadline func(time.Time) er | |
| default: | ||
| } | ||
|
|
||
| // Periodically re-announce every owned IP so neighbor caches and switch | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧪 Maintainability & Tests — Manual interval bookkeeping (time.Since(lastAnnounce) >= announceInterval) inside the listener loop couples accuracy to readTimeout and requires bespoke timing logic. Suggestion:
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A couple of places we could address this:
Fine to leave the runtime guard as-is either way; it’s just the API/UX bit I’m flagging. |
||
| l.reannounceAll() | ||
| l.lastAnnounce = time.Now() | ||
| } | ||
|
|
||
| if err := setDeadline(time.Now().Add(l.readTimeout)); err != nil { | ||
| logCtx.WithError(err).Warn("Failed to set read deadline") | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.