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 5b239b6..c7c0639 100755 --- a/.gitignore +++ b/.gitignore @@ -54,4 +54,6 @@ cover.out .github/skills -.vscode \ No newline at end of file +.vscode +# 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/internal/tools/dynamic/crdwatch_envtest_test.go b/internal/tools/dynamic/crdwatch_envtest_test.go new file mode 100644 index 0000000..eccd43a --- /dev/null +++ b/internal/tools/dynamic/crdwatch_envtest_test.go @@ -0,0 +1,180 @@ +//go:build envtest + +// Functional (real-apiserver) test of core-provider's ACTUAL cache-staleness mechanism: +// WatchCRDsAndInvalidate keeping the long-lived DeferredDiscoveryRESTMapper (NewRESTMapper) fresh. +// +// This is the honest reproduction of the "umbrella stops emitting Compositions until the cdc is +// restarted" symptom and its fix. It uses the REAL production code paths (NewRESTMapper + +// WatchCRDsAndInvalidate), registers a CRD MID-RUN against a live kube-apiserver, and proves: +// - WATCHED mapper: after the CRD is created, the mapper resolves the new kind on its own within the +// coalescing window — NO process restart (the fix works end-to-end). +// - UNWATCHED mapper (negative control): the same mid-run CRD stays invisible (stale) until an +// explicit Reset() — i.e. exactly what a controller restart used to do by hand. +// +// Run: KUBEBUILDER_ASSETS=$(setup-envtest use -p path 1.36.0) \ +// go test -tags envtest ./internal/tools/dynamic/ -run Envtest -v +package dynamic + +import ( + "context" + "os" + "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" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +func crdwatchBoolPtr(b bool) *bool { return &b } + +// mkCompositionCRD builds a composition.krateo.io/v1-0-0 CRD (the shape the umbrella's inst.crdExists +// gate and the cdc's GVKtoGVR/IsNamespaced both depend on resolving). +func mkCompositionCRD(plural, kind string) *apixv1.CustomResourceDefinition { + return &apixv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: plural + ".composition.krateo.io"}, + Spec: apixv1.CustomResourceDefinitionSpec{ + Group: "composition.krateo.io", + Names: apixv1.CustomResourceDefinitionNames{ + Plural: plural, + Singular: plural[:len(plural)-1], + Kind: kind, + ListKind: kind + "List", + }, + 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: crdwatchBoolPtr(true)}, + }, + }, + }, + }, + }, + }, + } +} + +func gkResolves(mapper meta.RESTMapper, gk schema.GroupKind) bool { + _, err := mapper.RESTMapping(gk, "v1-0-0") + return err == nil +} + +func pollResolves(mapper meta.RESTMapper, gk schema.GroupKind, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for { + if gkResolves(mapper, gk) { + return true + } + if time.Now().After(deadline) { + return false + } + time.Sleep(150 * time.Millisecond) + } +} + +func waitCRDEstablished(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_WatchCRDsAndInvalidate_Functional is the gold-standard proof. One apiserver, two REAL +// production mappers over it: one guarded by WatchCRDsAndInvalidate (the fix), one bare (the pre-fix +// behavior). Both are warmed before the CRD exists, then a single mid-run CRD create is observed +// against both — the watched mapper self-heals without a restart; the unwatched one stays stale until +// an explicit Reset (what a restart did by hand). +func TestEnvtest_WatchCRDsAndInvalidate_Functional(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) + } + + // REAL production mappers (DeferredDiscoveryRESTMapper over a MemCache discovery client). + watched, err := NewRESTMapper(cfg) + if err != nil { + t.Fatalf("NewRESTMapper (watched): %v", err) + } + unwatched, err := NewRESTMapper(cfg) + if err != nil { + t.Fatalf("NewRESTMapper (unwatched): %v", err) + } + + // Guard only `watched` with the REAL watch. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if err := WatchCRDsAndInvalidate(ctx, cfg, watched, nil); err != nil { + t.Fatalf("WatchCRDsAndInvalidate: %v", err) + } + // Let the CRD informer establish its initial list/watch before we mutate. + time.Sleep(1 * time.Second) + + gk := schema.GroupKind{Group: "composition.krateo.io", Kind: "Widget"} + + // Warm BOTH mappers' discovery caches while the kind is absent (populates the stale delegate). + if gkResolves(watched, gk) || gkResolves(unwatched, gk) { + t.Fatalf("precondition: Widget must not resolve before its CRD exists") + } + + // Register the CRD MID-RUN — no restart of anything. + if _, err := ac.ApiextensionsV1().CustomResourceDefinitions().Create(context.Background(), mkCompositionCRD("widgets", "Widget"), metav1.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) { + t.Fatalf("create Widget CRD: %v", err) + } + waitCRDEstablished(t, ac, "widgets.composition.krateo.io") + + // FIX: the watched mapper must resolve the new kind on its own, within the 2s coalescing window + // (plus generous margin for informer delivery) — WITHOUT a restart. + if !pollResolves(watched, gk, 12*time.Second) { + t.Fatalf("WATCHED mapper never picked up the mid-run CRD; WatchCRDsAndInvalidate did NOT refresh discovery") + } + t.Logf("WATCHED mapper resolved Widget after a mid-run CRD create with NO restart (fix works)") + + // NEGATIVE CONTROL: the unwatched mapper is still stale (its MemCache delegate was warmed empty and + // nothing invalidated it). This reproduces the original "restart required" symptom. + if gkResolves(unwatched, gk) { + t.Fatalf("UNWATCHED mapper unexpectedly self-refreshed; the negative control is invalid") + } + t.Logf("UNWATCHED mapper is still stale (Widget unresolved) — reproduces the pre-fix 'restart required' behavior") + + // A manual Reset (what a process restart effectively forced) recovers the unwatched mapper. + resetter, ok := unwatched.(interface{ Reset() }) + if !ok { + t.Fatalf("production mapper does not implement Reset(); WatchCRDsAndInvalidate would be a no-op") + } + resetter.Reset() + if !pollResolves(unwatched, gk, 8*time.Second) { + t.Fatalf("after an explicit Reset the unwatched mapper still cannot resolve Widget") + } + t.Logf("UNWATCHED mapper resolved Widget only after an explicit Reset() (the manual-restart equivalent)") +} diff --git a/internal/tools/dynamic/crdwatch_test.go b/internal/tools/dynamic/crdwatch_test.go index 243f083..402a78b 100644 --- a/internal/tools/dynamic/crdwatch_test.go +++ b/internal/tools/dynamic/crdwatch_test.go @@ -5,6 +5,7 @@ import ( "testing" "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/client-go/rest" ) // A RESTMapper without a Reset() method must make WatchCRDsAndInvalidate a no-op that @@ -20,3 +21,23 @@ func TestWatchCRDsAndInvalidate_NoResetIsNoop(t *testing.T) { t.Fatalf("WatchCRDsAndInvalidate() with a non-resettable mapper = %v, want nil", err) } } + +// A resettable mapper takes the ACTIVE path: WatchCRDsAndInvalidate builds a dynamic client from the +// config and starts the CRD watch, returning nil once setup succeeds. A dummy Host constructs the +// client without dialing; the informer then never syncs, which the watch handles gracefully (it logs +// and degrades rather than erroring). The async add->Reset behaviour itself is proven end-to-end +// against a real apiserver in crdwatch_envtest_test.go (build tag: envtest). +func TestWatchCRDsAndInvalidate_ResettableMapperSetupOK(t *testing.T) { + mock := &MockResettableMapper{MockRESTMapper: &MockRESTMapper{scopeName: meta.RESTScopeNameNamespace}} + if _, ok := interface{}(mock).(interface{ Reset() }); !ok { + t.Fatal("precondition: MockResettableMapper must implement Reset()") + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Setup must succeed (return nil) even though the watch will never sync against this dummy host. + if err := WatchCRDsAndInvalidate(ctx, &rest.Config{Host: "http://127.0.0.1:1"}, mock, nil); err != nil { + t.Fatalf("WatchCRDsAndInvalidate() setup with a resettable mapper = %v, want nil", err) + } +}