ipcache: add tests reproducing host-IP-as-world misclassification during agent restart

pkg/ipcache/metadata_restart_test.go

@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Cilium
+
+package ipcache
+
+// TestHostIPWorldFallbackDuringRestartWindow and related tests reproduce a bug
+// where node/host IPs that fall within a CiliumCIDRGroup CIDR are transiently
+// misclassified as "world" identity during rolling Cilium agent restarts.
+//
+// Root cause (two code paths, both required):
+//
+// 1. pkg/ipcache/restore/local_identity_restorer.go:128
+//    dumpOldIPCache() only restores IdentityScopeLocal and ReservedIdentityIngress
+//    identities. ReservedIdentityHost (scope=global, id=1) is explicitly excluded.
+//    After ipcachemap.Recreate(), the new BPF ipcache map has no entry for host IPs.
+//
+// 2. daemon/cmd/daemon.go startup ordering
+//    K8sWatcher.InitK8sSubsystem() starts at line 202 (begins processing
+//    CiliumCIDRGroups). syncHostIPs.StartAndWaitFirst() is not called until
+//    line 249. During this window, a host IP covered by a CiliumCIDRGroup
+//    receives only a cidrgroup label — no reserved:host.
+//
+// 3. pkg/ipcache/metadata.go:798 (resolveLabels)
+//    Any IP without reserved:host, reserved:remote-node, reserved:health, or
+//    reserved:ingress has AddWorldLabel() called on it. A host IP with only
+//    a cidrgroup label is therefore assigned world identity.
+//
+// Impact: CNPs using "fromEntities: cluster" do not cover world (id=2). Traffic
+// from the misclassified host IP is denied with policy_denied.
+
+import (
+	"net/netip"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	cmtypes "github.com/cilium/cilium/pkg/clustermesh/types"
+	"github.com/cilium/cilium/pkg/identity"
+	ipcacheTypes "github.com/cilium/cilium/pkg/ipcache/types"
+	"github.com/cilium/cilium/pkg/labels"
+	"github.com/cilium/cilium/pkg/option"
+	"github.com/cilium/cilium/pkg/source"
+)
+
+// cidrGroupLabels returns a Labels set simulating what a CiliumCIDRGroup
+// reconciler injects via UpsertMetadata for an IP that matches a
+// CiliumCIDRGroup (e.g. a group covering a node-local subnet).
+func cidrGroupLabels(groupName string) labels.Labels {
+	return labels.Labels{
+		groupName: labels.NewLabel(groupName, "", labels.LabelSourceCIDRGroup),
+	}
+}
+
+// TestHostIPWorldFallbackDuringRestartWindow reproduces the bug where a host IP
+// covered by a CiliumCIDRGroup is assigned world identity because resolveLabels()
+// runs with only cidrgroup labels — before syncHostIPs has inserted reserved:host.
+//
+// This test asserts the CURRENT BUGGY BEHAVIOUR. It is expected to fail once
+// the root cause is fixed (e.g. by ensuring host IPs are seeded into ipcache
+// metadata before CiliumCIDRGroup processing can trigger resolveLabels for
+// those prefixes, or by restoring host identity entries in dumpOldIPCache).
+func TestHostIPWorldFallbackDuringRestartWindow(t *testing.T) {
+	s := setupIPCacheTestSuite(t)
+	ctx := t.Context()
+
+	// Disable PolicyCIDRMatchMode to avoid interference from node-CIDR matching.
+	oldVal := option.Config.PolicyCIDRMatchMode
+	t.Cleanup(func() { option.Config.PolicyCIDRMatchMode = oldVal })
+	option.Config.PolicyCIDRMatchMode = []string{}
+
+	// A host IP that falls within a CiliumCIDRGroup subnet.
+	hostIPPrefix := cmtypes.NewLocalPrefixCluster(netip.MustParsePrefix("10.161.39.126/32"))
+
+	// ── Stage 1: Restart window ──────────────────────────────────────────────
+	// The K8s watcher has processed a CiliumCIDRGroup covering this IP's subnet.
+	// The ipcache BPF map has been recreated empty (dumpOldIPCache skipped this
+	// IP since ReservedIdentityHost is not locally-scoped). syncHostIPs has NOT
+	// run yet — only the cidrgroup label is present.
+	s.IPIdentityCache.metadata.upsertLocked(
+		hostIPPrefix,
+		source.Generated,
+		"cidrgroup-resource-uid",
+		cidrGroupLabels("example-local-subnet"),
+	)
+
+	_, err := s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix})
+	require.NoError(t, err)
+
+	entry, ok := s.IPIdentityCache.ipToIdentityCache["10.161.39.126/32"]
+	require.True(t, ok, "expected an identity entry for the host IP")
+
+	assignedID := entry.ID
+
+	// Verify the assigned identity is NOT reserved:host (id=1).
+	// This demonstrates the bug: the IP should be host but is not.
+	assert.NotEqual(t, identity.ReservedIdentityHost, assignedID,
+		"BUG REPRODUCED: host IP was not assigned ReservedIdentityHost (id=1). "+
+			"Got id=%d. This occurs because resolveLabels() ran with only cidrgroup "+
+			"labels (no reserved:host) during the restart window before syncHostIPs "+
+			"executed.",
+		assignedID)
+
+	// Verify the assigned identity carries a world label — the world fallback fired.
+	resolvedIdentity := s.Allocator.LookupIdentityByID(ctx, assignedID)
+	require.NotNil(t, resolvedIdentity, "identity %d should be resolvable", assignedID)
+	assert.True(t,
+		resolvedIdentity.Labels.HasWorldLabel() || resolvedIdentity.Labels.HasWorldIPv4Label(),
+		"BUG: host IP was assigned world identity (id=%d, labels=%v). "+
+			"resolveLabels() called AddWorldLabel() because HasHostLabel()=false. "+
+			"Traffic from this IP will be denied by CNPs that use 'fromEntities: cluster' "+
+			"because world (id=2) is not in the cluster entity.",
+		assignedID, resolvedIdentity.Labels)
+
+	// ── Stage 2: syncHostIPs runs ────────────────────────────────────────────
+	// After daemon initialisation completes (daemon.go:249), syncHostIPs inserts
+	// the reserved:host label. resolveLabels() now sees HasHostLabel()=true,
+	// sets isInCluster=true, removes cidrgroup labels, and does NOT add world.
+	s.IPIdentityCache.metadata.upsertLocked(
+		hostIPPrefix,
+		source.Local,
+		"daemon-reserved",
+		labels.LabelHost,
+	)
+
+	_, err = s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix})
+	require.NoError(t, err)
+
+	correctedEntry, ok := s.IPIdentityCache.ipToIdentityCache["10.161.39.126/32"]
+	require.True(t, ok)
+
+	// After syncHostIPs runs, the identity must be corrected to reserved:host.
+	assert.Equal(t, identity.ReservedIdentityHost, correctedEntry.ID,
+		"After syncHostIPs inserts reserved:host, identity should be corrected to "+
+			"ReservedIdentityHost (id=1). Got id=%d.", correctedEntry.ID)
+}
+
+// TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst verifies the CORRECT
+// behaviour: when reserved:host is already present before CIDRGroup labels arrive,
+// resolveLabels() correctly identifies the IP as in-cluster and does not add
+// the world label.
+//
+// This is the inverse of TestHostIPWorldFallbackDuringRestartWindow and
+// documents the expected steady-state behaviour (no restart window).
+func TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst(t *testing.T) {
+	s := setupIPCacheTestSuite(t)
+	ctx := t.Context()
+
+	oldVal := option.Config.PolicyCIDRMatchMode
+	t.Cleanup(func() { option.Config.PolicyCIDRMatchMode = oldVal })
+	option.Config.PolicyCIDRMatchMode = []string{}
+
+	hostIPPrefix := cmtypes.NewLocalPrefixCluster(netip.MustParsePrefix("10.161.39.126/32"))
+
+	// syncHostIPs runs FIRST (correct startup order / no restart window).
+	s.IPIdentityCache.metadata.upsertLocked(
+		hostIPPrefix,
+		source.Local,
+		"daemon-reserved",
+		labels.LabelHost,
+	)
+
+	// CiliumCIDRGroup label arrives afterwards (normal steady-state order).
+	s.IPIdentityCache.metadata.upsertLocked(
+		hostIPPrefix,
+		source.Generated,
+		"cidrgroup-resource-uid",
+		cidrGroupLabels("example-local-subnet"),
+	)
+
+	_, err := s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix})
+	require.NoError(t, err)
+
+	entry, ok := s.IPIdentityCache.ipToIdentityCache["10.161.39.126/32"]
+	require.True(t, ok)
+
+	// When reserved:host is present first, identity must be ReservedIdentityHost.
+	assert.Equal(t, identity.ReservedIdentityHost, entry.ID,
+		"When reserved:host is in ipcache metadata before CIDRGroup labels arrive, "+
+			"the identity must be ReservedIdentityHost (id=1). Got id=%d.", entry.ID)
+
+	resolvedIdentity := s.Allocator.LookupIdentityByID(ctx, entry.ID)
+	require.NotNil(t, resolvedIdentity)
+	assert.False(t,
+		resolvedIdentity.Labels.HasWorldLabel() || resolvedIdentity.Labels.HasWorldIPv4Label(),
+		"Identity must not have world label when reserved:host is present. Labels: %v",
+		resolvedIdentity.Labels)
+}
+
+// TestHostIPWorldFallbackRaceSimulation simulates the actual daemon startup
+// sequencing using real goroutines and explicit synchronisation barriers.
+//
+// This mirrors the two concurrent actors from daemon.go:
+//   - goroutine A: daemon.go:202 — K8sWatcher processes CiliumCIDRGroups
+//   - goroutine B: daemon.go:249 — syncHostIPs.StartAndWaitFirst() completes
+//
+// The barrier channel enforces the ordering that causes the bug: A completes
+// and triggers label injection BEFORE B starts. The test then confirms:
+//   1. During the window (after A, before B): world identity is assigned.
+//   2. After B completes: identity is corrected to reserved:host.
+//
+// Run with -race to confirm there are no concurrent memory access violations
+// (the bug is a *logical* ordering issue, not a data race):
+//
+//	go test ./pkg/ipcache/... -run TestHostIPWorldFallbackRaceSimulation -race -v
+func TestHostIPWorldFallbackRaceSimulation(t *testing.T) {
+	s := setupIPCacheTestSuite(t)
+	ctx := t.Context()
+
+	oldVal := option.Config.PolicyCIDRMatchMode
+	t.Cleanup(func() { option.Config.PolicyCIDRMatchMode = oldVal })
+	option.Config.PolicyCIDRMatchMode = []string{}
+
+	hostIPPrefix := cmtypes.NewLocalPrefixCluster(netip.MustParsePrefix("10.0.1.1/32"))
+	resource := ipcacheTypes.NewResourceID(ipcacheTypes.ResourceKindDaemon, "", "test")
+
+	// cidrGroupDone is closed when goroutine A has finished injecting CIDRGroup
+	// labels — i.e. the end of the "window" during which the bug is present.
+	cidrGroupDone := make(chan struct{})
+
+	// ── Goroutine A: K8sWatcher / CiliumCIDRGroup reconciler ─────────────────
+	// Simulates daemon.go:202: K8sWatcher.InitK8sSubsystem() starts processing
+	// CiliumCIDRGroups. Uses the same public API that the reconciler uses:
+	// IPCache.UpsertMetadataBatch().
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		defer close(cidrGroupDone)
+
+		// Simulate the CIDRGroup reconciler upserting cidrgroup labels for a
+		// subnet that contains a host IP.
+		s.IPIdentityCache.metadata.upsertLocked(
+			hostIPPrefix,
+			source.Generated,
+			resource,
+			cidrGroupLabels("example-local-subnet"),
+		)
+		// Synchronously inject labels to simulate the reconciler draining its queue.
+		_, _ = s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix})
+	}()
+
+	// Wait for goroutine A to finish: we are now inside the race window.
+	// The host IP has been assigned an identity, but syncHostIPs has not run yet.
+	<-cidrGroupDone
+
+	// ── Observe the intermediate state (the race window) ─────────────────────
+	// At this point in the real daemon, any endpoint regeneration triggered by
+	// the CIDRGroup update would see world identity for the host IP.
+	windowEntry, ok := s.IPIdentityCache.ipToIdentityCache["10.0.1.1/32"]
+	require.True(t, ok, "host IP should have an identity during the race window")
+
+	windowIdentity := s.Allocator.LookupIdentityByID(ctx, windowEntry.ID)
+	require.NotNil(t, windowIdentity)
+
+	// BUG: during the window, world identity is assigned.
+	assert.NotEqual(t, identity.ReservedIdentityHost, windowEntry.ID,
+		"RACE WINDOW OBSERVED: host IP has id=%d (not ReservedIdentityHost) "+
+			"while syncHostIPs has not yet run", windowEntry.ID)
+	assert.True(t,
+		windowIdentity.Labels.HasWorldLabel() || windowIdentity.Labels.HasWorldIPv4Label(),
+		"RACE WINDOW: host IP labels=%v should contain world label", windowIdentity.Labels)
+
+	// ── Goroutine B: syncHostIPs.StartAndWaitFirst() ─────────────────────────
+	// Simulates daemon.go:249: syncHostIPs runs after daemon init completes.
+	// This is the same call that syncHostIPs.sync() makes at line 205:
+	//   s.params.IPCache.UpsertMetadata(p, source.Local, daemonResourceID, lbls)
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+
+		s.IPIdentityCache.metadata.upsertLocked(
+			hostIPPrefix,
+			source.Local,
+			resource,
+			labels.LabelHost,
+		)
+		_, _ = s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix})
+	}()
+
+	// Wait for goroutine B to finish (syncHostIPs has now run).
+	wg.Wait()
+
+	// ── Verify correction ─────────────────────────────────────────────────────
+	// After syncHostIPs runs, resolveLabels() sees HasHostLabel()=true,
+	// sets isInCluster=true, removes the cidrgroup label, and does not add world.
+	// The identity must be corrected to ReservedIdentityHost.
+	correctedEntry, ok := s.IPIdentityCache.ipToIdentityCache["10.0.1.1/32"]
+	require.True(t, ok)
+	assert.Equal(t, identity.ReservedIdentityHost, correctedEntry.ID,
+		"After syncHostIPs runs, identity must be corrected to ReservedIdentityHost (id=1). "+
+			"Got id=%d.", correctedEntry.ID)
+}
+
+// TestHostIPWorldFallbackStress exercises the ordering sensitivity by running
+// many iterations where goroutine A (CIDRGroup) and goroutine B (syncHostIPs)
+// race without an explicit ordering barrier.
+//
+// When A wins the race (cidrgroup before host), world identity is transiently
+// assigned. When B wins (host before cidrgroup), host identity is assigned
+// directly. The final state should always be ReservedIdentityHost regardless
+// of which goroutine "won" — but during the bug window, it may not be.
+//
+// Run this with:
+//
+//	go test ./pkg/ipcache/... -run TestHostIPWorldFallbackStress -race -count=5 -v
+//
+// The -race flag will confirm there are no concurrent memory violations.
+// The bug is a logical ordering issue, invisible to the race detector.
+func TestHostIPWorldFallbackStress(t *testing.T) {
+	const iterations = 50
+
+	oldVal := option.Config.PolicyCIDRMatchMode
+	t.Cleanup(func() { option.Config.PolicyCIDRMatchMode = oldVal })
+	option.Config.PolicyCIDRMatchMode = []string{}
+
+	resource := ipcacheTypes.NewResourceID(ipcacheTypes.ResourceKindDaemon, "", "stress-test")
+
+	worldWins := 0
+	hostWins := 0
+
+	for i := 0; i < iterations; i++ {
+		s := setupIPCacheTestSuite(t)
+		ctx := t.Context()
+		hostIPPrefix := cmtypes.NewLocalPrefixCluster(netip.MustParsePrefix("10.0.1.1/32"))
+
+		var wg sync.WaitGroup
+		var mu sync.Mutex
+		var intermediateID identity.NumericIdentity
+
+		// Goroutine A: CiliumCIDRGroup reconciler (daemon.go:202).
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			s.IPIdentityCache.metadata.upsertLocked(
+				hostIPPrefix, source.Generated, resource,
+				cidrGroupLabels("example-local-subnet"),
+			)
+			prefixes := []cmtypes.PrefixCluster{hostIPPrefix}
+			_, _ = s.IPIdentityCache.doInjectLabels(ctx, prefixes)
+
+			// Capture the identity at the moment A finishes.
+			mu.Lock()
+			if e, ok := s.IPIdentityCache.ipToIdentityCache["10.0.1.1/32"]; ok {
+				intermediateID = e.ID
+			}
+			mu.Unlock()
+		}()
+
+		// Goroutine B: syncHostIPs (daemon.go:249).
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			s.IPIdentityCache.metadata.upsertLocked(
+				hostIPPrefix, source.Local, resource,
+				labels.LabelHost,
+			)
+			_, _ = s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix})
+		}()
+
+		wg.Wait()
+
+		// Check the intermediate identity captured by goroutine A.
+		mu.Lock()
+		id := intermediateID
+		mu.Unlock()
+
+		if id == identity.ReservedIdentityHost {
+			hostWins++
+		} else {
+			worldWins++
+		}
+
+		// The FINAL state (after both goroutines) must always be host.
+		finalEntry, ok := s.IPIdentityCache.ipToIdentityCache["10.0.1.1/32"]
+		require.True(t, ok)
+		assert.Equal(t, identity.ReservedIdentityHost, finalEntry.ID,
+			"iteration %d: final identity must be ReservedIdentityHost after "+
+				"both goroutines complete. Got id=%d.", i, finalEntry.ID)
+
+		s.IPIdentityCache.Shutdown()
+	}
+
+	t.Logf("Over %d iterations: cidrgroup-first (world identity window)=%d, "+
+		"host-first (no bug window)=%d",
+		iterations, worldWins, hostWins)
+
+	// In most runs, at least some iterations will show the world identity window.
+	// If worldWins==0 always, the goroutine scheduler always ran B before A,
+	// which would be surprising over 50 iterations.
+	t.Logf("NOTE: worldWins=%d means the race window was observable %d times. "+
+		"A value of 0 does not mean the bug is fixed — it means the goroutine "+
+		"scheduler happened to always run syncHostIPs first in this run.", worldWins, worldWins)
+}