diff --git a/.claude/settings.json b/.claude/settings.json index 658ab11..229e0f5 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -56,7 +56,10 @@ "Bash(go list *)", "Bash(gofmt -w internal/provider/config.go)", "Bash(gofmt -l .)", - "Bash(git restore *)" + "Bash(git restore *)", + "Bash(make manifests *)", + "Bash(make test *)", + "Bash(KUBEBUILDER_ASSETS=\"/Users/jan.novak/srv/go/egress-proxies-operator/bin/k8s/1.36.2-darwin-arm64\" go test -race ./internal/controller/)" ], "additionalDirectories": [ "/Users/jan.novak/srv/go/egress-proxies-operator/.claude", diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 1f40c91..e2bdbcd 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -4,6 +4,14 @@ kind: ClusterRole metadata: name: manager-role rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - list + - watch - apiGroups: - crawl.example.com resources: 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 0cda686..7287b69 100644 --- a/docs/plans-executions/2026-08-07-1747-proxy-operator.md +++ b/docs/plans-executions/2026-08-07-1747-proxy-operator.md @@ -8,7 +8,7 @@ Pairs with [docs/plans/2026-08-07-1747-proxy-operator.md](../plans/2026-08-07-17 - [x] Step 1 — API types (`api/v1alpha1/proxy_types.go`) - [x] Step 2 — Provider contract (`internal/provider/`) - [x] Step 3 — Kubernetes pod provider (`internal/provider/kubernetes/`; first built as an in-memory mock, then replaced — see the two Step 3 sections below) -- [ ] Step 4 — Reconciler (`internal/controller/`) +- [x] Step 4 — Reconciler (`internal/controller/`) - [ ] Step 5 — Health engine (`internal/health/`) - [ ] Step 6 — Lease store (`internal/lease/`) - [ ] Step 7 — Discovery API (`internal/discovery/`) @@ -560,3 +560,69 @@ Verified: `bin/kustomize build config/default` and `... config/crd` both render cleanly (no dangling references), `go build`/`go vet` clean with and without `-tags=e2e`, `make test` green with coverage numbers identical to pre-cleanup. + +## Step 4 — Reconciler (`internal/controller/`) + +Implemented the state machine per the plan's action table: +`proxy_controller.go` (dispatch + managed/external/delete paths, cloud-init +resolution, spec-hash annotation persistence, Secret→Proxy watch mapping), +`status.go` (condition reasons, `computePhase`, the single deferred +`patchStatusIfChanged`), and `spechash.go` (explicit +`{placement, resolved cloud-init, port}` hash input, SHA-256 hex). Tests: +the action-table suite against a fake client with an in-test `stubProvider` +(`reconcile_test.go`), `computePhase` truth table, spec-hash +stability/normalization/sensitivity tables, and a rewritten envtest suite +(`proxy_controller_test.go`) driving full lifecycles — provision→Running, +spec-change replacement, finalizer deletion, External tracking — against +the real apiserver with real CRD defaulting. + +**Deviation from the plan's `Requeue: true` rows:** `ctrl.Result{Requeue}` +is deprecated in controller-runtime v0.24 (verified in the vendored source, +`pkg/reconcile/reconcile.go`: "Deprecated: Use `RequeueAfter` instead"), and +golangci's staticcheck would flag it. Those rows use a fifth configurable +interval instead, `RequeueNow` (default 1s) — same "process the next state +promptly" semantics, still shrinkable in tests like the other four. + +**A real bug the new tests caught on their first run** (both the fake-client +and envtest suites, independently): in the replacement path's +instance-is-gone branch, the status clear (`ProviderID = ""`) was staged +*before* `setSpecHash`'s metadata `Update` — and `client.Update` refreshes +the whole object from the server's response, *including status*, so the +staged clear was silently overwritten and the proxy wedged with a stale +providerID. Fix: stage status changes only after any metadata Update +(the create branch already did it in that order). Worth remembering for +every future reconciler: **`r.Update` clobbers in-memory status staged +before it.** + +Two judgment calls the plan left open, now documented in code: + +- `computePhase` maps Provisioned=True with no Healthy verdict yet to + `Provisioning`, not `Ready` — a proxy nobody has probed shouldn't be + advertised as Ready. Health (Step 5) flips it. +- `deletionFailure` (the finalizer path's error handler) never latches + `ErrPermanent` the way `providerFailure` does — latching there would + wedge the object forever with no retry; it keeps retrying visibly + instead. + +The permanent-failure latch compares the condition's `observedGeneration` +against the CR generation, so a spec edit automatically clears Failed and +retries — no manual annotation-poking needed to recover. + +Verification: + +```bash +make test # regenerates manifests (role.yaml gains secrets get;list;watch), envtest green +KUBEBUILDER_ASSETS="$PWD/bin/k8s/1.36.2-darwin-arm64" go test -race ./internal/controller/ +go test -short ./internal/controller/ # 0.6s — envtest suite correctly skipped +go build -tags=e2e ./... && go vet -tags=e2e ./... +``` + +`internal/controller` at 75.9% coverage; the envtest suite runs in ~6s and +is now guarded by `testing.Short()` per the testing conventions. + +Worth noting: the envtest specs simulate instance state by mutating the +stub between direct `Reconcile` calls rather than running the manager — +deterministic and fast, at the cost of not exercising watch-driven +requeues; Step 11's manager-driven cases cover that. The Secret watch is +wired in `SetupWithManager` but the label-restricted Secret cache it +assumes arrives with `cmd/main.go` in Step 10. diff --git a/internal/controller/proxy_controller.go b/internal/controller/proxy_controller.go index 57c5a6b..3cf3529 100644 --- a/internal/controller/proxy_controller.go +++ b/internal/controller/proxy_controller.go @@ -18,46 +18,381 @@ package controller import ( "context" + "errors" + "fmt" + "time" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" ) -// ProxyReconciler reconciles a Proxy object +// ProxyReconciler reconciles Proxy objects as a state machine: every +// reconcile derives exactly one action from (spec, status, provider Get), +// performs it, and requeues. Status is written at most once per reconcile, +// by the deferred patch in Reconcile. type ProxyReconciler struct { client.Client Scheme *runtime.Scheme + + // Providers maps spec.provider values to configured backends. + Providers map[string]provider.Provider + + // Poll intervals are struct fields, never consts, so tests can shrink + // them to milliseconds. + ProvisioningPoll time.Duration // while waiting for an instance to reach Running + DriftPoll time.Duration // between re-checks of a Running instance + DeletionPoll time.Duration // while waiting for an instance to disappear + QuotaRetry time.Duration // after ErrQuotaExceeded; slow, off the backoff curve + RequeueNow time.Duration // "process the next state promptly" (Result.Requeue is deprecated) } // +kubebuilder:rbac:groups=crawl.example.com,resources=proxies,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=crawl.example.com,resources=proxies/status,verbs=get;update;patch // +kubebuilder:rbac:groups=crawl.example.com,resources=proxies/finalizers,verbs=update +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch -// Reconcile is part of the main kubernetes reconciliation loop which aims to -// move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the Proxy object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.24.1/pkg/reconcile -func (r *ProxyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - _ = logf.FromContext(ctx) +// Reconcile fetches the Proxy named by req into p (r.Get fills the struct +// through the pointer), dispatches to the delete/external/managed state +// machines, and flushes any status change exactly once on the way out. +func (r *ProxyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, err error) { + var p crawlv1alpha1.Proxy + if err := r.Get(ctx, req.NamespacedName, &p); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + base := p.DeepCopy() + defer func() { + // NotFound is expected when this reconcile just removed the last + // finalizer and the object is already gone. + if perr := r.patchStatusIfChanged(ctx, base, &p); perr != nil && !apierrors.IsNotFound(perr) { + err = errors.Join(err, perr) + } + }() - // TODO(user): your logic here + switch { + case !p.DeletionTimestamp.IsZero(): + return r.reconcileDelete(ctx, &p) + case p.Spec.Mode == crawlv1alpha1.ModeExternal: + return r.reconcileExternal(ctx, &p) + default: + return r.reconcileManaged(ctx, &p) + } +} +func (r *ProxyReconciler) reconcileManaged(ctx context.Context, p *crawlv1alpha1.Proxy) (ctrl.Result, error) { + log := logf.FromContext(ctx) + + if controllerutil.AddFinalizer(p, crawlv1alpha1.FinalizerName) { + // The Update event re-triggers reconciliation; provisioning starts + // on the next pass, with the finalizer safely persisted first. + return ctrl.Result{}, r.Update(ctx, p) + } + + // Permanent-failure latch: once this generation has failed permanently, + // stop calling the provider until the spec changes. + if cond := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionProvisioned); cond != nil && + cond.Status == metav1.ConditionFalse && cond.Reason == ReasonPermanentError && + cond.ObservedGeneration == p.Generation { + return ctrl.Result{}, nil + } + + prov, ok := r.Providers[p.Spec.Provider] + if !ok { + setProvisioned(p, metav1.ConditionFalse, ReasonPermanentError, + fmt.Sprintf("provider %q is not configured", p.Spec.Provider)) + return ctrl.Result{}, nil + } + + cloudInit, err := r.resolveCloudInit(ctx, p) + if err != nil { + setProvisioned(p, metav1.ConditionFalse, ReasonCloudInitError, err.Error()) + return ctrl.Result{}, err + } + hash := specHash(p, cloudInit) + + if p.Status.ProviderID == "" { + id, err := prov.Create(ctx, provider.CreateRequest{ + Name: provider.NameFromUID(p.UID), + UID: string(p.UID), + Namespace: p.Namespace, + ProxyName: p.Name, + Placement: placementFrom(p.Spec.Placement), + CloudInit: cloudInit, + Port: p.EffectivePort(), + }) + if err != nil { + return r.providerFailure(p, err) + } + log.Info("created instance", "provider", p.Spec.Provider, "providerID", id) + if err := r.setSpecHash(ctx, p, hash); err != nil { + return ctrl.Result{}, err + } + p.Status.ProviderID = id + p.Status.IP = "" + setProvisioned(p, metav1.ConditionFalse, ReasonProvisioning, "instance created; waiting for it to run") + return ctrl.Result{RequeueAfter: r.ProvisioningPoll}, nil + } + + if ann := p.Annotations[crawlv1alpha1.AnnotationSpecHash]; ann != hash { + if ann == "" { + // Adopt: an instance provisioned before the hash-input struct + // gained a field (or by an older operator version) keeps its + // instance; replacing the whole fleet on upgrade would be wrong. + if err := r.setSpecHash(ctx, p, hash); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: r.RequeueNow}, nil + } + return r.replaceInstance(ctx, p, prov, hash) + } + + inst, err := prov.Get(ctx, p.Status.ProviderID) + if provider.Class(err) == provider.ErrNotFound { + p.Status.ProviderID = "" + p.Status.IP = "" + return ctrl.Result{RequeueAfter: r.RequeueNow}, nil + } + if err != nil { + return r.providerFailure(p, err) + } + + switch inst.State { + case provider.StateProvisioning: + p.Status.IP = "" + setProvisioned(p, metav1.ConditionFalse, ReasonProvisioning, "waiting for the instance to run") + return ctrl.Result{RequeueAfter: r.ProvisioningPoll}, nil + case provider.StateRunning: + p.Status.IP = inst.IP + setProvisioned(p, metav1.ConditionTrue, ReasonCreated, "instance is running") + return ctrl.Result{RequeueAfter: r.DriftPoll}, nil + default: // Stopped, Terminated: cattle, not pets — delete and recreate. + if err := prov.Delete(ctx, p.Status.ProviderID); err != nil { + return r.providerFailure(p, err) + } + log.Info("deleting instance for recreation", "providerID", p.Status.ProviderID, "state", inst.State) + p.Status.IP = "" + setProvisioned(p, metav1.ConditionFalse, ReasonRecreating, + fmt.Sprintf("instance is %s; deleting it for recreation", inst.State)) + return ctrl.Result{RequeueAfter: r.DeletionPoll}, nil + } +} + +// replaceInstance handles a spec-hash mismatch. The replacement instance has +// the same deterministic name as the old one (both derive from the CR UID), +// so recreating before the old instance is fully gone would hit "already +// exists" — hence: delete, poll to NotFound, only then advance the hash and +// let the create branch run. +func (r *ProxyReconciler) replaceInstance(ctx context.Context, p *crawlv1alpha1.Proxy, prov provider.Provider, hash string) (ctrl.Result, error) { + _, err := prov.Get(ctx, p.Status.ProviderID) + if provider.Class(err) == provider.ErrNotFound { + // Old instance is gone. The Update inside setSpecHash refreshes p + // from the server — including status — so the status clear must be + // staged after it, or it would be silently overwritten. A crash + // between the two writes recovers either way: the create branch's + // Create is idempotent by name, and a stale ID resolves to NotFound + // again. + if err := r.setSpecHash(ctx, p, hash); err != nil { + return ctrl.Result{}, err + } + p.Status.ProviderID = "" + p.Status.IP = "" + return ctrl.Result{RequeueAfter: r.RequeueNow}, nil + } + if err != nil { + return r.providerFailure(p, err) + } + if err := prov.Delete(ctx, p.Status.ProviderID); err != nil { + return r.providerFailure(p, err) + } + logf.FromContext(ctx).Info("replacing instance after spec change", "providerID", p.Status.ProviderID) + p.Status.IP = "" + setProvisioned(p, metav1.ConditionFalse, ReasonReplacing, "spec changed; deleting the old instance before recreating") + return ctrl.Result{RequeueAfter: r.DeletionPoll}, nil +} + +func (r *ProxyReconciler) reconcileDelete(ctx context.Context, p *crawlv1alpha1.Proxy) (ctrl.Result, error) { + if !controllerutil.ContainsFinalizer(p, crawlv1alpha1.FinalizerName) { + return ctrl.Result{}, nil + } + if p.Status.ProviderID == "" { + // Nothing was ever recorded as created; orphan GC reaps any stray + // instance a crashed create might have left behind. + controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName) + return ctrl.Result{}, r.Update(ctx, p) + } + prov, ok := r.Providers[p.Spec.Provider] + if !ok { + return ctrl.Result{}, fmt.Errorf( + "provider %q is not configured; cannot clean up instance %s", p.Spec.Provider, p.Status.ProviderID) + } + _, err := prov.Get(ctx, p.Status.ProviderID) + if provider.Class(err) == provider.ErrNotFound { + controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName) + return ctrl.Result{}, r.Update(ctx, p) + } + if err != nil { + return r.deletionFailure(err) + } + if err := prov.Delete(ctx, p.Status.ProviderID); err != nil { + return r.deletionFailure(err) + } + logf.FromContext(ctx).Info("deleting instance", "providerID", p.Status.ProviderID) + setProvisioned(p, metav1.ConditionFalse, ReasonDeleting, "deleting the instance before removing the finalizer") + return ctrl.Result{RequeueAfter: r.DeletionPoll}, nil +} + +func (r *ProxyReconciler) reconcileExternal(_ context.Context, p *crawlv1alpha1.Proxy) (ctrl.Result, error) { + if p.Spec.Endpoint == nil { + // CEL guarantees an endpoint on any object that went through the API + // server; tolerate its absence instead of panicking. + setProvisioned(p, metav1.ConditionFalse, ReasonPermanentError, "external proxy has no endpoint") + return ctrl.Result{}, nil + } + p.Status.IP = p.Spec.Endpoint.Host + setProvisioned(p, metav1.ConditionTrue, ReasonExternalEndpoint, "tracking an external endpoint") return ctrl.Result{}, nil } -// SetupWithManager sets up the controller with the Manager. +// providerFailure translates a classified provider error into the +// state-machine's reaction: transient errors ride the workqueue's +// exponential backoff, quota errors back off slowly without counting as +// errors, and permanent errors latch Failed and stop retrying. +func (r *ProxyReconciler) providerFailure(p *crawlv1alpha1.Proxy, err error) (ctrl.Result, error) { + switch provider.Class(err) { + case provider.ErrQuotaExceeded: + setProvisioned(p, metav1.ConditionFalse, ReasonQuotaExceeded, err.Error()) + return ctrl.Result{RequeueAfter: r.QuotaRetry}, nil + case provider.ErrPermanent: + setProvisioned(p, metav1.ConditionFalse, ReasonPermanentError, err.Error()) + return ctrl.Result{}, nil + default: + return ctrl.Result{}, err + } +} + +// deletionFailure is providerFailure for the finalizer path, where latching +// a permanent failure would wedge the object forever with no retry — keep +// retrying instead, visibly, until cleanup succeeds or an operator +// intervenes. +func (r *ProxyReconciler) deletionFailure(err error) (ctrl.Result, error) { + if provider.Class(err) == provider.ErrQuotaExceeded { + return ctrl.Result{RequeueAfter: r.QuotaRetry}, nil + } + return ctrl.Result{}, err +} + +// resolveCloudInit returns the effective cloud-init user-data, reading the +// referenced Secret if one is used. Both the spec hash and CreateRequest see +// only resolved content, so rotating a Secret triggers replacement. +func (r *ProxyReconciler) resolveCloudInit(ctx context.Context, p *crawlv1alpha1.Proxy) (string, error) { + ci := p.Spec.CloudInit + if ci == nil { + return "", nil + } + if ci.Inline != "" { + return ci.Inline, nil + } + if ci.SecretRef == nil { + return "", nil + } + key := ci.SecretRef.Key + if key == "" { + key = crawlv1alpha1.DefaultCloudInitSecretKey + } + var sec corev1.Secret + if err := r.Get(ctx, client.ObjectKey{Namespace: p.Namespace, Name: ci.SecretRef.Name}, &sec); err != nil { + return "", fmt.Errorf("resolving cloudInit secret %q: %w", ci.SecretRef.Name, err) + } + data, ok := sec.Data[key] + if !ok { + return "", fmt.Errorf("cloudInit secret %q has no key %q", ci.SecretRef.Name, key) + } + return string(data), nil +} + +// setSpecHash persists the spec-hash annotation. Status changes staged on p +// are untouched by the Update (they live on the status subresource) and are +// flushed by the deferred patch in Reconcile. +func (r *ProxyReconciler) setSpecHash(ctx context.Context, p *crawlv1alpha1.Proxy, hash string) error { + if p.Annotations[crawlv1alpha1.AnnotationSpecHash] == hash { + return nil + } + if p.Annotations == nil { + p.Annotations = map[string]string{} + } + p.Annotations[crawlv1alpha1.AnnotationSpecHash] = hash + return r.Update(ctx, p) +} + +func placementFrom(ps *crawlv1alpha1.PlacementSpec) provider.Placement { + if ps == nil { + return provider.Placement{} + } + return provider.Placement{ + Region: ps.Region, + Zone: ps.Zone, + MachineType: ps.MachineType, + Image: ps.Image, + } +} + +// proxiesForSecret maps a Secret event to the Proxies whose cloudInit +// references it, so rotating a Secret re-triggers the replacement check. +func (r *ProxyReconciler) proxiesForSecret(ctx context.Context, obj client.Object) []reconcile.Request { + var list crawlv1alpha1.ProxyList + if err := r.List(ctx, &list, client.InNamespace(obj.GetNamespace())); err != nil { + logf.FromContext(ctx).Error(err, "listing proxies for secret event", "secret", obj.GetName()) + return nil + } + var reqs []reconcile.Request + for i := range list.Items { + p := &list.Items[i] + if ci := p.Spec.CloudInit; ci != nil && ci.SecretRef != nil && ci.SecretRef.Name == obj.GetName() { + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(p)}) + } + } + return reqs +} + +// SetupWithManager sets up the controller with the Manager. The Secret watch +// only fires for Secrets the manager's cache holds; the composition root +// (cmd/main.go) restricts that cache to labelled cloud-init Secrets. func (r *ProxyReconciler) SetupWithManager(mgr ctrl.Manager) error { + r.applyDefaults() return ctrl.NewControllerManagedBy(mgr). For(&crawlv1alpha1.Proxy{}). Named("proxy"). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.proxiesForSecret)). + WithOptions(controller.Options{MaxConcurrentReconciles: 3}). Complete(r) } + +func (r *ProxyReconciler) applyDefaults() { + if r.ProvisioningPoll == 0 { + r.ProvisioningPoll = 10 * time.Second + } + if r.DriftPoll == 0 { + r.DriftPoll = 2 * time.Minute + } + if r.DeletionPoll == 0 { + r.DeletionPoll = 10 * time.Second + } + if r.QuotaRetry == 0 { + r.QuotaRetry = 5 * time.Minute + } + if r.RequeueNow == 0 { + r.RequeueNow = time.Second + } +} diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go index e10d5da..1c173ef 100644 --- a/internal/controller/proxy_controller_test.go +++ b/internal/controller/proxy_controller_test.go @@ -17,77 +17,250 @@ limitations under the License. package controller import ( - "context" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + 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/provider" ) -var _ = Describe("Proxy Controller", func() { - Context("When reconciling a resource", func() { - const ( - resourceName = "test-resource" - resourceNamespace = "default" - ) +// These specs drive the reconciler against a real envtest API server, so +// CRD structural defaulting and CEL validation are live — the parts the +// fake-client action-table tests can't cover. The provider stays a stub: +// envtest has no kubelet or cloud, so instance state is simulated by +// mutating the stub between reconciles. +var _ = Describe("Proxy controller", func() { + const ns = "default" - ctx := context.Background() - - typeNamespacedName := types.NamespacedName{ - Name: resourceName, - Namespace: resourceNamespace, + newEnvtestReconciler := func(stub *stubProvider) *ProxyReconciler { + return &ProxyReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Providers: map[string]provider.Provider{"stub": stub}, + ProvisioningPoll: 50 * time.Millisecond, + DriftPoll: 100 * time.Millisecond, + DeletionPoll: 50 * time.Millisecond, + QuotaRetry: 200 * time.Millisecond, + RequeueNow: 10 * time.Millisecond, } - proxy := &crawlv1alpha1.Proxy{} + } - BeforeEach(func() { - By("creating the custom resource for the Kind Proxy") - err := k8sClient.Get(ctx, typeNamespacedName, proxy) - if err != nil && errors.IsNotFound(err) { - resource := &crawlv1alpha1.Proxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - Namespace: resourceNamespace, - }, - // A minimal, schema-valid spec so this placeholder test survives the - // CEL/CRD validation added in Step 1. Rewritten wholesale in Step 4 - // alongside the real reconciler and envtest suite. - Spec: crawlv1alpha1.ProxySpec{ - Mode: crawlv1alpha1.ModeExternal, - Endpoint: &crawlv1alpha1.EndpointSpec{Host: "10.0.0.1"}, - }, - } - Expect(k8sClient.Create(ctx, resource)).To(Succeed()) + envReconcile := func(r *ProxyReconciler, name string) (ctrl.Result, error) { + return r.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: ns, Name: name}, + }) + } + + fetch := func(name string) *crawlv1alpha1.Proxy { + p := &crawlv1alpha1.Proxy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, p)).To(Succeed()) + return p + } + + // cleanup drives a Managed proxy's finalizer to completion so one spec's + // leftovers can't leak into another. Registered via DeferCleanup so it + // runs even when the spec body fails mid-way. + cleanup := func(r *ProxyReconciler, stub *stubProvider, name string) { + p := &crawlv1alpha1.Proxy{} + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, p) + if apierrors.IsNotFound(err) { + return + } + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient.Delete(ctx, p)).To(Succeed()) + stub.getErr = provider.Wrap(provider.ErrNotFound, "get", "stub", "", nil) + for range 3 { + if _, err := envReconcile(r, name); err != nil { + break } - }) - - AfterEach(func() { - // TODO(user): Cleanup logic after each test, like removing the resource instance. - resource := &crawlv1alpha1.Proxy{} - err := k8sClient.Get(ctx, typeNamespacedName, resource) - Expect(err).NotTo(HaveOccurred()) - - By("Cleanup the specific resource instance Proxy") - Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) - }) - It("should successfully reconcile the resource", func() { - By("Reconciling the created resource") - controllerReconciler := &ProxyReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + if apierrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, p)) { + return } + } + Fail("cleanup did not drive the proxy " + name + " to deletion") + } - _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: typeNamespacedName, - }) - Expect(err).NotTo(HaveOccurred()) - // TODO(user): Add more specific assertions depending on your controller's reconciliation logic. - // Example: If you expect a certain status condition after reconciliation, verify it here. - }) + managedSpec := func() crawlv1alpha1.ProxySpec { + return crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, + Provider: "stub", + } + } + + It("provisions a Managed proxy through to Running", func() { + const name = "e2e-provision" + stub := &stubProvider{createID: "inst-1"} + 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()) + + By("adding the finalizer on the first pass") + res, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(Equal(ctrl.Result{})) + Expect(fetch(name).Finalizers).To(ContainElement(crawlv1alpha1.FinalizerName)) + + By("creating the instance on the second pass") + res, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(r.ProvisioningPoll)) + p := fetch(name) + Expect(p.Status.ProviderID).To(Equal("inst-1")) + Expect(p.Annotations).To(HaveKey(crawlv1alpha1.AnnotationSpecHash)) + // The real API server defaulted spec.port; the create request must + // have seen it. + Expect(p.Spec.Port).To(Equal(crawlv1alpha1.DefaultPort)) + Expect(stub.lastCreate.Port).To(Equal(crawlv1alpha1.DefaultPort)) + Expect(stub.lastCreate.Name).To(Equal(provider.NameFromUID(p.UID))) + + By("polling while the instance provisions") + stub.getInst = &provider.Instance{ID: "inst-1", State: provider.StateProvisioning} + res, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(r.ProvisioningPoll)) + Expect(fetch(name).Status.Phase).To(Equal(crawlv1alpha1.PhaseProvisioning)) + + By("publishing the IP once the instance runs") + stub.getInst = &provider.Instance{ID: "inst-1", IP: "10.9.8.7", State: provider.StateRunning} + res, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(r.DriftPoll)) + p = fetch(name) + Expect(p.Status.IP).To(Equal("10.9.8.7")) + cond := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionProvisioned) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionTrue)) + Expect(p.Status.ObservedGeneration).To(Equal(p.Generation)) + }) + + It("replaces the instance when the spec changes", func() { + const name = "e2e-replace" + stub := &stubProvider{createID: "inst-old"} + 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-old", IP: "10.0.0.1", State: provider.StateRunning} + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + oldHash := fetch(name).Annotations[crawlv1alpha1.AnnotationSpecHash] + + By("editing a replacement-triggering field") + p := fetch(name) + p.Spec.Port = 8080 + Expect(k8sClient.Update(ctx, p)).To(Succeed()) + + By("deleting the old instance first") + res, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(r.DeletionPoll)) + Expect(stub.deleteCalls).To(Equal(1)) + p = fetch(name) + Expect(p.Annotations[crawlv1alpha1.AnnotationSpecHash]).To(Equal(oldHash), + "hash must not advance while the old instance still exists") + cond := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionProvisioned) + Expect(cond.Reason).To(Equal(ReasonReplacing)) + + By("advancing the hash once the old instance is gone") + stub.getErr = provider.Wrap(provider.ErrNotFound, "get", "stub", "inst-old", nil) + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + p = fetch(name) + Expect(p.Status.ProviderID).To(BeEmpty()) + Expect(p.Annotations[crawlv1alpha1.AnnotationSpecHash]).NotTo(Equal(oldHash)) + + By("creating the replacement") + stub.createID = "inst-new" + stub.getErr = nil + stub.getInst = nil + res, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(r.ProvisioningPoll)) + Expect(stub.createCalls).To(Equal(2)) + Expect(fetch(name).Status.ProviderID).To(Equal("inst-new")) + Expect(stub.lastCreate.Port).To(Equal(int32(8080))) + }) + + It("cleans up the instance on delete via the finalizer", func() { + const name = "e2e-delete" + stub := &stubProvider{createID: "inst-del"} + 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-del", IP: "10.0.0.2", State: provider.StateRunning} + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + + By("deleting the CR — the finalizer holds it") + Expect(k8sClient.Delete(ctx, fetch(name))).To(Succeed()) + res, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(r.DeletionPoll)) + Expect(stub.deleteCalls).To(Equal(1)) + Expect(fetch(name).Status.Phase).To(Equal(crawlv1alpha1.PhaseDeleting)) + + By("removing the finalizer once the instance is gone") + stub.getErr = provider.Wrap(provider.ErrNotFound, "get", "stub", "inst-del", nil) + _, err = envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, &crawlv1alpha1.Proxy{}) + Expect(apierrors.IsNotFound(err)).To(BeTrue(), "proxy should be fully deleted") + }) + + It("tracks an External proxy without touching providers", func() { + const name = "e2e-external" + stub := &stubProvider{} + r := newEnvtestReconciler(stub) + + Expect(k8sClient.Create(ctx, &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeExternal, + Endpoint: &crawlv1alpha1.EndpointSpec{Host: "203.0.113.7"}, + }, + })).To(Succeed()) + + res, err := envReconcile(r, name) + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(Equal(ctrl.Result{})) + + p := fetch(name) + Expect(p.Status.IP).To(Equal("203.0.113.7")) + Expect(p.Finalizers).To(BeEmpty()) + cond := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionProvisioned) + Expect(cond).NotTo(BeNil()) + Expect(cond.Reason).To(Equal(ReasonExternalEndpoint)) + Expect(stub.createCalls + stub.getCalls + stub.deleteCalls).To(BeZero()) + + By("deleting without any finalizer round-trip") + Expect(k8sClient.Delete(ctx, p)).To(Succeed()) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, &crawlv1alpha1.Proxy{}) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) }) diff --git a/internal/controller/reconcile_test.go b/internal/controller/reconcile_test.go new file mode 100644 index 0000000..6f0150a --- /dev/null +++ b/internal/controller/reconcile_test.go @@ -0,0 +1,550 @@ +package controller + +import ( + "context" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" +) + +// Distinct per-interval values so an asserted ctrl.Result is unambiguous +// about which action-table row produced it. +const ( + tProvisioningPoll = 11 * time.Second + tDriftPoll = 22 * time.Second + tDeletionPoll = 33 * time.Second + tQuotaRetry = 44 * time.Second + tRequeueNow = 55 * time.Millisecond +) + +const ( + testProxyName = "p1" + testNamespace = "default" + testUID = types.UID("11111111-2222-3333-4444-555555555555") +) + +// stubProvider is the plan's in-test Provider stub: a handful of lines, no +// config format, no fault-injection surface beyond settable fields. +type stubProvider struct { + createID string + createErr error + getInst *provider.Instance + getErr error + deleteErr error + + createCalls, getCalls, deleteCalls int + lastCreate provider.CreateRequest +} + +func (s *stubProvider) Create(_ context.Context, req provider.CreateRequest) (string, error) { + s.createCalls++ + s.lastCreate = req + if s.createErr != nil { + return "", s.createErr + } + return s.createID, nil +} + +func (s *stubProvider) Get(_ context.Context, _ string) (*provider.Instance, error) { + s.getCalls++ + if s.getErr != nil { + return nil, s.getErr + } + return s.getInst, nil +} + +func (s *stubProvider) Delete(_ context.Context, _ string) error { + s.deleteCalls++ + return s.deleteErr +} + +func (s *stubProvider) ListByTag(context.Context) ([]provider.Instance, error) { + return nil, nil +} + +func notFoundErr() error { + return provider.Wrap(provider.ErrNotFound, "get", "stub", "some-id", nil) +} + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := crawlv1alpha1.AddToScheme(s); err != nil { + t.Fatalf("adding crawl scheme: %v", err) + } + if err := corev1.AddToScheme(s); err != nil { + t.Fatalf("adding core scheme: %v", err) + } + return s +} + +// managedProxy returns a Managed proxy that already carries the finalizer — +// the state most action-table rows start from. Mutators adjust from there. +func managedProxy(mut ...func(*crawlv1alpha1.Proxy)) *crawlv1alpha1.Proxy { + p := &crawlv1alpha1.Proxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: testProxyName, + Namespace: testNamespace, + UID: testUID, + Generation: 1, + Finalizers: []string{crawlv1alpha1.FinalizerName}, + }, + Spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, + Provider: "stub", + }, + } + for _, m := range mut { + m(p) + } + return p +} + +func withProviderID(id string) func(*crawlv1alpha1.Proxy) { + return func(p *crawlv1alpha1.Proxy) { p.Status.ProviderID = id } +} + +func withSpecHashAnnotation(hash string) func(*crawlv1alpha1.Proxy) { + return func(p *crawlv1alpha1.Proxy) { + if p.Annotations == nil { + p.Annotations = map[string]string{} + } + p.Annotations[crawlv1alpha1.AnnotationSpecHash] = hash + } +} + +func deleting() func(*crawlv1alpha1.Proxy) { + return func(p *crawlv1alpha1.Proxy) { + now := metav1.Now() + p.DeletionTimestamp = &now + } +} + +func newTestReconciler(t *testing.T, stub *stubProvider, objs ...client.Object) *ProxyReconciler { + t.Helper() + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithStatusSubresource(&crawlv1alpha1.Proxy{}). + WithObjects(objs...). + Build() + return &ProxyReconciler{ + Client: c, + Providers: map[string]provider.Provider{"stub": stub}, + ProvisioningPoll: tProvisioningPoll, + DriftPoll: tDriftPoll, + DeletionPoll: tDeletionPoll, + QuotaRetry: tQuotaRetry, + RequeueNow: tRequeueNow, + } +} + +func doReconcile(t *testing.T, r *ProxyReconciler) (ctrl.Result, error) { + t.Helper() + return r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: testProxyName}, + }) +} + +func getProxy(t *testing.T, r *ProxyReconciler) *crawlv1alpha1.Proxy { + t.Helper() + var p crawlv1alpha1.Proxy + if err := r.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: testProxyName}, &p); err != nil { + t.Fatalf("getting proxy: %v", err) + } + return &p +} + +func assertCondition(t *testing.T, p *crawlv1alpha1.Proxy, condType string, status metav1.ConditionStatus, reason string) { + t.Helper() + cond := apimeta.FindStatusCondition(p.Status.Conditions, condType) + if cond == nil { + t.Fatalf("condition %s missing, have %+v", condType, p.Status.Conditions) + } + if cond.Status != status || cond.Reason != reason { + t.Errorf("condition %s = %s/%s, want %s/%s", condType, cond.Status, cond.Reason, status, reason) + } + if cond.ObservedGeneration != p.Generation { + t.Errorf("condition %s observedGeneration = %d, want %d", condType, cond.ObservedGeneration, p.Generation) + } +} + +// TestReconcile_actionTable exercises every row of the plan's action table +// by calling Reconcile directly against a fake client. Caveat (documented in +// the plan): the fake client runs neither CEL validation nor structural +// defaulting — the envtest suite covers those. +func TestReconcile_actionTable(t *testing.T) { + t.Parallel() + + // The fixture's hash: no placement, no cloud-init, defaulted port. + freshHash := specHash(managedProxy(), "") + + tests := []struct { + name string + proxy *crawlv1alpha1.Proxy + extraObjs []client.Object + stub *stubProvider + wantResult ctrl.Result + wantErr bool + verify func(t *testing.T, r *ProxyReconciler, stub *stubProvider) + }{ + { + name: "managed without finalizer gets one and stops", + proxy: managedProxy(func(p *crawlv1alpha1.Proxy) { + p.Finalizers = nil + }), + stub: &stubProvider{}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + p := getProxy(t, r) + if len(p.Finalizers) != 1 || p.Finalizers[0] != crawlv1alpha1.FinalizerName { + t.Errorf("finalizers = %v, want [%s]", p.Finalizers, crawlv1alpha1.FinalizerName) + } + if stub.createCalls+stub.getCalls+stub.deleteCalls != 0 { + t.Errorf("provider was called before the finalizer was persisted") + } + }, + }, + { + name: "empty providerID creates the instance", + proxy: managedProxy(), + stub: &stubProvider{createID: "stub-id-1"}, + wantResult: ctrl.Result{RequeueAfter: tProvisioningPoll}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + p := getProxy(t, r) + if p.Status.ProviderID != "stub-id-1" { + t.Errorf("providerID = %q, want stub-id-1", p.Status.ProviderID) + } + if got := p.Annotations[crawlv1alpha1.AnnotationSpecHash]; got != freshHash { + t.Errorf("spec-hash annotation = %q, want %q", got, freshHash) + } + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonProvisioning) + if p.Status.Phase != crawlv1alpha1.PhaseProvisioning { + t.Errorf("phase = %s, want Provisioning", p.Status.Phase) + } + want := provider.CreateRequest{ + Name: provider.NameFromUID(testUID), + UID: string(testUID), + Namespace: testNamespace, + ProxyName: testProxyName, + Port: crawlv1alpha1.DefaultPort, + } + if stub.lastCreate != want { + t.Errorf("CreateRequest = %+v, want %+v", stub.lastCreate, want) + } + }, + }, + { + name: "provisioning instance polls again", + proxy: managedProxy(withProviderID("stub-id-1"), withSpecHashAnnotation(freshHash)), + stub: &stubProvider{getInst: &provider.Instance{ID: "stub-id-1", State: provider.StateProvisioning}}, + wantResult: ctrl.Result{RequeueAfter: tProvisioningPoll}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + if p.Status.IP != "" { + t.Errorf("ip = %q, want empty while provisioning", p.Status.IP) + } + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonProvisioning) + }, + }, + { + name: "running instance publishes IP", + proxy: managedProxy(withProviderID("stub-id-1"), withSpecHashAnnotation(freshHash)), + stub: &stubProvider{ + getInst: &provider.Instance{ID: "stub-id-1", IP: "10.1.2.3", State: provider.StateRunning}, + }, + wantResult: ctrl.Result{RequeueAfter: tDriftPoll}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + if p.Status.IP != "10.1.2.3" { + t.Errorf("ip = %q, want 10.1.2.3", p.Status.IP) + } + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonCreated) + if p.Status.Phase != crawlv1alpha1.PhaseProvisioning { + t.Errorf("phase = %s, want Provisioning until a health verdict exists", p.Status.Phase) + } + }, + }, + { + name: "stopped instance is deleted for recreation", + proxy: managedProxy(withProviderID("stub-id-1"), withSpecHashAnnotation(freshHash)), + stub: &stubProvider{getInst: &provider.Instance{ID: "stub-id-1", State: provider.StateStopped}}, + wantResult: ctrl.Result{RequeueAfter: tDeletionPoll}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + if stub.deleteCalls != 1 { + t.Errorf("deleteCalls = %d, want 1", stub.deleteCalls) + } + assertCondition(t, getProxy(t, r), crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonRecreating) + }, + }, + { + name: "vanished instance clears ID for recreation", + proxy: managedProxy(withProviderID("stub-id-1"), withSpecHashAnnotation(freshHash), + func(p *crawlv1alpha1.Proxy) { p.Status.IP = "10.1.2.3" }), + stub: &stubProvider{getErr: notFoundErr()}, + wantResult: ctrl.Result{RequeueAfter: tRequeueNow}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + if p.Status.ProviderID != "" || p.Status.IP != "" { + t.Errorf("providerID/ip = %q/%q, want both cleared", p.Status.ProviderID, p.Status.IP) + } + }, + }, + { + name: "hash mismatch deletes the old instance", + proxy: managedProxy(withProviderID("stub-id-1"), withSpecHashAnnotation("stale-hash")), + stub: &stubProvider{ + getInst: &provider.Instance{ID: "stub-id-1", IP: "10.1.2.3", State: provider.StateRunning}, + }, + wantResult: ctrl.Result{RequeueAfter: tDeletionPoll}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + if stub.deleteCalls != 1 { + t.Errorf("deleteCalls = %d, want 1", stub.deleteCalls) + } + p := getProxy(t, r) + // The hash must not advance until the old instance is gone, + // or a crash would strand a half-replaced proxy. + if got := p.Annotations[crawlv1alpha1.AnnotationSpecHash]; got != "stale-hash" { + t.Errorf("spec-hash annotation = %q, want still stale-hash", got) + } + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonReplacing) + }, + }, + { + name: "empty annotation adopts instead of replacing", + proxy: managedProxy(withProviderID("stub-id-1")), + stub: &stubProvider{}, + wantResult: ctrl.Result{RequeueAfter: tRequeueNow}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + p := getProxy(t, r) + if got := p.Annotations[crawlv1alpha1.AnnotationSpecHash]; got != freshHash { + t.Errorf("spec-hash annotation = %q, want %q", got, freshHash) + } + if p.Status.ProviderID != "stub-id-1" { + t.Errorf("providerID = %q, want untouched stub-id-1", p.Status.ProviderID) + } + if stub.deleteCalls != 0 { + t.Errorf("deleteCalls = %d, want 0 — adoption must not replace", stub.deleteCalls) + } + }, + }, + { + name: "mismatch with instance gone advances the hash", + proxy: managedProxy(withProviderID("stub-id-1"), withSpecHashAnnotation("stale-hash")), + stub: &stubProvider{getErr: notFoundErr()}, + wantResult: ctrl.Result{RequeueAfter: tRequeueNow}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + if p.Status.ProviderID != "" { + t.Errorf("providerID = %q, want cleared", p.Status.ProviderID) + } + if got := p.Annotations[crawlv1alpha1.AnnotationSpecHash]; got != freshHash { + t.Errorf("spec-hash annotation = %q, want advanced to %q", got, freshHash) + } + }, + }, + { + name: "deletion with no providerID removes the finalizer", + proxy: managedProxy(deleting()), + stub: &stubProvider{}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + assertProxyGone(t, r) + if stub.deleteCalls != 0 { + t.Errorf("deleteCalls = %d, want 0", stub.deleteCalls) + } + }, + }, + { + name: "deletion with instance already gone removes the finalizer", + proxy: managedProxy(deleting(), withProviderID("stub-id-1")), + stub: &stubProvider{getErr: notFoundErr()}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + assertProxyGone(t, r) + }, + }, + { + name: "deletion deletes the instance and polls", + proxy: managedProxy(deleting(), withProviderID("stub-id-1")), + stub: &stubProvider{ + getInst: &provider.Instance{ID: "stub-id-1", State: provider.StateRunning}, + }, + wantResult: ctrl.Result{RequeueAfter: tDeletionPoll}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + if stub.deleteCalls != 1 { + t.Errorf("deleteCalls = %d, want 1", stub.deleteCalls) + } + p := getProxy(t, r) + if p.Status.Phase != crawlv1alpha1.PhaseDeleting { + t.Errorf("phase = %s, want Deleting", p.Status.Phase) + } + }, + }, + { + name: "external proxy tracks its endpoint without a finalizer", + proxy: managedProxy(func(p *crawlv1alpha1.Proxy) { + p.Finalizers = nil + p.Spec = crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeExternal, + Endpoint: &crawlv1alpha1.EndpointSpec{Host: "203.0.113.7", Port: 8080}, + } + }), + stub: &stubProvider{}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + p := getProxy(t, r) + if p.Status.IP != "203.0.113.7" { + t.Errorf("ip = %q, want the endpoint host", p.Status.IP) + } + if len(p.Finalizers) != 0 { + t.Errorf("finalizers = %v, want none on External", p.Finalizers) + } + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonExternalEndpoint) + if stub.createCalls+stub.getCalls+stub.deleteCalls != 0 { + t.Errorf("provider was called for an External proxy") + } + }, + }, + { + name: "quota error backs off slowly without failing", + proxy: managedProxy(), + stub: &stubProvider{ + createErr: provider.Wrap(provider.ErrQuotaExceeded, "create", "stub", "", nil), + }, + wantResult: ctrl.Result{RequeueAfter: tQuotaRetry}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonQuotaExceeded) + if p.Status.Phase == crawlv1alpha1.PhaseFailed { + t.Errorf("phase = Failed, want anything but — quota is a wait, not a failure") + } + }, + }, + { + name: "permanent error latches Failed and stops calling the provider", + proxy: managedProxy(), + stub: &stubProvider{ + createErr: provider.Wrap(provider.ErrPermanent, "create", "stub", "", nil), + }, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + p := getProxy(t, r) + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonPermanentError) + if p.Status.Phase != crawlv1alpha1.PhaseFailed { + t.Errorf("phase = %s, want Failed", p.Status.Phase) + } + if res, err := doReconcile(t, r); err != nil || res != (ctrl.Result{}) { + t.Errorf("second reconcile = %+v, %v; want empty result, nil", res, err) + } + if stub.createCalls != 1 { + t.Errorf("createCalls = %d after latch, want 1", stub.createCalls) + } + }, + }, + { + name: "transient error is returned for workqueue backoff", + proxy: managedProxy(), + stub: &stubProvider{ + createErr: provider.Wrap(provider.ErrTransient, "create", "stub", "", nil), + }, + wantResult: ctrl.Result{}, + wantErr: true, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + if p.Status.Phase != crawlv1alpha1.PhasePending { + t.Errorf("phase = %s, want still Pending", p.Status.Phase) + } + }, + }, + { + name: "unconfigured provider is a permanent failure", + proxy: managedProxy(func(p *crawlv1alpha1.Proxy) { + p.Spec.Provider = "no-such-provider" + }), + stub: &stubProvider{}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + p := getProxy(t, r) + assertCondition(t, p, crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonPermanentError) + if p.Status.Phase != crawlv1alpha1.PhaseFailed { + t.Errorf("phase = %s, want Failed", p.Status.Phase) + } + }, + }, + { + name: "cloud-init secret is resolved into the create request", + proxy: managedProxy(func(p *crawlv1alpha1.Proxy) { + p.Spec.CloudInit = &crawlv1alpha1.CloudInitSpec{ + SecretRef: &crawlv1alpha1.SecretKeySelector{Name: "ci-secret"}, + } + }), + extraObjs: []client.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "ci-secret", Namespace: testNamespace}, + Data: map[string][]byte{"user-data": []byte("#cloud-config\npackages: [squid]")}, + }, + }, + stub: &stubProvider{createID: "stub-id-1"}, + wantResult: ctrl.Result{RequeueAfter: tProvisioningPoll}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + if want := "#cloud-config\npackages: [squid]"; stub.lastCreate.CloudInit != want { + t.Errorf("CreateRequest.CloudInit = %q, want the resolved secret content", stub.lastCreate.CloudInit) + } + }, + }, + { + name: "missing cloud-init secret errors and marks the condition", + proxy: managedProxy(func(p *crawlv1alpha1.Proxy) { + p.Spec.CloudInit = &crawlv1alpha1.CloudInitSpec{ + SecretRef: &crawlv1alpha1.SecretKeySelector{Name: "absent"}, + } + }), + stub: &stubProvider{}, + wantResult: ctrl.Result{}, + wantErr: true, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + assertCondition(t, getProxy(t, r), crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonCloudInitError) + if stub.createCalls != 0 { + t.Errorf("createCalls = %d, want 0 with unresolved cloud-init", stub.createCalls) + } + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + objs := append([]client.Object{tc.proxy}, tc.extraObjs...) + r := newTestReconciler(t, tc.stub, objs...) + res, err := doReconcile(t, r) + if (err != nil) != tc.wantErr { + t.Fatalf("Reconcile error = %v, wantErr %v", err, tc.wantErr) + } + if res != tc.wantResult { + t.Errorf("Result = %+v, want %+v", res, tc.wantResult) + } + tc.verify(t, r, tc.stub) + }) + } +} + +func assertProxyGone(t *testing.T, r *ProxyReconciler) { + t.Helper() + var p crawlv1alpha1.Proxy + err := r.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: testProxyName}, &p) + if !apierrors.IsNotFound(err) { + t.Errorf("proxy still exists (err=%v), want NotFound after finalizer removal", err) + } +} diff --git a/internal/controller/spechash.go b/internal/controller/spechash.go new file mode 100644 index 0000000..cdd18f7 --- /dev/null +++ b/internal/controller/spechash.go @@ -0,0 +1,41 @@ +package controller + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" +) + +// specHashInput is the explicit set of fields whose change requires +// replacing the instance. Deliberately not ProxySpec wholesale: adding a +// spec field that doesn't affect the VM (attributes, maxLeases, healthCheck) +// must not churn the fleet on operator upgrade. +type specHashInput struct { + Placement *crawlv1alpha1.PlacementSpec `json:"placement,omitempty"` + CloudInit string `json:"cloudInit,omitempty"` + Port int32 `json:"port"` +} + +// specHash returns the hex SHA-256 of the replacement-triggering spec +// fields. cloudInit is the already-resolved content, so rotating a +// referenced Secret changes the hash even though the spec is untouched. +func specHash(p *crawlv1alpha1.Proxy, cloudInit string) string { + in := specHashInput{ + Placement: p.Spec.Placement, + CloudInit: cloudInit, + Port: p.EffectivePort(), + } + // nil and empty placement mean the same thing; hash them identically. + if in.Placement != nil && *in.Placement == (crawlv1alpha1.PlacementSpec{}) { + in.Placement = nil + } + b, err := json.Marshal(in) + if err != nil { + // A struct of strings and an int32 cannot fail to marshal. + panic(err) + } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]) +} diff --git a/internal/controller/spechash_test.go b/internal/controller/spechash_test.go new file mode 100644 index 0000000..e4a8736 --- /dev/null +++ b/internal/controller/spechash_test.go @@ -0,0 +1,118 @@ +package controller + +import ( + "regexp" + "testing" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" +) + +func hashProxy(mut ...func(*crawlv1alpha1.Proxy)) *crawlv1alpha1.Proxy { + p := &crawlv1alpha1.Proxy{ + Spec: crawlv1alpha1.ProxySpec{ + Mode: crawlv1alpha1.ModeManaged, + Provider: "stub", + Port: 3128, + Placement: &crawlv1alpha1.PlacementSpec{ + Zone: "europe-west1-b", + MachineType: "e2-micro", + }, + }, + } + for _, m := range mut { + m(p) + } + return p +} + +func TestSpecHash_stability(t *testing.T) { + t.Parallel() + + p := hashProxy() + h1 := specHash(p, "cloud-init-content") + h2 := specHash(p.DeepCopy(), "cloud-init-content") + if h1 != h2 { + t.Errorf("same input hashed differently: %s vs %s", h1, h2) + } + if !regexp.MustCompile(`^[0-9a-f]{64}$`).MatchString(h1) { + t.Errorf("hash %q is not hex SHA-256", h1) + } + + // Fields outside the replacement set must not affect the hash — that is + // the whole point of an explicit hash-input struct. + q := hashProxy(func(p *crawlv1alpha1.Proxy) { + p.Spec.Attributes = map[string]string{"geo": "eu"} + five := int32(5) + p.Spec.MaxLeases = &five + p.Spec.HealthCheck = &crawlv1alpha1.HealthCheckSpec{IntervalSeconds: 60} + }) + if specHash(q, "cloud-init-content") != h1 { + t.Error("non-replacement spec fields changed the hash") + } +} + +func TestSpecHash_normalization(t *testing.T) { + t.Parallel() + + nilPlacement := hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Placement = nil }) + emptyPlacement := hashProxy(func(p *crawlv1alpha1.Proxy) { + p.Spec.Placement = &crawlv1alpha1.PlacementSpec{} + }) + if specHash(nilPlacement, "") != specHash(emptyPlacement, "") { + t.Error("nil and empty placement hashed differently") + } + + // An unset port and an explicit default port mean the same instance. + unsetPort := hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Port = 0 }) + defaultPort := hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Port = crawlv1alpha1.DefaultPort }) + if specHash(unsetPort, "") != specHash(defaultPort, "") { + t.Error("unset port and explicit default port hashed differently") + } +} + +func TestSpecHash_sensitivity(t *testing.T) { + t.Parallel() + + base := specHash(hashProxy(), "cloud-init") + + tests := []struct { + name string + proxy *crawlv1alpha1.Proxy + cloudInit string + }{ + { + name: "port change", + proxy: hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Port = 8080 }), + cloudInit: "cloud-init", + }, + { + name: "zone change", + proxy: hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Placement.Zone = "us-east1-c" }), + cloudInit: "cloud-init", + }, + { + name: "machine type change", + proxy: hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Placement.MachineType = "e2-small" }), + cloudInit: "cloud-init", + }, + { + name: "image change", + proxy: hashProxy(func(p *crawlv1alpha1.Proxy) { p.Spec.Placement.Image = "debian-13" }), + cloudInit: "cloud-init", + }, + { + name: "resolved cloud-init change (secret rotation)", + proxy: hashProxy(), + cloudInit: "rotated-cloud-init", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if specHash(tc.proxy, tc.cloudInit) == base { + t.Error("hash did not change") + } + }) + } +} diff --git a/internal/controller/status.go b/internal/controller/status.go new file mode 100644 index 0000000..3940c85 --- /dev/null +++ b/internal/controller/status.go @@ -0,0 +1,83 @@ +package controller + +import ( + "context" + + "k8s.io/apimachinery/pkg/api/equality" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" +) + +// Reasons used on the Provisioned condition. The Healthy condition is owned +// by the health engine (internal/health) and only represented here. +const ( + ReasonProvisioning = "Provisioning" + ReasonCreated = "Created" + ReasonReplacing = "Replacing" + ReasonRecreating = "Recreating" + ReasonQuotaExceeded = "QuotaExceeded" + ReasonPermanentError = "PermanentError" + ReasonCloudInitError = "CloudInitError" + ReasonExternalEndpoint = "ExternalEndpoint" + ReasonDeleting = "Deleting" +) + +// setProvisioned stages the Provisioned condition on p. Nothing is written +// to the API server here; the deferred patch in Reconcile flushes it. +// ObservedGeneration is passed explicitly — SetStatusCondition does not +// populate it, and without it every condition would report generation 0. +func setProvisioned(p *crawlv1alpha1.Proxy, status metav1.ConditionStatus, reason, message string) { + apimeta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{ + Type: crawlv1alpha1.ConditionProvisioned, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: p.Generation, + }) +} + +// computePhase derives status.phase from deletionTimestamp and the +// Provisioned/Healthy conditions. Pure, so the truth table is unit-testable. +func computePhase(p *crawlv1alpha1.Proxy) crawlv1alpha1.ProxyPhase { + if !p.DeletionTimestamp.IsZero() { + return crawlv1alpha1.PhaseDeleting + } + prov := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionProvisioned) + if prov == nil { + return crawlv1alpha1.PhasePending + } + if prov.Status != metav1.ConditionTrue { + // Quota exhaustion is a slow-retry wait, not a terminal state — only + // a permanent error latches Failed. + if prov.Reason == ReasonPermanentError { + return crawlv1alpha1.PhaseFailed + } + return crawlv1alpha1.PhaseProvisioning + } + healthy := apimeta.FindStatusCondition(p.Status.Conditions, crawlv1alpha1.ConditionHealthy) + switch { + case healthy == nil || healthy.Status == metav1.ConditionUnknown: + // Provisioned but no health verdict yet: still being brought into + // service. + return crawlv1alpha1.PhaseProvisioning + case healthy.Status == metav1.ConditionTrue: + return crawlv1alpha1.PhaseReady + default: + return crawlv1alpha1.PhaseUnhealthy + } +} + +// patchStatusIfChanged recomputes the derived status fields and issues one +// status patch — or none, when nothing changed. This is the only place the +// reconciler writes status. +func (r *ProxyReconciler) patchStatusIfChanged(ctx context.Context, base, p *crawlv1alpha1.Proxy) error { + p.Status.ObservedGeneration = p.Generation + p.Status.Phase = computePhase(p) + if equality.Semantic.DeepEqual(base.Status, p.Status) { + return nil + } + return r.Status().Patch(ctx, p, client.MergeFrom(base)) +} diff --git a/internal/controller/status_test.go b/internal/controller/status_test.go new file mode 100644 index 0000000..05843e8 --- /dev/null +++ b/internal/controller/status_test.go @@ -0,0 +1,112 @@ +package controller + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + crawlv1alpha1 "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/api/v1alpha1" +) + +func TestComputePhase_truthTable(t *testing.T) { + t.Parallel() + + cond := func(condType string, status metav1.ConditionStatus, reason string) metav1.Condition { + return metav1.Condition{Type: condType, Status: status, Reason: reason} + } + + tests := []struct { + name string + deleting bool + conditions []metav1.Condition + want crawlv1alpha1.ProxyPhase + }{ + { + name: "deletionTimestamp wins over everything", + deleting: true, + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonCreated), + cond(crawlv1alpha1.ConditionHealthy, metav1.ConditionTrue, "Probing"), + }, + want: crawlv1alpha1.PhaseDeleting, + }, + { + name: "no conditions is Pending", + want: crawlv1alpha1.PhasePending, + }, + { + name: "provisioning in progress", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonProvisioning), + }, + want: crawlv1alpha1.PhaseProvisioning, + }, + { + name: "replacing counts as provisioning", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonReplacing), + }, + want: crawlv1alpha1.PhaseProvisioning, + }, + { + name: "quota exhaustion is not Failed", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonQuotaExceeded), + }, + want: crawlv1alpha1.PhaseProvisioning, + }, + { + name: "permanent error is Failed", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionFalse, ReasonPermanentError), + }, + want: crawlv1alpha1.PhaseFailed, + }, + { + name: "provisioned without a health verdict stays Provisioning", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonCreated), + }, + want: crawlv1alpha1.PhaseProvisioning, + }, + { + name: "provisioned with Healthy Unknown stays Provisioning", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonCreated), + cond(crawlv1alpha1.ConditionHealthy, metav1.ConditionUnknown, "NoProbeYet"), + }, + want: crawlv1alpha1.PhaseProvisioning, + }, + { + name: "provisioned and healthy is Ready", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonCreated), + cond(crawlv1alpha1.ConditionHealthy, metav1.ConditionTrue, "Probing"), + }, + want: crawlv1alpha1.PhaseReady, + }, + { + name: "provisioned but unhealthy is Unhealthy", + conditions: []metav1.Condition{ + cond(crawlv1alpha1.ConditionProvisioned, metav1.ConditionTrue, ReasonCreated), + cond(crawlv1alpha1.ConditionHealthy, metav1.ConditionFalse, "ProbeFailed"), + }, + want: crawlv1alpha1.PhaseUnhealthy, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + p := &crawlv1alpha1.Proxy{} + p.Status.Conditions = tc.conditions + if tc.deleting { + now := metav1.Now() + p.DeletionTimestamp = &now + } + if got := computePhase(p); got != tc.want { + t.Errorf("computePhase() = %s, want %s", got, tc.want) + } + }) + } +} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 54fc19b..36e9c89 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -49,6 +49,9 @@ var ( ) func TestControllers(t *testing.T) { + if testing.Short() { + t.Skip("skipping envtest suite in -short mode") + } RegisterFailHandler(Fail) RunSpecs(t, "Controller Suite")