From 5b295892fb5589b22703498355ab4e559277a61a Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Wed, 5 Aug 2026 08:23:48 +0200 Subject: [PATCH 1/2] test: cache-staleness coverage for helm/v3 + envtest CI Unit tests for the helm/v3 discovery-cache invalidation primitives: CRD informer edge cases (unchanged-spec => no Reset, changed-spec => Reset, nil invalidator/client no-op, rapid-add churn, ctx-cancel, logger message); the DeferredDiscoveryRESTMapper staleness proof (NoMatch until Invalidate+Reset) including the successful-then-stale case where a bare Invalidate is insufficient and only Reset recovers; WithCRDInformer wiring + idempotent Close; and the Install mapping-miss reset-then-retry contract. Plus a build-tagged (envtest) functional test that renders the installer's inst.crdExists mechanism against a real apiserver, registers a CRD mid-run, and proves the render picks it up without a client restart. Adds a Makefile (test / test-race / test-envtest via setup-envtest) and a test GitHub Actions workflow running both on PR/push to main. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LJsLqtryCgWwEt8FnPE1se --- .github/workflows/test.yaml | 32 +++ .gitignore | 3 + Makefile | 35 +++ helm/v3/cachedclients_stale_success_test.go | 94 +++++++ helm/v3/cachedclients_staleness_test.go | 192 +++++++++++++ helm/v3/crdexists_envtest_test.go | 227 +++++++++++++++ helm/v3/crdinformer_edge_test.go | 292 ++++++++++++++++++++ helm/v3/mappingmiss_retry_test.go | 189 +++++++++++++ helm/v3/withcrdinformer_test.go | 144 ++++++++++ 9 files changed, 1208 insertions(+) create mode 100644 .github/workflows/test.yaml create mode 100644 Makefile create mode 100644 helm/v3/cachedclients_stale_success_test.go create mode 100644 helm/v3/cachedclients_staleness_test.go create mode 100644 helm/v3/crdexists_envtest_test.go create mode 100644 helm/v3/crdinformer_edge_test.go create mode 100644 helm/v3/mappingmiss_retry_test.go create mode 100644 helm/v3/withcrdinformer_test.go diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml new file mode 100644 index 0000000..44132e4 --- /dev/null +++ b/.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 diff --git a/.gitignore b/.gitignore index c6bd34b..602cc13 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,6 @@ experiments/** **/token.txt **/*.crt **/*.key + +# make test-envtest installs setup-envtest here +/bin/ diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..788a686 --- /dev/null +++ b/Makefile @@ -0,0 +1,35 @@ +# 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) + GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest + +.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}' diff --git a/helm/v3/cachedclients_stale_success_test.go b/helm/v3/cachedclients_stale_success_test.go new file mode 100644 index 0000000..492a630 --- /dev/null +++ b/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) + } +} diff --git a/helm/v3/cachedclients_staleness_test.go b/helm/v3/cachedclients_staleness_test.go new file mode 100644 index 0000000..6410b3f --- /dev/null +++ b/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) + } +} diff --git a/helm/v3/crdexists_envtest_test.go b/helm/v3/crdexists_envtest_test.go new file mode 100644 index 0000000..7359398 --- /dev/null +++ b/helm/v3/crdexists_envtest_test.go @@ -0,0 +1,227 @@ +//go:build envtest + +// Functional (real-apiserver) test of the cache-staleness surface. Unlike the fake-discovery unit +// tests, this stands up a real kube-apiserver via controller-runtime/envtest, renders the ACTUAL +// installer gating mechanism (inst.crdExists = a helm `lookup` of every CustomResourceDefinition, +// ranged in-memory), registers a CRD MID-RUN, and re-renders on the SAME long-lived helm client to +// answer the load-bearing question empirically: does the render pick up a newly-registered CRD +// WITHOUT a process restart, and does helm.WithCRDInformer change that outcome? +// +// Run: KUBEBUILDER_ASSETS=$(setup-envtest use -p path 1.36.0) go test -tags envtest ./helm/v3/ -run Envtest -v +package helm + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "context" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" + + apixv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiextclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + helmconfig "github.com/krateo-platformops/plumbing/helm" +) + +func boolp(b bool) *bool { return &b } + +// widgetCRD is a composition.krateo.io/v1-0-0 Widget CRD — the shape inst.crdExists matches on +// (spec.group == composition.krateo.io, spec.names.kind, a served version "v1-0-0"). +func widgetCRD() *apixv1.CustomResourceDefinition { + return &apixv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "widgets.composition.krateo.io"}, + Spec: apixv1.CustomResourceDefinitionSpec{ + Group: "composition.krateo.io", + Names: apixv1.CustomResourceDefinitionNames{ + Plural: "widgets", + Singular: "widget", + Kind: "Widget", + ListKind: "WidgetList", + }, + Scope: apixv1.NamespaceScoped, + Versions: []apixv1.CustomResourceDefinitionVersion{ + { + Name: "v1-0-0", + Served: true, + Storage: true, + Schema: &apixv1.CustomResourceValidation{ + OpenAPIV3Schema: &apixv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apixv1.JSONSchemaProps{ + "spec": {Type: "object", XPreserveUnknownFields: boolp(true)}, + }, + }, + }, + }, + }, + }, + } +} + +// serveCRDProbeChart packages a tiny chart (a gzipped tar, served over HTTP at a .tgz URL — the form +// the plumbing getter accepts) whose single template reproduces the inst.crdExists logic: list every +// CustomResourceDefinition (a built-in GVK, so its own discovery is never stale) and range it looking +// for the composition.krateo.io Widget kind at served version v1-0-0. Returns the chart .tgz URL. +func serveCRDProbeChart(t *testing.T) string { + t.Helper() + files := []struct{ name, body string }{ + {"crdprobe/Chart.yaml", "apiVersion: v2\nname: crdprobe\nversion: 0.1.0\n"}, + {"crdprobe/templates/probe.yaml", `{{- $crds := (lookup "apiextensions.k8s.io/v1" "CustomResourceDefinition" "" "").items -}} +{{- $found := "" -}} +{{- range $crds -}} +{{- if and (eq .spec.group "composition.krateo.io") (eq .spec.names.kind "Widget") -}} +{{- range .spec.versions -}}{{- if and (eq .name "v1-0-0") .served -}}{{- $found = "true" -}}{{- end -}}{{- end -}} +{{- end -}} +{{- end -}} +apiVersion: v1 +kind: ConfigMap +metadata: + name: crdprobe-result +data: + widgetCRDSeen: "{{ $found }}" + crdCount: "{{ len $crds }}" +`}, + } + + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + for _, f := range files { + if err := tw.WriteHeader(&tar.Header{Name: f.name, Mode: 0o644, Size: int64(len(f.body))}); err != nil { + t.Fatalf("tar header %s: %v", f.name, err) + } + if _, err := tw.Write([]byte(f.body)); err != nil { + t.Fatalf("tar write %s: %v", f.name, err) + } + } + if err := tw.Close(); err != nil { + t.Fatalf("tar close: %v", err) + } + if err := gz.Close(); err != nil { + t.Fatalf("gzip close: %v", err) + } + tgz := buf.Bytes() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/gzip") + _, _ = w.Write(tgz) + })) + t.Cleanup(srv.Close) + return srv.URL + "/crdprobe-0.1.0.tgz" +} + +// widgetSeen extracts the widgetCRDSeen value from a rendered manifest ("" = CRD not seen). +func widgetSeen(manifest string) string { + for _, line := range strings.Split(manifest, "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "widgetCRDSeen:") { + return strings.Trim(strings.TrimSpace(strings.TrimPrefix(line, "widgetCRDSeen:")), `"`) + } + } + return "" +} + +// renderProbe runs a DryRunServer install (executes `lookup` against the live apiserver, persists +// nothing) on the given client and returns the rendered manifest. +func renderProbe(t *testing.T, hc *client, chartURL, releaseName string) string { + t.Helper() + rel, err := hc.Install(context.Background(), releaseName, chartURL, &helmconfig.InstallConfig{ + ActionConfig: &helmconfig.ActionConfig{DryRun: helmconfig.DryRunServer}, + Namespace: "default", + }) + if err != nil { + t.Fatalf("render (%s): %v", releaseName, err) + } + return rel.GetManifest() +} + +func waitEstablished(t *testing.T, ac apiextclient.Interface, name string) { + t.Helper() + deadline := time.Now().Add(30 * time.Second) + for time.Now().Before(deadline) { + crd, err := ac.ApiextensionsV1().CustomResourceDefinitions().Get(context.Background(), name, metav1.GetOptions{}) + if err == nil { + for _, c := range crd.Status.Conditions { + if c.Type == apixv1.Established && c.Status == apixv1.ConditionTrue { + return + } + } + } + time.Sleep(300 * time.Millisecond) + } + t.Fatalf("CRD %s never became Established", name) +} + +// TestEnvtest_CRDExistsRender_PicksUpMidRunCRD is the diagnostic: it registers the Widget CRD while a +// long-lived helm client is alive and asserts the SAME client's next crdExists render sees it — no +// restart. It runs the assertion for BOTH the cdc's current construction (WithCache only) and the +// proposed fix (WithCRDInformer), logging the empirical outcome of each so the fix's necessity/location +// is grounded in observed behavior, not assumption. +func TestEnvtest_CRDExistsRender_PicksUpMidRunCRD(t *testing.T) { + if os.Getenv("KUBEBUILDER_ASSETS") == "" { + t.Skip("KUBEBUILDER_ASSETS not set; run via setup-envtest") + } + env := &envtest.Environment{} + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest apiserver: %v", err) + } + defer func() { _ = env.Stop() }() + + ac, err := apiextclient.NewForConfig(cfg) + if err != nil { + t.Fatalf("apiextensions client: %v", err) + } + chartURL := serveCRDProbeChart(t) + + // ---- cdc-current construction: WithCache only (cachedClients stays nil) ---- + cur, err := NewClient(cfg, WithNamespace("default"), WithCache()) + if err != nil { + t.Fatalf("new WithCache client: %v", err) + } + defer func() { _ = cur.Close() }() + t.Logf("cdc-current client cachedClients==nil: %v", cur.cachedClients == nil) + + if got := widgetSeen(renderProbe(t, cur, chartURL, "probe-cur-1")); got != "" { + t.Fatalf("precondition: Widget CRD should be absent, render reported widgetCRDSeen=%q", got) + } + + // Register the CRD MID-RUN — no client restart, no Close. + if _, err := ac.ApiextensionsV1().CustomResourceDefinitions().Create(context.Background(), widgetCRD(), metav1.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) { + t.Fatalf("create Widget CRD: %v", err) + } + waitEstablished(t, ac, "widgets.composition.krateo.io") + + seenCur := widgetSeen(renderProbe(t, cur, chartURL, "probe-cur-2")) + t.Logf("EMPIRICAL [WithCache only]: same client crdExists render after mid-run CRD => widgetCRDSeen=%q", seenCur) + + // ---- cdc-fixed construction: WithCRDInformer (shared cachedClients + informer invalidation) ---- + fix, err := NewClient(cfg, WithNamespace("default"), WithCache(), WithCRDInformer(cfg, time.Minute)) + if err != nil { + t.Fatalf("new WithCRDInformer client: %v", err) + } + defer func() { _ = fix.Close() }() + t.Logf("cdc-fixed client cachedClients==nil: %v", fix.cachedClients == nil) + + seenFix := widgetSeen(renderProbe(t, fix, chartURL, "probe-fix-1")) + t.Logf("EMPIRICAL [WithCRDInformer]: crdExists render (CRD already present) => widgetCRDSeen=%q", seenFix) + + // The load-bearing claim the user asked to verify: crdExists sees a mid-run CRD without a restart. + // The crdExists path lists a BUILT-IN GVK (CustomResourceDefinition), so both constructions are + // expected to see it; this assertion pins that and will surface it loudly if reality differs. + if seenCur != "true" { + t.Errorf("REGRESSION/DIAGNOSTIC: WithCache-only client did NOT see the mid-run CRD in crdExists render "+ + "(widgetCRDSeen=%q); if this fails, crdExists IS stale at the helm layer", seenCur) + } + if seenFix != "true" { + t.Errorf("WithCRDInformer client did not see the present CRD (widgetCRDSeen=%q)", seenFix) + } +} diff --git a/helm/v3/crdinformer_edge_test.go b/helm/v3/crdinformer_edge_test.go new file mode 100644 index 0000000..b25a869 --- /dev/null +++ b/helm/v3/crdinformer_edge_test.go @@ -0,0 +1,292 @@ +package helm + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + apixv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + fakeapix "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// newEdgeTestCRD builds a minimal, valid CRD used across the edge-case tests below. +func newEdgeTestCRD(name, group, plural, kind string) *apixv1.CustomResourceDefinition { + return &apixv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: apixv1.CustomResourceDefinitionSpec{ + Group: group, + Names: apixv1.CustomResourceDefinitionNames{ + Plural: plural, + Singular: strings.TrimSuffix(plural, "s"), + Kind: kind, + }, + Scope: apixv1.NamespaceScoped, + Versions: []apixv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + }, + }, + }, + } +} + +// TestCRDInformer_UpdateUnchangedSpec_NoReset asserts that an Update event whose Spec is +// reflect.DeepEqual to the prior object does NOT invalidate the discovery cache. The informer's +// UpdateFunc short-circuits on an unchanged Spec, so invalidator.Reset must not be called even +// though a Modified event is delivered (RV bumped so the reflector actually delivers it). +func TestCRDInformer_UpdateUnchangedSpec_NoReset(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + fakeClient := fakeapix.NewSimpleClientset() + invalidator := newFakeInvalidator() + + inf := NewCRDInformer(1*time.Minute, fakeClient, invalidator, func(format string, v ...interface{}) {}) + if err := inf.Start(ctx); err != nil { + t.Fatalf("start informer: %v", err) + } + // Let the informer's initial List complete and its Watch establish before mutating; otherwise the + // Create/Update race watch setup and events are missed (mirrors the 500ms note in the existing test). + time.Sleep(500 * time.Millisecond) + + crd := newEdgeTestCRD("nochange.example.com", "example.com", "nochanges", "NoChange") + created, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("create crd: %v", err) + } + // Add fires => drain the one expected Reset. + if !invalidator.WaitCall(2 * time.Second) { + t.Fatalf("invalidate not called on CRD add") + } + + // Update the object but keep the Spec byte-identical; only bump RV (and a metadata-only change) so + // the reflector delivers a Modified event whose Spec is DeepEqual to the old one. + updated := created.DeepCopy() + updated.ResourceVersion = "2" + if updated.Labels == nil { + updated.Labels = map[string]string{} + } + updated.Labels["touched"] = "true" + if _, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Update(ctx, updated, metav1.UpdateOptions{}); err != nil { + t.Fatalf("update crd: %v", err) + } + + // The UpdateFunc must NOT call Reset for an unchanged Spec. + if invalidator.WaitCall(1 * time.Second) { + t.Fatalf("invalidate WAS called on an unchanged-Spec update; expected no Reset") + } +} + +// TestCRDInformer_UpdateChangedSpec_Reset asserts the complementary case: an Update whose Spec +// actually differs DOES invalidate the cache. +func TestCRDInformer_UpdateChangedSpec_Reset(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + fakeClient := fakeapix.NewSimpleClientset() + invalidator := newFakeInvalidator() + + inf := NewCRDInformer(1*time.Minute, fakeClient, invalidator, func(format string, v ...interface{}) {}) + if err := inf.Start(ctx); err != nil { + t.Fatalf("start informer: %v", err) + } + time.Sleep(500 * time.Millisecond) + + crd := newEdgeTestCRD("changed.example.com", "example.com", "changeds", "Changed") + created, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("create crd: %v", err) + } + if !invalidator.WaitCall(2 * time.Second) { + t.Fatalf("invalidate not called on CRD add") + } + + // Genuinely change the Spec. + updated := created.DeepCopy() + updated.ResourceVersion = "2" + updated.Spec.PreserveUnknownFields = true + if _, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Update(ctx, updated, metav1.UpdateOptions{}); err != nil { + t.Fatalf("update crd: %v", err) + } + + if !invalidator.WaitCall(2 * time.Second) { + t.Fatalf("invalidate not called on a changed-Spec update; expected Reset") + } +} + +// TestCRDInformer_NilInvalidator_NoPanic ensures a nil invalidator is tolerated: Start succeeds and +// CRD events flow through the handlers without panicking (every handler guards `c.invalidator != nil`). +func TestCRDInformer_NilInvalidator_NoPanic(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + fakeClient := fakeapix.NewSimpleClientset() + + inf := NewCRDInformer(1*time.Minute, fakeClient, nil, func(format string, v ...interface{}) {}) + if err := inf.Start(ctx); err != nil { + t.Fatalf("start informer with nil invalidator: %v", err) + } + time.Sleep(500 * time.Millisecond) + + crd := newEdgeTestCRD("nilinv.example.com", "example.com", "nilinvs", "NilInv") + if _, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{}); err != nil { + t.Fatalf("create crd: %v", err) + } + if err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Delete(ctx, crd.Name, metav1.DeleteOptions{}); err != nil { + t.Fatalf("delete crd: %v", err) + } + // Give the handlers time to run; a panic in an informer goroutine would crash the test binary. + time.Sleep(500 * time.Millisecond) +} + +// TestCRDInformer_NilApiExtCli_NoOp asserts Start returns nil and does nothing when apiExtCli is nil. +func TestCRDInformer_NilApiExtCli_NoOp(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + invalidator := newFakeInvalidator() + inf := NewCRDInformer(1*time.Minute, nil, invalidator, func(format string, v ...interface{}) {}) + + if err := inf.Start(ctx); err != nil { + t.Fatalf("expected nil error for nil apiExtCli, got: %v", err) + } + // No client => no informer => no events => Reset must never fire. + if invalidator.WaitCall(500 * time.Millisecond) { + t.Fatalf("invalidate called despite nil apiExtCli; Start should be a no-op") + } +} + +// TestCRDInformer_ManyRapidAdds_ResetPerEvent creates several CRDs in quick succession and asserts +// Reset fires for each Add (bounded per-event wait). This guards the crdExists-after-register path +// that the cdc must honor for every newly registered CRD, not just the first. +func TestCRDInformer_ManyRapidAdds_ResetPerEvent(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + fakeClient := fakeapix.NewSimpleClientset() + invalidator := newFakeInvalidator() + + inf := NewCRDInformer(1*time.Minute, fakeClient, invalidator, func(format string, v ...interface{}) {}) + if err := inf.Start(ctx); err != nil { + t.Fatalf("start informer: %v", err) + } + time.Sleep(500 * time.Millisecond) + + const n = 5 + for i := 0; i < n; i++ { + suffix := string(rune('a' + i)) + crd := newEdgeTestCRD( + "rapid"+suffix+".example.com", + "example.com", + "rapid"+suffix+"s", + "Rapid"+strings.ToUpper(suffix), + ) + if _, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{}); err != nil { + t.Fatalf("create crd %d: %v", i, err) + } + } + + for i := 0; i < n; i++ { + if !invalidator.WaitCall(2 * time.Second) { + t.Fatalf("invalidate not called for rapid add #%d (expected one Reset per Add)", i) + } + } +} + +// TestCRDInformer_ContextCancel_StopsCleanly cancels the context and asserts the informer stops +// without panicking. After cancellation no further Reset should be delivered for new events. +func TestCRDInformer_ContextCancel_StopsCleanly(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + + fakeClient := fakeapix.NewSimpleClientset() + invalidator := newFakeInvalidator() + + inf := NewCRDInformer(1*time.Minute, fakeClient, invalidator, func(format string, v ...interface{}) {}) + if err := inf.Start(ctx); err != nil { + t.Fatalf("start informer: %v", err) + } + time.Sleep(500 * time.Millisecond) + + // Stop the informer. + cancel() + // Allow the factory's Start(ctx.Done()) goroutines to unwind. + time.Sleep(500 * time.Millisecond) + + // Creating a CRD after cancellation must not trigger Reset (informer is stopped) and must not panic. + crd := newEdgeTestCRD("afterstop.example.com", "example.com", "afterstops", "AfterStop") + if _, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Create(context.Background(), crd, metav1.CreateOptions{}); err != nil { + t.Fatalf("create crd after cancel: %v", err) + } + if invalidator.WaitCall(1 * time.Second) { + t.Fatalf("invalidate called after context cancel; informer should be stopped") + } +} + +// TestCRDInformer_LoggerReceivesMessage_OnAddAndDelete captures the logger callback and asserts it +// receives non-empty messages on Add and Delete events. +func TestCRDInformer_LoggerReceivesMessage_OnAddAndDelete(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + fakeClient := fakeapix.NewSimpleClientset() + invalidator := newFakeInvalidator() + + var mu sync.Mutex + var messages []string + logger := func(format string, v ...interface{}) { + mu.Lock() + defer mu.Unlock() + messages = append(messages, format) + } + + inf := NewCRDInformer(1*time.Minute, fakeClient, invalidator, logger) + if err := inf.Start(ctx); err != nil { + t.Fatalf("start informer: %v", err) + } + time.Sleep(500 * time.Millisecond) + + crd := newEdgeTestCRD("logged.example.com", "example.com", "loggeds", "Logged") + if _, err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Create(ctx, crd, metav1.CreateOptions{}); err != nil { + t.Fatalf("create crd: %v", err) + } + if !invalidator.WaitCall(2 * time.Second) { + t.Fatalf("invalidate not called on CRD add") + } + if err := fakeClient.ApiextensionsV1().CustomResourceDefinitions().Delete(ctx, crd.Name, metav1.DeleteOptions{}); err != nil { + t.Fatalf("delete crd: %v", err) + } + if !invalidator.WaitCall(2 * time.Second) { + t.Fatalf("invalidate not called on CRD delete") + } + // Let the logger callbacks (invoked from the handler after Reset) run. + time.Sleep(200 * time.Millisecond) + + mu.Lock() + defer mu.Unlock() + sawAdd := false + sawDelete := false + for _, m := range messages { + if m == "" { + t.Fatalf("logger received an empty message") + } + if strings.Contains(m, "added") { + sawAdd = true + } + if strings.Contains(m, "deleted") { + sawDelete = true + } + } + if !sawAdd { + t.Fatalf("logger did not receive an add message; got %v", messages) + } + if !sawDelete { + t.Fatalf("logger did not receive a delete message; got %v", messages) + } +} diff --git a/helm/v3/mappingmiss_retry_test.go b/helm/v3/mappingmiss_retry_test.go new file mode 100644 index 0000000..3a70acf --- /dev/null +++ b/helm/v3/mappingmiss_retry_test.go @@ -0,0 +1,189 @@ +package helm + +import ( + "errors" + "fmt" + "testing" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + discoveryfake "k8s.io/client-go/discovery/fake" + memory "k8s.io/client-go/discovery/cached/memory" + "k8s.io/client-go/restmapper" + clientgotesting "k8s.io/client-go/testing" +) + +// TestIsRESTMappingMiss_Variants complements TestIsRESTMappingMiss in mappingmiss_test.go. +// That test already covers: nil, an unrelated error, helm's full stringified render error, the +// bare "resource mapping not found" substring, and a %w-wrapped typed *meta.NoKindMatchError. +// Here we pin down the REMAINING branches of isRESTMappingMiss (read in client.go) so every +// variant it claims to match is exercised exactly once and false-positives stay excluded: +// - meta.IsNoMatchError on a DIRECT (unwrapped) typed error, +// - the "no matches for kind" substring on its own, +// - the "ensure CRDs are installed first" substring on its own, +// - two more unrelated errors (a wrapped one, and one whose text merely resembles but does +// not contain any sentinel substring) that must NOT trip the matcher. +func TestIsRESTMappingMiss_Variants(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + { + name: "direct typed NoKindMatchError (unwrapped, via meta.IsNoMatchError)", + err: &meta.NoKindMatchError{ + GroupKind: schema.GroupKind{Group: "composition.krateo.io", Kind: "SnowplowCrds"}, + SearchedVersions: []string{"v1-9-0"}, + }, + want: true, + }, + { + name: "no matches for kind substring standalone", + err: errors.New(`no matches for kind "Portal" in version "widgets.templates.krateo.io/v1beta1"`), + want: true, + }, + { + name: "ensure CRDs are installed first substring standalone", + err: errors.New("could not build manifest: ensure CRDs are installed first"), + want: true, + }, + { + name: "no matches for kind wrapped with %w", + err: fmt.Errorf("render step: %w", errors.New(`no matches for kind "FooBar"`)), + want: true, + }, + { + name: "unrelated wrapped error", + err: fmt.Errorf("load chart: %w", errors.New("dial tcp: i/o timeout")), + want: false, + }, + { + name: "resembling but non-matching text", + err: errors.New("mapping the values file failed to parse"), + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isRESTMappingMiss(tc.err); got != tc.want { + t.Fatalf("isRESTMappingMiss(%q) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// newMappingMissMapper builds the exact discovery/mapper stack the production client wires in +// CachedClients (client_getter.go NewCachedClients): a mutable fake discovery client wrapped by +// a MemCacheClient, wrapped by a DeferredDiscoveryRESTMapper. The returned *FakeDiscovery lets a +// test register a new APIResource at runtime to simulate a CRD that landed AFTER the mapper first +// populated its cache. It returns the concrete *DeferredDiscoveryRESTMapper because that is the +// SAME concrete type held by CachedClients.mapper on which client.Install calls Reset(). +func newMappingMissMapper(seed ...*metav1.APIResourceList) (*restmapper.DeferredDiscoveryRESTMapper, *discoveryfake.FakeDiscovery) { + fd := &discoveryfake.FakeDiscovery{Fake: &clientgotesting.Fake{}} + fd.Resources = append(fd.Resources, seed...) + memCache := memory.NewMemCacheClient(fd) + mapper := restmapper.NewDeferredDiscoveryRESTMapper(memCache) + return mapper, fd +} + +// TestMapperResetPicksUpNewlyRegisteredKind proves the reset-then-retry CONTRACT that +// client.Install depends on. In client.go, Install on an isRESTMappingMiss error does exactly: +// +// c.cachedClients.mapper.Reset() +// // re-init the action config and run install() a second time +// +// The load-bearing claim is that mapper.Reset() causes the very next mapping attempt to observe a +// kind that was NOT resolvable before (the stale-discovery-cache bug: a composition CRD created +// moments earlier in the same bootstrap is invisible until the cache is dropped). We drive the +// SAME concrete *DeferredDiscoveryRESTMapper the client holds: +// +// attempt 1 (pre-registration) -> RESTMapping MUST miss with a NoKindMatchError +// (this is precisely what isRESTMappingMiss classifies true) +// register the CRD's resource -> simulate the CRD add the informer would have raced +// Reset() -> the single invalidation Install performs before its retry +// attempt 2 (post-Reset) -> RESTMapping MUST now succeed +// +// Without the Reset() between the two attempts the MemCacheClient keeps serving its first snapshot +// and attempt 2 would miss identically — so this asserts the reset is causally required, which is +// the whole point of the fix the cdc must wire. +// +// What an envtest/integration test would ADD (and this unit deliberately does not attempt, because +// client.Install requires a real chart render + apiserver and there is no unexported seam to +// intercept the two install() calls — see client.go, the mapper field is a concrete type, not an +// injectable interface): that Install invokes Reset EXACTLY ONCE and retries EXACTLY ONCE end to +// end. That behavioral count is covered by the integration suite in client_test.go (build tag +// integration). Here we prove the mechanism the retry relies on. +func TestMapperResetPicksUpNewlyRegisteredKind(t *testing.T) { + gk := schema.GroupKind{Group: "composition.krateo.io", Kind: "SnowplowCrds"} + const version = "v1-9-0" + + // Seed one unrelated, always-present group (core "v1"). The MemCacheClient treats a discovery + // surface with ZERO groups as an error and forces the DeferredDiscoveryRESTMapper to reload on + // every call (a noisy retry storm); a real cluster always exposes core/v1, so seeding it makes + // attempt 1 a CLEAN NoKindMatchError for our target kind rather than a discovery failure — which + // is exactly the stale-cache condition we want to model (discovery works, our new kind just is + // not in it yet). + baseGroup := &metav1.APIResourceList{ + GroupVersion: "v1", + APIResources: []metav1.APIResource{ + {Name: "pods", Namespaced: true, Kind: "Pod"}, + }, + } + + // Start without the target kind: it is unknown, exactly like a CRD that has not yet been + // observed by the discovery cache. + mapper, fd := newMappingMissMapper(baseGroup) + + // Attempt 1: must miss, and must miss in the way isRESTMappingMiss classifies as a mapping miss. + _, err := mapper.RESTMapping(gk, version) + if err == nil { + t.Fatalf("attempt 1: expected a mapping miss for %s/%s before registration, got nil", gk, version) + } + if !isRESTMappingMiss(err) { + t.Fatalf("attempt 1: error should be classified as a REST mapping miss, got %T: %v", err, err) + } + + // Simulate the CRD landing: register its served resource so discovery now knows the kind + // (keeping the pre-existing base group, as a real cluster would). + fd.Resources = []*metav1.APIResourceList{ + baseGroup, + { + GroupVersion: gk.Group + "/" + version, + APIResources: []metav1.APIResource{ + { + Name: "snowplowcrds", + Namespaced: true, + Kind: gk.Kind, + }, + }, + }, + } + + // Control check: WITHOUT a Reset the memcache still serves its first (empty) snapshot, so the + // mapping is still a miss. This pins that Reset is causally required, not incidental. + if _, err := mapper.RESTMapping(gk, version); err == nil { + t.Fatalf("control: mapping unexpectedly succeeded before Reset — the memcache should still be stale") + } else if !isRESTMappingMiss(err) { + t.Fatalf("control: pre-Reset error should still be a mapping miss, got %T: %v", err, err) + } + + // This is the single line client.Install performs before retrying install(). + mapper.Reset() + + // Attempt 2 (the retry): the newly registered kind must now resolve. + mapping, err := mapper.RESTMapping(gk, version) + if err != nil { + t.Fatalf("attempt 2 (post-Reset): expected %s/%s to resolve after Reset, got error: %v", gk, version, err) + } + if mapping == nil { + t.Fatalf("attempt 2 (post-Reset): expected a non-nil RESTMapping") + } + if mapping.GroupVersionKind.Kind != gk.Kind { + t.Fatalf("attempt 2 (post-Reset): resolved kind = %q, want %q", mapping.GroupVersionKind.Kind, gk.Kind) + } + if mapping.Resource.Resource != "snowplowcrds" { + t.Fatalf("attempt 2 (post-Reset): resolved resource = %q, want %q", mapping.Resource.Resource, "snowplowcrds") + } +} diff --git a/helm/v3/withcrdinformer_test.go b/helm/v3/withcrdinformer_test.go new file mode 100644 index 0000000..2686c30 --- /dev/null +++ b/helm/v3/withcrdinformer_test.go @@ -0,0 +1,144 @@ +package helm + +import ( + "testing" + "time" + + "k8s.io/client-go/rest" +) + +// testRESTConfig returns the smallest *rest.Config that lets NewClient (and its construction-time +// probe.Init) succeed WITHOUT dialing the apiserver. The Host is a loopback address that is never +// contacted at construction: action.Configuration.Init and WithCRDInformer build their clients +// lazily (the CRD informer's List/Watch runs asynchronously in factory.Start and does not block or +// fail construction). This keeps the file to construction/wiring assertions; the live +// CRD-add -> mapper.Reset() path is covered by crdinformer_test.go +// (TestStartCRDInformer_Add_Update_Delete_InvokeInvalidate). +func testRESTConfig() *rest.Config { + return &rest.Config{Host: "https://127.0.0.1:1"} +} + +// TestWithCRDInformerSetsCachedClients asserts requirement (1): NewClient with WithCRDInformer wires +// the discovery/mapper cache (c.cachedClients) that the CRD informer resets, and does not error with a +// minimal rest.Config. +func TestWithCRDInformerSetsCachedClients(t *testing.T) { + cfg := testRESTConfig() + + c, err := NewClient(cfg, WithCRDInformer(cfg, 1*time.Minute)) + if err != nil { + t.Fatalf("NewClient with WithCRDInformer: unexpected error: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + + if c.cachedClients == nil { + t.Fatalf("expected cachedClients to be set by WithCRDInformer, got nil") + } + if c.cachedClients.mapper == nil { + t.Fatalf("expected cachedClients.mapper to be non-nil (informer resets it on CRD change)") + } + if c.cachedClients.discoveryClient == nil { + t.Fatalf("expected cachedClients.discoveryClient to be non-nil") + } + if c.crdInformerCancel == nil { + t.Fatalf("expected crdInformerCancel to be set so Close() can stop the informer") + } +} + +// TestNewClientWithoutCRDInformerLeavesCachedClientsNil asserts requirement (2): this documents the +// current cdc gap — without WithCRDInformer there is no CRD informer and no shared cache to reset, so a +// stale discovery/RESTMapper wedges the umbrella render (crdExists misses a freshly-registered CRD). +func TestNewClientWithoutCRDInformerLeavesCachedClientsNil(t *testing.T) { + cfg := testRESTConfig() + + c, err := NewClient(cfg) + if err != nil { + t.Fatalf("NewClient: unexpected error: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + + if c.cachedClients != nil { + t.Fatalf("expected cachedClients to be nil without WithCRDInformer (the cdc gap), got %#v", c.cachedClients) + } + if c.crdInformerCancel != nil { + t.Fatalf("expected crdInformerCancel to be nil without WithCRDInformer, got non-nil") + } +} + +// TestWithCacheAndCRDInformerBothSucceed asserts requirement (3): the two options compose on one client +// — the disk cache and the shared CRD-informer cache are independent and both get wired. +func TestWithCacheAndCRDInformerBothSucceed(t *testing.T) { + cfg := testRESTConfig() + + c, err := NewClient(cfg, + WithCache(), + WithCRDInformer(cfg, 1*time.Minute), + ) + if err != nil { + t.Fatalf("NewClient with WithCache + WithCRDInformer: unexpected error: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + + if c.cache == nil { + t.Fatalf("expected disk cache to be set by WithCache, got nil") + } + if c.cachedClients == nil { + t.Fatalf("expected cachedClients to be set by WithCRDInformer, got nil") + } +} + +// TestWithCRDInformerBeforeCacheOrderIndependent asserts requirement (3), option order swapped: the +// functional options must not depend on application order. +func TestWithCRDInformerBeforeCacheOrderIndependent(t *testing.T) { + cfg := testRESTConfig() + + c, err := NewClient(cfg, + WithCRDInformer(cfg, 30*time.Second), + WithCache(), + ) + if err != nil { + t.Fatalf("NewClient with WithCRDInformer + WithCache: unexpected error: %v", err) + } + t.Cleanup(func() { _ = c.Close() }) + + if c.cache == nil { + t.Fatalf("expected disk cache to be set by WithCache, got nil") + } + if c.cachedClients == nil { + t.Fatalf("expected cachedClients to be set by WithCRDInformer, got nil") + } +} + +// TestCloseCancelsInformerAndIsIdempotent asserts requirement (4): Close() cancels the informer context +// (crdInformerCancel) without panicking and is safe to call more than once. +func TestCloseCancelsInformerAndIsIdempotent(t *testing.T) { + cfg := testRESTConfig() + + c, err := NewClient(cfg, WithCRDInformer(cfg, 1*time.Minute)) + if err != nil { + t.Fatalf("NewClient with WithCRDInformer: unexpected error: %v", err) + } + + if err := c.Close(); err != nil { + t.Fatalf("first Close: unexpected error: %v", err) + } + // Calling Close again must not panic and must not error (crdInformerCancel is idempotent, disk + // cache is nil here). + if err := c.Close(); err != nil { + t.Fatalf("second Close: unexpected error: %v", err) + } +} + +// TestCloseWithoutInformerNoPanic asserts requirement (4) for the no-informer path: Close() on a client +// built without WithCRDInformer (crdInformerCancel == nil) must not panic. +func TestCloseWithoutInformerNoPanic(t *testing.T) { + cfg := testRESTConfig() + + c, err := NewClient(cfg) + if err != nil { + t.Fatalf("NewClient: unexpected error: %v", err) + } + + if err := c.Close(); err != nil { + t.Fatalf("Close without informer: unexpected error: %v", err) + } +} From 61442f9f502bafb8760670b3f5c7b5cad8ed3a07 Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Wed, 5 Aug 2026 19:57:45 +0200 Subject: [PATCH 2/2] fix(ci): pin setup-envtest to release-0.22 (go 1.25 compatible) `make test-envtest` installed setup-envtest@latest, which is now v0.24.x and requires go >= 1.26; under this module's go 1.25 toolchain (GOTOOLCHAIN=local) it failed the envtest CI job with "requires go >= 1.26.0". Pin to @release-0.22, matching sigs.k8s.io/controller-runtime v0.22.3 in go.mod. Verified: installs cleanly under go 1.25. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LJsLqtryCgWwEt8FnPE1se --- Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 788a686..19dc2ab 100644 --- a/Makefile +++ b/Makefile @@ -22,7 +22,10 @@ $(LOCALBIN): .PHONY: setup-envtest setup-envtest: $(SETUP_ENVTEST) ## Install setup-envtest into ./bin. $(SETUP_ENVTEST): | $(LOCALBIN) - GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest + # 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.