From d595a93d362149292e547f8a9f7352fd6c63f0a6 Mon Sep 17 00:00:00 2001 From: Jan Novak Date: Sun, 9 Aug 2026 19:07:13 +0200 Subject: [PATCH] Close Step 11 test gaps: CEL envtest cases, defaults assertion, quota/permanent/adopt flows; make test runs -race Co-Authored-By: Claude --- Makefile | 2 +- .../2026-08-07-1747-proxy-operator.md | 54 +++- internal/controller/proxy_controller_test.go | 254 ++++++++++++++++++ 3 files changed, 308 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 39a77ad..7ac27a5 100644 --- a/Makefile +++ b/Makefile @@ -61,7 +61,7 @@ vet: ## Run go vet against code. .PHONY: test test: manifests generate fmt vet setup-envtest ## Run tests. - KUBEBUILDER_ASSETS="$(shell "$(ENVTEST)" use $(ENVTEST_K8S_VERSION) --bin-dir "$(LOCALBIN)" -p path)" go test $$(go list ./... | grep -v /e2e) -coverprofile cover.out + KUBEBUILDER_ASSETS="$(shell "$(ENVTEST)" use $(ENVTEST_K8S_VERSION) --bin-dir "$(LOCALBIN)" -p path)" go test -race $$(go list ./... | grep -v /e2e) -coverprofile cover.out # TODO(user): To use a different vendor for e2e tests, modify the setup under 'tests/e2e'. # The default setup assumes Kind is pre-installed and builds/loads the Manager Docker image locally. diff --git a/docs/plans-executions/2026-08-07-1747-proxy-operator.md b/docs/plans-executions/2026-08-07-1747-proxy-operator.md index dd3e7e9..1291560 100644 --- a/docs/plans-executions/2026-08-07-1747-proxy-operator.md +++ b/docs/plans-executions/2026-08-07-1747-proxy-operator.md @@ -15,7 +15,7 @@ Pairs with [docs/plans/2026-08-07-1747-proxy-operator.md](../plans/2026-08-07-17 - [x] Step 8 — GCP provider (`internal/provider/gcp/`) - [x] Step 9 — Orphan GC + metrics - [x] Step 10 — Wiring, config, docs -- [ ] Step 11 — Tests +- [x] Step 11 — Tests - [ ] Verification (vet/test/kind e2e) + commit, push, open MR ## Step 0 — Branch and scaffold @@ -1009,3 +1009,55 @@ a local process doesn't have; in-cluster deployments keep the secure default from the kustomize patch. The `providers` map wrapping happens *before* any consumer sees it, so the reconciler and GC only ever hold instrumented providers. + +## Step 11 — Tests + +Most of the plan's Step 11 list was deliberately front-loaded into the +step that built each component (the action-table suite, computePhase and +SpecHash tables, lease-store matrix incl. the concurrent `-race` case, +discovery httptest suite, health threshold/CONNECT tests, GCP +`buildInsertRequest` + classification, name-derivation tests from +Step 2). This step closed what remained — the envtest-only coverage — +and audited the list item by item. + +Added to `internal/controller/proxy_controller_test.go`: + +- **The CEL cases only a real API server can test** (fake clients run + neither CEL nor structural defaulting): six invalid-create rejections + (Managed-without-provider, External-with-provider, + External-without-endpoint, Managed-with-endpoint, cloudInit + both/neither), mode-mutation rejection, provider mutation *and removal* + rejection (the `has(self.x)==has(oldSelf.x)` form exists exactly for + the removal case), and the `+kubebuilder:default={}` assertion — a + Proxy created with no `healthCheck` comes back with every nested + default materialized, plus port and maxLeases defaults. +- **Ready-through-health**: Managed proxy walks to Running (phase still + Provisioning — "no health verdict yet must not be Ready"), then a fake + `HealthSnapshotter` supplies a healthy snapshot and the phase flips to + Ready with latency in status. +- **Quota + permanent, envtest edition**: quota → condition + QuotaExceeded, `RequeueAfter = QuotaRetry`, nil error, phase *not* + Failed; then permanent → Failed and the generation latch provably stops + further provider calls. +- **Adopt**: strip the spec-hash annotation off a Running proxy (as an + operator upgrade with a changed hash-input struct would), reconcile, + and assert the annotation is restored byte-identical, the providerID + unchanged, and zero provider deletes. + +One repo-wide change: `make test` now runs with `-race` (the plan's +"everything runs with -race" was previously only true of the manual +verification runs, not the canonical target): + +```make +go test -race $$(go list ./... | grep -v /e2e) -coverprofile cover.out +``` + +Everything green on the first run of the new specs; full suite ~9 s for +the controller package under race, `-short` still skips envtest in 0.6 s. + +Worth noting: the provider-removal CEL test has a subtlety worth keeping — +removing `provider` alone would also trip the required-iff rule, so the +test flips mode and adds an endpoint in the same update to isolate the +immutability rules as the thing that rejects. The plan's remaining +checklist item is Verification: the throwaway-kind-cluster run of the +README quickstart, then push + MR. diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go index 1c173ef..c02e940 100644 --- a/internal/controller/proxy_controller_test.go +++ b/internal/controller/proxy_controller_test.go @@ -28,6 +28,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/health" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" ) @@ -233,6 +234,105 @@ var _ = Describe("Proxy controller", func() { Expect(apierrors.IsNotFound(err)).To(BeTrue(), "proxy should be fully deleted") }) + It("reaches Ready once the health engine has a verdict", func() { + const name = "e2e-ready" + stub := &stubProvider{createID: "inst-rdy"} + r := newEnvtestReconciler(stub) + DeferCleanup(func() { cleanup(r, stub, name) }) + + Expect(k8sClient.Create(ctx, &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: managedSpec(), + })).To(Succeed()) + _, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + stub.getInst = &provider.Instance{ID: "inst-rdy", IP: "10.3.3.3", State: provider.StateRunning} + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(fetch(name).Status.Phase).To(Equal(crawlv1alpha1.PhaseProvisioning), + "no health verdict yet — must not be Ready") + + By("supplying a healthy snapshot") + r.Health = fakeSnapshotter{ok: true, snap: health.Snapshot{ + Healthy: true, Latency: 21 * time.Millisecond, LastProbe: time.Now(), + }} + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + p := fetch(name) + Expect(p.Status.Phase).To(Equal(crawlv1alpha1.PhaseReady)) + Expect(p.Status.LatencyMillis).To(Equal(int64(21))) + }) + + It("treats quota exhaustion as a wait and a permanent error as Failed", func() { + const name = "e2e-errors" + stub := &stubProvider{ + createErr: provider.Wrap(provider.ErrQuotaExceeded, "create", "stub", "", nil), + } + r := newEnvtestReconciler(stub) + DeferCleanup(func() { cleanup(r, stub, name) }) + + Expect(k8sClient.Create(ctx, &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: managedSpec(), + })).To(Succeed()) + _, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + + By("quota: condition set, slow requeue, phase NOT Failed") + res, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred(), "quota must not count as an error (stays off the backoff curve)") + Expect(res.RequeueAfter).To(Equal(r.QuotaRetry)) + p := fetch(name) + cond := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionProvisioned) + Expect(cond.Reason).To(Equal(ReasonQuotaExceeded)) + Expect(p.Status.Phase).NotTo(Equal(crawlv1alpha1.PhaseFailed)) + + By("permanent: phase Failed and no further provider calls") + stub.createErr = provider.Wrap(provider.ErrPermanent, "create", "stub", "", nil) + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(fetch(name).Status.Phase).To(Equal(crawlv1alpha1.PhaseFailed)) + callsAfterLatch := stub.createCalls + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(stub.createCalls).To(Equal(callsAfterLatch), "the latch must stop provider calls") + }) + + It("adopts an instance when the spec-hash annotation is stripped", func() { + const name = "e2e-adopt" + stub := &stubProvider{createID: "inst-adopt"} + r := newEnvtestReconciler(stub) + DeferCleanup(func() { cleanup(r, stub, name) }) + + Expect(k8sClient.Create(ctx, &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: managedSpec(), + })).To(Succeed()) + _, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + stub.getInst = &provider.Instance{ID: "inst-adopt", IP: "10.4.4.4", State: provider.StateRunning} + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + originalHash := fetch(name).Annotations[crawlv1alpha1.AnnotationSpecHash] + Expect(originalHash).NotTo(BeEmpty()) + + By("stripping the annotation, as an operator-version upgrade with a changed hash input would") + p := fetch(name) + delete(p.Annotations, crawlv1alpha1.AnnotationSpecHash) + Expect(k8sClient.Update(ctx, p)).To(Succeed()) + + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + p = fetch(name) + Expect(p.Annotations[crawlv1alpha1.AnnotationSpecHash]).To(Equal(originalHash), "hash must be restored") + Expect(p.Status.ProviderID).To(Equal("inst-adopt"), "adoption must keep the instance") + Expect(stub.deleteCalls).To(BeZero(), "adoption must never replace") + }) + It("tracks an External proxy without touching providers", func() { const name = "e2e-external" stub := &stubProvider{} @@ -264,3 +364,157 @@ var _ = Describe("Proxy controller", func() { Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) + +// These specs assert the CRD's CEL rules and structural defaulting against +// the real envtest API server — the fake client runs neither, which is the +// documented caveat on the action-table unit tests. +var _ = Describe("Proxy CRD validation (CEL)", func() { + const ns = "default" + + managed := func(name string) *crawlv1alpha1.Proxy { + return &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, + Provider: "stub", + }, + } + } + external := func(name string) *crawlv1alpha1.Proxy { + return &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeExternal, + Endpoint: &crawlv1alpha1.EndpointSpec{Host: "203.0.113.9"}, + }, + } + } + mustCreate := func(p *crawlv1alpha1.Proxy) { + GinkgoHelper() + Expect(k8sClient.Create(ctx, p)).To(Succeed()) + DeferCleanup(func() { _ = k8sClient.Delete(ctx, p) }) + } + + It("rejects invalid creates", func() { + invalid := []struct { + about string + spec crawlv1alpha1.ProxySpec + want string + }{ + { + about: "Managed without provider", + spec: crawlv1alpha1.ProxySpec{Mode: crawlv1alpha1.ModeManaged}, + want: "provider is required when mode is Managed", + }, + { + about: "External with provider", + spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeExternal, Provider: "stub", + Endpoint: &crawlv1alpha1.EndpointSpec{Host: "203.0.113.9"}, + }, + want: "provider must not be set when mode is External", + }, + { + about: "External without endpoint", + spec: crawlv1alpha1.ProxySpec{Mode: crawlv1alpha1.ModeExternal}, + want: "endpoint is required when mode is External", + }, + { + about: "Managed with endpoint", + spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, Provider: "stub", + Endpoint: &crawlv1alpha1.EndpointSpec{Host: "203.0.113.9"}, + }, + want: "endpoint must not be set when mode is Managed", + }, + { + about: "cloudInit with both inline and secretRef", + spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, Provider: "stub", + CloudInit: &crawlv1alpha1.CloudInitSpec{ + Inline: "#cloud-config", + SecretRef: &crawlv1alpha1.SecretKeySelector{Name: "s"}, + }, + }, + want: "exactly one of inline or secretRef", + }, + { + about: "cloudInit with neither inline nor secretRef", + spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, Provider: "stub", + CloudInit: &crawlv1alpha1.CloudInitSpec{}, + }, + want: "exactly one of inline or secretRef", + }, + } + for _, tc := range invalid { + p := &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: "cel-invalid", Namespace: ns}, + Spec: tc.spec, + } + err := k8sClient.Create(ctx, p) + Expect(err).To(HaveOccurred(), tc.about) + Expect(err.Error()).To(ContainSubstring(tc.want), tc.about) + } + }) + + It("rejects mode mutation", func() { + p := external("cel-mode-immutable") + mustCreate(p) + p.Spec = crawlv1alpha1.ProxySpec{Mode: crawlv1alpha1.ModeManaged, Provider: "stub"} + err := k8sClient.Update(ctx, p) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("mode is immutable")) + }) + + It("rejects provider mutation and removal", func() { + p := managed("cel-provider-immutable") + mustCreate(p) + + p.Spec.Provider = "other" + err := k8sClient.Update(ctx, p) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("provider is immutable")) + + // Removal must also be rejected — the has()==has() form exists + // exactly because a field-level rule would not fire on absence. + // (Dropping provider alone would also trip the required-iff rule, + // so flip mode too and check the immutability rules win.) + fresh := fetchProxy(ns, "cel-provider-immutable") + fresh.Spec.Provider = "" + fresh.Spec.Endpoint = &crawlv1alpha1.EndpointSpec{Host: "203.0.113.9"} + fresh.Spec.Mode = crawlv1alpha1.ModeExternal + err = k8sClient.Update(ctx, fresh) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("immutable")) + }) + + It("materializes every nested healthCheck default when healthCheck is omitted", func() { + p := managed("cel-defaults") + mustCreate(p) + + got := fetchProxy(ns, "cel-defaults") + // The +kubebuilder:default={} assertion: structural defaulting only + // descends into values that exist, so without it a nil healthCheck + // would get none of these. + hc := got.Spec.HealthCheck + Expect(hc).NotTo(BeNil()) + Expect(hc.ProbeURL).To(Equal(crawlv1alpha1.DefaultProbeURL)) + Expect(hc.IntervalSeconds).To(Equal(crawlv1alpha1.DefaultHealthCheckIntervalSeconds)) + Expect(hc.TimeoutSeconds).To(Equal(crawlv1alpha1.DefaultHealthCheckTimeoutSeconds)) + Expect(hc.FailureThreshold).To(Equal(crawlv1alpha1.DefaultFailureThreshold)) + Expect(hc.SuccessThreshold).To(Equal(crawlv1alpha1.DefaultSuccessThreshold)) + Expect(hc.ExpectedStatusCodes).To(Equal(crawlv1alpha1.DefaultExpectedStatusCodes)) + + Expect(got.Spec.Port).To(Equal(crawlv1alpha1.DefaultPort)) + Expect(got.Spec.MaxLeases).NotTo(BeNil()) + Expect(*got.Spec.MaxLeases).To(Equal(crawlv1alpha1.DefaultMaxLeases)) + }) +}) + +func fetchProxy(ns, name string) *crawlv1alpha1.Proxy { + GinkgoHelper() + p := &crawlv1alpha1.Proxy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, p)).To(Succeed()) + return p +}