[ipam/eni] wait for ENI interface before CNI ADD route setup

Signed-off-by: Jared Ledvina <jared.ledvina@datadoghq.com>

Author: jaredledvina

daemon/infraendpoints/infra_ip_allocation.go

@@ -21,8 +21,6 @@ import (
 	"github.com/cilium/statedb"
 	"github.com/vishvananda/netlink"
 	"go4.org/netipx"
-	"golang.org/x/sys/unix"
-	"k8s.io/apimachinery/pkg/util/wait"
 
 	"github.com/cilium/cilium/pkg/common"
 	linuxrouting "github.com/cilium/cilium/pkg/datapath/linux/routing"
@@ -33,6 +31,7 @@ import (
 	"github.com/cilium/cilium/pkg/ipam"
 	ipamOption "github.com/cilium/cilium/pkg/ipam/option"
 	"github.com/cilium/cilium/pkg/logging/logfields"
+	"github.com/cilium/cilium/pkg/mac"
 	"github.com/cilium/cilium/pkg/mtu"
 	"github.com/cilium/cilium/pkg/node"
 	"github.com/cilium/cilium/pkg/option"
@@ -221,32 +220,11 @@ func (r *infraIPAllocator) reallocateOldRouterIPs(fromK8s, fromFS net.IP) (resul
 }
 
 func (r *infraIPAllocator) waitForENI(ctx context.Context, macAddr string) error {
-	bo := wait.Backoff{
-		Duration: 250 * time.Millisecond,
-		Factor:   2,
-		Jitter:   0.2,
-		Steps:    5,
-	}
-
-	findENIByMAC := func(ctx context.Context) (bool, error) {
-		links, err := safenetlink.LinkList()
-		if err != nil {
-			return false, fmt.Errorf("unable to list interfaces: %w", err)
-		}
-
-		for _, l := range links {
-			// filter out slave devices
-			if l.Attrs().RawFlags&unix.IFF_SLAVE != 0 {
-				continue
-			}
-			if l.Attrs().HardwareAddr.String() == macAddr {
-				return true, nil
-			}
-		}
-		return false, nil
+	parsedMAC, err := net.ParseMAC(macAddr)
+	if err != nil {
+		return fmt.Errorf("invalid MAC address %q: %w", macAddr, err)
 	}
-
-	return wait.ExponentialBackoffWithContext(ctx, bo, findENIByMAC)
+	return linuxrouting.WaitForENIInterface(ctx, mac.MAC(parsedMAC))
 }
 
 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.
 			if err := r.waitForENI(ctx, result.PrimaryMAC); err != nil {
 				r.logger.Warn("unable to find ENI netlink interface, this will likely lead to an error in configuring the router routes and rules",
 					logfields.MACAddr, result.PrimaryMAC,
+					logfields.Error, err,
 				)
 			}
 		}
@@ -425,7 +404,7 @@ func (r *infraIPAllocator) allocateHealthIPs(oldV4HealthIP netip.Addr, oldV6Heal
 	return nil
 }
 
-func (r *infraIPAllocator) allocateIngressIPs(oldV4IngressIP net.IP, oldV6IngressIP net.IP) error {
+func (r *infraIPAllocator) allocateIngressIPs(ctx context.Context, oldV4IngressIP net.IP, oldV6IngressIP net.IP) error {
 	if !r.daemonConfig.EnableEnvoyConfig {
 		return nil
 	}
@@ -475,6 +454,17 @@ func (r *infraIPAllocator) allocateIngressIPs(oldV4IngressIP net.IP, oldV6Ingres
 			if ingressRouting, err := r.parseRoutingInfo(result); err != nil {
 				r.logger.Warn("Unable to allocate ingress information for ENI", logfields.Error, err)
 			} else {
+				// Mirror the router-interface guard above: wait for the ENI
+				// before configuring its routes and rules.
+				if r.daemonConfig.IPAM == ipamOption.IPAMENI {
+					if err := r.waitForENI(ctx, result.PrimaryMAC); err != nil {
+						r.logger.Warn("unable to find ENI netlink interface, this will likely lead to an error in configuring the ingress routes and rules",
+							logfields.MACAddr, result.PrimaryMAC,
+							logfields.Error, err,
+						)
+					}
+				}
+
 				if err := ingressRouting.Configure(
 					result.IP,
 					r.mtuManager.GetDeviceMTU(),
@@ -553,7 +543,7 @@ func (r *infraIPAllocator) AllocateIPs(ctx context.Context) error {
 		return fmt.Errorf("failed to allocate service loopback IPs: %w", err)
 	}
 
-	if err := r.allocateIngressIPs(localNode.IPv4IngressIP, localNode.IPv6IngressIP); err != nil {
+	if err := r.allocateIngressIPs(ctx, localNode.IPv4IngressIP, localNode.IPv6IngressIP); err != nil {
 		return fmt.Errorf("failed to allocate ingress IPs: %w", err)
 	}
 

daemon/infraendpoints/infra_ip_allocation_test.go

@@ -4,6 +4,7 @@
 package infraendpoints
 
 import (
+	"context"
 	"fmt"
 	"net"
 	"net/netip"
@@ -323,3 +324,15 @@ func Test_getCiliumHostIPsFromFile(t *testing.T) {
 		})
 	}
 }
+
+// TestWaitForENIInvalidMAC verifies that a malformed MAC fails fast instead of
+// delegating to the netlink poll and exhausting the backoff.
+func TestWaitForENIInvalidMAC(t *testing.T) {
+	r := &infraIPAllocator{
+		logger: hivetest.Logger(t),
+	}
+
+	err := r.waitForENI(context.Background(), "not-a-valid-mac")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "invalid MAC address")
+}

pkg/datapath/linux/routing/routing.go

@@ -4,6 +4,7 @@
 package linuxrouting
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"log/slog"
@@ -13,6 +14,7 @@ import (
 	"github.com/vishvananda/netlink"
 	"go4.org/netipx"
 	"golang.org/x/sys/unix"
+	"k8s.io/apimachinery/pkg/util/wait"
 
 	"github.com/cilium/statedb"
 
@@ -24,8 +26,49 @@ import (
 	"github.com/cilium/cilium/pkg/logging/logfields"
 	"github.com/cilium/cilium/pkg/mac"
 	"github.com/cilium/cilium/pkg/option"
+	"github.com/cilium/cilium/pkg/time"
 )
 
+// WaitForENIInterfaceBackoff bounds how long WaitForENIInterface polls before
+// giving up. ENI attachment is asynchronous: the operator can report an ENI as
+// attached (and cilium-agent can hand out an IP from it) before its netlink
+// device is visible, so resolving the ifindex by MAC can race. The original
+// router-only guard used 5 steps (~4.5s), which proved too short in practice;
+// this shared helper widens it to 7 (~19s worst case with jitter).
+var WaitForENIInterfaceBackoff = wait.Backoff{
+	Duration: 250 * time.Millisecond,
+	Factor:   2,
+	Jitter:   0.2,
+	Steps:    7,
+}
+
+// WaitForENIInterface blocks until a non-slave netlink interface with the
+// given MAC is observed, or ctx is cancelled or WaitForENIInterfaceBackoff is
+// exhausted. Shared by the router, ingress, and CNI ADD paths, which all
+// resolve an ENI's ifindex by MAC shortly after it is reported as attached.
+func WaitForENIInterface(ctx context.Context, macAddr mac.MAC) error {
+	findENIByMAC := func(ctx context.Context) (bool, error) {
+		links, err := safenetlink.LinkList()
+		if err != nil {
+			return false, fmt.Errorf("unable to list interfaces: %w", err)
+		}
+
+		for _, l := range links {
+			// Linux slave devices have the same MAC address as their master
+			// device, but we want the master device.
+			if l.Attrs().RawFlags&unix.IFF_SLAVE != 0 {
+				continue
+			}
+			if l.Attrs().HardwareAddr.String() == macAddr.String() {
+				return true, nil
+			}
+		}
+		return false, nil
+	}
+
+	return wait.ExponentialBackoffWithContext(ctx, WaitForENIInterfaceBackoff, findENIByMAC)
+}
+
 // useCompatEgressPriority determines whether to use the new or old style egress rule.
 // Old style rules are only used in Azure IPAM mode.
 func (info *RoutingInfo) useCompatEgressPriority() bool {

pkg/datapath/linux/routing/routing_test.go

@@ -4,13 +4,16 @@
 package linuxrouting
 
 import (
+	"context"
 	"net"
 	"net/netip"
 	"testing"
+	"time"
 
 	"github.com/cilium/hive/hivetest"
 	"github.com/stretchr/testify/require"
 	"github.com/vishvananda/netlink"
+	"k8s.io/apimachinery/pkg/util/wait"
 
 	"github.com/cilium/cilium/pkg/datapath/linux/linux_defaults"
 	"github.com/cilium/cilium/pkg/datapath/linux/route"
@@ -331,6 +334,145 @@ func getFakes(t *testing.T, ipamMode string, masquerade bool, withZeroCIDR bool)
 	return netip.MustParseAddr("192.168.2.123"), *fakeRoutingInfo
 }
 
+// withTestBackoff swaps WaitForENIInterfaceBackoff for a faster one so tests
+// need not wait out the production-sized backoff. It mutates a package-level
+// global, so callers must not use t.Parallel().
+func withTestBackoff(t *testing.T, bo wait.Backoff) {
+	t.Helper()
+	orig := WaitForENIInterfaceBackoff
+	WaitForENIInterfaceBackoff = bo
+	t.Cleanup(func() { WaitForENIInterfaceBackoff = orig })
+}
+
+// TestPrivilegedWaitForENIInterfaceAlreadyPresent: returns immediately when
+// the interface already exists.
+func TestPrivilegedWaitForENIInterfaceAlreadyPresent(t *testing.T) {
+	setupLinuxRoutingSuite(t)
+	withTestBackoff(t, wait.Backoff{Duration: 10 * time.Millisecond, Factor: 2, Steps: 3})
+
+	ns := netns.NewNetNS(t)
+	require.NoError(t, ns.Do(func() error {
+		macAddr, err := mac.ParseMAC("00:11:22:33:44:66")
+		require.NoError(t, err)
+
+		cleanup := createDummyDevice(t, macAddr)
+		defer cleanup()
+
+		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+		defer cancel()
+
+		require.NoError(t, WaitForENIInterface(ctx, macAddr))
+		return nil
+	}))
+}
+
+// TestPrivilegedWaitForENIInterfaceAppearsLate: the interface is absent when
+// WaitForENIInterface is first called but shows up shortly after, so the
+// helper must poll until it appears rather than fail immediately.
+func TestPrivilegedWaitForENIInterfaceAppearsLate(t *testing.T) {
+	setupLinuxRoutingSuite(t)
+	withTestBackoff(t, wait.Backoff{Duration: 20 * time.Millisecond, Factor: 1.5, Jitter: 0.1, Steps: 10})
+
+	// Each goroutine enters the namespace via its own ns.Do call: ns.Do pins
+	// the netns switch to the OS thread backing the goroutine it creates, so a
+	// bare `go func(){...}()` inside an ns.Do closure would run in the host
+	// netns, not ns.
+	ns := netns.NewNetNS(t)
+
+	macAddr, err := mac.ParseMAC("00:11:22:33:44:77")
+	require.NoError(t, err)
+
+	require.NoError(t, ns.Do(func() error {
+		require.False(t, linkExistsWithMAC(t, macAddr), "interface must not exist yet")
+		return nil
+	}))
+
+	// Propagate the goroutine's failure back over a channel rather than
+	// calling require/t.FailNow from it: those must run on the test's own
+	// goroutine, so the assertions happen on the main goroutine below.
+	type deviceResult struct {
+		cleanup func()
+		err     error
+	}
+	resultCh := make(chan deviceResult, 1)
+	go func() {
+		// Simulate the ENI's netlink interface showing up asynchronously,
+		// after WaitForENIInterface has already started polling.
+		time.Sleep(60 * time.Millisecond)
+		var res deviceResult
+		res.err = ns.Do(func() error {
+			dummy := &netlink.Dummy{
+				LinkAttrs: netlink.LinkAttrs{
+					Name:         "linuxrout-test",
+					HardwareAddr: net.HardwareAddr(macAddr),
+				},
+			}
+			if err := netlink.LinkAdd(dummy); err != nil {
+				return err
+			}
+			// Delete from inside the namespace: res.cleanup runs on the main
+			// goroutine after the host netns has been restored, so a direct
+			// LinkDel would target the host netns instead.
+			res.cleanup = func() { _ = ns.Do(func() error { return netlink.LinkDel(dummy) }) }
+			return nil
+		})
+		resultCh <- res
+	}()
+
+	var waitErr error
+	require.NoError(t, ns.Do(func() error {
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		waitErr = WaitForENIInterface(ctx, macAddr)
+		return nil
+	}))
+
+	res := <-resultCh
+	require.NoError(t, res.err, "failed to create dummy device asynchronously")
+	require.NotNil(t, res.cleanup)
+	defer res.cleanup()
+
+	require.NoError(t, waitErr, "WaitForENIInterface should succeed once the interface appears")
+}
+
+// TestPrivilegedWaitForENIInterfaceTimeout verifies that WaitForENIInterface
+// gives up and returns an error if the interface never appears within the
+// configured backoff.
+func TestPrivilegedWaitForENIInterfaceTimeout(t *testing.T) {
+	setupLinuxRoutingSuite(t)
+	withTestBackoff(t, wait.Backoff{Duration: 10 * time.Millisecond, Factor: 1.5, Steps: 3})
+
+	ns := netns.NewNetNS(t)
+	require.NoError(t, ns.Do(func() error {
+		macAddr, err := mac.ParseMAC("00:11:22:33:44:88")
+		require.NoError(t, err)
+
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+
+		err = WaitForENIInterface(ctx, macAddr)
+		require.Error(t, err, "interface never appears, so WaitForENIInterface should give up and return an error")
+		return nil
+	}))
+}
+
+// TestWaitForENIInterfaceContextCancelled: a cancelled context returns
+// promptly instead of exhausting the backoff. The backoff is deliberately
+// enormous, so the test hangs rather than passing spuriously if cancellation
+// is not honoured.
+func TestWaitForENIInterfaceContextCancelled(t *testing.T) {
+	withTestBackoff(t, wait.Backoff{Duration: time.Hour, Factor: 1, Steps: 100})
+
+	macAddr, err := mac.ParseMAC("00:11:22:33:44:99")
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	err = WaitForENIInterface(ctx, macAddr)
+	require.Error(t, err, "an already-cancelled context should cause WaitForENIInterface to return without exhausting the backoff")
+}
+
 func linkExistsWithMAC(t *testing.T, macAddr mac.MAC) bool {
 	links, err := safenetlink.LinkList()
 	require.NoError(t, err)