Skip to content

Commit b9037bc

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 2efb257 commit b9037bc

6 files changed

Lines changed: 213 additions & 30 deletions

File tree

daemon/infraendpoints/infra_ip_allocation.go

Lines changed: 7 additions & 27 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) {
@@ -298,6 +276,7 @@ func (r *infraIPAllocator) reallocateRouterIPs(ctx context.Context, family node.
298276
if err := r.waitForENI(ctx, result.PrimaryMAC); err != nil {
299277
r.logger.Error("Unable to find ENI netlink interface, this will likely lead to an error in configuring the router routes and rules",
300278
logfields.MACAddr, result.PrimaryMAC,
279+
logfields.Error, err,
301280
)
302281
}
303282
}
@@ -480,6 +459,7 @@ func (r *infraIPAllocator) allocateIngressIPs(ctx context.Context, oldV4IngressI
480459
if err := r.waitForENI(ctx, result.PrimaryMAC); err != nil {
481460
r.logger.Error("Unable to find ENI netlink interface, this will likely lead to an error in configuring the ingress routes and rules",
482461
logfields.MACAddr, result.PrimaryMAC,
462+
logfields.Error, err,
483463
)
484464
}
485465

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
}

plugins/cilium-cni/cmd/interface.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package cmd
55

66
import (
7+
"context"
78
"fmt"
89
"log/slog"
910
"net"
@@ -13,9 +14,11 @@ import (
1314
"github.com/cilium/cilium/api/v1/models"
1415
linuxrouting "github.com/cilium/cilium/pkg/datapath/linux/routing"
1516
"github.com/cilium/cilium/pkg/ip"
17+
ipamOption "github.com/cilium/cilium/pkg/ipam/option"
18+
"github.com/cilium/cilium/pkg/logging/logfields"
1619
)
1720

18-
func interfaceAdd(logger *slog.Logger, ipConfig *current.IPConfig, ipam *models.IPAMAddressResponse, conf *models.DaemonConfigurationStatus) error {
21+
func interfaceAdd(ctx context.Context, logger *slog.Logger, ipConfig *current.IPConfig, ipam *models.IPAMAddressResponse, conf *models.DaemonConfigurationStatus) error {
1922
if ipam == nil {
2023
return fmt.Errorf("missing IPAM configuration")
2124
}
@@ -68,6 +71,18 @@ func interfaceAdd(logger *slog.Logger, ipConfig *current.IPConfig, ipam *models.
6871
return fmt.Errorf("unable to parse routing info: %w", err)
6972
}
7073

74+
// Otherwise Configure can transiently fail CNI ADD with "interface with MAC ... not found",
75+
// leaving the pod stuck in ContainerCreating.
76+
if conf.IpamMode == ipamOption.IPAMENI {
77+
if err := linuxrouting.WaitForENIInterface(ctx, routingInfo.MasterIfMAC); err != nil {
78+
logger.Warn(
79+
"Unable to find ENI netlink interface, this will likely lead to an error configuring the pod's IP rules and routes",
80+
logfields.MACAddr, routingInfo.MasterIfMAC,
81+
logfields.Error, err,
82+
)
83+
}
84+
}
85+
7186
if err := routingInfo.Configure(
7287
ip.AddrFromIP(ipConfig.Address.IP),
7388
int(conf.DeviceMTU),

0 commit comments

Comments
 (0)