diff --git a/pkg/ipcache/metadata_restart_test.go b/pkg/ipcache/metadata_restart_test.go new file mode 100644 index 0000000000000..21bb5fa4d5ba2 --- /dev/null +++ b/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) +} diff --git a/pkg/ipcache/restore/local_identity_restorer_test.go b/pkg/ipcache/restore/local_identity_restorer_test.go new file mode 100644 index 0000000000000..eff4f01a7545c --- /dev/null +++ b/pkg/ipcache/restore/local_identity_restorer_test.go @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Cilium + +package restoration + +// TestHostIdentityExcludedFromIPCacheRestoration and related tests document the +// restoration filter in dumpOldIPCache that contributes to the host-IP-as-world +// misclassification bug (see pkg/ipcache/metadata_restart_test.go for the full +// reproduction). +// +// The filter at local_identity_restorer.go:128: +// +// if nid.Scope() == identity.IdentityScopeLocal || +// nid == identity.ReservedIdentityIngress { +// localPrefixes[k.Prefix()] = nid +// } +// +// ReservedIdentityHost (id=1) has IdentityScopeGlobal (scope bits = 0). +// It does not satisfy either condition and is therefore never included in the +// restored set. After ipcachemap.Recreate() (cell.go:118) wipes the ipcache BPF +// map, host IP entries are absent until syncHostIPs.StartAndWaitFirst() runs +// (daemon.go:249). +// +// If a CiliumCIDRGroup covering the host IP is processed between those two +// points, resolveLabels() in metadata.go receives only the cidrgroup label, +// finds isInCluster=false, and calls AddWorldLabel() — assigning world identity +// to what should be a host IP. + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/cilium/cilium/pkg/identity" +) + +// TestHostIdentityExcludedFromIPCacheRestoration documents the restoration +// filter condition that causes host IPs to lose their identity on restart. +// It tests the exact predicate used by dumpOldIPCache at +// local_identity_restorer.go:128. +func TestHostIdentityExcludedFromIPCacheRestoration(t *testing.T) { + type testCase struct { + name string + id identity.NumericIdentity + wantRestored bool + explanation string + } + + cases := []testCase{ + { + name: "ReservedIdentityHost is NOT restored", + id: identity.ReservedIdentityHost, // id=1, scope=global + wantRestored: false, + explanation: "ReservedIdentityHost (id=1) has IdentityScopeGlobal (scope=0). " + + "It does not satisfy Scope()==IdentityScopeLocal and is not " + + "ReservedIdentityIngress. Host IP entries are therefore absent from " + + "the new ipcache BPF map until syncHostIPs runs. If a CiliumCIDRGroup " + + "covering the host IP is processed during this window, resolveLabels() " + + "assigns world identity instead of host.", + }, + { + name: "ReservedIdentityWorld is NOT restored", + id: identity.ReservedIdentityWorld, // id=2, scope=global + wantRestored: false, + explanation: "World identity is re-added as a /0 catch-all by syncHostIPs, not via restoration.", + }, + { + name: "ReservedIdentityIngress IS restored", + id: identity.ReservedIdentityIngress, + wantRestored: true, + explanation: "Ingress is explicitly included via the nid==ReservedIdentityIngress check.", + }, + { + name: "A local-scope CIDR identity IS restored", + id: identity.NumericIdentity(1<<24 + 42), // IdentityScopeLocal | 42 + wantRestored: true, + explanation: "Local-scope CIDR identities are restored to preserve numeric identity " + + "stability across restarts. This is the primary use case for dumpOldIPCache.", + }, + { + name: "A remote-node-scope identity is NOT restored", + id: identity.NumericIdentity(2<<24 + 1), // IdentityScopeRemoteNode | 1 + wantRestored: false, + explanation: "Remote-node-scope identities are re-derived from the node manager, not restored.", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // This is the exact filter predicate from dumpOldIPCache: + // local_identity_restorer.go:128 + wouldBeRestored := tc.id.Scope() == identity.IdentityScopeLocal || + tc.id == identity.ReservedIdentityIngress + + assert.Equal(t, tc.wantRestored, wouldBeRestored, + "%s\n"+ + " identity id: %d\n"+ + " identity scope: %d (IdentityScopeLocal=%d, IdentityScopeGlobal=%d)\n"+ + " filter result: restored=%v", + tc.explanation, + tc.id, + tc.id.Scope(), + identity.IdentityScopeLocal, + identity.IdentityScopeGlobal, + wouldBeRestored, + ) + }) + } +} + +// TestHostIdentityScopeIsGlobal explicitly verifies that ReservedIdentityHost +// has global scope — the direct mechanical reason it is excluded from the +// dumpOldIPCache restoration filter and why the world fallback can fire. +func TestHostIdentityScopeIsGlobal(t *testing.T) { + hostScope := identity.ReservedIdentityHost.Scope() + + assert.Equal(t, identity.IdentityScopeGlobal, hostScope, + "ReservedIdentityHost must have IdentityScopeGlobal (scope=0). "+ + "This means it is excluded by the dumpOldIPCache filter "+ + "(local_identity_restorer.go:128) which only retains IdentityScopeLocal "+ + "and ReservedIdentityIngress. Host IP entries are therefore absent from "+ + "the new ipcache BPF map after Recreate() until syncHostIPs completes.") + + assert.NotEqual(t, identity.IdentityScopeLocal, hostScope, + "If this assertion fails, the bug would be self-healing: "+ + "host identity would be restored from the old BPF map and the world fallback "+ + "would not occur during the CiliumCIDRGroup processing window.") +}