fix(cache): GetStatusKeys returns cache keys, not raw node IDs

pkg/cache/v3/simple.go

@@ -1137,16 +1137,30 @@ func (cache *snapshotCache) GetStatusInfo(node string) StatusInfo {
 }
 
 // GetStatusKeys retrieves all node IDs in the status map.
+//
+// These are the keys the status and snapshot maps are actually keyed by -
+// hash.ID(node), as computed in CreateWatch/CreateDeltaWatch and stored by
+// getOrCreateStatus - so a caller can pass any of them straight to
+// GetSnapshot, ClearSnapshot or SetSnapshot.
+//
+// It previously returned statusInfo.GetNode().GetId(), the raw node ID off the
+// stored proto. For a NodeHash that returns node.Id verbatim (IDHash) the two
+// are the same string and nothing was visibly wrong. For any hash that derives
+// a key from more than node.Id - a per-pod or per-shard scheme, say - they
+// differ, and every GetSnapshot made from this list misses: lookups return no
+// snapshot, writes land on a key no watch is registered against, and both fail
+// silently because a missing snapshot is not an error condition on those paths.
 func (cache *snapshotCache) GetStatusKeys() []string {
-	// cache.mu.RLock()
-	// defer cache.mu.RUnlock()
-
-	all := cache.allStatus()
-	out := make([]string, 0, len(all))
-	for _, statusInfo := range all {
-		{
-			out = append(out, statusInfo.GetNode().GetId())
+	out := make([]string, 0, cacheShards)
+	for i := range cache.shards {
+		shard := &cache.shards[i]
+		shard.mu.RLock()
+		for key, info := range shard.status {
+			if info != nil {
+				out = append(out, key)
+			}
 		}
+		shard.mu.RUnlock()
 	}
 
 	return out

pkg/cache/v3/status_keys_test.go

@@ -0,0 +1,56 @@
+package cache_test
+
+import (
+	"testing"
+
+	core "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
+	discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
+	cache "github.com/envoyproxy/go-control-plane/pkg/cache/v3"
+	"github.com/envoyproxy/go-control-plane/pkg/server/stream/v3"
+)
+
+// suffixHash derives a key from more than node.Id, the way a per-pod or
+// per-shard scheme does. IDHash returns node.Id verbatim, which is what hid
+// this: with IDHash the cache key and the raw node ID are the same string.
+type suffixHash struct{}
+
+func (suffixHash) ID(node *core.Node) string {
+	if node == nil {
+		return ""
+	}
+	return node.GetId() + "~pod-1"
+}
+func (h suffixHash) CacheIndex(node *core.Node) int { return h.CacheIndexFromKey(h.ID(node)) }
+func (suffixHash) CacheIndexFromKey(key string) int { return len(key) % 8 }
+
+// GetStatusKeys must return keys that work with GetSnapshot. The status map is
+// keyed by hash.ID(node) (getOrCreateStatus), so returning the raw node ID off
+// the stored proto instead makes every lookup built from this list miss as soon
+// as the hash derives a key from anything but node.Id - silently, because a
+// missing snapshot is not an error on the write paths.
+func TestGetStatusKeysAreUsableAsCacheKeys(t *testing.T) {
+	c := cache.NewSnapshotCache(false, suffixHash{}, nil)
+
+	node := &core.Node{Id: "node-a"}
+	responses := make(chan cache.DeltaResponse, 1)
+	cancel, _ := c.CreateDeltaWatch(&discovery.DeltaDiscoveryRequest{
+		Node:    node,
+		TypeUrl: testTypes[0],
+	}, stream.NewStreamState(true, nil), responses)
+	defer cancel()
+
+	keys := c.GetStatusKeys()
+	if len(keys) != 1 {
+		t.Fatalf("GetStatusKeys returned %d keys, want 1: %v", len(keys), keys)
+	}
+
+	want := suffixHash{}.ID(node)
+	if keys[0] != want {
+		t.Fatalf("GetStatusKeys returned %q, want the cache key %q. The status map is keyed by hash.ID(node); returning the raw node ID makes every GetSnapshot built from this list miss.", keys[0], want)
+	}
+
+	// The contract that matters: the key round-trips through the cache.
+	if info := c.GetStatusInfo(keys[0]); info == nil {
+		t.Fatalf("GetStatusInfo(%q) found nothing - the key does not address the status map", keys[0])
+	}
+}