Fill the provider blueprint's gaps after a full sweep

A pass over the API types, spec hash, health and discovery call sites,
manager manifests, samples, README, provider docs, plans and the PR review
found rules the blueprint did not state: the IP must be reachable on the
proxy port by the health engine and clients, providers must be safe for
concurrent use, the manager's read-only/128Mi runtime envelope, catalogue
edits bypass the spec hash, Delete is re-called while in progress, and
provider names must be DNS-1123. Adds matrix rows, a deployment checklist
item, and the still-open review findings and broken samples as gaps.

README's "Adding a provider" restated an outdated contract; it now points
to docs/providers.md, and the providers table gains the kvm row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

README.md

@@ -104,7 +104,8 @@ make deploy IMG=<registry>/egress-proxies-operator:dev
 
 - Provider config comes from the `providers-config` ConfigMap
   ([config/manager/providers_config.yaml](config/manager/providers_config.yaml));
-  the default ships only the kubernetes provider.
+  the default ships the kubernetes provider plus a `kvm-home` entry that needs
+  the `kvm-ssh` Secret (without it the manager starts and warns).
 - The discovery API is exposed by the
   `controller-manager-discovery-service` Service on port 8090.
 - Auth: create the token Secret, or the API serves **unauthenticated**
@@ -185,46 +186,16 @@ can be two differently-configured instances of the same `gcp` type (see
 | --- | --- | --- | --- |
 | `kubernetes` | A real Squid pod (`ubuntu/squid:6.6-24.04_edge` by default) in the same cluster the operator runs in | `image` (optional) | Needs no cloud account — local dev, CI, and the kind quickstart. Pods share the cluster's egress IP, so it exercises the full lifecycle but not distinct egress paths. |
 | `gcp` | A Compute Engine VM with an ephemeral external IP | `project` (required), `network`, `networkTag`, `diskSizeGb` | The real egress fleet. Deliberately uses only four API calls (Insert / Get / Delete / AggregatedList), all fire-and-forget: `Create` returns as soon as the operation is submitted and the reconciler discovers progress by polling `Get`. Auth is Application Default Credentials — workload identity in-cluster, `gcloud` ADC locally; no key-file plumbing. |
+| `kvm` | A libvirt VM on one of your own hypervisors, reached over SSH | `sshKeyFile`, `knownHostsFile` (unless `insecureIgnoreHostKey`), `machineTypes`, `hosts` (required); `images`, `diskSizeGb`, `vendorData`, `installGuestAgent` | Home-lab or on-prem egress. The base image is downloaded into each host's pool on first use; `Create` waits for the VM to start. See [docs/kvm-provider.md](docs/kvm-provider.md). |
 
 ### Adding a provider
 
-A new backend (Hetzner, AWS, ...) is four pieces; the contract lives in
+The contract, the rules each caller depends on, a feature matrix of the
-[internal/provider/provider.go](internal/provider/provider.go):
+existing providers and a step-by-step checklist are in
-
+[docs/providers.md](docs/providers.md). Start there: several rules (a
-1. **Implement the 4-method `Provider` interface** in a new
+self-contained `providerID`, `Get` returning `ErrNotFound` only when the
-   `internal/provider/<type>/` package:
+instance is really gone, `CreatedAt` read back for orphan GC) are enforced by
-   - `Create` submits and returns — it never blocks until the VM runs, and
+callers, not by the interface.
-     must be idempotent keyed on `req.Name` (a deterministic name derived
-     from the Proxy's UID), so a repeat call after a crash finds the
-     existing instance instead of duplicating it.
-   - `Get` returns `provider.ErrNotFound` as a *normal* outcome — the
-     reconciler branches on it for replacement and adoption, so don't
-     treat it as exceptional.
-   - `Delete` is idempotent: deleting an already-gone instance is not an
-     error.
-   - `ListByTag` returns every instance the operator ever tagged, for
-     orphan GC.
-2. **Tag every created resource** with `LabelManaged=true` and
-   `LabelUID=<Proxy UID>`, and report `CreatedAt` — orphan GC relies on
-   all three to find owned resources and skip in-flight creates.
-3. **Classify every returned error** with `provider.Wrap` into the
-   four-sentinel taxonomy in
-   [internal/provider/errors.go](internal/provider/errors.go)
-   (`ErrNotFound` / `ErrQuotaExceeded` / `ErrTransient` / `ErrPermanent`)
-   — the reconciler decides retry, slow backoff, or latching `Failed`
-   purely from that classification, never from provider-specific types.
-4. **Wire it up**: add a type-specific config block in
-   [internal/provider/config.go](internal/provider/config.go), and
-   register the constructor in the builtins map in
-   [cmd/main.go](cmd/main.go) (`"<type>": <pkg>.New`). The registry
-   ([internal/provider/registry](internal/provider/registry/registry.go))
-   handles named instances, and the metrics wrapper is applied
-   automatically.
-
-Test against a fake API seam rather than the real cloud — see the
-`instancesAPI` seam in
-[internal/provider/gcp/gcp.go](internal/provider/gcp/gcp.go) for the
-pattern.
 
 ## Tracing
 

docs/plans-executions/2026-09-16-0841-provider-blueprint.md

@@ -126,3 +126,52 @@ operation level). A proposal notes that the kvm `libvirtAPI` seam takes no
 `ctx`, so per-RPC spans need that plumbed in.
 
 The links were re-checked, including the new `architecture.md#10-tracing-internaltracing` anchor.
+
+### Follow-up — a completeness sweep
+
+The user asked for one more pass over everything provider-related. After the
+tracing miss, this pass read beyond the provider packages:
+- the Proxy API types and the spec hash;
+- the health engine and discovery call sites that consume `Instance.IP`;
+- the manager Deployment, the providers ConfigMap and the samples;
+- README, testing.md, kvm-provider.md and the GCP docs;
+- the initial prompt, the proxy-operator and kvm plans, and the 2026-08-10 PR
+  review;
+- the CHANGELOG.
+
+What the blueprint was missing, now added:
+- **What the instance is for.** The IP and port must be reachable from the
+  operator's health probes and from discovery clients. Providers don't know
+  which proxy runs; the proxy comes from cloud-init, except for kubernetes.
+- **Concurrency.** Three reconciles, GC and checks can call a provider at once,
+  so it MUST be safe for concurrent use.
+- **The runtime envelope:** read-only root filesystem, a 64Mi `/tmp`, a 128Mi
+  memory limit, credentials as optional mounts.
+- **Create:** `placement` can't be validated by the CRD, so error messages must
+  name the field. Catalogue edits in config aren't in the spec hash.
+- **Delete** is re-called every `DeletionPoll` while a delete is in progress.
+- **Provider names** must be DNS-1123 for `ProviderHealth`, and `spec.provider`
+  is immutable.
+- **Metric names** in §6.
+- **Three matrix rows:** proxy software, IP reachability, distinct egress.
+- **A deployment checklist item:** credentials mount, config entries, sample
+  Proxy, Makefile helper.
+- **Gaps:**
+  - still open from the PR review: GC has no installation identity; an
+    unconfigured provider latches Proxies; kubernetes Pod RBAC always installed
+    and no `OwnerReference` on Pods;
+  - the duplicated type list;
+  - samples that can't work: `proxy_gcp.yaml` uses a debian-cloud image without
+    cloud-init, and `proxy_kvm.yaml` places on `nas1` where configs define
+    `beelink`;
+  - the `"mock"` example in the `spec.provider` doc.
+
+Outside the plan's "leave other docs alone" line, README.md changed:
+- Its "Adding a provider" section restated the contract, with the old "Create
+  never blocks" rule that kvm contradicts. It now points to `docs/providers.md`
+  instead of keeping a second copy that disagrees.
+- The "Implemented providers" table gained the missing kvm row.
+- The deploy note no longer claims the default ConfigMap ships only the
+  kubernetes provider.
+
+The links were re-checked for providers.md and README.md.

docs/providers.md

@@ -42,7 +42,54 @@ polling `Get` (`ProvisioningPoll` and `DeletionPoll` 10s, `RequeueNow` 1s,
 Proxies reconcile at once (`MaxConcurrentReconciles: 3`, `:442`).
 
 Providers are named instances of a type. `gcp-eu` and `gcp-us` can both have
-type `gcp`. The name is what `Proxy.spec.provider` refers to.
+type `gcp`. The name is what `Proxy.spec.provider` refers to. `spec.provider` is
+immutable, so a Proxy never moves between providers.
+
+**What the instance is for.** It is a forward proxy with its own egress path.
+Two parties connect to `http://<Instance.IP>:<spec.port>`:
+
+- The health engine, from the operator pod, sends a real `CONNECT` through it
+  (`internal/health/engine.go:226`).
+- Crawler clients, which get `ip` and `port` from the discovery API
+  (`internal/discovery/handlers.go:37`).
+
+So the IP a provider reports MUST be reachable on that port from the operator's
+pod network and from the clients. That may require firewalls, security groups
+or routing: gcp needs a manual firewall rule whose source range covers the
+cluster's egress IP ([gcp-vm-validation.md](gcp-vm-validation.md)).
+
+Providers don't know which proxy software runs. The proxy is installed and
+configured by the Proxy's `cloudInit`, and building install logic into the
+operator is an explicit non-goal (`docs/prompts/__initial-prompt.md` §12).
+kubernetes is the exception: a Pod has no cloud-init, so it generates a Squid
+config from `Port` itself.
+
+**Who calls, and from where.**
+
+- **Leader only:** the reconciler, orphan GC, the health engine and the provider
+  health monitor.
+- **Every replica, at startup:** the constructors run in `cmd/main.go` before
+  the manager starts.
+- **Concurrently:** up to 3 reconciles, a GC sweep and a health-check round can
+  call the same provider at the same time. A provider **MUST be safe for
+  concurrent use**, including two calls for the same instance name. Nothing
+  above the provider serialises them.
+
+**Where it runs.** The manager container runs:
+
+- as non-root, with a **read-only root filesystem**
+  (`config/manager/manager.yaml:57`, `:122`);
+- with a 64Mi `emptyDir` at `/tmp` (`:176`);
+- with a **128Mi memory limit** (`:144`).
+
+So a provider:
+
+- MUST stream large payloads rather than buffering them (kvm streams image
+  downloads, `image.go:182`);
+- MUST write scratch files only under `/tmp`;
+- gets credentials either as mounted files (the providers ConfigMap at
+  `/etc/proxy-operator`, optional Secret mounts such as `kvm-ssh`) or from the
+  environment (ADC, the in-cluster service account).
 
 ## 2. The contract, method by method
 
@@ -91,6 +138,16 @@ Rules:
 - **SHOULD reject a request it can never satisfy** with `ErrPermanent` before
   touching the backend: a missing zone, machine type or image, or a name that is
   not in its catalogue (gcp `gcp.go:137-141`, kvm `kvm.go:126-144`, `:160-164`).
+  The CRD can't validate `placement`, because it is provider-agnostic, so this
+  error is the user's only feedback. The message SHOULD name the offending field
+  and value.
+- **Catalogue changes don't replace instances.** The spec hash covers
+  `placement`, the resolved `cloudInit` and `port` (`spechash.go:15-19`), but not
+  the provider config. When a placement value names a config entry (kvm's
+  `images` and `machineTypes`), changing what that entry means affects only new
+  instances. For kvm it can even corrupt running VMs
+  ([kvm-provider.md § The image cache](kvm-provider.md#the-image-cache)). Document
+  this, and tell users to add a new key rather than edit one in use.
 - **MAY ignore `Placement` fields that don't apply** (`provider.go:56-57`). If a
   field is ignored, the provider's doc page MUST say so, because the fields are
   still part of the spec hash. Changing an ignored field replaces the instance
@@ -145,8 +202,12 @@ Rules:
 ### Delete(ctx, providerID) error
 
 - **MUST be idempotent.** Deleting something that is already gone returns nil.
-  *Why:* the reconciler calls `Delete` again on every poll until `Get` returns
+  *Why:* GC and the reconciler can race, and a crash can repeat a call.
-  NotFound (`:305-311`). GC and the reconciler can also race.
+- **MUST accept being called again while an earlier delete is still in
+  progress.** Treat "already deleting" as success, and SHOULD keep that cheap.
+  *Why:* while `Get` still returns the instance, the reconciler calls `Delete`
+  again on every `DeletionPoll` (10s) until `Get` returns NotFound (`:305-311`).
+  gcp submits a new delete operation each time.
 - **MUST remove everything `Create` made**: disks, volumes, seeds, addresses.
   Either delete them explicitly or rely on the backend doing it (gcp
   `AutoDelete` on the boot disk).
@@ -237,6 +298,11 @@ Pitfalls:
 - **`CreatedAt`:** read it from the backend's own creation timestamp. If the
   backend has none, write it into the metadata at creation (kvm:
   `kvm.go:277-298`).
+- **Provider names:** keep a configured `name` a lowercase DNS-1123 label.
+  `spec.provider` allows at most 63 characters (`proxy_types.go:236`). The
+  `ProviderHealth` object is named after the provider, and the monitor skips
+  writing it for a name that isn't a valid object name (`monitor.go:141`,
+  `:248`). The metrics still work. Config loading doesn't check this yet (§9).
 
 ## 5. Optional interfaces: `Checker`
 
@@ -279,8 +345,11 @@ A provider has three layers to think about; the first comes for free.
   `provider.id` and `provider.result` (the error class) as attributes
   (`internal/provider/tracing.go:27-80`). `ErrNotFound` keeps the span status
   Ok, because polling to NotFound is expected.
-- `provider.WithMetrics` counts each call by provider, op and result
+- `provider.WithMetrics` counts each call as
-  (`metrics.go:15-74`).
+  `proxy_operator_provider_requests_total{provider,op,result}`
+  (`metrics.go:15-74`, `internal/metrics/metrics.go:40-43`). A `Checker` feeds
+  `proxy_operator_provider_target_up`, `_check_duration_seconds` and
+  `_check_timestamp_seconds`, labelled `{provider,target}`.
 - The spans sit under `Reconcile Proxy → reconcile.managed |
   reconcile.replaceInstance | reconcile.delete`, or under `gc.sweep`.
 
@@ -363,6 +432,9 @@ the per-provider notes below the table.
 | Tracing: child spans per backend call | yes: parent-gated transport | yes: SDK's otelhttp | **no** |
 | Unit tests with a fake | yes: controller-runtime fake client | yes: `fakeAPI` + httptest | yes: `fakeAPI` + in-process SSH server |
 | Real-backend test | kind e2e | manual runbook | `//go:build integration` |
+| Proxy software | generated Squid config (`pod.go:60-75`) | from `cloudInit`; image must run cloud-init (debian-cloud doesn't) | from `cloudInit`, plus the guest-agent vendor-data |
+| IP reachable from the operator | Pod network, so in-cluster only (not from `make run-dev` on a laptop) | external IP, once a firewall rule allows the cluster's egress IP | LAN address on the host's bridge; the operator needs a route to that LAN |
+| Distinct egress IP per proxy | no: the cluster's egress | yes: ephemeral external IP | whatever the host's LAN routes through; not per VM by itself |
 
 ### kubernetes notes (`internal/provider/kubernetes`)
 
@@ -496,15 +568,23 @@ the per-provider notes below the table.
     (`internal/controller/proxy_controller.go:86-88`); `controller-gen` scans
     `./...`, so either place works. Then run `make manifests` and check
     `git diff config/rbac/role.yaml`.
-11. **Docs:**
+11. **Deployment:**
+    - A credentials mount in `config/manager/manager.yaml`, marked
+      `optional: true` so the manager still starts without it (see `kvm-ssh`).
+    - A commented entry in `config/manager/providers_config.yaml` and
+      `config/samples/providers-config.yaml`.
+    - A `config/samples/proxy_<type>.yaml` with a cloud-init that actually
+      installs a proxy on the sample image.
+    - Optionally, a Makefile helper for the Secret (`make kvm-ssh-secret`).
+12. **Docs:**
     - A `docs/<type>-provider.md` covering prerequisites, config reference,
       placement mapping, what lands on the backend, the health check, and known
       gaps.
     - A row in the architecture.md components table, and a block in §5 and
       §11. If the provider adds spans, update the span topology in §10.
     - A column in the §7 matrix above.
-    - A sample in `config/samples/`.
+    - A row in README.md "Implemented providers".
-12. **Optional:** a demo script under `docs/demo/`.
+13. **Optional:** a demo script under `docs/demo/`.
 
 ## 9. Gaps and proposed features
 
@@ -529,6 +609,22 @@ These are listed only. Nothing below is designed or implemented.
   credentials, needs a manager restart.
 - **Stale comments.** `provider.go:3-4` and `registry/registry.go:5-12` still
   describe a `mock` subpackage, which was replaced by the kubernetes provider.
+  `spec.provider`'s doc (`proxy_types.go:232`, and so the generated CRD) still
+  gives `"mock"` as an example.
+- **GC doesn't know about installations.** Two clusters sharing one GCP project
+  or hypervisor delete each other's instances, because GC only knows its own
+  live UIDs. This was a merge-blocker in the 2026-08-10 PR review
+  ([docs/reviews](reviews/2026-08-10-1134-proxy-operator-pr-review.md)) and is
+  still open.
+- **An unconfigured provider latches the Proxy.** A Proxy naming a provider
+  missing from the config is latched `PermanentError`. Adding the provider and
+  restarting doesn't change `metadata.generation`, and `spec.provider` is
+  immutable, so the Proxy stays latched until it is recreated (same review).
+- **Provider names aren't validated** as DNS-1123 at config load (§4).
+- **The type list exists twice:** the config allow-list (`config.go:102-108`)
+  and the constructor map in `cmd/main.go:195-205` must be kept in step by hand.
+- **Delete is resubmitted on every deletion poll** (§2). For gcp that is a new
+  API operation every 10s until the VM is gone.
 
 **kubernetes**
 
@@ -542,6 +638,9 @@ These are listed only. Nothing below is designed or implemented.
 - **Unguarded Pods.** No securityContext, no resources, and
   `http_access allow all` (`pod.go`).
 - **No logging** in the package, and **no Checker `Details`**.
+- **No `OwnerReference` on Pods.** A bypassed finalizer leaves the Pod behind.
+- **Pod RBAC is always installed:** cluster-wide `pods` create/delete ships even
+  when no kubernetes provider is configured (`proxy_controller.go:86-88`).
 
 **Tracing**
 
@@ -567,6 +666,10 @@ These are listed only. Nothing below is designed or implemented.
 - **The SDK client is never closed**, and `Placement.Region` is unused.
 - **Wrong label value in the docs.** `gcp-vm-validation.md` filters on
   `labels.proxy-operator-managed=yes`; the value is `true`.
+- **The sample can't work.** `config/samples/proxy_gcp.yaml` uses a
+  `debian-cloud` image, which has no cloud-init, so its Squid install never runs
+  ([gcp-in-specific-project.md](gcp-in-specific-project.md) uses Ubuntu for that
+  reason).
 
 **kvm** (see also [kvm-provider.md § Known gaps](kvm-provider.md#known-gaps))
 
@@ -585,6 +688,8 @@ These are listed only. Nothing below is designed or implemented.
   during the call.
 - **No reused-overlay check.** An existing overlay is reused without checking
   its backing image or size.
+- **Sample mismatch.** `config/samples/proxy_kvm.yaml` places on host `nas1`,
+  while both providers configs define only `beelink`.
 - **Doc drift in kvm-provider.md:**
   - It says ARP is the fallback; the code tries ARP first.
   - It says `host` and `name` are on every log line; the check and list paths
@@ -614,3 +719,12 @@ These are listed only. Nothing below is designed or implemented.
 - **Checker `Details`** for kubernetes (server version) and gcp (quota
   headroom).
 - **Internal-IP mode** for gcp, and a firewall rule managed per provider.
+- **An installation-identity mark**, set on create and filtered on in
+  `ListByTag`, so installations sharing a backend don't collect each other's
+  instances.
+- **A permanent-failure latch that sees provider availability**, so adding a
+  missing provider releases the Proxies waiting on it.
+- **Validate provider names at load**, and derive the config type allow-list
+  from the constructor map.
+- **Per-provider RBAC** as an optional kustomize component, and an
+  `OwnerReference` on kubernetes Pods.