From 93ec997e138aacc6ea4a530a16588d4243b2342c Mon Sep 17 00:00:00 2001 From: Sean Duncan Date: Fri, 20 Mar 2026 16:11:26 -0400 Subject: [PATCH 1/3] ipcache: add tests reproducing host-IP-as-world misclassification on restart Add two test files that reproduce and document a bug where node/host IPs inside a local-DC CiliumCIDRGroup are misclassified as world identity during rolling Cilium agent restarts. Root cause (two cooperating code paths): 1. local_identity_restorer.go:128 - dumpOldIPCache() only restores identities with IdentityScopeLocal or ReservedIdentityIngress. ReservedIdentityHost (id=1, scope=global) is excluded. After ipcachemap.Recreate() wipes the BPF map, host IP entries are gone until syncHostIPs runs. 2. daemon.go startup ordering - K8sWatcher.InitK8sSubsystem() (line 202) starts CiliumCIDRGroup processing before syncHostIPs.StartAndWaitFirst() (line 249). During this window a host IP in a local-DC CiliumCIDRGroup receives only a cidrgroup label. 3. metadata.go:798 resolveLabels() - without reserved:host, HasHostLabel()=false, isInCluster=false, AddWorldLabel() fires. The IP is assigned world identity. Impact: traffic from node IPs to cluster-dns is denied because the CNP uses fromEntities:cluster (which excludes world). Confirmed in production at us1.fed.dog (plain1, us-gov-west-1a) on 2026-03-20 with 10,551 drops over 48h. pkg/ipcache/metadata_restart_test.go: - TestHostIPWorldFallbackDuringRestartWindow: asserts the buggy behaviour where a host IP with only a cidrgroup label gets world identity. Expected to fail once the root cause is fixed. - TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst: asserts the correct steady-state behaviour where reserved:host arrives before cidrgroup labels. pkg/ipcache/restore/local_identity_restorer_test.go: - TestHostIdentityExcludedFromIPCacheRestoration: directly tests the filter condition at local_identity_restorer.go:128 for all relevant identity types. - TestHostIdentityScopeIsGlobal: verifies that ReservedIdentityHost.Scope() returns IdentityScopeGlobal, which is why it is excluded from restoration. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/ipcache/metadata_restart_test.go | 187 ++++++++++++++++++ .../restore/local_identity_restorer_test.go | 135 +++++++++++++ 2 files changed, 322 insertions(+) create mode 100644 pkg/ipcache/metadata_restart_test.go create mode 100644 pkg/ipcache/restore/local_identity_restorer_test.go diff --git a/pkg/ipcache/metadata_restart_test.go b/pkg/ipcache/metadata_restart_test.go new file mode 100644 index 0000000000000..e4503bd27c195 --- /dev/null +++ b/pkg/ipcache/metadata_restart_test.go @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Cilium + +package ipcache + +// TestHostIPWorldFallbackDuringRestartWindow and TestHostIdentityRestorationGap +// reproduce a bug observed in production (us1.fed.dog, 2026-03-20) where node/host +// IPs in a local-DC CIDR were misclassified as "world" identity during rolling +// Cilium agent restarts, causing policy_denied drops against cluster-dns. +// +// Root cause (two code paths, both required): +// +// 1. pkg/ipcache/restore/local_identity_restorer.go:128 +// dumpOldIPCache() filters restored identities to IdentityScopeLocal and +// ReservedIdentityIngress only. ReservedIdentityHost (scope=global, id=1) is +// explicitly excluded. After ipcachemap.Recreate(), the new BPF map has no +// entry for host IPs. +// +// 2. daemon/cmd/daemon.go startup ordering +// K8sWatcher.InitK8sSubsystem() starts at line 202 (begins processing +// CiliumCIDRGroups managed by fabric-k8s-controller). syncHostIPs.StartAndWaitFirst() +// is not called until line 249. During this window, a host IP in the local-DC +// CiliumCIDRGroup (e.g. 10.160.0.0/14) receives only a cidrgroup label. +// +// 3. pkg/ipcache/metadata.go:798 (resolveLabels) +// Any IP without reserved:host, reserved:remote-node, reserved:health, or +// reserved:ingress label has AddWorldLabel() called on it. A host IP with only +// a cidrgroup label therefore becomes world — which is not covered by the +// cluster-dns CNP's "fromEntities: cluster" ingress rule, causing drops. + +import ( + "net/netip" + "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" + "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 the CiliumCIDRGroup reconciler +// (fabric-k8s-controller) injects via UpsertMetadata for an IP that matches a +// CiliumCIDRGroup (e.g. the "local-dc" group covering 10.160.0.0/14). +func cidrGroupLabels(groupName string) labels.Labels { + return labels.Labels{ + groupName: labels.NewLabel(groupName, "", labels.LabelSourceCIDRGroup), + } +} + +// TestHostIPWorldFallbackDuringRestartWindow reproduces the bug where a host IP +// is assigned world identity because resolveLabels() runs with only cidrgroup labels +// — before syncHostIPs has inserted the reserved:host label. +// +// This test asserts the CURRENT BUGGY BEHAVIOR. It is expected to fail once the +// bug is fixed (e.g. by ensuring host IPs are seeded into ipcache metadata before +// CiliumCIDRGroup processing can trigger resolveLabels for those prefixes). +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{} + + // The host IP observed in production: 10.161.39.126 (in 10.160.0.0/14, localDc CIDR). + // 8,258 drops were recorded against cluster-dns over 48h. + hostIPPrefix := cmtypes.NewLocalPrefixCluster(netip.MustParsePrefix("10.161.39.126/32")) + + // ── Stage 1: Restart window ────────────────────────────────────────────── + // K8sWatcher has processed the "local-dc" CiliumCIDRGroup. The ipcache BPF + // map has been recreated empty (RestoreLocalIdentities skipped this IP since + // ReservedIdentityHost is not locally-scoped). syncHostIPs has NOT run yet. + // + // Only the cidrgroup label is present — no reserved:host. + s.IPIdentityCache.metadata.upsertLocked( + hostIPPrefix, + source.Generated, + "cidrgroup-resource-uid", + cidrGroupLabels("local-dc"), + ) + + _, 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 10.161.39.126/32") + + 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 10.161.39.126 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 has 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 10.161.39.126/32 was assigned world identity (id=%d, labels=%v). "+ + "resolveLabels() called AddWorldLabel() because HasHostLabel()=false. "+ + "This causes policy_denied drops: the cluster-dns CNP allows 'fromEntities: cluster' "+ + "but world (id=2) is not in the cluster entity.", + assignedID, resolvedIdentity.Labels) + + // ── Stage 2: syncHostIPs runs ──────────────────────────────────────────── + // After daemon initialization completes (daemon.go:249), syncHostIPs inserts + // the reserved:host label for this IP. resolveLabels() now sees HasHostLabel()=true, + // sets isInCluster=true, removes the cidrgroup label, 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 present before CIDRGroup labels are processed, +// 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("local-dc"), + ) + + _, 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, the identity must be ReservedIdentityHost. + assert.Equal(t, identity.ReservedIdentityHost, entry.ID, + "When reserved:host is already 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) +} 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..8b3585b283b57 --- /dev/null +++ b/pkg/ipcache/restore/local_identity_restorer_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Cilium + +package restoration + +// TestHostIdentityExcludedFromIPCacheRestoration tests that the ipcache +// restoration logic (dumpOldIPCache) explicitly excludes ReservedIdentityHost +// entries from the set of identities that survive an agent restart. +// +// This is one of two root causes for the host-IP-as-world misclassification bug: +// +// dumpOldIPCache() at local_identity_restorer.go:128: +// +// if nid.Scope() == identity.IdentityScopeLocal || +// nid == identity.ReservedIdentityIngress { +// localPrefixes[k.Prefix()] = nid // host identity NEVER matches +// } +// +// Because ReservedIdentityHost (id=1) has IdentityScopeGlobal (scope bits = 0), +// it does not pass the IdentityScopeLocal check. It is also not +// ReservedIdentityIngress. The host IP entry from the OLD ipcache BPF map is +// therefore NEVER written into localPrefixes and is NEVER restored into the new +// ipcache metadata. +// +// Consequence: after ipcachemap.Recreate() wipes the BPF map (cell.go:118), +// there is a window before syncHostIPs runs (daemon.go:249) during which host +// IPs have no ipcache metadata entry. If a CiliumCIDRGroup covering the host IP +// is processed during this window, resolveLabels() sees only the cidrgroup label, +// calls AddWorldLabel(), and assigns world identity — causing policy_denied drops. + +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. +// +// The filter in dumpOldIPCache (local_identity_restorer.go:128) is: +// +// nid.Scope() == identity.IdentityScopeLocal || nid == identity.ReservedIdentityIngress +// +// This test verifies the scope/identity values used by the filter and shows +// which identities are retained vs dropped during restoration. +func TestHostIdentityExcludedFromIPCacheRestoration(t *testing.T) { + type testCase struct { + name string + id identity.NumericIdentity + wantRestored bool + explanation string + } + + // These cases mirror the exact filter condition at local_identity_restorer.go:128. + cases := []testCase{ + { + name: "ReservedIdentityHost is NOT restored", + id: identity.ReservedIdentityHost, // id=1, scope=global + wantRestored: false, + explanation: "ReservedIdentityHost (id=1) has IdentityScopeGlobal (scope bits = 0). " + + "It does not satisfy Scope()==IdentityScopeLocal and is not ReservedIdentityIngress. " + + "BUG: after ipcachemap.Recreate(), host IP entries are missing from the new ipcache " + + "until syncHostIPs runs. If CiliumCIDRGroup processing happens first, " + + "resolveLabels() assigns world identity to the host IP.", + }, + { + 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, which is why it is excluded from the restoration filter. +// This is the direct mechanical reason for the misclassification bug. +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. As a result, host IP entries are lost from "+ + "the ipcache BPF map after ipcachemap.Recreate() and are not re-inserted "+ + "until syncHostIPs.StartAndWaitFirst() completes (daemon.go:249).") + + assert.NotEqual(t, identity.IdentityScopeLocal, hostScope, + "If this assertion fails, the bug would be self-healing: "+ + "host identity would be restored and the world fallback would not occur.") +} From b2be3c85af74c825fbebd573f67fa0a8c2c1c697 Mon Sep 17 00:00:00 2001 From: Sean Duncan Date: Fri, 20 Mar 2026 16:21:13 -0400 Subject: [PATCH 2/3] ipcache: add tests reproducing host-IP-as-world misclassification on restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add two test files that reproduce and document a bug where host IPs covered by a CiliumCIDRGroup subnet are misclassified as world identity during rolling Cilium agent restarts. Root cause (two cooperating code paths): 1. local_identity_restorer.go:128 - dumpOldIPCache() only restores identities with IdentityScopeLocal or ReservedIdentityIngress. ReservedIdentityHost (id=1, scope=global) is excluded. After ipcachemap.Recreate() wipes the BPF map, host IP entries are gone until syncHostIPs runs. 2. daemon.go startup ordering - K8sWatcher.InitK8sSubsystem() (line 202) starts CiliumCIDRGroup processing before syncHostIPs.StartAndWaitFirst() (line 249). During this window a host IP covered by a CiliumCIDRGroup receives only a cidrgroup label — no reserved:host. 3. metadata.go:798 resolveLabels() - without reserved:host, HasHostLabel()=false, isInCluster=false, AddWorldLabel() fires. The IP is assigned world identity. Impact: traffic from the misclassified host IP is denied by CNPs that use fromEntities:cluster, because world (id=2) is not in the cluster entity. pkg/ipcache/metadata_restart_test.go: - TestHostIPWorldFallbackDuringRestartWindow: inserts only a cidrgroup label for a host IP (no reserved:host), runs doInjectLabels, and asserts that world identity is assigned. Expected to fail once the root cause is fixed. - TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst: verifies correct steady-state behaviour when reserved:host arrives before cidrgroup labels. pkg/ipcache/restore/local_identity_restorer_test.go: - TestHostIdentityExcludedFromIPCacheRestoration: tests the exact filter predicate from dumpOldIPCache line 128 for all relevant identity types. - TestHostIdentityScopeIsGlobal: verifies that ReservedIdentityHost.Scope() returns IdentityScopeGlobal, the mechanical reason it is excluded. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/ipcache/metadata_restart_test.go | 92 +++++++++---------- .../restore/local_identity_restorer_test.go | 79 ++++++++-------- 2 files changed, 82 insertions(+), 89 deletions(-) diff --git a/pkg/ipcache/metadata_restart_test.go b/pkg/ipcache/metadata_restart_test.go index e4503bd27c195..894d6cc867da1 100644 --- a/pkg/ipcache/metadata_restart_test.go +++ b/pkg/ipcache/metadata_restart_test.go @@ -3,30 +3,30 @@ package ipcache -// TestHostIPWorldFallbackDuringRestartWindow and TestHostIdentityRestorationGap -// reproduce a bug observed in production (us1.fed.dog, 2026-03-20) where node/host -// IPs in a local-DC CIDR were misclassified as "world" identity during rolling -// Cilium agent restarts, causing policy_denied drops against cluster-dns. +// 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() filters restored identities to IdentityScopeLocal and -// ReservedIdentityIngress only. ReservedIdentityHost (scope=global, id=1) is -// explicitly excluded. After ipcachemap.Recreate(), the new BPF map has no -// entry for host IPs. +// 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 managed by fabric-k8s-controller). syncHostIPs.StartAndWaitFirst() -// is not called until line 249. During this window, a host IP in the local-DC -// CiliumCIDRGroup (e.g. 10.160.0.0/14) receives only a cidrgroup label. +// 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 label has AddWorldLabel() called on it. A host IP with only -// a cidrgroup label therefore becomes world — which is not covered by the -// cluster-dns CNP's "fromEntities: cluster" ingress rule, causing drops. +// 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" @@ -42,9 +42,9 @@ import ( "github.com/cilium/cilium/pkg/source" ) -// cidrGroupLabels returns a Labels set simulating what the CiliumCIDRGroup reconciler -// (fabric-k8s-controller) injects via UpsertMetadata for an IP that matches a -// CiliumCIDRGroup (e.g. the "local-dc" group covering 10.160.0.0/14). +// 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), @@ -52,12 +52,13 @@ func cidrGroupLabels(groupName string) labels.Labels { } // TestHostIPWorldFallbackDuringRestartWindow reproduces the bug where a host IP -// is assigned world identity because resolveLabels() runs with only cidrgroup labels -// — before syncHostIPs has inserted the reserved:host label. +// 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 BEHAVIOR. It is expected to fail once the -// bug is fixed (e.g. by ensuring host IPs are seeded into ipcache metadata before -// CiliumCIDRGroup processing can trigger resolveLabels for those prefixes). +// 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() @@ -67,54 +68,53 @@ func TestHostIPWorldFallbackDuringRestartWindow(t *testing.T) { t.Cleanup(func() { option.Config.PolicyCIDRMatchMode = oldVal }) option.Config.PolicyCIDRMatchMode = []string{} - // The host IP observed in production: 10.161.39.126 (in 10.160.0.0/14, localDc CIDR). - // 8,258 drops were recorded against cluster-dns over 48h. + // A host IP that falls within a CiliumCIDRGroup subnet. hostIPPrefix := cmtypes.NewLocalPrefixCluster(netip.MustParsePrefix("10.161.39.126/32")) // ── Stage 1: Restart window ────────────────────────────────────────────── - // K8sWatcher has processed the "local-dc" CiliumCIDRGroup. The ipcache BPF - // map has been recreated empty (RestoreLocalIdentities skipped this IP since - // ReservedIdentityHost is not locally-scoped). syncHostIPs has NOT run yet. - // - // Only the cidrgroup label is present — no reserved:host. + // 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("local-dc"), + 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 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 10.161.39.126 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.", + "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 has a world label — the world fallback fired. + // 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 10.161.39.126/32 was assigned world identity (id=%d, labels=%v). "+ + "BUG: host IP was assigned world identity (id=%d, labels=%v). "+ "resolveLabels() called AddWorldLabel() because HasHostLabel()=false. "+ - "This causes policy_denied drops: the cluster-dns CNP allows 'fromEntities: cluster' "+ - "but world (id=2) is not in the cluster entity.", + "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 initialization completes (daemon.go:249), syncHostIPs inserts - // the reserved:host label for this IP. resolveLabels() now sees HasHostLabel()=true, - // sets isInCluster=true, removes the cidrgroup label, and does NOT add world. + // 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, @@ -135,7 +135,7 @@ func TestHostIPWorldFallbackDuringRestartWindow(t *testing.T) { } // TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst verifies the CORRECT -// behaviour: when reserved:host is present before CIDRGroup labels are processed, +// 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. // @@ -164,7 +164,7 @@ func TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst(t *testing.T) { hostIPPrefix, source.Generated, "cidrgroup-resource-uid", - cidrGroupLabels("local-dc"), + cidrGroupLabels("example-local-subnet"), ) _, err := s.IPIdentityCache.doInjectLabels(ctx, []cmtypes.PrefixCluster{hostIPPrefix}) @@ -173,10 +173,10 @@ func TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst(t *testing.T) { entry, ok := s.IPIdentityCache.ipToIdentityCache["10.161.39.126/32"] require.True(t, ok) - // When reserved:host is present, the identity must be ReservedIdentityHost. + // When reserved:host is present first, identity must be ReservedIdentityHost. assert.Equal(t, identity.ReservedIdentityHost, entry.ID, - "When reserved:host is already in ipcache metadata before CIDRGroup labels "+ - "arrive, the identity must be ReservedIdentityHost (id=1). Got id=%d.", 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) diff --git a/pkg/ipcache/restore/local_identity_restorer_test.go b/pkg/ipcache/restore/local_identity_restorer_test.go index 8b3585b283b57..eff4f01a7545c 100644 --- a/pkg/ipcache/restore/local_identity_restorer_test.go +++ b/pkg/ipcache/restore/local_identity_restorer_test.go @@ -3,30 +3,28 @@ package restoration -// TestHostIdentityExcludedFromIPCacheRestoration tests that the ipcache -// restoration logic (dumpOldIPCache) explicitly excludes ReservedIdentityHost -// entries from the set of identities that survive an agent restart. +// 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). // -// This is one of two root causes for the host-IP-as-world misclassification bug: +// The filter at local_identity_restorer.go:128: // -// dumpOldIPCache() at local_identity_restorer.go:128: +// if nid.Scope() == identity.IdentityScopeLocal || +// nid == identity.ReservedIdentityIngress { +// localPrefixes[k.Prefix()] = nid +// } // -// if nid.Scope() == identity.IdentityScopeLocal || -// nid == identity.ReservedIdentityIngress { -// localPrefixes[k.Prefix()] = nid // host identity NEVER matches -// } +// 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). // -// Because ReservedIdentityHost (id=1) has IdentityScopeGlobal (scope bits = 0), -// it does not pass the IdentityScopeLocal check. It is also not -// ReservedIdentityIngress. The host IP entry from the OLD ipcache BPF map is -// therefore NEVER written into localPrefixes and is NEVER restored into the new -// ipcache metadata. -// -// Consequence: after ipcachemap.Recreate() wipes the BPF map (cell.go:118), -// there is a window before syncHostIPs runs (daemon.go:249) during which host -// IPs have no ipcache metadata entry. If a CiliumCIDRGroup covering the host IP -// is processed during this window, resolveLabels() sees only the cidrgroup label, -// calls AddWorldLabel(), and assigns world identity — causing policy_denied drops. +// 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" @@ -38,32 +36,27 @@ import ( // TestHostIdentityExcludedFromIPCacheRestoration documents the restoration // filter condition that causes host IPs to lose their identity on restart. -// -// The filter in dumpOldIPCache (local_identity_restorer.go:128) is: -// -// nid.Scope() == identity.IdentityScopeLocal || nid == identity.ReservedIdentityIngress -// -// This test verifies the scope/identity values used by the filter and shows -// which identities are retained vs dropped during restoration. +// 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 + name string + id identity.NumericIdentity + wantRestored bool + explanation string } - // These cases mirror the exact filter condition at local_identity_restorer.go:128. cases := []testCase{ { name: "ReservedIdentityHost is NOT restored", id: identity.ReservedIdentityHost, // id=1, scope=global wantRestored: false, - explanation: "ReservedIdentityHost (id=1) has IdentityScopeGlobal (scope bits = 0). " + - "It does not satisfy Scope()==IdentityScopeLocal and is not ReservedIdentityIngress. " + - "BUG: after ipcachemap.Recreate(), host IP entries are missing from the new ipcache " + - "until syncHostIPs runs. If CiliumCIDRGroup processing happens first, " + - "resolveLabels() assigns world identity to the host IP.", + 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", @@ -116,8 +109,8 @@ func TestHostIdentityExcludedFromIPCacheRestoration(t *testing.T) { } // TestHostIdentityScopeIsGlobal explicitly verifies that ReservedIdentityHost -// has global scope, which is why it is excluded from the restoration filter. -// This is the direct mechanical reason for the misclassification bug. +// 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() @@ -125,11 +118,11 @@ func TestHostIdentityScopeIsGlobal(t *testing.T) { "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. As a result, host IP entries are lost from "+ - "the ipcache BPF map after ipcachemap.Recreate() and are not re-inserted "+ - "until syncHostIPs.StartAndWaitFirst() completes (daemon.go:249).") + "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 and the world fallback would not occur.") + "host identity would be restored from the old BPF map and the world fallback "+ + "would not occur during the CiliumCIDRGroup processing window.") } From 63e21eb1816f5900e5ae4a74462e0a50579a8a56 Mon Sep 17 00:00:00 2001 From: Sean Duncan Date: Fri, 20 Mar 2026 17:17:38 -0400 Subject: [PATCH 3/3] ipcache: add goroutine-based race simulation for host-IP-as-world bug Extend the test suite with two additional tests that simulate the race condition using real goroutines, mirroring the actual concurrent actors in the daemon startup sequence. TestHostIPWorldFallbackRaceSimulation: - Uses two goroutines separated by an explicit ordering barrier (channel). - Goroutine A (K8sWatcher / CIDRGroup reconciler): upserts cidrgroup labels and injects. Represents daemon.go:202 completing before syncHostIPs. - After A closes cidrGroupDone, the test observes the intermediate state: world identity is assigned (the bug window is open). - Goroutine B (syncHostIPs): unblocked by cidrGroupDone, upserts host label. - After B completes the test verifies identity is corrected to host. - Run with -race to confirm there are no data races (the bug is a logical ordering issue, not a concurrent memory violation). TestHostIPWorldFallbackStress: - Runs 50 iterations of truly concurrent goroutines (A and B race without a barrier) and records how often A wins the race (world window observed) vs B wins (host identity assigned directly). - Asserts the FINAL state is always ReservedIdentityHost after both goroutines complete, regardless of scheduling order. - Useful for amplifying the race: run with -count=5 -race across many iterations to observe the ordering sensitivity. Neither test modifies the existing sequential tests. All four tests remain passing with the bug present and the goroutine-based tests provide a more faithful simulation of the daemon startup race. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- pkg/ipcache/metadata_restart_test.go | 208 +++++++++++++++++++++++++++ 1 file changed, 208 insertions(+) diff --git a/pkg/ipcache/metadata_restart_test.go b/pkg/ipcache/metadata_restart_test.go index 894d6cc867da1..21bb5fa4d5ba2 100644 --- a/pkg/ipcache/metadata_restart_test.go +++ b/pkg/ipcache/metadata_restart_test.go @@ -30,6 +30,7 @@ package ipcache import ( "net/netip" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -37,6 +38,7 @@ import ( 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" @@ -185,3 +187,209 @@ func TestWorldFallbackDoesNotOccurWhenHostLabelPresentFirst(t *testing.T) { "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) +}