[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"
@@ -220,33 +219,21 @@ func (r *infraIPAllocator) reallocateOldRouterIPs(fromK8s, fromFS net.IP) (resul
 	return result
 }
 
+// waitForENI blocks until the ENI's netlink interface for macAddr is
+// observed on the node, guarding against the case where the operator has
+// reported the ENI as attached (and handed out IPs from it) before the
+// interface is actually visible via netlink. This delegates to the shared
+// linuxrouting.WaitForENIInterface helper, which is also used by the CNI ADD
+// datapath setup path and (below) the ingress IP path; see
+// https://github.com/cilium/cilium/pull/41954,
+// https://github.com/cilium/cilium/pull/47295, and
+// https://github.com/cilium/cilium/issues/45414.
 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 +286,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 +413,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 +463,20 @@ 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 {
+				// wait for the ENI to be up and running before configuring
+				// routes and rules, mirroring the guard used for the router
+				// interface above. See
+				// https://github.com/cilium/cilium/pull/41954 and
+				// https://github.com/cilium/cilium/pull/47295.
+				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 +555,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,17 @@ func Test_getCiliumHostIPsFromFile(t *testing.T) {
 		})
 	}
 }
+
+// TestWaitForENIInvalidMAC verifies that waitForENI fails fast with a clear
+// error when handed a malformed MAC address, rather than delegating to the
+// netlink poll and exhausting the backoff. This exercises the net.ParseMAC
+// guard added when the helper was refactored onto linuxrouting.WaitForENIInterface.
+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,73 @@ 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 will poll
+// for an ENI's netlink interface to appear 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 the
+// corresponding netlink device is actually visible on the node. Any code
+// path that resolves the ENI's ifindex by MAC address immediately after
+// that can lose the race and fail with "interface with MAC ... not found".
+//
+// This was first observed and guarded against for the router interface in
+// https://github.com/cilium/cilium/pull/41954, which used a 5-step backoff
+// (sleeps of 250ms, 500ms, 1s, 2s -- ~3.75s before jitter, up to ~4.5s with
+// the 20% jitter). That window has since proven too short in some
+// environments -- the guard still fires as a CI flake, see
+// https://github.com/cilium/cilium/issues/45414 -- so the shared helper below
+// extends it to 7 steps (7 condition checks, so 6 sleeps of 250ms, 500ms,
+// 1s, 2s, 4s, 8s -- ~15.75s before jitter, up to ~18.9s with the 20% jitter)
+// to give the ENI more time to show up. If it still never appears,
+// WaitForENIInterface returns an error; its callers (in
+// daemon/infraendpoints and plugins/cilium-cni) then log a warning and fall
+// through to their Configure call, which is what ultimately fails the
+// operation (e.g. CNI ADD) with "interface with MAC ... not found".
+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 address is observed on the node, or until ctx is cancelled or
+// WaitForENIInterfaceBackoff is exhausted.
+//
+// This is shared by every code path that needs to resolve an ENI's ifindex
+// by MAC address shortly after the ENI is reported as attached: the router
+// interface, the ingress IP interface, and the per-pod CNI ADD datapath
+// setup performed by retrieveIfIndexFromMAC below.
+//
+// See https://github.com/cilium/cilium/pull/41954 (original router-only
+// guard, merged) and https://github.com/cilium/cilium/pull/47295 (equivalent
+// guard for the ingress path, proposed upstream but not yet merged).
+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 {