test: cache-staleness coverage for helm/v3 + envtest CI

.github/workflows/test.yaml

@@ -0,0 +1,32 @@
+name: test
+
+on:
+  pull_request:
+    branches:
+      - main
+  push:
+    branches:
+      - main
+
+permissions:
+  contents: read
+
+jobs:
+  test:
+    name: unit + envtest
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Set up Go
+        uses: actions/setup-go@v6
+        with:
+          go-version-file: go.mod
+          cache: true
+
+      - name: Unit tests (race)
+        run: make test-race
+
+      - name: Functional cache-staleness tests (envtest)
+        run: make test-envtest

.gitignore

@@ -32,3 +32,6 @@ experiments/**
 **/token.txt
 **/*.crt
 **/*.key
+
+# make test-envtest installs setup-envtest here
+/bin/

Makefile

@@ -0,0 +1,38 @@
+# Test targets. `test` runs the fast unit suite; `test-envtest` runs the build-tagged
+# functional cache-staleness tests against a real kube-apiserver via controller-runtime/envtest.
+
+SHELL := /usr/bin/env bash
+
+LOCALBIN ?= $(CURDIR)/bin
+SETUP_ENVTEST ?= $(LOCALBIN)/setup-envtest
+# Kubebuilder envtest control-plane (kube-apiserver + etcd) version used by the `-tags envtest` tests.
+ENVTEST_K8S_VERSION ?= 1.36.0
+
+.PHONY: test
+test: ## Run the unit tests (fast; excludes the envtest-tagged functional tests).
+	go test ./... -count=1
+
+.PHONY: test-race
+test-race: ## Run the unit tests with the race detector.
+	go test ./... -race -count=1
+
+$(LOCALBIN):
+	mkdir -p $(LOCALBIN)
+
+.PHONY: setup-envtest
+setup-envtest: $(SETUP_ENVTEST) ## Install setup-envtest into ./bin.
+$(SETUP_ENVTEST): | $(LOCALBIN)
+	# Pin to the controller-runtime 0.22 line (matches sigs.k8s.io/controller-runtime v0.22.3 in
+	# go.mod). Do NOT use @latest: newer setup-envtest (v0.24+) requires go >= 1.26 and fails under
+	# this module's go 1.25 toolchain (GOTOOLCHAIN=local) with "requires go >= 1.26.0".
+	GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.22
+
+.PHONY: test-envtest
+test-envtest: setup-envtest ## Run the envtest (real-apiserver) functional cache-staleness tests.
+	KUBEBUILDER_ASSETS="$$($(SETUP_ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" \
+		go test -tags envtest ./... -count=1
+
+.PHONY: help
+help: ## Show this help.
+	@grep -hE '^[a-zA-Z_-]+:.*?## ' $(MAKEFILE_LIST) | \
+		awk 'BEGIN{FS=":.*?## "}{printf "  \033[36m%-16s\033[0m %s\n",$$1,$$2}'

helm/v3/cachedclients_stale_success_test.go

