diff --git a/README.md b/README.md index df3df65..271f9fa 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,8 @@ make deploy IMG=/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 -[internal/provider/provider.go](internal/provider/provider.go): - -1. **Implement the 4-method `Provider` interface** in a new - `internal/provider//` package: - - `Create` submits and returns — it never blocks until the VM runs, and - 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=`, 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) (`"": .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. +The contract, the rules each caller depends on, a feature matrix of the +existing providers and a step-by-step checklist are in +[docs/providers.md](docs/providers.md). Start there: several rules (a +self-contained `providerID`, `Get` returning `ErrNotFound` only when the +instance is really gone, `CreatedAt` read back for orphan GC) are enforced by +callers, not by the interface. ## Tracing diff --git a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md index 97fef0b..e7f961b 100644 --- a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md +++ b/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. diff --git a/docs/providers.md b/docs/providers.md index 94c6b6b..c94edaf 100644 --- a/docs/providers.md +++ b/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://:`: + +- 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 - NotFound (`:305-311`). GC and the reconciler can also race. + *Why:* GC and the reconciler can race, and a crash can repeat a call. +- **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 - (`metrics.go:15-74`). +- `provider.WithMetrics` counts each call as + `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_.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/-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/`. -12. **Optional:** a demo script under `docs/demo/`. + - A row in README.md "Implemented providers". +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.