From d96e56f1a0a822cb8fad038af0202e35b190fcfa Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 08:54:40 +0000 Subject: [PATCH 01/11] Write the provider blueprint docs/providers.md collects the provider contract that was spread across comments, callers and per-provider docs: MUST/SHOULD/MAY rules per method, each tied to the reconciler or GC behaviour it protects, the error taxonomy, a feature matrix for kubernetes/gcp/kvm with file:line notes, a checklist for adding a provider, and the known gaps. Linked from architecture.md and CLAUDE.md. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 7 +- docs/architecture.md | 2 +- .../2026-09-16-0841-provider-blueprint.md | 70 +++ docs/providers.md | 520 ++++++++++++++++++ 4 files changed, 595 insertions(+), 4 deletions(-) create mode 100644 docs/plans-executions/2026-09-16-0841-provider-blueprint.md create mode 100644 docs/providers.md diff --git a/CLAUDE.md b/CLAUDE.md index 08b04a9..62b750b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,9 +27,10 @@ entrypoint yet, so there's nothing to run locally. Documented in [docs/architecture.md](docs/architecture.md). It holds the component table, the reconcile flow, and one section per runnable: health engine, discovery and -lease API, orphan GC, metrics, tracing, and provider health checks. Provider-specific -detail is in `docs/kvm-provider.md` and the GCP docs; the test layout is in -`docs/testing.md`. +lease API, orphan GC, metrics, tracing, and provider health checks. The provider +contract (MUST/SHOULD rules, feature matrix, checklist for adding a provider, known +gaps) is in [docs/providers.md](docs/providers.md). Provider-specific detail is in +`docs/kvm-provider.md` and the GCP docs; the test layout is in `docs/testing.md`. ## Conventions diff --git a/docs/architecture.md b/docs/architecture.md index c847d55..983d69c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -6,7 +6,7 @@ |---|---|---|---|---| | Proxy + ProviderHealth CRDs | `api/v1alpha1` | types | — | `Proxy` spec/status, CEL validation, defaulting, pure helpers; the status-only, cluster-scoped `ProviderHealth` | | Reconciler | `internal/controller` | controller | yes (with the manager) | the state machine: provision, replace, delete, represent health | -| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator | +| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator; rules and per-provider matrix in [providers.md](providers.md) | | kubernetes provider | `internal/provider/kubernetes` | library | — | real Squid pods in this cluster (local dev/CI) | | gcp provider | `internal/provider/gcp` | library | — | Compute Engine VMs, four API calls, fire-and-forget ops | | kvm provider | `internal/provider/kvm` | library | — | VMs on libvirt hosts over SSH-forwarded sockets; image cache, overlay disks, NoCloud seeds | diff --git a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md new file mode 100644 index 0000000..b5c3589 --- /dev/null +++ b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md @@ -0,0 +1,70 @@ +# Execution: provider blueprint +Plan: [docs/plans/2026-09-16-0841-provider-blueprint.md](../plans/2026-09-16-0841-provider-blueprint.md) + +- [x] Step 0 — Save the plan; wait for the merge +- [x] Step 1 — Collect evidence (read-only, in parallel) +- [x] Step 2 — Write `docs/providers.md` +- [ ] Step 3 — Verify, then open the MR + +## Step 0 — Save the plan; wait for the merge + +The plan was committed on `feat/provider-health` (`80b2e9c`) and reached `main` +through PR #6. After the user confirmed the merge, the branch was cut from the +updated `main`: + +```bash +git switch main && git pull --ff-only +git switch -c feat/provider-blueprint +``` + +Worth noting: `.claude/settings.json` has local, uncommitted permission edits +that carried over through the switch. They are deliberately left out of this +branch. + +## Step 1 — Collect evidence (read-only, in parallel) + +Three Explore agents answered the plan's 12-item questionnaire, one each for +kubernetes, gcp and kvm. Meanwhile I read the shared layer: +`internal/provider/*.go`, `registry.go`, the provider wiring in `cmd/main.go`, +`reconcileManaged`/`replaceInstance`/`reconcileDelete` in the controller, +`internal/gc/gc.go`, and how the monitor uses `Checker`. + +Before relying on the agents' claims, I spot-checked them against the code. One +was wrong and did not go into the doc: the kvm agent said `Delete` skips volume +removal when `DomainDestroy` returns `ErrNoDomain`. In fact `removeDomain` +returns nil there, and `Delete` still calls `removeVolumes` afterwards +(`kvm.go:477-482`). + +Rules that exist only in the callers, and not in any comment, were found by +reading the controller and GC. They went into the doc as MUST rules: +- A zero `CreatedAt` makes an instance immediately collectable (`gc.go:120`). +- `Get` returning `ErrPermanent` on the deletion path makes the reconciler call + `Delete` anyway. +- Replacement waits for `Get` to return NotFound before creating an instance + with the same name. +- `Stopped` and `Terminated` both lead to delete-and-recreate. + +Worth noting, found outside this plan's scope and not changed here: +- `docs/gcp-vm-validation.md` filters on `labels.proxy-operator-managed=yes`, + but the value is `true`. +- According to the gcp agent, `docs/gcp-in-specific-project.md` holds a real + project ID, IAM output and an egress IP. The user was told. + +## Step 2 — Write `docs/providers.md` + +Wrote the eight sections the plan lists. Linked the new doc from the Provider +contract row in `docs/architecture.md` and from the CLAUDE.md "Architecture" +section. + +Judgment calls: +- The file:line evidence sits in per-provider notes under the matrix, as + planned. The contract rules in §2 cite the reconciler and GC lines each rule + protects. +- The gap list includes documentation drift (kvm-provider.md's ARP order and + "from startup" dial, gcp-vm-validation.md's label value and its claim that + async errors reach status), as well as code gaps. Both would mislead the next + provider author in the same way. +- "Proposed features" stays a flat list with no design, as the user decided. + +Worth noting: the line numbers in the doc are a snapshot. The doc says so and +asks readers to look for the named function when a line has moved. diff --git a/docs/providers.md b/docs/providers.md new file mode 100644 index 0000000..49f7c68 --- /dev/null +++ b/docs/providers.md @@ -0,0 +1,520 @@ +# Providers: the blueprint + +This page is for whoever writes the next provider. It explains the contract a +provider must meet, which caller depends on each rule, how the three existing +providers (kubernetes, gcp, kvm) meet it, and what is still missing. + +Where the rest lives: + +- **Data flow:** the calls each provider makes are in + [architecture.md §5](architecture.md#5-what-provider-calls-do-in-the-outside-world). +- **Health checks:** the monitor that runs them is in + [architecture.md §11](architecture.md#11-provider-health-checks-internalproviderhealth). +- **Per-provider guides:** [kvm-provider.md](kvm-provider.md) and + [gcp-vm-validation.md](gcp-vm-validation.md). +- **Code:** the contract itself is + [`internal/provider/provider.go`](../internal/provider/provider.go), + [`errors.go`](../internal/provider/errors.go) and + [`check.go`](../internal/provider/check.go). + +Rule keywords: **MUST** means breaking the rule breaks a caller. **SHOULD** +means every current provider does it and a new one needs a reason not to. +**MAY** means it's allowed. + +Code references are `path:line` at the time of writing. When a line has moved, +look for the function named next to it. + +## 1. What a provider is + +A provider is a Go type implementing `provider.Provider`: four methods over one +VM-like instance, identified by an opaque `providerID` string. Nothing else in +the operator knows how a backend works: + +- **The reconciler** (`internal/controller`) calls `Create`, `Get` and `Delete` + and branches only on the error class and `Instance.State`. +- **Orphan GC** (`internal/gc`) calls `ListByTag` and `Delete`. +- **The provider health monitor** (`internal/providerhealth`) calls the optional + `Checker`. + +The reconciler never watches backend resources. Everything it knows comes from +polling `Get` (`ProvisioningPoll` and `DeletionPoll` 10s, `RequeueNow` 1s, +`QuotaRetry` 5m, `internal/controller/proxy_controller.go:452-467`). Up to 3 +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. + +## 2. The contract, method by method + +### Create(ctx, CreateRequest) (providerID, error) + +`CreateRequest` carries: + +- `Name`: from `NameFromUID`, 22 characters, `proxy-` plus base32. +- `UID`: the owning Proxy's UID. +- `Namespace` and `ProxyName`. +- `Placement`: `Region`, `Zone`, `MachineType`, `Image`. +- `CloudInit`: already resolved from inline text or a Secret. +- `Port`. + +Rules: + +- **MUST be idempotent by `req.Name`.** A repeat call for a name that already + exists returns that instance's providerID with a nil error. It must not return + an error and must not create a duplicate. + *Why:* the reconciler records the ID in status only after `Create` returns + (`proxy_controller.go:155-172`). A crash between the two, or a status write + that is lost, means `Create` runs again for the same Proxy. `replaceInstance` + also depends on this to recover from a crash between its two writes + (`:240-244`). +- **MUST mark the instance as managed when it is created**, with + `LabelManaged=true` and `LabelUID=` or the backend's equivalent. kvm + uses domain `` because libvirt has no labels. + *Why:* orphan GC finds instances only through that mark (`internal/gc/gc.go:105`), + and never deletes one with an empty UID (`gc.go:113-117`). An instance that + exists for a while without the mark leaks if the operator crashes. +- **MUST return a providerID that is self-contained.** `Get` and `Delete` + receive only the ID, and the config or spec may have changed since `Create`, + for example a zone edit during a replacement. So put the location in the ID: + - kubernetes: `/` + - gcp: `zones//instances/` + - kvm: `hosts//domains/` + + Validate any config values that end up in the ID so they can't contain the + separator (`config_kvm.go:108-114`). +- **MUST honour `ctx`.** The reconcile context is cancelled on shutdown and when + leadership is lost. +- **SHOULD return once the request is submitted, not once the instance is + running.** Readiness is discovered by polling `Get`. A blocking `Create` holds + one of the 3 reconcile workers for its whole duration. kvm breaks this rule on + purpose, see §8. +- **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`). +- **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 + and has no other effect. +- **SHOULD deliver `CloudInit` as user-data.** A provider that can't (kubernetes) + documents that it ignores the field. +- **MUST NOT log `CloudInit` contents.** It can hold secrets. Log its byte count + instead. gcp and kvm test this with a sentinel string (`kvm_test.go:975-995`, + `gcp_test.go:330-407`). + +### Get(ctx, providerID) (*Instance, error) + +- **MUST return `ErrNotFound` when, and only when, the instance is gone.** + *Why:* `ErrNotFound` triggers three actions: + - In normal reconciling, the reconciler clears the ID and creates again + (`proxy_controller.go:197-202`). + - During replacement, it is the signal to create the new instance + (`:237-251`). + - During deletion, it releases the finalizer (`:287-291`). + + A false NotFound during an outage creates a duplicate or loses the instance. + "Unreachable", "wrong credentials" and "can't tell" are Transient or + Permanent, never NotFound. +- **MUST keep returning the instance, not NotFound, while a delete is in + progress** and the name is still taken. + *Why:* the replacement instance gets the same name. The reconciler waits for + NotFound before creating it (`:228-233` comment, `:237-251`). A NotFound too + early would make `Create` adopt the dying instance. +- **MUST report `Running` only with a non-empty `IP`.** Running without an + address yet is `Provisioning`. + *Why:* the IP from a `Running` answer goes straight into status and the health + engine (`:212-215`). All three providers do this. +- **MUST report `Stopped` or `Terminated` only when the instance will not + recover by itself.** SHOULD map states it doesn't recognise to `Stopped`. + *Why:* the reconciler deletes and recreates on either state (`:216-226`); the + instances are treated as disposable. The two states cause exactly the same + reaction. +- **MUST fill `UID` and `CreatedAt` by reading them back from the backend**, + never from memory. + *Why:* GC compares `UID` with the live Proxies. It deletes anything whose + `CreatedAt` is older than `MinAge` (10m), and **a zero `CreatedAt` counts as + very old** (`gc.go:120`). A provider that forgets `CreatedAt` makes instances + still being created collectable. An unparsable timestamp gives the zero time + in both gcp and kvm. That is accepted because it never shields an orphan + forever (`gcp.go:253-256`). +- **SHOULD return `ErrPermanent` for an ID it can never resolve**, such as a + malformed ID or a host no longer in the config. + *Why:* the deletion path then tries `Delete` anyway and releases the + finalizer if that works (`:292-302`). If the provider returned Transient, + deletion would poll forever. + +### 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. +- **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). + *Why:* GC sees only what `ListByTag` returns, so secondary resources have no + other cleanup path. +- **MAY return before the instance is gone** (gcp, kubernetes), as long as `Get` + keeps showing it until it is (see Get). +- **SHOULD succeed for an ID whose `Get` fails permanently** when there is + nothing left it could reach. Example: kvm on a host that was removed from the + config returns nil (`kvm.go:466-476`). + *Why:* on the deletion path a permanent error isn't latched. It keeps + retrying (`deletionFailure`, `:349-359`), so a Proxy is only released through + the `crawl.example.com/skip-cleanup` annotation. + +### ListByTag(ctx) ([]Instance, error) + +- **MUST return only instances that carry the managed mark**, each with an `ID` + that `Delete` accepts, plus `UID` and `CreatedAt`. + *Why:* GC deletes every listed instance whose UID isn't live and whose age is + past `MinAge` (`gc.go:111-131`). Listing something without the mark is how an + operator deletes a VM it doesn't own. +- **MUST return an error rather than a list it can't vouch for.** An error makes + GC skip that provider for the sweep (`gc.go:105-110`). +- **MAY leave out instances it couldn't read.** Leaving something out is always + safe, because GC never deletes what isn't listed. The current providers make + different choices here: + - gcp silently leaves out unreachable zones (`ReturnPartialSuccess`, + `gcp.go:229`). + - kvm fails the whole list when one host is down but skips a single unreadable + domain (`kvm.go:540-565`, `:580-596`). + - kubernetes makes a single call. +- **SHOULD cover the whole scope the provider can create in**: every namespace, + zone or host. Anything outside that scope is never collected. +- **Scope caveat:** GC protects instances by UID, not by operator install. Two + installs, or two clusters, sharing one GCP project or hypervisor will delete + each other's instances ([gcp-vm-validation.md](gcp-vm-validation.md#gotchas)). + Two provider entries of one install that share a scope are safe, because they + share the live-UID set. + +## 3. Error taxonomy + +Every error a provider method returns MUST be built with +`provider.Wrap(class, op, providerName, id, err)`. `op` is one of `create`, +`get`, `delete` or `list`, and `providerName` is the **configured name** +(`cfg.Name`), not the type. + +`Wrap` keeps both `errors.Is(err, class)` and `errors.As` to the SDK error +working (`errors.go:53-62`). The shared metrics and tracing decorators label +results with the class (`metrics.go:61-74`, `tracing.go:67-80`). + +| Class | Use it for | During normal reconciling | While deleting | +|---|---|---|---| +| `ErrNotFound` | the instance doesn't exist | Get: recreate or advance (§2); other ops: returned as the error, so exponential backoff | Get: release the finalizer | +| `ErrQuotaExceeded` | quota or rate limit | condition `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` | +| `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same | +| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: condition `PermanentError`, no more provider calls until `metadata.generation` changes (`:130-135`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) | + +Pitfalls: + +- **Unclassified means Transient.** `provider.Class` returns `ErrTransient` for + any error not built with `Wrap` (`errors.go:71-91`). That is the safe default, + but a permanent failure that was never classified retries forever, and a quota + error backs off far too fast. +- **Permanent is sticky.** Use it only for errors that editing the Proxy spec or + fixing the config can resolve. A 403 that could be a quota problem is the hard + case: gcp checks the error reason (`gcp/errors.go:31-34`), while kubernetes + can't tell quota from RBAC and picks Permanent (`kubernetes.go:201-206`). +- **Don't return NotFound from `Create`.** The reconciler treats it as Transient + there, but it misleads the metrics and anyone reading logs. kvm can currently + do this (§8). +- **Classify in one `classify` function** with a table-driven test. All three + providers do. + +## 4. Identity, names and GC marks + +- **Names:** `provider.NameFromUID(uid)` gives `proxy-` plus 16 lowercase base32 + characters (`name.go:29-33`). It is valid as a GCP name, a DNS label and a + libvirt domain name. A backend with stricter rules SHOULD map it + deterministically rather than invent a new scheme, because idempotency depends + on the name being the same every time. +- **providerID:** opaque to everyone except the provider. Keep it + `/`-shaped, parse it strictly, and return `ErrPermanent` for + a malformed one (§2). +- **Marks:** `LabelManaged = "proxy-operator-managed"` with value `"true"`, and + `LabelUID = "proxy-operator-uid"` (`provider.go:28-32`). Use native labels or + tags where the backend has them. Otherwise store them in metadata that is + written in the same call that creates the instance (kvm: `metadata.go`). +- **`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`). + +## 5. Optional interfaces: `Checker` + +`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). The +monitor type-asserts it on the **unwrapped** provider, before `cmd/main.go` adds +the decorators (`cmd/main.go:211-222`). A provider without it shows up as +unsupported, not as failed. + +- **MUST never fail as a whole.** Every failure is a `TargetStatus` with + `Ready=false`. +- **MUST return promptly once `ctx` ends.** The monitor gives the whole round + one timeout (`monitor.go:180`). +- **MUST return one entry per target, in a stable order.** Targets are what can + fail independently: the apiserver, a project, each hypervisor. +- **MUST NOT disrupt reconciles.** A check that shares a connection with + provisioning must not tear that connection down when it times out. kvm uses + `hostPool.probe` for this, not `call` (`kvm/check.go:65`, `kvm/conn.go:213-242`). +- **SHOULD use the shared reasons** (`Reachable`, `Timeout`, `Unreachable`, + `Unauthorized`, `Forbidden`, `APIError`). Add provider-specific reasons only + for failures an admin would fix differently: kvm has `SSHHandshakeFailed`, + `PoolNotFound` and others. +- **SHOULD check with the same credentials, verb and scope that `ListByTag` and + `Create` use**, and be cheap: one page, one object. +- **MAY fill `Details`** with facts useful when triaging, such as versions or + capacity. Only kvm does today. + +## 6. Feature matrix + +Legend: **yes** / **partial** / **no**, with a short note. The evidence is in +the per-provider notes below the table. + +| Feature | kubernetes | gcp | kvm | +|---|---|---|---| +| providerID self-contained | yes: `ns/name` | yes: `zones/z/instances/n`; project from config | yes: `hosts/h/domains/n` | +| Create idempotent | yes: AlreadyExists = success | yes: 409 = success | yes: lookup by name; overlay reused, seed rebuilt | +| Create verifies adopted instance's UID | no | no | no | +| Create non-blocking | yes | yes: operation not awaited | **no**: waits for `DomainCreate`; first use of an image downloads it | +| Placement used | none | Zone, MachineType, Image | Zone (host), MachineType, Image | +| CloudInit delivered | **no**: ignored | yes: `user-data` metadata | yes: NoCloud seed ISO | +| Port used | yes: Squid config and containerPort | no: firewall is manual | no | +| GC marks on create | yes: Pod labels | yes: instance labels | yes: domain ``; volumes unmarked | +| States produced | Provisioning, Running, Terminated | all four | Provisioning, Running, Stopped | +| IP source | PodIP | external NAT IP only | ARP, then guest agent; first IPv4 | +| UID / CreatedAt readback | yes | yes | yes | +| Delete idempotent | yes | yes | yes, at each step | +| Delete waits until gone | no | no | yes | +| Delete cleans secondary resources | n/a | yes: boot disk auto-deleted | yes: overlay and seed; base image kept | +| ListByTag scope | cluster-wide | one project, all zones | configured hosts | +| ListByTag on partial failure | n/a: single call | leaves out failed zones | whole list fails if a host is down; unreadable domain skipped | +| Quota detection | no | yes: 429 and 403 quota reasons | no | +| Error.Provider = configured name | **no**: hardcoded `"kubernetes"` | yes | yes | +| Checker | yes: `apiserver` | yes: the project | yes: each host | +| Checker Details | no | no | yes | +| Credentials | in-cluster config / kubeconfig | ADC | SSH key + known_hosts files, usually a Secret | +| Credential rotation without restart | n/a | via ADC | no: cached after first load | +| Connection handling | client-go | SDK client; SDK retries on Get and List | one pooled SSH+libvirt connection per host, keepalive | +| Logging | none in the package | V(1) ops, V(2) detail, wire log | V(1) ops and RPCs, V(2) detail | +| 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` | + +### kubernetes notes (`internal/provider/kubernetes`) + +- **ID:** `kubernetes.go:152-165`. An empty name part is not rejected. +- **Create:** `kubernetes.go:90-100` and `pod.go:23-50`. Only `Name`, + `Namespace`, `UID` and `Port` are used. Squid config is generated from the + port and passed through `SQUID_CONF` (`pod.go:38-75`). +- **State:** `stateFromPod`, `kubernetes.go:185-199`. Pending, or Running with + no PodIP, is Provisioning. Succeeded, Failed and Unknown are Terminated. A Pod + being deleted still reports its phase. +- **Delete:** `kubernetes.go:118-127`. ListByTag: `kubernetes.go:134-146`. +- **Errors:** `classify`, `kubernetes.go:201-218`. 429 and timeouts are + Transient. Forbidden, Invalid, BadRequest and Unauthorized are Permanent. + Nothing is Quota. +- **Check:** `check.go:21-53`. It lists at most one managed Pod cluster-wide, so + it proves read and list access, not create. +- **Config:** only `kubernetes.image` (`config.go:45-48`, default at + `kubernetes.go:28`). + +### gcp notes (`internal/provider/gcp`) + +- **ID:** `gcp.go:289-299`. +- **Create:** `gcp.go:135-167` and `insert.go:34-74`. `Operation.Wait` is never + called. There is one NIC with an ephemeral external IP, and the boot disk has + `AutoDelete`. +- **State:** `mapState`, `gcp.go:271-287`. RUNNING without a NAT IP is + Provisioning. Unknown statuses are Stopped. +- **Get:** `gcp.go:172-192`. Delete: `gcp.go:196-219`. ListByTag: + `gcp.go:224-244`. +- **Errors:** `errors.go:20-39`. Non-googleapi errors are Transient. 404 is + NotFound. 429 and 403 with reason `quotaExceeded` or `rateLimitExceeded` are + Quota. 400, 401 and other 403s are Permanent. +- **Check:** `check.go:30-78`. One `AggregatedList` page with `maxResults=1`. +- **Config:** `project` is required. `network`, `networkTag` and `diskSizeGb` + have defaults (`config.go:51-61`, `insert.go:12-30`). +- **Logging:** V(1) and V(2) tiers in `gcp.go`. The wire log in `wirelog.go` + runs at V(5), with payloads elided unless `--gcp-wire-log-full-payloads` is + set. + +### kvm notes (`internal/provider/kvm`) + +- **ID:** `id.go:11-21`. Host names are restricted by `config_kvm.go:108-114`. +- **Create:** `kvm.go:154-221`: + - Looks up the domain by name and starts it if needed (`ensureRunning`, + `:300-318`). + - Otherwise: base image (`image.go`), then overlay (reused if it exists, + `:225-245`), then seed (always rebuilt, `:249-275`), then define with + metadata (`:277-298`), then start. +- **Get:** `kvm.go:323-356`. States come from `mapState` (`:442-453`): nostate + is Provisioning, running or blocked without an IP is Provisioning, everything + else is Stopped. +- **Addresses:** `address` (`:397-428`) tries ARP first, then the guest agent. +- **Delete:** `kvm.go:461-538`. Destroy, undefine (with NVRAM, managed save and + snapshot metadata), then remove the overlay and seed by name. A host that is no + longer configured is a success. +- **ListByTag:** `kvm.go:545-596`. +- **Errors:** `errors.go:21-42`. No domain or no volume is NotFound. XML, + unsupported, invalid-argument, auth and access errors, and a missing pool, are + Permanent. Everything else is Transient, including SSH handshake and auth + failures. Nothing is Quota. +- **Check:** `check.go:41-139`. Runs concurrently per host through + `hostPool.probe`. Reasons are tagged by the dial stage. +- **Connections:** `conn.go`. Dials lazily, one connection per host with a 15s + keepalive. On cancel, `call` closes the shared connection; `probe` abandons + the RPC and leaves the connection open. +- **Full guide:** [kvm-provider.md](kvm-provider.md). + +## 7. Adding a provider: checklist + +1. **Package:** create `internal/provider//` with the provider type and a + compile-time `var _ provider.Provider = (*Provider)(nil)` (and + `provider.Checker`). + - Put the backend SDK behind a small interface (gcp `instancesAPI`, kvm + `libvirtAPI`) so a fake can stand in for it in tests. + - Build the logger with + `logf.FromContext(ctx).WithName("").WithValues("provider", name)`. +2. **Config:** in `internal/provider/config.go`: + - Add a `Config` block and a `ProviderConfig` field. + - Add the block to `setBlocks`. + - Add the type to the `switch p.Type` allow-list (`config.go:102-108`). + - Add required-field validation, in its own `config_.go` if it is + large. + - Add rows to `config_test.go`. + + The file is parsed with `UnmarshalStrict`, so unknown keys already fail. +3. **Constructor:** `New(ctx, provider.ProviderConfig) (provider.Provider, + error)`, registered in the `registry.Build` map in `cmd/main.go:195-205`. + - A constructor error stops the manager. + - So fail only on config errors, and **don't dial the backend here**: one + unreachable hypervisor or API must not take down the other providers (kvm + logs a warning and connects lazily, `kvm.go:89-101`). +4. **Decorators:** nothing to do. `cmd/main.go:219-222` wraps every provider in + `WithTracing(WithMetrics(...))`. Don't count requests or open per-operation + spans yourself. +5. **Contract:** implement each rule in §2, and test each one against the fake: + - repeat `Create` + - `Get` after `Delete` + - `Delete` twice + - malformed ID + - unknown state mapped to `Stopped` + - `Running` requires an IP + - `UID` and `CreatedAt` read back + - `ListByTag` excludes unmarked instances +6. **Errors:** one `classify` plus a table test covering every class, including + "not an SDK error → Transient". Use `cfg.Name` in `provider.Wrap`. +7. **Checker:** shared reasons first, a table test for the reasons, and a test + that `Check` returns promptly with an expired `ctx`. +8. **Logging:** + - V(1): one line per operation outcome, plus a `logAPIError`-style line with + the raw backend error. + - V(2): request and response detail. + - Never log CloudInit contents; add a sentinel test for it. +9. **Real-backend test:** behind `//go:build integration`, skipped when its + environment variables are unset. It runs the whole lifecycle: create, create + again, poll until Running, list, delete, delete again, and confirm it is gone + and cleaned up. See `kvm/kvm_integration_test.go` and + [testing.md](testing.md). +10. **RBAC:** if the provider talks to the cluster, add package-level + `+kubebuilder:rbac` markers, then run `make manifests` and check + `git diff config/rbac/role.yaml`. +11. **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. + - A column in the §6 matrix above. + - A sample in `config/samples/`. +12. **Optional:** a demo script under `docs/demo/`. + +## 8. Gaps and proposed features + +These are listed only. Nothing below is designed or implemented. + +### Current gaps + +**Across providers and the contract** + +- **Adoption isn't verified.** `Create` adopts any existing instance with the + right name without checking its `LabelUID`. None of the three providers check + it. +- **The Create comment doesn't match kvm.** The contract says `Create` returns + as soon as the request is submitted (`provider.go:90-94`). kvm blocks until + the domain has started, and on first use of an image, for the whole download. +- **`Stopped` and `Terminated` are indistinguishable to the reconciler.** + kubernetes never produces Stopped, and kvm never produces Terminated. +- **Different ListByTag partial-failure policies.** gcp leaves failed parts out, + kvm fails the whole list. Both are safe for GC; the contract doesn't say which + to prefer. +- **Config is read once per process.** Adding a provider or host, or rotating + 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. + +**kubernetes** + +- **Wrong provider name in errors.** `Error.Provider` is hardcoded to + `"kubernetes"` (`kubernetes.go:105-216`), so errors and logs show the wrong + name when the configured name differs. +- **Spec fields ignored.** `CloudInit` and all of `Placement` are ignored. +- **No quota class.** A quota 403 is Permanent. +- **Deleting Pods look healthy.** A Pod being deleted still reports its phase, + so it can read as `Running`. +- **Unguarded Pods.** No securityContext, no resources, and + `http_access allow all` (`pod.go`). +- **No logging** in the package, and **no Checker `Details`**. + +**gcp** + +- **Asynchronous insert failures are never read.** An insert that the API + accepts but that fails later (quota, bad image) is never seen. The next `Get` + returns NotFound and the reconciler creates it again, so the reason never + reaches status. [gcp-vm-validation.md](gcp-vm-validation.md#gotchas) says + such errors do reach status. +- **External IP only.** A VM without an external NAT IP stays `Provisioning` + forever. +- **No firewall management.** The rule for the proxy port is a manual + prerequisite, and `req.Port` is unused. +- **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`. + +**kvm** (see also [kvm-provider.md § Known gaps](kvm-provider.md#known-gaps)) + +- **No quota or capacity detection.** +- **Create blocks**, including the first image download on each host. +- **GC blind spots.** Volumes without a domain are invisible to GC, and so are + VMs on a host removed from the config. `Delete` treats the latter as done. +- **SSH material is cached after the first successful load**, so rotating keys + needs a restart. +- **Cancelling one operation cuts off the others on that host.** A cancelled + `call` closes the shared per-host connection, aborting other operations in + flight on that host (`conn.go:199-211`). +- **SSH auth failures retry forever.** SSH authentication and host-key failures + are Transient, while libvirt auth errors are Permanent. +- **`Create` can return `ErrNotFound`** when a volume or domain disappears + during the call. +- **No reused-overlay check.** An existing overlay is reused without checking + its backing image or size. +- **Doc drift in kvm-provider.md:** + - It says ARP is the fallback; the code tries ARP first. + - It describes a connection kept open "from startup"; the dial is lazy. + - It says `host` and `name` are on every log line; the check and list paths + lack them. + +### Proposed features (not implemented) + +- **A shared conformance suite:** for example `internal/provider/providertest`, + which runs the §2 rules against any provider plus a fake backend, so a new + provider gets the contract tests for free. +- **Verify the UID when `Create` adopts an instance.** Return `ErrPermanent` on + a mismatch instead of adopting an instance that belongs to someone else. +- **Report asynchronous create failures:** gcp keeps the operation name, and + `Get` reports the failure instead of NotFound. +- **Capacity or quota signals** for kvm, such as pool free space and host + memory, mapped to `ErrQuotaExceeded`. +- **GC for secondary resources:** mark volumes and similar resources, and let + `ListByTag` or a second listing method cover them. +- **Reload credentials and config without a restart.** +- **Checker `Details`** for kubernetes (server version) and gcp (quota + headroom). +- **Internal-IP mode** for gcp, and a firewall rule managed per provider. From e08f23bcb37a6e5f12586054f855e72cd5bea08e Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 08:57:52 +0000 Subject: [PATCH 02/11] Correct the provider blueprint after verification A re-check against the code found four mismatches: the kvm connection is opened at startup by the leader's first health check, so that isn't doc drift; cmd/main.go, not the monitor, type-asserts Checker; QuotaExceeded and PermanentError are reasons on Provisioned, not conditions; and the latch line range. The checklist also now matches how the existing providers do interface assertions and RBAC markers. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-16-0841-provider-blueprint.md | 36 +++++++++++++++++-- docs/providers.md | 30 +++++++++------- 2 files changed, 50 insertions(+), 16 deletions(-) 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 b5c3589..b092194 100644 --- a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md +++ b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md @@ -4,7 +4,7 @@ Plan: [docs/plans/2026-09-16-0841-provider-blueprint.md](../plans/2026-09-16-084 - [x] Step 0 — Save the plan; wait for the merge - [x] Step 1 — Collect evidence (read-only, in parallel) - [x] Step 2 — Write `docs/providers.md` -- [ ] Step 3 — Verify, then open the MR +- [x] Step 3 — Verify, then open the MR ## Step 0 — Save the plan; wait for the merge @@ -61,10 +61,40 @@ Judgment calls: planned. The contract rules in §2 cite the reconciler and GC lines each rule protects. - The gap list includes documentation drift (kvm-provider.md's ARP order and - "from startup" dial, gcp-vm-validation.md's label value and its claim that - async errors reach status), as well as code gaps. Both would mislead the next + log keys, gcp-vm-validation.md's label value and its claim that async errors + reach status), as well as code gaps. Both would mislead the next provider author in the same way. - "Proposed features" stays a flat list with no design, as the user decided. Worth noting: the line numbers in the doc are a snapshot. The doc says so and asks readers to look for the named function when a line has moved. + +## Step 3 — Verify, then open the MR + +An Explore agent re-checked the doc against the code: about 150 claims, +including every matrix cell. It found four mismatches, and all four were fixed: +- **Startup dial:** I had listed kvm-provider.md's "connection kept open from + startup" as doc drift. It isn't: the pool dials lazily, but the provider + health monitor runs a check as soon as the leader starts + (`monitor.go:147`, default interval 1m), and that check opens the connection. + The bullet was removed and the kvm notes now say this. +- **Checker wording:** §5 said the monitor type-asserts `Checker`. It is + `cmd/main.go:216`; the monitor only sees nil or not. +- **Reasons, not conditions:** §3 called `QuotaExceeded` and `PermanentError` + conditions. They are reasons on `Provisioned`. +- **Latch lines:** the line range is `:133-139`, not `:130-135`. + +It also noted that none of the providers assert `var _ provider.Provider`, and +that the kubernetes provider's RBAC markers live in the controller package. The +§7 checklist now says both. + +Link check (every relative `](…)` target and anchor in the new and edited docs +resolves), then the sanity build: + +```bash +go build ./... && go test -short ./... +# all packages ok +``` + +Worth noting: no CHANGELOG entry yet. Per CLAUDE.md it goes in after the user +confirms the doc. diff --git a/docs/providers.md b/docs/providers.md index 49f7c68..3821318 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -200,9 +200,9 @@ results with the class (`metrics.go:61-74`, `tracing.go:67-80`). | Class | Use it for | During normal reconciling | While deleting | |---|---|---|---| | `ErrNotFound` | the instance doesn't exist | Get: recreate or advance (§2); other ops: returned as the error, so exponential backoff | Get: release the finalizer | -| `ErrQuotaExceeded` | quota or rate limit | condition `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` | +| `ErrQuotaExceeded` | quota or rate limit | `Provisioned=False` with reason `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` | | `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same | -| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: condition `PermanentError`, no more provider calls until `metadata.generation` changes (`:130-135`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) | +| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: `Provisioned=False` with reason `PermanentError`, no more provider calls until `metadata.generation` changes (`:133-139`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) | Pitfalls: @@ -240,10 +240,10 @@ Pitfalls: ## 5. Optional interfaces: `Checker` -`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). The -monitor type-asserts it on the **unwrapped** provider, before `cmd/main.go` adds -the decorators (`cmd/main.go:211-222`). A provider without it shows up as -unsupported, not as failed. +`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). +`cmd/main.go` type-asserts it on the **unwrapped** provider (`cmd/main.go:216`) +before adding the decorators (`:219-222`). The monitor treats a nil Checker as +unsupported, not as failed (`monitor.go:212`). - **MUST never fail as a whole.** Every failure is a `TargetStatus` with `Ready=false`. @@ -358,16 +358,18 @@ the per-provider notes below the table. failures. Nothing is Quota. - **Check:** `check.go:41-139`. Runs concurrently per host through `hostPool.probe`. Reasons are tagged by the dial stage. -- **Connections:** `conn.go`. Dials lazily, one connection per host with a 15s - keepalive. On cancel, `call` closes the shared connection; `probe` abandons +- **Connections:** `conn.go`. One connection per host with a 15s keepalive. + The pool dials on first use; with provider checks on (the default), the + leader's first health check opens it at startup. On cancel, `call` closes the shared connection; `probe` abandons the RPC and leaves the connection open. - **Full guide:** [kvm-provider.md](kvm-provider.md). ## 7. Adding a provider: checklist 1. **Package:** create `internal/provider//` with the provider type and a - compile-time `var _ provider.Provider = (*Provider)(nil)` (and - `provider.Checker`). + compile-time interface assertions: `var _ provider.Checker = (*Provider)(nil)` + as in `kubernetes/check.go:15`, and the same for `provider.Provider`, which + the existing providers don't assert yet. - Put the backend SDK behind a small interface (gcp `instancesAPI`, kvm `libvirtAPI`) so a fake can stand in for it in tests. - Build the logger with @@ -413,8 +415,11 @@ the per-provider notes below the table. again, poll until Running, list, delete, delete again, and confirm it is gone and cleaned up. See `kvm/kvm_integration_test.go` and [testing.md](testing.md). -10. **RBAC:** if the provider talks to the cluster, add package-level - `+kubebuilder:rbac` markers, then run `make manifests` and check +10. **RBAC:** if the provider talks to the cluster, add `+kubebuilder:rbac` + markers in their own comment block, not inside a doc comment. The kubernetes + provider's Pod markers live next to the reconciler's + (`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:** - A `docs/-provider.md` covering prerequisites, config reference, @@ -497,7 +502,6 @@ These are listed only. Nothing below is designed or implemented. its backing image or size. - **Doc drift in kvm-provider.md:** - It says ARP is the fallback; the code tries ARP first. - - It describes a connection kept open "from startup"; the dial is lazy. - It says `host` and `name` are on every log line; the check and list paths lack them. From 01c27b5f35d16759c60f1de084d8722356003550 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 09:14:06 +0000 Subject: [PATCH 03/11] Cover tracing in the provider blueprint The blueprint reduced tracing to "don't open per-operation spans", which reads as "nothing to do". Add an observability section: the operation spans the decorators give for free, the child spans per backend call a provider should add (parent-gated transport or a span decorator over the SDK seam), the ctx/logger rules that keep traceID on log lines, and the status per provider: kubernetes and gcp have child spans, kvm has none. Matrix rows, the checklist item, gaps and a kvm proposal follow suit. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-16-0841-provider-blueprint.md | 28 +++++ docs/providers.md | 110 ++++++++++++++++-- 2 files changed, 129 insertions(+), 9 deletions(-) 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 b092194..97fef0b 100644 --- a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md +++ b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md @@ -98,3 +98,31 @@ go build ./... && go test -short ./... Worth noting: no CHANGELOG entry yet. Per CLAUDE.md it goes in after the user confirms the doc. + +### Follow-up — tracing was missing from the blueprint + +The user pointed out, after the PR was opened, that the blueprint said almost +nothing about tracing. The plan's questionnaire item 10 had collected the +evidence, but the doc reduced it to a matrix row about logging and a checklist +line telling authors not to open spans. That line even suggests tracing needs +no work. A provider author needs the opposite message: the operation span comes +free, and child spans for backend calls are theirs to add. + +Added a new §6, "Observability: tracing, logs and metrics", and renumbered the +matrix, checklist and gaps sections to §7–§9. It covers: +- **Three layers:** decorator operation spans; child spans per backend call, + with parent-gated HTTP transports or a span decorator over the SDK seam; and + logs. +- **The ctx and logger rules** that keep `traceID` on log lines. +- **A per-provider status table:** + - kubernetes: parent-gated `RestConfigWrapper` spans. + - gcp: the Google SDK's own `otelhttp`, never disabled by the provider. + - kvm: no spans. `trace.go` is a log decorator. + +Also added two tracing rows to the §7 matrix, rewrote checklist item 8 as +"Tracing and logging" with an in-memory span-recorder test, and added gaps (kvm +has no child spans; only kubernetes is checked against Tempo, and only at +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. diff --git a/docs/providers.md b/docs/providers.md index 3821318..94c6b6b 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -87,7 +87,7 @@ Rules: - **SHOULD return once the request is submitted, not once the instance is running.** Readiness is discovered by polling `Get`. A blocking `Create` holds one of the 3 reconcile workers for its whole duration. kvm breaks this rule on - purpose, see §8. + purpose, see §9. - **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`). @@ -216,7 +216,7 @@ Pitfalls: can't tell quota from RBAC and picks Permanent (`kubernetes.go:201-206`). - **Don't return NotFound from `Create`.** The reconciler treats it as Transient there, but it misleads the metrics and anyone reading logs. kvm can currently - do this (§8). + do this (§9). - **Classify in one `classify` function** with a table-driven test. All three providers do. @@ -263,7 +263,72 @@ unsupported, not as failed (`monitor.go:212`). - **MAY fill `Details`** with facts useful when triaging, such as versions or capacity. Only kvm does today. -## 6. Feature matrix +## 6. Observability: tracing, logs and metrics + +Tracing is OpenTelemetry, set up in `internal/tracing` from the standard +`OTEL_*` environment variables. With no exporter configured, spans are no-ops +and logs are unchanged ([architecture.md §10](architecture.md#10-tracing-internaltracing)). +A provider has three layers to think about; the first comes for free. + +**1. Operation spans and metrics: the decorators, nothing to write.** +`cmd/main.go:219-222` wraps every provider in +`WithTracing(WithMetrics(...))`: + +- `provider.WithTracing` opens one client span per call, `provider.create`, + `provider.get`, `provider.delete` or `provider.list`, with `provider.name`, + `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`). +- The spans sit under `Reconcile Proxy → reconcile.managed | + reconcile.replaceInstance | reconcile.delete`, or under `gc.sweep`. + +Rules: + +- **MUST NOT** open its own per-operation span or count its own requests. They + would duplicate the decorators. +- **MUST pass the caller's `ctx`** to every backend call and log through + `logf.FromContext(ctx)` **at call time**. The decorator starts the span with + `tracing.StartSpan`, which puts a logger carrying `traceID` and `spanID` into + `ctx` (`internal/tracing/logger.go:32-44`). A logger cached at construction, + or a backend call made with `context.Background()`, loses the trace. +- **Health checks bypass the decorators** (`cmd/main.go:211-218`). A + `Checker` runs under the monitor's `providerhealth.check` span + (`monitor.go:176`), and the same `ctx` rules apply to it. + +**2. Child spans for backend calls: SHOULD.** +One operation span says that `Create` took 40 seconds. Child spans say which +backend call took them. Every provider SHOULD produce one child span per backend +request, under the operation span: + +- **HTTP backends:** wrap the SDK's transport with `otelhttp`. Gate it on an + existing parent span, as `tracing.RestConfigWrapper` does + (`internal/tracing/transport.go:12-51`). Otherwise background traffic such as + watches, token refreshes or keepalives becomes root spans. Check first whether + the SDK already does this: Google's does. +- **Non-HTTP backends** (RPC over SSH, gRPC without interceptors, CLIs): start + child spans yourself with `tracing.Start(ctx, ".")`, around each + RPC and around long steps such as a dial or an image download. Use a + decorator over the SDK seam, not call-site code. +- **MUST NOT** put secrets or CloudInit contents in span attributes. Put + identifiers, sizes and error codes there. + +**3. Logs.** V(1): one line per operation outcome and per failed backend call. +V(2): curated detail. V(5): raw wire, if the backend has one. Log lines are +matched to traces by the `traceID` key from layer 1, which is how kvm's +per-RPC log lines join a trace today. + +What each provider does now: + +| Layer | kubernetes | gcp | kvm | +|---|---|---|---| +| 1. Operation spans and metrics | yes: decorators | yes: decorators | yes: decorators | +| 2. Child spans per backend call | yes: `tracing.RestConfigWrapper()` on its `rest.Config` (`cmd/main.go:196-200`, `kubernetes.go:58-60`), parent-gated | yes: the Google SDK adds `otelhttp` itself unless telemetry is disabled (`google.golang.org/api/transport/http/dial.go:308-312`); not parent-gated, but every call site has a parent | **no**: `kvm/trace.go` is a log decorator, not spans | +| 3. Logs carry `traceID`/`spanID` | n/a: the package doesn't log | yes, including the V(5) wire log (`gcp/wirelog.go:54-59`) | yes: every `libvirt call` line at V(1) | +| Tested against a real collector | yes: kind e2e checks the `provider.create` and `provider.delete` traces in Tempo (`test/e2e/tracing_test.go:200-238`), operation spans only | no | no | + +## 7. Feature matrix Legend: **yes** / **partial** / **no**, with a short note. The evidence is in the per-provider notes below the table. @@ -294,6 +359,8 @@ the per-provider notes below the table. | Credential rotation without restart | n/a | via ADC | no: cached after first load | | Connection handling | client-go | SDK client; SDK retries on Get and List | one pooled SSH+libvirt connection per host, keepalive | | Logging | none in the package | V(1) ops, V(2) detail, wire log | V(1) ops and RPCs, V(2) detail | +| Tracing: operation spans (§6) | yes | yes | yes | +| 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` | @@ -364,7 +431,7 @@ the per-provider notes below the table. the RPC and leaves the connection open. - **Full guide:** [kvm-provider.md](kvm-provider.md). -## 7. Adding a provider: checklist +## 8. Adding a provider: checklist 1. **Package:** create `internal/provider//` with the provider type and a compile-time interface assertions: `var _ provider.Checker = (*Provider)(nil)` @@ -405,11 +472,19 @@ the per-provider notes below the table. "not an SDK error → Transient". Use `cfg.Name` in `provider.Wrap`. 7. **Checker:** shared reasons first, a table test for the reasons, and a test that `Check` returns promptly with an expired `ctx`. -8. **Logging:** +8. **Tracing and logging** (§6): + - Child spans for backend calls: an existing SDK instrumentation, a + parent-gated transport wrapper, or a span decorator over the SDK seam. + Add a test with an in-memory span recorder + (`sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(tracetest.NewSpanRecorder()))`, + as in `internal/provider/tracing_test.go`) asserting the spans are + children of the operation span. + - Take the logger from `ctx` on every call so log lines carry `traceID`. - V(1): one line per operation outcome, plus a `logAPIError`-style line with the raw backend error. - V(2): request and response detail. - - Never log CloudInit contents; add a sentinel test for it. + - Never put CloudInit contents in logs or span attributes; add a sentinel + test for it. 9. **Real-backend test:** behind `//go:build integration`, skipped when its environment variables are unset. It runs the whole lifecycle: create, create again, poll until Running, list, delete, delete again, and confirm it is gone @@ -426,12 +501,12 @@ the per-provider notes below the table. 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. - - A column in the §6 matrix above. + §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/`. -## 8. Gaps and proposed features +## 9. Gaps and proposed features These are listed only. Nothing below is designed or implemented. @@ -468,6 +543,16 @@ These are listed only. Nothing below is designed or implemented. `http_access allow all` (`pod.go`). - **No logging** in the package, and **no Checker `Details`**. +**Tracing** + +- **kvm has no child spans.** A kvm trace shows `provider.create` and nothing + under it: no libvirt RPCs, no SSH dial, no image download, although that is + where the time goes. Correlation works only through the `traceID` on its V(1) + log lines, which needs debug logging on. +- **Only kubernetes is tested against a real collector**, and that test checks + only the operation spans. Nothing checks that gcp's SDK spans or the + kubernetes API spans actually nest under `provider.*`. + **gcp** - **Asynchronous insert failures are never read.** An insert that the API @@ -507,6 +592,13 @@ These are listed only. Nothing below is designed or implemented. ### Proposed features (not implemented) +- **Child spans in kvm:** extend the `tracingAPI` decorator in `kvm/trace.go`, + which already wraps every libvirt call, to also start a span per RPC, and add + spans for the SSH dial and the image download. The seam's methods take no + `ctx` today (`hostPool.call` hands `fn` only the API), so the span context has + to be plumbed in. +- **Tempo e2e checks for child spans**, and for gcp and kvm through their real + backend runbooks. - **A shared conformance suite:** for example `internal/provider/providertest`, which runs the §2 rules against any provider plus a fake backend, so a new provider gets the contract tests for free. From 0bb699bf09d34fb54d2899b422ccec5b0fba25a7 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 09:18:40 +0000 Subject: [PATCH 04/11] 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) --- README.md | 47 ++----- .../2026-09-16-0841-provider-blueprint.md | 49 +++++++ docs/providers.md | 130 ++++++++++++++++-- 3 files changed, 180 insertions(+), 46 deletions(-) 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. From cf89ac4c9125088a5ea20a5f410de825bff6ea4a Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:13:01 +0000 Subject: [PATCH 05/11] Plan the split of the provider blueprint into contract, appendix, template and index Co-Authored-By: Claude Fable 5.1 --- .../2026-09-16-1108-provider-docs-split.md | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 docs/plans/2026-09-16-1108-provider-docs-split.md diff --git a/docs/plans/2026-09-16-1108-provider-docs-split.md b/docs/plans/2026-09-16-1108-provider-docs-split.md new file mode 100644 index 0000000..410bac5 --- /dev/null +++ b/docs/plans/2026-09-16-1108-provider-docs-split.md @@ -0,0 +1,99 @@ +# Plan: Split the provider blueprint into contract, appendix, template and index + +**Created:** 2026-09-16 11:08 + +## Context + +`docs/providers.md` is one 731-line blueprint that mixes the generic provider contract with how kubernetes, gcp and kvm happen to meet it, plus gaps and proposals. The user wants to hand an agent a self-contained set of rules for building a new provider (Proxmox is next), so the generic part must stand alone, state explicitly where providers are allowed to differ (a pod is ready at once, a VM has a provisioning window, kvm blocks in Create, gcp does not), and leave implementation detail to a separate appendix. A third file, a fill-in spec template, lets the user write the new provider's specifics in the same vocabulary. `docs/providers.md` becomes a short index so every existing link keeps working. + +Decisions already made by the user: two new files plus a template; `providers.md` becomes an index; the contract is fully abstract (never names an existing provider); work continues on the checked-out branch `feat/provider-blueprint`. + +Conventions for all four files: code references are `path` plus function or symbol name, never line numbers; rule keywords MUST / SHOULD / MAY as today, and every MUST names the caller and what breaks; no em-dashes in prose. The contract cites only shared-layer files (`internal/provider/*.go`, `internal/provider/registry`, `internal/controller`, `internal/gc`, `internal/providerhealth`, `internal/tracing`, `cmd/main.go`, `config/manager`, `api/v1alpha1`) and never `internal/provider/{kubernetes,gcp,kvm}`. + +## Facts verified against the code that the new contract must carry + +- Callers put no deadline on the context. Only `Monitor.checkOnce` (`internal/providerhealth/monitor.go`) uses `WithTimeout`; the reconcile and GC contexts are cancel-only. `Sweeper.sweep` (`internal/gc/gc.go`) loops providers sequentially, so one hung `ListByTag` stalls orphan GC for every provider. +- The reconciler's Stopped/Terminated branch in `reconcileManaged` (`internal/controller/proxy_controller.go`) deletes and requeues without clearing `ProviderID`, so a backend that reports a freshly created, not-yet-started instance as Stopped gets it deleted right after creation. kvm avoids this only by blocking `Create` through start. +- `Sweeper.sweep` treats a zero `CreatedAt` as collectable now, skips `UID == ""`, and skips anything younger than `MinAge` (10m). +- `cmd/main.go` type-asserts `provider.Checker` on the unwrapped provider before `WithTracing(WithMetrics(...))`; `registry.Build` fails the whole manager on a constructor error, so constructors must not dial. +- Manager runtime: non-root, read-only rootfs, `/tmp` emptyDir 64Mi, 128Mi limit, distroless static image with CA roots (`config/manager/manager.yaml`, `Dockerfile`). +- Corrections to earlier notes, for the appendix: kvm Checker `Details` also carry `user` and `pool`; the kubernetes provider has no seam interface (it injects `client.Client` via `newWithClient` and tests use the fake client); kvm `classify` has an `isClassified` pass-through for errors already wrapped with a class. + +## Files + +### 1. `docs/provider-contract.md` (new, the generic contract) + +Every behaviour axis gets a stable kebab-case code in backticks, used verbatim by the appendix table and the template so the three files can be cross-checked by grep. + +Sections: + +1. **Preamble**: audience, reading order (contract, template, appendix), rule keywords, "path plus symbol, never line numbers", where the rest lives. +2. **What a provider is and who calls it**: the types in `provider.go`, `name.go`; callers and entry points (`reconcileManaged`, `replaceInstance`, `reconcileDelete`, `providerFailure`, `deletionFailure`, `placementFrom`, poll intervals, `MaxConcurrentReconciles`; `Sweeper.sweep` and `MinAge`; `Monitor.checkOnce`; decorators; `registry.Build`); what the instance is for (health probe CONNECT in `internal/health`, discovery handing out ip and port); leader-only vs every-replica; concurrency (MUST be safe, including two calls for one name); runtime envelope; time and cancellation (no caller deadline, MUST honour cancel, SHOULD bound each backend call, what breaks: a pinned worker, a stalled GC sweep, a wedged check round). +3. **The contract, method by method**: current §2 rewritten abstractly, plus the new points. Create: idempotent by name, mark on create or within a documented window shorter than `MinAge`, self-contained ID, ctx, blocking vs submit, reject-early with `ErrPermanent` naming field and value, catalogue changes are outside the spec hash (`spechash.go`), MAY ignore Placement fields but MUST document, SHOULD deliver CloudInit, MUST NOT log it, and an eventually consistent backend MUST NOT let `Get` answer NotFound for an ID `Create` just returned. Get: NotFound only when gone, keep answering during delete, Running only with IP, Stopped/Terminated only when it will not recover, the "defined but not started" rule (block Create through start, or report Provisioning using written metadata or a task handle), behaviour during async operations and locks (last known state or Transient, never NotFound or Permanent), UID and CreatedAt read back, Permanent for an unresolvable ID. Delete: idempotent, re-entrant while deleting, removes secondary resources, MAY return before gone, SHOULD succeed where Get fails permanently. ListByTag: marked instances only with ID, UID, CreatedAt; error rather than an unvouched list; MAY omit unreadable parts; SHOULD cover the whole creatable scope; no installation identity. +4. **Behaviour profiles**: one subsection per axis with the question, allowed profiles, what callers tolerate, obligations per profile. Axes: `readiness`, `create-return`, `delete-return`, `ip-source`, `gc-mark`, `created-at`, `list-partial`, `quota`, `not-found`, `primary-key`, `provisioning-state`, `async-ops`, `secondary-resources`, `credentials`, `client-kind`, `timeouts`, `logging`, `child-spans`, `backend-test`. Closing table: code, profiles, tolerated by which caller, template entry. +5. **Error taxonomy**: `Wrap`, `Class`, `Error` fields, `Op` values, configured name not type; the class-to-reaction table; pitfalls; NotFound detection guidance (404, error code, list-and-miss, and why list-and-miss is forbidden with a partial list); the quota-vs-permission 403 case stated abstractly. +6. **Identity, names, marks, CreatedAt**: `NameFromUID` shape, providerID strict parsing, marks and written metadata, provider name as DNS-1123 (`Monitor` skip, `spec.provider` MaxLength), primary key not the name. +7. **Credentials, TLS and secret hygiene** (new): source and read time (constructor MUST NOT dial, SHOULD warn on a missing optional mount and fail at first use), never in logs, spans, error strings or `Error.Err`, TLS trust options and the naming of an insecure toggle, scratch only under `/tmp`. +8. **Optional interfaces: Checker**: current §5 rules; `status.type` comment in `api/v1alpha1/providerhealth_types.go` must list the new type and the CRD be regenerated. +9. **Observability**: the three layers, span and metric names, ctx and logger rules, child-span strategies by `client-kind`, no secrets or CloudInit in attributes, log tiers. +10. **Configuration and wiring**: `ProviderConfig`, `setBlocks`, the allow-list in `Config.validate`, `UnmarshalStrict`, `config_.go`, identifiers pasted into IDs, `registry.Constructor`, the map in `cmd/main.go`, decorator order, the duplicated type list. +11. **Testing obligations**: fake behind a seam; the contract test list (current checklist item 5 plus Get during an async operation, the not-yet-started state, NotFound after Create is forbidden); classify table test; Checker tests; sentinel tests for CloudInit and credentials; span recorder test; integration test behind `//go:build integration` with `_TEST_*` variables, `make test-integration-`, a section in `docs/testing.md`. +12. **Checklist for adding a provider**: current §8 generalised, plus: Makefile target, testing.md section, `hack/providers-dev.yaml` decision, `ProviderHealth` type comment and `make manifests`, RBAC diff, CHANGELOG, appendix row and section, the filled template committed as `docs/-provider-spec.md`, README row, architecture.md rows. +13. **Known limits of the contract**: adoption unverified; Stopped and Terminated indistinguishable; config read once; no installation identity in GC; unconfigured provider latches; type list duplicated; Delete resubmitted every poll; no caller deadline; no conformance suite. +14. **Related documents**. + +### 2. `docs/provider-spec-template.md` (new, the questionnaire) + +Preamble: copy to `docs/-provider-spec.md`, fill every Answer line, hand it to the implementing agent with the contract and appendix; unanswered entries block implementation. Header block: type, example configured name, backend and API version, SDK or thin client, author, date. + +Fixed entry format: Question; Allowed answers (profile names linking to the contract §4 subsection); What each answer obliges; `Answer:` blank; `Evidence:` blank. + +Entries: identity and providerID shape; primary key (`primary-key`); placement mapping (Region, Zone, MachineType, Image, ignored fields); image source; cloud-init delivery; machine type mapping; network and IP discovery (`ip-source`, `readiness`); GC marks and CreatedAt (`gc-mark`, `created-at`); state mapping table incl. not yet started (`provisioning-state`); Create blocking and task tracking (`create-return`, `async-ops`); Delete and secondary resources (`delete-return`, `secondary-resources`); ListByTag scope and partial failure (`list-partial`); error classification table (`not-found`, `quota`); credentials and TLS (`credentials`); config block fields, defaults, validation; Checker targets, reasons, Details; SDK vs thin client and child spans (`client-kind`, `child-spans`); timeouts (`timeouts`); concurrency; logging tiers (`logging`); tests (`backend-test`); deployment (mounts, samples, Makefile helper, RBAC, dev config); docs; known limits accepted at design time. + +### 3. `docs/providers-existing.md` (new, the appendix) + +Sources: current `providers.md` §6 table, §7 matrix and per-provider notes, §9 per-provider gaps and proposed features, `docs/kvm-provider.md`, the gcp and kubernetes package facts. Links to architecture.md §5, §11 and Decisions, and to the README table, rather than duplicating them. + +Sections: preamble; **profile table** with exactly one row per contract axis code and one column per existing provider, each cell a profile name plus a symbol as evidence; **other differences** (matrix rows that are not axes: Placement used, Port used, CloudInit delivered, states produced, Error.Provider correctness, credential rotation, connection handling, Checker Details, fakes, proxy software, reachability, distinct egress, adopted-UID check); **observability status** (current §6 table); one section each for **kubernetes**, **gcp**, **kvm** with the same sub-headings: package layout and seam, config block, ID format, Create, Get and state mapping table, IP source, Delete, ListByTag, classify table, Check (targets, reasons, Details), logging, tracing layer, tests and fakes, deployment bits, known gaps; **gaps shared by the existing providers**; **proposed features** (current list, unchanged). + +### 4. `docs/providers.md` (rewritten in place as the index) + +One paragraph on the split; one line each for contract, template, appendix; the per-provider guides (`docs/kvm-provider.md`, `docs/gcp-vm-validation.md`, `docs/gcp-in-specific-project.md`); the architecture.md sections; reading order for a new provider; a note that the pre-split blueprint is in git history under this path. + +### 5. One-line link edits + +- `README.md` "### Adding a provider": point at the contract, appendix and template, with `docs/providers.md` named as the index. Keep the "Start there" sentence. +- `CLAUDE.md` "## Architecture": same replacement for the providers.md sentence. +- `docs/architecture.md` components row "Provider contract": rules in `provider-contract.md`, profile table in `providers-existing.md`. +- Historical files under `docs/plans*` keep their links; they resolve to the index. + +## Steps (one commit per step, on `feat/provider-blueprint`) + +- **Step 0**: copy this plan to `docs/plans/-provider-docs-split.md` with the real timestamp and commit it alone, before anything else. +- **Step 1**: write `docs/provider-contract.md`; freeze the axis code list. Create `docs/plans-executions/-provider-docs-split.md` with the status checklist. +- **Step 2**: write `docs/provider-spec-template.md` from contract §4 and §12. +- **Step 3**: write `docs/providers-existing.md`, replacing every `file:line` from the old blueprint with `file` plus symbol. +- **Step 4**: rewrite `docs/providers.md` as the index; apply the three link edits. +- **Step 5**: run the verification below; fix findings. +- **Step 6**: final execution summary entry. CHANGELOG entry at the top only after the user confirms. Push and MR only when the user asks (per memory, push uses the token helper and the MR goes through the Gitea API). + +## Verification (read-only shell, from the repo root) + +1. Every MUST names a caller: `grep -n MUST docs/provider-contract.md` and confirm each bullet cites a shared-layer path or symbol and states what breaks. +2. Every cited path exists: extract `(internal|cmd|config|docs|api|hack|test)/...` tokens from the four files and `test -e` each. +3. Every cited symbol exists: extract backticked identifiers, drop paths, keywords and YAML keys, and `grep -rqw` each across `*.go`, `Makefile`, `*.yaml`. +4. Contract is abstract: `grep -niE "kubernetes|gcp|kvm|libvirt|squid|google|compute engine|internal/provider/(kubernetes|gcp|kvm)" docs/provider-contract.md` is empty. +5. Profiles line up 1:1: for each axis code from contract §4 headings, it appears in the appendix table and in the template; the appendix table has one row per code. +6. Links resolve: extract `](target)` from the four files plus README, CLAUDE.md and architecture.md; check file targets exist and anchors match slugified headings. +7. No em-dashes in the four files or in the added lines of the three edited files. +8. No line references: `grep -nE '\.go:[0-9]+|:[0-9]+-[0-9]+' ` is empty. +9. Nothing lost: compare the gap and proposal bullets of `git show HEAD:docs/providers.md` against the appendix and contract §13 by keyword. +10. `go build ./... && go test -short ./...` still pass (docs-only change). + +## Execution summary must record + +- Step 1: the frozen axis list; the two contract additions found in verification (sequential GC sweep; the Stopped branch re-deleting a not-yet-started instance); per-call timeout facts as verified for each existing provider. +- Step 3: the fact corrections applied (kvm Details keys, kubernetes seam, kvm `isClassified`); matrix rows dropped in favour of the profile table. +- Step 4: the exact link edits. +- Step 5: the verification commands as run with trimmed output. +- Worth noting: CHANGELOG headings keep the repo's own em-dash format; the stale `mock` comments in `provider.go`, `registry.go` and `proxy_types.go` stay listed as gaps and are not fixed here. From 8d764de336b3b64f5c3e77a7dd5b0d5af0314efb Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:17:56 +0000 Subject: [PATCH 06/11] Write the generic provider contract Fully abstract: MUST/SHOULD/MAY rules with the caller behind each one, 19 behaviour-profile axes with stable codes, credential hygiene, testing obligations, the checklist and the contract's known limits. Co-Authored-By: Claude Fable 5.1 --- .../2026-09-16-1108-provider-docs-split.md | 65 ++ docs/provider-contract.md | 860 ++++++++++++++++++ 2 files changed, 925 insertions(+) create mode 100644 docs/plans-executions/2026-09-16-1108-provider-docs-split.md create mode 100644 docs/provider-contract.md diff --git a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md new file mode 100644 index 0000000..b1a6fc9 --- /dev/null +++ b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md @@ -0,0 +1,65 @@ +# Execution: Split the provider blueprint into contract, appendix, template and index + +Plan: [docs/plans/2026-09-16-1108-provider-docs-split.md](../plans/2026-09-16-1108-provider-docs-split.md) + +- [x] Step 0 — Save and commit the plan +- [x] Step 1 — Write `docs/provider-contract.md` +- [ ] Step 2 — Write `docs/provider-spec-template.md` +- [ ] Step 3 — Write `docs/providers-existing.md` +- [ ] Step 4 — Rewrite `docs/providers.md` as the index; link edits +- [ ] Step 5 — Verification sweep +- [ ] Step 6 — Final summary, CHANGELOG after confirmation + +## Step 0 — Save and commit the plan + +Committed as `cf89ac4` on the existing `feat/provider-blueprint` branch, on +its own, before any document work. The pre-existing local modification of +`.claude/settings.json` was left out of the commit. + +## Step 1 — Write `docs/provider-contract.md` + +2026-09-16 11:17. Written from scratch rather than edited down from the old +blueprint, so that nothing provider-specific could survive by accident. Every +MUST names its caller by function (`reconcileManaged`, `replaceInstance`, +`reconcileDelete`, `providerFailure`, `deletionFailure`, `Sweeper.sweep`, +`Monitor.checkOnce`, `Monitor.run`, `registry.Build`) and states what breaks. + +**Axis codes frozen** (19, §4.1 to §4.19): `readiness`, `create-return`, +`delete-return`, `ip-source`, `gc-mark`, `created-at`, `list-partial`, +`quota`, `not-found`, `primary-key`, `provisioning-state`, `async-ops`, +`secondary-resources`, `credentials`, `client-kind`, `timeouts`, `logging`, +`child-spans`, `backend-test`. The appendix table and the template are keyed +on these strings; the §4.20 summary table's last column gives the template +entry number each axis maps to. + +**Two rules that were not in the old blueprint**, both found by reading the +callers while verifying: + +- `Sweeper.sweep` loops providers sequentially in one goroutine, and no + caller puts a deadline on the context (only `Monitor.checkOnce` uses + `WithTimeout`). So a hung `ListByTag` stalls orphan GC for every provider. + Recorded in §1.5 and the `timeouts` axis. +- The Stopped branch of `reconcileManaged` calls `Delete` and requeues + without clearing `ProviderID`, then deletes again on every poll until + NotFound. A backend that reports a freshly created, never started instance + as stopped would have it deleted right after creation. Recorded as a MUST + in §2.2 and as the `provisioning-state` axis with three allowed profiles. + +Checks run on the draft (all clean): + +```sh +grep -niE "kubernetes|gcp|kvm|libvirt|squid|google|compute engine" docs/provider-contract.md +grep -n "—" docs/provider-contract.md +grep -nE '\.go:[0-9]+|:[0-9]+-[0-9]+' docs/provider-contract.md +grep -ohE "(internal|cmd|config|docs|api|hack|test)/[A-Za-z0-9_./-]+" docs/provider-contract.md | sort -u | while read p; do test -e "$p" || echo "MISSING $p"; done +``` + +The path check reports three expected misses: `config/samples/proxy_.yaml` +is a pattern, `docs/provider-spec-template.md` is written in Step 2, and +`internal/provider/providertest` is a proposal named in §13. + +Worth noting: the contract refers to "the operator pod" and "the cluster's +own API" rather than naming the platform, so that the abstractness grep stays +meaningful. The `hack/providers-dev.yaml` rule is phrased as "only if the type +needs no credentials" because that file exists for `make run-dev` without any +Secret. diff --git a/docs/provider-contract.md b/docs/provider-contract.md new file mode 100644 index 0000000..6181de3 --- /dev/null +++ b/docs/provider-contract.md @@ -0,0 +1,860 @@ +# The provider contract + +This document is the specification a provider must meet. It is written for +whoever builds the next one, human or agent, and it deliberately says nothing +about how any existing provider is built. Those details are in +[providers-existing.md](providers-existing.md); the questionnaire a new +provider's design must answer is [provider-spec-template.md](provider-spec-template.md). +Read the three in that order: this contract, then the template, then the +appendix for worked examples. + +Rule keywords: **MUST** means breaking the rule breaks a caller, and every MUST +below names the caller and what breaks. **SHOULD** means every current provider +does it and a new one needs a written reason not to. **MAY** means it is +allowed and the choice must be recorded in the provider's spec. + +Code references are a file path plus a function or symbol name, never a line +number. Where a rule is enforced by a caller, that caller is one of: + +- the **reconciler**, `internal/controller/proxy_controller.go`; +- **orphan GC**, `internal/gc/gc.go`; +- the **provider health monitor**, `internal/providerhealth/monitor.go`; +- the **decorators**, `internal/provider/metrics.go` and + `internal/provider/tracing.go`; +- the **composition root**, `cmd/main.go`. + +## 1. What a provider is and who calls it + +### 1.1 The types + +A provider is a Go type implementing `provider.Provider` +(`internal/provider/provider.go`): four methods over one VM-like instance, +identified by an opaque `providerID` string. + +- `Create(ctx, CreateRequest) (providerID, error)` +- `Get(ctx, providerID) (*Instance, error)` +- `Delete(ctx, providerID) error` +- `ListByTag(ctx) ([]Instance, error)` + +`CreateRequest` carries `Name` (from `NameFromUID` in `internal/provider/name.go`, +22 characters, `proxy-` plus base32), `UID` (the owning Proxy's UID), +`Namespace`, `ProxyName`, `Placement` (`Region`, `Zone`, `MachineType`, +`Image`), `CloudInit` (already resolved from inline text or a Secret) and +`Port`. + +`Instance` carries `ID`, `IP`, `State` (one of `StateProvisioning`, +`StateRunning`, `StateStopped`, `StateTerminated`), `UID` and `CreatedAt`. + +The GC marks are the constants `LabelManaged` (value `LabelManagedYes`) and +`LabelUID`, also in `provider.go`. + +Providers are named instances of a type: two entries of one type with +different config blocks are two providers. The configured name is what +`Proxy.spec.provider` refers to, and it is immutable, so a Proxy never moves +between providers. + +### 1.2 The callers + +Nothing above the provider watches backend resources. Everything the operator +knows about an instance comes from polling `Get`. + +- **The reconciler** calls `Create`, `Get` and `Delete` from `reconcileManaged`, + `replaceInstance` and `reconcileDelete`, and branches only on the error class + (`providerFailure`, `deletionFailure`) and on `Instance.State`. It maps the + CRD's placement to `Placement` one to one in `placementFrom`. Its poll + intervals are set in `applyDefaults`: `ProvisioningPoll` and `DeletionPoll` + 10s, `DriftPoll` 2m, `RequeueNow` 1s, `QuotaRetry` 5m. Up to 3 Proxies + reconcile at once (`MaxConcurrentReconciles` in `SetupWithManager`). +- **Orphan GC** calls `ListByTag` and `Delete` from `Sweeper.sweep`, every + `Interval` (10m), and never deletes an instance younger than `MinAge` (10m). + It loops over providers one after another in a single goroutine. +- **The provider health monitor** calls the optional `Checker` from + `Monitor.checkOnce`, every `--provider-check-interval`, all providers + concurrently under one timeout. +- **The composition root** builds every provider through `registry.Build` + (`internal/provider/registry/registry.go`) before the manager starts, on + every replica, then wraps each one in `WithTracing(WithMetrics(...))`. + +**Where the calls come from.** The reconciler, GC, the health engine and the +monitor run on the leader only. The constructors run on every replica at +startup. + +**Concurrency.** Up to 3 reconciles, a GC sweep and a health-check round can +call the same provider at the same time, and GC's `Delete` can overlap the +reconciler's `Delete` for the same instance. A provider **MUST be safe for +concurrent use, including two calls for the same instance name**. Nothing +above the provider serialises them. + +### 1.3 What the instance is for + +The instance is a forward proxy with its own egress path. Two parties connect +to `http://:`: + +- the health engine, from the operator pod, which sends a real `CONNECT` + through it (`probe` in `internal/health/probe.go`); +- clients, which get `ip` and `port` from the discovery API (`viewOf` in + `internal/discovery/handlers.go`). + +So the IP a provider reports **MUST be reachable on that port from the +operator's pod network and from the clients**. Whatever that takes on the +backend (firewall rules, security groups, routing) is either done by the +provider or documented as a prerequisite in the provider's guide. + +The provider does not know which proxy software runs. The proxy is installed +and configured by the Proxy's `cloudInit`; building install logic into the +operator is a stated non-goal (`docs/prompts/__initial-prompt.md`, non-goals). +A backend that cannot take user-data (see `readiness` in §4) documents that +it ignores the field and how the proxy gets configured instead. + +### 1.4 Where it runs + +The manager container (`config/manager/manager.yaml`) runs as non-root with a +read-only root filesystem, a 64Mi `emptyDir` at `/tmp` and a 128Mi memory +limit, from a distroless static image (`Dockerfile`) that carries the public +CA roots and nothing else. So a provider: + +- **MUST stream large payloads** rather than buffering them in memory; +- **MUST write scratch files only under `/tmp`**, and delete them; +- gets credentials either as mounted files (the providers ConfigMap at + `/etc/proxy-operator`, optional Secret mounts) or from the environment or + the pod's own identity. See §7. + +### 1.5 Time and cancellation + +**No caller puts a deadline on the context.** The reconcile and GC contexts +are cancelled on shutdown and on loss of leadership, and otherwise never. +The only bounded context is the health monitor's (`Monitor.checkOnce`, one +timeout per round). + +- **MUST honour `ctx`**: return promptly once it is cancelled, on every + method. +- **SHOULD bound every backend call itself**: a dial timeout, a per-request + timeout, a cap on any wait loop. What breaks otherwise: a hung `Create`, + `Get` or `Delete` pins one of the 3 reconcile workers until the manager + stops; a hung `ListByTag` stalls orphan GC for every provider, because + `Sweeper.sweep` runs them sequentially; a `Check` that ignores its context + wedges a monitor round. Choose budgets against the polls: a `Get` that + routinely outlives `ProvisioningPoll` gains nothing. + +## 2. The contract, method by method + +### 2.1 Create(ctx, CreateRequest) (providerID, error) + +- **MUST be idempotent by `req.Name`.** A repeat call for a name that already + exists returns that instance's providerID with a nil error, creates no + duplicate and returns no error. + *Caller:* the reconciler records the ID in status only after `Create` + returns (`reconcileManaged`). A crash between the two, or a lost status + write, runs `Create` again for the same Proxy; `replaceInstance` recovers + from a crash between its two writes the same way. +- **MUST mark the instance as managed**, with `LabelManaged=LabelManagedYes` + and `LabelUID=req.UID` or the backend's equivalent, in the same call that + creates it. When the backend's create call cannot carry the mark, the + window between creation and marking **MUST be far shorter than `MinAge`** + and documented in the provider's spec (`gc-mark` in §4). + *Caller:* orphan GC finds instances only through the mark and never deletes + one whose UID is empty (`Sweeper.sweep`). An instance that exists without + the mark leaks if the operator crashes before marking it. +- **MUST return a providerID that is self-contained.** `Get` and `Delete` + receive only the ID, and the config or spec may have changed since + `Create` (a zone edit during a replacement). Put whatever `Get` needs to + find the instance into the ID, and validate config values that end up in + the ID so they cannot contain the separator. See `primary-key` in §4 for + backends that allocate their own identifier. +- **MUST NOT let `Get` answer NotFound for an ID that `Create` just + returned.** A backend with eventual consistency between create and read + needs a read-after-write guarantee or a retry inside `Create`. + *Caller:* on NotFound the reconciler clears the ID and creates again + (`reconcileManaged`), so a lagging read makes a duplicate. +- **MUST honour `ctx`** (§1.5). +- **MAY return once the request is submitted, or block until the instance is + running** (`create-return` in §4). Submit-and-return is preferred: readiness + is discovered by polling `Get`, and a blocking `Create` holds one of the 3 + workers for its whole duration. A provider that blocks MUST still honour + cancellation and SHOULD cap the wait. +- **SHOULD reject a request it can never satisfy** with `ErrPermanent` + before touching the backend: a missing or unknown zone, machine type or + image, or any placement value not in its catalogue. The CRD cannot 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 do not replace instances.** The spec hash + (`specHashInput` in `internal/controller/spechash.go`) covers `placement`, + the resolved `cloudInit` and `port`, not the provider config. When a + placement value names a config entry, changing what that entry means affects + only new instances. The provider's guide MUST say so and tell users to add a + new key rather than edit one in use. +- **MAY ignore `Placement` fields that do not apply.** If a field is ignored, + the provider's guide MUST say so, because the field is still part of the + spec hash: changing it replaces the instance and has no other effect. +- **SHOULD deliver `CloudInit` as user-data**, byte for byte, without parsing + or re-rendering it. A provider that cannot documents that it ignores the + field. +- **MUST NOT log `CloudInit` contents**, put them in span attributes, or + include them in error messages. It can hold secrets. Log its byte count. + Test this with a sentinel string (§11). + +### 2.2 Get(ctx, providerID) (*Instance, error) + +- **MUST return `ErrNotFound` when, and only when, the instance is gone.** + *Caller:* `ErrNotFound` triggers three actions. In normal reconciling the + reconciler clears the ID and creates again (`reconcileManaged`). During + replacement it is the signal to create the new instance + (`replaceInstance`). During deletion it releases the finalizer + (`reconcileDelete`). A false NotFound during an outage creates a duplicate + or loses the instance. "Unreachable", "wrong credentials" and "cannot tell" + are Transient or Permanent, never NotFound. +- **MUST keep returning the instance, not NotFound, while a delete is in + progress** and the name is still taken. + *Caller:* the replacement instance gets the same name. `replaceInstance` + waits for NotFound before creating it; a NotFound too early makes `Create` + adopt the dying instance. +- **MUST report `StateRunning` only with a non-empty `IP`.** Running without + an address yet is `StateProvisioning`. + *Caller:* the IP from a Running answer goes straight into status and to the + health engine (`reconcileManaged`). +- **MUST report `StateStopped` or `StateTerminated` only when the instance + will not recover by itself.** SHOULD map states it does not recognise to + `StateStopped`. + *Caller:* the reconciler reacts to both states identically: it calls + `Delete` and requeues after `DeletionPoll` without clearing the ID, then + polls `Get` and deletes again until NotFound, then creates + (`reconcileManaged`). Instances are disposable, so this is the safe answer + for anything unknown, but it makes the next rule critical. +- **MUST NOT report a freshly created instance that has not been started yet + as `StateStopped`.** A backend with no provisioning state of its own, where + "defined but not yet started" and "stopped" look the same, MUST resolve this + in one of two ways (`provisioning-state` in §4): block `Create` until the + instance is started, or record enough at creation (written metadata, a task + handle) for `Get` to answer `StateProvisioning`. + *Caller:* the same branch of `reconcileManaged` would delete the instance + right after it was created, forever. +- **During an asynchronous operation or a backend lock** (a clone or a delete + still running, an instance locked by the backend), `Get` MUST return either + the last known state or `ErrTransient`, never NotFound and never + Permanent (`async-ops` in §4). +- **MUST fill `UID` and `CreatedAt` by reading them back from the backend**, + never from memory. + *Caller:* GC compares `UID` with the live Proxies and deletes anything whose + `CreatedAt` is older than `MinAge`. **A zero `CreatedAt` counts as very + old** (`Sweeper.sweep`), so a provider that forgets it makes instances still + being created collectable. An unparsable timestamp giving the zero time is + accepted: it never shields an orphan forever. +- **SHOULD return `ErrPermanent` for an ID it can never resolve**: a malformed + ID, a location no longer in the config. + *Caller:* `reconcileDelete` then tries `Delete` anyway and releases the + finalizer if that works. If the provider returned Transient, deletion would + poll forever. + +### 2.3 Delete(ctx, providerID) error + +- **MUST be idempotent.** Deleting something already gone returns nil. + *Caller:* GC and the reconciler race, and a crash repeats the call. +- **MUST accept being called again while an earlier delete is still in + progress.** Treat "already deleting" as success, and SHOULD keep that cheap. + *Caller:* while `Get` still returns the instance, `reconcileDelete` and the + Stopped branch of `reconcileManaged` call `Delete` again every + `DeletionPoll` until `Get` returns NotFound. +- **MUST remove everything `Create` made**: disks, volumes, seeds, addresses, + uploaded files. Either delete them explicitly or rely on the backend + cascading (`secondary-resources` in §4). + *Caller:* GC sees only what `ListByTag` returns, so secondary resources + have no other cleanup path. +- **MAY return before the instance is gone** (`delete-return` in §4), as long + as `Get` keeps showing it until it is. +- **SHOULD succeed for an ID whose `Get` fails permanently** when there is + nothing left the provider could reach, and log that it did nothing. + *Caller:* on the deletion path a permanent error is not latched; it retries + forever (`deletionFailure`), and the Proxy is then only released through the + `AnnotationSkipCleanup` annotation (`api/v1alpha1/proxy_types.go`). + +### 2.4 ListByTag(ctx) ([]Instance, error) + +- **MUST return only instances that carry the managed mark**, each with an + `ID` that `Delete` accepts, plus `UID` and `CreatedAt`. `State` and `IP` + are not used by GC. + *Caller:* GC deletes every listed instance whose UID is not live and whose + age is past `MinAge` (`Sweeper.sweep`). Listing something without the mark + is how an operator deletes an instance it does not own. +- **MUST return an error rather than a list it cannot vouch for.** An error + makes GC skip that provider for this sweep (`Sweeper.sweep`). +- **MAY leave out instances it could not read.** Leaving something out is + always safe, because GC never deletes what is not listed. Pick a policy + (`list-partial` in §4) and document it. +- **SHOULD cover the whole scope the provider can create in**: every + namespace, zone, node or host. Anything outside that scope is never + collected. +- **Scope caveat:** GC protects instances by UID, not by installation. Two + operator installs sharing one backend delete each other's instances (§13). + Two provider entries of one install that share a scope are safe, because + they share the live-UID set. + +## 3. What a container-style backend and a VM-style backend have in common + +The contract above is the same for both, but the profiles in §4 differ. +A backend that runs a container has an instance that is ready seconds after +`Create`, gets its address from the platform, is deleted in one call and has +no secondary resources; a backend that boots a VM has a provisioning window +of minutes, may discover the address only after the guest reports it, may +need several deletes, and leaves disks and seeds behind unless told +otherwise. Neither is preferred. What matters is that the provider's spec +names its profile on every axis, because the appendix table and the test list +are keyed by those names. + +## 4. Behaviour profiles + +Each axis below has a stable code. The provider's spec (the template) answers +every axis; the appendix maps each existing provider to its profile on every +axis. A profile is not a permission to skip a MUST in §2; it says which of +the allowed ways of meeting it the provider chose. + +### 4.1 `readiness` + +*When is an instance usable after `Create`?* + +- **immediate**: the platform starts it at once; `Get` reports Running + within a poll or two. +- **provisioning-window**: minutes of booting; `Get` reports Provisioning + for that time. + +Callers tolerate both: the reconciler polls at `ProvisioningPoll` either +way. An immediate provider still MUST report Provisioning until the IP is +known. + +### 4.2 `create-return` + +*Does `Create` return once the request is submitted or once the instance is +running?* + +- **submit**: preferred. Readiness comes from `Get`. +- **blocking**: allowed. Costs one of 3 workers for the duration, and any + first-use setup (an image download) runs inside it. MUST honour `ctx`, + SHOULD cap the wait, and MUST keep every step idempotent so a cut-off + attempt resumes. + +### 4.3 `delete-return` + +*Does `Delete` return before the instance is gone?* + +- **returns-early**: `Get` keeps returning the instance until it is gone + (§2.2), and repeated `Delete` calls are cheap. +- **waits**: `Delete` returns nil only when nothing is left. Bound the wait + and honour `ctx`. + +### 4.4 `ip-source` + +*Where does `Instance.IP` come from?* + +- **assigned-at-create**: the backend reports it with the instance. +- **discovered**: an agent in the guest, a DHCP lease, a neighbour table. + Discovery failures mean "no address yet", never an error; `Get` returns + Provisioning. The provider's guide MUST say what the guest needs (an agent + package, outbound access to install it) and what happens without it. + +### 4.5 `gc-mark` + +*Where do `LabelManaged` and `LabelUID` live?* + +- **native-labels**: labels or tags the backend indexes; `ListByTag` can + filter server-side. +- **written-metadata**: a description, an annotation or a metadata element + written by the provider; `ListByTag` lists everything and filters + client-side. +- **mark-window**: the create call cannot carry the mark and a second call + writes it. Allowed only when the window is a few seconds, documented, and + the unmarked instance is cleaned by the same Proxy's next reconcile (§2.1). + +### 4.6 `created-at` + +*Where does `CreatedAt` come from?* + +- **backend-timestamp**: the backend's own creation time. +- **written-at-create**: the provider writes the time into the mark + metadata in the create call. A backend with no timestamp MUST do this. + +### 4.7 `list-partial` + +*What does `ListByTag` do when part of its scope is unreachable?* + +- **fail-whole**: one unreachable part fails the list; GC skips the provider + this sweep. Nothing is collected until every part answers. +- **omit-unreachable**: unreachable parts are left out and the rest is + returned. Orphans in the reachable parts are collected; the provider MUST + log what it left out. + +Both are safe (§2.4). A single unreadable instance MAY always be skipped. + +### 4.8 `quota` + +*Can the provider tell "no capacity" from other failures?* + +- **detected**: a recognisable quota, rate-limit or out-of-resources signal + maps to `ErrQuotaExceeded`. +- **absent**: capacity errors map to `ErrTransient`. The guide MUST say so, + and the `Checker` SHOULD expose a capacity figure in `Details` instead. + +### 4.9 `not-found` + +*How does the provider know an instance is gone?* + +- **status-404**: a distinct not-found status or error code from the + backend. +- **error-code**: a code or message the backend uses for "does not exist", + matched exactly and covered by the classify table test with real captured + responses. +- **list-and-miss**: absence from a listing. Allowed only when the listing + can never be partial; otherwise a missing part reads as "gone" and §2.2 is + broken. + +### 4.10 `primary-key` + +*Is `req.Name` the backend's key?* + +- **name-is-key**: the backend addresses instances by name. `Create` looks + the name up, the ID is `/`. +- **allocated-id**: the backend allocates an identifier and the name is a + label. `Create` MUST first find an existing instance by name (idempotency), + then allocate. Allocation MUST be safe against a concurrent `Create` for + another Proxy: a deterministic identifier derived from `req.UID`, or an + allocate-then-verify loop. The ID SHOULD embed the allocated key and the + name; `Get` and `Delete` use the allocated key. If instances can move + between locations (migration), the ID MUST NOT depend on the location. + +### 4.11 `provisioning-state` + +*Does the backend have a state for "being created"?* + +- **native**: the backend reports a provisioning or staging state, mapped to + `StateProvisioning`. +- **create-blocks**: no such state; `Create` blocks until the instance is + started, so `Get` never sees "defined but not started". +- **marker**: no such state and `Create` returns early; the provider records + at creation (metadata or a task handle) what lets `Get` answer + `StateProvisioning` for an instance that was never started, and + `StateStopped` for one that stopped afterwards. + +### 4.12 `async-ops` + +*Does the backend run create and delete as asynchronous tasks?* + +- **none**: calls complete synchronously. +- **task-handle**: the backend returns a task the provider can poll. The + provider SHOULD keep the handle where `Get` can find it (metadata or the + ID) so a failed create can be reported instead of read as NotFound. +- **backend-lock**: the backend locks the instance during a task. `Get` + returns the last known state or Transient while locked (§2.2); `Delete` + on a locked instance returns Transient, not Permanent. + +### 4.13 `secondary-resources` + +*What else does `Create` make, and who removes it?* + +- **none**. +- **cascade**: the backend deletes them with the instance (auto-delete + flags). The provider MUST set those flags at create. +- **explicit**: the provider deletes them by name in `Delete`, tolerating + their absence. Resources that can exist without an instance (a crash + between creating them and creating the instance) are a documented GC blind + spot unless `ListByTag` covers them. + +### 4.14 `credentials` + +*Where do credentials and trust material come from?* + +- **mounted-files**: paths in the config block, pointing into an optional + Secret mount (§7). +- **environment**: standard SDK environment discovery. +- **workload-identity**: the pod's own identity. + +TLS trust for an HTTPS backend with a private CA is one of **system-roots**, +**ca-file** (a path in the config block) or **insecure** (a boolean whose +name starts with `insecure`, warned about at startup). + +### 4.15 `client-kind` + +*What talks to the backend?* + +- **vendor-sdk**: put it behind a small interface (the seam) so a fake can + stand in. +- **thin-client**: a `net/http` client written in the provider package. + Preferred when no maintained SDK exists (`CLAUDE.md`: stdlib over + third-party). It MUST set transport timeouts, MUST carry the backend's + status and body in one error type that `classify` reads, and SHOULD wrap + its transport for child spans (§9). +- **rpc-over-ssh** or another non-HTTP transport: one pooled connection per + target, a keepalive, and a documented answer to "what does a cancelled call + do to the shared connection". + +### 4.16 `timeouts` + +*How is each backend call bounded?* (§1.5) + +- **per-call**: every request has its own deadline, independent of `ctx`. +- **ctx-only**: the provider relies on `ctx`. Allowed only for backends + whose client already enforces a timeout; the spec MUST cite it. + +### 4.17 `logging` + +*Which log tiers does the provider produce?* The logger MUST come from +`logf.FromContext(ctx)` at call time (§9). + +- **tiered**: V(1) one line per operation outcome and per failed backend + call; V(2) request and response detail; V(5) raw wire if there is one. +- **outcome-only**: V(1) only. +- **none**: allowed only for a backend whose client logs on its own. + +### 4.18 `child-spans` + +*How do backend calls appear under the operation span?* (§9) + +- **sdk-instrumentation**: the SDK emits spans itself. +- **transport-wrapper**: a parent-gated `otelhttp` transport. +- **span-decorator**: a decorator over the seam that starts one span per + call. +- **none**: a documented gap, with log correlation through `traceID` only. + +### 4.19 `backend-test` + +*How is the provider proven against the real backend?* + +- **cluster-e2e**: the kind end-to-end suite. +- **integration-tag**: a test behind `//go:build integration`, skipped + without its environment variables, with a `make test-integration-` + target and a section in `docs/testing.md`. +- **manual-runbook**: a documented by-hand run. Allowed only when the + backend cannot be reached from CI or a developer machine. + +### 4.20 Summary + +| Axis | Profiles | Who tolerates the difference | Template entry | +|---|---|---|---| +| `readiness` | immediate, provisioning-window | reconciler polls either way | 7 | +| `create-return` | submit, blocking | reconciler; worker budget | 10 | +| `delete-return` | returns-early, waits | reconciler re-calls Delete | 11 | +| `ip-source` | assigned-at-create, discovered | reconciler gates Running on IP | 7 | +| `gc-mark` | native-labels, written-metadata, mark-window | GC | 8 | +| `created-at` | backend-timestamp, written-at-create | GC `MinAge` | 8 | +| `list-partial` | fail-whole, omit-unreachable | GC | 12 | +| `quota` | detected, absent | reconciler `QuotaRetry` | 13 | +| `not-found` | status-404, error-code, list-and-miss | reconciler | 13 | +| `primary-key` | name-is-key, allocated-id | reconciler idempotency | 2 | +| `provisioning-state` | native, create-blocks, marker | reconciler Stopped branch | 9 | +| `async-ops` | none, task-handle, backend-lock | reconciler | 10 | +| `secondary-resources` | none, cascade, explicit | GC blind spots | 11 | +| `credentials` | mounted-files, environment, workload-identity | deployment | 14 | +| `client-kind` | vendor-sdk, thin-client, rpc-over-ssh | tests, tracing | 17 | +| `timeouts` | per-call, ctx-only | worker and GC budget | 18 | +| `logging` | tiered, outcome-only, none | operators | 20 | +| `child-spans` | sdk-instrumentation, transport-wrapper, span-decorator, none | tracing | 17 | +| `backend-test` | cluster-e2e, integration-tag, manual-runbook | CI | 21 | + +## 5. Error taxonomy + +Every error a provider method returns **MUST be built with +`provider.Wrap(class, op, providerName, id, err)`** (`internal/provider/errors.go`). +`op` is one of `create`, `get`, `delete` or `list`, and `providerName` is the +**configured name** (`ProviderConfig.Name`), not the type. +*Caller:* the decorators label metrics and spans with the class +(`resultLabel`), and the reconciler branches on `provider.Class`. + +`Wrap` keeps both `errors.Is(err, class)` and `errors.As` to the backend's +error type working (`Error.Unwrap` returns both). + +| Class | Use it for | During normal reconciling | While deleting | +|---|---|---|---| +| `ErrNotFound` | the instance does not exist | Get: recreate or advance (§2.2); other ops: returned as the error, so exponential backoff | Get: release the finalizer | +| `ErrQuotaExceeded` | quota, rate limit, out of capacity | `Provisioned=False` with reason `QuotaExceeded`, requeue after `QuotaRetry`, not counted as an error (`providerFailure`) | requeue after `QuotaRetry` (`deletionFailure`) | +| `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same | +| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: `Provisioned=False` with reason `PermanentError`, no more provider calls until `metadata.generation` changes (`reconcileManaged`) | error returned and retried forever; a Permanent `Get` also triggers `Delete` (`reconcileDelete`) | + +Pitfalls: + +- **Unclassified means Transient.** `provider.Class` returns `ErrTransient` + for any error not built with `Wrap`. That is the safe default, but a + permanent failure that was never classified retries forever, and a quota + error backs off far too fast. +- **Permanent is sticky.** Use it only for errors that editing the Proxy spec + or fixing the config can resolve. A permission-denied status that the + backend also uses for quota is the hard case: read the backend's reason if + it gives one, and if it does not, pick Permanent and document it. +- **Do not return NotFound from `Create`.** The reconciler treats it as + Transient there, but it misleads the metrics and anyone reading logs. +- **NotFound detection** follows the `not-found` profile (§4.9). A backend + that reports "does not exist" as a generic server error needs the exact + message or code matched, with real captured responses in the test table. +- **Classify in one `classify` function** with a table-driven test covering + every class, including "not a backend error at all, so Transient". + +## 6. Identity, names, marks, CreatedAt + +- **Names:** `NameFromUID(uid)` gives `proxy-` plus 16 lowercase base32 + characters. It is a valid DNS label. A backend with stricter rules SHOULD + map it deterministically rather than invent a new scheme, because + idempotency depends on the name being the same every time. A backend that + keys on an allocated identifier follows `primary-key` (§4.10). +- **providerID:** opaque to everyone except the provider. Keep it + `/`-shaped or `/`-shaped, parse + it strictly, and return `ErrPermanent` for a malformed one (§2.2). +- **Marks:** `LabelManaged` with value `LabelManagedYes`, and `LabelUID`. + Use native labels or tags where the backend has them; otherwise write them + into metadata in the create call (§4.5). +- **`CreatedAt`:** the backend's creation timestamp, or written into the mark + metadata at creation (§4.6). +- **Provider names:** a configured `name` MUST be a lowercase DNS-1123 label + of at most 63 characters (`spec.provider` in + `api/v1alpha1/proxy_types.go`). The `ProviderHealth` object is named after + the provider; `Monitor.Start` warns and `Monitor.publish` skips the object + for a name that is not a valid object name. Config loading does not check + this yet (§13). + +## 7. Credentials, TLS and secret hygiene + +- **The constructor MUST NOT dial the backend.** `registry.Build` aborts the + whole provider set on a constructor error, and the manager exits: one + unreachable backend must not take the other providers down. Fail in the + constructor only on config errors. +- **Credential files SHOULD be read lazily.** A mount marked `optional: true` + is empty when the Secret is missing; the constructor logs a warning and the + first call fails Transient until the Secret lands. A file that exists but + does not parse is Permanent. +- **Credentials MUST NOT appear in logs, span attributes, error strings or + `Error.Err`.** Backend errors that echo a request URL or header with a + token in it MUST be sanitised before wrapping. The same holds for + `CloudInit` (§2.1). Test both with a sentinel (§11). +- **TLS trust** follows `credentials` (§4.14). An insecure toggle MUST be + named so that it reads as insecure, default off, and MUST be logged as a + warning at startup when on. +- **Rotation.** State in the spec whether rotating the Secret takes effect + without a restart. Reading files on every dial makes rotation free; caching + them after first load needs a restart and must say so. +- **Scratch files** only under `/tmp` (§1.4), removed when the call ends. + +## 8. Optional interfaces: `Checker` + +`provider.Checker` is `Check(ctx) []TargetStatus` (`internal/provider/check.go`). +`cmd/main.go` type-asserts it on the **unwrapped** provider before adding the +decorators, and the monitor treats a nil Checker as unsupported, not failed +(`Monitor.run`). + +- **MUST never fail as a whole.** Every failure is a `TargetStatus` with + `Ready=false`. + *Caller:* `Monitor.checkOnce` has no error path per provider. +- **MUST return promptly once `ctx` ends.** The monitor gives the whole round + one timeout. +- **MUST return one entry per target, in a stable order.** Targets are what + can fail independently: an apiserver, a project, each host or node. + *Caller:* the monitor logs transitions per target (`logTransitions`) and + the gauges are labelled `{provider,target}`. +- **MUST NOT disrupt reconciles.** A check that shares a connection with + provisioning must not tear that connection down when it times out. +- **SHOULD use the shared reasons** (`ReasonReachable`, `ReasonTimeout`, + `ReasonUnreachable`, `ReasonUnauthorized`, `ReasonForbidden`, + `ReasonAPIError`). Add provider-specific reasons only for failures an admin + would fix differently, and document each one in the guide. +- **SHOULD check with the same credentials, verb and scope that `ListByTag` + and `Create` use**, and be cheap: one page, one object. +- **MAY fill `Details`** with facts useful when triaging: versions, capacity, + the account in use. +- The `status.type` field of `ProviderHealth` + (`api/v1alpha1/providerhealth_types.go`) lists the known types in its doc + comment; add the new type there and run `make manifests`. + +## 9. Observability + +Tracing is OpenTelemetry, set up in `internal/tracing` from the standard +`OTEL_*` environment variables. With no exporter configured, spans are no-ops +and logs are unchanged ([architecture.md §10](architecture.md#10-tracing-internaltracing)). + +**Layer 1, operation spans and metrics: nothing to write.** `cmd/main.go` +wraps every provider in `WithTracing(WithMetrics(...))`: + +- `WithTracing` opens one client span per call, named `provider.create`, + `provider.get`, `provider.delete` or `provider.list`, with + `provider.name`, `provider.id` and `provider.result` (the error class) as + attributes. `ErrNotFound` keeps the span status Ok, because polling to + NotFound is expected. +- `WithMetrics` counts each call as + `proxy_operator_provider_requests_total{provider,op,result}` + (`internal/metrics/metrics.go`). A `Checker` feeds + `proxy_operator_provider_target_up`, + `proxy_operator_provider_check_duration_seconds` and + `proxy_operator_provider_check_timestamp_seconds`, labelled + `{provider,target}`. + +Rules: + +- **MUST NOT open its own per-operation span or count its own requests.** + They would duplicate the decorators. +- **MUST pass the caller's `ctx` to every backend call and take the logger + from `logf.FromContext(ctx)` at call time.** `tracing.StartSpan` + (`internal/tracing/logger.go`) puts a logger carrying `traceID` and + `spanID` into `ctx`. A logger cached at construction, or a backend call + made with `context.Background()`, loses the trace. +- **Health checks bypass the decorators.** A `Checker` runs under the + monitor's `providerhealth.check` span, and the same `ctx` rules apply. + +**Layer 2, child spans per backend call: SHOULD.** One operation span says +that `Create` took 40 seconds; child spans say which backend call took them. +Follow `child-spans` (§4.18): + +- HTTP backends: wrap the transport with `otelhttp`, gated on an existing + parent span as `tracing.RestConfigWrapper` does + (`internal/tracing/transport.go`), so background traffic (token refreshes, + keepalives) does not become root spans. Check first whether the SDK already + does this. +- Non-HTTP backends: start child spans yourself with + `tracing.Start(ctx, ".")` around each call and around long + steps such as a dial or a download, in a decorator over the seam, not at + call sites. +- **MUST NOT put secrets or CloudInit in span attributes.** Put identifiers, + sizes and error codes there. + +**Layer 3, logs.** Follow `logging` (§4.17). Log lines join a trace through +the `traceID` key from layer 1. + +## 10. Configuration and wiring + +The providers file is parsed by `LoadConfig` (`internal/provider/config.go`) +with `UnmarshalStrict`, so unknown keys already fail. + +- Add a `Config` struct and a `ProviderConfig` field for it. +- Add the block to `setBlocks`, so a block that does not match `Type` is + rejected. +- Add the type to the allow-list in `Config.validate`. The same list exists a + second time as the constructor map in `cmd/main.go`; both must change + (§13). +- Add required-field validation in its own `config_.go` when it is + more than a few lines, with rows in `config_test.go`. +- **Validate every config value that is pasted into an identifier**: a + location name that becomes part of the providerID must not contain the + separator; a catalogue key that becomes a file or volume name must be a + safe filename. +- The constructor has the signature `registry.Constructor`: + `func(ctx, provider.ProviderConfig) (provider.Provider, error)`. Register + it in the map in `cmd/main.go`. Flags a provider needs are defined there + too and passed through a `NewWith...` variant, never read from the + environment inside the package. +- Decorators are applied by the composition root, in the order + `WithTracing(WithMetrics(p))`, after the Checker has been collected from + the unwrapped provider. + +## 11. Testing obligations + +`CLAUDE.md` applies: stdlib `testing`, table-driven, tests next to source, +`t.Parallel()` where safe. + +- **A fake behind the seam.** Every unit test runs against an in-memory + fake of the backend, never the network. +- **Contract tests**, each against the fake: + - repeat `Create` returns the same ID and creates nothing; + - `Get` after `Delete` returns `ErrNotFound`; + - `Delete` twice succeeds; + - malformed ID is `ErrPermanent` from `Get` and `Delete`; + - an unknown backend state maps to `StateStopped`; + - `StateRunning` requires an IP, and a running instance without one is + `StateProvisioning`; + - `UID` and `CreatedAt` are read back from the fake, not remembered; + - `ListByTag` excludes unmarked instances and instances with an empty UID + are still listed with `UID == ""`; + - `Get` during an in-progress delete still returns the instance; + - `Get` during an asynchronous operation or lock returns the last state or + Transient (`async-ops`); + - a created but not yet started instance is never `StateStopped` + (`provisioning-state`); + - a cancelled `ctx` makes every method return promptly. +- **Classify**: one table test covering every class, every backend code the + provider matches, and "not a backend error at all, so Transient". Use + captured real responses for message matching. +- **Checker**: a table test for the reasons, a test that `Check` returns + promptly with an expired `ctx`, and a test that it returns one entry per + target in config order. +- **Sentinel tests**: a `CloudInit` containing a sentinel string and a + credential containing another never appear in any log line, span attribute + or error string. +- **Tracing**: with an in-memory span recorder + (`sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(tracetest.NewSpanRecorder()))`, + as in `internal/provider/tracing_test.go`), the child spans are children + of the operation span. +- **Real-backend test** per `backend-test` (§4.19). It runs the whole + lifecycle: create, create again, poll until Running, list, delete, delete + again, confirm it is gone and that no secondary resource remains. Keep it + compiling with `go vet -tags=integration ./...`. + +## 12. Checklist for adding a provider + +1. **Spec first.** Copy `docs/provider-spec-template.md` to + `docs/-provider-spec.md` and answer every entry. Unanswered entries + block implementation. +2. **Package** `internal/provider//` with the provider type, a + compile-time assertion for `provider.Provider` and one for + `provider.Checker`, the seam interface, and a logger built from + `logf.FromContext(ctx).WithName("").WithValues("provider", name)`. +3. **Config** per §10, including `config_test.go` rows. +4. **Constructor** `New(ctx, provider.ProviderConfig)` registered in + `cmd/main.go`; no dialing. +5. **Contract** per §2, tested per §11. +6. **Errors** per §5, `classify` plus its table test, `ProviderConfig.Name` + in every `Wrap`. +7. **Checker** per §8, with the `status.type` comment updated and + `make manifests` run. +8. **Tracing and logging** per §9 and §4.17, §4.18. +9. **Real-backend test** per §4.19, with a `make test-integration-` + target and a section in `docs/testing.md`. +10. **RBAC**: if the provider talks to the cluster's own API, add + `+kubebuilder:rbac` markers in their own comment block, never inside a + doc comment, then `make manifests` and check + `git diff config/rbac/role.yaml`. +11. **Deployment**: an `optional: true` credentials mount in + `config/manager/manager.yaml`; a commented entry in + `config/manager/providers_config.yaml` and + `config/samples/providers-config.yaml`; a `config/samples/proxy_.yaml` + whose cloud-init installs a proxy on the sample image; a Makefile helper + for the Secret if there is one; `hack/providers-dev.yaml` only if the + type needs no credentials. +12. **Docs**: `docs/-provider.md` (prerequisites, config reference, + placement mapping, what lands on the backend, the health check, logging, + known gaps); a row and a section in `providers-existing.md`; a row in the + `architecture.md` components table and a block in its §5 and §11 + (§10 too if the provider adds spans); a row in the README "Implemented + providers" table; a `CHANGELOG.md` entry once the user has confirmed it + works. +13. **Optional:** a demo script under `docs/demo/`. + +## 13. Known limits of the contract + +These are facts a new provider has to live with today. None is designed or +scheduled. + +- **Adoption is not verified.** `Create` adopts any existing instance with + the right name without checking its `LabelUID`. +- **`StateStopped` and `StateTerminated` are indistinguishable** to the + reconciler; both mean delete and recreate. +- **Config is read once per process.** Adding a provider or a location, or + rotating credentials a provider caches, needs a manager restart. +- **GC has no installation identity.** Two installs sharing one backend + delete each other's instances, because GC only knows its own live UIDs. +- **An unconfigured provider latches the Proxy.** A Proxy naming a provider + missing from the config is latched `PermanentError`; adding the provider + and restarting does not change `metadata.generation`, and `spec.provider` + is immutable, so the Proxy stays latched until it is recreated. +- **Provider names are not validated** as DNS-1123 at config load. +- **The type list exists twice**: `Config.validate` and the constructor map + in `cmd/main.go`. +- **`Delete` is resubmitted on every deletion poll** until `Get` says + NotFound. +- **Callers set no deadline** (§1.5); each provider bounds itself. +- **There is no conformance suite.** The §11 contract tests are written per + provider; `internal/provider/providertest` is a proposal in the appendix. +- **Stale comments** in `internal/provider/provider.go`, + `internal/provider/registry/registry.go` and `api/v1alpha1/proxy_types.go` + still describe a `mock` provider that no longer exists. + +## 14. Related documents + +- [providers.md](providers.md): the index. +- [provider-spec-template.md](provider-spec-template.md): the questionnaire. +- [providers-existing.md](providers-existing.md): how the existing providers + meet this contract, and their gaps. +- [architecture.md §5](architecture.md#5-what-provider-calls-do-in-the-outside-world), + [§10](architecture.md#10-tracing-internaltracing) and + [§11](architecture.md#11-provider-health-checks-internalproviderhealth). +- [testing.md](testing.md). From d61868fe2df100979e4c5e037e2e696cbcd799f9 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:28:52 +0000 Subject: [PATCH 07/11] Add the provider spec template Twenty-four fill-in entries, one per decision the contract leaves to the provider, numbered to match the contract's profile table. Co-Authored-By: Claude Fable 5.1 --- .../2026-09-16-1108-provider-docs-split.md | 26 +- docs/provider-spec-template.md | 455 ++++++++++++++++++ 2 files changed, 480 insertions(+), 1 deletion(-) create mode 100644 docs/provider-spec-template.md diff --git a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md index b1a6fc9..20413dd 100644 --- a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md +++ b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md @@ -4,7 +4,7 @@ Plan: [docs/plans/2026-09-16-1108-provider-docs-split.md](../plans/2026-09-16-11 - [x] Step 0 — Save and commit the plan - [x] Step 1 — Write `docs/provider-contract.md` -- [ ] Step 2 — Write `docs/provider-spec-template.md` +- [x] Step 2 — Write `docs/provider-spec-template.md` - [ ] Step 3 — Write `docs/providers-existing.md` - [ ] Step 4 — Rewrite `docs/providers.md` as the index; link edits - [ ] Step 5 — Verification sweep @@ -63,3 +63,27 @@ own API" rather than naming the platform, so that the abstractness grep stays meaningful. The `hack/providers-dev.yaml` rule is phrased as "only if the type needs no credentials" because that file exists for `make run-dev` without any Secret. + +## Step 2 — Write `docs/provider-spec-template.md` + +2026-09-16 11:28. Twenty-four entries in a fixed shape (Question, Allowed +answers, Obligations, Answer, Evidence). Thirteen of them carry an axis code +and their numbers are the ones the contract's §4.20 table points at; the +other eleven (identity, placement, image, cloud-init, machine type, config +block, Checker, concurrency, deployment, docs, accepted limits) are decisions +the contract leaves open without being a profile axis. Entries 9, 13 and 15 +carry an empty table to fill in (state mapping, error classification, config +fields); entry 21 carries the contract §11 test list as checkboxes so a +filled spec doubles as the test plan. + +Check that every axis code appears in the template: + +```sh +for c in $(grep -oE '^### 4\.[0-9]+ `[a-z-]+`' docs/provider-contract.md | grep -oE '`[a-z-]+`' | tr -d '`'); do + grep -q "\`$c\`" docs/provider-spec-template.md || echo "template missing $c" +done +``` + +Clean. Worth noting: the preamble allows "verify against the target version" +only on an Evidence line, never as an Answer, because the whole point of the +file is that the implementer does not guess. diff --git a/docs/provider-spec-template.md b/docs/provider-spec-template.md new file mode 100644 index 0000000..a0f3f8d --- /dev/null +++ b/docs/provider-spec-template.md @@ -0,0 +1,455 @@ +# Provider spec template + +Copy this file to `docs/-provider-spec.md`, fill in every **Answer** +line, and hand the result to whoever implements the provider together with +[provider-contract.md](provider-contract.md) and +[providers-existing.md](providers-existing.md). An unanswered entry blocks +implementation: it is a decision the contract leaves to the provider, and the +implementer must not guess it. + +Where an entry names an axis in backticks, the allowed answers are the +profiles of that axis in contract §4, and the entry number matches the last +column of the contract's §4.20 table. **Evidence** is where the answer was +confirmed: a link into the backend's API documentation, or the experiment +that was run. "Verify against the target version" is a valid interim +answer only for the Evidence line, never for the Answer. + +## Header + +- **Type** (the `type:` value in the providers config, lowercase): +- **Example configured name**: +- **Backend and API version this spec was written against**: +- **Client** (`vendor-sdk` with the module path, or `thin-client`): +- **Author and date**: + +## 1. Identity and providerID + +**Question.** What is the providerID's shape, which location parts does it +embed, and what validates the config values pasted into it? + +**Allowed answers.** `/`, `/`, or a +location-free `` (contract §6; required when instances can +migrate, see entry 2). + +**Obligations.** Strict parsing; `ErrPermanent` for a malformed ID from `Get` +and `Delete`; a regexp on every config value that becomes an ID segment, in +`config_.go`. + +**Answer:** + +**Evidence:** + +## 2. Primary key (`primary-key`) + +**Question.** Is `req.Name` the backend's key, or does the backend allocate an +identifier? + +**Allowed answers.** `name-is-key`; `allocated-id`. + +**Obligations.** `allocated-id`: `Create` finds an existing instance by name +before allocating; allocation is safe against a concurrent `Create` for +another Proxy (deterministic from `req.UID`, or allocate-then-verify); `Get` +and `Delete` use the allocated key; state how a collision with an instance +the operator does not own is detected and reported. + +**Answer:** + +**Evidence:** + +## 3. Placement mapping + +**Question.** What do `Placement.Region`, `Zone`, `MachineType` and `Image` +mean for this backend, which are required, which are ignored, and what +happens when a value is unknown? + +**Allowed answers.** Per field: a config catalogue key, a backend identifier +passed through, or ignored (contract §2.1, MAY ignore with documentation). + +**Obligations.** Unknown values are rejected with `ErrPermanent` naming the +field and value before any backend call; ignored fields are listed in the +provider's guide because they are still in the spec hash; an empty value with +a single-choice catalogue may default, and the guide says so. + +**Answer:** + +**Evidence:** + +## 4. Image source + +**Question.** Where does the boot image come from, and what does a catalogue +edit do to running instances? + +**Allowed answers.** A catalogue in the config block (key to URL or backend +image ID); a backend template or image referenced directly by +`Placement.Image`; a URL fetched by the provider into the backend on first +use. + +**Obligations.** If the provider fetches or converts images: streaming, no +buffering (contract §1.4), a completeness marker so a cut-off fetch is redone, +concurrency control so two Creates fetch once, and a written warning that +replacing an image in use may corrupt instances that depend on it. If the +backend owns the image: the prerequisite is documented in the guide. + +**Answer:** + +**Evidence:** + +## 5. Cloud-init delivery + +**Question.** How does `CreateRequest.CloudInit` reach the guest, byte for +byte? + +**Allowed answers.** User-data metadata on the instance; a NoCloud seed +built in-process and attached as media; a file placed on the backend and +referenced from the instance config; not delivered (documented, with how the +proxy gets configured instead). + +**Obligations.** Size limits of the channel stated; whether vendor-data or a +second document can ride along; the sentinel test (contract §11); the channel +must be fully reachable through the API the provider uses (no step that needs +shell access on the backend unless the spec says the provider has it). + +**Answer:** + +**Evidence:** + +## 6. Machine type mapping + +**Question.** How does `Placement.MachineType` become CPU, memory and disk? + +**Allowed answers.** A catalogue of named profiles in the config block; a +backend size name passed through. + +**Obligations.** A catalogue needs validation of each profile; disk size +default and its config field named. + +**Answer:** + +**Evidence:** + +## 7. Network and IP discovery (`readiness`, `ip-source`) + +**Question.** Which network does the instance attach to, where does its +address come from, how long until it is usable, and what makes it reachable +from the operator pod and from clients on `spec.port`? + +**Allowed answers.** `readiness`: `immediate`; `provisioning-window`. +`ip-source`: `assigned-at-create`; `discovered` (name the mechanism: guest +agent, DHCP lease, neighbour table, and the order if several). + +**Obligations.** `discovered`: what the guest must run, how it gets +installed, what happens without it; discovery failure is Provisioning, not an +error; only routable IPv4 is reported (state the filter). Reachability: +firewall or routing steps the provider performs, or the prerequisite the +guide states. Whether `Port` is used by the provider at all. + +**Answer:** + +**Evidence:** + +## 8. GC marks and CreatedAt (`gc-mark`, `created-at`) + +**Question.** Where do `LabelManaged` and `LabelUID` live, where does +`CreatedAt` come from, and which secondary resources are marked? + +**Allowed answers.** `gc-mark`: `native-labels`; `written-metadata`; +`mark-window`. `created-at`: `backend-timestamp`; `written-at-create`. + +**Obligations.** `mark-window`: the window's length in seconds, why it is +far below `MinAge`, and how the same Proxy's next reconcile cleans an +unmarked instance. Any value-format constraint of the backend's labels +(allowed characters, length) and how a Proxy UID fits it. + +**Answer:** + +**Evidence:** + +## 9. State mapping (`provisioning-state`) + +**Question.** How does every backend state map to `StateProvisioning`, +`StateRunning`, `StateStopped`, `StateTerminated`, and how does `Get` tell +"defined but never started" from "stopped"? + +**Allowed answers.** A table of backend state (plus lock or task flags) to +provider state, with the unknown-state row mapping to `StateStopped`. +`provisioning-state`: `native`; `create-blocks`; `marker`. + +**Obligations.** Running requires an IP; `marker` names what is recorded at +creation and how `Get` reads it; `create-blocks` names the cap on the wait. + +**Answer:** + +| Backend state | Lock or task | Provider state | +|---|---|---| +| | | | + +**Evidence:** + +## 10. Create: blocking and task tracking (`create-return`, `async-ops`) + +**Question.** What does `Create` wait for before returning, and how are +asynchronous backend tasks tracked? + +**Allowed answers.** `create-return`: `submit`; `blocking`. `async-ops`: +`none`; `task-handle`; `backend-lock`. + +**Obligations.** `task-handle`: where the handle is kept and how a failed +task surfaces (in `Get`, as which class, with which message). `backend-lock`: +what `Get` and `Delete` return while locked. Every step of `Create` is +idempotent so a cut-off attempt resumes; list the steps and what each finds +on a retry. + +**Answer:** + +**Evidence:** + +## 11. Delete and secondary resources (`delete-return`, `secondary-resources`) + +**Question.** In what order does `Delete` tear things down, what does it do +when called again mid-delete, and what does `Get` return meanwhile? + +**Allowed answers.** `delete-return`: `returns-early`; `waits`. +`secondary-resources`: `none`; `cascade` (name the flags set at create); +`explicit` (name each resource and how it is found by name). + +**Obligations.** Idempotent at every step; "already deleting" is success and +cheap; a location no longer in the config is a logged no-op success; any +resource that can outlive the instance without being listable is a documented +GC blind spot. + +**Answer:** + +**Evidence:** + +## 12. ListByTag scope and partial failure (`list-partial`) + +**Question.** What is the full scope `ListByTag` enumerates, and what happens +when part of it is unreachable? + +**Allowed answers.** `fail-whole`; `omit-unreachable`. + +**Obligations.** The scope covers everything the provider can create in; +server-side filtering on the mark where the backend allows it; a single +unreadable instance may be skipped with a log line; the cost of one list at +fleet size. + +**Answer:** + +**Evidence:** + +## 13. Error classification (`not-found`, `quota`) + +**Question.** Which backend responses map to which of the four classes, and +which signal means "does not exist"? + +**Allowed answers.** A table of backend status or code (with message pattern +where needed) to class. `not-found`: `status-404`; `error-code`; +`list-and-miss`. `quota`: `detected`; `absent`. + +**Obligations.** One `classify`, a table test with captured real responses +for every matched message; the permission-denied-versus-quota case decided; +authentication failures, host-key or certificate failures, and cancellation +each assigned a class with a reason. + +**Answer:** + +| Backend response | Class | Why | +|---|---|---| +| | | | + +**Evidence:** + +## 14. Credentials and TLS (`credentials`) + +**Question.** What credential does the operator use, where does it come from, +when is it read, and how is the backend's TLS certificate trusted? + +**Allowed answers.** `mounted-files` (Secret name, mount path, file names, +`optional: true`); `environment`; `workload-identity`. TLS: `system-roots`; +`ca-file`; `insecure` (the flag's exact name). + +**Obligations.** The minimum privilege set the credential needs, listed; +rotation behaviour (free, or restart needed); the sentinel test for the +credential (contract §7); the constructor never dials. + +**Answer:** + +**Evidence:** + +## 15. Config block + +**Question.** What are the fields of `Config`, their defaults, and their +validation? + +**Allowed answers.** A table: field, required, default, validation, and which +identifier or path the value ends up in. + +**Obligations.** Contract §10: `setBlocks`, the type allow-list in +`Config.validate` and the constructor map in `cmd/main.go`, `config_.go` +with its tests, regexps for values pasted into IDs or file names. + +**Answer:** + +| Field | Required | Default | Validation | Ends up in | +|---|---|---|---|---| +| | | | | | + +**Evidence:** + +## 16. Checker + +**Question.** What are the check targets, what does one check do, which +reasons can it report, and what goes into `Details`? + +**Allowed answers.** One target per thing that fails independently; the +shared reasons from `internal/provider/check.go` first, provider-specific +reasons only with a stated "what the admin does differently"; `Details` keys +listed. + +**Obligations.** Same credential, verb and scope as `ListByTag` and `Create`; +cheap (one page, one object); never disrupts a reconcile that shares a +connection; returns promptly on `ctx`; the `status.type` comment in +`api/v1alpha1/providerhealth_types.go` updated. + +**Answer:** + +**Evidence:** + +## 17. Client and child spans (`client-kind`, `child-spans`) + +**Question.** What talks to the backend, and how do its calls appear under the +operation span? + +**Allowed answers.** `client-kind`: `vendor-sdk` (module path, and whether it +is maintained); `thin-client`; `rpc-over-ssh`. `child-spans`: +`sdk-instrumentation`; `transport-wrapper`; `span-decorator`; `none`. + +**Obligations.** The seam interface's name and the fake that implements it; +`thin-client`: one error type carrying status and body, transport timeouts, +parent-gated `otelhttp`; `none` is a documented gap; the span recorder test +(contract §11). + +**Answer:** + +**Evidence:** + +## 18. Timeouts (`timeouts`) + +**Question.** What bounds each backend call, the dial, and any long step such +as a download or a wait loop, and what does a cancelled `ctx` do to a shared +connection? + +**Allowed answers.** `per-call` (the budgets, per operation); `ctx-only` +(cite the client's own timeout). + +**Obligations.** Contract §1.5: no caller sets a deadline; a hung call pins a +worker or stalls GC. Cancellation behaviour on a shared connection stated +(abandon the call, or close the connection). + +**Answer:** + +**Evidence:** + +## 19. Concurrency + +**Question.** What is shared between concurrent calls (connections, caches, +locks), and what happens when two calls for the same instance name overlap? + +**Allowed answers.** A description of the shared state and its locking. + +**Obligations.** Safe for 3 reconciles, a GC sweep and a check round at once; +two `Delete` calls for one instance overlap safely; one fetch per image per +location when several Creates race. + +**Answer:** + +**Evidence:** + +## 20. Logging (`logging`) + +**Question.** Which tiers does the provider log, with which keys on every +line? + +**Allowed answers.** `tiered`; `outcome-only`; `none` (cite the client's own +logging). + +**Obligations.** Logger from `logf.FromContext(ctx)` on every call; never +`CloudInit` or credentials; `provider` and the instance name on every line +that concerns an instance. + +**Answer:** + +**Evidence:** + +## 21. Tests (`backend-test`) + +**Question.** What is the fake, which contract tests exist, and how is the +provider proven against the real backend? + +**Allowed answers.** `backend-test`: `cluster-e2e`; `integration-tag` (the +`_TEST_*` variables, the `make test-integration-` target); `manual-runbook`. + +**Obligations.** Every item of the contract §11 list, ticked here: + +- [ ] repeat `Create` +- [ ] `Get` after `Delete` +- [ ] `Delete` twice +- [ ] malformed ID +- [ ] unknown state maps to `StateStopped` +- [ ] `StateRunning` requires an IP +- [ ] `UID` and `CreatedAt` read back +- [ ] `ListByTag` excludes unmarked instances +- [ ] `Get` during an in-progress delete +- [ ] `Get` during an asynchronous operation or lock +- [ ] never started is never `StateStopped` +- [ ] cancelled `ctx` returns promptly +- [ ] `classify` table with captured responses +- [ ] Checker reasons, expired `ctx`, stable order +- [ ] sentinel tests for `CloudInit` and the credential +- [ ] child spans under the operation span +- [ ] real-backend lifecycle test + +**Answer:** + +**Evidence:** + +## 22. Deployment + +**Question.** What changes in `config/` and the Makefile? + +**Allowed answers.** The list: the `optional: true` mount in +`config/manager/manager.yaml`; the commented entries in +`config/manager/providers_config.yaml` and +`config/samples/providers-config.yaml`; `config/samples/proxy_.yaml` +with a cloud-init that installs a proxy on the sample image; a Makefile +helper for the Secret; RBAC markers if the provider talks to the cluster's +own API; whether `hack/providers-dev.yaml` gets an entry. + +**Answer:** + +**Evidence:** + +## 23. Docs + +**Question.** Which documents are written or updated? + +**Allowed answers.** `docs/-provider.md` with its section list; the row +and section in `providers-existing.md`; the `architecture.md` rows (components +table, §5, §11, and §10 if spans are added); the README row; the +`docs/testing.md` section; the `CHANGELOG.md` entry after confirmation. + +**Answer:** + +**Evidence:** + +## 24. Known limits accepted at design time + +**Question.** Which of the contract's §13 limits bite this provider in +practice, and which new gaps does this design accept? + +**Allowed answers.** A list, each with the consequence for an operator and +the workaround. + +**Answer:** + +**Evidence:** From 2d959af313e11a9f6a6cccb61e001a479754f9ee Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:36:03 +0000 Subject: [PATCH 08/11] Add the appendix on the existing providers Profile table keyed on the contract's axis codes, per-provider reference sections for kubernetes, gcp and kvm, shared gaps and proposed features. Co-Authored-By: Claude Fable 5.1 --- .../2026-09-16-1108-provider-docs-split.md | 36 +- docs/providers-existing.md | 392 ++++++++++++++++++ 2 files changed, 427 insertions(+), 1 deletion(-) create mode 100644 docs/providers-existing.md diff --git a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md index 20413dd..3966a62 100644 --- a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md +++ b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md @@ -5,7 +5,7 @@ Plan: [docs/plans/2026-09-16-1108-provider-docs-split.md](../plans/2026-09-16-11 - [x] Step 0 — Save and commit the plan - [x] Step 1 — Write `docs/provider-contract.md` - [x] Step 2 — Write `docs/provider-spec-template.md` -- [ ] Step 3 — Write `docs/providers-existing.md` +- [x] Step 3 — Write `docs/providers-existing.md` - [ ] Step 4 — Rewrite `docs/providers.md` as the index; link edits - [ ] Step 5 — Verification sweep - [ ] Step 6 — Final summary, CHANGELOG after confirmation @@ -87,3 +87,37 @@ done Clean. Worth noting: the preamble allows "verify against the target version" only on an Evidence line, never as an Answer, because the whole point of the file is that the implementer does not guess. + +## Step 3 — Write `docs/providers-existing.md` + +2026-09-16 11:35. The profile table has exactly one row per contract axis +code, checked by the same loop as Step 2 with a row count. Everything else +comes from the old blueprint's §6, §7 and §9 plus a fresh read of the three +packages, with every `file:line` replaced by a symbol name. + +Corrections to earlier notes applied while writing: kvm's Checker `Details` +also carry `user` and `pool`; the kubernetes provider has no seam interface +(`newWithClient` injects a `client.Client`, the fake client stands in); kvm's +`classify` has an `isClassified` pass-through. Timeouts were checked per +provider for the `timeouts` row: kubernetes sets no `rest.Config.Timeout`, +gcp relies on SDK defaults, kvm bounds only the SSH handshake +(`defaultHandshakeTimeout` in `dialSSH`), so all three are weaker than the +contract's SHOULD and the table says so. + +Symbol check on the appendix (every backticked identifier grepped for in +Go, Makefile and YAML sources): + +```sh +grep -ohE '`[A-Za-z_][A-Za-z0-9_.]*`' docs/providers-existing.md | tr -d '`' | sort -u | while read s; do + grep -rqwF --include='*.go' --include='Makefile' --include='*.yaml' -- "$s" . || echo "$s" +done +``` + +The residue is file names, dotted `Type.method` forms, the `timeouts` axis +code, libvirt's `nostate`, and the API names `OwnerReference` and +`rest.Config.Timeout`; nothing unexpected. + +Worth noting: matrix rows that the profile table now carries (Create +non-blocking, GC marks, IP source, quota detection, Checker presence, +credentials, logging, tracing, tests) were dropped from "Other differences" +rather than kept twice. diff --git a/docs/providers-existing.md b/docs/providers-existing.md new file mode 100644 index 0000000..9019293 --- /dev/null +++ b/docs/providers-existing.md @@ -0,0 +1,392 @@ +# The existing providers + +This is the appendix to [provider-contract.md](provider-contract.md): how the +three providers that exist today (kubernetes, gcp, kvm) meet the contract, +which profile each one chose on every axis, and what is still missing. It is +reference material for whoever writes the next provider, not a user guide; +the user guides are [kvm-provider.md](kvm-provider.md), +[gcp-vm-validation.md](gcp-vm-validation.md) and +[gcp-in-specific-project.md](gcp-in-specific-project.md). The README's +"Implemented providers" table stays the short public summary. + +The per-provider call flows are drawn in +[architecture.md §5](architecture.md#5-what-provider-calls-do-in-the-outside-world), +the health checks are tabulated in +[§11](architecture.md#11-provider-health-checks-internalproviderhealth), and the +judgment calls behind several rows below are recorded under its "Decisions" +list. They are linked, not repeated. + +Code references are a file path plus a function or symbol name. + +## 1. Profile table + +One row per axis of contract §4, in the same order, with the profile name +and the symbol that shows it. + +| Axis | kubernetes | gcp | kvm | +|---|---|---|---| +| `readiness` | immediate: a Pod is scheduled within seconds (`stateFromPod`) | provisioning-window: minutes of boot (`mapState`) | provisioning-window: boot plus, on first use, an image download (`imageCache.ensure`) | +| `create-return` | submit: returns after `client.Create` | submit: `Insert` is never awaited, `Operation.Wait` is not called | **blocking**: waits through `DomainCreate` (`ensureRunning`), and the first image fetch runs inside (`imageCache.fetch`) | +| `delete-return` | returns-early: `client.Delete`, Pod terminates on its own | returns-early: `Delete` submits an operation each time | waits: destroy, undefine and volume removal complete before return (`removeDomain`, `removeVolumes`) | +| `ip-source` | assigned-at-create: `PodIP` (`instanceFromPod`) | assigned-at-create: the external NAT IP of the first NIC (`toInstance`) | discovered: ARP table first, then the guest agent, first routable IPv4 (`address`, `firstIPv4`) | +| `gc-mark` | native-labels: Pod labels (`buildPod`) | native-labels: instance labels (`buildInsertRequest`) | written-metadata: a namespaced `` element written in `DomainDefineXML` (`metadata.go`) | +| `created-at` | backend-timestamp: `CreationTimestamp` | backend-timestamp: `CreationTimestamp`, parse error gives zero | written-at-create: `createdAt` in the metadata element (`newInstanceMetadata`) | +| `list-partial` | n/a: one call, all namespaces | omit-unreachable: `ReturnPartialSuccess` on `AggregatedList` | fail-whole per host, one unreadable domain skipped (`ListByTag`, `managedInstance`) | +| `quota` | absent: a quota 403 is Permanent (`classify`) | detected: 429, and 403 with reason `quotaExceeded` or `rateLimitExceeded` (`classify`, `hasReason`) | absent (`classify`) | +| `not-found` | status-404: `apierrors.IsNotFound` | status-404: `isNotFound` | error-code: `ErrNoDomain`, `ErrNoStorageVol` (`hasCode`) | +| `primary-key` | name-is-key: `namespace/name` | name-is-key: `zones//instances/` | name-is-key: `hosts//domains/` | +| `provisioning-state` | native: `PodPending` | native: `PROVISIONING`, `STAGING`, `REPAIRING` | create-blocks: a domain is defined and started in one `Create`; `nostate` maps to Provisioning | +| `async-ops` | none | task-handle, but the handle is discarded: an insert that fails after acceptance is never read | none | +| `secondary-resources` | none | cascade: `AutoDelete` on the boot disk (`buildInsertRequest`) | explicit: overlay and seed removed by name (`removeVolumes`); base image kept | +| `credentials` | workload-identity: in-cluster config or kubeconfig (`NewWithTransportWrapper`) | environment: Application Default Credentials | mounted-files: SSH key and known_hosts from the optional `kvm-ssh` Secret, read lazily (`newLazySSH`) | +| `client-kind` | vendor-sdk: controller-runtime `client.Client`, no seam interface | vendor-sdk: `cloud.google.com/go/compute` behind `instancesAPI` | rpc-over-ssh: `go-libvirt` over an SSH-forwarded unix socket, behind `libvirtAPI` (`conn.go`) | +| `timeouts` | ctx-only: no `rest.Config.Timeout` is set | ctx-only: SDK defaults, with SDK retries on Get and List | per-call for the dial only (`defaultHandshakeTimeout` in `dialSSH`); RPCs are ctx-only and a cancelled `call` closes the connection | +| `logging` | none | tiered: V(1) outcomes and `logAPIError`, V(2) detail, V(5) wire log (`wirelog.go`) | tiered: V(1) outcomes and one `libvirt call` line per RPC (`trace.go`), V(2) XML and addresses; no wire tier | +| `child-spans` | transport-wrapper: `tracing.RestConfigWrapper()` on its own `rest.Config`, parent-gated | sdk-instrumentation: the SDK adds `otelhttp` itself; not parent-gated, but every call site has a parent | **none**: `trace.go` is a log decorator, not spans | +| `backend-test` | cluster-e2e: kind (`test/e2e`) | manual-runbook: [gcp-vm-validation.md](gcp-vm-validation.md) | integration-tag: `kvm_integration_test.go`, `make test-integration-kvm` | + +## 2. Other differences + +Rows that are not profile axes but that a new provider author compares +against. + +| Feature | kubernetes | gcp | kvm | +|---|---|---|---| +| Create verifies the adopted instance's UID | no | no | no | +| Placement used | none | Zone, MachineType, Image | Zone (host), MachineType, Image | +| CloudInit delivered | **no**: ignored | yes: `user-data` metadata | yes: NoCloud seed ISO built in-process (`buildSeedISO`) | +| Port used | yes: Squid config and containerPort (`squidConf`) | no: firewall is manual | no | +| States produced | Provisioning, Running, Terminated | all four | Provisioning, Running, Stopped | +| Deleting instance still reports its state | yes: a Pod being deleted reports its phase | n/a | n/a | +| Error.Provider is the configured name | **no**: hardcoded `"kubernetes"` | yes | yes | +| Checker target | `apiserver` | the project | each host, concurrently | +| Checker Details | none | none | `user`, `pool`, `hostname`, `libvirtVersion`, `poolState`, `poolCapacity`, `poolAvailable` | +| Credential rotation without restart | n/a | via ADC | no: cached after first successful load | +| Connection handling | client-go | SDK client, never closed | one pooled SSH plus libvirt connection per host, 15s keepalive (`hostPool`) | +| Unit-test double | controller-runtime fake client | `fakeAPI` behind `instancesAPI`, plus `httptest` for the real SDK client in `check_test.go` | `fakeAPI` behind `libvirtAPI`, plus an in-process `x/crypto/ssh` server in `conn_test.go` | +| Proxy software | generated Squid config (`pod.go`) | from `cloudInit`; the image must run cloud-init | from `cloudInit`, plus a vendor-data script that installs the guest agent (`seed.go`) | +| 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 | + +## 3. Observability status + +| Layer | kubernetes | gcp | kvm | +|---|---|---|---| +| 1. Operation spans and metrics | yes: decorators | yes: decorators | yes: decorators | +| 2. Child spans per backend call | yes: `tracing.RestConfigWrapper()`, parent-gated (`cmd/main.go`, `NewWithTransportWrapper`) | yes: the SDK's own `otelhttp`; not parent-gated | **no** | +| 3. Logs carry `traceID` and `spanID` | n/a: the package does not log | yes, including the V(5) wire log | yes: every `libvirt call` line at V(1) | +| Tested against a real collector | yes: the kind e2e checks `provider.create` and `provider.delete` in Tempo (`test/e2e/tracing_test.go`), operation spans only | no | no | + +## 4. kubernetes (`internal/provider/kubernetes`) + +- **Package and seam.** `kubernetes.go`, `pod.go`, `check.go`. There is no + seam interface: `newWithClient` injects a controller-runtime + `client.Client`, and tests use the fake client. `New` calls + `NewWithTransportWrapper`, which builds its own uncached client from + `ctrl.GetConfig()` with an optional `transport.WrapperFunc` for tracing. +- **Config.** `KubernetesConfig.Image` only, defaulting to the Squid image + constant in `kubernetes.go`. +- **ID.** `providerID(namespace, name)` gives `namespace/name`; + `parseProviderID` uses `cache.SplitMetaNamespaceKey` and rejects an empty + namespace with `ErrPermanent`. An empty name part is not rejected. +- **Create.** `buildPod` (pure) then `client.Create`; `IsAlreadyExists` is + success. Only `Name`, `Namespace`, `UID` and `Port` are used. The Squid + config is generated by `squidConf` from the port and passed through the + `SQUID_CONF` environment variable, so there is exactly one object per + proxy. +- **Get and state.** `stateFromPod`: + + | Pod phase | PodIP | Provider state | + |---|---|---| + | Pending | any | Provisioning | + | Running | empty | Provisioning | + | Running | set | Running | + | Succeeded, Failed, Unknown | any | Terminated | + + `instanceFromPod` sets `IP` only when the state is Running. A Pod with a + deletion timestamp still reports its phase. `UID` from the `LabelUID` + label, `CreatedAt` from `CreationTimestamp`. +- **Delete.** `client.Delete`, `IsNotFound` ignored. +- **ListByTag.** One `client.List` across all namespaces with + `MatchingLabels{LabelManaged: LabelManagedYes}`; this is what the + cluster-wide Pod RBAC is for. +- **Errors.** `classify`: `IsNotFound` NotFound; `IsTooManyRequests`, + `IsServerTimeout`, `IsTimeout` Transient; `IsForbidden`, `IsInvalid`, + `IsBadRequest`, `IsUnauthorized` Permanent; default Transient. Nothing is + Quota, because a quota 403 and an RBAC 403 cannot be told apart. +- **Check.** Lists at most one managed Pod cluster-wide (`client.Limit(1)`), + proving read and list access, not create. Target `apiserver`, address the + rest config host. `checkReason`: context errors and timeouts + `ReasonTimeout`; `IsUnauthorized`; `IsForbidden`; any `APIStatus` + `ReasonAPIError`; else `ReasonUnreachable`. +- **Logging and tracing.** No log lines in the package. Child spans come + from the parent-gated transport wrapper. +- **Tests.** `kubernetes_test.go`, `pod_test.go`, `check_test.go` over the + fake client; the kind e2e (`test/e2e`) is the real-backend run. +- **Deployment.** RBAC markers for Pods live next to the reconciler's in + `internal/controller/proxy_controller.go`. `hack/providers-dev.yaml` holds + only this provider, for `make run-dev`. +- **Known gaps.** + - `Error.Provider` is hardcoded to `"kubernetes"`, so errors and logs show + the wrong name when the configured name differs. + - `CloudInit` and all of `Placement` are ignored. + - No quota class: a quota 403 is Permanent. + - A Pod being deleted still reports its phase, so it can read as Running. + - No securityContext, no resources, and `http_access allow all` in + `pod.go`. + - No logging in the package, and no Checker `Details`. + - No `OwnerReference` on Pods: a bypassed finalizer leaves the Pod behind. + - The Pod RBAC is always installed, even when no kubernetes provider is + configured. + +## 5. gcp (`internal/provider/gcp`) + +- **Package and seam.** `gcp.go`, `insert.go`, `errors.go`, `check.go`, + `wirelog.go`. The seam is `instancesAPI` (`Insert`, `Get`, `Delete`, + `AggregatedList`, `Probe`); `realInstances` adapts the SDK client and + `newWithAPI` is the test constructor. `New` calls `NewWithWireOptions`, + which takes the `--gcp-wire-log-full-payloads` flag from `cmd/main.go`. +- **Config.** `GCPConfig`: `project` required; `network`, `networkTag`, + `diskSizeGb` defaulted in `withDefaults`. +- **ID.** `formatProviderID(zone, name)` gives + `zones//instances/`; `parseProviderID` rejects anything else + with `ErrPermanent`. The project comes from the config. +- **Create.** Validates `Zone`, `MachineType` and `Image` up front and + returns `ErrPermanent` naming the missing field. `buildInsertRequest` + (pure) builds one NIC with an ephemeral external IP (`ONE_TO_ONE_NAT`) and + a boot disk with `AutoDelete`, labelled with both marks. `Insert` is + submitted and never awaited; 409 (`isAlreadyExists`) is success. +- **Get and state.** `mapState(status, ip)`: + + | Instance status | NAT IP | Provider state | + |---|---|---| + | PROVISIONING, STAGING, REPAIRING | any | Provisioning | + | RUNNING | empty | Provisioning | + | RUNNING | set | Running | + | STOPPING, STOPPED, SUSPENDING, SUSPENDED | any | Stopped | + | TERMINATED | any | Terminated | + | anything else | any | Stopped | + + `toInstance` reads the NAT IP of the first access config of the first + NIC, `UID` from the label, and `CreatedAt` from `CreationTimestamp` with + the parse error dropped (zero time on failure). +- **Delete.** Submits a delete operation and returns; 404 (`isNotFound`) is + success. Every deletion poll submits a new operation. +- **ListByTag.** `AggregatedList` with `managedFilter` on the managed label + only and `ReturnPartialSuccess`, so an unreachable zone is left out + silently; the zone is recovered from the URL by `lastPathSegment`. +- **Errors.** `classify`: non-`googleapi` errors Transient; 404 NotFound; + 429 Quota; 403 with reason `quotaExceeded` or `rateLimitExceeded` + (`hasReason`) Quota; 400, 401 and other 403s Permanent; other codes + Transient. `logAPIError` records the HTTP status and reasons at V(1) + because `classify` collapses them. +- **Check.** `Probe` makes one `AggregatedList` page with `maxResults=1` + and the managed filter: credentials, the Compute API and + `compute.instances.list` in one call. Target is the project, address + `compute.googleapis.com`. `checkReason`: context errors `ReasonTimeout`; + non-`googleapi` errors `ReasonUnreachable`; 401; 403; else + `ReasonAPIError`. +- **Logging.** `logger(ctx)` from the context, named `gcp` with + `provider`. V(2) for the curated insert request (never the user-data), + V(1) per operation outcome, V(2) per listed instance. The wire log in + `wirelog.go` runs at V(5) with payloads elided unless the flag is set. +- **Tracing.** The SDK adds `otelhttp` itself unless telemetry is disabled; + not parent-gated. +- **Tests.** `insert_test.go` (pure builder), `errors_test.go` (code + table), `gcp_test.go` (IDs, 409, state mapping, list filter and partial + success) over `fakeAPI`; `check_test.go` also drives the real SDK client + against `httptest` to prove exactly one request with `maxResults=1`. + Real backend: the manual runbook in [gcp-vm-validation.md](gcp-vm-validation.md). +- **Deployment.** Credentials by ADC; the runbook mounts a service-account + key and points `GOOGLE_APPLICATION_CREDENTIALS` at it. A commented entry + in both providers configs, `config/samples/proxy_gcp.yaml`, and demo + scripts under `docs/demo/`. +- **Known gaps.** + - Asynchronous insert failures are never read: an insert the API accepts + but that fails later (quota, bad image) is seen only as NotFound on the + next `Get`, and the reconciler creates again, so the reason never + reaches status. [gcp-vm-validation.md](gcp-vm-validation.md#gotchas) + still says such errors do reach status. + - External IP only: a VM without an external NAT IP stays Provisioning + forever. + - No firewall management: the rule for the proxy port is a manual + prerequisite, and `req.Port` is unused. + - 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 cannot 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). + +## 6. kvm (`internal/provider/kvm`) + +The full user guide is [kvm-provider.md](kvm-provider.md); this section is +the author's view. + +- **Package and seam.** `kvm.go` (the four methods), `conn.go` (SSH dial, + `hostPool`, `libvirtAPI`), `image.go` (`imageCache`), `seed.go` (NoCloud + ISO and vendor-data), `domainxml.go`, `volume.go`, `metadata.go`, `id.go`, + `errors.go`, `check.go`, `trace.go`. The seam is `libvirtAPI`, the twenty + go-libvirt calls the provider uses; `fake_test.go` implements it in memory + and `newWithDialer` injects the dialer and the HTTP client. +- **Config.** `KVMConfig` in `internal/provider/config_kvm.go`: + `sshKeyFile`, `knownHostsFile` or `insecureIgnoreHostKey`, `diskSizeGb`, + `vendorData`, `installGuestAgent`, `images` (`url`, `sha256`, `format`, + `firmware`), `machineTypes` (`vcpus`, `memoryMiB`), `hosts` (`name`, + `address`, `user`, `socket`, `pool`, `bridge`). `validateKVM` restricts + image keys and host names with `kvmIdentifier` because they become a volume + name and an ID segment. +- **ID.** `formatProviderID(host, name)` gives `hosts//domains/`. +- **Create.** `host` resolves `Placement.Zone` (optional with one host), + the machine type must be a catalogue key, `resolveImage` accepts a + catalogue key or an http(s) URL. Then, inside one `hostPool.call`: + `DomainLookupByName`, and if found `ensureRunning`; otherwise + `imageCache.ensure` (download on first use, streamed through the operator + into the pool with a sha256 marker volume), `ensureOverlay` (reused if it + exists), `writeSeed` (always rebuilt: a cut-off upload must not be + trusted), `defineDomain` with the metadata element, `ensureRunning`. + `Create` returns only when the domain is running. +- **Get and state.** `mapState(state, ip)`: + + | libvirt state | IP | Provider state | + |---|---|---| + | nostate | any | Provisioning | + | running, blocked | empty | Provisioning | + | running, blocked | set | Running | + | paused, shutdown, shutoff, crashed, pmsuspended | any | Stopped | + + `address` asks the ARP table first, then the guest agent, and + `firstIPv4` drops loopback, link-local and unspecified addresses. Metadata + is read back by `metadata` and parsed by `parseInstanceMetadata`; a domain + without the element reports an empty UID. +- **Delete.** `removeDomain` (destroy, tolerating not running or gone; then + `DomainUndefineFlags` with NVRAM, managed save and snapshot metadata) and + `removeVolumes` (overlay and seed by name, a missing pool is a no-op). A + host no longer in the config is a logged no-op success. +- **ListByTag.** `ConnectListAllDomains` on every host; `managedInstance` + keeps domains whose metadata says managed and skips a domain whose + metadata or state cannot be read. One unreachable host fails the whole + list. +- **Errors.** `classify`: `isClassified` errors keep their class; + `ErrNoDomain`, `ErrNoStorageVol` NotFound; XML, unsupported, + invalid-argument, auth and access errors, and a missing pool, Permanent; + everything else Transient, including SSH handshake and auth failures. + Nothing is Quota. `logAPIError` adds `libvirtErrorFields`. +- **Check.** `checkHost` per host concurrently through `hostPool.probe`, + not `call`, so a check that runs out of time abandons its RPCs instead of + closing the connection a reconcile may be using. Two cheap RPCs + (`ConnectGetLibVersion`, `ConnectGetHostname`) then the pool must exist + and be running. Reasons: the shared ones plus `SSHMaterialUnavailable`, + `SSHHandshakeFailed`, `LibvirtConnectFailed`, `LibvirtError`, + `PoolNotFound`, `PoolNotActive`, chosen by `checkReason` from the + `dialError` stage. Details: `user`, `pool`, `hostname`, `libvirtVersion`, + `poolState`, `poolCapacity`, `poolAvailable`. +- **Connections.** `hostPool` keeps one SSH plus libvirt connection per + host, dialled lazily (`lazySSH.dial`, `sshAuth.dial`, `dialSSH`, + `connectLibvirt`), with a 15s `keepalive`. `hostEntry.acquire` is a + one-slot semaphore so waiting can end with the caller's context. A + cancelled `call` closes the shared connection to unblock the RPC; + `probe` abandons it and leaves the connection open. The SSH material is + read once by `lazySSH.load` and cached. +- **Logging.** Logger from the context, named `kvm`, with `provider`, + `host` and `name`. V(1): one line per operation outcome and one + `libvirt call` line per RPC from the `tracingAPI` decorator in `trace.go` + (rpc name, identifiers, duration, error fields). V(2): domain and volume + XML, metadata, addresses. Upload and download bodies are never logged. +- **Tracing.** No child spans; `trace.go` is a log decorator. +- **Tests.** Pure builders (`domainxml_test.go`, `volume_test.go`, + `seed_test.go`, `id_test.go`, `metadata_test.go`); `errors_test.go`, + `conn_test.go` (pool behaviour and a real in-process SSH server); + `image_test.go`, `kvm_test.go` (Create resumes after every partial step, + every Delete already-gone case, ListByTag policies, logging tiers, a + cloud-init sentinel never logged); `trace_test.go`; `check_test.go`. Real + backend: `kvm_integration_test.go` behind `//go:build integration`, driven + by `KVM_TEST_*` variables and `make test-integration-kvm`, described in + [testing.md](testing.md#the-kvm-integration-test-real-hypervisor). +- **Deployment.** The optional `kvm-ssh` Secret mounted at + `/etc/proxy-operator/kvm` (`config/manager/manager.yaml`), + `make kvm-ssh-secret`, a commented entry in both providers configs, + `config/samples/proxy_kvm.yaml`. +- **Known gaps** (see also [kvm-provider.md § Known gaps](kvm-provider.md#known-gaps)). + - No quota or capacity detection. + - `Create` blocks, including the first image download on each host. + - GC blind spots: volumes without a domain are invisible to GC, and so are + VMs on a host removed from the config, which `Delete` treats as done. + - SSH material is cached after the first successful load, so rotating + keys needs a restart. + - Cancelling one operation cuts off the others on that host: a cancelled + `call` closes the shared per-host connection. + - SSH auth failures retry forever: SSH authentication and host-key + failures are Transient, while libvirt auth errors are Permanent. + - `Create` can return `ErrNotFound` when a volume or domain disappears + 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, but the code + tries ARP first; it says `host` and `name` are on every log line, but + the check and list paths lack them. + +## 7. Gaps shared by the existing providers + +- **Adoption is not verified.** None of the three checks `LabelUID` when + `Create` finds an existing instance with the right name. +- **The Create comment does not match kvm.** The interface comment in + `internal/provider/provider.go` says `Create` returns as soon as the + request is submitted; kvm blocks until the domain has started. +- **`Stopped` and `Terminated` are produced inconsistently.** kubernetes + never produces Stopped, kvm never produces Terminated, and the reconciler + treats them the same. +- **Different `ListByTag` partial-failure policies.** gcp leaves failed + parts out, kvm fails the whole list. Both are safe. +- **Stale `mock` comments** in `internal/provider/provider.go`, + `internal/provider/registry/registry.go` and `api/v1alpha1/proxy_types.go` + (and so the generated CRD's `spec.provider` description). +- **GC does not know about installations.** Two clusters sharing one project + or hypervisor delete each other's instances. 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. +- **Tracing.** kvm has no child spans, so a kvm trace shows `provider.create` + and nothing under it; correlation works only through `traceID` on its V(1) + log lines. Only kubernetes is tested against a real collector, and that + test checks only the operation spans; nothing checks that gcp's SDK spans + or the kubernetes API spans nest under `provider.*`. + +## 8. Proposed features + +Listed only. Nothing below is designed or implemented. + +- **Child spans in kvm:** extend the `tracingAPI` decorator in + `kvm/trace.go`, which already wraps every libvirt call, to also start a + span per RPC, and add spans for the SSH dial and the image download. The + seam's methods take no `ctx` today (`hostPool.call` hands `fn` only the + API), so the span context has to be plumbed in. +- **Tempo e2e checks for child spans**, and for gcp and kvm through their + real backend runbooks. +- **A shared conformance suite:** for example `internal/provider/providertest`, + which runs the contract §11 tests against any provider plus a fake + backend, so a new provider gets the contract tests for free. +- **Verify the UID when `Create` adopts an instance.** Return `ErrPermanent` + on a mismatch instead of adopting an instance that belongs to someone else. +- **Report asynchronous create failures:** gcp keeps the operation name, and + `Get` reports the failure instead of NotFound. +- **Capacity or quota signals** for kvm, such as pool free space and host + memory, mapped to `ErrQuotaExceeded`. +- **GC for secondary resources:** mark volumes and similar resources, and + let `ListByTag` or a second listing method cover them. +- **Reload credentials and config without a restart.** +- **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 do not 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. From 86b8d21be397c57b4443b90b6d70e08ecc126c53 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:36:49 +0000 Subject: [PATCH 09/11] Turn docs/providers.md into an index and repoint its inbound links Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 12 +- README.md | 9 +- docs/architecture.md | 2 +- .../2026-09-16-1108-provider-docs-split.md | 21 +- docs/providers.md | 769 +----------------- 5 files changed, 74 insertions(+), 739 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 62b750b..1026f73 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,10 +27,14 @@ entrypoint yet, so there's nothing to run locally. Documented in [docs/architecture.md](docs/architecture.md). It holds the component table, the reconcile flow, and one section per runnable: health engine, discovery and -lease API, orphan GC, metrics, tracing, and provider health checks. The provider -contract (MUST/SHOULD rules, feature matrix, checklist for adding a provider, known -gaps) is in [docs/providers.md](docs/providers.md). Provider-specific detail is in -`docs/kvm-provider.md` and the GCP docs; the test layout is in `docs/testing.md`. +lease API, orphan GC, metrics, tracing, and provider health checks. The generic +provider contract (MUST/SHOULD rules, behaviour profiles, checklist for adding a +provider, known limits) is in [docs/provider-contract.md](docs/provider-contract.md); +a new provider starts from [docs/provider-spec-template.md](docs/provider-spec-template.md); +how the existing providers meet the contract, with their gaps, is in +[docs/providers-existing.md](docs/providers-existing.md); [docs/providers.md](docs/providers.md) +indexes them. Provider-specific detail is in `docs/kvm-provider.md` and the GCP +docs; the test layout is in `docs/testing.md`. ## Conventions diff --git a/README.md b/README.md index 271f9fa..832b2f1 100644 --- a/README.md +++ b/README.md @@ -190,9 +190,12 @@ can be two differently-configured instances of the same `gcp` type (see ### Adding a provider -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 +The generic contract is [docs/provider-contract.md](docs/provider-contract.md), +the questionnaire a new provider's design fills in is +[docs/provider-spec-template.md](docs/provider-spec-template.md), and how the +existing providers meet the contract is +[docs/providers-existing.md](docs/providers-existing.md); +[docs/providers.md](docs/providers.md) indexes them. 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. diff --git a/docs/architecture.md b/docs/architecture.md index 983d69c..f1a9703 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -6,7 +6,7 @@ |---|---|---|---|---| | Proxy + ProviderHealth CRDs | `api/v1alpha1` | types | — | `Proxy` spec/status, CEL validation, defaulting, pure helpers; the status-only, cluster-scoped `ProviderHealth` | | Reconciler | `internal/controller` | controller | yes (with the manager) | the state machine: provision, replace, delete, represent health | -| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator; rules and per-provider matrix in [providers.md](providers.md) | +| Provider contract | `internal/provider` | library | — | `Provider` interface, optional `Checker`, error taxonomy, deterministic naming, config, metrics decorator; rules in [provider-contract.md](provider-contract.md), per-provider profile table in [providers-existing.md](providers-existing.md) | | kubernetes provider | `internal/provider/kubernetes` | library | — | real Squid pods in this cluster (local dev/CI) | | gcp provider | `internal/provider/gcp` | library | — | Compute Engine VMs, four API calls, fire-and-forget ops | | kvm provider | `internal/provider/kvm` | library | — | VMs on libvirt hosts over SSH-forwarded sockets; image cache, overlay disks, NoCloud seeds | diff --git a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md index 3966a62..bbc56ab 100644 --- a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md +++ b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md @@ -6,7 +6,7 @@ Plan: [docs/plans/2026-09-16-1108-provider-docs-split.md](../plans/2026-09-16-11 - [x] Step 1 — Write `docs/provider-contract.md` - [x] Step 2 — Write `docs/provider-spec-template.md` - [x] Step 3 — Write `docs/providers-existing.md` -- [ ] Step 4 — Rewrite `docs/providers.md` as the index; link edits +- [x] Step 4 — Rewrite `docs/providers.md` as the index; link edits - [ ] Step 5 — Verification sweep - [ ] Step 6 — Final summary, CHANGELOG after confirmation @@ -121,3 +121,22 @@ Worth noting: matrix rows that the profile table now carries (Create non-blocking, GC marks, IP source, quota detection, Checker presence, credentials, logging, tracing, tests) were dropped from "Other differences" rather than kept twice. + +## Step 4 — Rewrite `docs/providers.md` as the index; link edits + +2026-09-16 11:40. `docs/providers.md` was rewritten in place (same path, so +the inbound links from `README.md`, `CLAUDE.md`, `docs/architecture.md` and +the historical plan files still resolve, and the pre-split blueprint stays +reachable in the path's git history). The three inbound sentences were +updated to name the new files: + +- `README.md` "### Adding a provider": the first sentence now names the + contract, the template and the appendix, with `docs/providers.md` as the + index; the "Start there" sentence is unchanged. +- `CLAUDE.md` "## Architecture": the providers.md sentence now names all + three files and the index. +- `docs/architecture.md` components row "Provider contract": "rules in + provider-contract.md, per-provider profile table in providers-existing.md". + +The historical files under `docs/plans/` and `docs/plans-executions/` keep +their `providers.md` links on purpose. diff --git a/docs/providers.md b/docs/providers.md index c94edaf..5fbb976 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -1,730 +1,39 @@ -# Providers: the blueprint - -This page is for whoever writes the next provider. It explains the contract a -provider must meet, which caller depends on each rule, how the three existing -providers (kubernetes, gcp, kvm) meet it, and what is still missing. - -Where the rest lives: - -- **Data flow:** the calls each provider makes are in - [architecture.md §5](architecture.md#5-what-provider-calls-do-in-the-outside-world). -- **Health checks:** the monitor that runs them is in - [architecture.md §11](architecture.md#11-provider-health-checks-internalproviderhealth). -- **Per-provider guides:** [kvm-provider.md](kvm-provider.md) and - [gcp-vm-validation.md](gcp-vm-validation.md). -- **Code:** the contract itself is - [`internal/provider/provider.go`](../internal/provider/provider.go), - [`errors.go`](../internal/provider/errors.go) and - [`check.go`](../internal/provider/check.go). - -Rule keywords: **MUST** means breaking the rule breaks a caller. **SHOULD** -means every current provider does it and a new one needs a reason not to. -**MAY** means it's allowed. - -Code references are `path:line` at the time of writing. When a line has moved, -look for the function named next to it. - -## 1. What a provider is - -A provider is a Go type implementing `provider.Provider`: four methods over one -VM-like instance, identified by an opaque `providerID` string. Nothing else in -the operator knows how a backend works: - -- **The reconciler** (`internal/controller`) calls `Create`, `Get` and `Delete` - and branches only on the error class and `Instance.State`. -- **Orphan GC** (`internal/gc`) calls `ListByTag` and `Delete`. -- **The provider health monitor** (`internal/providerhealth`) calls the optional - `Checker`. - -The reconciler never watches backend resources. Everything it knows comes from -polling `Get` (`ProvisioningPoll` and `DeletionPoll` 10s, `RequeueNow` 1s, -`QuotaRetry` 5m, `internal/controller/proxy_controller.go:452-467`). Up to 3 -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. `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 - -### Create(ctx, CreateRequest) (providerID, error) - -`CreateRequest` carries: - -- `Name`: from `NameFromUID`, 22 characters, `proxy-` plus base32. -- `UID`: the owning Proxy's UID. -- `Namespace` and `ProxyName`. -- `Placement`: `Region`, `Zone`, `MachineType`, `Image`. -- `CloudInit`: already resolved from inline text or a Secret. -- `Port`. - -Rules: - -- **MUST be idempotent by `req.Name`.** A repeat call for a name that already - exists returns that instance's providerID with a nil error. It must not return - an error and must not create a duplicate. - *Why:* the reconciler records the ID in status only after `Create` returns - (`proxy_controller.go:155-172`). A crash between the two, or a status write - that is lost, means `Create` runs again for the same Proxy. `replaceInstance` - also depends on this to recover from a crash between its two writes - (`:240-244`). -- **MUST mark the instance as managed when it is created**, with - `LabelManaged=true` and `LabelUID=` or the backend's equivalent. kvm - uses domain `` because libvirt has no labels. - *Why:* orphan GC finds instances only through that mark (`internal/gc/gc.go:105`), - and never deletes one with an empty UID (`gc.go:113-117`). An instance that - exists for a while without the mark leaks if the operator crashes. -- **MUST return a providerID that is self-contained.** `Get` and `Delete` - receive only the ID, and the config or spec may have changed since `Create`, - for example a zone edit during a replacement. So put the location in the ID: - - kubernetes: `/` - - gcp: `zones//instances/` - - kvm: `hosts//domains/` - - Validate any config values that end up in the ID so they can't contain the - separator (`config_kvm.go:108-114`). -- **MUST honour `ctx`.** The reconcile context is cancelled on shutdown and when - leadership is lost. -- **SHOULD return once the request is submitted, not once the instance is - running.** Readiness is discovered by polling `Get`. A blocking `Create` holds - one of the 3 reconcile workers for its whole duration. kvm breaks this rule on - purpose, see §9. -- **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 - and has no other effect. -- **SHOULD deliver `CloudInit` as user-data.** A provider that can't (kubernetes) - documents that it ignores the field. -- **MUST NOT log `CloudInit` contents.** It can hold secrets. Log its byte count - instead. gcp and kvm test this with a sentinel string (`kvm_test.go:975-995`, - `gcp_test.go:330-407`). - -### Get(ctx, providerID) (*Instance, error) - -- **MUST return `ErrNotFound` when, and only when, the instance is gone.** - *Why:* `ErrNotFound` triggers three actions: - - In normal reconciling, the reconciler clears the ID and creates again - (`proxy_controller.go:197-202`). - - During replacement, it is the signal to create the new instance - (`:237-251`). - - During deletion, it releases the finalizer (`:287-291`). - - A false NotFound during an outage creates a duplicate or loses the instance. - "Unreachable", "wrong credentials" and "can't tell" are Transient or - Permanent, never NotFound. -- **MUST keep returning the instance, not NotFound, while a delete is in - progress** and the name is still taken. - *Why:* the replacement instance gets the same name. The reconciler waits for - NotFound before creating it (`:228-233` comment, `:237-251`). A NotFound too - early would make `Create` adopt the dying instance. -- **MUST report `Running` only with a non-empty `IP`.** Running without an - address yet is `Provisioning`. - *Why:* the IP from a `Running` answer goes straight into status and the health - engine (`:212-215`). All three providers do this. -- **MUST report `Stopped` or `Terminated` only when the instance will not - recover by itself.** SHOULD map states it doesn't recognise to `Stopped`. - *Why:* the reconciler deletes and recreates on either state (`:216-226`); the - instances are treated as disposable. The two states cause exactly the same - reaction. -- **MUST fill `UID` and `CreatedAt` by reading them back from the backend**, - never from memory. - *Why:* GC compares `UID` with the live Proxies. It deletes anything whose - `CreatedAt` is older than `MinAge` (10m), and **a zero `CreatedAt` counts as - very old** (`gc.go:120`). A provider that forgets `CreatedAt` makes instances - still being created collectable. An unparsable timestamp gives the zero time - in both gcp and kvm. That is accepted because it never shields an orphan - forever (`gcp.go:253-256`). -- **SHOULD return `ErrPermanent` for an ID it can never resolve**, such as a - malformed ID or a host no longer in the config. - *Why:* the deletion path then tries `Delete` anyway and releases the - finalizer if that works (`:292-302`). If the provider returned Transient, - deletion would poll forever. - -### Delete(ctx, providerID) error - -- **MUST be idempotent.** Deleting something that is already gone returns nil. - *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). - *Why:* GC sees only what `ListByTag` returns, so secondary resources have no - other cleanup path. -- **MAY return before the instance is gone** (gcp, kubernetes), as long as `Get` - keeps showing it until it is (see Get). -- **SHOULD succeed for an ID whose `Get` fails permanently** when there is - nothing left it could reach. Example: kvm on a host that was removed from the - config returns nil (`kvm.go:466-476`). - *Why:* on the deletion path a permanent error isn't latched. It keeps - retrying (`deletionFailure`, `:349-359`), so a Proxy is only released through - the `crawl.example.com/skip-cleanup` annotation. - -### ListByTag(ctx) ([]Instance, error) - -- **MUST return only instances that carry the managed mark**, each with an `ID` - that `Delete` accepts, plus `UID` and `CreatedAt`. - *Why:* GC deletes every listed instance whose UID isn't live and whose age is - past `MinAge` (`gc.go:111-131`). Listing something without the mark is how an - operator deletes a VM it doesn't own. -- **MUST return an error rather than a list it can't vouch for.** An error makes - GC skip that provider for the sweep (`gc.go:105-110`). -- **MAY leave out instances it couldn't read.** Leaving something out is always - safe, because GC never deletes what isn't listed. The current providers make - different choices here: - - gcp silently leaves out unreachable zones (`ReturnPartialSuccess`, - `gcp.go:229`). - - kvm fails the whole list when one host is down but skips a single unreadable - domain (`kvm.go:540-565`, `:580-596`). - - kubernetes makes a single call. -- **SHOULD cover the whole scope the provider can create in**: every namespace, - zone or host. Anything outside that scope is never collected. -- **Scope caveat:** GC protects instances by UID, not by operator install. Two - installs, or two clusters, sharing one GCP project or hypervisor will delete - each other's instances ([gcp-vm-validation.md](gcp-vm-validation.md#gotchas)). - Two provider entries of one install that share a scope are safe, because they - share the live-UID set. - -## 3. Error taxonomy - -Every error a provider method returns MUST be built with -`provider.Wrap(class, op, providerName, id, err)`. `op` is one of `create`, -`get`, `delete` or `list`, and `providerName` is the **configured name** -(`cfg.Name`), not the type. - -`Wrap` keeps both `errors.Is(err, class)` and `errors.As` to the SDK error -working (`errors.go:53-62`). The shared metrics and tracing decorators label -results with the class (`metrics.go:61-74`, `tracing.go:67-80`). - -| Class | Use it for | During normal reconciling | While deleting | -|---|---|---|---| -| `ErrNotFound` | the instance doesn't exist | Get: recreate or advance (§2); other ops: returned as the error, so exponential backoff | Get: release the finalizer | -| `ErrQuotaExceeded` | quota or rate limit | `Provisioned=False` with reason `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` | -| `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same | -| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: `Provisioned=False` with reason `PermanentError`, no more provider calls until `metadata.generation` changes (`:133-139`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) | - -Pitfalls: - -- **Unclassified means Transient.** `provider.Class` returns `ErrTransient` for - any error not built with `Wrap` (`errors.go:71-91`). That is the safe default, - but a permanent failure that was never classified retries forever, and a quota - error backs off far too fast. -- **Permanent is sticky.** Use it only for errors that editing the Proxy spec or - fixing the config can resolve. A 403 that could be a quota problem is the hard - case: gcp checks the error reason (`gcp/errors.go:31-34`), while kubernetes - can't tell quota from RBAC and picks Permanent (`kubernetes.go:201-206`). -- **Don't return NotFound from `Create`.** The reconciler treats it as Transient - there, but it misleads the metrics and anyone reading logs. kvm can currently - do this (§9). -- **Classify in one `classify` function** with a table-driven test. All three - providers do. - -## 4. Identity, names and GC marks - -- **Names:** `provider.NameFromUID(uid)` gives `proxy-` plus 16 lowercase base32 - characters (`name.go:29-33`). It is valid as a GCP name, a DNS label and a - libvirt domain name. A backend with stricter rules SHOULD map it - deterministically rather than invent a new scheme, because idempotency depends - on the name being the same every time. -- **providerID:** opaque to everyone except the provider. Keep it - `/`-shaped, parse it strictly, and return `ErrPermanent` for - a malformed one (§2). -- **Marks:** `LabelManaged = "proxy-operator-managed"` with value `"true"`, and - `LabelUID = "proxy-operator-uid"` (`provider.go:28-32`). Use native labels or - tags where the backend has them. Otherwise store them in metadata that is - written in the same call that creates the instance (kvm: `metadata.go`). -- **`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` - -`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). -`cmd/main.go` type-asserts it on the **unwrapped** provider (`cmd/main.go:216`) -before adding the decorators (`:219-222`). The monitor treats a nil Checker as -unsupported, not as failed (`monitor.go:212`). - -- **MUST never fail as a whole.** Every failure is a `TargetStatus` with - `Ready=false`. -- **MUST return promptly once `ctx` ends.** The monitor gives the whole round - one timeout (`monitor.go:180`). -- **MUST return one entry per target, in a stable order.** Targets are what can - fail independently: the apiserver, a project, each hypervisor. -- **MUST NOT disrupt reconciles.** A check that shares a connection with - provisioning must not tear that connection down when it times out. kvm uses - `hostPool.probe` for this, not `call` (`kvm/check.go:65`, `kvm/conn.go:213-242`). -- **SHOULD use the shared reasons** (`Reachable`, `Timeout`, `Unreachable`, - `Unauthorized`, `Forbidden`, `APIError`). Add provider-specific reasons only - for failures an admin would fix differently: kvm has `SSHHandshakeFailed`, - `PoolNotFound` and others. -- **SHOULD check with the same credentials, verb and scope that `ListByTag` and - `Create` use**, and be cheap: one page, one object. -- **MAY fill `Details`** with facts useful when triaging, such as versions or - capacity. Only kvm does today. - -## 6. Observability: tracing, logs and metrics - -Tracing is OpenTelemetry, set up in `internal/tracing` from the standard -`OTEL_*` environment variables. With no exporter configured, spans are no-ops -and logs are unchanged ([architecture.md §10](architecture.md#10-tracing-internaltracing)). -A provider has three layers to think about; the first comes for free. - -**1. Operation spans and metrics: the decorators, nothing to write.** -`cmd/main.go:219-222` wraps every provider in -`WithTracing(WithMetrics(...))`: - -- `provider.WithTracing` opens one client span per call, `provider.create`, - `provider.get`, `provider.delete` or `provider.list`, with `provider.name`, - `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 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`. - -Rules: - -- **MUST NOT** open its own per-operation span or count its own requests. They - would duplicate the decorators. -- **MUST pass the caller's `ctx`** to every backend call and log through - `logf.FromContext(ctx)` **at call time**. The decorator starts the span with - `tracing.StartSpan`, which puts a logger carrying `traceID` and `spanID` into - `ctx` (`internal/tracing/logger.go:32-44`). A logger cached at construction, - or a backend call made with `context.Background()`, loses the trace. -- **Health checks bypass the decorators** (`cmd/main.go:211-218`). A - `Checker` runs under the monitor's `providerhealth.check` span - (`monitor.go:176`), and the same `ctx` rules apply to it. - -**2. Child spans for backend calls: SHOULD.** -One operation span says that `Create` took 40 seconds. Child spans say which -backend call took them. Every provider SHOULD produce one child span per backend -request, under the operation span: - -- **HTTP backends:** wrap the SDK's transport with `otelhttp`. Gate it on an - existing parent span, as `tracing.RestConfigWrapper` does - (`internal/tracing/transport.go:12-51`). Otherwise background traffic such as - watches, token refreshes or keepalives becomes root spans. Check first whether - the SDK already does this: Google's does. -- **Non-HTTP backends** (RPC over SSH, gRPC without interceptors, CLIs): start - child spans yourself with `tracing.Start(ctx, ".")`, around each - RPC and around long steps such as a dial or an image download. Use a - decorator over the SDK seam, not call-site code. -- **MUST NOT** put secrets or CloudInit contents in span attributes. Put - identifiers, sizes and error codes there. - -**3. Logs.** V(1): one line per operation outcome and per failed backend call. -V(2): curated detail. V(5): raw wire, if the backend has one. Log lines are -matched to traces by the `traceID` key from layer 1, which is how kvm's -per-RPC log lines join a trace today. - -What each provider does now: - -| Layer | kubernetes | gcp | kvm | -|---|---|---|---| -| 1. Operation spans and metrics | yes: decorators | yes: decorators | yes: decorators | -| 2. Child spans per backend call | yes: `tracing.RestConfigWrapper()` on its `rest.Config` (`cmd/main.go:196-200`, `kubernetes.go:58-60`), parent-gated | yes: the Google SDK adds `otelhttp` itself unless telemetry is disabled (`google.golang.org/api/transport/http/dial.go:308-312`); not parent-gated, but every call site has a parent | **no**: `kvm/trace.go` is a log decorator, not spans | -| 3. Logs carry `traceID`/`spanID` | n/a: the package doesn't log | yes, including the V(5) wire log (`gcp/wirelog.go:54-59`) | yes: every `libvirt call` line at V(1) | -| Tested against a real collector | yes: kind e2e checks the `provider.create` and `provider.delete` traces in Tempo (`test/e2e/tracing_test.go:200-238`), operation spans only | no | no | - -## 7. Feature matrix - -Legend: **yes** / **partial** / **no**, with a short note. The evidence is in -the per-provider notes below the table. - -| Feature | kubernetes | gcp | kvm | -|---|---|---|---| -| providerID self-contained | yes: `ns/name` | yes: `zones/z/instances/n`; project from config | yes: `hosts/h/domains/n` | -| Create idempotent | yes: AlreadyExists = success | yes: 409 = success | yes: lookup by name; overlay reused, seed rebuilt | -| Create verifies adopted instance's UID | no | no | no | -| Create non-blocking | yes | yes: operation not awaited | **no**: waits for `DomainCreate`; first use of an image downloads it | -| Placement used | none | Zone, MachineType, Image | Zone (host), MachineType, Image | -| CloudInit delivered | **no**: ignored | yes: `user-data` metadata | yes: NoCloud seed ISO | -| Port used | yes: Squid config and containerPort | no: firewall is manual | no | -| GC marks on create | yes: Pod labels | yes: instance labels | yes: domain ``; volumes unmarked | -| States produced | Provisioning, Running, Terminated | all four | Provisioning, Running, Stopped | -| IP source | PodIP | external NAT IP only | ARP, then guest agent; first IPv4 | -| UID / CreatedAt readback | yes | yes | yes | -| Delete idempotent | yes | yes | yes, at each step | -| Delete waits until gone | no | no | yes | -| Delete cleans secondary resources | n/a | yes: boot disk auto-deleted | yes: overlay and seed; base image kept | -| ListByTag scope | cluster-wide | one project, all zones | configured hosts | -| ListByTag on partial failure | n/a: single call | leaves out failed zones | whole list fails if a host is down; unreadable domain skipped | -| Quota detection | no | yes: 429 and 403 quota reasons | no | -| Error.Provider = configured name | **no**: hardcoded `"kubernetes"` | yes | yes | -| Checker | yes: `apiserver` | yes: the project | yes: each host | -| Checker Details | no | no | yes | -| Credentials | in-cluster config / kubeconfig | ADC | SSH key + known_hosts files, usually a Secret | -| Credential rotation without restart | n/a | via ADC | no: cached after first load | -| Connection handling | client-go | SDK client; SDK retries on Get and List | one pooled SSH+libvirt connection per host, keepalive | -| Logging | none in the package | V(1) ops, V(2) detail, wire log | V(1) ops and RPCs, V(2) detail | -| Tracing: operation spans (§6) | yes | yes | yes | -| 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`) - -- **ID:** `kubernetes.go:152-165`. An empty name part is not rejected. -- **Create:** `kubernetes.go:90-100` and `pod.go:23-50`. Only `Name`, - `Namespace`, `UID` and `Port` are used. Squid config is generated from the - port and passed through `SQUID_CONF` (`pod.go:38-75`). -- **State:** `stateFromPod`, `kubernetes.go:185-199`. Pending, or Running with - no PodIP, is Provisioning. Succeeded, Failed and Unknown are Terminated. A Pod - being deleted still reports its phase. -- **Delete:** `kubernetes.go:118-127`. ListByTag: `kubernetes.go:134-146`. -- **Errors:** `classify`, `kubernetes.go:201-218`. 429 and timeouts are - Transient. Forbidden, Invalid, BadRequest and Unauthorized are Permanent. - Nothing is Quota. -- **Check:** `check.go:21-53`. It lists at most one managed Pod cluster-wide, so - it proves read and list access, not create. -- **Config:** only `kubernetes.image` (`config.go:45-48`, default at - `kubernetes.go:28`). - -### gcp notes (`internal/provider/gcp`) - -- **ID:** `gcp.go:289-299`. -- **Create:** `gcp.go:135-167` and `insert.go:34-74`. `Operation.Wait` is never - called. There is one NIC with an ephemeral external IP, and the boot disk has - `AutoDelete`. -- **State:** `mapState`, `gcp.go:271-287`. RUNNING without a NAT IP is - Provisioning. Unknown statuses are Stopped. -- **Get:** `gcp.go:172-192`. Delete: `gcp.go:196-219`. ListByTag: - `gcp.go:224-244`. -- **Errors:** `errors.go:20-39`. Non-googleapi errors are Transient. 404 is - NotFound. 429 and 403 with reason `quotaExceeded` or `rateLimitExceeded` are - Quota. 400, 401 and other 403s are Permanent. -- **Check:** `check.go:30-78`. One `AggregatedList` page with `maxResults=1`. -- **Config:** `project` is required. `network`, `networkTag` and `diskSizeGb` - have defaults (`config.go:51-61`, `insert.go:12-30`). -- **Logging:** V(1) and V(2) tiers in `gcp.go`. The wire log in `wirelog.go` - runs at V(5), with payloads elided unless `--gcp-wire-log-full-payloads` is - set. - -### kvm notes (`internal/provider/kvm`) - -- **ID:** `id.go:11-21`. Host names are restricted by `config_kvm.go:108-114`. -- **Create:** `kvm.go:154-221`: - - Looks up the domain by name and starts it if needed (`ensureRunning`, - `:300-318`). - - Otherwise: base image (`image.go`), then overlay (reused if it exists, - `:225-245`), then seed (always rebuilt, `:249-275`), then define with - metadata (`:277-298`), then start. -- **Get:** `kvm.go:323-356`. States come from `mapState` (`:442-453`): nostate - is Provisioning, running or blocked without an IP is Provisioning, everything - else is Stopped. -- **Addresses:** `address` (`:397-428`) tries ARP first, then the guest agent. -- **Delete:** `kvm.go:461-538`. Destroy, undefine (with NVRAM, managed save and - snapshot metadata), then remove the overlay and seed by name. A host that is no - longer configured is a success. -- **ListByTag:** `kvm.go:545-596`. -- **Errors:** `errors.go:21-42`. No domain or no volume is NotFound. XML, - unsupported, invalid-argument, auth and access errors, and a missing pool, are - Permanent. Everything else is Transient, including SSH handshake and auth - failures. Nothing is Quota. -- **Check:** `check.go:41-139`. Runs concurrently per host through - `hostPool.probe`. Reasons are tagged by the dial stage. -- **Connections:** `conn.go`. One connection per host with a 15s keepalive. - The pool dials on first use; with provider checks on (the default), the - leader's first health check opens it at startup. On cancel, `call` closes the shared connection; `probe` abandons - the RPC and leaves the connection open. -- **Full guide:** [kvm-provider.md](kvm-provider.md). - -## 8. Adding a provider: checklist - -1. **Package:** create `internal/provider//` with the provider type and a - compile-time interface assertions: `var _ provider.Checker = (*Provider)(nil)` - as in `kubernetes/check.go:15`, and the same for `provider.Provider`, which - the existing providers don't assert yet. - - Put the backend SDK behind a small interface (gcp `instancesAPI`, kvm - `libvirtAPI`) so a fake can stand in for it in tests. - - Build the logger with - `logf.FromContext(ctx).WithName("").WithValues("provider", name)`. -2. **Config:** in `internal/provider/config.go`: - - Add a `Config` block and a `ProviderConfig` field. - - Add the block to `setBlocks`. - - Add the type to the `switch p.Type` allow-list (`config.go:102-108`). - - Add required-field validation, in its own `config_.go` if it is - large. - - Add rows to `config_test.go`. - - The file is parsed with `UnmarshalStrict`, so unknown keys already fail. -3. **Constructor:** `New(ctx, provider.ProviderConfig) (provider.Provider, - error)`, registered in the `registry.Build` map in `cmd/main.go:195-205`. - - A constructor error stops the manager. - - So fail only on config errors, and **don't dial the backend here**: one - unreachable hypervisor or API must not take down the other providers (kvm - logs a warning and connects lazily, `kvm.go:89-101`). -4. **Decorators:** nothing to do. `cmd/main.go:219-222` wraps every provider in - `WithTracing(WithMetrics(...))`. Don't count requests or open per-operation - spans yourself. -5. **Contract:** implement each rule in §2, and test each one against the fake: - - repeat `Create` - - `Get` after `Delete` - - `Delete` twice - - malformed ID - - unknown state mapped to `Stopped` - - `Running` requires an IP - - `UID` and `CreatedAt` read back - - `ListByTag` excludes unmarked instances -6. **Errors:** one `classify` plus a table test covering every class, including - "not an SDK error → Transient". Use `cfg.Name` in `provider.Wrap`. -7. **Checker:** shared reasons first, a table test for the reasons, and a test - that `Check` returns promptly with an expired `ctx`. -8. **Tracing and logging** (§6): - - Child spans for backend calls: an existing SDK instrumentation, a - parent-gated transport wrapper, or a span decorator over the SDK seam. - Add a test with an in-memory span recorder - (`sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(tracetest.NewSpanRecorder()))`, - as in `internal/provider/tracing_test.go`) asserting the spans are - children of the operation span. - - Take the logger from `ctx` on every call so log lines carry `traceID`. - - V(1): one line per operation outcome, plus a `logAPIError`-style line with - the raw backend error. - - V(2): request and response detail. - - Never put CloudInit contents in logs or span attributes; add a sentinel - test for it. -9. **Real-backend test:** behind `//go:build integration`, skipped when its - environment variables are unset. It runs the whole lifecycle: create, create - again, poll until Running, list, delete, delete again, and confirm it is gone - and cleaned up. See `kvm/kvm_integration_test.go` and - [testing.md](testing.md). -10. **RBAC:** if the provider talks to the cluster, add `+kubebuilder:rbac` - markers in their own comment block, not inside a doc comment. The kubernetes - provider's Pod markers live next to the reconciler's - (`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. **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 row in README.md "Implemented providers". -13. **Optional:** a demo script under `docs/demo/`. - -## 9. Gaps and proposed features - -These are listed only. Nothing below is designed or implemented. - -### Current gaps - -**Across providers and the contract** - -- **Adoption isn't verified.** `Create` adopts any existing instance with the - right name without checking its `LabelUID`. None of the three providers check - it. -- **The Create comment doesn't match kvm.** The contract says `Create` returns - as soon as the request is submitted (`provider.go:90-94`). kvm blocks until - the domain has started, and on first use of an image, for the whole download. -- **`Stopped` and `Terminated` are indistinguishable to the reconciler.** - kubernetes never produces Stopped, and kvm never produces Terminated. -- **Different ListByTag partial-failure policies.** gcp leaves failed parts out, - kvm fails the whole list. Both are safe for GC; the contract doesn't say which - to prefer. -- **Config is read once per process.** Adding a provider or host, or rotating - 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** - -- **Wrong provider name in errors.** `Error.Provider` is hardcoded to - `"kubernetes"` (`kubernetes.go:105-216`), so errors and logs show the wrong - name when the configured name differs. -- **Spec fields ignored.** `CloudInit` and all of `Placement` are ignored. -- **No quota class.** A quota 403 is Permanent. -- **Deleting Pods look healthy.** A Pod being deleted still reports its phase, - so it can read as `Running`. -- **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** - -- **kvm has no child spans.** A kvm trace shows `provider.create` and nothing - under it: no libvirt RPCs, no SSH dial, no image download, although that is - where the time goes. Correlation works only through the `traceID` on its V(1) - log lines, which needs debug logging on. -- **Only kubernetes is tested against a real collector**, and that test checks - only the operation spans. Nothing checks that gcp's SDK spans or the - kubernetes API spans actually nest under `provider.*`. - -**gcp** - -- **Asynchronous insert failures are never read.** An insert that the API - accepts but that fails later (quota, bad image) is never seen. The next `Get` - returns NotFound and the reconciler creates it again, so the reason never - reaches status. [gcp-vm-validation.md](gcp-vm-validation.md#gotchas) says - such errors do reach status. -- **External IP only.** A VM without an external NAT IP stays `Provisioning` - forever. -- **No firewall management.** The rule for the proxy port is a manual - prerequisite, and `req.Port` is unused. -- **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)) - -- **No quota or capacity detection.** -- **Create blocks**, including the first image download on each host. -- **GC blind spots.** Volumes without a domain are invisible to GC, and so are - VMs on a host removed from the config. `Delete` treats the latter as done. -- **SSH material is cached after the first successful load**, so rotating keys - needs a restart. -- **Cancelling one operation cuts off the others on that host.** A cancelled - `call` closes the shared per-host connection, aborting other operations in - flight on that host (`conn.go:199-211`). -- **SSH auth failures retry forever.** SSH authentication and host-key failures - are Transient, while libvirt auth errors are Permanent. -- **`Create` can return `ErrNotFound`** when a volume or domain disappears - 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 - lack them. - -### Proposed features (not implemented) - -- **Child spans in kvm:** extend the `tracingAPI` decorator in `kvm/trace.go`, - which already wraps every libvirt call, to also start a span per RPC, and add - spans for the SSH dial and the image download. The seam's methods take no - `ctx` today (`hostPool.call` hands `fn` only the API), so the span context has - to be plumbed in. -- **Tempo e2e checks for child spans**, and for gcp and kvm through their real - backend runbooks. -- **A shared conformance suite:** for example `internal/provider/providertest`, - which runs the §2 rules against any provider plus a fake backend, so a new - provider gets the contract tests for free. -- **Verify the UID when `Create` adopts an instance.** Return `ErrPermanent` on - a mismatch instead of adopting an instance that belongs to someone else. -- **Report asynchronous create failures:** gcp keeps the operation name, and - `Get` reports the failure instead of NotFound. -- **Capacity or quota signals** for kvm, such as pool free space and host - memory, mapped to `ErrQuotaExceeded`. -- **GC for secondary resources:** mark volumes and similar resources, and let - `ListByTag` or a second listing method cover them. -- **Reload credentials and config without a restart.** -- **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. +# Providers + +The provider documentation is split into three files so that a new provider +can be specified and built from the generic rules alone, without reading how +the existing ones happen to work. + +- **[provider-contract.md](provider-contract.md)**: what every provider MUST, + SHOULD and MAY do, which caller depends on each rule, the behaviour profiles + where providers are allowed to differ, the error taxonomy, credential + hygiene, testing obligations, the checklist for adding a provider, and the + contract's known limits. It never names an existing provider. +- **[provider-spec-template.md](provider-spec-template.md)**: the questionnaire + a new provider's design answers, one entry per decision the contract leaves + open. Copy it to `docs/-provider-spec.md`, fill it in, and hand it to + the implementer with the other two files. +- **[providers-existing.md](providers-existing.md)**: how kubernetes, gcp and + kvm meet the contract, a table mapping each of them to every profile, their + per-provider reference notes, shared gaps and proposed features. + +Per-provider user guides: [kvm-provider.md](kvm-provider.md), +[gcp-vm-validation.md](gcp-vm-validation.md) and +[gcp-in-specific-project.md](gcp-in-specific-project.md). + +Related sections of [architecture.md](architecture.md): the components table, +[§5 what provider calls do](architecture.md#5-what-provider-calls-do-in-the-outside-world), +[§10 tracing](architecture.md#10-tracing-internaltracing), +[§11 provider health checks](architecture.md#11-provider-health-checks-internalproviderhealth) +and the "Decisions" list. The test layout is in [testing.md](testing.md). + +## Reading order for a new provider + +1. The contract, end to end. +2. The template, filled in for the new backend; every unanswered entry is a + design decision still to make. +3. The appendix, for a worked example of each profile the spec chose. +4. The user guide of the existing provider closest to the new one. + +The single-file blueprint that preceded this split is in the git history of +this path. From c6dda03e51e48c5e2f15c4a658030434c2318391 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:37:53 +0000 Subject: [PATCH 10/11] Record the verification sweep for the provider docs split Co-Authored-By: Claude Fable 5.1 --- .../2026-09-16-1108-provider-docs-split.md | 90 ++++++++++++++++++- 1 file changed, 88 insertions(+), 2 deletions(-) diff --git a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md index bbc56ab..c7a5f65 100644 --- a/docs/plans-executions/2026-09-16-1108-provider-docs-split.md +++ b/docs/plans-executions/2026-09-16-1108-provider-docs-split.md @@ -7,8 +7,8 @@ Plan: [docs/plans/2026-09-16-1108-provider-docs-split.md](../plans/2026-09-16-11 - [x] Step 2 — Write `docs/provider-spec-template.md` - [x] Step 3 — Write `docs/providers-existing.md` - [x] Step 4 — Rewrite `docs/providers.md` as the index; link edits -- [ ] Step 5 — Verification sweep -- [ ] Step 6 — Final summary, CHANGELOG after confirmation +- [x] Step 5 — Verification sweep +- [x] Step 6 — Final summary, CHANGELOG after confirmation ## Step 0 — Save and commit the plan @@ -140,3 +140,89 @@ updated to name the new files: The historical files under `docs/plans/` and `docs/plans-executions/` keep their `providers.md` links on purpose. + +## Step 5 — Verification sweep + +2026-09-16 11:45. All ten checks from the plan, run against the working tree +after Step 4; nothing needed fixing. + +Links (file targets and anchors) across the four new files plus `README.md`, +`CLAUDE.md` and `docs/architecture.md`, with anchors compared against +slugified headings of the target file: + +```sh +python3 - <<'PY' +import re, pathlib +files = ["docs/provider-contract.md","docs/provider-spec-template.md","docs/providers-existing.md","docs/providers.md","README.md","CLAUDE.md","docs/architecture.md"] +def slug(h): + h = re.sub(r'`','',h).strip().lower(); h = re.sub(r'[^\w\s-]','',h) + return re.sub(r'\s+','-',h).strip('-') +def headings(p): + return {slug(m.group(2)) for m in (re.match(r'^(#{1,6})\s+(.*)$', l) for l in open(p)) if m} +bad = 0 +for f in files: + for m in re.finditer(r'\]\(([^)\s]+)\)', open(f).read()): + t = m.group(1) + if t.startswith(('http://','https://','mailto:')): continue + path, _, anchor = t.partition('#') + target = pathlib.Path(f) if path == '' else pathlib.Path(f).parent/path + if not target.exists(): print(f, "missing", t); bad += 1; continue + if anchor and target.suffix == '.md' and anchor not in headings(target): print(f, "bad anchor", t); bad += 1 +print("link problems:", bad) +PY +``` + +Output: `link problems: 0`. + +Em-dashes, line references and the contract's abstractness: + +```sh +grep -n "—" docs/provider-contract.md docs/provider-spec-template.md docs/providers-existing.md docs/providers.md +git diff 2d959af -- README.md CLAUDE.md docs/architecture.md | grep '^+' | grep "—" +grep -nE '\.go:[0-9]+|:[0-9]+-[0-9]+' docs/provider-contract.md docs/provider-spec-template.md docs/providers-existing.md docs/providers.md +grep -niE "kubernetes|gcp|kvm|libvirt|squid|google|compute engine" docs/provider-contract.md +``` + +The only em-dash hit is the pre-existing "Leader-elected" cell of the +`architecture.md` components row, which shows as an added line because the +row's last cell was edited; no new em-dash was written. The other three +greps are empty. + +Nothing lost from the old blueprint's §9: the bold leads of every gap and +proposal bullet in `git show 0bb699b:docs/providers.md` were looked up in +the new files. The literal-prefix match reported 22 misses, all caused by +rewording ("isn't" to "is not", bold removed), so each topic was re-checked +by a distinctive phrase and all 22 are present in `providers-existing.md` +or contract §13. + +Paths and symbols: every `path/...` token in the four files exists except +the `config/samples/proxy_.yaml` pattern and the +`internal/provider/providertest` proposal; every backticked identifier in +the contract and template resolves in Go, Makefile or YAML sources except +file names, dotted `Type.method` forms, profile names and the `NewWith...` +placeholder. + +Axis alignment: the 19 axis codes from contract §4 each appear once as a +row in the appendix table (six cells per row) and at least once in the +template. + +`go build ./... && go test -short ./...`: all packages `ok`. + +## Step 6 — Final summary + +The split is complete on `feat/provider-blueprint`: five commits after the +plan commit, one per step. The CHANGELOG entry is deliberately not written +yet; per `CLAUDE.md` it is added once the user confirms the documents read +the way they want. Nothing has been pushed. + +Worth noting for whoever picks this up: + +- The contract's §4.20 table maps axis codes to template entry numbers; if + entries are renumbered, that column must follow. +- The `CHANGELOG.md` heading format in `CLAUDE.md` uses an em-dash; the + "no em-dashes" rule applied to the four new documents only. +- The stale `mock` comments in `internal/provider/provider.go`, + `internal/provider/registry/registry.go` and `api/v1alpha1/proxy_types.go` + are still listed as gaps, not fixed here. +- The next planned use of these files is a Proxmox provider: copy the + template to `docs/proxmox-provider-spec.md` and fill it in before any code. From 625cc7cd6455c25d549b9a284002e079e2e46bf7 Mon Sep 17 00:00:00 2001 From: kacerr Date: Wed, 16 Sep 2026 11:47:24 +0000 Subject: [PATCH 11/11] Log the provider docs split in the changelog Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c84bf86..5f1c060 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## 2026-09-16 11:46 UTC — Provider blueprint split into contract, spec template and appendix + +- **`docs/provider-contract.md`** is the generic provider specification: MUST/SHOULD/MAY + rules with the caller behind each one, 19 behaviour-profile axes with stable codes + (where providers may differ, such as blocking vs submit-and-return Create, or a + backend with no provisioning state), error taxonomy, credential hygiene, testing + obligations, the adding-a-provider checklist and the contract's known limits. It + names no existing provider. +- **`docs/provider-spec-template.md`** is the fill-in questionnaire a new provider's + design answers before implementation; entry numbers match the contract's profile + table. +- **`docs/providers-existing.md`** maps kubernetes, gcp and kvm to every profile and + keeps their per-provider reference notes, shared gaps and proposed features. +- **`docs/providers.md`** is now an index; README, CLAUDE.md and architecture.md point + at the new files. This supersedes the single-file blueprint written earlier on the + same branch. +- Two rules the callers enforce that were not written down before: orphan GC sweeps + providers sequentially with no deadline, so a hung `ListByTag` stalls all collection; + and the reconciler deletes an instance a backend reports as Stopped before start. + ## 2026-09-16 08:45 UTC — Provider health checks: ProviderHealth CRD, per-provider checks, gauges - **Each configured provider is now checked on a schedule** and reported in a