@@ -0,0 +1,94 @@
+package helm
+
+import (
+	"sync"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/client-go/discovery/cached/memory"
+	"k8s.io/client-go/restmapper"
+)
+
+// TestDeferredMapper_SuccessfulMapping_StaleUntilReset pins the most dangerous staleness class the
+// rest of the suite structurally cannot catch: a kind that RESOLVED successfully, whose CRD is then
+// removed (or version-bumped so this exact mapping disappears). DeferredDiscoveryRESTMapper.RESTMapping
+// only auto-heals on the `err != nil && !cl.Fresh()` branch, so a *successful* stale mapping
+// short-circuits the heal and is served forever from the cached delegate.
+//
+// The load-bearing consequence: a bare memcache.Invalidate() is INSUFFICIENT here — the mapper's
+// delegate is not rebuilt while it still answers — whereas mapper.Reset() (which nils the delegate)
+// recovers. This is precisely why the cache-staleness fix must call mapper.Reset() via the CRD
+// informer, not merely invalidate the discovery cache. (The complementary miss->register->resolve
+// path is covered by TestDeferredMapper_StaleUntilInvalidated in cachedclients_staleness_test.go.)
+func TestDeferredMapper_SuccessfulMapping_StaleUntilReset(t *testing.T) {
+	// Start WITH the Widget CRD present in discovery.
+	initial := append(coreOnlyResources(), widgetResourceList())
+	fake := newFakeDiscovery(initial)
+	memcache := memory.NewMemCacheClient(fake)
+	mapper := restmapper.NewDeferredDiscoveryRESTMapper(memcache)
+
+	// Warm up: the kind resolves, the mapper caches its delegate, and memcache becomes Fresh.
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); err != nil {
+		t.Fatalf("precondition: Widget should resolve while its CRD is present, got: %v", err)
+	}
+
+	// Simulate the CRD going away (deleted, or its version bumped so this exact GVK no longer exists).
+	fake.Resources = coreOnlyResources()
+
+	// Bare cache invalidation WITHOUT a mapper Reset: the cached delegate still answers Widget, so
+	// RESTMapping returns err==nil and never reaches the freshness-guarded heal. The stale SUCCESS
+	// is served. This assertion documents (not endorses) that Invalidate alone is not enough.
+	memcache.Invalidate()
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); err != nil {
+		t.Fatalf("stale-success: after a bare Invalidate the cached delegate should STILL resolve "+
+			"Widget (heal is skipped on the success path), but got: %v", err)
+	}
+
+	// Only an explicit Reset (delegate=nil) forces a rebuild from the now-empty discovery -> NoMatch.
+	mapper.Reset()
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); !meta.IsNoMatchError(err) {
+		t.Fatalf("after Reset the removed Widget must no longer map; want NoMatch, got: %v", err)
+	}
+}
+
+// TestDeferredMapper_ConcurrentResetAndMapping exercises concurrent Reset() + RESTMapping() on a single
+// SHARED DeferredDiscoveryRESTMapper — the exact reuse pattern the cdc has, where one CachedClients.mapper
+// is shared across many composition reconciles (readers) while the CRD informer / Install-retry calls
+// Reset() (writer). It must not panic, deadlock, or corrupt the mapper. Run the package with -race to
+// catch data races on the shared instance.
+func TestDeferredMapper_ConcurrentResetAndMapping(t *testing.T) {
+	fake := newFakeDiscovery(coreOnlyResources())
+	memcache := memory.NewMemCacheClient(fake)
+	mapper := restmapper.NewDeferredDiscoveryRESTMapper(memcache)
+
+	// Widget is present throughout: a well-behaved shared mapper must keep resolving it despite the
+	// concurrent Reset storm (each Reset just forces a re-discovery, never a permanent loss).
+	fake.Resources = append(fake.Resources, widgetResourceList())
+
+	var wg sync.WaitGroup
+	const readers = 8
+	const iters = 200
+
+	for i := 0; i < readers; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for j := 0; j < iters; j++ {
+				_, _ = mapper.RESTMapping(widgetGroupKind, "v1")
+			}
+		}()
+	}
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for j := 0; j < iters; j++ {
+			mapper.Reset()
+		}
+	}()
+	wg.Wait()
+
+	// After the storm, a final lookup must still succeed: no permanent corruption from concurrent access.
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); err != nil {
+		t.Fatalf("after a concurrent Reset/RESTMapping storm, Widget should still resolve, got: %v", err)
+	}
+}

helm/v3/cachedclients_staleness_test.go

