Skip to content

Commit fe893a6

Browse files
committed
[ipam/eni] wait for ENI interface before CNI ADD route setup
Signed-off-by: Jared Ledvina <jared.ledvina@datadoghq.com>
1 parent 67dfc5a commit fe893a6

6 files changed

Lines changed: 223 additions & 32 deletions

File tree

daemon/infraendpoints/infra_ip_allocation.go

Lines changed: 17 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import (
2121
"github.com/cilium/statedb"
2222
"github.com/vishvananda/netlink"
2323
"go4.org/netipx"
24-
"golang.org/x/sys/unix"
25-
"k8s.io/apimachinery/pkg/util/wait"
2624

2725
"github.com/cilium/cilium/pkg/common"
2826
linuxrouting "github.com/cilium/cilium/pkg/datapath/linux/routing"
@@ -33,6 +31,7 @@ import (
3331
"github.com/cilium/cilium/pkg/ipam"
3432
ipamOption "github.com/cilium/cilium/pkg/ipam/option"
3533
"github.com/cilium/cilium/pkg/logging/logfields"
34+
"github.com/cilium/cilium/pkg/mac"
3635
"github.com/cilium/cilium/pkg/mtu"
3736
"github.com/cilium/cilium/pkg/node"
3837
"github.com/cilium/cilium/pkg/option"
@@ -221,32 +220,11 @@ func (r *infraIPAllocator) reallocateOldRouterIPs(fromK8s, fromFS net.IP) (resul
221220
}
222221

223222
func (r *infraIPAllocator) waitForENI(ctx context.Context, macAddr string) error {
224-
bo := wait.Backoff{
225-
Duration: 250 * time.Millisecond,
226-
Factor: 2,
227-
Jitter: 0.2,
228-
Steps: 5,
229-
}
230-
231-
findENIByMAC := func(ctx context.Context) (bool, error) {
232-
links, err := safenetlink.LinkList()
233-
if err != nil {
234-
return false, fmt.Errorf("unable to list interfaces: %w", err)
235-
}
236-
237-
for _, l := range links {
238-
// filter out slave devices
239-
if l.Attrs().RawFlags&unix.IFF_SLAVE != 0 {
240-
continue
241-
}
242-
if l.Attrs().HardwareAddr.String() == macAddr {
243-
return true, nil
244-
}
245-
}
246-
return false, nil
223+
parsedMAC, err := net.ParseMAC(macAddr)
224+
if err != nil {
225+
return fmt.Errorf("invalid MAC address %q: %w", macAddr, err)
247226
}
248-
249-
return wait.ExponentialBackoffWithContext(ctx, bo, findENIByMAC)
227+
return linuxrouting.WaitForENIInterface(ctx, mac.MAC(parsedMAC))
250228
}
251229

252230
func (r *infraIPAllocator) reallocateRouterIPs(ctx context.Context, family node.AddressingFamily, fromK8s, fromFS net.IP) (routerIP net.IP, err error) {
@@ -299,6 +277,7 @@ func (r *infraIPAllocator) reallocateRouterIPs(ctx context.Context, family node.
299277
if err := r.waitForENI(ctx, result.PrimaryMAC); err != nil {
300278
r.logger.Warn("unable to find ENI netlink interface, this will likely lead to an error in configuring the router routes and rules",
301279
logfields.MACAddr, result.PrimaryMAC,
280+
logfields.Error, err,
302281
)
303282
}
304283
}
@@ -425,7 +404,7 @@ func (r *infraIPAllocator) allocateHealthIPs(oldV4HealthIP netip.Addr, oldV6Heal
425404
return nil
426405
}
427406

428-
func (r *infraIPAllocator) allocateIngressIPs(oldV4IngressIP net.IP, oldV6IngressIP net.IP) error {
407+
func (r *infraIPAllocator) allocateIngressIPs(ctx context.Context, oldV4IngressIP net.IP, oldV6IngressIP net.IP) error {
429408
if !r.daemonConfig.EnableEnvoyConfig {
430409
return nil
431410
}
@@ -475,6 +454,15 @@ func (r *infraIPAllocator) allocateIngressIPs(oldV4IngressIP net.IP, oldV6Ingres
475454
if ingressRouting, err := r.parseRoutingInfo(result); err != nil {
476455
r.logger.Warn("Unable to allocate ingress information for ENI", logfields.Error, err)
477456
} else {
457+
if r.daemonConfig.IPAM == ipamOption.IPAMENI {
458+
if err := r.waitForENI(ctx, result.PrimaryMAC); err != nil {
459+
r.logger.Warn("unable to find ENI netlink interface, this will likely lead to an error in configuring the ingress routes and rules",
460+
logfields.MACAddr, result.PrimaryMAC,
461+
logfields.Error, err,
462+
)
463+
}
464+
}
465+
478466
if err := ingressRouting.Configure(
479467
result.IP,
480468
r.mtuManager.GetDeviceMTU(),
@@ -553,7 +541,7 @@ func (r *infraIPAllocator) AllocateIPs(ctx context.Context) error {
553541
return fmt.Errorf("failed to allocate service loopback IPs: %w", err)
554542
}
555543

556-
if err := r.allocateIngressIPs(localNode.IPv4IngressIP, localNode.IPv6IngressIP); err != nil {
544+
if err := r.allocateIngressIPs(ctx, localNode.IPv4IngressIP, localNode.IPv6IngressIP); err != nil {
557545
return fmt.Errorf("failed to allocate ingress IPs: %w", err)
558546
}
559547

daemon/infraendpoints/infra_ip_allocation_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package infraendpoints
55

66
import (
7+
"context"
78
"fmt"
89
"net"
910
"net/netip"
@@ -323,3 +324,13 @@ func Test_getCiliumHostIPsFromFile(t *testing.T) {
323324
})
324325
}
325326
}
327+
328+
func TestWaitForENIInvalidMAC(t *testing.T) {
329+
r := &infraIPAllocator{
330+
logger: hivetest.Logger(t),
331+
}
332+
333+
err := r.waitForENI(context.Background(), "not-a-valid-mac")
334+
require.Error(t, err)
335+
require.ErrorContains(t, err, "invalid MAC address")
336+
}

pkg/datapath/linux/routing/routing.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package linuxrouting
55

66
import (
7+
"context"
78
"errors"
89
"fmt"
910
"log/slog"
@@ -13,6 +14,7 @@ import (
1314
"github.com/vishvananda/netlink"
1415
"go4.org/netipx"
1516
"golang.org/x/sys/unix"
17+
"k8s.io/apimachinery/pkg/util/wait"
1618

1719
"github.com/cilium/statedb"
1820

@@ -24,8 +26,42 @@ import (
2426
"github.com/cilium/cilium/pkg/logging/logfields"
2527
"github.com/cilium/cilium/pkg/mac"
2628
"github.com/cilium/cilium/pkg/option"
29+
"github.com/cilium/cilium/pkg/time"
2730
)
2831

32+
// ENI attachment is asynchronous; resolving its ifindex by MAC can race the
33+
// netlink device becoming visible.
34+
var WaitForENIInterfaceBackoff = wait.Backoff{
35+
Duration: 250 * time.Millisecond,
36+
Factor: 2,
37+
Jitter: 0.2,
38+
Steps: 7,
39+
}
40+
41+
// WaitForENIInterface polls until an interface with the given MAC appears, or
42+
// the context or backoff is exhausted.
43+
func WaitForENIInterface(ctx context.Context, macAddr mac.MAC) error {
44+
findENIByMAC := func(ctx context.Context) (bool, error) {
45+
links, err := safenetlink.LinkList()
46+
if err != nil {
47+
return false, fmt.Errorf("unable to list interfaces: %w", err)
48+
}
49+
50+
for _, l := range links {
51+
// Slave devices share their master's MAC; skip them.
52+
if l.Attrs().RawFlags&unix.IFF_SLAVE != 0 {
53+
continue
54+
}
55+
if l.Attrs().HardwareAddr.String() == macAddr.String() {
56+
return true, nil
57+
}
58+
}
59+
return false, nil
60+
}
61+
62+
return wait.ExponentialBackoffWithContext(ctx, WaitForENIInterfaceBackoff, findENIByMAC)
63+
}
64+
2965
// useCompatEgressPriority determines whether to use the new or old style egress rule.
3066
// Old style rules are only used in Azure IPAM mode.
3167
func (info *RoutingInfo) useCompatEgressPriority() bool {

pkg/datapath/linux/routing/routing_test.go

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,16 @@
44
package linuxrouting
55

66
import (
7+
"context"
78
"net"
89
"net/netip"
910
"testing"
11+
"time"
1012

1113
"github.com/cilium/hive/hivetest"
1214
"github.com/stretchr/testify/require"
1315
"github.com/vishvananda/netlink"
16+
"k8s.io/apimachinery/pkg/util/wait"
1417

1518
"github.com/cilium/cilium/pkg/datapath/linux/linux_defaults"
1619
"github.com/cilium/cilium/pkg/datapath/linux/route"
@@ -331,6 +334,125 @@ func getFakes(t *testing.T, ipamMode string, masquerade bool, withZeroCIDR bool)
331334
return netip.MustParseAddr("192.168.2.123"), *fakeRoutingInfo
332335
}
333336

337+
// withTestBackoff swaps in a faster backoff for tests. Mutates a package
338+
// global, so callers must not use t.Parallel().
339+
func withTestBackoff(t *testing.T, bo wait.Backoff) {
340+
t.Helper()
341+
orig := WaitForENIInterfaceBackoff
342+
WaitForENIInterfaceBackoff = bo
343+
t.Cleanup(func() { WaitForENIInterfaceBackoff = orig })
344+
}
345+
346+
func TestPrivilegedWaitForENIInterfaceAlreadyPresent(t *testing.T) {
347+
setupLinuxRoutingSuite(t)
348+
withTestBackoff(t, wait.Backoff{Duration: 10 * time.Millisecond, Factor: 2, Steps: 3})
349+
350+
ns := netns.NewNetNS(t)
351+
require.NoError(t, ns.Do(func() error {
352+
macAddr, err := mac.ParseMAC("00:11:22:33:44:66")
353+
require.NoError(t, err)
354+
355+
cleanup := createDummyDevice(t, macAddr)
356+
defer cleanup()
357+
358+
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
359+
defer cancel()
360+
361+
require.NoError(t, WaitForENIInterface(ctx, macAddr))
362+
return nil
363+
}))
364+
}
365+
366+
func TestPrivilegedWaitForENIInterfaceAppearsLate(t *testing.T) {
367+
setupLinuxRoutingSuite(t)
368+
withTestBackoff(t, wait.Backoff{Duration: 20 * time.Millisecond, Factor: 1.5, Jitter: 0.1, Steps: 10})
369+
370+
// ns.Do pins the netns to its own goroutine's OS thread; a bare
371+
// `go func(){}()` inside it would run in the host netns instead.
372+
ns := netns.NewNetNS(t)
373+
374+
macAddr, err := mac.ParseMAC("00:11:22:33:44:77")
375+
require.NoError(t, err)
376+
377+
require.NoError(t, ns.Do(func() error {
378+
require.False(t, linkExistsWithMAC(t, macAddr), "interface must not exist yet")
379+
return nil
380+
}))
381+
382+
// require/FailNow must run on the test goroutine, so failures are propagated back over a channel instead.
383+
type deviceResult struct {
384+
cleanup func()
385+
err error
386+
}
387+
resultCh := make(chan deviceResult, 1)
388+
go func() {
389+
time.Sleep(60 * time.Millisecond)
390+
var res deviceResult
391+
res.err = ns.Do(func() error {
392+
dummy := &netlink.Dummy{
393+
LinkAttrs: netlink.LinkAttrs{
394+
Name: "linuxrout-test",
395+
HardwareAddr: net.HardwareAddr(macAddr),
396+
},
397+
}
398+
if err := netlink.LinkAdd(dummy); err != nil {
399+
return err
400+
}
401+
// Wrapped in ns.Do because cleanup runs after the host netns has been restored.
402+
res.cleanup = func() { _ = ns.Do(func() error { return netlink.LinkDel(dummy) }) }
403+
return nil
404+
})
405+
resultCh <- res
406+
}()
407+
408+
var waitErr error
409+
require.NoError(t, ns.Do(func() error {
410+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
411+
defer cancel()
412+
waitErr = WaitForENIInterface(ctx, macAddr)
413+
return nil
414+
}))
415+
416+
res := <-resultCh
417+
require.NoError(t, res.err, "failed to create dummy device asynchronously")
418+
require.NotNil(t, res.cleanup)
419+
defer res.cleanup()
420+
421+
require.NoError(t, waitErr, "WaitForENIInterface should succeed once the interface appears")
422+
}
423+
424+
func TestPrivilegedWaitForENIInterfaceTimeout(t *testing.T) {
425+
setupLinuxRoutingSuite(t)
426+
withTestBackoff(t, wait.Backoff{Duration: 10 * time.Millisecond, Factor: 1.5, Steps: 3})
427+
428+
ns := netns.NewNetNS(t)
429+
require.NoError(t, ns.Do(func() error {
430+
macAddr, err := mac.ParseMAC("00:11:22:33:44:88")
431+
require.NoError(t, err)
432+
433+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
434+
defer cancel()
435+
436+
err = WaitForENIInterface(ctx, macAddr)
437+
require.Error(t, err, "interface never appears, so WaitForENIInterface should give up and return an error")
438+
return nil
439+
}))
440+
}
441+
442+
// Backoff is deliberately enormous so the test hangs (rather than passing spuriously) if cancellation is not honored.
443+
func TestWaitForENIInterfaceContextCancelled(t *testing.T) {
444+
withTestBackoff(t, wait.Backoff{Duration: time.Hour, Factor: 1, Steps: 100})
445+
446+
macAddr, err := mac.ParseMAC("00:11:22:33:44:99")
447+
require.NoError(t, err)
448+
449+
ctx, cancel := context.WithCancel(context.Background())
450+
cancel()
451+
452+
err = WaitForENIInterface(ctx, macAddr)
453+
require.Error(t, err, "an already-cancelled context should cause WaitForENIInterface to return without exhausting the backoff")
454+
}
455+
334456
func linkExistsWithMAC(t *testing.T, macAddr mac.MAC) bool {
335457
links, err := safenetlink.LinkList()
336458
require.NoError(t, err)

plugins/cilium-cni/cmd/cmd.go

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"github.com/cilium/cilium/pkg/datapath/connector"
3434
"github.com/cilium/cilium/pkg/datapath/link"
3535
"github.com/cilium/cilium/pkg/datapath/linux/route"
36+
linuxrouting "github.com/cilium/cilium/pkg/datapath/linux/routing"
3637
"github.com/cilium/cilium/pkg/datapath/linux/safenetlink"
3738
"github.com/cilium/cilium/pkg/datapath/linux/sysctl"
3839
"github.com/cilium/cilium/pkg/datapath/tables"
@@ -704,6 +705,22 @@ func (cmd *Cmd) Add(args *skel.CmdArgs) (err error) {
704705
//
705706
// See nodeport_snat_fwd_ipv4 in bpf/lib/nodeport_egress.h
706707
if ipv4IsEnabled(ipam) {
708+
// Otherwise ifindexFromMac can fail and leave ParentInterfaceIndex at 0,
709+
// silently breaking IPv4 masquerade reply routing.
710+
if parsedMAC, perr := mac.ParseMAC(ipam.IPv4.MasterMac); perr != nil {
711+
scopedLogger.Error(
712+
"Invalid ENI master MAC address",
713+
logfields.MACAddr, ipam.IPv4.MasterMac,
714+
logfields.Error, perr,
715+
)
716+
} else if werr := linuxrouting.WaitForENIInterface(context.TODO(), parsedMAC); werr != nil {
717+
scopedLogger.Warn(
718+
"Unable to find ENI netlink interface before resolving the parent ifindex; IPv4 masquerade reply routing may be misconfigured for this endpoint",
719+
logfields.MACAddr, ipam.IPv4.MasterMac,
720+
logfields.Error, werr,
721+
)
722+
}
723+
707724
ifindex, err := ifindexFromMac(ipam.IPv4.MasterMac)
708725
if err == nil {
709726
ep.ParentInterfaceIndex = ifindex
@@ -815,15 +832,17 @@ func (cmd *Cmd) Add(args *skel.CmdArgs) (err error) {
815832
}
816833

817834
if needsEndpointRoutingOnHost(conf) {
835+
// context.TODO(): the CNI skel API has no ADD deadline to thread through;
836+
// the wait inside interfaceAdd is bounded by WaitForENIInterfaceBackoff instead.
818837
if ipam.IPv4 != nil && ipConfig != nil {
819-
err = interfaceAdd(scopedLogger, ipConfig, ipam.IPv4, conf)
838+
err = interfaceAdd(context.TODO(), scopedLogger, ipConfig, ipam.IPv4, conf)
820839
if err != nil {
821840
return fmt.Errorf("unable to setup interface datapath: %w", err)
822841
}
823842
}
824843

825844
if ipam.IPv6 != nil && ipv6Config != nil {
826-
err = interfaceAdd(scopedLogger, ipv6Config, ipam.IPv6, conf)
845+
err = interfaceAdd(context.TODO(), scopedLogger, ipv6Config, ipam.IPv6, conf)
827846
if err != nil {
828847
return fmt.Errorf("unable to setup interface datapath: %w", err)
829848
}

0 commit comments

Comments
 (0)