@@ -0,0 +1,192 @@
+package helm
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/discovery/cached/memory"
+	discoveryfake "k8s.io/client-go/discovery/fake"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/restmapper"
+	clienttesting "k8s.io/client-go/testing"
+)
+
+// widgetGroupVersion / widgetGroupKind describe the new CRD (group example.com/v1, kind Widget)
+// that a fresh CRD registration would introduce. Discovery starts WITHOUT it.
+var (
+	widgetGroupVersion = "example.com/v1"
+	widgetGroupKind    = schema.GroupKind{Group: "example.com", Kind: "Widget"}
+)
+
+// coreOnlyResources is the initial discovery state: only the core v1 group, no example.com/v1.
+func coreOnlyResources() []*metav1.APIResourceList {
+	return []*metav1.APIResourceList{
+		{
+			GroupVersion: "v1",
+			APIResources: []metav1.APIResource{
+				{
+					Name:       "configmaps",
+					Namespaced: true,
+					Kind:       "ConfigMap",
+				},
+			},
+		},
+	}
+}
+
+// widgetResourceList is the discovery entry that appears once the Widget CRD registers.
+func widgetResourceList() *metav1.APIResourceList {
+	return &metav1.APIResourceList{
+		GroupVersion: widgetGroupVersion,
+		APIResources: []metav1.APIResource{
+			{
+				Name:         "widgets",
+				SingularName: "widget",
+				Namespaced:   true,
+				Kind:         "Widget",
+				Group:        "example.com",
+				Version:      "v1",
+			},
+		},
+	}
+}
+
+// newFakeDiscovery builds a mutable FakeDiscovery whose Resources list can be appended to at runtime
+// to simulate a CRD registering after the discovery/RESTMapper cache has already been warmed.
+func newFakeDiscovery(initial []*metav1.APIResourceList) *discoveryfake.FakeDiscovery {
+	return &discoveryfake.FakeDiscovery{
+		Fake: &clienttesting.Fake{
+			Resources: initial,
+		},
+	}
+}
+
+// TestNewCachedClients_ReturnsDeferredMapperAndCachedDiscovery verifies the shape of the value
+// NewCachedClients hands back: a *restmapper.DeferredDiscoveryRESTMapper and a non-nil
+// discovery.CachedDiscoveryInterface. These are the two objects the cdc must keep and invalidate.
+func TestNewCachedClients_ReturnsDeferredMapperAndCachedDiscovery(t *testing.T) {
+	cc, err := NewCachedClients(&rest.Config{Host: "https://127.0.0.1:6443"})
+	if err != nil {
+		t.Fatalf("NewCachedClients: %v", err)
+	}
+
+	if cc.mapper == nil {
+		t.Fatal("expected non-nil mapper")
+	}
+	// mapper is concretely a *restmapper.DeferredDiscoveryRESTMapper.
+	var _ *restmapper.DeferredDiscoveryRESTMapper = cc.mapper
+
+	if cc.discoveryClient == nil {
+		t.Fatal("expected non-nil discoveryClient")
+	}
+	// discoveryClient satisfies discovery.CachedDiscoveryInterface (compile-time + runtime).
+	var _ discovery.CachedDiscoveryInterface = cc.discoveryClient
+}
+
+// TestDeferredMapper_StaleUntilInvalidated is THE CORE staleness proof. It reproduces the bug the
+// cdc exhibits: after a new CRD registers, the DeferredDiscoveryRESTMapper (backed by a memcache
+// discovery client) keeps answering RESTMapping with a NoMatch error until the discovery cache is
+// invalidated. Without that invalidation, the umbrella's crdExists lookup misses the new kind and
+// Pass B skips the component — exactly the reported symptom. This is precisely what the CRD
+// informer's mapper.Reset() wiring (which the cdc does NOT install) is meant to trigger.
+func TestDeferredMapper_StaleUntilInvalidated(t *testing.T) {
+	fake := newFakeDiscovery(coreOnlyResources())
+	memcache := memory.NewMemCacheClient(fake)
+	mapper := restmapper.NewDeferredDiscoveryRESTMapper(memcache)
+
+	// (a) Before the CRD exists, RESTMapping must miss.
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); err == nil {
+		t.Fatal("expected NoMatch before CRD registers, got nil error")
+	} else if !meta.IsNoMatchError(err) {
+		t.Fatalf("expected NoMatch error before CRD registers, got: %v", err)
+	}
+
+	// (b) A new CRD registers: append the Widget resource to discovery. The underlying discovery
+	// server now knows Widget, but the memcache + deferred mapper have already cached the miss.
+	fake.Resources = append(fake.Resources, widgetResourceList())
+
+	// (c) THE BUG: RESTMapping still misses because the cache is stale. The memcache now reports Fresh
+	// (the (a) lookup populated it), so the mapper's self-heal-on-miss path is disarmed and it keeps
+	// serving the pre-CRD delegate.
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); err == nil {
+		t.Fatal("expected STALE NoMatch after CRD registers but before invalidation, got nil error " +
+			"(cache unexpectedly fresh)")
+	} else if !meta.IsNoMatchError(err) {
+		t.Fatalf("expected STALE NoMatch after CRD registers but before invalidation, got: %v", err)
+	}
+
+	// (d) THE FIX MECHANISM: reset the deferred mapper. Reset() internally invalidates the underlying
+	// cached (memcache) discovery client AND drops the mapper's cached delegate, forcing a fresh
+	// discovery on the next request. This single call is exactly what the CRD informer invokes as its
+	// invalidator — and exactly what the cdc fails to wire.
+	mapper.Reset()
+
+	// (e) RESTMapping now resolves the freshly-registered Widget kind.
+	m, err := mapper.RESTMapping(widgetGroupKind, "v1")
+	if err != nil {
+		t.Fatalf("expected RESTMapping to succeed after invalidation+reset, got: %v", err)
+	}
+	if m == nil {
+		t.Fatal("expected non-nil RESTMapping after invalidation+reset")
+	}
+	if got := m.Resource.Resource; got != "widgets" {
+		t.Fatalf("expected resolved resource %q, got %q", "widgets", got)
+	}
+	if got := m.GroupVersionKind.Kind; got != "Widget" {
+		t.Fatalf("expected resolved kind %q, got %q", "Widget", got)
+	}
+}
+
+// TestDeferredMapper_StaleWhileFresh_HealsWhenNotFresh documents empirically WHY the staleness
+// happens and WHERE the invalidation must land: the DeferredDiscoveryRESTMapper only auto-re-discovers
+// on a miss when its underlying cached discovery client reports NOT fresh (see RESTMapping's
+// `!d.cl.Fresh()` guard). Once the memcache has been populated it reports Fresh, so repeated lookups
+// keep serving the stale delegate no matter how many times they are retried — this is the cdc bug.
+// Clearing the memcache's freshness bit (memcache.Invalidate(), which is exactly what mapper.Reset()
+// does under the hood, and what the CRD informer must ultimately trigger) makes the very next lookup
+// self-heal. mapper.Reset() and memcache.Invalidate() are therefore equivalent triggers here; doing
+// NEITHER is what leaves the mapper stale.
+func TestDeferredMapper_StaleWhileFresh_HealsWhenNotFresh(t *testing.T) {
+	fake := newFakeDiscovery(coreOnlyResources())
+	memcache := memory.NewMemCacheClient(fake)
+	mapper := restmapper.NewDeferredDiscoveryRESTMapper(memcache)
+
+	// Warm the mapper's delegate AND the memcache with a lookup that misses. After this, the memcache
+	// reports Fresh, so the auto-heal-on-miss path is disarmed.
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); !meta.IsNoMatchError(err) {
+		t.Fatalf("expected NoMatch on warm-up, got: %v", err)
+	}
+	if !memcache.Fresh() {
+		t.Fatal("expected memcache to report Fresh after warm-up lookup populated it")
+	}
+
+	// CRD registers.
+	fake.Resources = append(fake.Resources, widgetResourceList())
+
+	// Do NOTHING to invalidate. Repeated lookups keep missing because the memcache still reports Fresh,
+	// so the mapper never re-discovers — this is the persistent staleness the cdc exhibits.
+	for i := 0; i < 3; i++ {
+		_, err := mapper.RESTMapping(widgetGroupKind, "v1")
+		if err == nil {
+			t.Fatalf("attempt %d: expected persistent stale NoMatch with no invalidation, got success", i)
+		}
+		if !meta.IsNoMatchError(err) {
+			t.Fatalf("attempt %d: expected NoMatch, got: %v", i, err)
+		}
+	}
+	if !memcache.Fresh() {
+		t.Fatal("expected memcache to still report Fresh (nothing invalidated it)")
+	}
+
+	// Clear the freshness bit. On the next miss the mapper sees !Fresh(), auto-resets, and re-discovers.
+	memcache.Invalidate()
+	if memcache.Fresh() {
+		t.Fatal("expected memcache to report NOT fresh after Invalidate()")
+	}
+	if _, err := mapper.RESTMapping(widgetGroupKind, "v1"); err != nil {
+		t.Fatalf("expected RESTMapping to self-heal after Invalidate() cleared freshness, got: %v", err)
+	}
+}